Re: [PATCH v4 05/11] transport: convert pre-push to hook API

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