Hi Eric, 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'll implement these comments in the next revision for sure. > 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? Thanks, Hariom
