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

> Jeff King <peff@peff.net> writes:
> 
> > 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
> >
> > or similar, which helps both cases.
> 
> Hmph.  Looks like a mild-enough middle ground, I guess.  As
> parse_sort_string() is private for "git tag" implementation, I
> actually would not mind if we taught a bit more context to it and
> phrase its messages differently.  Perhaps something like this, where
> the config parser will tell what variable it came from with "var"
> and the command line parser will pass NULL there.
> 
> static int parse_sort_string(const char *var, const char *value, int *sort)
> {
> 	...
> 	if (strcmp(value, "refname")) {
> 		if (!var)
>                 	return error(_("unsupported sort specification '%s'"), value);
> 		else {
>                 	warning(_("unsupported sort specification '%s' in variable '%s'"),
>                         	var, value);
> 			return -1;
> 		}
> 	}
> 
> 	*sort = (type | flags);
> 
> 	return 0;
> }
> 

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