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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> On Fri, Jul 11, 2014 at 10:24:07AM -0700, Jacob Keller wrote:
>
>> Updated to include changes due to Junio's feedback. This has not resolved
>> whether we should fail on a configuration error or simply warn. It appears that
>> we actually seem to error out more than warn, so I am unsure what the correct
>> action is here.
>
> Yeah, we're quite inconsistent there. In some cases we silently ignore
> something unknown (e.g., a color.diff.* slot that we do not understand),
> but in most cases if it is a config key we understand but a value we do
> not, we complain and die.

Hm, that's bad---we've become less and less careful over time,
perhaps?

As we want to be able to enhance semantics of existing configuration
variables without having to introduce new but similar ones, we would
really want to make sure that those who share the same .git/config
or $HOME/.gitconfig across different versions of Git would not have
to suffer too much (i.e. forcing them to "config --unset" when using
their older Git is not nice).

> It's probably user-unfriendly to be silent for those cases, though. The
> user gets no feedback on why their config value is doing nothing.
>
> I tend to think that warning is not much better than erroring out. It is
> helpful if you are running a single-shot of an old version (which is
> something that I do a lot when testing old versions), but would quickly
> become irritating if you were actually using an old version of git
> day-to-day.
>
> I dunno. Maybe it is worth making life easier for people in the former
> category.

... "former cat" meaning "less irritating for single-shot use"?  I
dunno...

>> +static int parse_sort_string(const char *arg, int *sort)
>> +{
>> +	int type = 0, flags = 0;
>> +
>> +	if (skip_prefix(arg, "-", &arg))
>> +		flags |= REVERSE_SORT;
>> +
>> +	if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
>> +		type = VERCMP_SORT;
>> +	else
>> +		type = STRCMP_SORT;
>> +
>> +	if (strcmp(arg, "refname"))
>> +		return error(_("unsupported sort specification %s"), arg);
>> +
>> +	*sort = (type | flags);
>> +
>> +	return 0;
>> +}
>
> Regardless of how we handle the error, I think this version that
> assembles the final bitfield at the end is easier to read than the
> original.

Yes, this part really is nicely done, I agree.