| Message ID | xmqqpnw61qkg.fsf_-_@gitster-ct.c.googlers.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | receive: denyCurrentBranch=updateinstead should not blindly update | expand |
On Fri, Oct 19, 2018 at 12:57 AM Junio C Hamano <gitster@pobox.com> wrote: > The handling of receive.denyCurrentBranch=updateInstead was added to > a switch statement that handles other values of the variable, but > all the other case arms only checked a condition to reject the > attempted push, or let later logic in the same function to still > intervene, so that a push that does not fast-forward (which is > checked after the switch statement in question) is still rejected. > > But the handling of updateInstead incorrectly took immediate effect, > without giving other checks a chance to intervene. > > Instead of calling update_worktree() that causes the side effect > immediately, just note the fact that we will need to call the > funciton later, and first give other checks chance to reject the s/funciton/function s/chance/a &/ > request. After the update-hook gets a chance to reject the push > (which happens as the last step in a series of checks), call > update_worktree() when we earlier detected the need to. > > Reported-by: Rajesh Madamanchi > Signed-off-by: Junio C Hamano <gitster@pobox.com>
Hi Junio, On Fri, 19 Oct 2018, Junio C Hamano wrote: > The handling of receive.denyCurrentBranch=updateInstead was added to > a switch statement that handles other values of the variable, but > all the other case arms only checked a condition to reject the > attempted push, or let later logic in the same function to still > intervene, so that a push that does not fast-forward (which is > checked after the switch statement in question) is still rejected. > > But the handling of updateInstead incorrectly took immediate effect, > without giving other checks a chance to intervene. > > Instead of calling update_worktree() that causes the side effect > immediately, just note the fact that we will need to call the > funciton later, and first give other checks chance to reject the > request. After the update-hook gets a chance to reject the push > (which happens as the last step in a series of checks), call > update_worktree() when we earlier detected the need to. Nicely explained, and the patch looks good to me, too. Thanks, Dscho > > Reported-by: Rajesh Madamanchi > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > builtin/receive-pack.c | 12 +++++++++--- > t/t5516-fetch-push.sh | 8 +++++++- > 2 files changed, 16 insertions(+), 4 deletions(-) > > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c > index 95740f4f0e..79ee320948 100644 > --- a/builtin/receive-pack.c > +++ b/builtin/receive-pack.c > @@ -1026,6 +1026,7 @@ static const char *update(struct command *cmd, struct shallow_info *si) > const char *ret; > struct object_id *old_oid = &cmd->old_oid; > struct object_id *new_oid = &cmd->new_oid; > + int do_update_worktree = 0; > > /* only refs/... are allowed */ > if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) { > @@ -1051,9 +1052,8 @@ static const char *update(struct command *cmd, struct shallow_info *si) > refuse_unconfigured_deny(); > return "branch is currently checked out"; > case DENY_UPDATE_INSTEAD: > - ret = update_worktree(new_oid->hash); > - if (ret) > - return ret; > + /* pass -- let other checks intervene first */ > + do_update_worktree = 1; > break; > } > } > @@ -1118,6 +1118,12 @@ static const char *update(struct command *cmd, struct shallow_info *si) > return "hook declined"; > } > > + if (do_update_worktree) { > + ret = update_worktree(new_oid->hash); > + if (ret) > + return ret; > + } > + > if (is_null_oid(new_oid)) { > struct strbuf err = STRBUF_INIT; > if (!parse_object(the_repository, old_oid)) { > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh > index 7a8f56db53..7316365a24 100755 > --- a/t/t5516-fetch-push.sh > +++ b/t/t5516-fetch-push.sh > @@ -1577,7 +1577,13 @@ test_expect_success 'receive.denyCurrentBranch = updateInstead' ' > test $(git -C .. rev-parse master) = $(git rev-parse HEAD) && > git diff --quiet && > git diff --cached --quiet > - ) > + ) && > + > + # (6) updateInstead intervened by fast-forward check > + test_must_fail git push void master^:master && > + test $(git -C void rev-parse HEAD) = $(git rev-parse master) && > + git -C void diff --quiet && > + git -C void diff --cached --quiet > ' > > test_expect_success 'updateInstead with push-to-checkout hook' ' > -- > 2.19.1-450-ga4b8ab5363 > >
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 95740f4f0e..79ee320948 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1026,6 +1026,7 @@ static const char *update(struct command *cmd, struct shallow_info *si) const char *ret; struct object_id *old_oid = &cmd->old_oid; struct object_id *new_oid = &cmd->new_oid; + int do_update_worktree = 0; /* only refs/... are allowed */ if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) { @@ -1051,9 +1052,8 @@ static const char *update(struct command *cmd, struct shallow_info *si) refuse_unconfigured_deny(); return "branch is currently checked out"; case DENY_UPDATE_INSTEAD: - ret = update_worktree(new_oid->hash); - if (ret) - return ret; + /* pass -- let other checks intervene first */ + do_update_worktree = 1; break; } } @@ -1118,6 +1118,12 @@ static const char *update(struct command *cmd, struct shallow_info *si) return "hook declined"; } + if (do_update_worktree) { + ret = update_worktree(new_oid->hash); + if (ret) + return ret; + } + if (is_null_oid(new_oid)) { struct strbuf err = STRBUF_INIT; if (!parse_object(the_repository, old_oid)) { diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 7a8f56db53..7316365a24 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1577,7 +1577,13 @@ test_expect_success 'receive.denyCurrentBranch = updateInstead' ' test $(git -C .. rev-parse master) = $(git rev-parse HEAD) && git diff --quiet && git diff --cached --quiet - ) + ) && + + # (6) updateInstead intervened by fast-forward check + test_must_fail git push void master^:master && + test $(git -C void rev-parse HEAD) = $(git rev-parse master) && + git -C void diff --quiet && + git -C void diff --cached --quiet ' test_expect_success 'updateInstead with push-to-checkout hook' '
The handling of receive.denyCurrentBranch=updateInstead was added to a switch statement that handles other values of the variable, but all the other case arms only checked a condition to reject the attempted push, or let later logic in the same function to still intervene, so that a push that does not fast-forward (which is checked after the switch statement in question) is still rejected. But the handling of updateInstead incorrectly took immediate effect, without giving other checks a chance to intervene. Instead of calling update_worktree() that causes the side effect immediately, just note the fact that we will need to call the funciton later, and first give other checks chance to reject the request. After the update-hook gets a chance to reject the push (which happens as the last step in a series of checks), call update_worktree() when we earlier detected the need to. Reported-by: Rajesh Madamanchi Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin/receive-pack.c | 12 +++++++++--- t/t5516-fetch-push.sh | 8 +++++++- 2 files changed, 16 insertions(+), 4 deletions(-)