git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jiang Xin <worldhello.net@gmail.com>
Cc: Han Xin <chiyutianyi@gmail.com>, Git List <git@vger.kernel.org>,
	Han Xin <hanxin.hx@alibaba-inc.com>,
	Jiang Xin <zhiyou.jx@alibaba-inc.com>
Subject: Re: [PATCH 2/2] send-pack: check atomic push before running GPG
Date: Tue, 15 Sep 2020 21:37:47 -0700	[thread overview]
Message-ID: <xmqqeen2xrok.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <CANYiYbHYi70ZcjDTyQ++_+njuZMF=TksPepH+wP+zNmhBABNAg@mail.gmail.com> (Jiang Xin's message of "Wed, 16 Sep 2020 09:53:24 +0800")

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.


  reply	other threads:[~2020-09-16  4:37 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-15  9:58 [PATCH 1/2] t5534: new test case for atomic signed push Han Xin
2020-09-15  9:58 ` [PATCH 2/2] send-pack: check atomic push before running GPG Han Xin
2020-09-15 21:02   ` Junio C Hamano
2020-09-15 21:40     ` Junio C Hamano
2020-09-16  1:53     ` Jiang Xin
2020-09-16  4:37       ` Junio C Hamano [this message]
2020-09-16 11:49         ` Jiang Xin
2020-09-16 23:44           ` Junio C Hamano
2020-09-18  4:50             ` [PATCH v2] send-pack: run GPG after atomic push checking Han Xin
2020-09-19  0:02               ` Junio C Hamano
2020-09-19 14:47                 ` [PATCH v3] " Han Xin
2020-09-19 23:02                   ` Junio C Hamano
2020-09-20  6:20                     ` [PATCH v4] " Han Xin
2020-09-16 17:35         ` [PATCH 2/2] send-pack: check atomic push before running GPG 韩欣(炽天)
2020-09-15 20:31 ` [PATCH 1/2] t5534: new test case for atomic signed push Junio C Hamano
2020-09-16  0:34   ` brian m. carlson
2020-09-15 20:34 ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqeen2xrok.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=chiyutianyi@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=hanxin.hx@alibaba-inc.com \
    --cc=worldhello.net@gmail.com \
    --cc=zhiyou.jx@alibaba-inc.com \
    --subject='Re: [PATCH 2/2] send-pack: check atomic push before running GPG' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).