From: 韩欣(炽天) Date: Wed, 16 Sep 2020 17:35:05 GMT Subject: Re: [PATCH 2/2] send-pack: check atomic push before running GPG Message-ID: In-Reply-To: 在 2020/9/16 下午12:37, Junio C Hamano 写道: > Jiang Xin 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. Thank you for your reply. These loops here really confuse me at first. But I found that the main effect of "Clear the status for each ref and see if we need to send the pack data" is to help us do a pre-check on the client side whether the push should be rejected. When the reference should be pushed, whether the status was changed to REF_STATUS_OK or REF_STATUS_EXPECTING_REPORT, it does not seem to affect the result of function generate_push_cert(). check_to_send_update() in generate_push_cert() only filters out references that needn't to be pushed. Just like brian m. carlson said, "that would be a nice change; after all, the user's key may involve a smartcard or a passphrase and avoiding needless hassle for the user would be desirable". It increase the perforcemance a little bit for failed atomic push and make it clear that client side requirements and the other side requirements. If there is something wrong with my understanding, I am very grateful \that you can help me point out the problems.