Re: sort --compare-version

[Bruce Korb]

Bob Proulx wrote:
>> @@ -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.

Yep.  That's the reason for the "compare-" prefix.  I didn't like
``--compare-version-sort'' for some sort of reason, too.  Ultimately,
someone pick another name if "compare-version" is aesthetically bad.

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

Yes.

>> +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] = sv_a;
>> +  textb[lenb] = sv_b;
> 
> Pardon me for not looking but why is texta[lena] saved, zeroed, and
> then restored?  A clue left behind there would be nice.

OK.   The reason is that strverscmp works on NUL terminated strings.
The specified fields are not guaranteed to be NUL terminated, but
are guaranteed to be writable.

>>        {
>> -    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.

It is guaranteed to be sufficient and certain to be inconsequential.
Yes, it uses a half dozen extra bytes of memory.  That can't be
a problem.

> 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';

I think this is an improvement that requires more maintenance than
simply making the buffer big enough for any conceivable list of
options.  So, marginally more intelligible and marginally more
maintenance.  Looks good to me. :)

> 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?

All the paperwork is in place.

>  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.

I'm glad you're willing to help 'cuz this isn't my day job.  :)

Cheers - Bruce