From: Adrian Ratiu <adrian.ratiu@collabora.com>
Date: Tue, 16 Dec 2025 09:09:52 GMT
Subject: Re: [PATCH v4 05/11] transport: convert pre-push to hook API
Message-ID: <87a4zihjxb.fsf@gentoo.mail-host-address-is-not-set>
In-Reply-To: <aUETgWJ5DJaEpugO@pks.im>

On Tue, 16 Dec 2025, Patrick Steinhardt <ps@pks.im> wrote:
> On Thu, Dec 04, 2025 at 04:15:29PM +0200, Adrian Ratiu wrote:
>> diff --git a/transport.c b/transport.c
>> index c7f06a7382..047f2cefba 100644
>> --- a/transport.c
>> +++ b/transport.c
>> @@ -1316,65 +1316,71 @@ static void die_with_unpushed_submodules(struct string_list *needs_pushing)
> [snip]
>> -	if (start_command(&proc)) {
>> -		finish_command(&proc);
>> -		return -1;
>> +	switch (r->status) {
>> +	case REF_STATUS_REJECT_ALREADY_EXISTS:
>> +	case REF_STATUS_REJECT_FETCH_FIRST:
>> +	case REF_STATUS_REJECT_NEEDS_FORCE:
>> +	case REF_STATUS_REJECT_NODELETE:
>> +	case REF_STATUS_REJECT_NONFASTFORWARD:
>> +	case REF_STATUS_REJECT_REMOTE_UPDATED:
>> +	case REF_STATUS_REJECT_SHALLOW:
>> +	case REF_STATUS_REJECT_STALE:
>> +	case REF_STATUS_UPTODATE:
>> +		return 0; /* skip refs which won't be pushed */
>> +	default:
>> +		break;
>>  	}
>>  
>> -	sigchain_push(SIGPIPE, SIG_IGN);
>> +	if (!r->peer_ref)
>> +		return 0;
>>  
>> -	strbuf_init(&buf, 256);
>> +	strbuf_reset(&data->buf);
>> +	strbuf_addf(&data->buf, "%s %s %s %s\n",
>> +		    r->peer_ref->name, oid_to_hex(&r->new_oid),
>> +		    r->name, oid_to_hex(&r->old_oid));
>>  
>> -	for (r = remote_refs; r; r = r->next) {
>> -		if (!r->peer_ref) continue;
>> -		if (r->status == REF_STATUS_REJECT_NONFASTFORWARD) continue;
>> -		if (r->status == REF_STATUS_REJECT_STALE) continue;
>> -		if (r->status == REF_STATUS_REJECT_REMOTE_UPDATED) continue;
>> -		if (r->status == REF_STATUS_UPTODATE) continue;
>
> The new code looks a lot nicer in my opinion. But one thing I wonder
> about is how these statements translate to the above switch statement.
> We ignore a lot more `status` values now compared to previously, and
> the reason for this is never explained.
>
> Am I missing anything obvious?

I thought it was obvious when writing the code :) it might not be.

The reason is the list was not exhaustive: for refs which we know
beforehand will not be pushed (status rejected), there is no need to
feed the pre-push hook stdin. It's saving a few cpu cycles in some
rejection corner cases, which were missed previously.

I could:
1. Split the extra *_REJECT_* case aditions into a separate commit,
   highlighting and explaining this better.
2. Drop the new cases since they are just a minor improvement in this
   series, not very important for the series overall.

Any preference?