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@xxxxxxxxx>
> ---
> 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
>
>
