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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 11, 2014, 22:17 UTC
Message-ID: <xmqqha2n35hq.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20140711210633.GA12546@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 34 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
>
> 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;
}

Show 8 quoted lines

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