On Fri, 2014-07-11 at 14:54 -0700, Junio C Hamano wrote:
> Jeff King <[email protected]> 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��y����b�X��ǧv�^�){.n�+����ا���ܨ}���Ơz�&j:+v�������zZ+��+zf���h���~����i���z� �w���?�����&�)ߢf
