Re: [PATCH] worktree: update is_bare heuristics

From: Duy Nguyen <pclouds@gmail.com>
Date: Apr 19, 2019, 10:50 UTC
Message-ID: <CACsJy8CoaOmJpFygQxrFyJmpzL4wyR0Dbck_TyVOFViRbtqAHQ@mail.gmail.com>
In-Reply-To: <20190418183000.78138-1-jonathantanmy@google.com>

On Fri, Apr 19, 2019 at 1:30 AM Jonathan Tan <jonathantanmy@google.com> wrote:

Show 6 quoted lines

>
> > You actually didn't spell out the problem with "git branch -D", or at
> > least the consequence (i.e. the submodule branch is deleted even if
> > it's checked out).
>
> Thanks - I'll do that in the commit message.

Another minor nit (because I was still puzzled why a submodule was considered bare/not bare)

> This is because get_main_worktree() in worktree.c sets is_bare on a
> worktree only using the heuristic that a repo is bare if the worktree's
> path ends in "/.git", and not bare otherwise.

s/ends/does not end/. Now it makes more sense because the submodule's main worktree is accidentally considered "bare" (i.e. no worktree), its HEAD is ignored.

Show 13 quoted lines

> > >         strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
> > >
> > >         worktree = xcalloc(1, sizeof(*worktree));
> > >         worktree->path = strbuf_detach(&worktree_path, NULL);
> > > -       worktree->is_bare = is_bare;
> > > +       worktree->is_bare = (is_bare_repository_cfg == 1) ||
> >
> > core.bare and core.worktree are special. When you access them standing
> > from the main worktree, you'll see them. But when you stand from a
> > secondary worktree, they are ignored.
>
> Just checking: I think that is_bare_repository_cfg ignores core.bare
> only if the config.worktree file is present?

No, both are ignored independently if you read them from a secondary worktree. Tested too with rev-parse, just to be sure.

> In the t2402 test '"list"
> all worktrees from linked with a bare main', "git worktree list" still
> observes the main worktree as bare.

Yes, because of bug :(

When git_config() is called again by cmd_worktree(), it does not treat core.worktree and core.bare special anymore. So is_bare_repository_cfg is reset from 0 to 1, even though I think its value at that point plays no role anymore. What matters is the value at setup_git_directory().

So yeah your code works in all cases by luck ;)

Fixing that one will not be easy (to avoid traps). I did try to delete is_bare_repository_cfg (on the ground that global vars are hard to manage, as seen here) only to discover that I could not simply delete the core.bare parsing code in git_default_core_config() without changing behaviour in some case [1]. I should probably revive that branch (cleaning up gitdir setup code a bit) and submit.

[1] https://gitlab.com/pclouds/git/commit/2cc46d698ebe7af295e0d91f68103675077df68e#db04685516ffb491eb4239291b892fc0784e1aea

Show 11 quoted lines

> > I don't think multiple-worktrees-on-submodules will be coming soon, so
> > it's probably ok. But maybe leave a note here.
>
> Observing that the problematic case is the 3rd one above, would this
> note work:
>
>   NEEDSWORK: If this function is called from a secondary worktree and
>   config.worktree is present, is_bare_repository_cfg will reflect the
>   contents of config.worktree, not the contents of the main worktree.
>   This means that worktree->is_bare may be set to 0 even if the main
>   worktree is configured to be bare.

Even though your code works perfectly now, I still think it's good to have some comment like this.

-- 
Duy