filevercmp incompatible change?

filevercmp incompatible change?

[(GalaxyMaster)]

Hello,

[I am not subscribed, so please CC me if you need any further info]

I think the following commit introduced a behavioural change that was not
intended: https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=1ba2b66ea45f9bc43cdc0f6f93efa59157d2b2ba

coreutils 9.1 are supplied with the previous version of that file and passes
the following test (which relies on filevercmp()):
===
galaxy@apollo:~/ls-test $ ~/rpm-work/BUILD/coreutils-9.1/src/ls -v -A .A .Z .a .z .zz~ .zz .zz.~1~ .0 .9 .zz.0 0 9 A Z a z zz~ zz zz.~1~ zz.0 | cat
.A
.Z
.a
.z
.zz~
.zz
.zz.~1~
.0
.9
.zz.0
0
9
A
Z
a
z
zz~
zz
zz.~1~
zz.0
galaxy@apollo:~/ls-test $
===

if I simply replace the filevercmp.c in coreutil's source tree with the one
from GNU gnulib and rebuild ls, the order changes:
===
galaxy@apollo:~/ls-test $ ~/rpm-work/BUILD/coreutils-9.1/src/ls -v -A .A .Z .a .z .zz~ .zz .zz.~1~ .0 .9 .zz.0 0 9 A Z a z zz~ zz zz.~1~ zz.0 | cat
.0
.9
.A
.Z
.a
.z
.zz~
.zz
.zz.~1~
.zz.0
0
9
A
Z
a
z
zz~
zz
zz.~1~
zz.0
galaxy@apollo:~/ls-test $
===

I am not very familiar with the version sorting (which was promoted by Debian
and seems to be widely adopted), but I believe the current version in GNU
gnulib is not following it and also it is worth mentioning that the coreutils
test was crafted by hand, so the order of the first example seems to be the
expected one.

The difference between two versions is very subtle (I am only including the
chunk with the real change):
===
[galaxy@apollo lib]$ diff -puN filevercmp.c{.included,}
--- filevercmp.c.included	2022-04-08 21:22:26.000000000 +1000
+++ filevercmp.c	2023-01-29 01:34:51.381264202 +1100
@@ -36,20 +37,22 @@ static idx_t
 file_prefixlen (char const *s, ptrdiff_t *len)
 {
   size_t n = *len;  /* SIZE_MAX if N == -1.  */
+  idx_t prefixlen = 0;

-  for (idx_t i = 0; ; i++)
+  for (idx_t i = 0; ; )
     {
-      idx_t prefixlen = i;
-      while (i + 1 < n && s[i] == '.' && (c_isalpha (s[i + 1])
-                                          || s[i + 1] == '~'))
-        for (i += 2; i < n && (c_isalnum (s[i]) || s[i] == '~'); i++)
-          continue;
-
       if (*len < 0 ? !s[i] : i == n)
         {
           *len = i;
           return prefixlen;
         }
+
+      i++;
+      prefixlen = i;
+      while (i + 1 < n && s[i] == '.' && (c_isalpha (s[i + 1])
+                                          || s[i + 1] == '~'))
+        for (i += 2; i < n && (c_isalnum (s[i]) || s[i] == '~'); i++)
+          continue;
     }
 }

===

It seems that the order was re-arranged and this triggers a side-effect, IMHO.

-- 
(GM)

Re: filevercmp incompatible change?

[(GalaxyMaster)]

All,

On Sun, Jan 29, 2023 at 01:58:03AM +1100, (GalaxyMaster) wrote:
> [I am not subscribed, so please CC me if you need any further info]
> 
> I think the following commit introduced a behavioural change that was not
> intended: https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=1ba2b66ea45f9bc43cdc0f6f93efa59157d2b2ba

Oh, silly me -- at the end of that commit the test was also adjusted, so
everything seems good.  However, I did not find any mentioning that the sort
order is changing with the commit and I think it may make some people scratch
their head for a moment (like, I spent almost an hour before I pinpointed it to
filevercmp.c).

-- 
(GM)

Re: filevercmp incompatible change?

[Bruno Haible]

Hi,

(GalaxyMaster) wrote:
> I think the following commit introduced a behavioural change that was not
> intended: https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=1ba2b66ea45f9bc43cdc0f6f93efa59157d2b2ba

The patch modified a unit test. This means that the change was intended.

Also it was discussed in this thread:
https://lists.gnu.org/archive/html/bug-gnulib/2022-06/msg00019.html

and accompanied with a coreutils change:
https://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=0ae619f3495dd9456e4a58226e16d3260b623a78

Bruno

Re: filevercmp incompatible change?

[Bruno Haible]

(GalaxyMaster) wrote:
> I spent almost an hour before I pinpointed it to filevercmp.c

In order to understand a commit, you typically need to navigate to the
mailing list archive. The commit may have a reference to the mailing
list posting, or you may have to look it up based on the date.

In the other direction, there was coreutils/NEWS change, and from this
commit or its date you can equally navigate to the mailing list posting
and commit on the Gnulib site.

Bruno

Re: filevercmp incompatible change?

[(GalaxyMaster)]

On Sat, Jan 28, 2023 at 04:26:30PM +0100, Bruno Haible wrote:
> (GalaxyMaster) wrote:
> > I spent almost an hour before I pinpointed it to filevercmp.c
> 
> In order to understand a commit, you typically need to navigate to the

Oh, I don't have any issues with commits and source code, it is the time to
locate why something is not working as expected.  In case of the version sort
test in coreutils, it required me to trace ls and see what functions it is
calling for the version sort, then I searched for commits and found the change.
I looked at the messages from the report of the bug till Paul fixed it, but a
crucial part of the knowledge was still missing -- it is the expected sorting
order of this Debian sort (without reading into the specification it is not
clear what the expected order is supposed to be).

Anyway, thanks.  All is good now.

-- 
(GM)