Re: [PATCH] receive-pack.c: don't miss exporting unsolicited push certificates

[Stefan Beller <stefanbeller@gmail.com>]

On 09.01.2015 17:52, Junio C Hamano wrote:
> 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.

> 
>     ... 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.

> 
> 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.


> 
> 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
>