Add flag --no-require-git to always respect gitignore files #1216

vegerot · 2022-12-29T01:40:23Z

Add flag --no-require-git to always respect gitignore files

Summary: Currently, --ignore-vcs only serves to unset --no-ignore-vcs.
There is currently no way to tell fd to respect gitignore files when not in a
git repository. This commit adds the flag --no-require-git to make fd always
respect all gitignore files.

This behaves the same as the --no-require-git option in ripgrep

This commit also contains an unrelated wording fix to CONTRIBUTING.md

Test Plan: tests/tests.rs

Background: I am using Sapling
for working with git repositories (including this commit ☺️). Since Sapling
uses .sl instead of .git, tools using the ignore crate (rg and fd) would show gitignored files.
I made a patch (vegerot/ripgrep@ebf17ee)
to ignore to respect gitignores with either .git or .sl. However,
@BurntSushi said he did not want to merge that patch and instead suggested I
use --no-require-git (BurntSushi/ripgrep#2374).
This works fine, but I couldn't use this workaround for my other favorite tool!
That's what this patch is for 😁

(a follow-up patch will add a similar FD_CONFIG_PATH environment variable
like RG_CONFIG_PATH)

Stack created with Sapling. Best reviewed with ReviewStack.

src/config.rs

tmccombs · 2022-12-29T04:21:08Z

I think I'd rather have a separate option for this. In particular, because changing the semantics of --ignore-vcs would mean there is no longer a way to exactly negate --no-ignore-vcs

vegerot · 2022-12-29T06:51:49Z

I think I'd rather have a separate option for this. In particular, because changing the semantics of --ignore-vcs would mean there is no longer a way to exactly negate --no-ignore-vcs

Good point. @tmccombs what do you think about using the same name as ripgrep's option: --{no-,}require-git?

tmccombs · 2022-12-29T07:37:57Z

That seems ok to me

vegerot · 2022-12-29T18:17:28Z

@tmccombs done ✅!

vegerot · 2022-12-30T00:17:58Z

I should add --{no-,}require-git to the test_opposing test
edit: done ✅

src/cli.rs

vegerot · 2023-01-03T18:04:18Z

src/cli.rs

+        long_help = "By default, fd will only respect global gitignore rules, .gitignore rules,\
+                     and local exclude rules if fd detects that you are searching inside a\
+                     git repository. This flag allows you to relax this restriction such that\
+                     fd will respect all git related ignore rules regardless of whether you're\
+                     searching in a git repository or not.\n\n\
+                     This flag behaves the same as --no-require-git flag from ripgrep.\n\n\
+                     This flag can be disabled with --require-git."


FYI this is the same description as --no-require-git for ripgrep https://github.com/BurntSushi/ripgrep/blob/3bb71b0cb8727ac43237af78ba5c707298de0128/crates/core/app.rs#L2216-L2225

In your opinion does this port require a more formal attribution?

fd isn't my project, but I personally wouldn't describe behavior in public docs by referencing other tools like this.

In your opinion does this port require a more formal attribution?

On my end, no. It's probably good sense to mention it in a commit message or perhaps a source code comment so that others looking at it in the future have proper context, but I don't think there's any need for anything more.

Thanks! I did already mention this in a source code comment and the commit message 👍

src/cli.rs

tests/tests.rs

sharkdp · 2023-01-13T14:50:38Z

src/cli.rs

+        long_help = "By default, fd will only respect global gitignore rules, .gitignore rules,\
+                     and local exclude rules if fd detects that you are searching inside a\
+                     git repository. This flag allows you to relax this restriction such that\
+                     fd will respect all git related ignore rules regardless of whether you're\
+                     searching in a git repository or not.\n\n\
+                     This flag can be disabled with --require-git."
+    )]


This contains several missing whitespace errors due to how you placed those backslashes (there should be a space in front). Please check the rendered version using cargo run -- --help or fd --help:

--no-require-git By default, fd will only respect global gitignore rules, .gitignore rules,and local exclude rules if fd detects that you are searching inside agit repository. This flag allows you to relax this restriction such thatfd will respect all git related ignore rules regardless of whether you'researching in a git repository or not. This flag can be disabled with --require-git.

Also, I would prefer if we could put that long help text in the doc comment, like for most other options.

How's this look?

sharkdp

Thank you very much. Added two comments. Also, please read this section: https://github.com/sharkdp/fd/blob/master/CONTRIBUTING.md#add-an-entry-to-the-changelog

@BurntSushi

