Re: [PATCH] diff: differentiate error handling in parse_color_moved_ws

[Stefan Beller <sbeller@google.com>]

On Sun, Nov 4, 2018 at 10:12 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Stefan Beller <sbeller@google.com> writes:
> >
> >>
> >> -static int parse_color_moved_ws(const char *arg)
> >> +static unsigned parse_color_moved_ws(const char *arg)
> >>  {
> >>      int ret = 0;
> >>      struct string_list l = STRING_LIST_INIT_DUP;
> >> @@ -312,15 +312,19 @@ static int parse_color_moved_ws(const char *arg)
> >>                      ret |= XDF_IGNORE_WHITESPACE;
> >>              else if (!strcmp(sb.buf, "allow-indentation-change"))
> >>                      ret |= COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE;
> >> -            else
> >> +            else {
> >> +                    ret |= COLOR_MOVED_WS_ERROR;
> >>                      error(_("ignoring unknown color-moved-ws mode '%s'"), sb.buf);
> >> +            }
> >> ...
> >>      } else if (skip_prefix(arg, "--color-moved-ws=", &arg)) {
> >> -            options->color_moved_ws_handling = parse_color_moved_ws(arg);
> >> +            unsigned cm = parse_color_moved_ws(arg);
> >> +            if (cm & COLOR_MOVED_WS_ERROR)
> >> +                    die("bad --color-moved-ws argument: %s", arg);
> >> +            options->color_moved_ws_handling = cm;
> >
> > Excellent.
> >
> > Will queue.  Perhaps a test or two can follow to ensure a bad value
> > from config does not kill while a command line does?
>
> Wait.  This does not fix
>
>         git -c diff.colormovedws=nonsense diff
>
> that dies with an error message---it should ignore the config and at
> moat issue a warning.

$ git -c core.abbrev=41 diff
error: abbrev length out of range: 41
fatal: unable to parse 'core.abbrev' from command-line config
$ ./git -c  diff.colormovedws=nonsense diff HEAD
error: ignoring unknown color-moved-ws mode 'nonsense'
fatal: unable to parse 'diff.colormovedws' from command-line config

Ah, I see the issue there. We actually have to return 'success' to the
config machinery after the warning claiming ignoring the setting or
we'd have to reword the warning to state we're not ignoring the bogus
setting.

> The command line handling of
>
>         git diff --color-moved-ws=nonsense
>
> does correctly die, but it first says "error: ignoring" before
> saying "fatal: bad argument", which is suboptimal.

So to find the analogous here, maybe:

$ git diff --color=bogus
error: option `color' expects "always", "auto", or "never"

> So, not so excellent (yet) X-<.

So to reach excellence, we'd want to reword the warning
message and a test.

Thanks,
Stefan