Re: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig
- From
Keller, Jacob E <jacob.e.keller@intel.com>
- Date
- Jul 11, 2014, 18:11 UTC
- Message-ID
- <1405102267.22963.35.camel@jekeller-desk1.amr.corp.intel.com>
- In-Reply-To
- <20140711174628.GC7856@sigill.intra.peff.net>
On Fri, 2014-07-11 at 13:46 -0400, Jeff King wrote:
Show 48 quoted lines
> On Fri, Jul 11, 2014 at 10:24:07AM -0700, Jacob Keller wrote:
>
> > Updated to include changes due to Junio's feedback. This has not resolved
> > whether we should fail on a configuration error or simply warn. It appears that
> > we actually seem to error out more than warn, so I am unsure what the correct
> > action is here.
>
> Yeah, we're quite inconsistent there. In some cases we silently ignore
> something unknown (e.g., a color.diff.* slot that we do not understand),
> but in most cases if it is a config key we understand but a value we do
> not, we complain and die.
>
> It's probably user-unfriendly to be silent for those cases, though. The
> user gets no feedback on why their config value is doing nothing.
>
> I tend to think that warning is not much better than erroring out. It is
> helpful if you are running a single-shot of an old version (which is
> something that I do a lot when testing old versions), but would quickly
> become irritating if you were actually using an old version of git
> day-to-day.
>
> I dunno. Maybe it is worth making life easier for people in the former
> category.
>
> > +static int parse_sort_string(const char *arg, int *sort)
> > +{
> > + int type = 0, flags = 0;
> > +
> > + if (skip_prefix(arg, "-", &arg))
> > + flags |= REVERSE_SORT;
> > +
> > + if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
> > + type = VERCMP_SORT;
> > + else
> > + type = STRCMP_SORT;
> > +
> > + if (strcmp(arg, "refname"))
> > + return error(_("unsupported sort specification %s"), arg);
> > +
> > + *sort = (type | flags);
> > +
> > + return 0;
> > +}
>
> Regardless of how we handle the error, I think this version that
> assembles the final bitfield at the end is easier to read than the
> original.
> Yes, I figured setting it up all at the end makes more sense, and is less prone to subtle bugs, since we don't directly modify sort using |= or relying on particular values of sort initially.
I personally prefer error out on options, even though it can make it a bit more difficult, though as far as I know unknown fields simply warn or are ignored. (ie: old versions of git just ignore unknown fields in configuration).
It's possible we should warn instead though, so that older gits work with new sorts that they don't understand.
I am ok with warning but I don't know the best practice for how to warn here instead of failing. Returning error causes a fatal "bad config" message. Any thoughts?
Thanks, Jake
Show 5 quoted lines
> -Peff > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html