patchworktree: update is_bare heuristics

When "git branch -D <name>" is run, Git usually first checks if that branch is currently checked out. But this check is not performed if the Git directory of that repository is not at "<repo>/.git", which is the case if that repository is a submodule that has its Git directory stored as "super/.git/modules/<repo>", for example.

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. This is_bare code was introduced in 92718b7438 ("worktree: add details to the worktree struct", 2015-10-08), following a pre-core.bare heuristic. This patch does 2 things:

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

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 t/t3200-branch.sh | 14 ++++++++++++++
 worktree.c        |  7 +++----
 2 files changed, 17 insertions(+), 4 deletions(-)

Jonathan Tan <jonathantanmy@google.com> writes:

[jc: pinging the current area expert for the multiple worktree setup for sanity checking]

I am not sure if the latter is worth keeping, but the former definitely is a huge improvement, I would imagine. Somebody did "git branch -D <that-branch>" by accident in a submodule checkout, or something?

That is a quite straightforward reproduction recipe.  Very good.

So, we'd drop /.git or /. from the end, without any behaviour change. But we no longer take the fact that it was not the ".git" subdirectory into account, as that is unreliable?

And instead core.bare being '1' is used as the definite sign that we are dealing with a bare repository.

It all makes sense to me.

Thanks for working on it.

Hi,

On Thu, 18 Apr 2019, Junio C Hamano wrote:

I think that both are improvements, with the former definitely being the more impactful one.

/me shudders at the thought of it

The patch makes tons of sense to me.

Thank you, Dscho

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.

Yes, that happened.

Thanks!

On Thu, Apr 18, 2019 at 4:22 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).

We can just call these two calls unconditionally, right? No harm done if we don't strip.

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

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. For the submodule case, you always stand at the submodule's main worktree, so it still works.

I don't think multiple-worktrees-on-submodules will be coming soon, so it's probably ok. But maybe leave a note here.

Thanks - I'll do that in the commit message.

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

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.

That's true.

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.

Yes.

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.

On Thu, Apr 18, 2019 at 11:30:00AM -0700, Jonathan Tan wrote:

I also think your version expresses the intent more clearly. We expect to see one or the other, but not "foo/./.git". And so (just as the code prior to your patch) we would not convert that to "foo".

I am not sure of exactly what the "/." is trying to accomplish, so maybe that double-strip _would_ be desirable, but if so it is definitely worthy of its own commit explaining why that is so.

Interestingly, the case in get_linked_worktree() makes a lot more sense because it has added "." as an absolute path itself, and is just cleaning up the results of its strbuf_add_absolute_path()[1]. Which makes me wonder if the "/." stripping in get_main_worktree() is actually cargo-culted and simply unnecessary.

-Peff

[1] It seems like it would be simpler to just use strbuf_getcwd() for
    this, but maybe I am missing something.

On Fri, Apr 19, 2019 at 1:42 AM Jeff King <peff@peff.net> wrote:

Yeah. It's added the same time get_linked_worktree() adds absolute paths and trims "/." in 5193490442 (worktree: add a function to get worktree details - 2015-10-08). Maybe it's because he wasn't sure if get_git_common_dir() could return ".", which makes it exactly the same as get_linked_worktree(). It's probably very unlikely that git_git_common_dir() could return ".", but I can't be sure either.

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

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

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.

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

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

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

When "git branch -D <name>" is run, Git usually first checks if that branch is currently checked out. But this check is not performed if the Git directory of that repository is not at "<repo>/.git", which is the case if that repository is a submodule that has its Git directory stored as "super/.git/modules/<repo>", for example. This results in the branch being deleted even though it is checked out.

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 does not end in "/.git", and not bare otherwise. This is_bare code was introduced in 92718b7438 ("worktree: add details to the worktree struct", 2015-10-08), following a pre-core.bare heuristic. This patch does 2 things:

 - 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 secondary worktrees, is_bare_repository() returns
   false (which is fine, since there is a worktree available). However,
   treating the main worktree as non-bare when it is bare causes issues:
   for example, failure to delete a branch from a secondary worktree
   that is referred to by a main worktree's HEAD, even if that main
   worktree is bare.

   In order to avoid that, also check core.bare when setting is_bare. If
   core.bare=1, trust it, and otherwise, use is_bare_repository().

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Thanks, everyone, for your reviews.

Changes:
 - s/ends/does not end/ in commit message (thanks, Duy)
 - better rationale for using (is_bare_repository_cfg == 1) in commit
   message, and added test to check that
 - added NEEDSWORK

Range-diff against v1:
1:  d0934b13ae ! 1:  62be513269 worktree: update is_bare heuristics
    @@ -6,12 +6,13 @@
         branch is currently checked out. But this check is not performed if the
         Git directory of that repository is not at "<repo>/.git", which is the
         case if that repository is a submodule that has its Git directory stored
    -    as "super/.git/modules/<repo>", for example.
    +    as "super/.git/modules/<repo>", for example. This results in the branch
    +    being deleted even though it is checked out.
     
         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. This is_bare code was
    -    introduced in 92718b7438 ("worktree: add details to the worktree
    +    path does not end in "/.git", and not bare otherwise. This is_bare code
    +    was introduced in 92718b7438 ("worktree: add details to the worktree
         struct", 2015-10-08), following a pre-core.bare heuristic. This patch
         does 2 things:
     
    @@ -22,12 +23,14 @@
            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
    +       from one of its secondary worktrees, is_bare_repository() returns
    +       false (which is fine, since there is a worktree available). However,
    +       treating the main worktree as non-bare when it is bare causes issues:
    +       for example, failure to delete a branch from a secondary worktree
    +       that is referred to by a main worktree's HEAD, even if that main
    +       worktree is bare.
    +
    +       In order to avoid that, also check core.bare when setting is_bare. If
            core.bare=1, trust it, and otherwise, use is_bare_repository().
     
         Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
    @@ -52,6 +55,16 @@
     +	git -C repo2/sub checkout -b work &&
     +	test_must_fail git -C repo2/sub branch -D work
     +'
    ++
    ++test_expect_success 'bare main worktree has HEAD at branch deleted by secondary worktree' '
    ++	test_when_finished "rm -rf nonbare base secondary" &&
    ++
    ++	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
    ++'
     +
      test_expect_success 'git branch --list -v with --abbrev' '
      	test_when_finished "git branch -D t" &&
    @@ -77,6 +90,13 @@
      	worktree = xcalloc(1, sizeof(*worktree));
      	worktree->path = strbuf_detach(&worktree_path, NULL);
     -	worktree->is_bare = is_bare;
    ++	/*
    ++	 * 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.
    ++	 */
     +	worktree->is_bare = (is_bare_repository_cfg == 1) ||
     +		is_bare_repository();
      	add_head_info(worktree);

 t/t3200-branch.sh | 24 ++++++++++++++++++++++++
 worktree.c        | 14 ++++++++++----
 2 files changed, 34 insertions(+), 4 deletions(-)