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

From: Stefan Beller <sbeller@google.com>
Date: Jul 18, 2018, 18:16 UTC
Message-ID: <CAGZ79kbOxD-GomEYGkOPe9bPT8sgik77hLaTHK1xN9Hk=g+BvA@mail.gmail.com>
In-Reply-To: <xmqqy3e8jv4x.fsf@gitster-ct.c.googlers.com>

On Wed, Jul 18, 2018 at 10:45 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 22 quoted lines

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

Show 8 quoted lines

> And I do not see a strong reason why "git log --color-moved" should
> not honor this setting, either, so I am not quite sure why we want
> this ifdef/endif pair to hide it from "git log --help".
>
> Or am I totally misunderstanding the reason why we want ifdef/endif
> here?
>
> Puzzled...

I am somewhat puzzled, too, by the use of ifdefs in Documentation/diff-options.txt, but I rationalized it as "man git diff" has all the details, whereas the other pages are a bit shorter and concise:

$ git grep -A 2 "ifdef::git-diff" Documentation/diff-options.txt
ifdef::git-diff[]
     This is the default.
endif::git-diff[]

ifdef::git-diff-core[]
     This is the default.
endif::git-diff-core[]

ifdef::git-diff[]
     It can be changed by the `color.ui` and `color.diff`
     configuration settings.

ifdef::git-diff[]
     This can be used to override configuration settings.
endif::git-diff[]

ifdef::git-diff[]
     It can be changed by the `diff.colorMoved` configuration setting.
endif::git-diff[]

ifdef::git-diff[]
     It can be set by the `diff.colorMovedWS` configuration setting.
endif::git-diff[]

So I followed the style that was already there, specifically 61e89eaae88 (diff: document the new --color-moved setting, 2017-06-30) which followed the style of 6999c54029b (config.txt,diff-options.txt: porcelain vs. plumbing for color.diff, 2011-04-27)

So I might have picked up a bad habit there or misunderstood the original?

Stefan