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

Stefan Beller [off-list ref] 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 <redacted>
---

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.