{"thread":{"id":"48902","subject":"[PATCH] diff.c: offer config option to control ws handling in move detection","startedAt":"2018-07-18T00:53:17Z","lastAt":"2018-07-19T18:00:47Z","messageCount":7,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"352846","messageId":"20180718005311.136329-1-sbeller@google.com","threadId":"48902","inReplyTo":null,"subject":"[PATCH] diff.c: offer config option to control ws handling in move detection","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-18T00:53:11Z","receivedAt":"2018-07-18T00:53:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller <sbeller@google.com>\n---\n\nThis is the cherry on the cake named sb/diff-color-move-more.\n\n Documentation/config.txt       | 5 +++++\n Documentation/diff-options.txt | 7 +++++--\n diff.c                         | 9 +++++++++\n 3 files changed, 19 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 2659153cb37..6ca7118b018 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1122,6 +1122,11 @@ diff.colorMoved::\n \ttrue the default color mode will be used. When set to false,\n \tmoved lines are not colored.\n \n+diff.colorMovedWS::\n+\tWhen moved lines are colored using e.g. the `diff.colorMoved` setting,\n+\tthis option controls the `<mode>` how spaces are treated\n+\tfor details of valid modes see '--color-moved-ws' in linkgit:git-diff[1].\n+\n color.diff.<slot>::\n \tUse customized color for diff colorization.  `<slot>` specifies\n \twhich part of the patch to use the specified color, and is one\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 143acd9417e..8da7fed4e22 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -294,8 +294,11 @@ dimmed_zebra::\n \n --color-moved-ws=<modes>::\n \tThis configures how white spaces are ignored when performing the\n-\tmove detection for `--color-moved`. These modes can be given\n-\tas a comma separated list:\n+\tmove detection for `--color-moved`.\n+ifdef::git-diff[]\n+\tIt can be set by the `diff.colorMovedWS` configuration setting.\n+endif::git-diff[]\n+\tThese modes can be given as a comma separated list:\n +\n --\n ignore-space-at-eol::\ndiff --git a/diff.c b/diff.c\nindex f51f0ac32f4..9de917108d8 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -35,6 +35,7 @@ static int diff_rename_limit_default = 400;\n static int diff_suppress_blank_empty;\n static int diff_use_color_default = -1;\n static int diff_color_moved_default;\n+static int diff_color_moved_ws_default;\n static int diff_context_default = 3;\n static int diff_interhunk_context_default;\n static const char *diff_word_regex_cfg;\n@@ -332,6 +333,13 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \t\tdiff_color_moved_default = cm;\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"diff.colormovedws\")) {\n+\t\tint cm = parse_color_moved_ws(value);\n+\t\tif (cm < 0)\n+\t\t\treturn -1;\n+\t\tdiff_color_moved_ws_default = cm;\n+\t\treturn 0;\n+\t}\n \tif (!strcmp(var, \"diff.context\")) {\n \t\tdiff_context_default = git_config_int(var, value);\n \t\tif (diff_context_default < 0)\n@@ -4327,6 +4335,7 @@ void diff_setup(struct diff_options *options)\n \t}\n \n \toptions->color_moved = diff_color_moved_default;\n+\toptions->color_moved_ws_handling = diff_color_moved_ws_default;\n }\n \n void diff_setup_done(struct diff_options *options)\n-- \n2.18.0.233.g985f88cf7e-goog\n\n"},{"id":"352928","messageId":"xmqqy3e8jv4x.fsf@gitster-ct.c.googlers.com","threadId":"48902","inReplyTo":"20180718005311.136329-1-sbeller@google.com","subject":"Re: [PATCH] diff.c: offer config option to control ws handling in move detection","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-18T17:45:02Z","receivedAt":"2018-07-18T17:45:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index 143acd9417e..8da7fed4e22 100644\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -294,8 +294,11 @@ dimmed_zebra::\n>  \n>  --color-moved-ws=<modes>::\n>  \tThis configures how white spaces are ignored when performing the\n> -\tmove detection for `--color-moved`. These modes can be given\n> -\tas a comma separated list:\n> +\tmove detection for `--color-moved`.\n> +ifdef::git-diff[]\n> +\tIt can be set by the `diff.colorMovedWS` configuration setting.\n> +endif::git-diff[]\n\nThe patch to diff.c::git_diff_ui_config() we see below does not seem\nto make any effort to make sure that this new configuration applies\nonly to \"git diff\" and that other commands like \"git log\" that call\ngit_diff_ui_config() are not affected.\n\nAnd I do not see a strong reason why \"git log --color-moved\" should\nnot honor this setting, either, so I am not quite sure why we want\nthis ifdef/endif pair to hide it from \"git log --help\".\n\nOr am I totally misunderstanding the reason why we want ifdef/endif\nhere?\n\nPuzzled...\n\n> diff --git a/diff.c b/diff.c\n> index f51f0ac32f4..9de917108d8 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -35,6 +35,7 @@ static int diff_rename_limit_default = 400;\n>  static int diff_suppress_blank_empty;\n>  static int diff_use_color_default = -1;\n>  static int diff_color_moved_default;\n> +static int diff_color_moved_ws_default;\n>  static int diff_context_default = 3;\n>  static int diff_interhunk_context_default;\n>  static const char *diff_word_regex_cfg;\n> @@ -332,6 +333,13 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n>  \t\tdiff_color_moved_default = cm;\n>  \t\treturn 0;\n>  \t}\n> +\tif (!strcmp(var, \"diff.colormovedws\")) {\n> +\t\tint cm = parse_color_moved_ws(value);\n> +\t\tif (cm < 0)\n> +\t\t\treturn -1;\n> +\t\tdiff_color_moved_ws_default = cm;\n> +\t\treturn 0;\n> +\t}\n>  \tif (!strcmp(var, \"diff.context\")) {\n>  \t\tdiff_context_default = git_config_int(var, value);\n>  \t\tif (diff_context_default < 0)\n> @@ -4327,6 +4335,7 @@ void diff_setup(struct diff_options *options)\n>  \t}\n>  \n>  \toptions->color_moved = diff_color_moved_default;\n> +\toptions->color_moved_ws_handling = diff_color_moved_ws_default;\n>  }\n>  \n>  void diff_setup_done(struct diff_options *options)\n"},{"id":"352936","messageId":"CAGZ79kbOxD-GomEYGkOPe9bPT8sgik77hLaTHK1xN9Hk=g+BvA@mail.gmail.com","threadId":"48902","inReplyTo":"xmqqy3e8jv4x.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] diff.c: offer config option to control ws handling in move detection","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-18T18:16:47Z","receivedAt":"2018-07-18T18:17:01Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Jul 18, 2018 at 10:45 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Stefan Beller <sbeller@google.com> writes:\n>\n> > diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> > index 143acd9417e..8da7fed4e22 100644\n> > --- a/Documentation/diff-options.txt\n> > +++ b/Documentation/diff-options.txt\n> > @@ -294,8 +294,11 @@ dimmed_zebra::\n> >\n> >  --color-moved-ws=<modes>::\n> >       This configures how white spaces are ignored when performing the\n> > -     move detection for `--color-moved`. These modes can be given\n> > -     as a comma separated list:\n> > +     move detection for `--color-moved`.\n> > +ifdef::git-diff[]\n> > +     It can be set by the `diff.colorMovedWS` configuration setting.\n> > +endif::git-diff[]\n>\n> The patch to diff.c::git_diff_ui_config() we see below does not seem\n> to make any effort to make sure that this new configuration applies\n> only to \"git diff\" and that other commands like \"git log\" that call\n> git_diff_ui_config() are not affected.\n\nThat is as intended. (We want to have it in git-log)\n\n> And I do not see a strong reason why \"git log --color-moved\" should\n> not honor this setting, either, so I am not quite sure why we want\n> this ifdef/endif pair to hide it from \"git log --help\".\n>\n> Or am I totally misunderstanding the reason why we want ifdef/endif\n> here?\n>\n> Puzzled...\n\nI am somewhat puzzled, too, by the use of ifdefs in\nDocumentation/diff-options.txt, but I rationalized it as\n\"man git diff\" has all the details, whereas the other pages are\na bit shorter and concise:\n\n$ git grep -A 2 \"ifdef::git-diff\" Documentation/diff-options.txt\nifdef::git-diff[]\n     This is the default.\nendif::git-diff[]\n\nifdef::git-diff-core[]\n     This is the default.\nendif::git-diff-core[]\n\nifdef::git-diff[]\n     It can be changed by the `color.ui` and `color.diff`\n     configuration settings.\n\nifdef::git-diff[]\n     This can be used to override configuration settings.\nendif::git-diff[]\n\nifdef::git-diff[]\n     It can be changed by the `diff.colorMoved` configuration setting.\nendif::git-diff[]\n\nifdef::git-diff[]\n     It can be set by the `diff.colorMovedWS` configuration setting.\nendif::git-diff[]\n\nSo I followed the style that was already there, specifically\n61e89eaae88 (diff: document the new --color-moved setting, 2017-06-30)\nwhich followed the style of\n6999c54029b (config.txt,diff-options.txt: porcelain vs. plumbing for\ncolor.diff, 2011-04-27)\n\nSo I might have picked up a bad habit there or misunderstood the original?\n\nStefan\n"},{"id":"353032","messageId":"xmqqtvovgped.fsf@gitster-ct.c.googlers.com","threadId":"48902","inReplyTo":"CAGZ79kbOxD-GomEYGkOPe9bPT8sgik77hLaTHK1xN9Hk=g+BvA@mail.gmail.com","subject":"Re: [PATCH] diff.c: offer config option to control ws handling in move detection","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-19T16:29:30Z","receivedAt":"2018-07-19T16:29:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On Wed, Jul 18, 2018 at 10:45 AM Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>> > diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n>> > index 143acd9417e..8da7fed4e22 100644\n>> > --- a/Documentation/diff-options.txt\n>> > +++ b/Documentation/diff-options.txt\n>> > @@ -294,8 +294,11 @@ dimmed_zebra::\n>> >\n>> >  --color-moved-ws=<modes>::\n>> >       This configures how white spaces are ignored when performing the\n>> > -     move detection for `--color-moved`. These modes can be given\n>> > -     as a comma separated list:\n>> > +     move detection for `--color-moved`.\n>> > +ifdef::git-diff[]\n>> > +     It can be set by the `diff.colorMovedWS` configuration setting.\n>> > +endif::git-diff[]\n>>\n>> The patch to diff.c::git_diff_ui_config() we see below does not seem\n>> to make any effort to make sure that this new configuration applies\n>> only to \"git diff\" and that other commands like \"git log\" that call\n>> git_diff_ui_config() are not affected.\n>\n> That is as intended. (We want to have it in git-log)\n\nAh, I think what is going on here, and I think I asked a wrong\nquestion.\n\n * diff-options.txt is used by the family of diff plumbing commands\n   (which actively do not want to participate in the coloring game\n   and do not call git_diff_ui_config()) as well as log family of\n   commands (which do pay attention to the config).\n\n * \"git grep '^:git-diff:'\" hits only Documentation/git-diff.txt.\n\nWhat the system currently does (which may not match what it should\ndo) is that Porcelain \"diff\", \"log\", etc. pay attention to the\nconfiguration while plumbing \"diff-{files,index,tree}\" don't, and\nuse of ifdef/endif achieves only half of that (i.e. excludes the\nsentence from plumbing manual pages).  It excludes too much and does\nnot say \"log\", \"show\", etc. also honor the configuration.\n\nI think the set of asciidoc attrs diff-options.txt files uses need\nsome serious clean-up.  For example, it defines :git-diff-core: that\nis only used once, whose intent seems to be \"we are describing diff\nplumbing\".  However, the way it does so is by excluding known\nPorcelain; if we ever add new diff porcelain (e.g. \"range-diff\"),\nthat way of 'definition by exclusion' would break.  The scheme is\nalready broken by forcing git-show.txt to define 'git-log' just like\ngit-log.txt does, meaning that it is not possible to make \"show\" to\nbe described differently from \"log\".  But let's leave that outside\nthis topic.\n\nBack to a more on-topic tangent.\n\nHow does this patch (and all the recent \"color\" options you added\nrecently) avoid spending unnecessary cycles and contaminating \"git\nformat-patch\" output, by the way?  builtin/log.c::cmd_format_patch()\nseems to eventually call into git_log_config() that ends with a call\nto diff_ui_config().\n\nThanks.\n"},{"id":"353047","messageId":"CAGZ79kZvHvMy6Lf-_Dtzbfaz2BFY2WonOfw-dDQVCOnS-a0=sA@mail.gmail.com","threadId":"48902","inReplyTo":"xmqqtvovgped.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] diff.c: offer config option to control ws handling in move detection","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-19T17:29:21Z","receivedAt":"2018-07-19T17:29:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jul 19, 2018 at 9:29 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Stefan Beller <sbeller@google.com> writes:\n>\n> > On Wed, Jul 18, 2018 at 10:45 AM Junio C Hamano <gitster@pobox.com> wrote:\n> >>\n> >> Stefan Beller <sbeller@google.com> writes:\n> >>\n> >> > diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> >> > index 143acd9417e..8da7fed4e22 100644\n> >> > --- a/Documentation/diff-options.txt\n> >> > +++ b/Documentation/diff-options.txt\n> >> > @@ -294,8 +294,11 @@ dimmed_zebra::\n> >> >\n> >> >  --color-moved-ws=<modes>::\n> >> >       This configures how white spaces are ignored when performing the\n> >> > -     move detection for `--color-moved`. These modes can be given\n> >> > -     as a comma separated list:\n> >> > +     move detection for `--color-moved`.\n> >> > +ifdef::git-diff[]\n> >> > +     It can be set by the `diff.colorMovedWS` configuration setting.\n> >> > +endif::git-diff[]\n> >>\n> >> The patch to diff.c::git_diff_ui_config() we see below does not seem\n> >> to make any effort to make sure that this new configuration applies\n> >> only to \"git diff\" and that other commands like \"git log\" that call\n> >> git_diff_ui_config() are not affected.\n> >\n> > That is as intended. (We want to have it in git-log)\n>\n> Ah, I think what is going on here, and I think I asked a wrong\n> question.\n>\n>  * diff-options.txt is used by the family of diff plumbing commands\n>    (which actively do not want to participate in the coloring game\n>    and do not call git_diff_ui_config()) as well as log family of\n>    commands (which do pay attention to the config).\n>\n>  * \"git grep '^:git-diff:'\" hits only Documentation/git-diff.txt.\n>\n> What the system currently does (which may not match what it should\n> do) is that Porcelain \"diff\", \"log\", etc. pay attention to the\n> configuration while plumbing \"diff-{files,index,tree}\" don't, and\n> use of ifdef/endif achieves only half of that (i.e. excludes the\n> sentence from plumbing manual pages).  It excludes too much and does\n> not say \"log\", \"show\", etc. also honor the configuration.\n>\n> I think the set of asciidoc attrs diff-options.txt files uses need\n> some serious clean-up.  For example, it defines :git-diff-core: that\n> is only used once, whose intent seems to be \"we are describing diff\n> plumbing\".  However, the way it does so is by excluding known\n> Porcelain; if we ever add new diff porcelain (e.g. \"range-diff\"),\n> that way of 'definition by exclusion' would break.  The scheme is\n> already broken by forcing git-show.txt to define 'git-log' just like\n> git-log.txt does, meaning that it is not possible to make \"show\" to\n> be described differently from \"log\".  But let's leave that outside\n> this topic.\n\nThen let's call this #leftoverbits ?\n\n> Back to a more on-topic tangent.\n>\n> How does this patch (and all the recent \"color\" options you added\n> recently) avoid spending unnecessary cycles and contaminating \"git\n> format-patch\" output, by the way?  builtin/log.c::cmd_format_patch()\n> seems to eventually call into git_log_config() that ends with a call\n> to diff_ui_config().\n\nThe color options are only using CPU cycles when we actually need to\ncolor things (if no-color is set, then the move detection is off).\n\n\"git format-patch HEAD^ --color\" produces red/green output\n(like git log would), so I do not see why --color-moved should impact\nformat-patch differently. (i.e. if the user requests format-patch with\ncolor-moved we can do it, otherwise, when colors are off, we do not\nspend cycles to compute the moves)\n\nMaybe I did not understand the gist of your question, still?\nStefan\n"},{"id":"353049","messageId":"xmqq601bgmh3.fsf@gitster-ct.c.googlers.com","threadId":"48902","inReplyTo":"CAGZ79kZvHvMy6Lf-_Dtzbfaz2BFY2WonOfw-dDQVCOnS-a0=sA@mail.gmail.com","subject":"Re: [PATCH] diff.c: offer config option to control ws handling in move detection","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-19T17:32:40Z","receivedAt":"2018-07-19T17:32:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> \"git format-patch HEAD^ --color\" produces red/green output\n> (like git log would), so I do not see why --color-moved should impact\n> format-patch differently. (i.e. if the user requests format-patch with\n> color-moved we can do it, otherwise, when colors are off, we do not\n> spend cycles to compute the moves)\n>\n> Maybe I did not understand the gist of your question, still?\n\n\"format-patch --color\" done by end-user, especially when combined\nwith \"--stdout\", would be useful to show coloured output.  That is\nwhat you are talking about in the above, but that is not what I was\nasking about.\n\nThe question was specifically about configuration.  You may say\n\"diff.colorwhatever = on\", but \"git format-patch\" with no command\nline override wouldn't want to be affected by that and produce a\npatch that won't apply, I would think.\n\n"},{"id":"353052","messageId":"CAGZ79kYo_zFfvX1LHod47BND=GbmzeaBfxhLbX_nmCPx0UATZQ@mail.gmail.com","threadId":"48902","inReplyTo":"xmqq601bgmh3.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] diff.c: offer config option to control ws handling in move detection","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-19T18:00:34Z","receivedAt":"2018-07-19T18:00:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jul 19, 2018 at 10:32 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Stefan Beller <sbeller@google.com> writes:\n>\n> > \"git format-patch HEAD^ --color\" produces red/green output\n> > (like git log would), so I do not see why --color-moved should impact\n> > format-patch differently. (i.e. if the user requests format-patch with\n> > color-moved we can do it, otherwise, when colors are off, we do not\n> > spend cycles to compute the moves)\n> >\n> > Maybe I did not understand the gist of your question, still?\n>\n> \"format-patch --color\" done by end-user, especially when combined\n> with \"--stdout\", would be useful to show coloured output.  That is\n> what you are talking about in the above, but that is not what I was\n> asking about.\n>\n> The question was specifically about configuration.  You may say\n> \"diff.colorwhatever = on\", but \"git format-patch\" with no command\n> line override wouldn't want to be affected by that and produce a\n> patch that won't apply, I would think.\n\nThe options introduced here (even the command line arguments)\ndo *nothing* if no color is on, i.e.\n\n    git diff --no-color --color-moved=blocks \\\n        --color-moved-ws=allow-indentation-change\n\nis the same as\n\n    git diff --no-color\n\nand as format-patch doesn't use colors by default giving\ncolor-moved settings (even via config) is a no-op, too?\n\nStefan\n"}]}