fix discovery #1620
fix discovery #1620
Conversation
Such repositories claim they are not bare, but also don't have a workdir with a .git directory inside of it (which is always needed unless certain variables are set). Now they are classified as repository that needs more detailed checking later, while indicating that it 'usually' would be having a worktree that is configured via git configuration.
Previously it was possible that a non-bare repository that didn't have worktree directory incorrectly claimed it had one.
It's nice to have a symmetric API and it fits into `gix`.
| @@ -39,6 +39,8 @@ pub mod is_git { | |||
| Metadata { source: std::io::Error, path: PathBuf }, | |||
| #[error("The repository's config file doesn't exist or didn't have a 'bare' configuration or contained core.worktree without value")] | |||
| Inconclusive, | |||
| #[error("Could not obtain current directory when conforming repository path")] | |||
There was a problem hiding this comment.
Is this, and the related conformed_git_dir variable, about something conforming to a standard or expectation? Or are they typos, where "confirming" and confirmed_git_dir should be written instead?
If "conform" is the intended word, then I suggest rephrasing this message for clarity and probably also renaming the conformed_git_dir variable to conforming_git_dir. If "confirm" is intended--i.e. if the intended meaning is that we are verifying that something is a git dir--then I recommend changing this to "confirming" and renaming the variable to confirmed_git_dir.
I'm not sure which meaning is intended--they both seem plausible in context--so I haven't proposed a specific edit at this time. Also, I couldn't propose it in a review comment easily, because at least as applied to the variable name, this is a preexisting ambiguity, and some occurrences of the variable name are in places this PR does not modify. If it is known that one of these two changes is desired, though, then I'd be pleased to open a PR for them after now that this PR is merged.
There was a problem hiding this comment.
Thanks for the hint - the affected variable name is definitely leaking here and I improved the message to be much more specific in this PR.
| /// The caller has to handle this, typically by reading the configuration. | ||
| /// | ||
| /// It could also be a directory which is non-bare by configuration, but is *not* named `.git`. | ||
| /// Unusual, but it's possible that a worktree is configured via `core.worktree`. |
There was a problem hiding this comment.
Is there also the possibility that the worktree path is established by the presence of a GIT_WORK_TREE environment variable? (Or by the presence of a GIT_DIR environment variable together with no GIT_WORK_TREE variable? In this case, I think the worktree is implicitly taken to be the process CWD, at least in Git.)
There was a problem hiding this comment.
In gitoxide, plumbing crates aren't allowed to read environment variables directly, so this is not handled here.
There is a function in gix though that reads these variables and uses them detect repositories from there.
It's most definitely not fully conforming with what Git does.
There was a problem hiding this comment.
Thanks--I've commented #1585 (comment) with a note about including or otherwise revisiting this when I open the other environment-related compatibility issues per the plan in #1585 (comment).
There was a problem hiding this comment.
Maybe as an additional note, despite claiming it's 'most definitely' not conforming, what I tried to say is that I didn't really try to make it conforming in the first place. To me it felt like environment variables are only needed in certain contexts, for instance, when creating a Git hook with gitoxide, and the implementation should rather be so that gitoxide can be used in certain situations (of which I probably only know one), and otherwise never be able to lead to something surprising. This is a general problem with environment variables, their implicitness, and gitoxide already makes them explicit by mapping every one of them to a git configuration variable, which is used in its place.
That's pretty nice in practice as gix config can now list them as well, and it's also clear that they will override what's there as they are (usually) last.
Tasks
try in GitButler
Discovered in fix 5099 gitbutlerapp/gitbutler#5100