On Thu, Jul 17, 2008 at 10:23 AM, Nagy Gabor <ngaba@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