From: Jeff King <peff@peff.net>
Date: Tue, 20 Jan 2026 19:38:57 GMT
Subject: Re: [PATCH 1/4] remote: return non-const pointer from error_buf()
Message-ID: <20260120193857.GC3295894@coredump.intra.peff.net>
In-Reply-To: <xmqqikcx3z5x.fsf@gitster.g>

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. ;)

-Peff