Summary: Currently, `--ignore-vcs` only serves to unset `--no-ignore-vcs`. There is currently no way to tell fd to respect gitignore files when not in a git repository. This commit adds the flag `--no-require-git` to make fd always respect all gitignore files. This behaves the same as the `--no-require-git` option in [ripgrep](https://github.com/BurntSushi/ripgrep/blob/3bb71b0cb8727ac43237af78ba5c707298de0128/crates/core/app.rs#L2214-L2226) This commit also contains an unrelated wording fix to CONTRIBUTING.md Test Plan: `tests/tests.rs` Background: I am using [Sapling](https://sapling-scm.com/docs/introduction/) for working with git repositories (including this commit ☺️). Since Sapling uses `.sl` instead of `.git`, tools using the `ignore` crate (rg and fd) would show gitignored files. I made a patch (vegerot/ripgrep@ebf17ee) to `ignore` to respect gitignores with _either_ `.git` or `.sl`. However, @BurntSushi said he did not want to merge that patch and instead suggested I use `--no-require-git` (BurntSushi/ripgrep#2374). This works fine, but I couldn't use this workaround for my other favorite tool! That's what this patch is for 😁 (a follow-up patch will add a similar `FD_CONFIG_PATH` environment variable like `RG_CONFIG_PATH`)

vegerot · 2023-01-13T21:55:41Z

Thank you very much. Added two comments. Also, please read this section: https://github.com/sharkdp/fd/blob/master/CONTRIBUTING.md#add-an-entry-to-the-changelog

done ✅! I also noticed in the doc you called it "Unreleased" but in CHANGELOG.md you called it "Upcoming release" so I updated that in CONTRIBUTING

vegerot · 2023-01-16T19:20:15Z

@tmccombs thank you! I don't have permission to merge this PR, so please do it for me (or make me a maintainer c: )

tmccombs · 2023-01-17T06:58:38Z

Merging is blocked for me. I think @sharkdp needs to approve because he requested changes.

sharkdp

Thank you for the updates!

jpcirrus · 2023-02-24T21:01:19Z

This commit neglected to update the man page doc/fd.1.

vegerot · 2023-02-25T05:30:54Z

@jpcirrus thanks for pointing this out! Would you mind contributing a patch?

sharkdp#1216 omitted to add the flag to the man page.

vegerot changed the title ~~placeholder for pull request~~ Make --ignore-vcs _always_ respect gitignore files Dec 29, 2022

vegerot commented Dec 29, 2022

View reviewed changes

src/config.rs Outdated Show resolved Hide resolved

vegerot force-pushed the pr1216 branch 2 times, most recently from 9a59a79 to 303caa1 Compare December 29, 2022 18:14

vegerot changed the title ~~Make --ignore-vcs _always_ respect gitignore files~~ Add flag --no-require-git to always respect gitignore files Dec 29, 2022

vegerot force-pushed the pr1216 branch 2 times, most recently from b0278dd to 960ee2a Compare December 29, 2022 20:21

tmccombs approved these changes Dec 30, 2022

View reviewed changes

src/cli.rs Outdated Show resolved Hide resolved

vegerot force-pushed the pr1216 branch from 960ee2a to a05236a Compare January 3, 2023 17:46

vegerot commented Jan 3, 2023

View reviewed changes

src/cli.rs Outdated Show resolved Hide resolved

vegerot force-pushed the pr1216 branch 2 times, most recently from d10c05d to 8e1c2f9 Compare January 3, 2023 20:48

sharkdp reviewed Jan 13, 2023

View reviewed changes

tests/tests.rs Outdated Show resolved Hide resolved

sharkdp reviewed Jan 13, 2023

View reviewed changes

sharkdp requested changes Jan 13, 2023

View reviewed changes

vegerot force-pushed the pr1216 branch 2 times, most recently from ee4da74 to 29db2ea Compare January 13, 2023 21:50

vegerot force-pushed the pr1216 branch from 29db2ea to 08c0d42 Compare January 13, 2023 21:52

tmccombs approved these changes Jan 16, 2023

View reviewed changes

sharkdp approved these changes Jan 17, 2023

View reviewed changes

sharkdp merged commit af9daff into sharkdp:master Jan 17, 2023

jpcirrus mentioned this pull request Feb 25, 2023

Add flag --no-require-git to man page #1263

Merged

tmccombs pushed a commit to tmccombs/fd that referenced this pull request Mar 20, 2023

Add flag --no-require-git to man page

b799cf1

sharkdp#1216 omitted to add the flag to the man page.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add flag --no-require-git to always respect gitignore files #1216

Add flag --no-require-git to always respect gitignore files #1216

vegerot commented Dec 29, 2022 •

edited

Loading

tmccombs commented Dec 29, 2022

vegerot commented Dec 29, 2022

tmccombs commented Dec 29, 2022

vegerot commented Dec 29, 2022

vegerot commented Dec 30, 2022 •

edited

Loading

vegerot Jan 3, 2023

BurntSushi Jan 3, 2023 •

edited

Loading

vegerot Jan 3, 2023 •

edited

Loading

sharkdp Jan 13, 2023

vegerot Jan 13, 2023

sharkdp left a comment •

edited

Loading

vegerot commented Jan 13, 2023

vegerot commented Jan 16, 2023

tmccombs commented Jan 17, 2023

sharkdp left a comment

jpcirrus commented Feb 24, 2023

vegerot commented Feb 25, 2023

Add flag --no-require-git to always respect gitignore files #1216

Add flag --no-require-git to always respect gitignore files #1216

Conversation

vegerot commented Dec 29, 2022 • edited Loading

tmccombs commented Dec 29, 2022

vegerot commented Dec 29, 2022

tmccombs commented Dec 29, 2022

vegerot commented Dec 29, 2022

vegerot commented Dec 30, 2022 • edited Loading

vegerot Jan 3, 2023

Choose a reason for hiding this comment

BurntSushi Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

vegerot Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

sharkdp Jan 13, 2023

Choose a reason for hiding this comment

vegerot Jan 13, 2023

Choose a reason for hiding this comment

sharkdp left a comment • edited Loading

Choose a reason for hiding this comment

vegerot commented Jan 13, 2023

vegerot commented Jan 16, 2023

tmccombs commented Jan 17, 2023

sharkdp left a comment

Choose a reason for hiding this comment

jpcirrus commented Feb 24, 2023

vegerot commented Feb 25, 2023

vegerot commented Dec 29, 2022 •

edited

Loading

vegerot commented Dec 30, 2022 •

edited

Loading

BurntSushi Jan 3, 2023 •

edited

Loading

vegerot Jan 3, 2023 •

edited

Loading

sharkdp left a comment •

edited

Loading