Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement #378 - add '--ignore' flag and using 'ignore' crate #384

Merged
merged 1 commit into from
Apr 21, 2020
Merged

Implement #378 - add '--ignore' flag and using 'ignore' crate #384

merged 1 commit into from
Apr 21, 2020

Conversation

Celeo
Copy link
Contributor

@Celeo Celeo commented Apr 16, 2020

Implements #378

@Celeo Celeo changed the title Implement #378 Implement #378 - read from .gitignore file to modify search globs Apr 16, 2020
@casey
Copy link
Owner

casey commented Apr 18, 2020

Yo! Thank you for tackling this, and sorry for not responding quicker!

I read a little bit about .gitignore handling, and I think it's somewhat tricky. Or, at least, that's what this comment suggested.

BurntSushi publishes the gitignore related functionality in the ignore crate.

Looking at it, it looks like it implements a lot of the things that I implemented, like hidden file skipping, so I wish I had found it earlier. Fortunately, it's built on top of walkdir, which is the same crate that I use for recursive directory search.

I think the best way to implement this is probably to switch out walkdir for ignore, and set standard_filters to false. This will, I think, be a no-op, i.e. without the standard filters, ignore will have exactly the same behavior as walkdir. Then, .gitignore-related functionality can be implemented in terms of ignore.

What do you think?

Edit: And, indeed, in a later commit, I can rip out a bunch of the functionality I wrote, like symlink following, in favor of the implementation in ignore.

@Celeo
Copy link
Contributor Author

Celeo commented Apr 18, 2020

Okay, sounds good! Leave it to BurntSushi to have already solved a complicated problem, haha.

src/error.rs Outdated Show resolved Hide resolved
@casey
Copy link
Owner

casey commented Apr 19, 2020

I consolidated the automatic changelog and documentation generators into a single binary, so now to update everything you can run cargo run --package gen all or just gen if you have just installed.

Copy link
Owner

@casey casey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start, added some comments!

src/error.rs Outdated Show resolved Hide resolved
src/walker.rs Show resolved Hide resolved
src/walker.rs Outdated Show resolved Hide resolved
src/subcommand/torrent/create.rs Outdated Show resolved Hide resolved
src/subcommand/torrent/create.rs Outdated Show resolved Hide resolved
@Celeo Celeo changed the title Implement #378 - read from .gitignore file to modify search globs Implement #378 - add '--ignore' flag and using 'ignore' crate Apr 19, 2020
@Celeo Celeo marked this pull request as ready for review April 19, 2020 21:36
@casey
Copy link
Owner

casey commented Apr 20, 2020

I opened this issue on ripgrep to ask about replicating hidden-attribute checking on macOS, but no need to hold up this diff.

@casey
Copy link
Owner

casey commented Apr 20, 2020

Just checked everything, and I think this is ready to merge once the tests pass.

@casey
Copy link
Owner

casey commented Apr 20, 2020

Oh, I forgot, we should also add tests that the various files are all respected, i.e. .gitignore, .ignore, and .git/info/exclude. Checking that git config --get core.excludesFile is respected is annoying enough that it doesn't seem worth testing.

@Celeo
Copy link
Contributor Author

Celeo commented Apr 20, 2020

While writing the tests, I discovered that the ignore crate only actually respects a .gitignore file if the directory being walked is a git repo, i.e. has had $ git init ran. While this probably won't be an issue for users, it definitely cost me some time.

Looks like .ignore files are respected regardless of git.

Bah, it's an option in the builder.

@Celeo
Copy link
Contributor Author

Celeo commented Apr 20, 2020

Hm, tests are passing for me.

@casey
Copy link
Owner

casey commented Apr 20, 2020

You can click through the build job and check out the test failure:

https://github.com/casey/intermodal/pull/384/checks?check_run_id=600603319#step:10:344

In the windows and mac branches, a file is given the platform-specific hidden attribute, and then the test checks that the file doesn't appear in the created torrent. For linux, which has no hidden attribute, the file is just removed. To fix the test, I think it can be modified to remove the file on macos as well as linux. (I.e. remove the if else cfg!(target_os = "macos") { .. })

@Celeo Celeo marked this pull request as draft April 20, 2020 04:29
@Celeo
Copy link
Contributor Author

Celeo commented Apr 20, 2020

I think my 2 PRs are having problems with each other for creating the changelog.

@casey
Copy link
Owner

casey commented Apr 21, 2020

