Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig

From: Keller, Jacob E <jacob.e.keller@intel.com>
Date: Jul 11, 2014, 21:08 UTC
Message-ID: <1405112907.22963.48.camel@jekeller-desk1.amr.corp.intel.com>
In-Reply-To: <20140711210633.GA12546@sigill.intra.peff.net>

On Fri, 2014-07-11 at 17:06 -0400, Jeff King wrote:

Show 33 quoted lines

> On Fri, Jul 11, 2014 at 01:51:35PM -0700, Jacob Keller wrote:
> 
> > +	if (!strcmp(var, "tag.sort")) {
> > +		if (!value)
> > +			return config_error_nonbool(var);
> > +		status = parse_sort_string(value, &tag_sort);
> > +		if (status) {
> > +			warning("using default lexicographic sort order");
> > +			tag_sort = STRCMP_SORT;
> > +		}
> 
> I think this is a good compromise of the issues we discussed earlier. As
> you noted, it should probably be marked for translation. I'm also not
> sure the message content is clear in all situations. If I have a broken
> tag.sort, I get:
> 
>   $ git config tag.sort bogus
>   $ git tag >/dev/null
>   error: unsupported sort specification bogus
>   warning: using default lexicographic sort order
> 
> Not too bad, though reminding me that the value "bogus" came from
> tag.sort would be nice. But what if I override it:
> 
>   $ git tag --sort=v:refname >/dev/null
>   error: unsupported sort specification bogus
>   warning: using default lexicographic sort order
> 
> That's actively wrong, because we are using v:refname from the
> command-line. Perhaps we could phrase it like:
> 
>   warning: ignoring invalid config option tag.sort
>

ok that makes more sense. I can't really avoid printing the warning at all since config parsing happens before the option parsing.

I like this wording. I will respin again :D

Thanks, Jake

Show 12 quoted lines

> or similar, which helps both cases.
> 
> As an aside, I also think the error line could more clearly mark the
> literal contents of the variable. E.g., one of:
> 
>   error: unsupported sort specification: bogus
> 
> or
> 
>   error: unsupported sort specification 'bogus'
> 
> -Peff