Re: [PATCH] worktree: update is_bare heuristics

[Duy Nguyen <pclouds@gmail.com>]

On Fri, Apr 19, 2019 at 1:30 AM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> > 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.

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

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