| Message ID | 20190417212128.52475-1-jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | worktree: update is_bare heuristics | expand |
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);
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
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 >
> 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.
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.
> > > - 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!
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.
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.
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);
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(-)