Re: [PATCH] worktree: update is_bare heuristics

From: Jonathan Tan <jonathantanmy@google.com>
Date: Apr 18, 2019, 18:59 UTC
Message-ID: <20190418185922.84159-1-jonathantanmy@google.com>
In-Reply-To: <nycvar.QRO.7.76.6.1904181157010.46@tvgsbejvaqbjf.bet>

Show 20 quoted lines

> > >  - Teach get_main_worktree() to use is_bare_repository() instead,
> > >    introduced in 7d1864ce67 ("Introduce is_bare_repository() and
> > >    core.bare configuration variable", 2007-01-07) and updated in
> > >    e90fdc39b6 ("Clean up work-tree handling", 2007-08-01). This solves
> > >    the "git branch -D <name>" problem described above. However...
> > >
> > >  - If a repository has core.bare=1 but the "git" command is being run
> > >    from one of its added worktrees, is_bare_repository() returns false
> > >    (which is fine, since there is a worktree available). However,
> > >    commands like "git worktree list" currently distinguish between a
> > >    repository that has core.bare=1 but has a worktree available, and a
> > >    repository that is truly non-bare (core.bare=0). To preserve this
> > >    distinction, also check core.bare when setting is_bare. If
> > >    core.bare=1, trust it, and otherwise, use is_bare_repository().
> >
> > I am not sure if the latter is worth keeping, but the former
> > definitely is a huge improvement, I would imagine.
> 
> I think that both are improvements, with the former definitely being the
> more impactful one.

Thanks.

These comments spurred me to look at it a bit closer, and omitting the latter not only results in "git worktree list" changes, but means that the following test now fails:

    test_expect_success 'bare main worktree has HEAD at branch deleted by secondary worktree' '
    	git init nonbare &&
    	test_commit -C nonbare x &&
    	git clone --bare nonbare bare &&
    	git -C bare worktree add --detach ../secondary master &&
    	git -C secondary branch -D master
    '

(At current master, it succeeds.)

In the next reroll, I'll include this test and update the commit message to use this as a rationale instead.

> > Somebody did "git branch -D <that-branch>" by accident in a submodule
> > checkout, or something?
> 
> /me shudders at the thought of it

Yes, that happened.

> The patch makes tons of sense to me.

Thanks!