Re: [PATCH] receive: denyCurrentBranch=updateinstead should not blindly update

> 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(-)
> 
> 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
> 
>