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

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