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

[Stefan Beller <sbeller@google.com>]

On Thu, Jul 19, 2018 at 9:29 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Stefan Beller <sbeller@google.com> writes:
>
> > 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.

Then let's call this #leftoverbits ?

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

The color options are only using CPU cycles when we actually need to
color things (if no-color is set, then the move detection is off).

"git format-patch HEAD^ --color" produces red/green output
(like git log would), so I do not see why --color-moved should impact
format-patch differently. (i.e. if the user requests format-patch with
color-moved we can do it, otherwise, when colors are off, we do not
spend cycles to compute the moves)

Maybe I did not understand the gist of your question, still?
Stefan