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

From: Stefan Beller <sbeller@google.com>
Date: Nov 5, 2018, 18:19 UTC
Message-ID: <CAGZ79kbJvX1Y8-iiAzXKKam7hy=pg-5+p_rzDse42-oCswMXSQ@mail.gmail.com>
In-Reply-To: <xmqqk1lshx26.fsf@gitster-ct.c.googlers.com>

On Sun, Nov 4, 2018 at 10:12 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 39 quoted lines

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

Show 6 quoted lines

> 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