Re: [PATCH 2/2] send-pack: check atomic push before running GPG

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 15, 2020, 21:02 UTC
Message-ID: <xmqqmu1qzrbo.fsf@gitster.c.googlers.com>
In-Reply-To: <20200915095827.52047-2-hanxin.hx@alibaba-inc.com>

Han Xin <chiyutianyi@gmail.com> writes:

> Atomic push may be rejected, which makes it meanigless to generate push
> cert first. Therefore, the push cert generation was moved after atomic
> check.

The overstatement "may be rejected" is probably a bit misleading the readers and reviewers. REF_STATUS_REJECT_NONFASTFORWARD may be observed by check_to_send_update() but the reason is set in set_ref_status_for_push(), which locally decides not to propose a no-ff ref update to the other side. At this point of the control flow, the other side hasn't have a chance to reject the push, because "we want you to update these refs to these new values" is yet to be sent (it is done after composing the push certificate).

    We may decide not to push (e.g. their ref may not fast forward
    to what we are pushing) at this point in the code.  Checking the
    condition first will save us to ask GPG to sign the push
    certificate, since we will not send it to the other side.

> -	if (!args->dry_run)
> -		advertise_shallow_grafts_buf(&req_buf);

Why should this be moved? It doesn't seem to have anything to do with the push certificate.

Show 8 quoted lines

> -
> -	if (!args->dry_run && push_cert_nonce)
> -		cmds_sent = generate_push_cert(&req_buf, remote_refs, args,
> -					       cap_buf.buf, push_cert_nonce);
> -
>  	/*
>  	 * Clear the status for each ref and see if we need to send
>  	 * the pack data.

This "Clear the status for each ref" worries me.

The generate_push_cert() function RELIES on ref->status and filters out the ref that failed to pass the local check from the generated push certificate. If you let the loop (post context of this hunk) run, ref->status will be updated by it, so the net effect of this patch is that it breaks "non-atomic" case that pushes multiple refs and one of ref fails to pass the local check.

IOW, generate_push_cert() MUST be called before this loop "clears the status for each ref" by assigning to ref->status.

Show 11 quoted lines

> @@ -489,6 +482,13 @@ int send_pack(struct send_pack_args *args,
>  			ref->status = REF_STATUS_EXPECTING_REPORT;
>  	}
>  
> +	if (!args->dry_run)
> +		advertise_shallow_grafts_buf(&req_buf);
> +
> +	if (!args->dry_run && push_cert_nonce)
> +		cmds_sent = generate_push_cert(&req_buf, remote_refs, args,
> +					       cap_buf.buf, push_cert_nonce);
> +

So, this change as-is is probably a bad idea.

I wonder if generate_push_cert() can be told about atomicity of the push, though. There is this loop in the function:

	int update_seen = 0;

	...
	for (ref = remote_refs; ref; ref = ref->next) {
		if (check_to_send_update(ref, args) < 0)
			continue;
		update_seen = 1;
		strbuf_addf(&cert, "%s %s %s\n",
			    oid_to_hex(&ref->old_oid),
			    oid_to_hex(&ref->new_oid),
			    ref->name);
	}
	if (!update_seen)
		goto free_return;

that makes it a no-op without invoking GPG if no update is needed. Perhaps we can extend it to

	int failure_seen = 0;
        int update_seen = 0;

	...
	for (ref = remote_refs; ref; ref = ref->next) {
		switch (check_to_send_update(ref, args)) {
		case CHECK_REF_STATUS_REJECTED:
			failure_seen = 1;
			break;
		case 0:
			update_seen = 1;
			break;
                case REF_STATUS_UPTODATE:
			break; /* OK */
		default:
			BUG("should not happen");
		}
		strbuf_addf(&cert, "%s %s %s\n",
			    oid_to_hex(&ref->old_oid),
			    oid_to_hex(&ref->new_oid),
			    ref->name);
	}
	if (!update_seen || (use_atomic && failure_seen))
		goto free_return;

to make it also a no-op when any local rejection under atomic mode?

Thanks.