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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 24, 2020, 22:27 UTC
Message-ID: <CAPig+cR9wwcbnuFmVuoDr6OSq29ZCSt6Kr5KtG3HPsDgM5mGSA@mail.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.2002241942120.46@tvgsbejvaqbjf.bet>

On Mon, Feb 24, 2020 at 1:58 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 13 quoted lines

> > On Mon, Feb 24, 2020 at 7:12 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > 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's not at all what the original said, which is reproduced here:

    if (!strbuf_strip_suffix(&worktree_path, "/.git"))
        strbuf_strip_suffix(&worktree_path, "/.");

It says "try stripping '/.git'; if that fails, try stripping '/.'". That is, it recognizes and handles two distinct cases: (1) the path to the .git directory of a non-bare repository, which always ends with "/.git", and (2) the path to a bare git repository, which always ends with "/.". So, the original code wasn't doing any sort of incremental stripping of suffixes; it was just handling two known distinct cases.

Perhaps you missed the '!' in the conditional?

> That reads pretty fine to me. It makes sense.

To me, it doesn't make sense to update the code as done by the patch since that just muddies the issue by making it seem as if get_git_common_dir() is indeterminately tacking on various suffixes rather than giving us deterministic results.

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

It's just three distinct cases my proposed code is handling; there are no twists.

Show 7 quoted lines

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

I don't have strong feelings about how it is tested, but would like to see some sort of test proving that it works as expected following this change.