{"thread":{"id":"52799","subject":"[PATCH 0/3] [GSoC] receive.denyCurrentBranch: respect all worktrees","startedAt":"2020-02-13T18:59:14Z","lastAt":"2020-02-27T15:59:00Z","messageCount":30,"participants":["Hariom Verma via GitGitGadget","Junio C Hamano","Johannes Schindelin","Hariom verma","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"391691","messageId":"pull.535.git.1581620351.gitgitgadget@gmail.com","threadId":"52799","inReplyTo":null,"subject":"[PATCH 0/3] [GSoC] receive.denyCurrentBranch: respect all worktrees","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-13T18:59:07Z","receivedAt":"2020-02-13T18:59:14Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"The receive.denyCurrentBranch config option controls what happens if you\npush to a branch that is checkout into a non-bare repository. By default, it\nrejects it. It can be disabled via ignore or warn. Another yet trickier\noption is updateInstead.\n\nWhen receive.denyCurrentBranch is set to updateInstead, a push that tries to\nupdate the branch that is currently checked out is accepted only when the\nindex and the working tree exactly matches the currently checked out commit,\nin which case the index and the working tree are updated to match the pushed\ncommit. Otherwise, the push is refused.\n\nHowever, this setting was forgotten when the git worktree command was\nintroduced: only the main worktree's current branch is respected. [ fixes:\n#331 ]\n\nIncidently, this change also fixes another bug i.e. \nreceive.denyCurrentBranch = true was ignored when pushing into a non-bare\nrepository's unborn current branch.\n\nThanks, @dscho for helping me out.\n\nRegards, Hariom\n\nHariom Verma (3):\n get_main_worktree(): allow it to be called in the Git directory\n t5509: initialized `pushee` as bare repository\n receive.denyCurrentBranch: respect all worktrees\n\n builtin/receive-pack.c | 37 +++++++++++++++++---------------\n t/t5509-fetch-push-namespaces.sh | 2 +-\n t/t5516-fetch-push.sh | 11 ++++++++++\n worktree.c | 1 +\n 4 files changed, 33 insertions(+), 18 deletions(-)\n\n\nbase-commit: 232378479ee6c66206d47a9be175e3a39682aea6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-535%2Fharry-hov%2Fdeny-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-535/harry-hov/deny-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/535\n-- \ngitgitgadget\n"},{"id":"391692","messageId":"d156d04ca87f9fcffb1c08a08576dddcdc64c055.1581620351.git.gitgitgadget@gmail.com","threadId":"52799","inReplyTo":"pull.535.git.1581620351.gitgitgadget@gmail.com","subject":"[PATCH 2/3] t5509: initialized `pushee` as bare repository","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-13T18:59:09Z","receivedAt":"2020-02-13T18:59:16Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"From: Hariom Verma \n\n`receive.denyCurrentBranch` currently has a bug where it allows pushing\ninto the current branch of a non-bare repository as long as it does not\nhave any commits. This would cause t5509 to fail once that bug is fixed\nbecause it pushes into an unborn current branch.\n\nIn t5509, no operations are performed inside `pushee`, as it is only a\ntarget for `git push` and `git ls-remote` calls. Therefore it does not\nneed to have a worktree. So, it is safe to change `pushee` to a bare\nrepository.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Hariom Verma \n---\n t/t5509-fetch-push-namespaces.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh\nindex 75cbfcc392..e3975bd21d 100755\n--- a/t/t5509-fetch-push-namespaces.sh\n+++ b/t/t5509-fetch-push-namespaces.sh\n@@ -20,7 +20,7 @@ test_expect_success setup '\n \t) &&\n \tcommit0=$(cd original && git rev-parse HEAD^) &&\n \tcommit1=$(cd original && git rev-parse HEAD) &&\n-\tgit init pushee &&\n+\tgit init --bare pushee &&\n \tgit init puller\n '\n \n-- \ngitgitgadget\n\n"},{"id":"391693","messageId":"902c8a3f17153ebd3871aa51e5cabe9338438655.1581620351.git.gitgitgadget@gmail.com","threadId":"52799","inReplyTo":"pull.535.git.1581620351.gitgitgadget@gmail.com","subject":"[PATCH 1/3] get_main_worktree(): allow it to be called in the Git directory","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-13T18:59:08Z","receivedAt":"2020-02-13T18:59:17Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"From: Hariom Verma \n\nWhen called in the Git directory of a non-bare repository, this function\nwould not return the directory of the main worktree, but of the Git\ndirectory instead.\n\nThe reason: when the Git directory is the current working directory, the\nabsolute path of the common directory will be reported with a trailing\n`/.git/.`, which the code of `get_main_worktree()` does not handle\ncorrectly.\n\nLet's fix this.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Hariom Verma \n---\n worktree.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/worktree.c b/worktree.c\nindex 5b4793caa3..7c8cd21317 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -51,6 +51,7 @@ static struct worktree *get_main_worktree(void)\n \tstruct strbuf worktree_path = STRBUF_INIT;\n \n \tstrbuf_add_absolute_path(&worktree_path, get_git_common_dir());\n+\tstrbuf_strip_suffix(&worktree_path, \"/.\");\n \tif (!strbuf_strip_suffix(&worktree_path, \"/.git\"))\n \t\tstrbuf_strip_suffix(&worktree_path, \"/.\");\n \n-- \ngitgitgadget\n\n"},{"id":"391694","messageId":"3352c0bffc19f17518b292ad36c38f902801b06a.1581620351.git.gitgitgadget@gmail.com","threadId":"52799","inReplyTo":"pull.535.git.1581620351.gitgitgadget@gmail.com","subject":"[PATCH 3/3] receive.denyCurrentBranch: respect all worktrees","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-13T18:59:10Z","receivedAt":"2020-02-13T18:59:18Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"From: Hariom Verma \n\nThe receive.denyCurrentBranch config option controls what happens if\nyou push to a branch that is checked out into a non-bare repository.\nBy default, it rejects it. It can be disabled via `ignore` or `warn`.\nAnother yet trickier option is `updateInstead`.\n\nHowever, this setting was forgotten when the git worktree command was\nintroduced: only the main worktree's current branch is respected.\n\nWith this change, all worktrees are respected.\n\nThat change also leads to revealing another bug,\ni.e. `receive.denyCurrentBranch = true` was ignored when pushing into a\nnon-bare repository's unborn current branch. As `is_ref_checked_out()`\nreturns 0 which means `receive-pack` does not get into conditional\nstatement to switch `deny_current_branch` accordingly(ignore, warn,\nrefuse, unconfigured, updateInstead).\n\nreceive.denyCurrentBranch uses the function `refs_resolve_ref_unsafe()`\n(called via `resolve_refdup()`) to resolve the symbolic ref HEAD, but\nthat function fails when HEAD does not point at a valid commit.\nAs we replace the call to `refs_resolve_ref_unsafe()` with\n`find_shared_symref()`, which has no problem finding the worktree for a\ngiven branch even if it is unborn yet, this bug is fixed at the same\ntime: receive.denyCurrentBranch now also handles worktrees with unborn\nbranches as intended.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Hariom Verma \n---\n builtin/receive-pack.c | 37 ++++++++++++++++++++-----------------\n t/t5516-fetch-push.sh | 11 +++++++++++\n 2 files changed, 31 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 411e0b4d99..b5ca3123b7 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -27,6 +27,7 @@\n #include \"object-store.h\"\n #include \"protocol.h\"\n #include \"commit-reach.h\"\n+#include \"worktree.h\"\n \n static const char * const receive_pack_usage[] = {\n \tN_(\"git receive-pack \"),\n@@ -816,16 +817,6 @@ static int run_update_hook(struct command *cmd)\n \treturn finish_command(&proc);\n }\n \n-static int is_ref_checked_out(const char *ref)\n-{\n-\tif (is_bare_repository())\n-\t\treturn 0;\n-\n-\tif (!head_name)\n-\t\treturn 0;\n-\treturn !strcmp(head_name, ref);\n-}\n-\n static char *refuse_unconfigured_deny_msg =\n \tN_(\"By default, updating the current branch in a non-bare repository\\n\"\n \t \"is denied, because it will make the index and work tree inconsistent\\n\"\n@@ -997,16 +988,27 @@ static const char *push_to_checkout(unsigned char *hash,\n \t\treturn NULL;\n }\n \n-static const char *update_worktree(unsigned char *sha1)\n+static const char *update_worktree(unsigned char *sha1, const struct worktree *worktree)\n {\n-\tconst char *retval;\n-\tconst char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : \"..\";\n+\tconst char *retval, *work_tree, *git_dir = NULL;\n \tstruct argv_array env = ARGV_ARRAY_INIT;\n \n+\tif (worktree && worktree->path)\n+\t\twork_tree = worktree->path;\n+\telse if (git_work_tree_cfg)\n+\t\twork_tree = git_work_tree_cfg;\n+\telse\n+\t\twork_tree = \"..\";\n+\n \tif (is_bare_repository())\n \t\treturn \"denyCurrentBranch = updateInstead needs a worktree\";\n+\t\n+\tif (worktree)\n+\t\tgit_dir = get_worktree_git_dir(worktree);\n+\tif (!git_dir)\n+\t\tgit_dir = get_git_dir();\n \n-\targv_array_pushf(&env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n+\targv_array_pushf(&env, \"GIT_DIR=%s\", absolute_path(git_dir));\n \n \tif (!find_hook(push_to_checkout_hook))\n \t\tretval = push_to_deploy(sha1, &env, work_tree);\n@@ -1026,6 +1028,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \tstruct object_id *old_oid = &cmd->old_oid;\n \tstruct object_id *new_oid = &cmd->new_oid;\n \tint do_update_worktree = 0;\n+\tconst struct worktree *worktree = is_bare_repository() ? NULL : find_shared_symref(\"HEAD\", name);\n \n \t/* only refs/... are allowed */\n \tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n@@ -1037,7 +1040,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \tfree(namespaced_name);\n \tnamespaced_name = strbuf_detach(&namespaced_name_buf, NULL);\n \n-\tif (is_ref_checked_out(namespaced_name)) {\n+\tif (worktree) {\n \t\tswitch (deny_current_branch) {\n \t\tcase DENY_IGNORE:\n \t\t\tbreak;\n@@ -1069,7 +1072,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t\treturn \"deletion prohibited\";\n \t\t}\n \n-\t\tif (head_name && !strcmp(namespaced_name, head_name)) {\n+\t\tif (worktree || (head_name && !strcmp(namespaced_name, head_name))) {\n \t\t\tswitch (deny_delete_current) {\n \t\t\tcase DENY_IGNORE:\n \t\t\t\tbreak;\n@@ -1118,7 +1121,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t}\n \n \tif (do_update_worktree) {\n-\t\tret = update_worktree(new_oid->hash);\n+\t\tret = update_worktree(new_oid->hash, find_shared_symref(\"HEAD\", name));\n \t\tif (ret)\n \t\t\treturn ret;\n \t}\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex c81ca360ac..6608e391f0 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1712,4 +1712,15 @@ test_expect_success 'updateInstead with push-to-checkout hook' '\n \t)\n '\n \n+test_expect_success 'denyCurrentBranch and worktrees' '\n+ git worktree add new-wt &&\n+\tgit clone . cloned &&\n+\ttest_commit -C cloned first &&\n+\ttest_config receive.denyCurrentBranch refuse &&\n+\ttest_must_fail git -C cloned push origin HEAD:new-wt &&\n+\ttest_config receive.denyCurrentBranch updateInstead &&\n+\tgit -C cloned push origin HEAD:new-wt &&\n+\ttest_must_fail git -C cloned push --delete origin new-wt\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"391697","messageId":"xmqqsgjeqm6w.fsf@gitster-ct.c.googlers.com","threadId":"52799","inReplyTo":"d156d04ca87f9fcffb1c08a08576dddcdc64c055.1581620351.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] t5509: initialized `pushee` as bare repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-13T20:14:31Z","receivedAt":"2020-02-13T20:14:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Hariom Verma via GitGitGadget\" writes:\n\n> From: Hariom Verma \n>\n> `receive.denyCurrentBranch` currently has a bug where it allows pushing\n> into the current branch of a non-bare repository as long as it does not\n> have any commits.\n\nCan patch 3/3 be split into two, so that the fix would protect an\nalready populated branch that is checked out anywhere (not in the\nprimary worktree--which is the bug you are fixing) from getting\nupdated but still allow an unborn branch to be updated, and then\nhave patch 4/3 that forbids an update to even an unborn branch\n\"checked out\" in any working tree? This update to the test can then\nbecome part of patch 4/3.\n\nThanks.\n"},{"id":"391699","messageId":"xmqqo8u2qla6.fsf@gitster-ct.c.googlers.com","threadId":"52799","inReplyTo":"xmqqsgjeqm6w.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/3] t5509: initialized `pushee` as bare repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-13T20:34:09Z","receivedAt":"2020-02-13T20:34:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"Hariom Verma via GitGitGadget\" writes:\n>\n>> From: Hariom Verma \n>>\n>> `receive.denyCurrentBranch` currently has a bug where it allows pushing\n>> into the current branch of a non-bare repository as long as it does not\n>> have any commits.\n>\n> Can patch 3/3 be split into two, so that the fix would protect an\n> already populated branch that is checked out anywhere (not in the\n> primary worktree--which is the bug you are fixing) from getting\n> updated but still allow an unborn branch to be updated, and then\n> have patch 4/3 that forbids an update to even an unborn branch\n> \"checked out\" in any working tree? This update to the test can then\n> become part of patch 4/3.\n\nOh, another thing. The patch 4/3 that starts forbidding a push into\na checked out unborn branch should also have a test that makes sure\nthat such an attempt fails. IOW, making the test repository used in\nthe test you changed to a bare one, to allow existing test to still\ntest what it wants to test, like you did in this patch is OK, but we\nwould want to have a new test that prepares a repository with the\nprimary and the secondary worktrees, check out an unborn branch in\neach of the worktrees, and make sure receive.denyCurrentBranch would\nprevent \"git push\" to update these branches.\n\nThanks.\n"},{"id":"391747","messageId":"nycvar.QRO.7.76.6.2002141252050.46@tvgsbejvaqbjf.bet","threadId":"52799","inReplyTo":"xmqqsgjeqm6w.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/3] t5509: initialized `pushee` as bare repository","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-02-14T11:59:06Z","receivedAt":"2020-02-14T11:59:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 13 Feb 2020, Junio C Hamano wrote:\n\n> \"Hariom Verma via GitGitGadget\" writes:\n>\n> > From: Hariom Verma \n> >\n> > `receive.denyCurrentBranch` currently has a bug where it allows pushing\n> > into the current branch of a non-bare repository as long as it does not\n> > have any commits.\n>\n> Can patch 3/3 be split into two,\n\nI actually don't think so. The `refs_resolve_unsafe()` function simply\nrequires a tip commit, so it is the wrong function to call in this\ncontext. And the fix for it is to use a more appropriate function, which\n3/3 already does (although for an unrelated reason).\n\nIn other words, a fix for one bug would be a fix for the other, and\n(probably) vice versa.\n\n> so that the fix would protect an already populated branch that is\n> checked out anywhere (not in the primary worktree--which is the bug you\n> are fixing) from getting updated but still allow an unborn branch to be\n> updated, and then have patch 4/3 that forbids an update to even an\n> unborn branch \"checked out\" in any working tree? This update to the\n> test can then become part of patch 4/3.\n\nI agree that this merits a regression test.\n\nThanks,\nDscho\n\n>\n> Thanks.\n>\n"},{"id":"391761","messageId":"xmqqpnehp5x4.fsf@gitster-ct.c.googlers.com","threadId":"52799","inReplyTo":"nycvar.QRO.7.76.6.2002141252050.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/3] t5509: initialized `pushee` as bare repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-14T15:03:35Z","receivedAt":"2020-02-14T15:03:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> Can patch 3/3 be split into two,\n>\n> I actually don't think so. The `refs_resolve_unsafe()` function simply\n> ...\n> In other words, a fix for one bug would be a fix for the other, and\n> (probably) vice versa.\n\nWhat mislead me was the way this step presented itself. It sounded\nas if the primary (and possibly the only) thing the series wanted to\nfix was to make .denyCurrentBranch pay attention to other worktrees,\nand while fixing that, it broke as collateral damage a \"feature\"\nthat denyCurrentBranch allows an unborn branch to be updated no\nmatter what and called it a bugfix when it was not a bug.\n\nIf the series is fixing two bugs, perhaps 2/3 can first fix it for a\nprimary worktree case by seeing what HEAD symref for the primary\nworktree points at is the target of a push without iterating over\nall the worktrees, have the test change in 2/3 (i.e. \"fixing the\n'unborn' case revealed a wrong expectation in an existing test\"),\nand a couple of new tests to see what a push from sideways would do\nto an unborn branch that is checked out in the primary worktree when\n.denyCurrentBranch is and isn't in effect.\n\nThen 3/3 can use the same logic to see if one worktree is OK with\nthe proposed ref update by the push used in 2/3 (which no longer\nuses refs_resolve_unsafe()') to check for all worktrees. The new\ntests introduced in 2/3 would be extended to see what happens when\nthe unborn branch getting updated by the push happens to be checked\nout in a secondary worktree.\n\nThat would have avoided misleading this reader.\n\nThanks.\n\n"},{"id":"391858","messageId":"CA+CkUQ-PERGy8xJ-a=5kzbN+N9f4uVQ35Hc4Aob70gJGz++fKQ@mail.gmail.com","threadId":"52799","inReplyTo":"xmqqpnehp5x4.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/3] t5509: initialized `pushee` as bare repository","fromName":"Hariom verma","fromEmail":"hariom18599@gmail.com","sentAt":"2020-02-15T21:52:48Z","receivedAt":"2020-02-15T21:53:12Z","isPatch":true,"sender":{"key":"hariom18599@gmail.com","avatar":"https://avatars.githubusercontent.com/u/37576387?v=4"},"body":"On Fri, Feb 14, 2020 at 8:33 PM Junio C Hamano wrote:\n> If the series is fixing two bugs, perhaps 2/3 can first fix it for a\n> primary worktree case by seeing what HEAD symref for the primary\n> worktree points at is the target of a push without iterating over\n> all the worktrees, have the test change in 2/3 (i.e. \"fixing the\n> 'unborn' case revealed a wrong expectation in an existing test\"),\n> and a couple of new tests to see what a push from sideways would do\n> to an unborn branch that is checked out in the primary worktree when\n> .denyCurrentBranch is and isn't in effect.\n>\n> Then 3/3 can use the same logic to see if one worktree is OK with\n> the proposed ref update by the push used in 2/3 (which no longer\n> uses refs_resolve_unsafe()') to check for all worktrees. The new\n> tests introduced in 2/3 would be extended to see what happens when\n> the unborn branch getting updated by the push happens to be checked\n> out in a secondary worktree.\n\nAs far as my understanding goes, what we want is:\n1) fixing `.denyCurrentBranch` for unborn branches in primary worktree. (2/3)\n2) writing test (expect it to fail if `unborn` & 'non-bare' case) (2/3)\n3) making `.denyCurrentBranch` respect all worktrees. (3/3)\n4) extending tests written in step 2 for secondary worktrees. (3/3)\n\nCorrect me if I'm wrong.\nAs I'm not entirely familiar with working and structure of\n`.denyCurrentBranch`. So I might need more explicit explanation.\n\nThanks,\nHariom\n"},{"id":"391882","messageId":"xmqqo8tym6sr.fsf@gitster-ct.c.googlers.com","threadId":"52799","inReplyTo":"CA+CkUQ-PERGy8xJ-a=5kzbN+N9f4uVQ35Hc4Aob70gJGz++fKQ@mail.gmail.com","subject":"Re: [PATCH 2/3] t5509: initialized `pushee` as bare repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-16T23:49:40Z","receivedAt":"2020-02-16T23:49:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hariom verma writes:\n\n> On Fri, Feb 14, 2020 at 8:33 PM Junio C Hamano wrote:\n>> If the series is fixing two bugs, perhaps 2/3 can first fix it for a\n>> primary worktree case by seeing what HEAD symref for the primary\n>> worktree points at is the target of a push without iterating over\n>> all the worktrees, have the test change in 2/3 (i.e. \"fixing the\n>> 'unborn' case revealed a wrong expectation in an existing test\"),\n>> and a couple of new tests to see what a push from sideways would do\n>> to an unborn branch that is checked out in the primary worktree when\n>> .denyCurrentBranch is and isn't in effect.\n>>\n>> Then 3/3 can use the same logic to see if one worktree is OK with\n>> the proposed ref update by the push used in 2/3 (which no longer\n>> uses refs_resolve_unsafe()') to check for all worktrees. The new\n>> tests introduced in 2/3 would be extended to see what happens when\n>> the unborn branch getting updated by the push happens to be checked\n>> out in a secondary worktree.\n>\n> As far as my understanding goes, what we want is:\n> 1) fixing `.denyCurrentBranch` for unborn branches in primary worktree. (2/3)\n> 2) writing test (expect it to fail if `unborn` & 'non-bare' case) (2/3)\n> 3) making `.denyCurrentBranch` respect all worktrees. (3/3)\n> 4) extending tests written in step 2 for secondary worktrees. (3/3)\n>\n> Correct me if I'm wrong.\n\nIf the above is what _you_ want, then there is nothing for me to\ncorrect ;-)\n\nWhat I suggested was somewhat different, though.\n\n 1) get_main_worktree() fix you have as [1/3] in the current round.\n\n 2) fix `.denyCurrentBranch` for unborn branches in the primary\n worktree, new tests for the cases I outlined in the message you\n are responding to, and adjusting the test (i.e. what you have\n as [2/3] in the current round).\n\n 3) fix `.denyCurrentBranch` to pay attention to HEAD of not just\n the primary worktree but of all the worktrees, and add tests.\n\nThanks.\n"},{"id":"392335","messageId":"5c749e044a3846d7757b198f29d984d181636556.1582410908.git.gitgitgadget@gmail.com","threadId":"52799","inReplyTo":"pull.535.v2.git.1582410908.gitgitgadget@gmail.com","subject":"[PATCH v2 2/4] t5509: initialized `pushee` as bare repository","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-22T22:35:05Z","receivedAt":"2020-02-22T22:35:14Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"From: Hariom Verma \n\n`receive.denyCurrentBranch` currently has a bug where it allows pushing\ninto non-bare repository using namespaces as long as it does not have any\ncommits. This would cause t5509 to fail once that bug is fixed because it\npushes into an unborn current branch.\n\nIn t5509, no operations are performed inside `pushee`, as it is only a\ntarget for `git push` and `git ls-remote` calls. Therefore it does not\nneed to have a worktree. So, it is safe to change `pushee` to a bare\nrepository.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Hariom Verma \n---\n t/t5509-fetch-push-namespaces.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh\nindex 75cbfcc392c..e3975bd21de 100755\n--- a/t/t5509-fetch-push-namespaces.sh\n+++ b/t/t5509-fetch-push-namespaces.sh\n@@ -20,7 +20,7 @@ test_expect_success setup '\n \t) &&\n \tcommit0=$(cd original && git rev-parse HEAD^) &&\n \tcommit1=$(cd original && git rev-parse HEAD) &&\n-\tgit init pushee &&\n+\tgit init --bare pushee &&\n \tgit init puller\n '\n \n-- \ngitgitgadget\n\n"},{"id":"392336","messageId":"pull.535.v2.git.1582410908.gitgitgadget@gmail.com","threadId":"52799","inReplyTo":"pull.535.git.1581620351.gitgitgadget@gmail.com","subject":"[PATCH v2 0/4] [GSoC] receive.denyCurrentBranch: respect all worktrees","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-22T22:35:03Z","receivedAt":"2020-02-22T22:35:14Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"The receive.denyCurrentBranch config option controls what happens if you\npush to a branch that is checkout into a non-bare repository. By default, it\nrejects it. It can be disabled via ignore or warn. Another yet trickier\noption is updateInstead.\n\nWhen receive.denyCurrentBranch is set to updateInstead, a push that tries to\nupdate the branch that is currently checked out is accepted only when the\nindex and the working tree exactly matches the currently checked out commit,\nin which case the index and the working tree are updated to match the pushed\ncommit. Otherwise, the push is refused.\n\nHowever, this setting was forgotten when the git worktree command was\nintroduced: only the main worktree's current branch is respected. [ fixes:\n#331 ]\n\nIncidently, this change also fixes another bug i.e. \nreceive.denyCurrentBranch = true was ignored when pushing into a non-bare\nrepository using ref namespaces.\n\nThanks, @dscho for helping me out.\n\nRegards, Hariom\n\nHariom Verma (4):\n get_main_worktree(): allow it to be called in the Git directory\n t5509: initialized `pushee` as bare repository\n bug: denyCurrentBranch and unborn branch with ref namespace\n receive.denyCurrentBranch: respect all worktrees\n\n builtin/receive-pack.c | 37 +++++++++++++++++---------------\n t/t5509-fetch-push-namespaces.sh | 11 +++++++++-\n t/t5516-fetch-push.sh | 11 ++++++++++\n worktree.c | 1 +\n 4 files changed, 42 insertions(+), 18 deletions(-)\n\n\nbase-commit: 232378479ee6c66206d47a9be175e3a39682aea6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-535%2Fharry-hov%2Fdeny-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-535/harry-hov/deny-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/535\n\nRange-diff vs v1:\n\n 1: 902c8a3f171 = 1: 8718facbc95 get_main_worktree(): allow it to be called in the Git directory\n 2: d156d04ca87 ! 2: 5c749e044a3 t5509: initialized `pushee` as bare repository\n @@ -3,9 +3,9 @@\n t5509: initialized `pushee` as bare repository\n \n `receive.denyCurrentBranch` currently has a bug where it allows pushing\n - into the current branch of a non-bare repository as long as it does not\n - have any commits. This would cause t5509 to fail once that bug is fixed\n - because it pushes into an unborn current branch.\n + into non-bare repository using namespaces as long as it does not have any\n + commits. This would cause t5509 to fail once that bug is fixed because it\n + pushes into an unborn current branch.\n \n In t5509, no operations are performed inside `pushee`, as it is only a\n target for `git push` and `git ls-remote` calls. Therefore it does not\n -: ----------- > 3: b3e573d44a9 bug: denyCurrentBranch and unborn branch with ref namespace\n 3: 3352c0bffc1 ! 4: 61e5f75a6f9 receive.denyCurrentBranch: respect all worktrees\n @@ -14,10 +14,10 @@\n \n That change also leads to revealing another bug,\n i.e. `receive.denyCurrentBranch = true` was ignored when pushing into a\n - non-bare repository's unborn current branch. As `is_ref_checked_out()`\n - returns 0 which means `receive-pack` does not get into conditional\n - statement to switch `deny_current_branch` accordingly(ignore, warn,\n - refuse, unconfigured, updateInstead).\n + non-bare repository's unborn current branch using ref namespaces. As\n + `is_ref_checked_out()` returns 0 which means `receive-pack` does not get\n + into conditional statement to switch `deny_current_branch` accordingly\n + (ignore, warn, refuse, unconfigured, updateInstead).\n \n receive.denyCurrentBranch uses the function `refs_resolve_ref_unsafe()`\n (called via `resolve_refdup()`) to resolve the symbolic ref HEAD, but\n @@ -26,7 +26,7 @@\n `find_shared_symref()`, which has no problem finding the worktree for a\n given branch even if it is unborn yet, this bug is fixed at the same\n time: receive.denyCurrentBranch now also handles worktrees with unborn\n - branches as intended.\n + branches as intended even while using ref namespaces.\n \n Helped-by: Johannes Schindelin \n Signed-off-by: Hariom Verma \n @@ -127,6 +127,19 @@\n \t\t\treturn ret;\n \t}\n \n + diff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh\n + --- a/t/t5509-fetch-push-namespaces.sh\n + +++ b/t/t5509-fetch-push-namespaces.sh\n +@@\n + \ttest_cmp expect actual\n + '\n + \n +-test_expect_failure 'denyCurrentBranch and unborn branch with ref namespace' '\n ++test_expect_success 'denyCurrentBranch and unborn branch with ref namespace' '\n + \tcd original &&\n + \tgit init unborn &&\n + \tgit remote add unborn-namespaced \"ext::git --namespace=namespace %s unborn\" &&\n +\n diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n --- a/t/t5516-fetch-push.sh\n +++ b/t/t5516-fetch-push.sh\n @@ -135,7 +148,7 @@\n '\n \n +test_expect_success 'denyCurrentBranch and worktrees' '\n -+ git worktree add new-wt &&\n ++\tgit worktree add new-wt &&\n +\tgit clone . cloned &&\n +\ttest_commit -C cloned first &&\n +\ttest_config receive.denyCurrentBranch refuse &&\n\n-- \ngitgitgadget\n"},{"id":"392337","messageId":"b3e573d44a99a828e710f06b723942107189afeb.1582410908.git.gitgitgadget@gmail.com","threadId":"52799","inReplyTo":"pull.535.v2.git.1582410908.gitgitgadget@gmail.com","subject":"[PATCH v2 3/4] bug: denyCurrentBranch and unborn branch with ref namespace","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-22T22:35:06Z","receivedAt":"2020-02-22T22:35:16Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"From: Hariom Verma \n\ngit supports an interesting feature 'Git namespaces' that allows\ndividing the refs of a single repository into multiple namespaces, each\nof which has its own branches, tags, and HEAD. But unfortunately, there\nexists a bug in `denyCurrentBranch` which allows pushing into a non-bare\nrepository using a ref namespace even if it does not have any commits\n\nHere is a nice and short demonstration of that bug.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Hariom Verma \n---\n t/t5509-fetch-push-namespaces.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh\nindex e3975bd21de..c89483fdba2 100755\n--- a/t/t5509-fetch-push-namespaces.sh\n+++ b/t/t5509-fetch-push-namespaces.sh\n@@ -152,4 +152,13 @@ test_expect_success 'clone chooses correct HEAD (v2)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_failure 'denyCurrentBranch and unborn branch with ref namespace' '\n+\tcd original &&\n+\tgit init unborn &&\n+\tgit remote add unborn-namespaced \"ext::git --namespace=namespace %s unborn\" &&\n+\ttest_must_fail git push unborn-namespaced HEAD:master &&\n+\ttest_config -C unborn receive.denyCurrentBranch updateInstead &&\n+\tgit push unborn-namespaced HEAD:master\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"392338","messageId":"8718facbc951614f19407afa6ca8d6110507483d.1582410908.git.gitgitgadget@gmail.com","threadId":"52799","inReplyTo":"pull.535.v2.git.1582410908.gitgitgadget@gmail.com","subject":"[PATCH v2 1/4] get_main_worktree(): allow it to be called in the Git directory","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-22T22:35:04Z","receivedAt":"2020-02-22T22:35:16Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"From: Hariom Verma \n\nWhen called in the Git directory of a non-bare repository, this function\nwould not return the directory of the main worktree, but of the Git\ndirectory instead.\n\nThe reason: when the Git directory is the current working directory, the\nabsolute path of the common directory will be reported with a trailing\n`/.git/.`, which the code of `get_main_worktree()` does not handle\ncorrectly.\n\nLet's fix this.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Hariom Verma \n---\n worktree.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/worktree.c b/worktree.c\nindex 5b4793caa34..7c8cd213171 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -51,6 +51,7 @@ static struct worktree *get_main_worktree(void)\n \tstruct strbuf worktree_path = STRBUF_INIT;\n \n \tstrbuf_add_absolute_path(&worktree_path, get_git_common_dir());\n+\tstrbuf_strip_suffix(&worktree_path, \"/.\");\n \tif (!strbuf_strip_suffix(&worktree_path, \"/.git\"))\n \t\tstrbuf_strip_suffix(&worktree_path, \"/.\");\n \n-- \ngitgitgadget\n\n"},{"id":"392339","messageId":"61e5f75a6f9a8579271870f6b8b95021055a96ad.1582410908.git.gitgitgadget@gmail.com","threadId":"52799","inReplyTo":"pull.535.v2.git.1582410908.gitgitgadget@gmail.com","subject":"[PATCH v2 4/4] receive.denyCurrentBranch: respect all worktrees","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-22T22:35:07Z","receivedAt":"2020-02-22T22:35:19Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"From: Hariom Verma \n\nThe receive.denyCurrentBranch config option controls what happens if\nyou push to a branch that is checked out into a non-bare repository.\nBy default, it rejects it. It can be disabled via `ignore` or `warn`.\nAnother yet trickier option is `updateInstead`.\n\nHowever, this setting was forgotten when the git worktree command was\nintroduced: only the main worktree's current branch is respected.\n\nWith this change, all worktrees are respected.\n\nThat change also leads to revealing another bug,\ni.e. `receive.denyCurrentBranch = true` was ignored when pushing into a\nnon-bare repository's unborn current branch using ref namespaces. As\n`is_ref_checked_out()` returns 0 which means `receive-pack` does not get\ninto conditional statement to switch `deny_current_branch` accordingly\n(ignore, warn, refuse, unconfigured, updateInstead).\n\nreceive.denyCurrentBranch uses the function `refs_resolve_ref_unsafe()`\n(called via `resolve_refdup()`) to resolve the symbolic ref HEAD, but\nthat function fails when HEAD does not point at a valid commit.\nAs we replace the call to `refs_resolve_ref_unsafe()` with\n`find_shared_symref()`, which has no problem finding the worktree for a\ngiven branch even if it is unborn yet, this bug is fixed at the same\ntime: receive.denyCurrentBranch now also handles worktrees with unborn\nbranches as intended even while using ref namespaces.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Hariom Verma \n---\n builtin/receive-pack.c | 37 +++++++++++++++++---------------\n t/t5509-fetch-push-namespaces.sh | 2 +-\n t/t5516-fetch-push.sh | 11 ++++++++++\n 3 files changed, 32 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 411e0b4d999..b5ca3123b78 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -27,6 +27,7 @@\n #include \"object-store.h\"\n #include \"protocol.h\"\n #include \"commit-reach.h\"\n+#include \"worktree.h\"\n \n static const char * const receive_pack_usage[] = {\n \tN_(\"git receive-pack \"),\n@@ -816,16 +817,6 @@ static int run_update_hook(struct command *cmd)\n \treturn finish_command(&proc);\n }\n \n-static int is_ref_checked_out(const char *ref)\n-{\n-\tif (is_bare_repository())\n-\t\treturn 0;\n-\n-\tif (!head_name)\n-\t\treturn 0;\n-\treturn !strcmp(head_name, ref);\n-}\n-\n static char *refuse_unconfigured_deny_msg =\n \tN_(\"By default, updating the current branch in a non-bare repository\\n\"\n \t \"is denied, because it will make the index and work tree inconsistent\\n\"\n@@ -997,16 +988,27 @@ static const char *push_to_checkout(unsigned char *hash,\n \t\treturn NULL;\n }\n \n-static const char *update_worktree(unsigned char *sha1)\n+static const char *update_worktree(unsigned char *sha1, const struct worktree *worktree)\n {\n-\tconst char *retval;\n-\tconst char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : \"..\";\n+\tconst char *retval, *work_tree, *git_dir = NULL;\n \tstruct argv_array env = ARGV_ARRAY_INIT;\n \n+\tif (worktree && worktree->path)\n+\t\twork_tree = worktree->path;\n+\telse if (git_work_tree_cfg)\n+\t\twork_tree = git_work_tree_cfg;\n+\telse\n+\t\twork_tree = \"..\";\n+\n \tif (is_bare_repository())\n \t\treturn \"denyCurrentBranch = updateInstead needs a worktree\";\n+\t\n+\tif (worktree)\n+\t\tgit_dir = get_worktree_git_dir(worktree);\n+\tif (!git_dir)\n+\t\tgit_dir = get_git_dir();\n \n-\targv_array_pushf(&env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n+\targv_array_pushf(&env, \"GIT_DIR=%s\", absolute_path(git_dir));\n \n \tif (!find_hook(push_to_checkout_hook))\n \t\tretval = push_to_deploy(sha1, &env, work_tree);\n@@ -1026,6 +1028,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \tstruct object_id *old_oid = &cmd->old_oid;\n \tstruct object_id *new_oid = &cmd->new_oid;\n \tint do_update_worktree = 0;\n+\tconst struct worktree *worktree = is_bare_repository() ? NULL : find_shared_symref(\"HEAD\", name);\n \n \t/* only refs/... are allowed */\n \tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n@@ -1037,7 +1040,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \tfree(namespaced_name);\n \tnamespaced_name = strbuf_detach(&namespaced_name_buf, NULL);\n \n-\tif (is_ref_checked_out(namespaced_name)) {\n+\tif (worktree) {\n \t\tswitch (deny_current_branch) {\n \t\tcase DENY_IGNORE:\n \t\t\tbreak;\n@@ -1069,7 +1072,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t\treturn \"deletion prohibited\";\n \t\t}\n \n-\t\tif (head_name && !strcmp(namespaced_name, head_name)) {\n+\t\tif (worktree || (head_name && !strcmp(namespaced_name, head_name))) {\n \t\t\tswitch (deny_delete_current) {\n \t\t\tcase DENY_IGNORE:\n \t\t\t\tbreak;\n@@ -1118,7 +1121,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t}\n \n \tif (do_update_worktree) {\n-\t\tret = update_worktree(new_oid->hash);\n+\t\tret = update_worktree(new_oid->hash, find_shared_symref(\"HEAD\", name));\n \t\tif (ret)\n \t\t\treturn ret;\n \t}\ndiff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh\nindex c89483fdba2..6270fb7b576 100755\n--- a/t/t5509-fetch-push-namespaces.sh\n+++ b/t/t5509-fetch-push-namespaces.sh\n@@ -152,7 +152,7 @@ test_expect_success 'clone chooses correct HEAD (v2)' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'denyCurrentBranch and unborn branch with ref namespace' '\n+test_expect_success 'denyCurrentBranch and unborn branch with ref namespace' '\n \tcd original &&\n \tgit init unborn &&\n \tgit remote add unborn-namespaced \"ext::git --namespace=namespace %s unborn\" &&\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex c81ca360ac4..49982b0fd90 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1712,4 +1712,15 @@ test_expect_success 'updateInstead with push-to-checkout hook' '\n \t)\n '\n \n+test_expect_success 'denyCurrentBranch and worktrees' '\n+\tgit worktree add new-wt &&\n+\tgit clone . cloned &&\n+\ttest_commit -C cloned first &&\n+\ttest_config receive.denyCurrentBranch refuse &&\n+\ttest_must_fail git -C cloned push origin HEAD:new-wt &&\n+\ttest_config receive.denyCurrentBranch updateInstead &&\n+\tgit -C cloned push origin HEAD:new-wt &&\n+\ttest_must_fail git -C cloned push --delete origin new-wt\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"392340","messageId":"CA+CkUQ8SrmvFJF_Wn-E-49W2Gi8p0qxVr99SgTWyFfA0t6iJaA@mail.gmail.com","threadId":"52799","inReplyTo":"xmqqo8tym6sr.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/3] t5509: initialized `pushee` as bare repository","fromName":"Hariom verma","fromEmail":"hariom18599@gmail.com","sentAt":"2020-02-22T22:54:23Z","receivedAt":"2020-02-22T22:56:12Z","isPatch":true,"sender":{"key":"hariom18599@gmail.com","avatar":"https://avatars.githubusercontent.com/u/37576387?v=4"},"body":"On Mon, Feb 17, 2020 at 5:19 AM Junio C Hamano wrote:\n> What I suggested was somewhat different, though.\n>\n> 1) get_main_worktree() fix you have as [1/3] in the current round.\n>\n> 2) fix `.denyCurrentBranch` for unborn branches in the primary\n> worktree, new tests for the cases I outlined in the message you\n> are responding to, and adjusting the test (i.e. what you have\n> as [2/3] in the current round).\n>\n> 3) fix `.denyCurrentBranch` to pay attention to HEAD of not just\n> the primary worktree but of all the worktrees, and add tests.\n\nI doubt that it's possible to solve these 2 issues separately.\nAs dscho said: \"a fix for one bug would be a fix for the other, and\n(probably) vice versa.\"\n\nAs I discussed with dscho, the best possible solution for this\nsituation is to demonstrate the bug and fix it in succeeding commit.\n\nI have sent this v2[1] for this patch.\n\nThanks,\nHariom\n\n[1]: https://lore.kernel.org/git/pull.535.v2.git.1582410908.gitgitgadget@gmail.com/\n"},{"id":"392343","messageId":"xmqqk14dsuj8.fsf@gitster-ct.c.googlers.com","threadId":"52799","inReplyTo":"b3e573d44a99a828e710f06b723942107189afeb.1582410908.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/4] bug: denyCurrentBranch and unborn branch with ref namespace","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-23T06:10:51Z","receivedAt":"2020-02-23T06:11:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Hariom Verma via GitGitGadget\" writes:\n\n> +test_expect_failure 'denyCurrentBranch and unborn branch with ref namespace' '\n\nPlease do not chdir around in the test script. The next person who\nadds new test after this test will be surprised that his/her test\ndoes not start at the top-level of the test/trash directory, but in\nthe \"original\" subdirectory.\n\nAnd no, adding \"&& cd ..\" at the end of this &&-cascade is *not* a\nfix---when any of the steps chained with && fails, such a \"we have\nmoved the process to a wrong place, so let's move back with 'cd ..'\"\nwill not get executed.\n\n> +\tcd original &&\n> +\tgit init unborn &&\n> +\tgit remote add unborn-namespaced \"ext::git --namespace=namespace %s unborn\" &&\n> +\ttest_must_fail git push unborn-namespaced HEAD:master &&\n> +\ttest_config -C unborn receive.denyCurrentBranch updateInstead &&\n> +\tgit push unborn-namespaced HEAD:master\n> +'\n\nWhat is often done to fix is to execute what you need to run in a\nsubshell, e.g.\n\n\ttest_expect_success 'demonstration' '\n\t\t(\n\t\t\tcd original &&\n\t\t\tgit init unborn &&\n\t\t\t...\n\t\t\ttest_must_fail git push ... &&\n\t\t\tgit -C unborn config ... &&\n\t\t\tgit push ...\n\t\t)\n\t'\n\nThe use of \"git config\" instead of \"test_config\" in the above\nillustration is deliberate---the latter does not work and should not\nbe used inside a subshell.\n\n> +\n> test_done\n"},{"id":"392344","messageId":"xmqqftf1su6n.fsf@gitster-ct.c.googlers.com","threadId":"52799","inReplyTo":"61e5f75a6f9a8579271870f6b8b95021055a96ad.1582410908.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 4/4] receive.denyCurrentBranch: respect all worktrees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-23T06:18:24Z","receivedAt":"2020-02-23T06:18:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Hariom Verma via GitGitGadget\" writes:\n\n> diff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh\n> index c89483fdba2..6270fb7b576 100755\n> --- a/t/t5509-fetch-push-namespaces.sh\n> +++ b/t/t5509-fetch-push-namespaces.sh\n> @@ -152,7 +152,7 @@ test_expect_success 'clone chooses correct HEAD (v2)' '\n> \ttest_cmp expect actual\n> '\n> \n> -test_expect_failure 'denyCurrentBranch and unborn branch with ref namespace' '\n> +test_expect_success 'denyCurrentBranch and unborn branch with ref namespace' '\n> \tcd original &&\n> \tgit init unborn &&\n> \tgit remote add unborn-namespaced \"ext::git --namespace=namespace %s unborn\" &&\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index c81ca360ac4..49982b0fd90 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -1712,4 +1712,15 @@ test_expect_success 'updateInstead with push-to-checkout hook' '\n> \t)\n> '\n> \n> +test_expect_success 'denyCurrentBranch and worktrees' '\n> +\tgit worktree add new-wt &&\n> +\tgit clone . cloned &&\n> +\ttest_commit -C cloned first &&\n> +\ttest_config receive.denyCurrentBranch refuse &&\n> +\ttest_must_fail git -C cloned push origin HEAD:new-wt &&\n> +\ttest_config receive.denyCurrentBranch updateInstead &&\n> +\tgit -C cloned push origin HEAD:new-wt &&\n> +\ttest_must_fail git -C cloned push --delete origin new-wt\n> +'\n> +\n> test_done\n\nThis adds one new test and also flips a test that was added in a\nseparate step that expected a failure to expect success, which looks\na bit strange.\n\nFor a series this small, having a test that demonstrates that the\nupdated code works as expected together with the fix to the code in\na single patch is easier to manage. After applying a single\ntest+fix patch, you can easily apply the same patch except for the\ntest part in reverse on top, if you need to see in what way the code\nwithout the change breaks by running the test.\n\nOn a truly large fix, sometimes it may make sense to add a failing\ntest and nothing else and then a separate step that changes the code\nand flips the expectation of the test from failure->success, but I\nthink a change this size is easier to handle without such an artificial\nsplit.\n\nThanks.\n"},{"id":"392345","messageId":"xmqqblppstx1.fsf@gitster-ct.c.googlers.com","threadId":"52799","inReplyTo":"5c749e044a3846d7757b198f29d984d181636556.1582410908.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/4] t5509: initialized `pushee` as bare repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-23T06:24:10Z","receivedAt":"2020-02-23T06:24:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Hariom Verma via GitGitGadget\" writes:\n\n> From: Hariom Verma \n> Subject: Re: [PATCH v2 2/4] t5509: initialized `pushee` as bare repository\n\ns/initialized/initialize/ at least, perhaps.\n\nSubject: [PATCH v2 2/4] t5509: use a bare repository for test push target\n\nmay be easier to understand, though. Then the first paragraph of\nthe body of the proposed message, which gives an excellent\ndescription of how the current tests rely on a bug that we plan to\nfix in a later step of the series, explains why we do not want to\npush into a non-bare repository.\n\n> `receive.denyCurrentBranch` currently has a bug where it allows pushing\n> into non-bare repository using namespaces as long as it does not have any\n> commits. This would cause t5509 to fail once that bug is fixed because it\n> pushes into an unborn current branch.\n\nAnd then you give a good description why not just it is safe, but it\nmakes more sense. Very well explained.\n\n> In t5509, no operations are performed inside `pushee`, as it is only a\n> target for `git push` and `git ls-remote` calls. Therefore it does not\n> need to have a worktree. So, it is safe to change `pushee` to a bare\n> repository.\n\n>\n> Helped-by: Johannes Schindelin \n> Signed-off-by: Hariom Verma \n> ---\n> t/t5509-fetch-push-namespaces.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh\n> index 75cbfcc392c..e3975bd21de 100755\n> --- a/t/t5509-fetch-push-namespaces.sh\n> +++ b/t/t5509-fetch-push-namespaces.sh\n> @@ -20,7 +20,7 @@ test_expect_success setup '\n> \t) &&\n> \tcommit0=$(cd original && git rev-parse HEAD^) &&\n> \tcommit1=$(cd original && git rev-parse HEAD) &&\n> -\tgit init pushee &&\n> +\tgit init --bare pushee &&\n> \tgit init puller\n> '\n"},{"id":"392360","messageId":"8718facbc951614f19407afa6ca8d6110507483d.1582484231.git.gitgitgadget@gmail.com","threadId":"52799","inReplyTo":"pull.535.v3.git.1582484231.gitgitgadget@gmail.com","subject":"[PATCH v3 1/3] get_main_worktree(): allow it to be called in the Git directory","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-23T18:57:08Z","receivedAt":"2020-02-23T18:57:16Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"From: Hariom Verma \n\nWhen called in the Git directory of a non-bare repository, this function\nwould not return the directory of the main worktree, but of the Git\ndirectory instead.\n\nThe reason: when the Git directory is the current working directory, the\nabsolute path of the common directory will be reported with a trailing\n`/.git/.`, which the code of `get_main_worktree()` does not handle\ncorrectly.\n\nLet's fix this.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Hariom Verma \n---\n worktree.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/worktree.c b/worktree.c\nindex 5b4793caa34..7c8cd213171 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -51,6 +51,7 @@ static struct worktree *get_main_worktree(void)\n \tstruct strbuf worktree_path = STRBUF_INIT;\n \n \tstrbuf_add_absolute_path(&worktree_path, get_git_common_dir());\n+\tstrbuf_strip_suffix(&worktree_path, \"/.\");\n \tif (!strbuf_strip_suffix(&worktree_path, \"/.git\"))\n \t\tstrbuf_strip_suffix(&worktree_path, \"/.\");\n \n-- \ngitgitgadget\n\n"},{"id":"392361","messageId":"pull.535.v3.git.1582484231.gitgitgadget@gmail.com","threadId":"52799","inReplyTo":"pull.535.v2.git.1582410908.gitgitgadget@gmail.com","subject":"[PATCH v3 0/3] [GSoC] receive.denyCurrentBranch: respect all worktrees","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-23T18:57:07Z","receivedAt":"2020-02-23T18:57:16Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"The receive.denyCurrentBranch config option controls what happens if you\npush to a branch that is checkout into a non-bare repository. By default, it\nrejects it. It can be disabled via ignore or warn. Another yet trickier\noption is updateInstead.\n\nWhen receive.denyCurrentBranch is set to updateInstead, a push that tries to\nupdate the branch that is currently checked out is accepted only when the\nindex and the working tree exactly matches the currently checked out commit,\nin which case the index and the working tree are updated to match the pushed\ncommit. Otherwise, the push is refused.\n\nHowever, this setting was forgotten when the git worktree command was\nintroduced: only the main worktree's current branch is respected. [ fixes:\n#331 ]\n\nIncidently, this change also fixes another bug i.e. \nreceive.denyCurrentBranch = true was ignored when pushing into a non-bare\nrepository using ref namespaces.\n\nThanks, @dscho for helping me out.\n\nRegards, Hariom\n\nHariom Verma (3):\n get_main_worktree(): allow it to be called in the Git directory\n t5509: use a bare repository for test push target\n receive.denyCurrentBranch: respect all worktrees\n\n builtin/receive-pack.c | 37 +++++++++++++++++---------------\n t/t5509-fetch-push-namespaces.sh | 13 ++++++++++-\n t/t5516-fetch-push.sh | 11 ++++++++++\n worktree.c | 1 +\n 4 files changed, 44 insertions(+), 18 deletions(-)\n\n\nbase-commit: 232378479ee6c66206d47a9be175e3a39682aea6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-535%2Fharry-hov%2Fdeny-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-535/harry-hov/deny-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/535\n\nRange-diff vs v2:\n\n 1: 8718facbc95 = 1: 8718facbc95 get_main_worktree(): allow it to be called in the Git directory\n 2: 5c749e044a3 ! 2: ae749310f06 t5509: initialized `pushee` as bare repository\n @@ -1,6 +1,6 @@\n Author: Hariom Verma \n \n - t5509: initialized `pushee` as bare repository\n + t5509: use a bare repository for test push target\n \n `receive.denyCurrentBranch` currently has a bug where it allows pushing\n into non-bare repository using namespaces as long as it does not have any\n 3: b3e573d44a9 < -: ----------- bug: denyCurrentBranch and unborn branch with ref namespace\n 4: 61e5f75a6f9 ! 3: d21a590d6c2 receive.denyCurrentBranch: respect all worktrees\n @@ -134,11 +134,18 @@\n \ttest_cmp expect actual\n '\n \n --test_expect_failure 'denyCurrentBranch and unborn branch with ref namespace' '\n +test_expect_success 'denyCurrentBranch and unborn branch with ref namespace' '\n - \tcd original &&\n - \tgit init unborn &&\n - \tgit remote add unborn-namespaced \"ext::git --namespace=namespace %s unborn\" &&\n ++\t(\n ++\t\tcd original &&\n ++\t\tgit init unborn &&\n ++\t\tgit remote add unborn-namespaced \"ext::git --namespace=namespace %s unborn\" &&\n ++\t\ttest_must_fail git push unborn-namespaced HEAD:master &&\n ++\t\tgit -C unborn config receive.denyCurrentBranch updateInstead &&\n ++\t\tgit push unborn-namespaced HEAD:master\n ++\t)\n ++'\n ++\n + test_done\n \n diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n --- a/t/t5516-fetch-push.sh\n\n-- \ngitgitgadget\n"},{"id":"392362","messageId":"ae749310f067c43429741987cd9f47c1ae4ceb3f.1582484231.git.gitgitgadget@gmail.com","threadId":"52799","inReplyTo":"pull.535.v3.git.1582484231.gitgitgadget@gmail.com","subject":"[PATCH v3 2/3] t5509: use a bare repository for test push target","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-23T18:57:09Z","receivedAt":"2020-02-23T18:57:17Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"From: Hariom Verma \n\n`receive.denyCurrentBranch` currently has a bug where it allows pushing\ninto non-bare repository using namespaces as long as it does not have any\ncommits. This would cause t5509 to fail once that bug is fixed because it\npushes into an unborn current branch.\n\nIn t5509, no operations are performed inside `pushee`, as it is only a\ntarget for `git push` and `git ls-remote` calls. Therefore it does not\nneed to have a worktree. So, it is safe to change `pushee` to a bare\nrepository.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Hariom Verma \n---\n t/t5509-fetch-push-namespaces.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh\nindex 75cbfcc392c..e3975bd21de 100755\n--- a/t/t5509-fetch-push-namespaces.sh\n+++ b/t/t5509-fetch-push-namespaces.sh\n@@ -20,7 +20,7 @@ test_expect_success setup '\n \t) &&\n \tcommit0=$(cd original && git rev-parse HEAD^) &&\n \tcommit1=$(cd original && git rev-parse HEAD) &&\n-\tgit init pushee &&\n+\tgit init --bare pushee &&\n \tgit init puller\n '\n \n-- \ngitgitgadget\n\n"},{"id":"392363","messageId":"d21a590d6c23f231c54b731b737c363b83660f79.1582484231.git.gitgitgadget@gmail.com","threadId":"52799","inReplyTo":"pull.535.v3.git.1582484231.gitgitgadget@gmail.com","subject":"[PATCH v3 3/3] receive.denyCurrentBranch: respect all worktrees","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-23T18:57:10Z","receivedAt":"2020-02-23T18:57:17Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"From: Hariom Verma \n\nThe receive.denyCurrentBranch config option controls what happens if\nyou push to a branch that is checked out into a non-bare repository.\nBy default, it rejects it. It can be disabled via `ignore` or `warn`.\nAnother yet trickier option is `updateInstead`.\n\nHowever, this setting was forgotten when the git worktree command was\nintroduced: only the main worktree's current branch is respected.\n\nWith this change, all worktrees are respected.\n\nThat change also leads to revealing another bug,\ni.e. `receive.denyCurrentBranch = true` was ignored when pushing into a\nnon-bare repository's unborn current branch using ref namespaces. As\n`is_ref_checked_out()` returns 0 which means `receive-pack` does not get\ninto conditional statement to switch `deny_current_branch` accordingly\n(ignore, warn, refuse, unconfigured, updateInstead).\n\nreceive.denyCurrentBranch uses the function `refs_resolve_ref_unsafe()`\n(called via `resolve_refdup()`) to resolve the symbolic ref HEAD, but\nthat function fails when HEAD does not point at a valid commit.\nAs we replace the call to `refs_resolve_ref_unsafe()` with\n`find_shared_symref()`, which has no problem finding the worktree for a\ngiven branch even if it is unborn yet, this bug is fixed at the same\ntime: receive.denyCurrentBranch now also handles worktrees with unborn\nbranches as intended even while using ref namespaces.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Hariom Verma \n---\n builtin/receive-pack.c | 37 +++++++++++++++++---------------\n t/t5509-fetch-push-namespaces.sh | 11 ++++++++++\n t/t5516-fetch-push.sh | 11 ++++++++++\n 3 files changed, 42 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 411e0b4d999..b5ca3123b78 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -27,6 +27,7 @@\n #include \"object-store.h\"\n #include \"protocol.h\"\n #include \"commit-reach.h\"\n+#include \"worktree.h\"\n \n static const char * const receive_pack_usage[] = {\n \tN_(\"git receive-pack \"),\n@@ -816,16 +817,6 @@ static int run_update_hook(struct command *cmd)\n \treturn finish_command(&proc);\n }\n \n-static int is_ref_checked_out(const char *ref)\n-{\n-\tif (is_bare_repository())\n-\t\treturn 0;\n-\n-\tif (!head_name)\n-\t\treturn 0;\n-\treturn !strcmp(head_name, ref);\n-}\n-\n static char *refuse_unconfigured_deny_msg =\n \tN_(\"By default, updating the current branch in a non-bare repository\\n\"\n \t \"is denied, because it will make the index and work tree inconsistent\\n\"\n@@ -997,16 +988,27 @@ static const char *push_to_checkout(unsigned char *hash,\n \t\treturn NULL;\n }\n \n-static const char *update_worktree(unsigned char *sha1)\n+static const char *update_worktree(unsigned char *sha1, const struct worktree *worktree)\n {\n-\tconst char *retval;\n-\tconst char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : \"..\";\n+\tconst char *retval, *work_tree, *git_dir = NULL;\n \tstruct argv_array env = ARGV_ARRAY_INIT;\n \n+\tif (worktree && worktree->path)\n+\t\twork_tree = worktree->path;\n+\telse if (git_work_tree_cfg)\n+\t\twork_tree = git_work_tree_cfg;\n+\telse\n+\t\twork_tree = \"..\";\n+\n \tif (is_bare_repository())\n \t\treturn \"denyCurrentBranch = updateInstead needs a worktree\";\n+\t\n+\tif (worktree)\n+\t\tgit_dir = get_worktree_git_dir(worktree);\n+\tif (!git_dir)\n+\t\tgit_dir = get_git_dir();\n \n-\targv_array_pushf(&env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n+\targv_array_pushf(&env, \"GIT_DIR=%s\", absolute_path(git_dir));\n \n \tif (!find_hook(push_to_checkout_hook))\n \t\tretval = push_to_deploy(sha1, &env, work_tree);\n@@ -1026,6 +1028,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \tstruct object_id *old_oid = &cmd->old_oid;\n \tstruct object_id *new_oid = &cmd->new_oid;\n \tint do_update_worktree = 0;\n+\tconst struct worktree *worktree = is_bare_repository() ? NULL : find_shared_symref(\"HEAD\", name);\n \n \t/* only refs/... are allowed */\n \tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n@@ -1037,7 +1040,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \tfree(namespaced_name);\n \tnamespaced_name = strbuf_detach(&namespaced_name_buf, NULL);\n \n-\tif (is_ref_checked_out(namespaced_name)) {\n+\tif (worktree) {\n \t\tswitch (deny_current_branch) {\n \t\tcase DENY_IGNORE:\n \t\t\tbreak;\n@@ -1069,7 +1072,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t\treturn \"deletion prohibited\";\n \t\t}\n \n-\t\tif (head_name && !strcmp(namespaced_name, head_name)) {\n+\t\tif (worktree || (head_name && !strcmp(namespaced_name, head_name))) {\n \t\t\tswitch (deny_delete_current) {\n \t\t\tcase DENY_IGNORE:\n \t\t\t\tbreak;\n@@ -1118,7 +1121,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t}\n \n \tif (do_update_worktree) {\n-\t\tret = update_worktree(new_oid->hash);\n+\t\tret = update_worktree(new_oid->hash, find_shared_symref(\"HEAD\", name));\n \t\tif (ret)\n \t\t\treturn ret;\n \t}\ndiff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh\nindex e3975bd21de..a67f792adf4 100755\n--- a/t/t5509-fetch-push-namespaces.sh\n+++ b/t/t5509-fetch-push-namespaces.sh\n@@ -152,4 +152,15 @@ test_expect_success 'clone chooses correct HEAD (v2)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'denyCurrentBranch and unborn branch with ref namespace' '\n+\t(\n+\t\tcd original &&\n+\t\tgit init unborn &&\n+\t\tgit remote add unborn-namespaced \"ext::git --namespace=namespace %s unborn\" &&\n+\t\ttest_must_fail git push unborn-namespaced HEAD:master &&\n+\t\tgit -C unborn config receive.denyCurrentBranch updateInstead &&\n+\t\tgit push unborn-namespaced HEAD:master\n+\t)\n+'\n+\n test_done\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex c81ca360ac4..49982b0fd90 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1712,4 +1712,15 @@ test_expect_success 'updateInstead with push-to-checkout hook' '\n \t)\n '\n \n+test_expect_success 'denyCurrentBranch and worktrees' '\n+\tgit worktree add new-wt &&\n+\tgit clone . cloned &&\n+\ttest_commit -C cloned first &&\n+\ttest_config receive.denyCurrentBranch refuse &&\n+\ttest_must_fail git -C cloned push origin HEAD:new-wt &&\n+\ttest_config receive.denyCurrentBranch updateInstead &&\n+\tgit -C cloned push origin HEAD:new-wt &&\n+\ttest_must_fail git -C cloned push --delete origin new-wt\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"392369","messageId":"CAPig+cTh-uu-obh9aeDOV9ptbVwRmkujgucbu9ei1Qa3qSNG_A@mail.gmail.com","threadId":"52799","inReplyTo":"8718facbc951614f19407afa6ca8d6110507483d.1582484231.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/3] get_main_worktree(): allow it to be called in the Git directory","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-02-24T01:42:32Z","receivedAt":"2020-02-24T01:42:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 23, 2020 at 1:57 PM Hariom Verma via GitGitGadget\n wrote:\n> get_main_worktree(): allow it to be called in the Git directory\n\nThis title is a bit too generic; it fails to explain what this patch\nis really fixing. Perhaps:\n\n get_main_worktree: correctly normalize worktree path when in .git dir\n\nor something.\n\n> When called in the Git directory of a non-bare repository, this function\n> would not return the directory of the main worktree, but of the Git\n> directory instead.\n\n\"Git directory\" is imprecise. As a reader, I can't tell if this means\nthe main worktree into which the project is checked out or the `.git`\ndirectory itself. Please write it instead as \"`.git` directory\".\n\n> The reason: when the Git directory is the current working directory, the\n> absolute path of the common directory will be reported with a trailing\n> `/.git/.`, which the code of `get_main_worktree()` does not handle\n> correctly.\n>\n> Signed-off-by: Hariom Verma \n> ---\n> diff --git a/worktree.c b/worktree.c\n> @@ -51,6 +51,7 @@ static struct worktree *get_main_worktree(void)\n> strbuf_add_absolute_path(&worktree_path, get_git_common_dir());\n> + strbuf_strip_suffix(&worktree_path, \"/.\");\n> if (!strbuf_strip_suffix(&worktree_path, \"/.git\"))\n> strbuf_strip_suffix(&worktree_path, \"/.\");\n\nThis change makes the code unnecessarily confusing and effectively\nturns the final line into dead code. I would much rather see the three\ncases spelled out explicitly, perhaps like this:\n\n if (!strbuf_strip_suffix(&worktree_path, \"/.git/.\") && /* in .git dir */\n !strbuf_strip_suffix(&worktree_path, \"/.git/\")) /* in worktree */\n strbuf_strip_suffix(&worktree_path, \"/.\"); /* in bare repo */\n\nAlso, please add a test to ensure that this behavior doesn't regress\nin the future. You can probably test it via the \"git worktree list\"\ncommand, so perhaps add the test to t/t2402-worktree-list.sh.\n"},{"id":"392402","messageId":"CA+CkUQ8ZsxesE=d+DQ+67SEHPJXdHjbSKhWeVifPeKBymqy8pw@mail.gmail.com","threadId":"52799","inReplyTo":"CAPig+cTh-uu-obh9aeDOV9ptbVwRmkujgucbu9ei1Qa3qSNG_A@mail.gmail.com","subject":"Re: [PATCH v3 1/3] get_main_worktree(): allow it to be called in the Git directory","fromName":"Hariom verma","fromEmail":"hariom18599@gmail.com","sentAt":"2020-02-24T11:09:08Z","receivedAt":"2020-02-24T11:09:23Z","isPatch":true,"sender":{"key":"hariom18599@gmail.com","avatar":"https://avatars.githubusercontent.com/u/37576387?v=4"},"body":"Hi Eric,\n\nOn Mon, Feb 24, 2020 at 7:12 AM Eric Sunshine wrote:\n>\n> This title is a bit too generic; it fails to explain what this patch\n> is really fixing. Perhaps:\n>\n> get_main_worktree: correctly normalize worktree path when in .git dir\n>\n> or something.\n>\n> \"Git directory\" is imprecise. As a reader, I can't tell if this means\n> the main worktree into which the project is checked out or the `.git`\n> directory itself. Please write it instead as \"`.git` directory\".\n> [...]\n> This change makes the code unnecessarily confusing and effectively\n> turns the final line into dead code. I would much rather see the three\n> cases spelled out explicitly, perhaps like this:\n>\n> if (!strbuf_strip_suffix(&worktree_path, \"/.git/.\") && /* in .git dir */\n> !strbuf_strip_suffix(&worktree_path, \"/.git/\")) /* in worktree */\n> strbuf_strip_suffix(&worktree_path, \"/.\"); /* in bare repo */\n\nI'll implement these comments in the next revision for sure.\n\n> Also, please add a test to ensure that this behavior doesn't regress\n> in the future. You can probably test it via the \"git worktree list\"\n> command, so perhaps add the test to t/t2402-worktree-list.sh.\n\nThere already exists tests in \"t/t2402-worktree-list.sh\" which lists and\nverifies all worktrees. Does this make sense to write a new test that\nalso does kinda the same thing?\n\nThanks,\nHariom\n"},{"id":"392412","messageId":"CAPig+cRhEhYVtSc=EXFSE9VoVwmRQ2yToqdm=c=4cLVWE1=Yvw@mail.gmail.com","threadId":"52799","inReplyTo":"CA+CkUQ8ZsxesE=d+DQ+67SEHPJXdHjbSKhWeVifPeKBymqy8pw@mail.gmail.com","subject":"Re: [PATCH v3 1/3] get_main_worktree(): allow it to be called in the Git directory","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-02-24T17:00:43Z","receivedAt":"2020-02-24T17:00:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Feb 24, 2020 at 6:09 AM Hariom verma wrote:\n> On Mon, Feb 24, 2020 at 7:12 AM Eric Sunshine wrote:\n> > if (!strbuf_strip_suffix(&worktree_path, \"/.git/.\") && /* in .git dir */\n> > !strbuf_strip_suffix(&worktree_path, \"/.git/\")) /* in worktree */\n> > strbuf_strip_suffix(&worktree_path, \"/.\"); /* in bare repo */\n> >\n> > Also, please add a test to ensure that this behavior doesn't regress\n> > in the future. You can probably test it via the \"git worktree list\"\n> > command, so perhaps add the test to t/t2402-worktree-list.sh.\n>\n> There already exists tests in \"t/t2402-worktree-list.sh\" which lists and\n> verifies all worktrees. Does this make sense to write a new test that\n> also does kinda the same thing?\n\nThe change this patch is making is to correctly strip the suffix\n\"/.git/.\" from the main worktree's path since that suffix was not\ngetting stripped correctly by the existing code. We want a test that\nverifies that the \"/.git/.\" suffix is indeed being stripped once this\nchange is applied. If there is an existing test which already checks\nthe output of \"git worktree list\" when invoked from within the .git\ndirectory, then that test should suffice, but then I would have\nexpected that you would have had to tweak the existing test to make it\nsucceed after this change. If there is no such test which verifies\nthat \"/.git/.\" is being stripped, then this patch should add one. \"git\nworktree list\" would be a possible way to implement such a test.\n"},{"id":"392427","messageId":"nycvar.QRO.7.76.6.2002241942120.46@tvgsbejvaqbjf.bet","threadId":"52799","inReplyTo":"CA+CkUQ8ZsxesE=d+DQ+67SEHPJXdHjbSKhWeVifPeKBymqy8pw@mail.gmail.com","subject":"Re: [PATCH v3 1/3] get_main_worktree(): allow it to be called in the Git directory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-02-24T18:58:38Z","receivedAt":"2020-02-24T18:58:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 24 Feb 2020, Hariom verma wrote:\n\n> On Mon, Feb 24, 2020 at 7:12 AM Eric Sunshine wrote:\n> >\n> > This title is a bit too generic; it fails to explain what this patch\n> > is really fixing. Perhaps:\n> >\n> > get_main_worktree: correctly normalize worktree path when in .git dir\n> >\n> > or something.\n> >\n> > \"Git directory\" is imprecise. As a reader, I can't tell if this means\n> > the main worktree into which the project is checked out or the `.git`\n> > directory itself. Please write it instead as \"`.git` directory\".\n> > [...]\n> > This change makes the code unnecessarily confusing and effectively\n> > turns the final line into dead code. I would much rather see the three\n> > cases spelled out explicitly, perhaps like this:\n> >\n> > if (!strbuf_strip_suffix(&worktree_path, \"/.git/.\") && /* in .git dir */\n> > !strbuf_strip_suffix(&worktree_path, \"/.git/\")) /* in worktree */\n> > strbuf_strip_suffix(&worktree_path, \"/.\"); /* in bare repo */\n\nI would be really cautious about that.\n\nTo me, the originally proposed change says: strip `/.`, if any. Then,\nstrip `/.git`, and if successful, strip another `/.`, if any.\n\nThat reads pretty fine to me. It makes sense.\n\nAbove-mentioned proposal, however, puts quite a few twists into my brain,\nas is a \"if neither X nor Y then Z\", and I find the code comments outright\nconfusing.\n\n> I'll implement these comments in the next revision for sure.\n\nI'd like to suggest taking a step back and reflecting whether _you_ like\nthe suggested version better. It is just a suggestion, after all, and if\nit was up to me, I would argue against it.\n\n> > Also, please add a test to ensure that this behavior doesn't regress\n> > in the future. You can probably test it via the \"git worktree list\"\n> > command, so perhaps add the test to t/t2402-worktree-list.sh.\n>\n> There already exists tests in \"t/t2402-worktree-list.sh\" which lists and\n> verifies all worktrees. Does this make sense to write a new test that\n> also does kinda the same thing?\n\nThe scenario in which we found the buggy behavior involved calling\n`find_shared_symref()`. I imagine that we could use `git branch -D` inside\nthe `.git` directory for the new regression test.\n\nBut yes, in my testing, `git worktree list` and `git -C .git worktree\nlist` do show a different top-level directory (the latter shows an\nincorrect one). Such a test case would find a splendid home in t2402.\n\nCiao,\nDscho\n"},{"id":"392430","messageId":"xmqqtv3fpzmm.fsf@gitster-ct.c.googlers.com","threadId":"52799","inReplyTo":"CA+CkUQ8ZsxesE=d+DQ+67SEHPJXdHjbSKhWeVifPeKBymqy8pw@mail.gmail.com","subject":"Re: [PATCH v3 1/3] get_main_worktree(): allow it to be called in the Git directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-24T19:13:37Z","receivedAt":"2020-02-24T19:13:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hariom verma writes:\n\n> Hi Eric,\n>\n> On Mon, Feb 24, 2020 at 7:12 AM Eric Sunshine wrote:\n>>\n>> This title is a bit too generic; it fails to explain what this patch\n>> is really fixing. Perhaps:\n>>\n>> get_main_worktree: correctly normalize worktree path when in .git dir\n>>\n>> or something.\n>>\n>> \"Git directory\" is imprecise. As a reader, I can't tell if this means\n>> the main worktree into which the project is checked out or the `.git`\n>> directory itself. Please write it instead as \"`.git` directory\".\n>> [...]\n>> This change makes the code unnecessarily confusing and effectively\n>> turns the final line into dead code. I would much rather see the three\n>> cases spelled out explicitly, perhaps like this:\n>>\n>> if (!strbuf_strip_suffix(&worktree_path, \"/.git/.\") && /* in .git dir */\n>> !strbuf_strip_suffix(&worktree_path, \"/.git/\")) /* in worktree */\n>> strbuf_strip_suffix(&worktree_path, \"/.\"); /* in bare repo */\n>\n> I'll implement these comments in the next revision for sure.\n>\n>> Also, please add a test to ensure that this behavior doesn't regress\n>> in the future. You can probably test it via the \"git worktree list\"\n>> command, so perhaps add the test to t/t2402-worktree-list.sh.\n>\n> There already exists tests in \"t/t2402-worktree-list.sh\" which lists and\n> verifies all worktrees. Does this make sense to write a new test that\n> also does kinda the same thing?\n\nI'd read Eric's suggestion as \"please make sure we have a test to\nensure...\". If there already are tests that protects the behaviour\nwe care about here, there is no need to duplicate it.\n\nThanks for working on this topic.\n"},{"id":"392451","messageId":"CAPig+cR9wwcbnuFmVuoDr6OSq29ZCSt6Kr5KtG3HPsDgM5mGSA@mail.gmail.com","threadId":"52799","inReplyTo":"nycvar.QRO.7.76.6.2002241942120.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3 1/3] get_main_worktree(): allow it to be called in the Git directory","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-02-24T22:27:14Z","receivedAt":"2020-02-24T22:27:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Feb 24, 2020 at 1:58 PM Johannes Schindelin\n wrote:\n> > On Mon, Feb 24, 2020 at 7:12 AM Eric Sunshine wrote:\n> > > This change makes the code unnecessarily confusing and effectively\n> > > turns the final line into dead code. I would much rather see the three\n> > > cases spelled out explicitly, perhaps like this:\n> > >\n> > > if (!strbuf_strip_suffix(&worktree_path, \"/.git/.\") && /* in .git dir */\n> > > !strbuf_strip_suffix(&worktree_path, \"/.git/\")) /* in worktree */\n> > > strbuf_strip_suffix(&worktree_path, \"/.\"); /* in bare repo */\n>\n> I would be really cautious about that.\n>\n> To me, the originally proposed change says: strip `/.`, if any. Then,\n> strip `/.git`, and if successful, strip another `/.`, if any.\n\nThat's not at all what the original said, which is reproduced here:\n\n if (!strbuf_strip_suffix(&worktree_path, \"/.git\"))\n strbuf_strip_suffix(&worktree_path, \"/.\");\n\nIt says \"try stripping '/.git'; if that fails, try stripping '/.'\".\nThat is, it recognizes and handles two distinct cases: (1) the path to\nthe .git directory of a non-bare repository, which always ends with\n\"/.git\", and (2) the path to a bare git repository, which always ends\nwith \"/.\". So, the original code wasn't doing any sort of incremental\nstripping of suffixes; it was just handling two known distinct cases.\n\nPerhaps you missed the '!' in the conditional?\n\n> That reads pretty fine to me. It makes sense.\n\nTo me, it doesn't make sense to update the code as done by the patch\nsince that just muddies the issue by making it seem as if\nget_git_common_dir() is indeterminately tacking on various suffixes\nrather than giving us deterministic results.\n\n> Above-mentioned proposal, however, puts quite a few twists into my brain,\n> as is a \"if neither X nor Y then Z\", and I find the code comments outright\n> confusing.\n\nIt's just three distinct cases my proposed code is handling; there are\nno twists.\n\n> The scenario in which we found the buggy behavior involved calling\n> `find_shared_symref()`. I imagine that we could use `git branch -D` inside\n> the `.git` directory for the new regression test.\n>\n> But yes, in my testing, `git worktree list` and `git -C .git worktree\n> list` do show a different top-level directory (the latter shows an\n> incorrect one). Such a test case would find a splendid home in t2402.\n\nI don't have strong feelings about how it is tested, but would like to\nsee some sort of test proving that it works as expected following this\nchange.\n"},{"id":"392621","messageId":"nycvar.QRO.7.76.6.2002271658250.46@tvgsbejvaqbjf.bet","threadId":"52799","inReplyTo":"CAPig+cR9wwcbnuFmVuoDr6OSq29ZCSt6Kr5KtG3HPsDgM5mGSA@mail.gmail.com","subject":"Re: [PATCH v3 1/3] get_main_worktree(): allow it to be called in the Git directory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-02-27T15:58:51Z","receivedAt":"2020-02-27T15:59:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Mon, 24 Feb 2020, Eric Sunshine wrote:\n\n> On Mon, Feb 24, 2020 at 1:58 PM Johannes Schindelin\n> wrote:\n> > > On Mon, Feb 24, 2020 at 7:12 AM Eric Sunshine wrote:\n> > > > This change makes the code unnecessarily confusing and effectively\n> > > > turns the final line into dead code. I would much rather see the three\n> > > > cases spelled out explicitly, perhaps like this:\n> > > >\n> > > > if (!strbuf_strip_suffix(&worktree_path, \"/.git/.\") && /* in .git dir */\n> > > > !strbuf_strip_suffix(&worktree_path, \"/.git/\")) /* in worktree */\n> > > > strbuf_strip_suffix(&worktree_path, \"/.\"); /* in bare repo */\n> >\n> > I would be really cautious about that.\n> >\n> > To me, the originally proposed change says: strip `/.`, if any. Then,\n> > strip `/.git`, and if successful, strip another `/.`, if any.\n>\n> That's not at all what the original said, which is reproduced here:\n>\n> if (!strbuf_strip_suffix(&worktree_path, \"/.git\"))\n> strbuf_strip_suffix(&worktree_path, \"/.\");\n>\n> It says \"try stripping '/.git'; if that fails, try stripping '/.'\".\n> That is, it recognizes and handles two distinct cases: (1) the path to\n> the .git directory of a non-bare repository, which always ends with\n> \"/.git\", and (2) the path to a bare git repository, which always ends\n> with \"/.\". So, the original code wasn't doing any sort of incremental\n> stripping of suffixes; it was just handling two known distinct cases.\n>\n> Perhaps you missed the '!' in the conditional?\n\nI totally did. Sorry!\n\nCiao,\nDscho\n"}]}