Fix+improve Git repository lookup (GIT_DIR; GIT_CEILING_DIRECTORIES)
#117
Conversation
|
One day I'd like to follow this up by adding tests for all the cases listed in #115, but I don't have the time or patience for it just yet. Partially because they would have to be in The manual testing I've done so far usually goes like this: mkdir /tmp/test
mkdir /tmp/test/git-1
mkdir /tmp/test/git-2
mkdir /tmp/test/not-git
(cd /tmp/test/git-1 && git init && touch a b && git add a)
(cd /tmp/test/git-2 && git init && touch c d && git add d)
(cd /tmp/test/not-git && touch e f)And then from there I For I think that would basically catch all the correct cases. Since the git stuff is a silently-gracefully-degrates feature, there aren't really any specific failure behaviors to check for. (Although I personally check that |
1. This fixes the case where GIT_DIR is not set, Exa/Eza is executed inside of a repo, but an argument path is outside of that repo. (The current working directory's Git repo was getting checked instead of whatever the Git repo for that path was. Now it will check correctly search for that path's repo again.) 2. Same as 1, but GIT_DIR is set, but a path is is outside of that repo. (GIT_DIR's repo was getting checked instead of whatever the Git repo for that path was. Now it will check GIT_DIR's repo for status first, but if a path is not in that repo, it will check for a repo for that path the normal way.)
Signed-off-by: Christina Sørensen <[email protected]>
I think this was the right approach here. Having the changes atomized is nice, but they're so closely related that keeping them in the same PR makes sense.
Never heard of this env var, seems super useful thou, TIL.
Yea, something like this is my priority right now, although I'm not sure it will be that specific PR. I think I'm gonna set something up after #101 is done, but that's annoyingly being blocked by a issue I'm working on...
I don't think there's anything super rust specific. I did remove the unused var, just to keep it a little tidy. Only other thing in this regard is that I changed your commit summaries to conventional commit. |
|
@sbatial are you available for testing this? (see: #117 (comment)) |
|
Yea. |
Cool. That was how I did it at first, but then I switched it to a variable because Exa's build was causing a "trivial cast" lint warning about it (I just assumed you had the same warning still enabled in Eza).
Oh cool, I'm glad there are well-defined conventions gaining much traction and slick websites documenting them. Thanks for sharing! Also, sorry I didn't notice and match it - I had looked at how Exa was doing it previously and didn't see any obvious convention, forgot to consciously check if y'all were doing it differently in this fork. Might end up adopting it by default in my projects - looking it over I have a mostly positive reaction.
|
|
Oh, I don't have a strong preference about this, but I would've thought both of my commits are "fix" type rather than one the To me a refactor is something that doesn't change the observable behavior (lesser in API impact than a fix)? |
|
I've looked into it and it looks good on first inspection. |
Signed-off-by: Christina Sørensen <[email protected]>
Co-authored-by: Alexander Kozhevnikov <[email protected]> Signed-off-by: Christina Sørensen <[email protected]>
Signed-off-by: Christina Sørensen <[email protected]>
After #119 and #120 we now do! About nits on conventional commits: valid, agree on most, but I don't think straying from it is a good idea because:
The accessibility concern is very valid thou, I guess we could require the
Perhaps this could have been a feat if anything. I don't wanna get too nit picky with this thou, but I'll consider for the next time. |
I also did some testing on this, but tbh I don't have the full mental model of this in my head yet, so I think I'm just gonna say this is reasonably tested and then hope if it breaks users will be quick to open an issue. |
|
All right.... I've gone over all the cases now I think.
Behaviours (while `GIT_DIR` is set if not stated otherwise)Outside a repomainShow git column but only an actual state for the PR 117Show no git column even with Inside the same dir to which
|
I was just about to send in my results so.. 😅 But yes. I came to the same conclusion. |
I think my message initially said I was still interested in how you tested it, but I ctrl-w'ed and lost my message so I rewrote a worse version.
Tests would save us so many head hurts, and they're very much one of my top priorities to get a good solution to. We just need to find a good way to do them, and I'm not sure vagrant is it... |
|
Nice! Thank you both for so thoroughly double-checking my work! I hope it was clear by
that I had already done some manual testing too (if it wasn't, sorry if I caused you to do more testing than you would've). In any case, I really appreciate you testing it too, because I was rather tired by the time I finished with the last version of the change and refactoring. Anyway, glad to see this merged, thanks and best of luck with other Eza work! |
Yeah. I don't have much experience with either Docker or Vagrant (but at least a little with Docker). |
I'm a bit confused about this outcome. If I'm in a directory without a repo, and which is not directly related to In my case, this is the folder structure (this is the usual workflow when using
Feels like any solution will be incomplete unless we also start understanding EDIT: see longer comment in #115 (comment) |
Did you actually check? I can't remember/check right this moment, but that doesn't sound like behavior that I would've implemented. In fact that sounds like behavior I would've considered too obviously wrong to be acceptable , since having this work is exactly the point of |
Yup, in the setup described above. I also rolled back to v0.10.6 and it started working as before. So definitely caused by this change. |
unfortunately, this behaviour is the result of the "ideal" behaviour described for case 2 in the linked issue. See my latest comment there. |
|
Hmm... I guess I didn't test that case properly and I misunderstood/mispredicted how libgit2 would work in that case. Sorry. I'll try to look into it this week. |
This PR fixes the
GIT_DIR-related issues mentioned in #115, and also adds support forGIT_CEILING_DIRECTORIES.(Normally I'd do those two things in separate PRs, but every line that the latter changes is also changed by the former - I put them in separate commits though, so unless you squash when merging, they'll stay as cleanly separated atoms of change in the history.)
GIT_CEILING_DIRECTORIESis actually my main goal here. I depend on Git's ceiling directory feature every day to keep a lot of my(exa|eza) --long --git ...invocations from freezing up (for 2-3 seconds at a time in the worst case). So I had done that change first (a couple days ago, after I realized that my PR to the Exa upstream probably wasn't going to move), then while testing that I noticed theGIT_DIRstuff.These changes are the first Rust code I've ever written, so if there's anything unusually off from how Rust code Is Supposed To Be, please feel free to let me know so I can fix it!