{"thread":{"id":"49577","subject":"receive.denyCurrentBranch=updateInstead updates working tree even when receive.denyNonFastForwards rejects push","startedAt":"2018-10-16T18:55:04Z","lastAt":"2018-10-22T09:32:49Z","messageCount":6,"participants":["Rajesh Madamanchi","Junio C Hamano","Eric Sunshine","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"360657","messageId":"CANahLXm7O6scvLdaL6bm14d5oGM5Zp+S-rpQDo554ssagRFNoQ@mail.gmail.com","threadId":"49577","inReplyTo":null,"subject":"receive.denyCurrentBranch=updateInstead updates working tree even when receive.denyNonFastForwards rejects push","fromName":"Rajesh Madamanchi","fromEmail":"rmadamanchi@gmail.com","sentAt":"2018-10-16T18:54:48Z","receivedAt":"2018-10-16T18:55:04Z","isPatch":false,"sender":{"key":"rmadamanchi@gmail.com","avatar":null},"body":"Hi, I am looking to report the below behavior when seems incorrect to\nme when receive.denyCurrentBranch is set to updateInstead and\nreceive.denyNonFastForwards is set to true. Below are the steps to\nreproduce the scenario. Please excuse my ignorance if I'm missing\nsomething fundamental.\n\nStep 1 - Setup remote repository (remote host):\ngit config --global receive.denyCurrentBranch updateInstead\ngit config --global receive.denyNonFastForwards true\nmkdir /tmp/hello\ncd /tmp/hello\ngit init\necho hello > hello.txt\ngit add . && git commit -m \"hello.txt\"\n\nStep 2 - Create 2 Clones (local host):\ngit clone ssh://REMOTEIP/tmp/hello /tmp/hello1\ngit clone ssh://REMOTEIP/tmp/hello /tmp/hello2\n\nStep 3 - Push a commit from Clone 1\ncd /tmp/hello1\necho hello1 > hello1.txt\ngit add . && git commit -m \"hello1.txt\"\ngit push\n--> at this point server working tree contains hello1.txt which is expected\n\nStep 4: Try to force push a commit from Clone 2\ncd /tmp/hello2\necho hello2 > hello2.txt\ngit add . && git commit -m \"hello2.txt\"\ngit push\n--> Remote rejects with message that remote contains work i do not have locally\ngit push --force\n--> Remote rejects again with error: denying non-fast-forward\nrefs/heads/master (you should pull first)\n--> At this point, since the push is rejected, I expect the servers\nworking tree to not contain any rejected changes. BUT the servers\nworking tree got updated to delete hello1.txt and create hello2.txt.\nPush rejected but not really.\n\nI also noticed the same behavior (incorrect) when the update hook\nrejects changes on the server (but not the pre-receive hook).\n\nThank you\n"},{"id":"360885","messageId":"xmqqsh124wqb.fsf@gitster-ct.c.googlers.com","threadId":"49577","inReplyTo":"CANahLXm7O6scvLdaL6bm14d5oGM5Zp+S-rpQDo554ssagRFNoQ@mail.gmail.com","subject":"Re: receive.denyCurrentBranch=updateInstead updates working tree even when receive.denyNonFastForwards rejects push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-19T00:16:12Z","receivedAt":"2018-10-19T00:16:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rajesh Madamanchi <rmadamanchi@gmail.com> writes:\n\n> Hi, I am looking to report the below behavior when seems incorrect to\n> me when receive.denyCurrentBranch is set to updateInstead and\n> receive.denyNonFastForwards is set to true.\n\nIt seems that we took a lazy but incorrect route while adding the\nDENY_UPDATE_INSTEAD hack to builtin/receive-pack.c::update() and new\ncode went to a wrong place in a series of checks.  Everythng else in\nthe same switch() statement either refuses or just decides to let\nlater step to update without taking actual action, so that later\nchecks such as \"the new tip commit must have been transferred\", \"the\nnew tip must be a fast-forward of the old tip\", etc., but the one\nfor DENY_UPDATE_INSTEAD immediately calls update_worktree() there.\nIt should be changed to decide to later call the function when\neverybody else in the series of checks agrees that it is OK to let\nthis push accepted, and then the actual call is made somewhere near\nwhere we call run_update_hook(), probably after the hook says it is\nOK to update.\n\n"},{"id":"360897","messageId":"xmqq4ldi38u7.fsf@gitster-ct.c.googlers.com","threadId":"49577","inReplyTo":"xmqqsh124wqb.fsf@gitster-ct.c.googlers.com","subject":"Re: receive.denyCurrentBranch=updateInstead updates working tree even when receive.denyNonFastForwards rejects push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-19T03:37:36Z","receivedAt":"2018-10-19T03:37:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Rajesh Madamanchi <rmadamanchi@gmail.com> writes:\n>\n>> Hi, I am looking to report the below behavior when seems incorrect to\n>> me when receive.denyCurrentBranch is set to updateInstead and\n>> receive.denyNonFastForwards is set to true.\n>\n> It seems that we took a lazy but incorrect route while adding the\n> DENY_UPDATE_INSTEAD hack to builtin/receive-pack.c::update() and new\n> code went to a wrong place in a series of checks.  Everythng else in\n> the same switch() statement either refuses or just decides to let\n> later step to update without taking actual action, so that later\n> checks such as \"the new tip commit must have been transferred\", \"the\n> new tip must be a fast-forward of the old tip\", etc., but the one\n> for DENY_UPDATE_INSTEAD immediately calls update_worktree() there.\n> It should be changed to decide to later call the function when\n> everybody else in the series of checks agrees that it is OK to let\n> this push accepted, and then the actual call is made somewhere near\n> where we call run_update_hook(), probably after the hook says it is\n> OK to update.\n\nSo here is a lunch-time hack that is not even compile tested but\nillustrates the idea outlined above.  We'd need to add tests to\nprotect the fix from future breakages (if the fix is correct, that\nis, which I do not quite know---but it feels right ;-).\n\n builtin/receive-pack.c | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 7f089be11e..4bf316dbba 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1050,9 +1050,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t\t\trefuse_unconfigured_deny();\n \t\t\treturn \"branch is currently checked out\";\n \t\tcase DENY_UPDATE_INSTEAD:\n-\t\t\tret = update_worktree(new_oid->hash);\n-\t\t\tif (ret)\n-\t\t\t\treturn ret;\n+\t\t\t/* pass -- let other checks intervene first */\n \t\t\tbreak;\n \t\t}\n \t}\n@@ -1117,6 +1115,12 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\treturn \"hook declined\";\n \t}\n \n+\tif (deny_current_branch == DENY_UPDATE_INSTEAD) {\n+\t\tret = update_worktree(new_oid->hash);\n+\t\tif (ret)\n+\t\t\treturn ret;\n+\t}\n+\n \tif (is_null_oid(new_oid)) {\n \t\tstruct strbuf err = STRBUF_INIT;\n \t\tif (!parse_object(the_repository, old_oid)) {\n"},{"id":"360899","messageId":"xmqqpnw61qkg.fsf_-_@gitster-ct.c.googlers.com","threadId":"49577","inReplyTo":"xmqq4ldi38u7.fsf@gitster-ct.c.googlers.com","subject":"[PATCH] receive: denyCurrentBranch=updateinstead should not blindly update","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-19T04:57:35Z","receivedAt":"2018-10-19T04:57:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The handling of receive.denyCurrentBranch=updateInstead was added to\na switch statement that handles other values of the variable, but\nall the other case arms only checked a condition to reject the\nattempted push, or let later logic in the same function to still\nintervene, so that a push that does not fast-forward (which is\nchecked after the switch statement in question) is still rejected.\n\nBut the handling of updateInstead incorrectly took immediate effect,\nwithout giving other checks a chance to intervene.\n\nInstead of calling update_worktree() that causes the side effect\nimmediately, just note the fact that we will need to call the\nfunciton later, and first give other checks chance to reject the\nrequest.  After the update-hook gets a chance to reject the push\n(which happens as the last step in a series of checks), call\nupdate_worktree() when we earlier detected the need to.\n\nReported-by: Rajesh Madamanchi\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/receive-pack.c | 12 +++++++++---\n t/t5516-fetch-push.sh  |  8 +++++++-\n 2 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 95740f4f0e..79ee320948 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1026,6 +1026,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \tconst char *ret;\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 \n \t/* only refs/... are allowed */\n \tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n@@ -1051,9 +1052,8 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\t\t\trefuse_unconfigured_deny();\n \t\t\treturn \"branch is currently checked out\";\n \t\tcase DENY_UPDATE_INSTEAD:\n-\t\t\tret = update_worktree(new_oid->hash);\n-\t\t\tif (ret)\n-\t\t\t\treturn ret;\n+\t\t\t/* pass -- let other checks intervene first */\n+\t\t\tdo_update_worktree = 1;\n \t\t\tbreak;\n \t\t}\n \t}\n@@ -1118,6 +1118,12 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\treturn \"hook declined\";\n \t}\n \n+\tif (do_update_worktree) {\n+\t\tret = update_worktree(new_oid->hash);\n+\t\tif (ret)\n+\t\t\treturn ret;\n+\t}\n+\n \tif (is_null_oid(new_oid)) {\n \t\tstruct strbuf err = STRBUF_INIT;\n \t\tif (!parse_object(the_repository, old_oid)) {\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 7a8f56db53..7316365a24 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1577,7 +1577,13 @@ test_expect_success 'receive.denyCurrentBranch = updateInstead' '\n \t\ttest $(git -C .. rev-parse master) = $(git rev-parse HEAD) &&\n \t\tgit diff --quiet &&\n \t\tgit diff --cached --quiet\n-\t)\n+\t) &&\n+\n+\t# (6) updateInstead intervened by fast-forward check\n+\ttest_must_fail git push void master^:master &&\n+\ttest $(git -C void rev-parse HEAD) = $(git rev-parse master) &&\n+\tgit -C void diff --quiet &&\n+\tgit -C void diff --cached --quiet\n '\n \n test_expect_success 'updateInstead with push-to-checkout hook' '\n-- \n2.19.1-450-ga4b8ab5363\n\n"},{"id":"360901","messageId":"CAPig+cT9vKDr2uYVS0XdM=KxN5JY4yrONni5iZnVL5GdE1taLw@mail.gmail.com","threadId":"49577","inReplyTo":"xmqqpnw61qkg.fsf_-_@gitster-ct.c.googlers.com","subject":"Re: [PATCH] receive: denyCurrentBranch=updateinstead should not blindly update","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-10-19T05:54:11Z","receivedAt":"2018-10-19T05:54:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Oct 19, 2018 at 12:57 AM Junio C Hamano <gitster@pobox.com> wrote:\n> The handling of receive.denyCurrentBranch=updateInstead was added to\n> a switch statement that handles other values of the variable, but\n> all the other case arms only checked a condition to reject the\n> attempted push, or let later logic in the same function to still\n> intervene, so that a push that does not fast-forward (which is\n> checked after the switch statement in question) is still rejected.\n>\n> But the handling of updateInstead incorrectly took immediate effect,\n> without giving other checks a chance to intervene.\n>\n> Instead of calling update_worktree() that causes the side effect\n> immediately, just note the fact that we will need to call the\n> funciton later, and first give other checks chance to reject the\n\ns/funciton/function\ns/chance/a &/\n\n> request.  After the update-hook gets a chance to reject the push\n> (which happens as the last step in a series of checks), call\n> update_worktree() when we earlier detected the need to.\n>\n> Reported-by: Rajesh Madamanchi\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n"},{"id":"361148","messageId":"nycvar.QRO.7.76.6.1810221132190.4546@tvgsbejvaqbjf.bet","threadId":"49577","inReplyTo":"xmqqpnw61qkg.fsf_-_@gitster-ct.c.googlers.com","subject":"Re: [PATCH] receive: denyCurrentBranch=updateinstead should not blindly update","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-10-22T09:32:41Z","receivedAt":"2018-10-22T09:32:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 19 Oct 2018, Junio C Hamano wrote:\n\n> The handling of receive.denyCurrentBranch=updateInstead was added to\n> a switch statement that handles other values of the variable, but\n> all the other case arms only checked a condition to reject the\n> attempted push, or let later logic in the same function to still\n> intervene, so that a push that does not fast-forward (which is\n> checked after the switch statement in question) is still rejected.\n> \n> But the handling of updateInstead incorrectly took immediate effect,\n> without giving other checks a chance to intervene.\n> \n> Instead of calling update_worktree() that causes the side effect\n> immediately, just note the fact that we will need to call the\n> funciton later, and first give other checks chance to reject the\n> request.  After the update-hook gets a chance to reject the push\n> (which happens as the last step in a series of checks), call\n> update_worktree() when we earlier detected the need to.\n\nNicely explained, and the patch looks good to me, too.\n\nThanks,\nDscho\n\n> \n> Reported-by: Rajesh Madamanchi\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  builtin/receive-pack.c | 12 +++++++++---\n>  t/t5516-fetch-push.sh  |  8 +++++++-\n>  2 files changed, 16 insertions(+), 4 deletions(-)\n> \n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 95740f4f0e..79ee320948 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1026,6 +1026,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n>  \tconst char *ret;\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>  \n>  \t/* only refs/... are allowed */\n>  \tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n> @@ -1051,9 +1052,8 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n>  \t\t\t\trefuse_unconfigured_deny();\n>  \t\t\treturn \"branch is currently checked out\";\n>  \t\tcase DENY_UPDATE_INSTEAD:\n> -\t\t\tret = update_worktree(new_oid->hash);\n> -\t\t\tif (ret)\n> -\t\t\t\treturn ret;\n> +\t\t\t/* pass -- let other checks intervene first */\n> +\t\t\tdo_update_worktree = 1;\n>  \t\t\tbreak;\n>  \t\t}\n>  \t}\n> @@ -1118,6 +1118,12 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n>  \t\treturn \"hook declined\";\n>  \t}\n>  \n> +\tif (do_update_worktree) {\n> +\t\tret = update_worktree(new_oid->hash);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\t}\n> +\n>  \tif (is_null_oid(new_oid)) {\n>  \t\tstruct strbuf err = STRBUF_INIT;\n>  \t\tif (!parse_object(the_repository, old_oid)) {\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index 7a8f56db53..7316365a24 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -1577,7 +1577,13 @@ test_expect_success 'receive.denyCurrentBranch = updateInstead' '\n>  \t\ttest $(git -C .. rev-parse master) = $(git rev-parse HEAD) &&\n>  \t\tgit diff --quiet &&\n>  \t\tgit diff --cached --quiet\n> -\t)\n> +\t) &&\n> +\n> +\t# (6) updateInstead intervened by fast-forward check\n> +\ttest_must_fail git push void master^:master &&\n> +\ttest $(git -C void rev-parse HEAD) = $(git rev-parse master) &&\n> +\tgit -C void diff --quiet &&\n> +\tgit -C void diff --cached --quiet\n>  '\n>  \n>  test_expect_success 'updateInstead with push-to-checkout hook' '\n> -- \n> 2.19.1-450-ga4b8ab5363\n> \n> \n"}]}