[PATCH] worktree: update is_bare heuristics

[PATCH] worktree: update is_bare heuristics

[Jonathan Tan]

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

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 478b82cf9b..db5c411e76 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -264,6 +264,20 @@ test_expect_success 'git branch --list -d t should fail' '
 	test_must_fail git rev-parse refs/heads/t
 '
 
+test_expect_success 'deleting checked-out branch from repo that is a submodule' '
+	test_when_finished "rm -rf repo1 repo2" &&
+
+	git init repo1 &&
+	git init repo1/sub &&
+	test_commit -C repo1/sub x &&
+	git -C repo1 submodule add ./sub &&
+	git -C repo1 commit -m "adding sub" &&
+
+	git clone --recurse-submodules repo1 repo2 &&
+	git -C repo2/sub checkout -b work &&
+	test_must_fail git -C repo2/sub branch -D work
+'
+
 test_expect_success 'git branch --list -v with --abbrev' '
 	test_when_finished "git branch -D t" &&
 	git branch t &&
diff --git a/worktree.c b/worktree.c
index b45bfeb9d3..5d52b2ba53 100644
--- a/worktree.c
+++ b/worktree.c
@@ -49,18 +49,17 @@ static struct worktree *get_main_worktree(void)
 	struct worktree *worktree = NULL;
 	struct strbuf path = STRBUF_INIT;
 	struct strbuf worktree_path = STRBUF_INIT;
-	int is_bare = 0;
 
 	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, "/.");
 
 	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) ||
+		is_bare_repository();
 	add_head_info(worktree);
 
 	strbuf_release(&path);
-- 
2.21.0.392.gf8f6787159e-goog

Re: [PATCH] worktree: update is_bare heuristics

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

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

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

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?

> +test_expect_success 'deleting checked-out branch from repo that is a submodule' '
> +	test_when_finished "rm -rf repo1 repo2" &&
> +
> +	git init repo1 &&
> +	git init repo1/sub &&
> +	test_commit -C repo1/sub x &&
> +	git -C repo1 submodule add ./sub &&
> +	git -C repo1 commit -m "adding sub" &&
> +
> +	git clone --recurse-submodules repo1 repo2 &&
> +	git -C repo2/sub checkout -b work &&
> +	test_must_fail git -C repo2/sub branch -D work
> +'

That is a quite straightforward reproduction recipe.  Very good.

> diff --git a/worktree.c b/worktree.c
> index b45bfeb9d3..5d52b2ba53 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -49,18 +49,17 @@ static struct worktree *get_main_worktree(void)
>  	struct worktree *worktree = NULL;
>  	struct strbuf path = STRBUF_INIT;
>  	struct strbuf worktree_path = STRBUF_INIT;
> -	int is_bare = 0;
>  
>  	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, "/.");

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?

>  	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) ||
> +		is_bare_repository();

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.

>  	add_head_info(worktree);
>  
>  	strbuf_release(&path);

Re: [PATCH] worktree: update is_bare heuristics

[Johannes Schindelin]

Hi,

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

> Jonathan Tan <jonathantanmy@google.com> writes:
>
> > 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().
>
> 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.

> Somebody did "git branch -D <that-branch>" by accident in a submodule
> checkout, or something?

/me shudders at the thought of it

The patch makes tons of sense to me.

Thank you,
Dscho

Re: [PATCH] worktree: update is_bare heuristics

[Jonathan Tan]

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

Re: [PATCH] worktree: update is_bare heuristics

[Duy Nguyen]

On Thu, Apr 18, 2019 at 4:22 AM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> 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...

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

>  - 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(-)
>
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 478b82cf9b..db5c411e76 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -264,6 +264,20 @@ test_expect_success 'git branch --list -d t should fail' '
>         test_must_fail git rev-parse refs/heads/t
>  '
>
> +test_expect_success 'deleting checked-out branch from repo that is a submodule' '
> +       test_when_finished "rm -rf repo1 repo2" &&
> +
> +       git init repo1 &&
> +       git init repo1/sub &&
> +       test_commit -C repo1/sub x &&
> +       git -C repo1 submodule add ./sub &&
> +       git -C repo1 commit -m "adding sub" &&
> +
> +       git clone --recurse-submodules repo1 repo2 &&
> +       git -C repo2/sub checkout -b work &&
> +       test_must_fail git -C repo2/sub branch -D work
> +'
> +
>  test_expect_success 'git branch --list -v with --abbrev' '
>         test_when_finished "git branch -D t" &&
>         git branch t &&
> diff --git a/worktree.c b/worktree.c
> index b45bfeb9d3..5d52b2ba53 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -49,18 +49,17 @@ static struct worktree *get_main_worktree(void)
>         struct worktree *worktree = NULL;
>         struct strbuf path = STRBUF_INIT;
>         struct strbuf worktree_path = STRBUF_INIT;
> -       int is_bare = 0;
>
>         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.

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

> +               is_bare_repository();
>         add_head_info(worktree);
>
>         strbuf_release(&path);
> --
> 2.21.0.392.gf8f6787159e-goog
>


-- 
Duy

Re: [PATCH] worktree: update is_bare heuristics

[Jonathan Tan]

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

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

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

Re: [PATCH] worktree: update is_bare heuristics

[Jeff King]

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

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

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.

Re: [PATCH] worktree: update is_bare heuristics

[Duy Nguyen]

On Fri, Apr 19, 2019 at 1:42 AM Jeff King <peff@peff.net> wrote:
>
> On Thu, Apr 18, 2019 at 11:30:00AM -0700, Jonathan Tan wrote:
>
> > > >         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.)
>
> 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.

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

Re: [PATCH] worktree: update is_bare heuristics

[Duy Nguyen]

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

[PATCH v2] worktree: update is_bare heuristics

[Jonathan Tan]

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

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 478b82cf9b..e9ad50b66d 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -264,6 +264,30 @@ test_expect_success 'git branch --list -d t should fail' '
 	test_must_fail git rev-parse refs/heads/t
 '
 
+test_expect_success 'deleting checked-out branch from repo that is a submodule' '
+	test_when_finished "rm -rf repo1 repo2" &&
+
+	git init repo1 &&
+	git init repo1/sub &&
+	test_commit -C repo1/sub x &&
+	git -C repo1 submodule add ./sub &&
+	git -C repo1 commit -m "adding sub" &&
+
+	git clone --recurse-submodules repo1 repo2 &&
+	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" &&
 	git branch t &&
diff --git a/worktree.c b/worktree.c
index b45bfeb9d3..4f66cd9ce1 100644
--- a/worktree.c
+++ b/worktree.c
@@ -49,18 +49,24 @@ static struct worktree *get_main_worktree(void)
 	struct worktree *worktree = NULL;
 	struct strbuf path = STRBUF_INIT;
 	struct strbuf worktree_path = STRBUF_INIT;
-	int is_bare = 0;
 
 	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, "/.");
 
 	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;
+	/*
+	 * 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);
 
 	strbuf_release(&path);
-- 
2.21.0.392.gf8f6787159e-goog