Re: [PATCH] diff.c: offer config option to control ws handling in move detection

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 19, 2018, 16:29 UTC
Message-ID: <xmqqtvovgped.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CAGZ79kbOxD-GomEYGkOPe9bPT8sgik77hLaTHK1xN9Hk=g+BvA@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

Show 25 quoted lines

> On Wed, Jul 18, 2018 at 10:45 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Stefan Beller <sbeller@google.com> writes:
>>
>> > diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
>> > index 143acd9417e..8da7fed4e22 100644
>> > --- a/Documentation/diff-options.txt
>> > +++ b/Documentation/diff-options.txt
>> > @@ -294,8 +294,11 @@ dimmed_zebra::
>> >
>> >  --color-moved-ws=<modes>::
>> >       This configures how white spaces are ignored when performing the
>> > -     move detection for `--color-moved`. These modes can be given
>> > -     as a comma separated list:
>> > +     move detection for `--color-moved`.
>> > +ifdef::git-diff[]
>> > +     It can be set by the `diff.colorMovedWS` configuration setting.
>> > +endif::git-diff[]
>>
>> The patch to diff.c::git_diff_ui_config() we see below does not seem
>> to make any effort to make sure that this new configuration applies
>> only to "git diff" and that other commands like "git log" that call
>> git_diff_ui_config() are not affected.
>
> That is as intended. (We want to have it in git-log)

Ah, I think what is going on here, and I think I asked a wrong question.

 * diff-options.txt is used by the family of diff plumbing commands
   (which actively do not want to participate in the coloring game
   and do not call git_diff_ui_config()) as well as log family of
   commands (which do pay attention to the config).

 * "git grep '^:git-diff:'" hits only Documentation/git-diff.txt.

What the system currently does (which may not match what it should do) is that Porcelain "diff", "log", etc. pay attention to the configuration while plumbing "diff-{files,index,tree}" don't, and use of ifdef/endif achieves only half of that (i.e. excludes the sentence from plumbing manual pages). It excludes too much and does not say "log", "show", etc. also honor the configuration.

I think the set of asciidoc attrs diff-options.txt files uses need some serious clean-up. For example, it defines :git-diff-core: that is only used once, whose intent seems to be "we are describing diff plumbing". However, the way it does so is by excluding known Porcelain; if we ever add new diff porcelain (e.g. "range-diff"), that way of 'definition by exclusion' would break. The scheme is already broken by forcing git-show.txt to define 'git-log' just like git-log.txt does, meaning that it is not possible to make "show" to be described differently from "log". But let's leave that outside this topic.

Back to a more on-topic tangent.

How does this patch (and all the recent "color" options you added recently) avoid spending unnecessary cycles and contaminating "git format-patch" output, by the way? builtin/log.c::cmd_format_patch() seems to eventually call into git_log_config() that ends with a call to diff_ui_config().

Thanks.