Re: [PATCH v5 3/4] remote-curl: introduce show_http_message_fatal() helper

[Jeff King <peff@peff.net>]

On Mon, Feb 23, 2026 at 02:20:04PM +0000, Vaidas Pilkauskas via GitGitGadget wrote:

> From: Vaidas Pilkauskas <vaidas.pilkauskas@shopify.com>
> 
> Several code paths in remote-curl.c follow the same pattern of calling
> show_http_message() to display server error messages followed by die()
> to terminate with an error. This duplication makes the code more verbose
> and harder to maintain.
> 
> Introduce a new show_http_message_fatal() helper function that combines
> these two operations. This function:
> 
> 1. Displays any HTTP error message from the server via show_http_message()
> 2. Calls die() with the provided error message
> 3. Returns NORETURN to help the compiler with control flow analysis
> 
> Refactor existing call sites in remote-curl.c to use this new helper,
> reducing code duplication and improving readability. This pattern will
> also be used by upcoming HTTP 429 rate limiting support.

I'm unconvinced that this actually makes things simpler. It doesn't save
any lines, the show_http_message() becomes less flexible, and we now
encode die-logic like the numeric code for exit() in the curl code.

> +static NORETURN void show_http_message_fatal(struct strbuf *type, struct strbuf *charset,
> +				    struct strbuf *msg, const char *fmt, ...)
>  {
>  	const char *p, *eol;
> +	va_list ap;
> +	report_fn die_message_routine = get_die_message_routine();

If we got the die_routine here and not the die_message_routine, we
wouldn't have to exit() ourselves. But sadly there is not a
get_die_routine() helper yet, so we'd have to add one.

>  	/*
>  	 * We only show text/plain parts, as other types are likely
>  	 * to be ugly to look at on the user's terminal.
>  	 */
>  	if (strcmp(type->buf, "text/plain"))
> -		return -1;
> +		goto out;
>  	if (charset->len)
>  		strbuf_reencode(msg, charset->buf, get_log_output_encoding());
>  
>  	strbuf_trim(msg);
>  	if (!msg->len)
> -		return -1;
> +		goto out;
>  
>  	p = msg->buf;
>  	do {

OK, we jump to the end since now we have to make sure to hit the exit
bits.

> @@ -391,7 +393,16 @@ static int show_http_message(struct strbuf *type, struct strbuf *charset,
>  		fprintf(stderr, "remote: %.*s\n", (int)(eol - p), p);
>  		p = eol + 1;
>  	} while(*eol);
> -	return 0;
> +
> +out:
> +	strbuf_release(type);
> +	strbuf_release(charset);
> +	strbuf_release(msg);

This cleanup seems pointless. These strbufs came from the caller, so
they are not our responsibility to release. But also, we're about to
call die(), so there is no need to clean them up.

> +	va_start(ap, fmt);
> +	die_message_routine(fmt, ap);
> +	va_end(ap);
> +	exit(128);

This part is the advertised change for the function, which looks correct.

>  static int get_protocol_http_header(enum protocol_version version,
> @@ -518,21 +529,21 @@ static struct discovery *discover_refs(const char *service, int for_push)
>  	case HTTP_OK:
>  		break;
>  	case HTTP_MISSING_TARGET:
> -		show_http_message(&type, &charset, &buffer);
> -		die(_("repository '%s' not found"),
> -		    transport_anonymize_url(url.buf));
> +		show_http_message_fatal(&type, &charset, &buffer,
> +					_("repository '%s' not found"),
> +					transport_anonymize_url(url.buf));

So this is what the refactoring bought us: we trade 3 lines for 3 lines.  ;)

If we could roll in policy decisions like anonymizing the URL, or
showing the curl_errorstr, I think it might be worth it. But we can't do
that easily because they are varargs for the %s format.

IMHO we should just drop this patch, and patch 4 should do the usual
show_http_message() and die().

-Peff