receive.denyCurrentBranch=updateInstead updates working tree even when receive.denyNonFastForwards rejects push

receive.denyCurrentBranch=updateInstead updates working tree even when receive.denyNonFastForwards rejects push

[Rajesh Madamanchi]

Hi, I am looking to report the below behavior when seems incorrect to
me when receive.denyCurrentBranch is set to updateInstead and
receive.denyNonFastForwards is set to true. Below are the steps to
reproduce the scenario. Please excuse my ignorance if I'm missing
something fundamental.

Step 1 - Setup remote repository (remote host):
git config --global receive.denyCurrentBranch updateInstead
git config --global receive.denyNonFastForwards true
mkdir /tmp/hello
cd /tmp/hello
git init
echo hello > hello.txt
git add . && git commit -m "hello.txt"

Step 2 - Create 2 Clones (local host):
git clone ssh://REMOTEIP/tmp/hello /tmp/hello1
git clone ssh://REMOTEIP/tmp/hello /tmp/hello2

Step 3 - Push a commit from Clone 1
cd /tmp/hello1
echo hello1 > hello1.txt
git add . && git commit -m "hello1.txt"
git push
--> at this point server working tree contains hello1.txt which is expected

Step 4: Try to force push a commit from Clone 2
cd /tmp/hello2
echo hello2 > hello2.txt
git add . && git commit -m "hello2.txt"
git push
--> Remote rejects with message that remote contains work i do not have locally
git push --force
--> Remote rejects again with error: denying non-fast-forward
refs/heads/master (you should pull first)
--> At this point, since the push is rejected, I expect the servers
working tree to not contain any rejected changes. BUT the servers
working tree got updated to delete hello1.txt and create hello2.txt.
Push rejected but not really.

I also noticed the same behavior (incorrect) when the update hook
rejects changes on the server (but not the pre-receive hook).

Thank you

Re: receive.denyCurrentBranch=updateInstead updates working tree even when receive.denyNonFastForwards rejects push

[Junio C Hamano]

Rajesh Madamanchi <rmadamanchi@gmail.com> writes:

> Hi, I am looking to report the below behavior when seems incorrect to
> me when receive.denyCurrentBranch is set to updateInstead and
> receive.denyNonFastForwards is set to true.

It seems that we took a lazy but incorrect route while adding the
DENY_UPDATE_INSTEAD hack to builtin/receive-pack.c::update() and new
code went to a wrong place in a series of checks.  Everythng else in
the same switch() statement either refuses or just decides to let
later step to update without taking actual action, so that later
checks such as "the new tip commit must have been transferred", "the
new tip must be a fast-forward of the old tip", etc., but the one
for DENY_UPDATE_INSTEAD immediately calls update_worktree() there.
It should be changed to decide to later call the function when
everybody else in the series of checks agrees that it is OK to let
this push accepted, and then the actual call is made somewhere near
where we call run_update_hook(), probably after the hook says it is
OK to update.

Re: receive.denyCurrentBranch=updateInstead updates working tree even when receive.denyNonFastForwards rejects push

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Rajesh Madamanchi <rmadamanchi@gmail.com> writes:
>
>> Hi, I am looking to report the below behavior when seems incorrect to
>> me when receive.denyCurrentBranch is set to updateInstead and
>> receive.denyNonFastForwards is set to true.
>
> It seems that we took a lazy but incorrect route while adding the
> DENY_UPDATE_INSTEAD hack to builtin/receive-pack.c::update() and new
> code went to a wrong place in a series of checks.  Everythng else in
> the same switch() statement either refuses or just decides to let
> later step to update without taking actual action, so that later
> checks such as "the new tip commit must have been transferred", "the
> new tip must be a fast-forward of the old tip", etc., but the one
> for DENY_UPDATE_INSTEAD immediately calls update_worktree() there.
> It should be changed to decide to later call the function when
> everybody else in the series of checks agrees that it is OK to let
> this push accepted, and then the actual call is made somewhere near
> where we call run_update_hook(), probably after the hook says it is
> OK to update.

So here is a lunch-time hack that is not even compile tested but
illustrates the idea outlined above.  We'd need to add tests to
protect the fix from future breakages (if the fix is correct, that
is, which I do not quite know---but it feels right ;-).

 builtin/receive-pack.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 7f089be11e..4bf316dbba 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1050,9 +1050,7 @@ 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 */
 			break;
 		}
 	}
@@ -1117,6 +1115,12 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 		return "hook declined";
 	}
 
+	if (deny_current_branch == DENY_UPDATE_INSTEAD) {
+		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)) {

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

[Junio C Hamano]

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

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

[Eric Sunshine]

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>

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

[Johannes Schindelin]

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