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

From: Eric Sunshine <sunshine@sunshineco.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: Hariom verma <hariom18599@gmail.com>, git <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v3 1/3] get_main_worktree(): allow it to be called in the Git directory
Date: Mon, 24 Feb 2020 17:27:14 -0500	[thread overview]
Message-ID: <CAPig+cR9wwcbnuFmVuoDr6OSq29ZCSt6Kr5KtG3HPsDgM5mGSA@mail.gmail.com> (raw)
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:
> > 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.

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