Re: [PATCH] diff: align move detection error handling with other options

[Junio C Hamano]

Stefan Beller  writes:

>Subject: Re: [PATCH] diff: align move detection error handling with other 
>options

When sending an updated version of existing topic, please make sure
you indicate as such with v$n etc.  I will assume that this is to
replace the patch queued on sb/diff-color-moved-config-option-fixup
topic.  Please do not assume that all messages on the References:
header are visible in the readers' MUA to show which thread it is a
response to.  At least a hint like v$n (or mentioning the name of
the topic branch if the previous round is already in 'pu') would
make the reader realize that the References: header can be used if
the reader wants to find in what context the patch is relevant,
especially when redoing a change whose previous round is more than a
week old, as we see too many changes in a day already.

> This changes the error handling for the options --color-moved-ws
> and --color-moved-ws to be like the rest of the options.
>
> 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.

OK.  That sound better than the original.

>
> Signed-off-by: Stefan Beller 
> ---
>
> c.f.
> ./git -c diff.colormovedws=bogus diff HEAD
> error: unknown color-moved-ws mode 'bogus'
> fatal: unable to parse 'diff.colormovedws' from command-line config

These double messages may be something we want to fix eventually,
but I think that the issue is not specific to the config callback of
diff API, but something the config API needs to support to allow its
users produce a better single message (the first part is merely
giving a more detailed explanation why Git was unable to parse, and
should ideally be folded into the second part).

More generally, even if git_diff_ui_config() was called, as long as
we do not do the "--color-moved" and "--color-moved-ws" operations,
the user shouldn't even get an "unknown mode" message, let alone
"fatal".  The above (and other existing uses of "return -1"s in the
same config callback) should actually become an example of what not
to do, as "diff HEAD" does not *care* what value that variable has.

But again, fixing that anti-pattern is a much larger change.  Once
we move diff.c to the more modern git_config_get*() based interface,
instead of the old style git_config() callback based interface, it
would become easier to update it so that the complaint will be given
(and kill the command) only when a variable whose value actually
_matters_ is unparsable.

Thanks.

[PATCH] diff: align move detection error handling with other options

[Stefan Beller]

This changes the error handling for the options --color-moved-ws
and --color-moved-ws to be like the rest of the options.

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

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

./git diff --color=bogus
error: option `color' expects "always", "auto", or "never"
./git -c diff.colormovedws=bogus diff HEAD
error: unknown color-moved-ws mode 'bogus'
fatal: unable to parse 'diff.colormovedws' from command-line config


 diff.c | 25 -
 diff.h |  3 ++-
 t/t4015-diff-whitespace.sh | 18 ++
 3 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/diff.c b/diff.c
index 8647db3d30..d7d467b605 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
-   error(_("ignoring unknown color-moved-ws mode '%s'"), 
sb.buf);
+   else {
+   ret |= COLOR_MOVED_WS_ERROR;
+   error(_("unknown color-moved-ws mode '%s', possible 
values are 'ignore-space-change', 'ignore-space-at-eol', 'ignore-all-space', 
'allow-indentation-change'"), sb.buf);
+   }
 
strbuf_release();
}
 
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(, 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;
@@ -5032,10 +5036,13 @@ int diff_opt_parse(struct diff_options *options,
else if (skip_prefix(arg, "--color-moved=", )) {
int cm = parse_color_moved(arg);
if (cm < 0)
-   die("bad --color-moved argument: %s", arg);
+   return error("bad --color-moved argument: %s", arg);
options->color_moved = cm;
} else if (skip_prefix(arg, "--color-moved-ws=", )) {
-   options->color_moved_ws_handling = parse_color_moved_ws(arg);
+   unsigned cm = parse_color_moved_ws(arg);
+   if (cm & COLOR_MOVED_WS_ERROR)
+   return -1;
+   options->color_moved_ws_handling = cm;
} else if (skip_to_optional_arg_default(arg, "--color-words", 
>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;
 };
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index a9fb226c5a..9a3e4fdfec 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -1890,6 +1890,24 @@ test_expect_success 'compare whitespace delta across 
moved blocks' '