From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 16 Dec 2025 08:08:33 GMT
Subject: Re: [PATCH v4 05/11] transport: convert pre-push to hook API
Message-ID: <aUETgWJ5DJaEpugO@pks.im>
In-Reply-To: <20251204141535.1986263-6-adrian.ratiu@collabora.com>

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?

Patrick