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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 14, 2018, 07:27 UTC
Message-ID: <xmqqa7mcnmoz.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20181113213357.205769-1-sbeller@google.com>

Stefan Beller <sbeller@google.com> 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.

Show 9 quoted lines

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

Show 8 quoted lines

>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> 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.