Junio C Hamano <gitster@xxxxxxxxx> 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.