I opened a PR on ripgrep BurntSushi/ripgrep#1557 to add a filter_entry method to ignore::WalkBuilder. I also opened #405 so I remember to add filtering by the hidden attribute back in.

I've been trying to get the merge workflow right, but it's currently a little weird, especially with respect to the changelog.

I'm trying to make sure every commit in the repo is signed with my PGP key, so end users can check the git history against a single, known PGP key. Also, I don't really like merge commits, or multiple commits per feature.

So what I've been doing, once a PR is ready to merge, is:

  • Pull the branch locally
  • Squash, rebase, and sign the resulting commit on local master
  • Force push the result to the PR branch
  • Wait for the tests to pass
  • Manually push the local commit to master, which marks the pull request as merged

The reason I don't use the "Squash and merge" button on GitHub is because that creates a squash commit which is signed with a key from GitHub, as opposed to mine.

Since I squash and merge commits, the changelog should have entries only for the main commit. I added code to make the changelog generator skip entries that have fixup! in the commit, so that's one option to make the changelog generator skip entries after the first.

However, this is not ideal, because fixup!, which is added with git commit --fixup expects the part of the message after fixup! to match a previous commit message, which isn't ideal. I'll figure out a way to make it less painful to tell the changelog generator to skip commits in the future.

For now I can just squash, regenerate the changelog, and merge.

Also, I'm not sure if force pushing to PR branches, which is currently part of my merge strategy, is a good idea. I'm not sure if contributors particularly like having their branch on GitHub mangled by my force pushes. Also, it makes the history of the PR harder to track.

I would switch to merging the changes in manually, in a staging branch, or another PR, but then if I merge them, it won't show the original PR as merged :/ Perhaps that's a better option though.

Definitely let me know if you have any ideas!

Add a '--ignore' flag that, when passed, causes `imdl torretn create` to
skip files listed in `.gitignore`, `.ignore`, `.git/info/exclude`, and
`git config --get core.excludesFile`.

Also switches from the `walkdir` crate to the `ignore` crate, which uses
`walkdir` internally, and which handles `.gitignore` and `.ignore`
files.

This changes the behavior of `-include-hidden` on MacOS to no longer
skip entries with the hidden attribute set, due to `ignore` not exposing
`walkdir`'s filter functionality.

A PR[0] is pending to add filtering to `ignore`, so hopefully this
functionality can be re-implemented soon.

[0] BurntSushi/ripgrep#1557

type: added
fixes:
- #378
@casey casey marked this pull request as ready for review April 21, 2020 02:21
@casey casey merged commit 9b72873 into casey:master Apr 21, 2020
@casey
Copy link
Owner

casey commented Apr 21, 2020

I just added an entry to the FAQ describing the full command line for creating a torrent with the contents of a .git repo.

@casey
Copy link
Owner

casey commented Apr 21, 2020

And thanks for tackling this!

@Celeo Celeo deleted the issue_378 branch April 21, 2020 03:19
@Celeo
Copy link
Contributor Author

Celeo commented Apr 21, 2020

Great!

I did see the signed commits requirement; I created and hooked up a key for my commits, and that made the CI job happy. If that's just for your commit, totally fine too!

As far as the PR merging process, hmmm, tricky. I don't mind a commit being force pushed to my fork/branch as the final step of getting it merged - it'd only happen once, and that's after I'm done touching it. You can see that I force-pushed to my branch(es) a lot haha. I've seen people take the commits from the PR, manually put them into the master branch in a manner where the contributor is still attributed as the author, but also so that the commit log stays clean, and then the PR is simply closed. This was on GitLab though; I don't remember if that site makes a differentiation between closed and merged.

Regardless, I'm happy to have contributed, a big thanks for all the help along the way!

@casey
Copy link
Owner

casey commented Apr 21, 2020

I did see the signed commits requirement; I created and hooked up a key for my commits, and that made the CI job happy. If that's just for your commit, totally fine too!

It would be ideal to preserve as many signatures as possible, but as far as I know it isn't possible to attach multiple signatures to a commit, so if I squash, merge, and sign, it nukes other signatures.

As far as the PR merging process, hmmm, tricky. I don't mind a commit being force pushed to my fork/branch as the final step of getting it merged…

In that case I'll keep force pushing and merging until someone gets irritated with me 😅

Regardless, I'm happy to have contributed, a big thanks for all the help along the way!

You bet!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants