Re: [PATCH 2/3] send-pack: mark failure of atomic push properly

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 25, 2020, 15:15 UTC
Message-ID: <xmqq5zesxy6y.fsf@gitster.c.googlers.com>
In-Reply-To: <20200325143608.45141-3-zhiyou.jx@alibaba-inc.com>

Jiang Xin <worldhello.net@gmail.com> writes:

> When pusing with SSH or other smart protocol, references are validated
> by function `check_to_send_update()` beforce they are sent in commands

beforce -> before

Show 16 quoted lines

> to `send_pack()` of "receve-pack".  For atomic push, if a reference is
> rejected after the validation, only references pushed by user should be
> marked as failure, instead of report failure on all remote references.
>
> Commit v2.22.0-1-g3bca1e7f9f (transport-helper: enforce atomic in
> push_refs_with_push, 2019-07-11) wanted to fix report issue of HTTP
> protocol, but marked all remote references failure for atomic push.
>
> Revert part of that commit and add additional status for function
> `atomic_push_failure()`.  The additional status for it except the
> "REF_STATUS_EXPECTING_REPORT" status are:
>
> - REF_STATUS_NONE : Not marked as "REF_STATUS_EXPECTING_REPORT" yet.
> - REF_STATUS_OK   : Assume OK for dryrun or status_report is disabled.
>
> This commit break test case in t5541, and will fix in other commit.

Presumably the fix in 3/3 is too big to be included in here? Let's see how it goes, as such a strategy sometimes helps understand the change and sometimes does not.

Show 13 quoted lines

> diff --git a/send-pack.c b/send-pack.c
> index 0407841ae8..ff016c468c 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -334,6 +334,8 @@ static int atomic_push_failure(struct send_pack_args *args,
>  			continue;
>  
>  		switch (ref->status) {
> +		case REF_STATUS_NONE:
> +		case REF_STATUS_OK:
>  		case REF_STATUS_EXPECTING_REPORT:
>  			ref->status = REF_STATUS_ATOMIC_PUSH_FAILED;
>  			continue;

This is fed the whole *remote_refs list, iterates over them but we only come here if the ref is actually pushed (i.e. peer_ref is set, or we are doing a mirror push), so it does sound like the right place to do this.