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

Add require-git option #6923

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sscheele
Copy link
Contributor

@sscheele sscheele commented May 1, 2023

Adds a require-git config option to the file picker and uses it to determine whether the ignore::WalkBuilder requires the current directory to be a git directory or not.

Closes #6909

@pascalkuthe
Copy link
Member

The code changes loog good but this needs documentation in book/SRC/configuration.md

@sscheele
Copy link
Contributor Author

sscheele commented May 1, 2023

The code changes loog good but this needs documentation in book/SRC/configuration.md

Done! I broke the convention of the book being the same as the comments in the code, though, because this one has the less intuitive property that it enables a new feature when set to false rather than true.

Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

LGTM

@pascalkuthe pascalkuthe added C-enhancement Category: Improvements E-easy Call for participation: Experience needed to fix: Easy / not much A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels May 1, 2023
@archseer
Copy link
Member

archseer commented May 1, 2023

Does this need to be an option? Why not just turn this on by default?

@sscheele
Copy link
Contributor Author

sscheele commented May 1, 2023

Does this need to be an option? Why not just turn this on by default?

Having an on-by-default behavior that invisibly uses the global gitignore to decide what files to hide from you seems pretty opaque to me. It might lead to confusion.

Maybe ideally there are fully separate settings for global/local? This might be a slightly bigger change, but definitely doable.

@pascalkuthe
Copy link
Member

Does this need to be an option? Why not just turn this on by default?

Having an on-by-default behavior that invisibly uses the global gitignore to decide what files to hide from you seems pretty opaque to me. It might lead to confusion.

Maybe ideally there are fully separate settings for global/local? This might be a slightly bigger change, but definitely doable.

It's not doable with ignore sadly. You would need to reimplement the git detection yourself

@sscheele
Copy link
Contributor Author

sscheele commented May 1, 2023

You would need to reimplement the git detection yourself

Yes, I agree - though if we could then use ignore to actually do the parsing and filesystem walk, I think it would just be a function that steps up the directory tree looking for .git directories. Still, rolling our own git detection does feel like it's a little bit out of scope, and I think the behavior we have (whether or not to force git-like behavior regardless of git status) is pretty straightforward for users.

@Niketin
Copy link

Niketin commented Jun 3, 2023

This pull request resolves the issue #6909 so it should probably be properly linked to it.

@the-mikedavis the-mikedavis requested a review from archseer June 7, 2023 21:59
Copy link
Contributor

@alevinval alevinval left a comment

Choose a reason for hiding this comment

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

Looks ready to ship

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements C-enhancement Category: Improvements E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Utilize .gitignore outside of Git repositories
6 participants