Re: [PATCH] get_ref_states: strdup entries and free util in stale list
On Tue, Dec 1, 2009 at 08:34, Junio C Hamano <gitster@pobox.com> wrote:
Show 19 quoted lines
> Bert Wesarg <bert.wesarg@googlemail.com> writes:
>
>> 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 ;-).
Yes. That makes perfectly sense. Sorry for the hassle.
Show 17 quoted lines
>
>> 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.
I don't see you point here. The current code is:
for (ref = stale_refs; ref; ref = ref->next) {
struct string_list_item *item =
string_list_append(abbrev_branch(ref->name), &states->stale);
item->util = xstrdup(ref->name);
}
So 0 == strcmp(item->string, abbrev_ref(item->util, "refs/heads/"))
should be true, shouldn't it?Bert
Bert Wesarg <bert.wesarg@googlemail.com>