Re: [PATCH] get_ref_states: strdup entries and free util in stale list

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 1, 2009, 07:34 UTC
Message-ID: <7vy6ln2llw.fsf@alter.siamese.dyndns.org>
In-Reply-To: <36ca99e90911302249r5f77f031o73cc7bb13dc375cf@mail.gmail.com>

Bert Wesarg <bert.wesarg@googlemail.com> writes:

Show 8 quoted lines

> On Tue, Dec 1, 2009 at 01:21, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> Hmm, the Subject: matches what the code does, but nobody mentions why it
>> is necessary (iow, what breaks in the current code and what becomes better
>> if the patch is applied). The blank space before your "S-o-b:" line is
>> for you to describe these things.
> Sure. unfortunately the code where the string list is filled is not in
> the patch. But if you look at the code it should be self-explanatory.

That is _exactly_ why I want the description in the commit log message. I don't want commit messages (or lack thereof) to force people to look at the code outside the patch.

Otherwise I'll have to ask _you_ to personally give the 7-line explanation you just gave us to anybody who runs "git log -p" with the default context size after this patch is applied. I do not think you have the bandwidth to handle that ;-).

> There is actually also an other solution: we could first strdup the
> ref name to the .util member and take this as the input for the
> abbrev_ref()/string list entry and safe there the strdup.

I too thought something like that may make sense, but it doesn't look like so, for two reasons:

 - string-list API is a bit cumbersome to use if you allocate the string
   yourself.  You will be made responsible for freeing them, and often you
   do so by setting strdup_strings to true immediately before calling
   string_list_clear(), which is kind of ugly;

 - The ref abbrev_branch() is called and the address of whose substring is
   taken to be used as "name" in handle_one_branch() is refspec.src, but
   what goes to .util is refname that is refspec.dst---they are different
   strings and one is not a substring of the other.

IOW, your patch text is good; it just would have been better with a bit more explanation.

Thanks, will queue in 'maint'.