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

[Junio C Hamano <gitster@pobox.com>]

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

>> +#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.