Re: [PATCH 4/5] versioncmp: pass full tagnames to swap_prereleases()

From: SZEDER Gábor <szeder@ira.uka.de>
Date: Sep 8, 2016, 20:37 UTC
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>:

Show 14 quoted lines

> 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