Hariom verma <hariom18599@xxxxxxxxx> writes: > 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? I'd read Eric's suggestion as "please make sure we have a test to ensure...". If there already are tests that protects the behaviour we care about here, there is no need to duplicate it. Thanks for working on this topic.
