Re: [PATCH] receive-pack.c: don't miss exporting unsolicited push certificates
- From
Stefan Beller <stefanbeller@gmail.com>
- Date
- Jan 10, 2015, 03:55 UTC
- Message-ID
- <54B0A2C1.4040602@gmail.com>
- In-Reply-To
- <xmqqmw5r9zck.fsf@gitster.dls.corp.google.com>
On 09.01.2015 17:52, Junio C Hamano wrote:
Show 12 quoted lines
> 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?
Yes we did. This is unrelated to the previous patch. I stuffed it into this thread as I found it was touching the same feature.
Show 7 quoted lines
> > ... 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.
I had problems with wording the commit message because I have no expertise with the feature. I am sorry for wasting your time there.
Show 7 quoted lines
> > 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?
yes that's what I was trying to hint at. The hook would just see it is unsolicited instead of not having the state available.
Show 49 quoted lines
>
> Still puzzled...
>
>>
>> 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);
>> }
>> }
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>