Re: [PATCH v3 20/23] try_remove_empty_parents(): don't trash argument contents

On Mon, Jan 02, 2017 at 05:27:59PM +0100, Michael Haggerty wrote:

quoted

or something. But I doubt the single allocation is breaking the bank,
and it has the nice side effect that callers can pass in a const string
(I didn't check yet whether that enables further cleanups).

The last patch in the series passes ref_update::refname to this
function, which is `const char *`. With your suggested change, either
that member would have to be made non-const, or it would have to be cast
to const at the `try_remove_empty_parents()` callsite.

Making the member non-const would be easy, though it loses a tiny bit of
documentation and safety against misuse. Also, scribbling even
temporarily over that member would mean that this code is not
thread-safe (though it seems unlikely that we would ever bother making
it multithreaded).

I think I prefer the current version because it loosens the coupling
between this function and its callers. But I don't mind either way if
somebody feels strongly about it.

OK, let's take what you have here, then.

As an aside, I wonder whether we would be discussing this at all if we
had stack-allocated strbufs that could be used without allocating heap
memory in the usual case.

I'm not sure. We still pay the memcpy(), though I don't know how
substantial that is compared to an allocation. For these small strings,
probably not very.

-Peff