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

> Jiang Xin <worldhello.net@gmail.com> writes:
>
>>>> -
>>>> -     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.
>> The next block ("Finally, tell the other end!") is what we send
>> commands to "receive-pack", right after some of the status are reset
>> to REF_STATUS_OK or REF_STATUS_EXPECTING_REPORT by this chunk of code.
>> So moving the generate_push_cert() part right before the "Finally,
>> tell the other end!" part LGTM.
> Sorry, I do not follow.  The loop in question is the one before
> "Finally tell the other end".  The loop ends like so:
>
> 	for (ref = remote_refs; ref; ref = ref->next) {
> 		...
> 		if (args->dry_run || !status_report)
> 			ref->status = REF_STATUS_OK;
> 		else
> 			ref->status = REF_STATUS_EXPECTING_REPORT;
> 	}
>
> and the patch moves a call to generate_push_cert() that looks at
> remote_refs _after_ this loop, but generate_push_cert() function
> uses a loop over remote_refs that uses check_to_send_update(), which
> looks at ref->status's value to decide what to do.  Its correct
> operation relies on ref->status NOT updated by the above loop.
>
> The loop prepares the status field so that we can then read and
> record the response against each ref updates from the other side.
>
> The ref->status field is set to EXPECTING_REPORT, later to be
> updated to REF_STATUS_OK or REF_STATUS_REMOTE_REJECT.  We can
> clobber the original value of ref->status at this point only because
> the loop depends on the fact that no check_to_send_update() call
> will happen after the loop (because the ref->status field the
> helper's operation depends on is already reset for the next phase of
> operation).  The patch that moves generate_push_cert() call below
> the loop, whether it is before or after the "Finally tell the other
> end" loop, is therefore fundamentally broken, isn't it?
>
> I do not think it would introduce such breakage if we teach
> generate_push_cert() to pay attention to the atomicity, and that can
> be done without reordering the calls in send_pack() to break the
> control flow.