From: SZEDER Gábor <szeder@ira.uka.de>
Date: Thu, 08 Sep 2016 20:37:27 GMT
Subject: Re: [PATCH 4/5] versioncmp: pass full tagnames to swap_prereleases()
Message-ID: <20160908223727.Horde.jVOOJ278ssZ3qkyjkmyqZD-@webmail.informatik.kit.edu>
In-Reply-To: <xmqq7fami8nj.fsf@gitster.mtv.corp.google.com>


Quoting Junio C Hamano <gitster@pobox.com>:

> SZEDER Gábor <szeder@ira.uka.de> writes:
>
>> - * Note that we don't have to deal with the situation when both p1 and
>> - * p2 start with the same suffix because the common part is already
>> + * Note that we don't have to deal with the situation when both s1 and
>> + * s2 contain the same suffix because the common part is already
>>   * consumed by the caller.
>
> "The common part is already consumed" was relevant while the
> function was fed p1 and p2, i.e. the first difference, but the whole
> point of passing the original s1 and s2 with ofs is so that the
> function can look behind ofs as necessary.  Is "already consumed"
> still correct (or relevant) with s/p/s/ you did to its calling
> convention?

Well, it's still correct in the sense that we don't have to worry about
finding the same suffix in both strings.  However, "consume" is not the
right word to use here, as incrementing an offset until it points past
the common part doesn't count as "consumption", so more rewording would
be necessary.

I'm not sure about the relevancy of this pararaph, or the relevancy of
the original version for that matter.  I mean, there is a different
character for sure, so it's really rather obvious that it can't
possibly be the same suffix in both, isn't it?  So I don't think it
adds much value, and don't mind deleting it in the reroll.


Best,
Gábor