Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 17, 2017, 16:03 UTC
Message-ID: <xmqqh92r99h7.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <ce5252a5-b94a-a389-6b3e-0e4d7b243210@drmicha.warpmail.net>

Michael J Gruber <git@drmicha.warpmail.net> writes:

Show 16 quoted lines

>> +#define COMPARE(attribute, smaller_is_better)	 \
>> +	if (name->attribute > attribute) \
>> +		return smaller_is_better; \
>> +	if (name->attribute < attribute) \
>> +		return !smaller_is_better
>
> I find that define pretty confusing. On first reading, the "==" case
> seems to be missing, but that is basically "pass" as one can see in the
> later code.
>
> Also, the comparison ">"  and "<" is used to switch between the cases
> "tag vs. non-tag" and "non-tag vs. tag" which happen to be encoded by
> the from_tag attribute beeing "1 vs. 0" resp "0 vs. 1" in the following:
> ...
> But in the next two instances you use it to do "actual" comparisons
> between distances and dates:

That is pretty much deliberate. The macro is designed to compare the attribute and return if one side is strictly better than the other side, and fall through to tie-breaker that follow (i.e. lack of "==" is very deliberate). Also it is not "return if one side is strictly smaller"---the second parameter decides if smaller means better or bigger means better (i.e. "from_tag" being 1 for a name based on tag and 0 for a name based on non-tag can use the macro by telling that the side that is strictly bigger is better).

> How about something like
> ...

I did a sequence of COMPARE() macros that cascade to implement tie-breaking logic because I thought it was the easiest to read and understand, and I did "tag vs non-tag first decides, and then tiebreak to favor shorter hops and then tiebreak on date, while paying attention to committer dates even when we are comparing two commits", because I thought that would be one of the easier logic to explain to the users.

But this topic is not my itch, and quite honestly I'd be happier if you took it over, improving both the implementation and the semantics it implements, following your best judgment. You are equipped with better motivation than I am to make these decisions.

This patch has turned up a bug in git-p4 in that it seems to have misunderstood that "name-rev HEAD" is a good way to learn which branch we are currently on (it never is); as far as I am concerned, it has served its purpose ;-)

Thanks.