Re: [PATCH] daemon: use strbuf for hostname info

[Jeff King <peff@peff.net>]

On Sat, Mar 07, 2015 at 01:54:12AM +0100, René Scharfe wrote:

> >These probably want to all be strbuf_release(). Again, I doubt it
> >matters much because this is a forked daemon serving only a single
> >request (so they'll get freed by the OS soon anyway), but I think
> >freeing the memory here follows the original intent.
> 
> Using a static strbuf means (in my mind) "don't worry about freeing,
> a memory leak won't happen anyway because we reuse allocations".
> The new code adds recycling of allocations, which I somehow expect
> in connection with static allocations where possible.  You're right
> that using strbuf_release() would match the original code more
> strictly.

I don't mind recycling allocations like this. It's just that I think in
this case it makes the code actively more confusing, because we don't
actually expect any of these allocations to be recycled, do we? We
fork+exec for each daemon connection, which should receive exactly one
`host=` parameter.

That being said, I don't mind it too much if the recycling stays here.

> But this block is a no-op anyway because it's the first thing we do
> to these (initially empty) variables.  That's not immediately
> obvious and should be addressed in a separate patch.

Ah, yeah, just reading the diff I thought this was cleanup, not
initialization.

-Peff