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

[Patrick Steinhardt <ps@pks.im>]

On Tue, Dec 16, 2025 at 11:09:52AM +0200, Adrian Ratiu wrote:
> 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?

I'd personally learn towards (2) unless it is fixing an actual bug that
can be demonstrated. In that case it might make sense to do (1) and
explain why this wasn't an issue until now.

Thanks!

Patrick