Re: Fix proposal for comparaison bugs in PostgreSQL::Version

On Tue, 28 Jun 2022 18:17:40 -0400
Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:

> On 2022-06-28 Tu 16:53, Jehan-Guillaume de Rorthais wrote:
> > ...
> > A better fix would be to store the version internally as version_num that
> > are trivial to compute and compare. Please, find in attachment an
> > implementation of this.
> >
> > The patch is a bit bigger because it improved the devel version to support
> > rc/beta/alpha comparison like 14rc2 > 14rc1.
> >
> > Moreover, it adds a bunch of TAP tests to check various use cases.
>
>
> Nice catch, but this looks like massive overkill. I think we can very
> simply fix the test in just a few lines of code, instead of a 190 line
> fix and a 130 line TAP test.

I explained why the patch was a little bit larger than required: it fixes the
bugs and do a little bit more. The _version_cmp sub is shorter and easier to
understand, I use multi-line code where I could probably fold them in a
one-liner, added some comments... Anyway, I don't feel the number of line
changed is "massive". But I can probably remove some code and shrink some other
if it is really important...

Moreover, to be honest, I don't mind the number of additional lines of TAP
tests. Especially since it runs really, really fast and doesn't hurt day-to-day
devs as it is independent from other TAP tests anyway. It could be 1k, if it
runs fast, is meaningful and helps avoiding futur regressions, I would welcome
the addition.

If we really want to save some bytes, I have a two lines worth of code fix that
looks more readable to me than fixing _version_cmp:

+++ b/src/test/perl/PostgreSQL/Version.pm
@@ -92,9 +92,13 @@ sub new
# Split into an array
my @numbers = split(/\./, $arg);

+ # make sure all digit of the array-represented version are set so we can
+ # keep _version_cmp code as a "simple" digit-to-digit comparison loop
+ $numbers[$_] += 0 for 0..3;
+
# Treat development versions as having a minor/micro version one less than
# the first released version of that branch.
- push @numbers, -1 if ($devel);
+ $numbers[3] = -1 if $devel;

$devel ||= "";

But again, in my humble opinion, the internal version array representation is
more a burden we should replace by the version_num...

> It was never intended to be able to compare markers like rc1 vs rc2, and
> I don't see any need for it. If you can show me a sane use case I'll
> have another look, but right now it seems quite unnecessary.

I don't have a practical use case right now, but I thought the module
would be more complete with these little few more line of codes. Now, keep in
mind these TAP modules might help external projects, not just core.

In fact, I wonder what was your original use case to support
devel/alpha/beta/rc versions, especially since it was actually not working?
Should we just get rid of this altogether and wait for an actual use case?

Cheers,