From: Bert Wesarg <bert.wesarg@googlemail.com>
Date: Tue, 01 Dec 2009 09:32:37 GMT
Subject: Re: [PATCH] get_ref_states: strdup entries and free util in stale list
Message-ID: <36ca99e90912010132iee0d42fo933aeb12833ad1ad@mail.gmail.com>
In-Reply-To: <7vy6ln2llw.fsf@alter.siamese.dyndns.org>

On Tue, Dec 1, 2009 at 08:34, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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