Re: [PATCH v3 1/3] get_main_worktree(): allow it to be called in the Git directory

From: Hariom verma <hariom18599@gmail.com>
Date: Feb 24, 2020, 11:09 UTC
Message-ID: <CA+CkUQ8ZsxesE=d+DQ+67SEHPJXdHjbSKhWeVifPeKBymqy8pw@mail.gmail.com>
In-Reply-To: <CAPig+cTh-uu-obh9aeDOV9ptbVwRmkujgucbu9ei1Qa3qSNG_A@mail.gmail.com>

Hi Eric,

On Mon, Feb 24, 2020 at 7:12 AM Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 19 quoted lines

>
> 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