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

[Jiang Xin <worldhello.net@gmail.com>]

Junio C Hamano <gitster@pobox.com> writes:

> > 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.
> 

To make it clear, I refactor the Han Xin's patch, quote and add comments
as follows (changes on whitespace are ignored):


>>         /*
>>          * NEEDSWORK: why does delete-refs have to be so specific to
>>          * send-pack machinery that set_ref_status_for_push() cannot
>>          * set this bit for us???
>>          */
>>         for (ref = remote_refs; ref; ref = ref->next)
>>             if (ref->deletion && !allow_deleting_refs)
>>                 ref->status = REF_STATUS_REJECT_NODELETE;
>>     
>>    -    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);
>>    -
>>         /*
>>          * Clear the status for each ref and see if we need to send
>>          * the pack data.
>>          */
>>         for (ref = remote_refs; ref; ref = ref->next) {
>>             switch (check_to_send_update(ref, args)) {
>>             case 0: /* no error */
>>                 break;
>>             case CHECK_REF_STATUS_REJECTED:
>>                 /*
>>                  * When we know the server would reject a ref update if
>>                  * we were to send it and we're trying to send the refs
>>                  * atomically, abort the whole operation.
>>                  */
>>                 if (use_atomic) {
>>                     strbuf_release(&req_buf);
>>                     strbuf_release(&cap_buf);
>>                     reject_atomic_push(remote_refs, args->send_mirror);
>>                     error("atomic push failed for ref %s. status: %d\n",
>>                           ref->name, ref->status);
>>                     return args->porcelain ? 0 : -1;
>>                 }
>>                 /* else fallthrough */
>>             default:
>>                 continue;
>>             }
>>             if (!ref->deletion)
>>                 need_pack_data = 1;
>>     
>>             if (args->dry_run || !status_report)
>>                 ref->status = REF_STATUS_OK;
>>             else
>>                 ref->status = REF_STATUS_EXPECTING_REPORT;
>>         }
>>     
>>    +    if (!args->dry_run)
>>    +        advertise_shallow_grafts_buf(&req_buf);
>>    +
>>    +
>>         /*
>>          * Finally, tell the other end!
>>          */
>>    +    if (!args->dry_run && push_cert_nonce)
>>    +        cmds_sent = generate_push_cert(&req_buf, remote_refs, args,
>>    +                           cap_buf.buf, push_cert_nonce);

Moving `generate_push_cert()` here, will: 
1. Increase the perforcemance a little bit for failed atomic push.
2. Make it clear that the commands will be sent only once.
   For GPG-signed push, commands will be sent via `generate_push_cert()`,
   and for non-GPG-signed push, commands will be sent using the following code.
3. For GPG-signed push, won't run the following loop.

>>    +    else if (!args->dry_run)
>>             for (ref = remote_refs; ref; ref = ref->next) {
>>                 char *old_hex, *new_hex;
>>     
>>    -            if (args->dry_run || push_cert_nonce)
>>    -                continue;
>>    -
>>                 if (check_to_send_update(ref, args) < 0)
>>                     continue;

In the original "Finally, tell the other end" block, the function
`check_to_send_update()` is also called for non-PGP-signed push.
The 'ref->status' changed by the "Clear the status" block won't 
make any difference for the return value of the function
`check_to_send_update()`. Refs even with status REF_STATUS_OK and
REF_STATUS_EXPECTING_REPORT will be sent to the server side.

>>     
>>                 old_hex = oid_to_hex(&ref->old_oid);
>>                 new_hex = oid_to_hex(&ref->new_oid);
>>                 if (!cmds_sent) {
>>                     packet_buf_write(&req_buf,
>>                              "%s %s %s%c%s",
>>                              old_hex, new_hex, ref->name, 0,
>>                              cap_buf.buf);
>>                     cmds_sent = 1;
>>                 } else {
>>                     packet_buf_write(&req_buf, "%s %s %s",
>>                              old_hex, new_hex, ref->name);
>>                 }
>>             }


> 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.