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

On Fri, 2014-07-11 at 14:22 -0400, Jeff King wrote:

On Fri, Jul 11, 2014 at 06:11:08PM +0000, Keller, Jacob E wrote:

quoted

I personally prefer error out on options, even though it can make it a
bit more difficult, though as far as I know unknown fields simply warn
or are ignored. (ie: old versions of git just ignore unknown fields in
configuration).

Right, we _have_ to ignore unknown config options, because we
specifically allow other programs built on git to store their config
with us (and anyway, our callback style of parsing means that no single
callback knows about all of the keys).

In the past we have staked out particular areas of the namespace,
though. E.g., the diff code said "I own all of color.diff.*, and if you
put in something I don't understand, I'll complain". That ended up being
annoying, and now we ignore slots we don't understand there.

So old gits will always silently ignore tag.sort if they don't know
about it, and we can't change that. The only thing we can change is:

quoted

It's possible we should warn instead though, so that older gits work
with new sorts that they don't understand.

Right. I think other config variables in similar situations will barf.
This is backwards-compatible as long as the new variables are a superset
(i.e., we only add new understood values, never remove or change the
meaning of existing values). It's just not forwards-compatible.

So should I respin this so that config option doesn't error out?

quoted

I am ok with warning but I don't know the best practice for how to warn
here instead of failing. Returning error causes a fatal "bad config"
message. Any thoughts?

The simplest thing is ignoring the return from parse_sort_string and
just calling "return 0". That will still say "error:", but continue on.
If you really want it to say "warning:", I think you'll have to pass a
flag into parse_sort_string. I'm not sure if it's worth the effort.

-Peff

Ok this makes sense, I am fine leaving it as error. Should I respin to
make it not die though?

Thanks,
Jake