Hi, On Mon, 24 Feb 2020, Hariom verma wrote: > On Mon, Feb 24, 2020 at 7:12 AM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > > > > This title is a bit too generic; it fails to explain what this patch > > is really fixing. Perhaps: > > > > get_main_worktree: correctly normalize worktree path when in .git dir > > > > or something. > > > > "Git directory" is imprecise. As a reader, I can't tell if this means > > the main worktree into which the project is checked out or the `.git` > > directory itself. Please write it instead as "`.git` directory". > > [...] > > This change makes the code unnecessarily confusing and effectively > > turns the final line into dead code. I would much rather see the three > > cases spelled out explicitly, perhaps like this: > > > > if (!strbuf_strip_suffix(&worktree_path, "/.git/.") && /* in .git dir */ > > !strbuf_strip_suffix(&worktree_path, "/.git/")) /* in worktree */ > > strbuf_strip_suffix(&worktree_path, "/."); /* in bare repo */ I would be really cautious about that. To me, the originally proposed change says: strip `/.`, if any. Then, strip `/.git`, and if successful, strip another `/.`, if any. That reads pretty fine to me. It makes sense. Above-mentioned proposal, however, puts quite a few twists into my brain, as is a "if neither X nor Y then Z", and I find the code comments outright confusing. > I'll implement these comments in the next revision for sure. I'd like to suggest taking a step back and reflecting whether _you_ like the suggested version better. It is just a suggestion, after all, and if it was up to me, I would argue against it. > > Also, please add a test to ensure that this behavior doesn't regress > > in the future. You can probably test it via the "git worktree list" > > command, so perhaps add the test to t/t2402-worktree-list.sh. > > There already exists tests in "t/t2402-worktree-list.sh" which lists and > verifies all worktrees. Does this make sense to write a new test that > also does kinda the same thing? The scenario in which we found the buggy behavior involved calling `find_shared_symref()`. I imagine that we could use `git branch -D` inside the `.git` directory for the new regression test. But yes, in my testing, `git worktree list` and `git -C .git worktree list` do show a different top-level directory (the latter shows an incorrect one). Such a test case would find a splendid home in t2402. Ciao, Dscho
