Re: receive.denyCurrentBranch=updateInstead updates working tree even when receive.denyNonFastForwards rejects push
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 19, 2018, 03:37 UTC
- Message-ID
- <xmqq4ldi38u7.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <xmqqsh124wqb.fsf@gitster-ct.c.googlers.com>
Junio C Hamano <gitster@pobox.com> writes:
Show 19 quoted lines
> 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)) {