On Fri, 2014-07-11 at 15:17 -0700, Junio C Hamano wrote:
> Jeff King <p...@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;
> }
>
Ok.. I suppose that could be done.
Thanks,
Jake
> > 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.
