On 12/31/2016 07:40 AM, Jeff King wrote:
> On Sat, Dec 31, 2016 at 04:13:00AM +0100, Michael Haggerty wrote:
>
>> It's bad manners and surprising and therefore error-prone.
>
> Agreed.
>
> I wondered while reading your earlier patch whether the
> safe_create_leading_directories() function had the same problem, but it
> carefully replaces the NUL it inserts.
>
>> -static void try_remove_empty_parents(char *refname)
>> +static void try_remove_empty_parents(const char *refname)
>> {
>> + struct strbuf buf = STRBUF_INIT;
>> char *p, *q;
>> int i;
>> - p = refname;
>> +
>> + strbuf_addstr(&buf, refname);
>
> I see here you just make a copy. I think it would be enough to do:
>
>> @@ -2305,10 +2306,11 @@ static void try_remove_empty_parents(char *refname)
>> q--;
>> if (q == p)
>> break;
>> - *q = '\0';
>> - if (rmdir(git_path("%s", refname)))
>> + strbuf_setlen(&buf, q - buf.buf);
>> + if (rmdir(git_path("%s", buf.buf)))
>> break;
>
> *q = '\0';
> r = rmdir(git_path("%s", refname));
> *q = '/';
>
> if (r)
> break;
>
> 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.
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.
Michael