Re: sort --compare-version

[Bob Proulx]

Bruce Korb wrote:
> This works for me :)
> Either ``-V'' or ``--compare-version'' will trigger the use of
> strverscmp in lieu of memcmp as the comparison function.

That looks very interesting to me.  I have often wanted a sort that
would sort using a version compare similar to the 'ls -v' version
comparison.  Thanks for working on it.  A few comments about the patch
itself and then some proceedural things about what needs to be done to
move this forward.

> @@ -353,6 +354,7 @@ Other options:\n\
> +  -V, --compare-version     compare embedded numbers as version numbers\n\

Looking at the existing options I see these:

Ordering options:
  -g, --general-numeric-sort  compare according to general numerical value
  -M, --month-sort            compare (unknown) < `JAN' < ... < `DEC'
  -n, --numeric-sort          compare according to string numerical value

That would mean to me that version comparison would need to be of the
form --<something>-sort and would need to be in the "Ordering
options:" section and not the "Other options:" section.  I suppose
that this option should be called --version-sort instead in order to
be consistent with the already existing options.

But!  Using --version-sort conflicts with --version.  Which means my
purpose here is simply to call this up for discussion such that some
reasonable choice can be made since this will need to be an exception
to the rule.  Comments?

I do think it needs to be moved into the "Ordering options:" section
through regardless.

> +/* Compare the keys TEXTA (of length LENA) and TEXTB (of length LENB)
> +   using strverscmp.  */
> +
> +static int
> +compare_version (char *restrict texta, size_t lena,
> +              char *restrict textb, size_t lenb)
> +{
> +  int diff;
> +  char sv_a = texta[lena];
> +  char sv_b = textb[lenb];
> +
> +  texta[lena] = textb[lenb] = '\0';
> +  diff = strverscmp (texta, textb);
> +
> +  texta[lena] = sv_a;
> +  textb[lenb] = sv_b;
> +
> +  return diff;
> +}

Pardon me for not looking but why is texta[lena] saved, zeroed, and
then restored?  A clue left behind there would be nice.

> @@ -2587,10 +2615,10 @@ check_ordering_compatibility (void)
>  
>    for (key = keylist; key; key = key->next)
>      if ((1 < (key->random + key->numeric + key->general_numeric + key->month
> -           + !!key->ignore))
> +           + key->version + !!key->ignore))
>       || (key->random && key->translate))
>        {
> -     char opts[7];
> +     char opts[sizeof short_options];
>       char *p = opts;
>       if (key->ignore == nondictionary)
>         *p++ = 'd';

I don't like the magic number 7 there (and I think it should have been
8 anyway meaning that you have also fixed a bug to be noted in the log
entry) but using sizeof short_options I think is not correct either.

I think I like this following technique better.  In my mind it makes
it much more self-documenting without using extra space.

      {
        char opts[sizeof "dfgiMnVR"];
        char *p = opts;
        if (key->ignore == nondictionary)
          *p++ = 'd';
        if (key->translate)
          *p++ = 'f';
        if (key->general_numeric)
          *p++ = 'g';
        if (key->ignore == nonprinting)
          *p++ = 'i';
        if (key->month)
          *p++ = 'M';
        if (key->numeric)
          *p++ = 'n';
        if (key->version)
          *p++ = 'V';
        if (key->random)
          *p++ = 'R';
        *p = '\0';
        incompatible_options (opts);
      }

In general I like the feature very much.  Thank you for working on
this.  Have you submitted the necessary copyright paperwork to the FSF
for contribution to coreutils?  Would you also be able to work on
other things needed to get this into coreutils such as ChangeLog
entry, test cases, info documentation, and NEWS entry?  I would be
willing to help with these tasks.

Bob