From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 20 Jan 2026 20:18:07 GMT
Subject: Re: [PATCH 1/4] remote: return non-const pointer from error_buf()
Message-ID: <xmqqbjioyr5s.fsf@gitster.g>
In-Reply-To: <20260120193857.GC3295894@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

> On Mon, Jan 19, 2026 at 04:28:42PM -0800, Junio C Hamano wrote:
>
>> Patrick Steinhardt <ps@pks.im> writes:
>> 
>> > This function signature is indeed quite misleading, and I'd argue that
>> > it continues to be so even after the change. I guess the intent is to
>> > make it a bit easier to print an error in functions that return a
>> > string.
>> >
>> > I'm not really a huge fan of this, but it's not a fault of this patch
>> > series, so let's read on.
>> 
>> I concur.  "If they do not return any useful value, they should be
>> void" was my first reaction, but presumably just like "return
>> error("message");" is a handy way to give message while signalling
>> an error to the caller, these are used to return NULL that signals
>> an error?  I do not offhand think of a good longer-term direction to
>> improve this one.
>
> Yes, that's exactly the purpose. I don't see many changes that could
> let it still fulfill that purpose, though perhaps one could argue that
> it is unnecessarily confusing for the small shortening of the code it
> provides (and ditto for error() itself).
>
> There is one thing it probably could do: return a "void *" instead. That
> would make it applicable to a wider variety of functions. But it also
> makes it even more obscure (IMHO), and this is a static-local function
> that is only used for functions that return strings anyway.
>
> If we wanted to make it more generic (and I do not think we want to), we
> can see that it differs from error() in two dimensions:
>
>   - error() writes to stderr, but error_buf() writes to a strbuf (or
>     nowhere if the strbuf is NULL)
>
>   - error() passes along integer "-1" to signal error, but error_buf()
>     passes along NULL
>
> So of the four combinations, we have:
>
>   - stderr / integer: error()
>   - stderr / pointer: not implemented
>   - strbuf / integer: not implemented
>   - strbuf / pointer: error_buf()
>
> One could imagine a suite of related functions: error_int(),
> error_null(), error_buf_int(), error_buf_null() that provide all four.
>
> But I do not see us clamoring to extend the pattern further. ;)

;-).  Perhaps stop being cute and doing

	if (... error ...) {
		format_error(err, _("error message"), ...);
                return NULL;
	}

without any magic might be more appropriate for a file-scope static
that is only used for a handful of times?  I do not think the
situation is bad enough to warrant patch noise like this, but it is
sufficiently bad that I wish we wrote them in such a more trivial
way in the first place X-<.