Re: [PATCH 12/19] tree-diff: remove special-case diff-emitting code for empty-tree cases

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 25, 2014, 17:45 UTC
Message-ID: <xmqq8urymaua.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20140325092040.GA3777@mini.zxlink>

Kirill Smelkov <kirr@navytux.spb.ru> writes:

Show 28 quoted lines

>> >  static int tree_entry_pathcmp(struct tree_desc *t1, struct tree_desc *t2)
>> >  {
>> >  	struct name_entry *e1, *e2;
>> >  	int cmp;
>> >  
>> > +	if (!t1->size)
>> > +		return t2->size ? +1 /* +∞ > c */  : 0 /* +∞ = +∞ */;
>> > +	else if (!t2->size)
>> > +		return -1;	/* c < +∞ */
>> 
>> Where do these "c" come from?  I somehow feel that these comments
>> are making it harder to understand what is going on.
>
> "c" means some finite "c"onstant here. When I was studying at school and
> at the university, it was common to denote constants via this letter -
> i.e. in algebra and operators they often show scalar multiplication as
>
>     c·A     (or α·A)
>
> etc. I understand it could maybe be confusing (but it came to me as
> surprise), so would the following be maybe better:
>
>         if (!t1->size)
>         	return t2->size ? +1 /* +∞ > const */  : 0 /* +∞ = +∞ */;
>         else if (!t2->size)
>         	return -1;	/* const < +∞ */
>
> ?

Not better at all, I am afraid. A "const" in the code usually means "something that does not change, as opposed to a variable", but what you are saying here is "t1 does not have an element but t2 still does. Pretend as if t1 has a virtual/fake element that is larger than any real element t2 may happen to have at the head of its queue", and you are labeling that "real element at the head of t2" as "const", but as the walker advances, the head element in t1 and t2 will change---they are not "const" in that sense, and the reader is left scratching his head seeing "const" there, wondering what the author of the comment meant.

"real" or "concrete" might be better a phrasing, but I do not think having "/* +inf > concrete */" there helps the reader understand what is going on in the first place. Perhaps:

        /*
         * When one side is empty, pretend that it has an element
         * that sorts later than what the other non-empty side has,
         * so that the caller advances the non-empty side without
         * touching the empty side.
         */
        if (!t1->size)
                return !t2->size ? 0 : 1;
        else if (!t2->size)
                return -1;

or something?