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

Support .jj as well as .git #2842

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Support .jj as well as .git #2842

wants to merge 1 commit into from

Conversation

fowles
Copy link

@fowles fowles commented Jun 22, 2024

  • Allow .jj dirs to count as vcs directories.
  • Simplify the control flow around resolving info/exclude

@fowles
Copy link
Author

fowles commented Jun 22, 2024

I totally understand if you don't want to expand things to support every oddball VCS, but so many tools I like (cargo watch, rg, fzf, fd) are all built on this common library that adding a bit of support at the core gets a lot of things to just work.

@fowles
Copy link
Author

fowles commented Jul 2, 2024

any chance I can get this considered?

@BurntSushi
Copy link
Owner

I think my main concerns here are:

  1. I don't even know what .jj is referring to. You didn't give a link or explain anything.
  2. This patch seems to assume that .jj and .git have identical semantics. Do they? There are literally no differences?
  3. This doubles the number of stat calls for every directory ripgrep searches. This is a rather annoying hurdle because I don't want this to be used as a bludgeon to forever fix ourselves to git. But at the same time, there is a cost here that needs to be weighed with the benefit.

@fowles
Copy link
Author

fowles commented Jul 2, 2024

Sorry about the lack of link, that is my bad. https://martinvonz.github.io/jj/latest/ is the best spot to read. It is a version control system like git, so .jj is a directory that contains metadata for it (just like .git).

As an alternate suggestion, I would recommend respecting .gitignore files even when there is no .git directory. Then you can cut the stat calls down from 1 to zero.

@BurntSushi
Copy link
Owner

As an alternate suggestion, I would recommend respecting .gitignore files even when there is no .git directory. Then you can cut the stat calls down from 1 to zero.

That already exists today with --no-require-git. But vcs directory detection is in general required for correctness so that you don't let gitignore files cross repository boundaries.

@fowles
Copy link
Author

fowles commented Jul 2, 2024

The problem that I run into with --no-require-git is that every tool built on the library exposes it differently (and some don't). Is there a way to make that controlled via env variable or something?

@BurntSushi
Copy link
Owner

No. I personally think that would be wildly inappropriate. ignore isn't a "standard," and trying to reach out into the environment in a library to do an end-runaround on the CLI interface just seems really bad to me. Like, there would, by construction, be no coupling between the environment variable and the CLI, which means there would be no way for ignore to know when to respect the environment variable versus some other option (which might override the environment variable). The only way to solve that would be to introduce more API machinery to deal with it. Sounds awful.

I'm not saying No to this PR, but it's going to require that I or someone goes and understands jj to ensure this is implemented correctly. At minimum. And I'll also need to make a judgment of whether the cost is worth it.

- Allow `.jj` dirs to count as vcs directories.
- Simplify the control flow around resolving info/exclude
@fowles
Copy link
Author

fowles commented Jul 3, 2024

I checked with folks on the jj discord to make sure that I was getting all the corners. I have updated the PR with that and also moved things around a tiny bit to minimize the number of stat calls as well as the number of allocations.

I absolutely understand that this is a judgement call on your end about complexity versus general utility. If there is any data I can provide to make that case, I would be happy to try, but I recognize that jj is a niche VCS. (I have hopes it will grow, but realistically I am not sure it will.)

@fowles
Copy link
Author

fowles commented Jul 16, 2024

Did you have a chance to come to a conclusion here?

@fowles
Copy link
Author

fowles commented Sep 4, 2024

Following up, I have verified this PR with jj's owner (and I am also a committer on it). So I think this is ready for review (pending your actual acceptance of the direction).

Thanks!

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

Successfully merging this pull request may close these issues.

2 participants