Re: [PATCH 3/3] transport-helper: enforce atomic in push_refs_with_push

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

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

Show 23 quoted lines

> diff --git a/transport-helper.c b/transport-helper.c
> index 20a7185ec4..ab3b52eb14 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -894,6 +894,21 @@ static int push_refs_with_push(struct transport *transport,
>  		case REF_STATUS_REJECT_STALE:
>  		case REF_STATUS_REJECT_ALREADY_EXISTS:
>  			if (atomic) {
> +				/* Mark other refs as failed */
> +				for (ref = remote_refs; ref; ref = ref->next) {
> +					if (!ref->peer_ref && !mirror)
> +						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;
> +					default:
> +						break; /* do nothing */
> +					}
> +				}

OK, so this is more in line with the check done in send_pack() that fails the push before we even send any pack data. I wonder if it is worth considering to move the logic of this loop into a helper function so that this and the other one in 2/3 can call it and stay in sync, something along the lines of

	void reject_push_to_other_refs(struct ref *ref, int mirror_mode)
	{
		for (; ref; ref = ref->next) {
			... one iteration of the above loop ...
		}
	}

Then the above part would become

		case REF_STATUS_REJECT_FOO:
			if (atomic)
				reject_push_to_other_refs(remote_refs, mirror);

and the part modified by 2/3 would also become

	static int atomic_push_failure(...)
	{
		reject_push_to_other_refs(remote_refs, args->send_mirror);
		return error("atomic push failed ...");
	}

I am not sure if it is better to keep 2/3 and 3/3 separate or make them into a single step, but perhaps it is because I am not getting the true reason why you made them separate in the first place.

Thanks.