On Fri, 2014-07-11 at 17:06 -0400, Jeff King wrote:
> 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
> 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
