Re: [PATCH] receive-pack.c: don't miss exporting unsolicited push certificates
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 10, 2015, 01:52 UTC
- Message-ID
- <xmqqmw5r9zck.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1420849874-32013-1-git-send-email-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> If the server is configured to not advertise push certificates, > a push certificate that gets pushed nevertheless will not be fully > recorded because push_cert_nonce is NULL.
Sorry, but I do not quite see what you are trying to get at.
When we did not advertise that this feature is supported, we know the incoming nonce is meaningless junk because we didn't supply the correct answer the pusher can give us; we do not even have the correct answer ourselves.
Besides, while reviewing the other patch, didn't we agree that we should reject such a push? Then there is nothing to log anyway, no?
... reads the patch and code beyond the context while scratching
head ...I notice that the above three lines do not correspond what you did in this patch. The certificate is exported via the blob interface fully with or without this change.
What is not given to the hook is the push-cert-nonce-status. While it is sufficient to tell the hook that we are not getting a good nonce (i.e. the hook does not see GIT_PUSH_CERT_NONCE_STATUS=G), we are not showing that nonce-status is "unsolicited", even though we internally compute and decide that; is that what you are trying to fix?
Still puzzled...
Show 41 quoted lines
>
> The recording of GIT_PUSH_CERT_NONCE_STATUS should be dependent on
> the status being there instead of push_cert_nonce being non NULL.
>
> Without this patch an unsolicited nonce never makes to the stage when
> being exported with GIT_PUSH_CERT_NONCE_STATUS, because in the unsolicited
> case push_cert_nonce is always NULL.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> builtin/receive-pack.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 628d13a..0e4878e 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -504,18 +504,18 @@ static void prepare_push_cert_sha1(struct child_process *proc)
> sigcheck.key ? sigcheck.key : "");
> argv_array_pushf(&proc->env_array, "GIT_PUSH_CERT_STATUS=%c",
> sigcheck.result);
> - if (push_cert_nonce) {
> + if (push_cert_nonce)
> argv_array_pushf(&proc->env_array,
> "GIT_PUSH_CERT_NONCE=%s",
> push_cert_nonce);
> + if (nonce_status)
> argv_array_pushf(&proc->env_array,
> "GIT_PUSH_CERT_NONCE_STATUS=%s",
> nonce_status);
> - if (nonce_status == NONCE_SLOP)
> - argv_array_pushf(&proc->env_array,
> - "GIT_PUSH_CERT_NONCE_SLOP=%ld",
> - nonce_stamp_slop);
> - }
> + if (nonce_status == NONCE_SLOP)
> + argv_array_pushf(&proc->env_array,
> + "GIT_PUSH_CERT_NONCE_SLOP=%ld",
> + nonce_stamp_slop);
> }
> }