Re: [PATCH v3 3/4] name-rev: provide debug output

From: Michael J Gruber <git@grubix.eu>
Date: Apr 3, 2017, 14:46 UTC
Message-ID: <c85bc2a1-56d8-8a02-6089-2b8cb3d39e99@grubix.eu>
In-Reply-To: <xmqqzig1xpm6.fsf@gitster.mtv.corp.google.com>

Junio C Hamano venit, vidit, dixit 31.03.2017 20:33:

Show 37 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Michael J Gruber <git@grubix.eu> writes:
>>
>>>> The only case that this change may make a difference I can think of
>>>> is when you have a tag object pointed at from outside refs/tags
>>>> (e.g. refs/heads/foo is a tag object); if you are trying to change
>>>> the definition of "from_tag" from the current "Is the tip inside
>>>> refs/tags/?" to "Is the tip either inside refs/tags/ or is it a tag
>>>> object anywhere?", that may be a good change (I didn't think things
>>>> through, though), but that shouldn't be hidden inside a commit that
>>>> claims to only add support for debugging.
>>>>
>>>> What problem are you solving?  
>>>
>>> Sorry, I forgot about that change and failed to mention it.
>>>
>>> It makes no difference in the non-debug case which cares about the
>>> Boolean only. In the debug case, I want to distinguish between
>>> annotated and lightweight tags, just like describe --debug does. By
>>> adding 1 via deref and passing this down, I know that an annotated tag
>>> gets the value 2, a lightweight tag 1 and everything else 0, just like
>>> describe --tags.
>>
>> So it sounds like you meant to do something else, and the
>> implementation is wrong for that something else (i.e. it wouldn't do
>> the right thing for a tag object outside refs/tags/, with or without
>> the "--debug" option passed).
> 
> The damage seems worse, but I may be misreading the code.
> 
> is_better_name() compares name->from_tag and from_tag numerically,
> because it was designed to take a boolean view of that variable.
> Now, an artificially bumped 2 gets compared with name->from_tag that
> may be 1 and gets different priority.  That artificially inflated
> value may be propagated to name->from_tag when the current tip is
> judged as a better basis for naming the object.

No, I checked not to change the existing behaviour.

If you look at the comment above that then you see that one of the sides of the comparison is a non-tag, so we compare 0 to 1 or 2, the boolean outcome being the same.

Show 5 quoted lines

> If this change is only for debugging, perhaps inside if(data->debug)
> you added, instead of looking at from_tag, you can look at both
> from_tag and deref to choose which prio-nmes to show, without
> butchering the value in from_tag variable to affect the existing
> code that is exercised with or without --debug?

What I did overlook, though, was that name-rev uses the notion "under refs/tags" for "being a tag".

In fact, it's puzzling how different describe and name-rev proceed and weigh the alternatives. I didn't mean to change that.

In retrospect, displaying the "same" debug information for the two commands doesn't make too much sense as long as they use different information. name-rev does-not distinguish between tag types, so why even display it?

I think I should change 3/4 to display exactly those bits that name-rev actually uses for weighing different possible descriptions; they are differents from the "describe-bits". So please withhold 3/4 and 4/4.

Michael