| Message ID | 20181102212316.208433-1-sbeller@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | diff: differentiate error handling in parse_color_moved_ws | expand |
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? Thanks.
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. 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, not so excellent (yet) X-<.
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
diff --git a/diff.c b/diff.c index 8647db3d30..f21f8b0332 100644 --- a/diff.c +++ b/diff.c @@ -291,7 +291,7 @@ static int parse_color_moved(const char *arg) return error(_("color moved setting must be one of 'no', 'default', 'blocks', 'zebra', 'dimmed-zebra', 'plain'")); } -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); + } strbuf_release(&sb); } if ((ret & COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE) && - (ret & XDF_WHITESPACE_FLAGS)) - die(_("color-moved-ws: allow-indentation-change cannot be combined with other white space modes")); + (ret & XDF_WHITESPACE_FLAGS)) { + error(_("color-moved-ws: allow-indentation-change cannot be combined with other white space modes")); + ret |= COLOR_MOVED_WS_ERROR; + } string_list_clear(&l, 0); @@ -341,8 +345,8 @@ int git_diff_ui_config(const char *var, const char *value, void *cb) return 0; } if (!strcmp(var, "diff.colormovedws")) { - int cm = parse_color_moved_ws(value); - if (cm < 0) + unsigned cm = parse_color_moved_ws(value); + if (cm & COLOR_MOVED_WS_ERROR) return -1; diff_color_moved_ws_default = cm; return 0; @@ -5035,7 +5039,10 @@ int diff_opt_parse(struct diff_options *options, die("bad --color-moved argument: %s", arg); options->color_moved = cm; } 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; } else if (skip_to_optional_arg_default(arg, "--color-words", &options->word_regex, NULL)) { options->use_color = 1; options->word_diff = DIFF_WORDS_COLOR; diff --git a/diff.h b/diff.h index ce5e8a8183..9e8061ca29 100644 --- a/diff.h +++ b/diff.h @@ -225,7 +225,8 @@ struct diff_options { /* XDF_WHITESPACE_FLAGS regarding block detection are set at 2, 3, 4 */ #define COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE (1<<5) - int color_moved_ws_handling; + #define COLOR_MOVED_WS_ERROR (1<<0) + unsigned color_moved_ws_handling; struct repository *repo; };
As we check command line options more strictly and allow configuration variables to be parsed more leniently, we need take different actions based on whether an unknown value is given on the command line or in the config. Move the die() call out of parse_color_moved_ws into the parsing of command line options. As the function returns a bit field, change its signature to return an unsigned instead of an int; add a new bit to signal errors. Once the error is signaled, we discard the other bits, such that it doesn't matter if the error bit overlaps with any other bit. Signed-off-by: Stefan Beller <sbeller@google.com> --- This is a fresh attempt to cleanup the sloppy part that was mentioned in https://public-inbox.org/git/xmqqa7nkf6o4.fsf@gitster-ct.c.googlers.com/ Another thing to follow up is to have color-moved-ws imply color-moved. Thanks, Stefan diff.c | 21 ++++++++++++++------- diff.h | 3 ++- 2 files changed, 16 insertions(+), 8 deletions(-)