Re: [PATCH v3] tag: support --sort=<spec>

From: Duy Nguyen <pclouds@gmail.com>
Date: Feb 26, 2014, 11:03 UTC
Message-ID: <CACsJy8CW9p-PDJC9zXBwXrxOk9GhBFk1nVtG1Xn76g3FN+KgaA@mail.gmail.com>
In-Reply-To: <20140226090511.GA32537@sigill.intra.peff.net>

On Wed, Feb 26, 2014 at 4:05 PM, Jeff King <peff@peff.net> wrote:

Show 10 quoted lines

> On Tue, Feb 25, 2014 at 07:22:15PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> versioncmp() is copied from string/strverscmp.c in glibc commit
>> ee9247c38a8def24a59eb5cfb7196a98bef8cfdc, reformatted to Git coding
>> style. The implementation is under LGPL-2.1 and according to [1] I can
>> relicense it to GPLv2.
>
> Cool. I think doing this makes the most sense, as we do not have to
> worry about build-time config (and I do not see any particular reason
> why we would want to use the system strverscmp on glibc systems).

Another reason I want to stay away from glibc is I want to fix the algorithm to sort YY-preXX and YY-rcXX before YY. There could be reasons that glibc might reject such a change even if it makes sense in our context. Even if we make it to newer glibc and fix compat version, people on older glibc will not receive the fix while people on compat do. Not so good.

Show 38 quoted lines

>> +static int parse_opt_sort(const struct option *opt, const char *arg, int unset)
>> +{
>> +     int *sort = opt->value;
>> +     if (*arg == '-') {
>> +             *sort = REVERSE_SORT;
>> +             arg++;
>> +     } else
>> +             *sort = STRCMP_SORT;
>> +     if (starts_with(arg, "version:")) {
>> +             *sort |= VERCMP_SORT;
>> +             arg += 8;
>> +     } else if (starts_with(arg, "v:")) {
>> +             *sort |= VERCMP_SORT;
>> +             arg += 2;
>> +     }
>> +     if (strcmp(arg, "refname"))
>> +             die(_("unsupported sort specification %s"), arg);
>
> I found this logic a bit weird, as STRCMP_SORT and VERCMP_SORT are not
> mutually exclusive flags, but REVERSE and STRCMP are. I would have
> thought REVERSE is the flag, and the other two are selections. Like:
>
>   int flags = 0;
>
>   if (*arg == '-') {
>           flags |= REVERSE_SORT;
>           arg++;
>   }
>   if (starts_with(arg, "version:")) {
>           *sort = VERCMP_SORT;
>           arg += 8;
>   } else
>           *sort = STRCMP_SORT;
>
>   *sort |= flags;
>
> I think they end up doing the same thing, but maybe I am missing
> something.

No you're not. I did not make it absolute beautiful because I expected it to be deleted soon after you polish your f-e-r series. Will resend with this change shortly.

-- 
Duy