[pacman-dev] vercmp discussion (was: String freeze for 3.2 release)

[Dan McGee]

On Thu, Jul 17, 2008 at 10:23 AM, Nagy Gabor <ngaba at bibl.u-szeged.hu> wrote:
>> It indeed looks like we just need to handle the case where it runs out
>> of segments on one string.
>> But we have to handle two cases : run out of segments with the
>> -release number or without it.
>> So in both cases, I handle it differently if the last remaining
>> segment starts with a letter or not.
>>
>> I am not 100% sure that it will work correctly in every single cases,
>> but the logic seems alright, there is no regression on all existing
>> vercmp test, and the 4 new tests you posted in an older thread now
>> pass fine.
>
> I did a quick read on the new vercmp code. This is not an easy-to-read
> code (and IMHO it is a bit overkill: strdup, free? - this is not
> related to your patch), but it seems OK to me. Overall it is much better
> than pre-patch state.

Here is the deal- this patch was meant to unify our code with the
upstream RPM code as much as possible. The one thing I shied away from
was using alloca() and using strdup/free instead.

> Some little observations:
> 1. This vercmp the code doesn't follow alpm code style (not related
> to your patch, again):
>        while (*one == '0') one++;
>        if(!(*one && *two)) break;
> I like this one-liners better than {} 3 liner version of these.
> Is this allowed in our patches, too?
In short, no. The idea here was to stick with the RPM upstream code so
someone can do another diff at a later time to rectify the changes.

> 2. A very little notice:
> Now 1.5b < 1.5, but 1.5.b > 1.5
> In the "block terminology" of the code, 1.5b and 1.5.b are identical.
> The difference appeared, because your patch uses isalpha() instead of
> some "next block is alpha?" computation. To be honest, this may be
> better than 1.5.b < 1.5, so you can skip this casuist note.
We need to take a step back here and examine things- RPM does a great
amount of work to make version comparison work as good as possible.

If we make a change to the core RPM code, we need to ask "why wasn't
this change made upstream" or "why don't they do it this way". I guess
that is how I see hardcoding things like "alpha" or "beta" into the
detection code- they don't do it and seem to be fine with that.

I feel for any changes proposed here, we need to explain why our
version should deviate from the upstream code (as we do in the case of
the pkgrel stuff) before I will really consider a patch to "fix"
behavior.

-Dan