{"thread":{"id":"50947","subject":"[PATCH] worktree: update is_bare heuristics","startedAt":"2019-04-17T21:21:35Z","lastAt":"2019-04-19T20:17:31Z","messageCount":10,"participants":["Jonathan Tan","Junio C Hamano","Johannes Schindelin","Duy Nguyen","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"374071","messageId":"20190417212128.52475-1-jonathantanmy@google.com","threadId":"50947","inReplyTo":null,"subject":"[PATCH] worktree: update is_bare heuristics","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-04-17T21:21:28Z","receivedAt":"2019-04-17T21:21:35Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When \"git branch -D \" is run, Git usually first checks if that\nbranch is currently checked out. But this check is not performed if the\nGit directory of that repository is not at \"/.git\", which is the\ncase if that repository is a submodule that has its Git directory stored\nas \"super/.git/modules/\", for example.\n\nThis is because get_main_worktree() in worktree.c sets is_bare on a\nworktree only using the heuristic that a repo is bare if the worktree's\npath ends in \"/.git\", and not bare otherwise. This is_bare code was\nintroduced in 92718b7438 (\"worktree: add details to the worktree\nstruct\", 2015-10-08), following a pre-core.bare heuristic. This patch\ndoes 2 things:\n\n - Teach get_main_worktree() to use is_bare_repository() instead,\n introduced in 7d1864ce67 (\"Introduce is_bare_repository() and\n core.bare configuration variable\", 2007-01-07) and updated in\n e90fdc39b6 (\"Clean up work-tree handling\", 2007-08-01). This solves\n the \"git branch -D \" problem described above. However...\n\n - If a repository has core.bare=1 but the \"git\" command is being run\n from one of its added worktrees, is_bare_repository() returns false\n (which is fine, since there is a worktree available). However,\n commands like \"git worktree list\" currently distinguish between a\n repository that has core.bare=1 but has a worktree available, and a\n repository that is truly non-bare (core.bare=0). To preserve this\n distinction, also check core.bare when setting is_bare. If\n core.bare=1, trust it, and otherwise, use is_bare_repository().\n\nSigned-off-by: Jonathan Tan \n---\n t/t3200-branch.sh | 14 ++++++++++++++\n worktree.c | 7 +++----\n 2 files changed, 17 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 478b82cf9b..db5c411e76 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -264,6 +264,20 @@ test_expect_success 'git branch --list -d t should fail' '\n \ttest_must_fail git rev-parse refs/heads/t\n '\n \n+test_expect_success 'deleting checked-out branch from repo that is a submodule' '\n+\ttest_when_finished \"rm -rf repo1 repo2\" &&\n+\n+\tgit init repo1 &&\n+\tgit init repo1/sub &&\n+\ttest_commit -C repo1/sub x &&\n+\tgit -C repo1 submodule add ./sub &&\n+\tgit -C repo1 commit -m \"adding sub\" &&\n+\n+\tgit clone --recurse-submodules repo1 repo2 &&\n+\tgit -C repo2/sub checkout -b work &&\n+\ttest_must_fail git -C repo2/sub branch -D work\n+'\n+\n test_expect_success 'git branch --list -v with --abbrev' '\n \ttest_when_finished \"git branch -D t\" &&\n \tgit branch t &&\ndiff --git a/worktree.c b/worktree.c\nindex b45bfeb9d3..5d52b2ba53 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -49,18 +49,17 @@ static struct worktree *get_main_worktree(void)\n \tstruct worktree *worktree = NULL;\n \tstruct strbuf path = STRBUF_INIT;\n \tstruct strbuf worktree_path = STRBUF_INIT;\n-\tint is_bare = 0;\n \n \tstrbuf_add_absolute_path(&worktree_path, get_git_common_dir());\n-\tis_bare = !strbuf_strip_suffix(&worktree_path, \"/.git\");\n-\tif (is_bare)\n+\tif (!strbuf_strip_suffix(&worktree_path, \"/.git\"))\n \t\tstrbuf_strip_suffix(&worktree_path, \"/.\");\n \n \tstrbuf_addf(&path, \"%s/HEAD\", get_git_common_dir());\n \n \tworktree = xcalloc(1, sizeof(*worktree));\n \tworktree->path = strbuf_detach(&worktree_path, NULL);\n-\tworktree->is_bare = is_bare;\n+\tworktree->is_bare = (is_bare_repository_cfg == 1) ||\n+\t\tis_bare_repository();\n \tadd_head_info(worktree);\n \n \tstrbuf_release(&path);\n-- \n2.21.0.392.gf8f6787159e-goog\n\n"},{"id":"374085","messageId":"xmqqo954gira.fsf@gitster-ct.c.googlers.com","threadId":"50947","inReplyTo":"20190417212128.52475-1-jonathantanmy@google.com","subject":"Re: [PATCH] worktree: update is_bare heuristics","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-18T02:16:41Z","receivedAt":"2019-04-18T02:16:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n[jc: pinging the current area expert for the multiple worktree setup\nfor sanity checking]\n\n> When \"git branch -D \" is run, Git usually first checks if that\n> branch is currently checked out. But this check is not performed if the\n> Git directory of that repository is not at \"/.git\", which is the\n> case if that repository is a submodule that has its Git directory stored\n> as \"super/.git/modules/\", for example.\n>\n> This is because get_main_worktree() in worktree.c sets is_bare on a\n> worktree only using the heuristic that a repo is bare if the worktree's\n> path ends in \"/.git\", and not bare otherwise. This is_bare code was\n> introduced in 92718b7438 (\"worktree: add details to the worktree\n> struct\", 2015-10-08), following a pre-core.bare heuristic. This patch\n> does 2 things:\n>\n> - Teach get_main_worktree() to use is_bare_repository() instead,\n> introduced in 7d1864ce67 (\"Introduce is_bare_repository() and\n> core.bare configuration variable\", 2007-01-07) and updated in\n> e90fdc39b6 (\"Clean up work-tree handling\", 2007-08-01). This solves\n> the \"git branch -D \" problem described above. However...\n>\n> - If a repository has core.bare=1 but the \"git\" command is being run\n> from one of its added worktrees, is_bare_repository() returns false\n> (which is fine, since there is a worktree available). However,\n> commands like \"git worktree list\" currently distinguish between a\n> repository that has core.bare=1 but has a worktree available, and a\n> repository that is truly non-bare (core.bare=0). To preserve this\n> distinction, also check core.bare when setting is_bare. If\n> core.bare=1, trust it, and otherwise, use is_bare_repository().\n\nI am not sure if the latter is worth keeping, but the former\ndefinitely is a huge improvement, I would imagine. Somebody\ndid \"git branch -D \" by accident in a submodule\ncheckout, or something?\n\n> +test_expect_success 'deleting checked-out branch from repo that is a submodule' '\n> +\ttest_when_finished \"rm -rf repo1 repo2\" &&\n> +\n> +\tgit init repo1 &&\n> +\tgit init repo1/sub &&\n> +\ttest_commit -C repo1/sub x &&\n> +\tgit -C repo1 submodule add ./sub &&\n> +\tgit -C repo1 commit -m \"adding sub\" &&\n> +\n> +\tgit clone --recurse-submodules repo1 repo2 &&\n> +\tgit -C repo2/sub checkout -b work &&\n> +\ttest_must_fail git -C repo2/sub branch -D work\n> +'\n\nThat is a quite straightforward reproduction recipe. Very good.\n\n> diff --git a/worktree.c b/worktree.c\n> index b45bfeb9d3..5d52b2ba53 100644\n> --- a/worktree.c\n> +++ b/worktree.c\n> @@ -49,18 +49,17 @@ static struct worktree *get_main_worktree(void)\n> \tstruct worktree *worktree = NULL;\n> \tstruct strbuf path = STRBUF_INIT;\n> \tstruct strbuf worktree_path = STRBUF_INIT;\n> -\tint is_bare = 0;\n> \n> \tstrbuf_add_absolute_path(&worktree_path, get_git_common_dir());\n> -\tis_bare = !strbuf_strip_suffix(&worktree_path, \"/.git\");\n> -\tif (is_bare)\n> +\tif (!strbuf_strip_suffix(&worktree_path, \"/.git\"))\n> \t\tstrbuf_strip_suffix(&worktree_path, \"/.\");\n\nSo, we'd drop /.git or /. from the end, without any behaviour\nchange. But we no longer take the fact that it was not the \".git\"\nsubdirectory into account, as that is unreliable?\n\n> \tstrbuf_addf(&path, \"%s/HEAD\", get_git_common_dir());\n> \n> \tworktree = xcalloc(1, sizeof(*worktree));\n> \tworktree->path = strbuf_detach(&worktree_path, NULL);\n> -\tworktree->is_bare = is_bare;\n> +\tworktree->is_bare = (is_bare_repository_cfg == 1) ||\n> +\t\tis_bare_repository();\n\nAnd instead core.bare being '1' is used as the definite sign\nthat we are dealing with a bare repository.\n\nIt all makes sense to me.\n\nThanks for working on it.\n\n> \tadd_head_info(worktree);\n> \n> \tstrbuf_release(&path);\n"},{"id":"374098","messageId":"nycvar.QRO.7.76.6.1904181157010.46@tvgsbejvaqbjf.bet","threadId":"50947","inReplyTo":"xmqqo954gira.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] worktree: update is_bare heuristics","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-18T09:59:53Z","receivedAt":"2019-04-18T09:59:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 18 Apr 2019, Junio C Hamano wrote:\n\n> Jonathan Tan writes:\n>\n> > When \"git branch -D \" is run, Git usually first checks if that\n> > branch is currently checked out. But this check is not performed if the\n> > Git directory of that repository is not at \"/.git\", which is the\n> > case if that repository is a submodule that has its Git directory stored\n> > as \"super/.git/modules/\", for example.\n> >\n> > This is because get_main_worktree() in worktree.c sets is_bare on a\n> > worktree only using the heuristic that a repo is bare if the worktree's\n> > path ends in \"/.git\", and not bare otherwise. This is_bare code was\n> > introduced in 92718b7438 (\"worktree: add details to the worktree\n> > struct\", 2015-10-08), following a pre-core.bare heuristic. This patch\n> > does 2 things:\n> >\n> > - Teach get_main_worktree() to use is_bare_repository() instead,\n> > introduced in 7d1864ce67 (\"Introduce is_bare_repository() and\n> > core.bare configuration variable\", 2007-01-07) and updated in\n> > e90fdc39b6 (\"Clean up work-tree handling\", 2007-08-01). This solves\n> > the \"git branch -D \" problem described above. However...\n> >\n> > - If a repository has core.bare=1 but the \"git\" command is being run\n> > from one of its added worktrees, is_bare_repository() returns false\n> > (which is fine, since there is a worktree available). However,\n> > commands like \"git worktree list\" currently distinguish between a\n> > repository that has core.bare=1 but has a worktree available, and a\n> > repository that is truly non-bare (core.bare=0). To preserve this\n> > distinction, also check core.bare when setting is_bare. If\n> > core.bare=1, trust it, and otherwise, use is_bare_repository().\n>\n> I am not sure if the latter is worth keeping, but the former\n> definitely is a huge improvement, I would imagine.\n\nI think that both are improvements, with the former definitely being the\nmore impactful one.\n\n> Somebody did \"git branch -D \" by accident in a submodule\n> checkout, or something?\n\n/me shudders at the thought of it\n\nThe patch makes tons of sense to me.\n\nThank you,\nDscho\n"},{"id":"374100","messageId":"CACsJy8BygpTQg5=8+2KkFCpaJBEkKx+ocVZoa0yRBAQvnkXVSw@mail.gmail.com","threadId":"50947","inReplyTo":"20190417212128.52475-1-jonathantanmy@google.com","subject":"Re: [PATCH] worktree: update is_bare heuristics","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-04-18T10:11:55Z","receivedAt":"2019-04-18T10:12:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Apr 18, 2019 at 4:22 AM Jonathan Tan wrote:\n>\n> When \"git branch -D \" is run, Git usually first checks if that\n> branch is currently checked out. But this check is not performed if the\n> Git directory of that repository is not at \"/.git\", which is the\n> case if that repository is a submodule that has its Git directory stored\n> as \"super/.git/modules/\", for example.\n>\n> This is because get_main_worktree() in worktree.c sets is_bare on a\n> worktree only using the heuristic that a repo is bare if the worktree's\n> path ends in \"/.git\", and not bare otherwise. This is_bare code was\n> introduced in 92718b7438 (\"worktree: add details to the worktree\n> struct\", 2015-10-08), following a pre-core.bare heuristic. This patch\n> does 2 things:\n>\n> - Teach get_main_worktree() to use is_bare_repository() instead,\n> introduced in 7d1864ce67 (\"Introduce is_bare_repository() and\n> core.bare configuration variable\", 2007-01-07) and updated in\n> e90fdc39b6 (\"Clean up work-tree handling\", 2007-08-01). This solves\n> the \"git branch -D \" problem described above. However...\n\nYou actually didn't spell out the problem with \"git branch -D\", or at\nleast the consequence (i.e. the submodule branch is deleted even if\nit's checked out).\n\n> - If a repository has core.bare=1 but the \"git\" command is being run\n> from one of its added worktrees, is_bare_repository() returns false\n> (which is fine, since there is a worktree available). However,\n> commands like \"git worktree list\" currently distinguish between a\n> repository that has core.bare=1 but has a worktree available, and a\n> repository that is truly non-bare (core.bare=0). To preserve this\n> distinction, also check core.bare when setting is_bare. If\n> core.bare=1, trust it, and otherwise, use is_bare_repository().\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> t/t3200-branch.sh | 14 ++++++++++++++\n> worktree.c | 7 +++----\n> 2 files changed, 17 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> index 478b82cf9b..db5c411e76 100755\n> --- a/t/t3200-branch.sh\n> +++ b/t/t3200-branch.sh\n> @@ -264,6 +264,20 @@ test_expect_success 'git branch --list -d t should fail' '\n> test_must_fail git rev-parse refs/heads/t\n> '\n>\n> +test_expect_success 'deleting checked-out branch from repo that is a submodule' '\n> + test_when_finished \"rm -rf repo1 repo2\" &&\n> +\n> + git init repo1 &&\n> + git init repo1/sub &&\n> + test_commit -C repo1/sub x &&\n> + git -C repo1 submodule add ./sub &&\n> + git -C repo1 commit -m \"adding sub\" &&\n> +\n> + git clone --recurse-submodules repo1 repo2 &&\n> + git -C repo2/sub checkout -b work &&\n> + test_must_fail git -C repo2/sub branch -D work\n> +'\n> +\n> test_expect_success 'git branch --list -v with --abbrev' '\n> test_when_finished \"git branch -D t\" &&\n> git branch t &&\n> diff --git a/worktree.c b/worktree.c\n> index b45bfeb9d3..5d52b2ba53 100644\n> --- a/worktree.c\n> +++ b/worktree.c\n> @@ -49,18 +49,17 @@ static struct worktree *get_main_worktree(void)\n> struct worktree *worktree = NULL;\n> struct strbuf path = STRBUF_INIT;\n> struct strbuf worktree_path = STRBUF_INIT;\n> - int is_bare = 0;\n>\n> strbuf_add_absolute_path(&worktree_path, get_git_common_dir());\n> - is_bare = !strbuf_strip_suffix(&worktree_path, \"/.git\");\n> - if (is_bare)\n> + if (!strbuf_strip_suffix(&worktree_path, \"/.git\"))\n> strbuf_strip_suffix(&worktree_path, \"/.\");\n\nWe can just call these two calls unconditionally, right? No harm done\nif we don't strip.\n\n>\n> strbuf_addf(&path, \"%s/HEAD\", get_git_common_dir());\n>\n> worktree = xcalloc(1, sizeof(*worktree));\n> worktree->path = strbuf_detach(&worktree_path, NULL);\n> - worktree->is_bare = is_bare;\n> + worktree->is_bare = (is_bare_repository_cfg == 1) ||\n\ncore.bare and core.worktree are special. When you access them standing\nfrom the main worktree, you'll see them. But when you stand from a\nsecondary worktree, they are ignored. It's more obvious with\ncore.worktree because if that affects all worktrees, what's the point\nof having multiple worktrees. Git will always go to the place\ncore.worktree points out.\n\nSo if this function is called from a secondary worktree, I'm not sure\nif it still works as expected because is_bare_repo may be false then.\nFor the submodule case, you always stand at the submodule's main\nworktree, so it still works.\n\nI don't think multiple-worktrees-on-submodules will be coming soon, so\nit's probably ok. But maybe leave a note here.\n\n> + is_bare_repository();\n> add_head_info(worktree);\n>\n> strbuf_release(&path);\n> --\n> 2.21.0.392.gf8f6787159e-goog\n>\n\n\n-- \nDuy\n"},{"id":"374125","messageId":"20190418183000.78138-1-jonathantanmy@google.com","threadId":"50947","inReplyTo":"CACsJy8BygpTQg5=8+2KkFCpaJBEkKx+ocVZoa0yRBAQvnkXVSw@mail.gmail.com","subject":"Re: [PATCH] worktree: update is_bare heuristics","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-04-18T18:30:00Z","receivedAt":"2019-04-18T18:30:07Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> You actually didn't spell out the problem with \"git branch -D\", or at\n> least the consequence (i.e. the submodule branch is deleted even if\n> it's checked out).\n\nThanks - I'll do that in the commit message.\n\n> > strbuf_add_absolute_path(&worktree_path, get_git_common_dir());\n> > - is_bare = !strbuf_strip_suffix(&worktree_path, \"/.git\");\n> > - if (is_bare)\n> > + if (!strbuf_strip_suffix(&worktree_path, \"/.git\"))\n> > strbuf_strip_suffix(&worktree_path, \"/.\");\n> \n> We can just call these two calls unconditionally, right? No harm done\n> if we don't strip.\n\nWe can, and no harm done. But this if/then pattern is also repeated in\nother parts of the file (e.g. get_linked_worktree()) so I'll leave it in\nfor consistency. (Also, for what it's worth, it's slightly faster if\nonly one strip is done.)\n\n> > strbuf_addf(&path, \"%s/HEAD\", get_git_common_dir());\n> >\n> > worktree = xcalloc(1, sizeof(*worktree));\n> > worktree->path = strbuf_detach(&worktree_path, NULL);\n> > - worktree->is_bare = is_bare;\n> > + worktree->is_bare = (is_bare_repository_cfg == 1) ||\n> \n> core.bare and core.worktree are special. When you access them standing\n> from the main worktree, you'll see them. But when you stand from a\n> secondary worktree, they are ignored.\n\nJust checking: I think that is_bare_repository_cfg ignores core.bare\nonly if the config.worktree file is present? In the t2402 test '\"list\"\nall worktrees from linked with a bare main', \"git worktree list\" still\nobserves the main worktree as bare. But in any case, you are right that\ncore.bare is sometimes ignored.\n\n> It's more obvious with\n> core.worktree because if that affects all worktrees, what's the point\n> of having multiple worktrees. Git will always go to the place\n> core.worktree points out.\n\nThat's true.\n\n> So if this function is called from a secondary worktree, I'm not sure\n> if it still works as expected because is_bare_repo may be false then.\n\nI think you're right that is_bare_repository() will always return false\nhere. So let's look at the cases where, running from a secondary\nworktree, we think that the main worktree should be observed as bare:\n\n - main worktree did not define core.bare\n - I don't know if this is possible (remember that we're running from\n a secondary worktree). But if it is, it seems that\n is_bare_repository_cfg will be -1, and worktree->is_bare will be\n set to 0 regardless of whether or not it is bare.\n\n - main worktree defines core.bare as 1; no config.worktree\n - is_bare_repository_cfg is 1, so we see the main worktree as bare.\n (This case is tested in t2402 '\"list\" all worktrees from linked\n with a bare main'.)\n\n - main worktree defines core.bare as 1, and secondary worktree defines\n core.bare as 0\n - I think that we'll see is_bare_repository_cfg as 0, so we won't see\n the main worktree as bare.\n\nThe only potentially problematic case seems to be the 3rd one.\n\n> For the submodule case, you always stand at the submodule's main\n> worktree, so it still works.\n\nYes.\n\n> I don't think multiple-worktrees-on-submodules will be coming soon, so\n> it's probably ok. But maybe leave a note here.\n\nObserving that the problematic case is the 3rd one above, would this\nnote work:\n\n NEEDSWORK: If this function is called from a secondary worktree and\n config.worktree is present, is_bare_repository_cfg will reflect the\n contents of config.worktree, not the contents of the main worktree.\n This means that worktree->is_bare may be set to 0 even if the main\n worktree is configured to be bare.\n"},{"id":"374128","messageId":"20190418184205.GA12260@sigill.intra.peff.net","threadId":"50947","inReplyTo":"20190418183000.78138-1-jonathantanmy@google.com","subject":"Re: [PATCH] worktree: update is_bare heuristics","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-18T18:42:05Z","receivedAt":"2019-04-18T18:42:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 18, 2019 at 11:30:00AM -0700, Jonathan Tan wrote:\n\n> > > strbuf_add_absolute_path(&worktree_path, get_git_common_dir());\n> > > - is_bare = !strbuf_strip_suffix(&worktree_path, \"/.git\");\n> > > - if (is_bare)\n> > > + if (!strbuf_strip_suffix(&worktree_path, \"/.git\"))\n> > > strbuf_strip_suffix(&worktree_path, \"/.\");\n> > \n> > We can just call these two calls unconditionally, right? No harm done\n> > if we don't strip.\n> \n> We can, and no harm done. But this if/then pattern is also repeated in\n> other parts of the file (e.g. get_linked_worktree()) so I'll leave it in\n> for consistency. (Also, for what it's worth, it's slightly faster if\n> only one strip is done.)\n\nI also think your version expresses the intent more clearly. We expect\nto see one or the other, but not \"foo/./.git\". And so (just as the code\nprior to your patch) we would not convert that to \"foo\".\n\nI am not sure of exactly what the \"/.\" is trying to accomplish, so maybe\nthat double-strip _would_ be desirable, but if so it is definitely\nworthy of its own commit explaining why that is so.\n\nInterestingly, the case in get_linked_worktree() makes a lot more sense\nbecause it has added \".\" as an absolute path itself, and is just\ncleaning up the results of its strbuf_add_absolute_path()[1]. Which\nmakes me wonder if the \"/.\" stripping in get_main_worktree() is actually\ncargo-culted and simply unnecessary.\n\n-Peff\n\n[1] It seems like it would be simpler to just use strbuf_getcwd() for\n this, but maybe I am missing something.\n"},{"id":"374129","messageId":"20190418185922.84159-1-jonathantanmy@google.com","threadId":"50947","inReplyTo":"nycvar.QRO.7.76.6.1904181157010.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] worktree: update is_bare heuristics","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-04-18T18:59:22Z","receivedAt":"2019-04-18T18:59:30Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > > - Teach get_main_worktree() to use is_bare_repository() instead,\n> > > introduced in 7d1864ce67 (\"Introduce is_bare_repository() and\n> > > core.bare configuration variable\", 2007-01-07) and updated in\n> > > e90fdc39b6 (\"Clean up work-tree handling\", 2007-08-01). This solves\n> > > the \"git branch -D \" problem described above. However...\n> > >\n> > > - If a repository has core.bare=1 but the \"git\" command is being run\n> > > from one of its added worktrees, is_bare_repository() returns false\n> > > (which is fine, since there is a worktree available). However,\n> > > commands like \"git worktree list\" currently distinguish between a\n> > > repository that has core.bare=1 but has a worktree available, and a\n> > > repository that is truly non-bare (core.bare=0). To preserve this\n> > > distinction, also check core.bare when setting is_bare. If\n> > > core.bare=1, trust it, and otherwise, use is_bare_repository().\n> >\n> > I am not sure if the latter is worth keeping, but the former\n> > definitely is a huge improvement, I would imagine.\n> \n> I think that both are improvements, with the former definitely being the\n> more impactful one.\n\nThanks.\n\nThese comments spurred me to look at it a bit closer, and omitting the\nlatter not only results in \"git worktree list\" changes, but means that\nthe following test now fails:\n\n test_expect_success 'bare main worktree has HEAD at branch deleted by secondary worktree' '\n \tgit init nonbare &&\n \ttest_commit -C nonbare x &&\n \tgit clone --bare nonbare bare &&\n \tgit -C bare worktree add --detach ../secondary master &&\n \tgit -C secondary branch -D master\n '\n\n(At current master, it succeeds.)\n\nIn the next reroll, I'll include this test and update the commit message\nto use this as a rationale instead.\n\n> > Somebody did \"git branch -D \" by accident in a submodule\n> > checkout, or something?\n> \n> /me shudders at the thought of it\n\nYes, that happened.\n\n> The patch makes tons of sense to me.\n\nThanks!\n"},{"id":"374140","messageId":"CACsJy8C9TDK8OT89FxVAobMtdHMNMih+hevnCetMpo2M9bSsDw@mail.gmail.com","threadId":"50947","inReplyTo":"20190418184205.GA12260@sigill.intra.peff.net","subject":"Re: [PATCH] worktree: update is_bare heuristics","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-04-19T01:33:09Z","receivedAt":"2019-04-19T01:33:39Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Apr 19, 2019 at 1:42 AM Jeff King wrote:\n>\n> On Thu, Apr 18, 2019 at 11:30:00AM -0700, Jonathan Tan wrote:\n>\n> > > > strbuf_add_absolute_path(&worktree_path, get_git_common_dir());\n> > > > - is_bare = !strbuf_strip_suffix(&worktree_path, \"/.git\");\n> > > > - if (is_bare)\n> > > > + if (!strbuf_strip_suffix(&worktree_path, \"/.git\"))\n> > > > strbuf_strip_suffix(&worktree_path, \"/.\");\n> > >\n> > > We can just call these two calls unconditionally, right? No harm done\n> > > if we don't strip.\n> >\n> > We can, and no harm done. But this if/then pattern is also repeated in\n> > other parts of the file (e.g. get_linked_worktree()) so I'll leave it in\n> > for consistency. (Also, for what it's worth, it's slightly faster if\n> > only one strip is done.)\n>\n> I also think your version expresses the intent more clearly. We expect\n> to see one or the other, but not \"foo/./.git\". And so (just as the code\n> prior to your patch) we would not convert that to \"foo\".\n>\n> I am not sure of exactly what the \"/.\" is trying to accomplish, so maybe\n> that double-strip _would_ be desirable, but if so it is definitely\n> worthy of its own commit explaining why that is so.\n>\n> Interestingly, the case in get_linked_worktree() makes a lot more sense\n> because it has added \".\" as an absolute path itself, and is just\n> cleaning up the results of its strbuf_add_absolute_path()[1]. Which\n> makes me wonder if the \"/.\" stripping in get_main_worktree() is actually\n> cargo-culted and simply unnecessary.\n\nYeah. It's added the same time get_linked_worktree() adds absolute\npaths and trims \"/.\" in 5193490442 (worktree: add a function to get\nworktree details - 2015-10-08). Maybe it's because he wasn't sure if\nget_git_common_dir() could return \".\", which makes it exactly the same\nas get_linked_worktree(). It's probably very unlikely that\ngit_git_common_dir() could return \".\", but I can't be sure either.\n-- \nDuy\n"},{"id":"374142","messageId":"20190419172128.130170-1-jonathantanmy@google.com","threadId":"50947","inReplyTo":"20190417212128.52475-1-jonathantanmy@google.com","subject":"[PATCH v2] worktree: update is_bare heuristics","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-04-19T17:21:28Z","receivedAt":"2019-04-19T18:19:06Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When \"git branch -D \" is run, Git usually first checks if that\nbranch is currently checked out. But this check is not performed if the\nGit directory of that repository is not at \"/.git\", which is the\ncase if that repository is a submodule that has its Git directory stored\nas \"super/.git/modules/\", for example. This results in the branch\nbeing deleted even though it is checked out.\n\nThis is because get_main_worktree() in worktree.c sets is_bare on a\nworktree only using the heuristic that a repo is bare if the worktree's\npath does not end in \"/.git\", and not bare otherwise. This is_bare code\nwas introduced in 92718b7438 (\"worktree: add details to the worktree\nstruct\", 2015-10-08), following a pre-core.bare heuristic. This patch\ndoes 2 things:\n\n - Teach get_main_worktree() to use is_bare_repository() instead,\n introduced in 7d1864ce67 (\"Introduce is_bare_repository() and\n core.bare configuration variable\", 2007-01-07) and updated in\n e90fdc39b6 (\"Clean up work-tree handling\", 2007-08-01). This solves\n the \"git branch -D \" problem described above. However...\n\n - If a repository has core.bare=1 but the \"git\" command is being run\n from one of its secondary worktrees, is_bare_repository() returns\n false (which is fine, since there is a worktree available). However,\n treating the main worktree as non-bare when it is bare causes issues:\n for example, failure to delete a branch from a secondary worktree\n that is referred to by a main worktree's HEAD, even if that main\n worktree is bare.\n\n In order to avoid that, also check core.bare when setting is_bare. If\n core.bare=1, trust it, and otherwise, use is_bare_repository().\n\nSigned-off-by: Jonathan Tan \n---\nThanks, everyone, for your reviews.\n\nChanges:\n - s/ends/does not end/ in commit message (thanks, Duy)\n - better rationale for using (is_bare_repository_cfg == 1) in commit\n message, and added test to check that\n - added NEEDSWORK\n\nRange-diff against v1:\n1: d0934b13ae ! 1: 62be513269 worktree: update is_bare heuristics\n @@ -6,12 +6,13 @@\n branch is currently checked out. But this check is not performed if the\n Git directory of that repository is not at \"/.git\", which is the\n case if that repository is a submodule that has its Git directory stored\n - as \"super/.git/modules/\", for example.\n + as \"super/.git/modules/\", for example. This results in the branch\n + being deleted even though it is checked out.\n \n This is because get_main_worktree() in worktree.c sets is_bare on a\n worktree only using the heuristic that a repo is bare if the worktree's\n - path ends in \"/.git\", and not bare otherwise. This is_bare code was\n - introduced in 92718b7438 (\"worktree: add details to the worktree\n + path does not end in \"/.git\", and not bare otherwise. This is_bare code\n + was introduced in 92718b7438 (\"worktree: add details to the worktree\n struct\", 2015-10-08), following a pre-core.bare heuristic. This patch\n does 2 things:\n \n @@ -22,12 +23,14 @@\n the \"git branch -D \" problem described above. However...\n \n - If a repository has core.bare=1 but the \"git\" command is being run\n - from one of its added worktrees, is_bare_repository() returns false\n - (which is fine, since there is a worktree available). However,\n - commands like \"git worktree list\" currently distinguish between a\n - repository that has core.bare=1 but has a worktree available, and a\n - repository that is truly non-bare (core.bare=0). To preserve this\n - distinction, also check core.bare when setting is_bare. If\n + from one of its secondary worktrees, is_bare_repository() returns\n + false (which is fine, since there is a worktree available). However,\n + treating the main worktree as non-bare when it is bare causes issues:\n + for example, failure to delete a branch from a secondary worktree\n + that is referred to by a main worktree's HEAD, even if that main\n + worktree is bare.\n +\n + In order to avoid that, also check core.bare when setting is_bare. If\n core.bare=1, trust it, and otherwise, use is_bare_repository().\n \n Signed-off-by: Jonathan Tan \n @@ -52,6 +55,16 @@\n +\tgit -C repo2/sub checkout -b work &&\n +\ttest_must_fail git -C repo2/sub branch -D work\n +'\n ++\n ++test_expect_success 'bare main worktree has HEAD at branch deleted by secondary worktree' '\n ++\ttest_when_finished \"rm -rf nonbare base secondary\" &&\n ++\n ++\tgit init nonbare &&\n ++\ttest_commit -C nonbare x &&\n ++\tgit clone --bare nonbare bare &&\n ++\tgit -C bare worktree add --detach ../secondary master &&\n ++\tgit -C secondary branch -D master\n ++'\n +\n test_expect_success 'git branch --list -v with --abbrev' '\n \ttest_when_finished \"git branch -D t\" &&\n @@ -77,6 +90,13 @@\n \tworktree = xcalloc(1, sizeof(*worktree));\n \tworktree->path = strbuf_detach(&worktree_path, NULL);\n -\tworktree->is_bare = is_bare;\n ++\t/*\n ++\t * NEEDSWORK: If this function is called from a secondary worktree and\n ++\t * config.worktree is present, is_bare_repository_cfg will reflect the\n ++\t * contents of config.worktree, not the contents of the main worktree.\n ++\t * This means that worktree->is_bare may be set to 0 even if the main\n ++\t * worktree is configured to be bare.\n ++\t */\n +\tworktree->is_bare = (is_bare_repository_cfg == 1) ||\n +\t\tis_bare_repository();\n \tadd_head_info(worktree);\n\n t/t3200-branch.sh | 24 ++++++++++++++++++++++++\n worktree.c | 14 ++++++++++----\n 2 files changed, 34 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 478b82cf9b..e9ad50b66d 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -264,6 +264,30 @@ test_expect_success 'git branch --list -d t should fail' '\n \ttest_must_fail git rev-parse refs/heads/t\n '\n \n+test_expect_success 'deleting checked-out branch from repo that is a submodule' '\n+\ttest_when_finished \"rm -rf repo1 repo2\" &&\n+\n+\tgit init repo1 &&\n+\tgit init repo1/sub &&\n+\ttest_commit -C repo1/sub x &&\n+\tgit -C repo1 submodule add ./sub &&\n+\tgit -C repo1 commit -m \"adding sub\" &&\n+\n+\tgit clone --recurse-submodules repo1 repo2 &&\n+\tgit -C repo2/sub checkout -b work &&\n+\ttest_must_fail git -C repo2/sub branch -D work\n+'\n+\n+test_expect_success 'bare main worktree has HEAD at branch deleted by secondary worktree' '\n+\ttest_when_finished \"rm -rf nonbare base secondary\" &&\n+\n+\tgit init nonbare &&\n+\ttest_commit -C nonbare x &&\n+\tgit clone --bare nonbare bare &&\n+\tgit -C bare worktree add --detach ../secondary master &&\n+\tgit -C secondary branch -D master\n+'\n+\n test_expect_success 'git branch --list -v with --abbrev' '\n \ttest_when_finished \"git branch -D t\" &&\n \tgit branch t &&\ndiff --git a/worktree.c b/worktree.c\nindex b45bfeb9d3..4f66cd9ce1 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -49,18 +49,24 @@ static struct worktree *get_main_worktree(void)\n \tstruct worktree *worktree = NULL;\n \tstruct strbuf path = STRBUF_INIT;\n \tstruct strbuf worktree_path = STRBUF_INIT;\n-\tint is_bare = 0;\n \n \tstrbuf_add_absolute_path(&worktree_path, get_git_common_dir());\n-\tis_bare = !strbuf_strip_suffix(&worktree_path, \"/.git\");\n-\tif (is_bare)\n+\tif (!strbuf_strip_suffix(&worktree_path, \"/.git\"))\n \t\tstrbuf_strip_suffix(&worktree_path, \"/.\");\n \n \tstrbuf_addf(&path, \"%s/HEAD\", get_git_common_dir());\n \n \tworktree = xcalloc(1, sizeof(*worktree));\n \tworktree->path = strbuf_detach(&worktree_path, NULL);\n-\tworktree->is_bare = is_bare;\n+\t/*\n+\t * NEEDSWORK: If this function is called from a secondary worktree and\n+\t * config.worktree is present, is_bare_repository_cfg will reflect the\n+\t * contents of config.worktree, not the contents of the main worktree.\n+\t * This means that worktree->is_bare may be set to 0 even if the main\n+\t * worktree is configured to be bare.\n+\t */\n+\tworktree->is_bare = (is_bare_repository_cfg == 1) ||\n+\t\tis_bare_repository();\n \tadd_head_info(worktree);\n \n \tstrbuf_release(&path);\n-- \n2.21.0.392.gf8f6787159e-goog\n\n"},{"id":"374163","messageId":"CACsJy8CoaOmJpFygQxrFyJmpzL4wyR0Dbck_TyVOFViRbtqAHQ@mail.gmail.com","threadId":"50947","inReplyTo":"20190418183000.78138-1-jonathantanmy@google.com","subject":"Re: [PATCH] worktree: update is_bare heuristics","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-04-19T10:50:39Z","receivedAt":"2019-04-19T20:17:31Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Apr 19, 2019 at 1:30 AM Jonathan Tan wrote:\n>\n> > You actually didn't spell out the problem with \"git branch -D\", or at\n> > least the consequence (i.e. the submodule branch is deleted even if\n> > it's checked out).\n>\n> Thanks - I'll do that in the commit message.\n\nAnother minor nit (because I was still puzzled why a submodule was\nconsidered bare/not bare)\n\n> This is because get_main_worktree() in worktree.c sets is_bare on a\n> worktree only using the heuristic that a repo is bare if the worktree's\n> path ends in \"/.git\", and not bare otherwise.\n\ns/ends/does not end/. Now it makes more sense because the submodule's\nmain worktree is accidentally considered \"bare\" (i.e. no worktree),\nits HEAD is ignored.\n\n> > > strbuf_addf(&path, \"%s/HEAD\", get_git_common_dir());\n> > >\n> > > worktree = xcalloc(1, sizeof(*worktree));\n> > > worktree->path = strbuf_detach(&worktree_path, NULL);\n> > > - worktree->is_bare = is_bare;\n> > > + worktree->is_bare = (is_bare_repository_cfg == 1) ||\n> >\n> > core.bare and core.worktree are special. When you access them standing\n> > from the main worktree, you'll see them. But when you stand from a\n> > secondary worktree, they are ignored.\n>\n> Just checking: I think that is_bare_repository_cfg ignores core.bare\n> only if the config.worktree file is present?\n\nNo, both are ignored independently if you read them from a secondary\nworktree. Tested too with rev-parse, just to be sure.\n\n> In the t2402 test '\"list\"\n> all worktrees from linked with a bare main', \"git worktree list\" still\n> observes the main worktree as bare.\n\nYes, because of bug :(\n\nWhen git_config() is called again by cmd_worktree(), it does not treat\ncore.worktree and core.bare special anymore. So is_bare_repository_cfg\nis reset from 0 to 1, even though I think its value at that point\nplays no role anymore. What matters is the value at\nsetup_git_directory().\n\nSo yeah your code works in all cases by luck ;)\n\nFixing that one will not be easy (to avoid traps). I did try to delete\nis_bare_repository_cfg (on the ground that global vars are hard to\nmanage, as seen here) only to discover that I could not simply delete\nthe core.bare parsing code in git_default_core_config() without\nchanging behaviour in some case [1]. I should probably revive that\nbranch (cleaning up gitdir setup code a bit) and submit.\n\n[1] https://gitlab.com/pclouds/git/commit/2cc46d698ebe7af295e0d91f68103675077df68e#db04685516ffb491eb4239291b892fc0784e1aea\n\n> > I don't think multiple-worktrees-on-submodules will be coming soon, so\n> > it's probably ok. But maybe leave a note here.\n>\n> Observing that the problematic case is the 3rd one above, would this\n> note work:\n>\n> NEEDSWORK: If this function is called from a secondary worktree and\n> config.worktree is present, is_bare_repository_cfg will reflect the\n> contents of config.worktree, not the contents of the main worktree.\n> This means that worktree->is_bare may be set to 0 even if the main\n> worktree is configured to be bare.\n\nEven though your code works perfectly now, I still think it's good to\nhave some comment like this.\n-- \nDuy\n"}]}