eza: Finding Git directory was broken by adding GIT_DIR support in the general case

A problem introduced by how GIT_DIR support was added: makes Eza find the wrong Git directory in some cases.

For any given path argument given to Eza:

✓ If GIT_DIR is set, and the path is inside that Git repo, great!
❌ If GIT_DIR is set, but the path is outside that Git repo, Eza lies: it prints the git status column but every line shows -- as if there are no changes (the status check is done against the wrong repo; Eza never tried finding the Git repo for that path, since it found the one at GIT_DIR and was satisfied).
1. ❌ If the path is in a different Git repo, the ideal behavior would be to show the correct Git status.
2. ❌ If the path is not in any Git repo, the ideal behaviour would be to not even show the Git column.
✓ If GIT_DIR is set to something that’s not a gitdir, great! (git2::Repository::open_from_env returns a not-found error, so Eza falls through to trying to normal git repository lookup on the path.)
1. ✓ If the path is in a Git repo, the normal lookup finds it and shows the correct Git status.
2. ✓ If the path is not in a Git repo, the Git column is correctly omitted.
❌ If GIT_DIR is not set, git2::Repository::open_from_env will try searching from the current directory (instead of searching from the given path), so:
1. ✓ if eza was executed inside a Git repo, and the path is inside the same repo, great (by coincidence)!
2. ❌ if eza was executed inside a Git repo, and the path is outside that repo, Eza lies (same as case 2).
3. ✓ if eza was executed outside of a Git repo, great!

In particular, note that last ❌ case is a regression: even if a user doesn’t use GIT_DIR, if they just happen to be inside a Git repo when they run Eza, the output will now show wrong status information for paths outside that repo (potentially importantly wrong! eza -l --git ../foo is something a user might run before deciding to rm -r ../foo based on the status infomation!) I’m surprised there wasn’t a test to catch this in the automated tests (or maybe there is, but it’s in the Vagrant tests which no one seems to be able to run)?

About this issue

Original URL
State: open
Created a year ago
Reactions: 2
Comments: 15 (10 by maintainers)

Most upvoted comments

For any given path argument given to Eza:

1. ✓ If `GIT_DIR` is set, and the path is _inside_ that Git repo, great!

2. ❌ If `GIT_DIR` is set, but the path is _outside_ that Git repo, Eza **lies**: it prints the git status column but every line shows `--` as if there are no changes (the status check is done against the wrong repo; Eza never tried finding the Git repo for that path, since it found the one at GIT_DIR and was satisfied).
   
   1. ❌ If the path is in _a different_ Git repo, the ideal behavior would be to show the correct Git status.
   2. ❌ If the path is _not in any_ Git repo, the ideal behaviour would be to not even show the Git column.

I disagree about the ideal behaviour noted here, simply because that is diverging from git upstream: as long as GIT_DIR is set, that will be the current git directory. It overrides all path searches.

For instance, even if the file is in a different repo, if we try to git add somepath, git will respect GIT_DIR, and NOT the closer repo. So then why should eza try to diverge from git behaviour by trying to guess at the “real” git directory of that file? Shouldn’t the output of eza --git match that of git status?

By respecting git behaviour, eza is NOT lying by printing the git columns in case 2. The user has to unset the GIT_DIR once they are done with the repo.

I agree that this is a bit counterintuitive, but trying to diverge from git is just going to cause more confusion.

PS: For case 4, I totally agree that this is a bug introduced by open_from_env. We should fix only that case to use the given path instead of $PWD.

PPS: The full solution

We can treat GIT_DIR as an additional repo, not as a replacement for any of the normal searches.

is also incorrect for the same reason. GIT_DIR being set overrides everything else in git. So eza should strive to do the same.

iamkroot on Aug 29, 2023

will you be submitting a PR for this?

Not anymore. (because I just did 😃 🎉 )

mentalisttraceur on Aug 11, 2023