Re: bug in comm --check-order

[Jim Meyering]

Bruno Haible wrote:
> "comm --check-order" does not detect all cases of misordered input.
...
> Attached is a fix. Feel free to add a unit test for this; I'm not familiar
> enough with coreutils to do that.

Thank you for the fine report and patch.
And thanks for adding memcmp2 to gnulib.

Here's your patch again, with your ChangeLog entry and
a slightly different one-line summary.

I'll apply it tomorrow, along with the following test and NEWS entry:

>From 739b9740202b6c1a0bd22a6868f81496cb9cbf3c Mon Sep 17 00:00:00 2001
From: Bruno Haible <address@hidden>
Date: Sun, 8 Mar 2009 18:25:29 +0100
Subject: [PATCH 1/2] comm: fix a bug in its new --check-order option

* src/comm.c: Include memcmp2.h.
(check_order): Use memcmp2 instead of memcmp.
* bootstrap.conf (gnulib_modules): Add memcmp2.
---
 bootstrap.conf |    2 +-
 src/comm.c     |    9 ++++-----
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/bootstrap.conf b/bootstrap.conf
index e61e241..0747bb8 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -71,7 +71,7 @@ gnulib_modules="
        manywarnings
        mbrtowc
        mbswidth
-       memcasecmp mempcpy
+       memcasecmp memcmp2 mempcpy
        memrchr mgetgroups
        mkancesdirs mkdir mkdir-p mkstemp mktime modechange
        mountlist mpsort obstack pathmax perl physmem
diff --git a/src/comm.c b/src/comm.c
index 4ec7e4a..c60936f 100644
--- a/src/comm.c
+++ b/src/comm.c
@@ -1,5 +1,5 @@
 /* comm -- compare two sorted files line by line.
-   Copyright (C) 86, 90, 91, 1995-2005, 2008 Free Software Foundation, Inc.
+   Copyright (C) 86, 90, 91, 1995-2005, 2008-2009 Free Software Foundation, 
Inc.

    This program is free software: you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -25,6 +25,7 @@
 #include "error.h"
 #include "quote.h"
 #include "stdio--.h"
+#include "memcmp2.h"
 #include "xmemcoll.h"

 /* The official name of this program (e.g., no `g' prefix).  */
@@ -199,10 +200,8 @@ check_order (struct linebuffer const *prev,
            order = xmemcoll (prev->buffer, prev->length - 1,
                              current->buffer, current->length - 1);
          else
-           {
-             size_t len = min (prev->length, current->length) - 1;
-             order = memcmp (prev->buffer, current->buffer, len);
-           }
+           order = memcmp2 (prev->buffer, prev->length - 1,
+                            current->buffer, current->length - 1);

          if (0 < order)
            {
--
1.6.2.rc1.285.gc5f54


>From da82fef936a80b7fd3d4914c3e14f67abdb29088 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sun, 8 Mar 2009 18:37:08 +0100
Subject: [PATCH 2/2] tests: add a test for newly-fixed bug in comm --check-order

* tests/misc/comm (ooo-prefix): Add a test for today's fix.
* NEWS (Bug fixes): Mention it.
---
 NEWS            |    5 +++++
 tests/misc/comm |    9 ++++++++-
 2 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/NEWS b/NEWS
index 0f6e853..608307e 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,11 @@ GNU coreutils NEWS                                    -*- 
outline -*-

 ** Bug fixes

+  comm's new --check-order option would fail to detect disorder on any pair
+  of lines where one was a prefix of the other.  For example, this would
+  fail to report the disorder: printf 'Xb\nX\n'>k; comm --check-order k k
+  [bug introduced in coreutils-7.0]
+
   cp once again diagnoses the invalid "cp -rl dir dir" right away,
   rather than after creating a very deep dir/dir/dir/... hierarchy.
   The bug strikes only with both --recursive (-r, -R) and --link (-l).
diff --git a/tests/misc/comm b/tests/misc/comm
index 81a8529..80a0c2d 100755
--- a/tests/misc/comm
+++ b/tests/misc/comm
@@ -2,7 +2,7 @@
 # -*- perl -*-
 # Test comm

-# Copyright (C) 2008 Free Software Foundation, Inc.
+# Copyright (C) 2008-2009 Free Software Foundation, Inc.

 # This program is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -116,6 +116,13 @@ my @Tests =
     {OUT => "\t\t2\n"},
     {ERR => "$prog: file 1 is not in sorted order\n"}],

+   # out-of-order, line 2 is a prefix of line 1
+   # until coreutils-7.2, this test would fail -- no disorder detected
+   ['ooo-prefix', '--check-order', {IN=>{a=>"Xa\nX\n"}}, {IN=>{b=>""}},
+    {EXIT=>1},
+    {OUT => "Xa\n"},
+    {ERR => "$prog: file 1 is not in sorted order\n"}],
+
    # alternate delimiter: ','
    ['delim-comma', '--output-delimiter=,', @inputs,
     {OUT=>"1\n,2\n,,3\n"} ],
--
1.6.2.rc1.285.gc5f54