From: Jeff King <peff@peff.net>
Date: Mon, 02 Jan 2017 17:10:44 GMT
Subject: Re: [PATCH v3 20/23] try_remove_empty_parents(): don't trash argument contents
Message-ID: <20170102171043.ebfeaaylf4waq6co@sigill.intra.peff.net>
In-Reply-To: <9215973c-8db1-8f5a-2dc7-3a0137dd5c62@alum.mit.edu>

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

> > 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