Re: [PATCH] worktree: update is_bare heuristics

From: Jonathan Tan <jonathantanmy@google.com>
Date: Apr 18, 2019, 18:30 UTC
Message-ID: <20190418183000.78138-1-jonathantanmy@google.com>
In-Reply-To: <CACsJy8BygpTQg5=8+2KkFCpaJBEkKx+ocVZoa0yRBAQvnkXVSw@mail.gmail.com>

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

Show 8 quoted lines

> >         strbuf_add_absolute_path(&worktree_path, get_git_common_dir());
> > -       is_bare = !strbuf_strip_suffix(&worktree_path, "/.git");
> > -       if (is_bare)
> > +       if (!strbuf_strip_suffix(&worktree_path, "/.git"))
> >                 strbuf_strip_suffix(&worktree_path, "/.");
> 
> We can just call these two calls unconditionally, right? No harm done
> if we don't strip.

We can, and no harm done. But this if/then pattern is also repeated in other parts of the file (e.g. get_linked_worktree()) so I'll leave it in for consistency. (Also, for what it's worth, it's slightly faster if only one strip is done.)

Show 10 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? In the t2402 test '"list" all worktrees from linked with a bare main', "git worktree list" still observes the main worktree as bare. But in any case, you are right that core.bare is sometimes ignored.

> It's more obvious with
> core.worktree because if that affects all worktrees, what's the point
> of having multiple worktrees. Git will always go to the place
> core.worktree points out.

That's true.

> So if this function is called from a secondary worktree, I'm not sure
> if it still works as expected because is_bare_repo may be false then.

I think you're right that is_bare_repository() will always return false here. So let's look at the cases where, running from a secondary worktree, we think that the main worktree should be observed as bare:

 - main worktree did not define core.bare
   - I don't know if this is possible (remember that we're running from
     a secondary worktree). But if it is, it seems that
     is_bare_repository_cfg will be -1, and worktree->is_bare will be
     set to 0 regardless of whether or not it is bare.

 - main worktree defines core.bare as 1; no config.worktree
   - is_bare_repository_cfg is 1, so we see the main worktree as bare.
     (This case is tested in t2402 '"list" all worktrees from linked
     with a bare main'.)

 - main worktree defines core.bare as 1, and secondary worktree defines
   core.bare as 0
   - I think that we'll see is_bare_repository_cfg as 0, so we won't see
     the main worktree as bare.

The only potentially problematic case seems to be the 3rd one.

> For the submodule case, you always stand at the submodule's main
> worktree, so it still works.

Yes.

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