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

Junio C Hamano [off-list ref] writes:

Rajesh Madamanchi [off-list ref] writes:

quoted

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