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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Feb 24, 2020, 18:58 UTC
Message-ID: <nycvar.QRO.7.76.6.2002241942120.46@tvgsbejvaqbjf.bet>
In-Reply-To: <CA+CkUQ8ZsxesE=d+DQ+67SEHPJXdHjbSKhWeVifPeKBymqy8pw@mail.gmail.com>

Hi,

On Mon, 24 Feb 2020, Hariom verma wrote:

Show 20 quoted lines

> On Mon, Feb 24, 2020 at 7:12 AM Eric Sunshine <sunshine@sunshineco.com> 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 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 reads pretty fine to me. It makes sense.

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.

> I'll implement these comments in the next revision for sure.

I'd like to suggest taking a step back and reflecting whether _you_ like the suggested version better. It is just a suggestion, after all, and if it was up to me, I would argue against it.

Show 7 quoted lines

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

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.

Ciao, Dscho