Re: [PATCH v2] tag: support configuring --sort via .gitconfig
- From
Keller, Jacob E <jacob.e.keller@intel.com>
- Date
- Jul 10, 2014, 20:38 UTC
- Message-ID
- <1405024718.4925.0.camel@jekeller-desk1.amr.corp.intel.com>
- In-Reply-To
- <20140710040731.GD28401@sigill.intra.peff.net>
On Thu, 2014-07-10 at 00:07 -0400, Jeff King wrote:
Show 31 quoted lines
> On Wed, Jul 09, 2014 at 03:36:51PM -0700, Jacob Keller wrote:
>
> > +static int parse_sort_string(const char *arg)
> > +{
> > + int sort = 0;
> > + int flags = 0;
> > +
> > + if (*arg == '-') {
> > + flags |= REVERSE_SORT;
> > + arg++;
> > + }
> > + if (starts_with(arg, "version:")) {
> > + sort = VERCMP_SORT;
> > + arg += 8;
> > + } else if (starts_with(arg, "v:")) {
> > + sort = VERCMP_SORT;
> > + arg += 2;
> > + } else
> > + sort = STRCMP_SORT;
> > + if (strcmp(arg, "refname"))
> > + die(_("unsupported sort specification %s"), arg);
> > + sort |= flags;
> > +
> > + return sort;
> > +}
>
> I know this is existing code you are moving, but I noticed it looks ripe
> for using skip_prefix. Perhaps while we are in the area we should do the
> following on top of your patch (I'd also be happy for it be squashed
> in, but that may be too much in one patch).
> I am fine with this being another patch or squashed in. I didn't even know that was available :) I also like putting the two equivalent conditionals into the same if block.
Thanks, Jake
Show 39 quoted lines
> -- >8 --
> Subject: [PATCH] tag: use skip_prefix instead of magic numbers
>
> We can make the parsing of the --sort parameter a bit more
> readable by having skip_prefix keep our pointer up to date.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/tag.c | 14 +++++---------
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/tag.c b/builtin/tag.c
> index a679e32..016df98 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -353,18 +353,14 @@ static int parse_sort_string(const char *arg)
> int sort = 0;
> int flags = 0;
>
> - if (*arg == '-') {
> + if (skip_prefix(arg, "-", &arg))
> flags |= REVERSE_SORT;
> - arg++;
> - }
> - if (starts_with(arg, "version:")) {
> - sort = VERCMP_SORT;
> - arg += 8;
> - } else if (starts_with(arg, "v:")) {
> +
> + if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
> sort = VERCMP_SORT;
> - arg += 2;
> - } else
> + else
> sort = STRCMP_SORT;
> +
> if (strcmp(arg, "refname"))
> die(_("unsupported sort specification %s"), arg);
> sort |= flags;