RE: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig
-Original Message- From: Junio C Hamano [mailto:gits...@pobox.com] Sent: Sunday, July 13, 2014 10:01 AM To: Jeff King Cc: Keller, Jacob E; git@vger.kernel.org Subject: Re: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig Jeff King p...@peff.net writes: On Fri, Jul 11, 2014 at 02:54:25PM -0700, Junio C Hamano wrote: 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? I don't think so. I think we've always been not-very-lenient with parsing values. Two examples: ... So I do not think we have ever had a rule, but if we did, it is probably silently ignore unknown keys, complain on unknown values. Yeah, somehow I was mixing up these two cases fuzzily in my mind while responding. Rejecting unknown keys is bad, but we cannot be perfectly forward compatible nor behave sensibly on unknown values while issuing errors against known-to-be-bad values, so your rule above sounds like the most sensible thing to do. The only difference is whether we halt or ignore the unknown value we complained about. Personally I am ok with either, but prefer the complain and choose the default so that older gits don't completely stop. But in some cases, the change is not easily compatible so then stopping might be preferred as the old git will not behave as expected. Thanks, Jake -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig
Jeff King p...@peff.net writes: On Fri, Jul 11, 2014 at 02:54:25PM -0700, Junio C Hamano wrote: 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? I don't think so. I think we've always been not-very-lenient with parsing values. Two examples: ... So I do not think we have ever had a rule, but if we did, it is probably silently ignore unknown keys, complain on unknown values. Yeah, somehow I was mixing up these two cases fuzzily in my mind while responding. Rejecting unknown keys is bad, but we cannot be perfectly forward compatible nor behave sensibly on unknown values while issuing errors against known-to-be-bad values, so your rule above sounds like the most sensible thing to do. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig
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.
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.
+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.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig
On Fri, 2014-07-11 at 13:46 -0400, Jeff King wrote:
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.
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.
+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, I figured setting it up all at the end makes more sense, and is
less prone to subtle bugs, since we don't directly modify sort using |=
or relying on particular values of sort initially.
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).
It's possible we should warn instead though, so that older gits work
with new sorts that they don't understand.
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?
Thanks,
Jake
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig
On Fri, Jul 11, 2014 at 06:11:08PM +, Keller, Jacob E wrote: 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: 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. 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 +, Keller, Jacob E wrote: 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: 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? 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
Re: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig
Jeff King p...@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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig
On Fri, 2014-07-11 at 14:54 -0700, Junio C Hamano wrote:
Jeff King p...@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.
The current revision of the patch prints an error and warning about not
using the configured tag value. It does still run. I added a test to
ensure this as well.
The easiest way to change overall behavior is to change the git-config's
die_on_error to be false, so that we no longer die on configuration
errors.
It appears this would resolve the issue for many configuration options
(not all, as I think a few are still hard coded to die) but it would be
a change that is well outside the scope of this patch.
I think that for now behavior I added is good, as we *know* that
tag.sort will add new parameters in the near future, so it makes no
sense to have it die on a config that is only slightly newer than the
git version.
Glad I could help. Junio if you could review the v7 I sent a bit ago for
any possible mistakes that I forgot to clean up that would be great.
Thanks,
Jake
N�r��yb�X��ǧv�^�){.n�+ا���ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf
