BUG: diff-{index,files,tree} (and git-gui) do not respect the diff.indentHeuristic config setting

BUG: diff-{index,files,tree} (and git-gui) do not respect the diff.indentHeuristic config setting

[Marc Branchaud]

So I have

	diff.indentHeuristic = true

and I noticed that git-gui was not using the heuristic.  This is because 
git-gui uses diff-index, and that does not respect the config setting, 
even though it supports the --indent-heuristic option.

And it looks like diff-files and diff-tree also have the same problem.

I tried a couple of quick-n-dirty things to fix it in diff-index, 
without success, and I've run out of git-hacking tame, so all I can do 
for now is throw out a bug report.

diff-index.c explicitly says "no 'diff' UI options" since 83ad63cfeb 
("diff: do not use configuration magic at the core-level", 2006-07-08), 
so maybe this needs to be fixed in git-gui (and maybe elsewhere), but to 
me it feels like the diff-foo commands should respect the setting.

(CC'ing Michael Haggerty, who added the heuristic.)

(This is git v2.12.2.)

		M.

Re: BUG: diff-{index,files,tree} (and git-gui) do not respect the diff.indentHeuristic config setting

[Jeff King]

On Tue, Apr 25, 2017 at 01:21:09PM -0400, Marc Branchaud wrote:

> So I have
> 
> 	diff.indentHeuristic = true
> 
> and I noticed that git-gui was not using the heuristic.  This is because
> git-gui uses diff-index, and that does not respect the config setting, even
> though it supports the --indent-heuristic option.
> 
> And it looks like diff-files and diff-tree also have the same problem.
> 
> I tried a couple of quick-n-dirty things to fix it in diff-index, without
> success, and I've run out of git-hacking tame, so all I can do for now is
> throw out a bug report.

Right, this is intentional. Those commands are scriptable plumbing, and
we try to avoid surprising their callers with behavior-changing config.

I could see an argument that the behavior change for this particular
option is subtle enough that it any script caller would not need to
care. The resulting diff is syntactically identical, and it's not like
Git makes promises about the exact diff algorithm it uses. We generally
tend to err on the side of caution with plumbing, though (for instance,
if you piped the result through patch-id, you'd probably get different
results with and without the config option set).

> diff-index.c explicitly says "no 'diff' UI options" since 83ad63cfeb ("diff:
> do not use configuration magic at the core-level", 2006-07-08), so maybe
> this needs to be fixed in git-gui (and maybe elsewhere), but to me it feels
> like the diff-foo commands should respect the setting.

Yes, if git-gui wants to respect the option, it needs to read the config
and turn on the command-line option. That's what add--interactive does,
for example.

It's possible we could make this simpler with a catch-all "allow diff
ui config" command-line option. The effect is the same, but it makes
things simpler for the caller.

-Peff

[PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.

[Marc Branchaud]

So here's my attempt at fixing this.

The thing I was missing is that init_revisions() calls diff_setup(), which
sets the xdl options.  It's therefore necessary to have the
diff_indent_heuristic flag set before calling init_revisions().

A naive way to get the indentHeuristic config option respected in the
diff-* plumbing commands is to make them use the git_diff_heuristic_config()
callback right at the start of their main cmd functions.

But I did not like that for two reasons:

* It would make these commands invoke git_config() twice.

* It doesn't avoid the problem if/when someone creates a new diff-something
  plumbing command, and forgets to set the diff_indent_heuristic flag before
  calling init_revisions().

So instead I chose to make the indentHeuristic option part of diff's basic
configuration, and in each of the diff plumbing commands I moved the call to
git_config() before the call to init_revisions().

This still doesn't really future-proof things for possible new diff plumbing
commands, because someone could still invoke init_revisions() before setting
up diff's basic configuration.  But I don't see an obvious way of ensuring
that the diff_indent_heuristic flag is respected regardless of when
diff_setup() is invoked.

		M.

Marc Branchaud (2):
  Make the indent heuristic part of diff's basic configuration.
  Have the diff-* builtins configure diff before initializing revisions.

 builtin/diff-files.c | 2 +-
 builtin/diff-index.c | 2 +-
 builtin/diff-tree.c  | 2 +-
 diff.c               | 6 +++---
 4 files changed, 6 insertions(+), 6 deletions(-)

[PATCH 1/2] Make the indent heuristic part of diff's basic configuration.

[Marc Branchaud]

Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
---
 diff.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/diff.c b/diff.c
index 11eef1c85..da96577ea 100644
--- a/diff.c
+++ b/diff.c
@@ -290,9 +290,6 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	if (git_diff_heuristic_config(var, value, cb) < 0)
-		return -1;
-
 	if (!strcmp(var, "diff.wserrorhighlight")) {
 		int val = parse_ws_error_highlight(value);
 		if (val < 0)
@@ -351,6 +348,9 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
 	if (starts_with(var, "submodule."))
 		return parse_submodule_config_option(var, value);
 
+	if (git_diff_heuristic_config(var, value, cb) < 0)
+		return -1;
+
 	return git_default_config(var, value, cb);
 }
 
-- 
2.13.0.rc1.15.g7dbea34e1.dirty

Re: [PATCH 1/2] Make the indent heuristic part of diff's basic configuration.

[Jeff King]

On Thu, Apr 27, 2017 at 04:50:36PM -0400, Marc Branchaud wrote:

> Subject: [PATCH 1/2] Make the indent heuristic part of diff's basic configuration.
>
> Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>

This needs to be explained better. Why were the options originally in
UI config, and why is it OK to move them to the basic config?

I think the argument is along the lines of "scripts shouldn't care
because the result is syntactically identical, and we reserve the right
to find any valid diff".

Also, our subject lines usually do not start with a capital, nor end
with a period. We usually try to specify the subsystem with a colon.
Like:

  diff: move indent heuristic to "basic" config

or similar.

-Peff

[PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.

[Marc Branchaud]

This makes the commands respect diff configuration options, such as
indentHeuristic.

Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
---
 builtin/diff-files.c | 2 +-
 builtin/diff-index.c | 2 +-
 builtin/diff-tree.c  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index 15c61fd8d..a572da9d5 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -20,9 +20,9 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 	int result;
 	unsigned options = 0;
 
+	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	init_revisions(&rev, prefix);
 	gitmodules_config();
-	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	rev.abbrev = 0;
 	precompose_argv(argc, argv);
 
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index 1af373d00..f084826a2 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -17,9 +17,9 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 	int i;
 	int result;
 
+	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	init_revisions(&rev, prefix);
 	gitmodules_config();
-	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	rev.abbrev = 0;
 	precompose_argv(argc, argv);
 
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 326f88b65..36a3a1976 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -105,9 +105,9 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 	struct setup_revision_opt s_r_opt;
 	int read_stdin = 0;
 
+	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	init_revisions(opt, prefix);
 	gitmodules_config();
-	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	opt->abbrev = 0;
 	opt->diff = 1;
 	opt->disable_stdin = 1;
-- 
2.13.0.rc1.15.g7dbea34e1.dirty

Re: [PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.

[Jeff King]

On Thu, Apr 27, 2017 at 04:50:37PM -0400, Marc Branchaud wrote:

> Subject: [PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.
>
> This makes the commands respect diff configuration options, such as
> indentHeuristic.
> 
> Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>

I think it would be helpful to future readers to explain what is going
on here. I.e., the bit about calling diff_setup_done(), which copies the
globals into the diff struct.

The same comments about the subject line from the last patch apply here,
too.

>  builtin/diff-files.c | 2 +-
>  builtin/diff-index.c | 2 +-
>  builtin/diff-tree.c  | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)

It would be nice to have a test. Testing that dirstat's permille option
has an effect might be the easiest way to do so.

> diff --git a/builtin/diff-files.c b/builtin/diff-files.c
> index 15c61fd8d..a572da9d5 100644
> --- a/builtin/diff-files.c
> +++ b/builtin/diff-files.c
> @@ -20,9 +20,9 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
>  	int result;
>  	unsigned options = 0;
>  
> +	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
>  	init_revisions(&rev, prefix);
>  	gitmodules_config();
> -	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
>  	rev.abbrev = 0;
>  	precompose_argv(argc, argv);

It's somewhat odd to me that diff_files uses a rev_info struct at all.
It doesn't traverse at all, and doesn't respect most of the options.
There's a half-hearted attempt to reject some obviously bogus cases, but
most of the options are just silently ignored.

I think it's mostly a historical wart (especially around the fact that
some diff options like combine_merges are in rev_info, which they
probably should not be). Anyway, none of that is your problem, and is
way outside the scope of this patch.

-Peff

Re: [PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Apr 27, 2017 at 04:50:37PM -0400, Marc Branchaud wrote:
>
>> Subject: [PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.
>>
>> This makes the commands respect diff configuration options, such as
>> indentHeuristic.
>> 
>> Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
>
> I think it would be helpful to future readers to explain what is going
> on here. I.e., the bit about calling diff_setup_done(), which copies the
> globals into the diff struct.
>
>...
>
>> +	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
>>  	init_revisions(&rev, prefix);
>>  	gitmodules_config();
>> -	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
>>  	rev.abbrev = 0;
>>  	precompose_argv(argc, argv);

This somehow made me worried at the first glance, but the matches
what is done by the Porcelain "git diff".  In other words, it was a
bug that the original called init_revisions() before doing the diff
configuration, and it does not have much to do with "now let's have
them honor indentHeuristic".  Even if we wanted to keep the indent
heuristic in the ui-config (not basic) section of the diff tweak
knobs, we should be applying this patch as a bugfix.

> It's somewhat odd to me that diff_files uses a rev_info struct at all.
> It doesn't traverse at all, and doesn't respect most of the options.
> There's a half-hearted attempt to reject some obviously bogus cases, but
> most of the options are just silently ignored.
>
> I think it's mostly a historical wart (especially around the fact that
> some diff options like combine_merges are in rev_info, which they
> probably should not be). Anyway, none of that is your problem, and is
> way outside the scope of this patch.

Yeah.  The underlying diff machinery was built to be easily usable
by revision traversal and that is probably the reason why we have
this entanglement that we probably could (and maybe we would want
to) detangle.  Surely not a theme of this topic.

Thanks.

Re: [PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.

[Jeff King]

On Sun, Apr 30, 2017 at 06:01:37PM -0700, Junio C Hamano wrote:

> >> +	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
> >>  	init_revisions(&rev, prefix);
> >>  	gitmodules_config();
> >> -	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
> >>  	rev.abbrev = 0;
> >>  	precompose_argv(argc, argv);
> 
> This somehow made me worried at the first glance, but the matches
> what is done by the Porcelain "git diff".  In other words, it was a
> bug that the original called init_revisions() before doing the diff
> configuration, and it does not have much to do with "now let's have
> them honor indentHeuristic".  Even if we wanted to keep the indent
> heuristic in the ui-config (not basic) section of the diff tweak
> knobs, we should be applying this patch as a bugfix.

Yeah, I agree it is an existing bug. The only other case I found is
dirstat. Doing:

  mkdir a b
  for i in 1 2; do echo content >a/$i; done
  for i in 1 2 3; do echo content >b/$i; done
  git -c diff.dirstat=50 diff-tree --dirstat --root HEAD

should respect the config and show only "b", but it doesn't currently
(and does with Marc's patch).

-Peff

Re: [PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.

[Jeff King]

On Mon, May 01, 2017 at 01:17:58AM -0400, Jeff King wrote:

> Yeah, I agree it is an existing bug. The only other case I found is
> dirstat. Doing:
> 
>   mkdir a b
>   for i in 1 2; do echo content >a/$i; done
>   for i in 1 2 3; do echo content >b/$i; done
>   git -c diff.dirstat=50 diff-tree --dirstat --root HEAD
> 
> should respect the config and show only "b", but it doesn't currently
> (and does with Marc's patch).

Er, there's a "git add a b; git commit -m foo" missing before running
the diff, of course.

-Peff

Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.

[Jeff King]

On Thu, Apr 27, 2017 at 04:50:35PM -0400, Marc Branchaud wrote:

> So here's my attempt at fixing this.
> 
> The thing I was missing is that init_revisions() calls diff_setup(), which
> sets the xdl options.  It's therefore necessary to have the
> diff_indent_heuristic flag set before calling init_revisions().
> 
> A naive way to get the indentHeuristic config option respected in the
> diff-* plumbing commands is to make them use the git_diff_heuristic_config()
> callback right at the start of their main cmd functions.
> 
> But I did not like that for two reasons:
> 
> * It would make these commands invoke git_config() twice.
> 
> * It doesn't avoid the problem if/when someone creates a new diff-something
>   plumbing command, and forgets to set the diff_indent_heuristic flag before
>   calling init_revisions().

Yeah, I think that would be the wrong way to go. Either this option is
diff_basic_config() or it is not. And if it is not, the plumbing commands
have no business checking it.

Thinking on it more, I think it probably should be basic config. We
discussed all along making these options the default, which shows that
plumbing interfaces do not need to be protected from them.

> So instead I chose to make the indentHeuristic option part of diff's basic
> configuration, and in each of the diff plumbing commands I moved the call to
> git_config() before the call to init_revisions().

Yes, I think that's the right thing to do.

> This still doesn't really future-proof things for possible new diff plumbing
> commands, because someone could still invoke init_revisions() before setting
> up diff's basic configuration.  But I don't see an obvious way of ensuring
> that the diff_indent_heuristic flag is respected regardless of when
> diff_setup() is invoked.

I think the config-based stuff would have to be bumped down to
setup_revisions(), leaving init_revisions() as a initialization
function. But that would break new cases, as some callers would
want to do:

  init_revisions(&revs);
  revs.foo = 1; /* override any config */
  setup_revisions(argc, argv, &revs); /* let command-line override us */

So you'd really need three phases:

  init_revisions(&revs); /* baked-in defaults */
  setup_revision_config(&revs);
  setup_revision_args(argc, argv, &revs);

and you could override each set of defaults in between the calls.  But
that would require tweaking each caller. In practice, I think the rule
"set up your config before calling init_revisions" is probably OK, now
that we know about it.

I think there is existing breakage in any case where a diff_basic_config
option is handled in diff_setup(). The only one I see is the dirstat
stuff. So I think:

  git config diff.dirstat changes,5
  git rev-list HEAD | git diff-tree --stdin --dirstat

was supposed to respect that "5" but doesn't. I didn't test, though.

-Peff

Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.

[Stefan Beller]

On Thu, Apr 27, 2017 at 1:50 PM, Marc Branchaud <marcnarc@xiplink.com> wrote:
> So here's my attempt at fixing this.
>
> The thing I was missing is that init_revisions() calls diff_setup(), which
> sets the xdl options.  It's therefore necessary to have the
> diff_indent_heuristic flag set before calling init_revisions().
>
> A naive way to get the indentHeuristic config option respected in the
> diff-* plumbing commands is to make them use the git_diff_heuristic_config()
> callback right at the start of their main cmd functions.
>
> But I did not like that for two reasons:
>
> * It would make these commands invoke git_config() twice.
>
> * It doesn't avoid the problem if/when someone creates a new diff-something
>   plumbing command, and forgets to set the diff_indent_heuristic flag before
>   calling init_revisions().
>
> So instead I chose to make the indentHeuristic option part of diff's basic
> configuration, and in each of the diff plumbing commands I moved the call to
> git_config() before the call to init_revisions().
>
> This still doesn't really future-proof things for possible new diff plumbing
> commands, because someone could still invoke init_revisions() before setting
> up diff's basic configuration.  But I don't see an obvious way of ensuring
> that the diff_indent_heuristic flag is respected regardless of when
> diff_setup() is invoked.
>

The feature was included in v2.11 (released 2016-11-29) and we got no
negative feedback. Quite the opposite, all feedback we got, was positive.
This could be explained by having the feature as an experimental feature
and users who would be broken by it, did not test it yet or did not speak up.

So I'd propose to turn it on by default and anyone negatively impacted by that
could then use the config to turn it off for themselves (including plumbing).

Something like this, maybe?

---8<---
From 58d9a1ef135eff9f85b165986e4bc13479914f8e Mon Sep 17 00:00:00 2001
From: Stefan Beller <sbeller@google.com>
Date: Thu, 27 Apr 2017 14:26:59 -0700
Subject: [PATCH] diff.c: enable indent heuristic by default

The feature was included in v2.11 (released 2016-11-29) and we got no
negative feedback. Quite the opposite, all feedback we got was positive.

Turn it on by default. users who dislike the feature can turn it off
using by setting diff.compactionHeuristic (which includes plumbing
commands, see prior patches)

Signed-off-by: Stefan Beller <sbeller@google.com>
---
diff --git a/diff.c b/diff.c
index 11eef1c85d..7f301c1b62 100644
--- a/diff.c
+++ b/diff.c
@@ -27,7 +27,7 @@
 #endif

 static int diff_detect_rename_default;
-static int diff_indent_heuristic; /* experimental */
+static int diff_indent_heuristic = 1;
 static int diff_rename_limit_default = 400;
 static int diff_suppress_blank_empty;
 static int diff_use_color_default = -1;

Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.

[Jeff King]

On Fri, Apr 28, 2017 at 10:34:15AM -0700, Stefan Beller wrote:

> > So instead I chose to make the indentHeuristic option part of diff's basic
> > configuration, and in each of the diff plumbing commands I moved the call to
> > git_config() before the call to init_revisions().
> [...]
> 
> The feature was included in v2.11 (released 2016-11-29) and we got no
> negative feedback. Quite the opposite, all feedback we got, was positive.
> This could be explained by having the feature as an experimental feature
> and users who would be broken by it, did not test it yet or did not speak up.

Yeah, if the point you're trying to make is "nobody will be mad if this
is turned on by default", I don't think shipping it as a config option
is very conclusive.

I think a more interesting argument is: we turned on renames by default
a few versions ago, which changes the diff in a much bigger way, and
nobody complained.

  As a side note, I do happen to know of one program that depends
  heavily on diffs remaining stable. Imagine you have a Git hosting site
  which lets people comment on lines of diffs. You need some way to
  address the lines of the diff so that the annotations appear in the
  correct position when you regenerate the diff later.

  One way to do it is to just position the comment at the n'th line of
  the diff. Which obviously breaks if the diff changes. IMHO that is a
  bug in that program, which should be fixed to use the line numbers
  from the original blob (which is still not foolproof, because a
  different diff algorithm may move a change such that the line isn't
  even part of the diff anymore).

  I'm not worried about this particular program, as I happen to know it
  has already been fixed. But it's possible others have made a similar
  mistake.

> So I'd propose to turn it on by default and anyone negatively impacted by that
> could then use the config to turn it off for themselves (including plumbing).
> 
> Something like this, maybe?

Yeah, as long as this is on top of Marc's patches, I think it is the
natural conclusion that we had planned.

I don't know if we would want to be extra paranoid about patch-ids.
There is no helping:

  git rev-list HEAD | git diff-tree --stdin -p | git patch-id --stable

because diff-tree doesn't know that it's trying for "--stable" output.
But the diffs we compute internally for patch-id could disable the
heuristics. I'm not sure if those matter, though. AFAIK those are used
only for internal comparisons within a single program. I.e., we never
compare them against input from the user, nor do we output them to the
user. So they'll change, but I don't think anybody would care.

-Peff

Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.

[Stefan Beller]

On Fri, Apr 28, 2017 at 3:04 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Apr 28, 2017 at 10:34:15AM -0700, Stefan Beller wrote:
>
>> > So instead I chose to make the indentHeuristic option part of diff's basic
>> > configuration, and in each of the diff plumbing commands I moved the call to
>> > git_config() before the call to init_revisions().
>> [...]
>>
>> The feature was included in v2.11 (released 2016-11-29) and we got no
>> negative feedback. Quite the opposite, all feedback we got, was positive.
>> This could be explained by having the feature as an experimental feature
>> and users who would be broken by it, did not test it yet or did not speak up.
>
> Yeah, if the point you're trying to make is "nobody will be mad if this
> is turned on by default", I don't think shipping it as a config option
> is very conclusive.
>
> I think a more interesting argument is: we turned on renames by default
> a few versions ago, which changes the diff in a much bigger way, and
> nobody complained.
>
>   As a side note, I do happen to know of one program that depends
>   heavily on diffs remaining stable. Imagine you have a Git hosting site
>   which lets people comment on lines of diffs. You need some way to
>   address the lines of the diff so that the annotations appear in the
>   correct position when you regenerate the diff later.
>
>   One way to do it is to just position the comment at the n'th line of
>   the diff. Which obviously breaks if the diff changes. IMHO that is a
>   bug in that program, which should be fixed to use the line numbers
>   from the original blob (which is still not foolproof, because a
>   different diff algorithm may move a change such that the line isn't
>   even part of the diff anymore).
>
>   I'm not worried about this particular program, as I happen to know it
>   has already been fixed. But it's possible others have made a similar
>   mistake.

Thanks for this enlightenment. :)

>> So I'd propose to turn it on by default and anyone negatively impacted by that
>> could then use the config to turn it off for themselves (including plumbing).
>>
>> Something like this, maybe?
>
> Yeah, as long as this is on top of Marc's patches, I think it is the
> natural conclusion that we had planned.

That is what I was trying to hint at with the commit message given.

> I don't know if we would want to be extra paranoid about patch-ids.
> There is no helping:
>
>   git rev-list HEAD | git diff-tree --stdin -p | git patch-id --stable
>
> because diff-tree doesn't know that it's trying for "--stable" output.
> But the diffs we compute internally for patch-id could disable the
> heuristics. I'm not sure if those matter, though. AFAIK those are used
> only for internal comparisons within a single program. I.e., we never
> compare them against input from the user, nor do we output them to the
> user. So they'll change, but I don't think anybody would care.
>
> -Peff

Well, I think as long as we have a reasonable easy way to undo
the default (hence keeping the option), we can proceed with caution
here, too.

Thanks,
Stefan

Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.

[Ævar Arnfjörð Bjarmason]

On Sat, Apr 29, 2017 at 12:04 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Apr 28, 2017 at 10:34:15AM -0700, Stefan Beller wrote:
>
>> > So instead I chose to make the indentHeuristic option part of diff's basic
>> > configuration, and in each of the diff plumbing commands I moved the call to
>> > git_config() before the call to init_revisions().
>> [...]
>>
>> The feature was included in v2.11 (released 2016-11-29) and we got no
>> negative feedback. Quite the opposite, all feedback we got, was positive.
>> This could be explained by having the feature as an experimental feature
>> and users who would be broken by it, did not test it yet or did not speak up.
>
> Yeah, if the point you're trying to make is "nobody will be mad if this
> is turned on by default", I don't think shipping it as a config option
> is very conclusive.
>
> I think a more interesting argument is: we turned on renames by default
> a few versions ago, which changes the diff in a much bigger way, and
> nobody complained.
>
>   As a side note, I do happen to know of one program that depends
>   heavily on diffs remaining stable. Imagine you have a Git hosting site
>   which lets people comment on lines of diffs. You need some way to
>   address the lines of the diff so that the annotations appear in the
>   correct position when you regenerate the diff later.
>
>   One way to do it is to just position the comment at the n'th line of
>   the diff. Which obviously breaks if the diff changes. IMHO that is a
>   bug in that program, which should be fixed to use the line numbers
>   from the original blob (which is still not foolproof, because a
>   different diff algorithm may move a change such that the line isn't
>   even part of the diff anymore).
>
>   I'm not worried about this particular program, as I happen to know it
>   has already been fixed. But it's possible others have made a similar
>   mistake.
>
>> So I'd propose to turn it on by default and anyone negatively impacted by that
>> could then use the config to turn it off for themselves (including plumbing).
>>
>> Something like this, maybe?
>
> Yeah, as long as this is on top of Marc's patches, I think it is the
> natural conclusion that we had planned.
>
> I don't know if we would want to be extra paranoid about patch-ids.
> There is no helping:
>
>   git rev-list HEAD | git diff-tree --stdin -p | git patch-id --stable
>
> because diff-tree doesn't know that it's trying for "--stable" output.
> But the diffs we compute internally for patch-id could disable the
> heuristics. I'm not sure if those matter, though. AFAIK those are used
> only for internal comparisons within a single program. I.e., we never
> compare them against input from the user, nor do we output them to the
> user. So they'll change, but I don't think anybody would care.

I have a few-million row table with commit_id as one column & patch_id
as another. I.e. a commit -> patch_id mapping.

This is used for an hourly reporting system of sorts, i.e. you can see
that you were working on commit X on Friday. It uses the patch-id to
de-duplicate that commit against some commit Y you may have pushed to
a topic branch, and already used for filling in hours.

I.e. it's a thing do DWYM in a rebase-heavy workflow where the commit
ids change, and where you're too lazy to have deleted the topic branch
afterwards (because they get pruned anyway).

It's fine for me if the patch-id changes, for most diffs it'll be the
same, and if not it'll auto-adjust eventually since the records I'm
inserting will all use the new algorithm.

But as the diff algorithm changes this system will start presenting
both X and Y in the UI as not-duplicate, because it'll have computed
the patch id of X with an older version of git, and the id of Y with
the new one.

I'm surely not the only one who's written something like this. Just
wanted to point out that systems like this do exist, I don't know of
any easier way with git to get a "is this rebased commit the same"
than patch-id, and for performance reasons you may be comparing patch
ids across git versions.

Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.

[Jeff King]

On Mon, May 01, 2017 at 12:34:38PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > I don't know if we would want to be extra paranoid about patch-ids.
> > There is no helping:
> >
> >   git rev-list HEAD | git diff-tree --stdin -p | git patch-id --stable
> >
> > because diff-tree doesn't know that it's trying for "--stable" output.
> > But the diffs we compute internally for patch-id could disable the
> > heuristics. I'm not sure if those matter, though. AFAIK those are used
> > only for internal comparisons within a single program. I.e., we never
> > compare them against input from the user, nor do we output them to the
> > user. So they'll change, but I don't think anybody would care.
> 
> I have a few-million row table with commit_id as one column & patch_id
> as another. I.e. a commit -> patch_id mapping.

Thanks for this data point. It's always interesting to hear about
unforeseen uses of the tools.

Out of curiosity, how do you generate the patch-ids? Is it with
something like diff-tree piped to patch-id?

I do feel a bit sad about breaking this case (or at the very least
forcing you to set an option to retain cross-version compatibility). But
my gut says that we don't want to lock ourselves into never changing the
diff algorithm (and I'm sure we've done it inadvertently a few times
over the years; even the recent switch to turning on renames would have
had that impact).

-Peff

Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I do feel a bit sad about breaking this case (or at the very least
> forcing you to set an option to retain cross-version compatibility). But
> my gut says that we don't want to lock ourselves into never changing the
> diff algorithm (and I'm sure we've done it inadvertently a few times
> over the years; even the recent switch to turning on renames would have
> had that impact).

IIRC, we never promised that different versions of Git produce the
same patch ID for the same change.  I do share that sadness, as not
making that promise would affect not just such an external database
keyed with patch-ids but also affects the rerere database, and those
who heavily depend on the rerere database would need to refresh them
every time diff/merge algorithm is updated, using something like
contrib/rerere-train.sh script.

But at the same time, I am very much against promising that the
textual diff output will never improve for human readability.

Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.

[Ævar Arnfjörð Bjarmason]

On Tue, May 9, 2017 at 5:16 AM, Jeff King <peff@peff.net> wrote:
> On Mon, May 01, 2017 at 12:34:38PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> > I don't know if we would want to be extra paranoid about patch-ids.
>> > There is no helping:
>> >
>> >   git rev-list HEAD | git diff-tree --stdin -p | git patch-id --stable
>> >
>> > because diff-tree doesn't know that it's trying for "--stable" output.
>> > But the diffs we compute internally for patch-id could disable the
>> > heuristics. I'm not sure if those matter, though. AFAIK those are used
>> > only for internal comparisons within a single program. I.e., we never
>> > compare them against input from the user, nor do we output them to the
>> > user. So they'll change, but I don't think anybody would care.
>>
>> I have a few-million row table with commit_id as one column & patch_id
>> as another. I.e. a commit -> patch_id mapping.
>
> Thanks for this data point. It's always interesting to hear about
> unforeseen uses of the tools.
>
> Out of curiosity, how do you generate the patch-ids? Is it with
> something like diff-tree piped to patch-id?

This:

    my $cmd = qq[git --git-dir="$repository_path" log --since="$since"
--until="$until" --all --pretty=format:%H --binary | git patch-id];
    open my $patch_id_fh, " $cmd |";

Which is part of a loop that generates since/until for continuous
pull/insertion. Also, a few lines later there's a workaround for the
git.git bug of patch-id being ^0+$ (fixed in 2485eab55c
("git-patch-id: do not trip over "no newline" markers", 2011-02-17)),
which gives you a sense of how long it's been since anyone's touched
this.

> I do feel a bit sad about breaking this case (or at the very least
> forcing you to set an option to retain cross-version compatibility). But
> my gut says that we don't want to lock ourselves into never changing the
> diff algorithm (and I'm sure we've done it inadvertently a few times
> over the years; even the recent switch to turning on renames would have
> had that impact).

As noted I think it's completely fine to change the patch-ids by
changing the diff algorithm.

I'm about to give some more detail on this in the other thread, but I
find that on our repos the indent heuristic changes the patch-id for
around 2% of patches, which seems fairly typical for non-changelog-y
code. You *then* need to be using topic branches you didn't delete as
well as having authored such a patch for this change to kick in, so
the impact is really minimal.

Even if it somehow changed 100% of the ids that would be fine too. It
would auto-heal as the same git version started reading & inserting
the ids, which are only relevant in a moving window.

Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.

[Jeff King]

On Tue, May 09, 2017 at 09:58:28AM +0200, Ævar Arnfjörð Bjarmason wrote:

> > Out of curiosity, how do you generate the patch-ids? Is it with
> > something like diff-tree piped to patch-id?
> 
> This:
> 
>     my $cmd = qq[git --git-dir="$repository_path" log --since="$since"
> --until="$until" --all --pretty=format:%H --binary | git patch-id];
>     open my $patch_id_fh, " $cmd |";

Ah, OK. I was specifically curious whether the decision to respect the
config switch in plumbing would have any impact for your script. But it
wouldn't, as it was already using log (though I suspect the real
protection for your script is that it is used from a vanilla
environment, not by random users).

-Peff

[PATCHv2 0/3] Make diff plumbing commands respect the indentHeuristic.

[Marc Branchaud]

v2: Fixed up the commit messages and added tests.

Marc Branchaud (2):
  diff: make the indent heuristic part of diff's basic configuration
  diff: have the diff-* builtins configure diff before initializing
    revisions

Stefan Beller (1):
  diff: enable indent heuristic by default

 builtin/diff-files.c   |  2 +-
 builtin/diff-index.c   |  2 +-
 builtin/diff-tree.c    |  2 +-
 diff.c                 |  8 +++---
 t/t4061-diff-indent.sh | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 73 insertions(+), 7 deletions(-)

-- 
2.13.0.rc1.15.gf67d331ad

[PATCHv2 1/3] diff: make the indent heuristic part of diff's basic configuration

[Marc Branchaud]

This heuristic was originally introduced as an experimental feature,
and therefore part of the UI configuration.

But the user often sees diffs generated by plumbing commands like
diff-tree.  Moving the indent heuristic into diff's basic configuration
prepares the way for diff plumbing commands to respect the setting.

The heuristic itself merely makes the diffs more aesthetically
pleasing, without changing their correctness.  Scripts that rely on
the diff plumbing commands should not care whether or not the heuristic
is employed.

Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
---
 diff.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/diff.c b/diff.c
index 11eef1c85..da96577ea 100644
--- a/diff.c
+++ b/diff.c
@@ -290,9 +290,6 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	if (git_diff_heuristic_config(var, value, cb) < 0)
-		return -1;
-
 	if (!strcmp(var, "diff.wserrorhighlight")) {
 		int val = parse_ws_error_highlight(value);
 		if (val < 0)
@@ -351,6 +348,9 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
 	if (starts_with(var, "submodule."))
 		return parse_submodule_config_option(var, value);
 
+	if (git_diff_heuristic_config(var, value, cb) < 0)
+		return -1;
+
 	return git_default_config(var, value, cb);
 }
 
-- 
2.13.0.rc1.15.gf67d331ad

[PATCHv2 2/3] diff: have the diff-* builtins configure diff before initializing revisions

[Marc Branchaud]

This makes the commands respect diff configuration options, such as
indentHeuristic, because init_revisions() calls diff_setup() which fills
in the diff_options struct.

Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
---
 builtin/diff-files.c   |  2 +-
 builtin/diff-index.c   |  2 +-
 builtin/diff-tree.c    |  2 +-
 t/t4061-diff-indent.sh | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 69 insertions(+), 3 deletions(-)

diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index 15c61fd8d..a572da9d5 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -20,9 +20,9 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 	int result;
 	unsigned options = 0;
 
+	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	init_revisions(&rev, prefix);
 	gitmodules_config();
-	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	rev.abbrev = 0;
 	precompose_argv(argc, argv);
 
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index 1af373d00..f084826a2 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -17,9 +17,9 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 	int i;
 	int result;
 
+	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	init_revisions(&rev, prefix);
 	gitmodules_config();
-	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	rev.abbrev = 0;
 	precompose_argv(argc, argv);
 
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 326f88b65..36a3a1976 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -105,9 +105,9 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 	struct setup_revision_opt s_r_opt;
 	int read_stdin = 0;
 
+	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	init_revisions(opt, prefix);
 	gitmodules_config();
-	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	opt->abbrev = 0;
 	opt->diff = 1;
 	opt->disable_stdin = 1;
diff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh
index 556450609..13d3dc96a 100755
--- a/t/t4061-diff-indent.sh
+++ b/t/t4061-diff-indent.sh
@@ -213,4 +213,70 @@ test_expect_success 'blame: --no-indent-heuristic overrides config' '
 	compare_blame spaces-expect out-blame2
 '
 
+test_expect_success 'diff-tree: nice spaces with --indent-heuristic' '
+	git diff-tree --indent-heuristic -p old new -- spaces.txt >out-diff-tree-compacted &&
+	compare_diff spaces-compacted-expect out-diff-tree-compacted
+'
+
+test_expect_success 'diff-tree: nice spaces with diff.indentHeuristic' '
+	git -c diff.indentHeuristic=true diff-tree -p old new -- spaces.txt >out-diff-tree-compacted2 &&
+	compare_diff spaces-compacted-expect out-diff-tree-compacted2
+'
+
+test_expect_success 'diff-tree: --no-indent-heuristic overrides config' '
+	git -c diff.indentHeuristic=true diff-tree --no-indent-heuristic -p old new -- spaces.txt >out-diff-tree &&
+	compare_diff spaces-expect out-diff-tree
+'
+
+test_expect_success 'diff-index: nice spaces with --indent-heuristic' '
+	git checkout -B diff-index &&
+	git reset --soft HEAD~ &&
+	git diff-index --indent-heuristic -p old -- spaces.txt >out-diff-index-compacted &&
+	compare_diff spaces-compacted-expect out-diff-index-compacted &&
+	git checkout -f master
+'
+
+test_expect_success 'diff-index: nice spaces with diff.indentHeuristic' '
+	git checkout -B diff-index &&
+	git reset --soft HEAD~ &&
+	git -c diff.indentHeuristic=true diff-index -p old -- spaces.txt >out-diff-index-compacted2 &&
+	compare_diff spaces-compacted-expect out-diff-index-compacted2 &&
+	git checkout -f master
+'
+
+test_expect_success 'diff-index: --no-indent-heuristic overrides config' '
+	git checkout -B diff-index &&
+	git reset --soft HEAD~ &&
+	git -c diff.indentHeuristic=true diff-index --no-indent-heuristic -p old -- spaces.txt >out-diff-index &&
+	compare_diff spaces-expect out-diff-index &&
+	git checkout -f master
+'
+
+test_expect_success 'diff-files: nice spaces with diff.indentHeuristic' '
+	git checkout -B diff-files &&
+	git reset HEAD~ &&
+	git -c diff.indentHeuristic=true diff-files -p spaces.txt >out-diff-files-raw &&
+	grep -v index out-diff-files-raw >out-diff-files-compacted &&
+	compare_diff spaces-compacted-expect out-diff-files-compacted &&
+	git checkout -f master
+'
+
+test_expect_success 'diff-files: nice spaces with diff.indentHeuristic' '
+	git checkout -B diff-files &&
+	git reset HEAD~ &&
+	git -c diff.indentHeuristic=true diff-files -p spaces.txt >out-diff-files-raw2 &&
+	grep -v index out-diff-files-raw2 >out-diff-files-compacted2 &&
+	compare_diff spaces-compacted-expect out-diff-files-compacted2 &&
+	git checkout -f master
+'
+
+test_expect_success 'diff-files: --no-indent-heuristic overrides config' '
+	git checkout -B diff-files &&
+	git reset HEAD~ &&
+	git -c diff.indentHeuristic=true diff-files --no-indent-heuristic -p spaces.txt >out-diff-files-raw3 &&
+	grep -v index out-diff-files-raw3 >out-diff-files &&
+	compare_diff spaces-expect out-diff-files &&
+	git checkout -f master
+'
+
 test_done
-- 
2.13.0.rc1.15.gf67d331ad

[PATCH 3/3] diff: enable indent heuristic by default

[Marc Branchaud]

From: Stefan Beller <sbeller@google.com>

The feature was included in v2.11 (released 2016-11-29) and we got no
negative feedback. Quite the opposite, all feedback we got was positive.

Turn it on by default. Users who dislike the feature can turn it off
by setting diff.compactionHeuristic (which includes plumbing commands,
see prior patches).

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
---
 diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index da96577ea..2c47ccb4a 100644
--- a/diff.c
+++ b/diff.c
@@ -27,7 +27,7 @@
 #endif
 
 static int diff_detect_rename_default;
-static int diff_indent_heuristic; /* experimental */
+static int diff_indent_heuristic = 1;
 static int diff_rename_limit_default = 400;
 static int diff_suppress_blank_empty;
 static int diff_use_color_default = -1;
-- 
2.13.0.rc1.15.gf67d331ad

Re: [PATCHv2 0/3] Make diff plumbing commands respect the indentHeuristic.

[Jeff King]

On Fri, Apr 28, 2017 at 06:33:12PM -0400, Marc Branchaud wrote:

> v2: Fixed up the commit messages and added tests.
> 
> Marc Branchaud (2):
>   diff: make the indent heuristic part of diff's basic configuration
>   diff: have the diff-* builtins configure diff before initializing
>     revisions
> 
> Stefan Beller (1):
>   diff: enable indent heuristic by default

Thanks, these look fine to me. I'd like to get an ACK from Michael, in
case he had some other reason for omitting them from git_diff_ui_config
(from my recollection, it's probably just a mix of conservatism and
following what the compaction heuristic had done).

-Peff

Re: [PATCHv2 0/3] Make diff plumbing commands respect the indentHeuristic.

[Jeff King]

On Sat, Apr 29, 2017 at 08:40:52AM -0400, Jeff King wrote:

> On Fri, Apr 28, 2017 at 06:33:12PM -0400, Marc Branchaud wrote:
> 
> > v2: Fixed up the commit messages and added tests.
> > 
> > Marc Branchaud (2):
> >   diff: make the indent heuristic part of diff's basic configuration
> >   diff: have the diff-* builtins configure diff before initializing
> >     revisions
> > 
> > Stefan Beller (1):
> >   diff: enable indent heuristic by default
> 
> Thanks, these look fine to me. I'd like to get an ACK from Michael, in
> case he had some other reason for omitting them from git_diff_ui_config
> (from my recollection, it's probably just a mix of conservatism and
> following what the compaction heuristic had done).

Sorry, I spoke too soon. The third one needs a few test adjustments
squashed in to pass the tests.

The fixes below for t4061 are pretty obvious, but the one in t4051 is
anything but. What happens is that we have a function:

  int dummy();
  {
     some body
  }

and modify it to look like:

  int dummy();
  int dummy();
  {
     some body
  }

and we expect that this counts as making a change to the function for
the purposes of -W. But with the indent heuristic, instead of saying "we
added an extra line after the first dummy()", we say "we added an extra
dummy() before the function". Which is perfectly reasonable.

I don't think duplicating the line is important to the test, as opposed
to making any random change.  But we can't just change a line in the
body itself, because the point of the test is making sure -W shows the
whole function. And the function is short enough that a change in the
body would show the whole thing via context anyway. So I opted to make
the change at the same spot, but make the diff less ambiguous.

diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
index 6154acb45..5f46c0341 100755
--- a/t/t4051-diff-function-context.sh
+++ b/t/t4051-diff-function-context.sh
@@ -72,7 +72,7 @@ test_expect_success 'setup' '
 
 	# overlap function context of 1st change and -u context of 2nd change
 	grep -v "delete me from hello" <"$dir/hello.c" >file.c &&
-	sed 2p <"$dir/dummy.c" >>file.c &&
+	sed "2aextra line" <"$dir/dummy.c" >>file.c &&
 	commit_and_tag changed_hello_dummy file.c &&
 
 	git checkout initial &&
diff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh
index 13d3dc96a..56d7d7760 100755
--- a/t/t4061-diff-indent.sh
+++ b/t/t4061-diff-indent.sh
@@ -153,7 +153,7 @@ test_expect_success 'prepare' '
 '
 
 test_expect_success 'diff: ugly spaces' '
-	git diff old new -- spaces.txt >out &&
+	git diff --no-indent-heuristic old new -- spaces.txt >out &&
 	compare_diff spaces-expect out
 '
 
@@ -183,7 +183,7 @@ test_expect_success 'diff: --indent-heuristic with --histogram' '
 '
 
 test_expect_success 'diff: ugly functions' '
-	git diff old new -- functions.c >out &&
+	git diff --no-indent-heuristic old new -- functions.c >out &&
 	compare_diff functions-expect out
 '
 
@@ -193,7 +193,7 @@ test_expect_success 'diff: nice functions with --indent-heuristic' '
 '
 
 test_expect_success 'blame: ugly spaces' '
-	git blame old..new -- spaces.txt >out-blame &&
+	git blame --no-indent-heuristic old..new -- spaces.txt >out-blame &&
 	compare_blame spaces-expect out-blame
 '
 

Re: [PATCHv2 0/3] Make diff plumbing commands respect the indentHeuristic.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
> index 6154acb45..5f46c0341 100755
> --- a/t/t4051-diff-function-context.sh
> +++ b/t/t4051-diff-function-context.sh
> @@ -72,7 +72,7 @@ test_expect_success 'setup' '
>  
>  	# overlap function context of 1st change and -u context of 2nd change
>  	grep -v "delete me from hello" <"$dir/hello.c" >file.c &&
> -	sed 2p <"$dir/dummy.c" >>file.c &&
> +	sed "2aextra line" <"$dir/dummy.c" >>file.c &&

I've never used 'a' (or 'i') command of sed without having it
immediately followed by a backslash-newline in my scripts.  How
portable is this addition, I have to wonder.  Can BSD folks give it
a quick test?

Thanks.

Re: [PATCHv2 0/3] Make diff plumbing commands respect the indentHeuristic.

[Jeff King]

On Sun, Apr 30, 2017 at 06:11:48PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
> > index 6154acb45..5f46c0341 100755
> > --- a/t/t4051-diff-function-context.sh
> > +++ b/t/t4051-diff-function-context.sh
> > @@ -72,7 +72,7 @@ test_expect_success 'setup' '
> >  
> >  	# overlap function context of 1st change and -u context of 2nd change
> >  	grep -v "delete me from hello" <"$dir/hello.c" >file.c &&
> > -	sed 2p <"$dir/dummy.c" >>file.c &&
> > +	sed "2aextra line" <"$dir/dummy.c" >>file.c &&
> 
> I've never used 'a' (or 'i') command of sed without having it
> immediately followed by a backslash-newline in my scripts.  How
> portable is this addition, I have to wonder.  Can BSD folks give it
> a quick test?

I think you're right that it needs the backslash. It's so rarely used
that I always forget which one is the portable way.

-Peff

PS Outside of our test scripts, I'd probably just have written:

     perl -lpe 'print "extra line" if $. == 2'

   I think we have traditionally preferred sed/awk to perl, but given
   the heavy use of vanilla perl elsewhere in the test suite, I think
   that is maybe just superstition at this point.

Re: [PATCHv2 0/3] Make diff plumbing commands respect the indentHeuristic.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> PS Outside of our test scripts, I'd probably just have written:
>
>      perl -lpe 'print "extra line" if $. == 2'
>
>    I think we have traditionally preferred sed/awk to perl, but given
>    the heavy use of vanilla perl elsewhere in the test suite, I think
>    that is maybe just superstition at this point.

I would have avoided sed with 'a', 'i' and 'c' in one-liners myself,
and a Perl script like the above would probably have been my choice.

[PATCHv3 0/4] Make diff plumbing commands respect the indentHeuristic.

[Marc Branchaud]

On 2017-04-29 09:14 AM, Jeff King wrote:
> On Sat, Apr 29, 2017 at 08:40:52AM -0400, Jeff King wrote:
> 
>> On Fri, Apr 28, 2017 at 06:33:12PM -0400, Marc Branchaud wrote:
>>
>>> v2: Fixed up the commit messages and added tests.
>>>
>>> Marc Branchaud (2):
>>>   diff: make the indent heuristic part of diff's basic configuration
>>>   diff: have the diff-* builtins configure diff before initializing
>>>     revisions
>>>
>>> Stefan Beller (1):
>>>   diff: enable indent heuristic by default
>>
>> Thanks, these look fine to me. I'd like to get an ACK from Michael, in
>> case he had some other reason for omitting them from git_diff_ui_config
>> (from my recollection, it's probably just a mix of conservatism and
>> following what the compaction heuristic had done).
> 
> Sorry, I spoke too soon. The third one needs a few test adjustments
> squashed in to pass the tests.

Doh!  That'll teach me to try to do this stuff at the end of a Friday...

One more try, then:

Changes since v2:

  Patch 1/4 : Unchanged.

  Patch 2/4 : Mentioned how the new behaviour matches the diff Porcelain.

  Patch 3/4 : Updated the tests.

  Patch 4/4 : (New) Jeff's add--interactive patch.

Thanks for all the help, Jeff!

		M.


Jeff King (1):
  add--interactive: drop diff.indentHeuristic handling

Marc Branchaud (2):
  diff: make the indent heuristic part of diff's basic configuration
  diff: have the diff-* builtins configure diff before initializing
    revisions

Stefan Beller (1):
  diff: enable indent heuristic by default

 builtin/diff-files.c             |  2 +-
 builtin/diff-index.c             |  2 +-
 builtin/diff-tree.c              |  2 +-
 diff.c                           |  8 ++---
 git-add--interactive.perl        |  4 ---
 t/t4051-diff-function-context.sh |  3 +-
 t/t4061-diff-indent.sh           | 72 ++++++++++++++++++++++++++++++++++++++--
 7 files changed, 78 insertions(+), 15 deletions(-)

-- 
2.13.0.rc1.15.gf67d331ad

[PATCHv3 1/4] diff: make the indent heuristic part of diff's basic configuration

[Marc Branchaud]

This heuristic was originally introduced as an experimental feature,
and therefore part of the UI configuration.

But the user often sees diffs generated by plumbing commands like
diff-tree.  Moving the indent heuristic into diff's basic configuration
prepares the way for diff plumbing commands to respect the setting.

The heuristic itself merely makes the diffs more aesthetically
pleasing, without changing their correctness.  Scripts that rely on
the diff plumbing commands should not care whether or not the heuristic
is employed.

Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
---
 diff.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/diff.c b/diff.c
index 11eef1c85..da96577ea 100644
--- a/diff.c
+++ b/diff.c
@@ -290,9 +290,6 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	if (git_diff_heuristic_config(var, value, cb) < 0)
-		return -1;
-
 	if (!strcmp(var, "diff.wserrorhighlight")) {
 		int val = parse_ws_error_highlight(value);
 		if (val < 0)
@@ -351,6 +348,9 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
 	if (starts_with(var, "submodule."))
 		return parse_submodule_config_option(var, value);
 
+	if (git_diff_heuristic_config(var, value, cb) < 0)
+		return -1;
+
 	return git_default_config(var, value, cb);
 }
 
-- 
2.13.0.rc1.15.gf67d331ad

[PATCHv3 2/4] diff: have the diff-* builtins configure diff before initializing revisions

[Marc Branchaud]

This matches how the diff Porcelain works.  It makes the plumbing commands
respect diff's configuration options, such as indentHeuristic, because
init_revisions() calls diff_setup() which fills in the diff_options struct.

Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
---
 builtin/diff-files.c   |  2 +-
 builtin/diff-index.c   |  2 +-
 builtin/diff-tree.c    |  2 +-
 t/t4061-diff-indent.sh | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 69 insertions(+), 3 deletions(-)

diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index 15c61fd8d..a572da9d5 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -20,9 +20,9 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 	int result;
 	unsigned options = 0;
 
+	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	init_revisions(&rev, prefix);
 	gitmodules_config();
-	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	rev.abbrev = 0;
 	precompose_argv(argc, argv);
 
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index 1af373d00..f084826a2 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -17,9 +17,9 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 	int i;
 	int result;
 
+	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	init_revisions(&rev, prefix);
 	gitmodules_config();
-	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	rev.abbrev = 0;
 	precompose_argv(argc, argv);
 
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 326f88b65..36a3a1976 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -105,9 +105,9 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 	struct setup_revision_opt s_r_opt;
 	int read_stdin = 0;
 
+	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	init_revisions(opt, prefix);
 	gitmodules_config();
-	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	opt->abbrev = 0;
 	opt->diff = 1;
 	opt->disable_stdin = 1;
diff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh
index 556450609..13d3dc96a 100755
--- a/t/t4061-diff-indent.sh
+++ b/t/t4061-diff-indent.sh
@@ -213,4 +213,70 @@ test_expect_success 'blame: --no-indent-heuristic overrides config' '
 	compare_blame spaces-expect out-blame2
 '
 
+test_expect_success 'diff-tree: nice spaces with --indent-heuristic' '
+	git diff-tree --indent-heuristic -p old new -- spaces.txt >out-diff-tree-compacted &&
+	compare_diff spaces-compacted-expect out-diff-tree-compacted
+'
+
+test_expect_success 'diff-tree: nice spaces with diff.indentHeuristic' '
+	git -c diff.indentHeuristic=true diff-tree -p old new -- spaces.txt >out-diff-tree-compacted2 &&
+	compare_diff spaces-compacted-expect out-diff-tree-compacted2
+'
+
+test_expect_success 'diff-tree: --no-indent-heuristic overrides config' '
+	git -c diff.indentHeuristic=true diff-tree --no-indent-heuristic -p old new -- spaces.txt >out-diff-tree &&
+	compare_diff spaces-expect out-diff-tree
+'
+
+test_expect_success 'diff-index: nice spaces with --indent-heuristic' '
+	git checkout -B diff-index &&
+	git reset --soft HEAD~ &&
+	git diff-index --indent-heuristic -p old -- spaces.txt >out-diff-index-compacted &&
+	compare_diff spaces-compacted-expect out-diff-index-compacted &&
+	git checkout -f master
+'
+
+test_expect_success 'diff-index: nice spaces with diff.indentHeuristic' '
+	git checkout -B diff-index &&
+	git reset --soft HEAD~ &&
+	git -c diff.indentHeuristic=true diff-index -p old -- spaces.txt >out-diff-index-compacted2 &&
+	compare_diff spaces-compacted-expect out-diff-index-compacted2 &&
+	git checkout -f master
+'
+
+test_expect_success 'diff-index: --no-indent-heuristic overrides config' '
+	git checkout -B diff-index &&
+	git reset --soft HEAD~ &&
+	git -c diff.indentHeuristic=true diff-index --no-indent-heuristic -p old -- spaces.txt >out-diff-index &&
+	compare_diff spaces-expect out-diff-index &&
+	git checkout -f master
+'
+
+test_expect_success 'diff-files: nice spaces with diff.indentHeuristic' '
+	git checkout -B diff-files &&
+	git reset HEAD~ &&
+	git -c diff.indentHeuristic=true diff-files -p spaces.txt >out-diff-files-raw &&
+	grep -v index out-diff-files-raw >out-diff-files-compacted &&
+	compare_diff spaces-compacted-expect out-diff-files-compacted &&
+	git checkout -f master
+'
+
+test_expect_success 'diff-files: nice spaces with diff.indentHeuristic' '
+	git checkout -B diff-files &&
+	git reset HEAD~ &&
+	git -c diff.indentHeuristic=true diff-files -p spaces.txt >out-diff-files-raw2 &&
+	grep -v index out-diff-files-raw2 >out-diff-files-compacted2 &&
+	compare_diff spaces-compacted-expect out-diff-files-compacted2 &&
+	git checkout -f master
+'
+
+test_expect_success 'diff-files: --no-indent-heuristic overrides config' '
+	git checkout -B diff-files &&
+	git reset HEAD~ &&
+	git -c diff.indentHeuristic=true diff-files --no-indent-heuristic -p spaces.txt >out-diff-files-raw3 &&
+	grep -v index out-diff-files-raw3 >out-diff-files &&
+	compare_diff spaces-expect out-diff-files &&
+	git checkout -f master
+'
+
 test_done
-- 
2.13.0.rc1.15.gf67d331ad

[PATCHv3 3/4] diff: enable indent heuristic by default

[Marc Branchaud]

From: Stefan Beller <sbeller@google.com>

The feature was included in v2.11 (released 2016-11-29) and we got no
negative feedback. Quite the opposite, all feedback we got was positive.

Turn it on by default. Users who dislike the feature can turn it off
by setting diff.indentHeuristic (which also configures plumbing commands,
see prior patches).

The change to t/t4051-diff-function-context.sh is needed because the
heuristic shifts the changed hunk in the patch.  To get the same result
regardless of the heuristic configuration, we modify the test file
differently:  We insert a completely new line after line 2, instead of
simply duplicating it.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>

---

Tested the sed "2a" command's escaping in both Ubuntu 17.04 and FreeBSD 10.3.
Threw in a little indenting so that it isn't too ugly.

 diff.c                           | 2 +-
 t/t4051-diff-function-context.sh | 3 ++-
 t/t4061-diff-indent.sh           | 6 +++---
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/diff.c b/diff.c
index da96577ea..2c47ccb4a 100644
--- a/diff.c
+++ b/diff.c
@@ -27,7 +27,7 @@
 #endif
 
 static int diff_detect_rename_default;
-static int diff_indent_heuristic; /* experimental */
+static int diff_indent_heuristic = 1;
 static int diff_rename_limit_default = 400;
 static int diff_suppress_blank_empty;
 static int diff_use_color_default = -1;
diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
index 6154acb45..3e6b485ec 100755
--- a/t/t4051-diff-function-context.sh
+++ b/t/t4051-diff-function-context.sh
@@ -72,7 +72,8 @@ test_expect_success 'setup' '
 
 	# overlap function context of 1st change and -u context of 2nd change
 	grep -v "delete me from hello" <"$dir/hello.c" >file.c &&
-	sed 2p <"$dir/dummy.c" >>file.c &&
+	sed "2a\\
+	     extra line" <"$dir/dummy.c" >>file.c &&
 	commit_and_tag changed_hello_dummy file.c &&
 
 	git checkout initial &&
diff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh
index 13d3dc96a..56d7d7760 100755
--- a/t/t4061-diff-indent.sh
+++ b/t/t4061-diff-indent.sh
@@ -153,7 +153,7 @@ test_expect_success 'prepare' '
 '
 
 test_expect_success 'diff: ugly spaces' '
-	git diff old new -- spaces.txt >out &&
+	git diff --no-indent-heuristic old new -- spaces.txt >out &&
 	compare_diff spaces-expect out
 '
 
@@ -183,7 +183,7 @@ test_expect_success 'diff: --indent-heuristic with --histogram' '
 '
 
 test_expect_success 'diff: ugly functions' '
-	git diff old new -- functions.c >out &&
+	git diff --no-indent-heuristic old new -- functions.c >out &&
 	compare_diff functions-expect out
 '
 
@@ -193,7 +193,7 @@ test_expect_success 'diff: nice functions with --indent-heuristic' '
 '
 
 test_expect_success 'blame: ugly spaces' '
-	git blame old..new -- spaces.txt >out-blame &&
+	git blame --no-indent-heuristic old..new -- spaces.txt >out-blame &&
 	compare_blame spaces-expect out-blame
 '
 
-- 
2.13.0.rc1.15.gf67d331ad

Re: [PATCHv3 3/4] diff: enable indent heuristic by default

[Jeff King]

On Mon, May 01, 2017 at 06:13:44PM -0400, Marc Branchaud wrote:

> From: Stefan Beller <sbeller@google.com>
> 
> The feature was included in v2.11 (released 2016-11-29) and we got no
> negative feedback. Quite the opposite, all feedback we got was positive.
> 
> Turn it on by default. Users who dislike the feature can turn it off
> by setting diff.indentHeuristic (which also configures plumbing commands,
> see prior patches).
> 
> The change to t/t4051-diff-function-context.sh is needed because the
> heuristic shifts the changed hunk in the patch.  To get the same result
> regardless of the heuristic configuration, we modify the test file
> differently:  We insert a completely new line after line 2, instead of
> simply duplicating it.
> 
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
> 
> ---
> 
> Tested the sed "2a" command's escaping in both Ubuntu 17.04 and FreeBSD 10.3.
> Threw in a little indenting so that it isn't too ugly.

I think that should be fine. The indenting will go into the output, but
we really don't care _what_ that line is, just as long as it's new
content.

> diff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh
> index 13d3dc96a..56d7d7760 100755
> --- a/t/t4061-diff-indent.sh
> +++ b/t/t4061-diff-indent.sh
> @@ -153,7 +153,7 @@ test_expect_success 'prepare' '
>  '
>  
>  test_expect_success 'diff: ugly spaces' '
> -	git diff old new -- spaces.txt >out &&
> +	git diff --no-indent-heuristic old new -- spaces.txt >out &&
>  	compare_diff spaces-expect out
>  '

I guess one could argue that most of t4061 should be rewritten. This
fixes the failures that the "ugly" version is no longer the default. But
the tests checking that diff.indentHeuristic=true works are basically
doing nothing (the interesting thing after this patch is that setting it
to false goes back to the ugly output).

I dunno. If we drop the config and options as experiments, then the
whole script basically goes away in favor of checking that the test case
has the non-ugly output. If that's our endgame, it may not be worth
spending too much time refactoring it. But if we're going to keep the
config indefinitely, maybe we should make sure it works.

-Peff

[PATCHv3 4/4] add--interactive: drop diff.indentHeuristic handling

[Marc Branchaud]

From: Jeff King <peff@peff.net>

Now that diff.indentHeuristic is handled automatically by the plumbing
commands, there's no need to propagate it manually.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
---
 git-add--interactive.perl | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 709a5f6ce..79d675b5b 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -46,7 +46,6 @@ my ($diff_new_color) =
 my $normal_color = $repo->get_color("", "reset");
 
 my $diff_algorithm = $repo->config('diff.algorithm');
-my $diff_indent_heuristic = $repo->config_bool('diff.indentheuristic');
 my $diff_filter = $repo->config('interactive.difffilter');
 
 my $use_readkey = 0;
@@ -730,9 +729,6 @@ sub parse_diff {
 	if (defined $diff_algorithm) {
 		splice @diff_cmd, 1, 0, "--diff-algorithm=${diff_algorithm}";
 	}
-	if ($diff_indent_heuristic) {
-		splice @diff_cmd, 1, 0, "--indent-heuristic";
-	}
 	if (defined $patch_mode_revision) {
 		push @diff_cmd, get_diff_reference($patch_mode_revision);
 	}
-- 
2.13.0.rc1.15.gf67d331ad

Re: [PATCHv3 0/4] Make diff plumbing commands respect the indentHeuristic.

[Stefan Beller]

> Changes since v2:
>
>   Patch 1/4 : Unchanged.
>
>   Patch 2/4 : Mentioned how the new behaviour matches the diff Porcelain.
>
>   Patch 3/4 : Updated the tests.

Thanks for picking up that patch and carrying it further!
The whole series looks good to me.

Thanks,
Stefan

[PATCH 4/3] add--interactive: drop diff.indentHeuristic handling

[Jeff King]

On Sat, Apr 29, 2017 at 08:40:52AM -0400, Jeff King wrote:

> On Fri, Apr 28, 2017 at 06:33:12PM -0400, Marc Branchaud wrote:
> 
> > v2: Fixed up the commit messages and added tests.
> > 
> > Marc Branchaud (2):
> >   diff: make the indent heuristic part of diff's basic configuration
> >   diff: have the diff-* builtins configure diff before initializing
> >     revisions
> > 
> > Stefan Beller (1):
> >   diff: enable indent heuristic by default
> 
> Thanks, these look fine to me. I'd like to get an ACK from Michael, in
> case he had some other reason for omitting them from git_diff_ui_config
> (from my recollection, it's probably just a mix of conservatism and
> following what the compaction heuristic had done).

We can also do this simplification on top (once the other test problems
are fixed, of course).

-- >8 --
Subject: add--interactive: drop diff.indentHeuristic handling

Now that diff.indentHeuristic is handled automatically by the plumbing
commands, there's no need to propagate it manually.

Signed-off-by: Jeff King <peff@peff.net>
---
I actually wonder if diff.algorithm should go into git_diff_basic_config
by the same rationale as the rest of this series.

 git-add--interactive.perl | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 709a5f6ce..79d675b5b 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -46,7 +46,6 @@ my ($diff_new_color) =
 my $normal_color = $repo->get_color("", "reset");
 
 my $diff_algorithm = $repo->config('diff.algorithm');
-my $diff_indent_heuristic = $repo->config_bool('diff.indentheuristic');
 my $diff_filter = $repo->config('interactive.difffilter');
 
 my $use_readkey = 0;
@@ -730,9 +729,6 @@ sub parse_diff {
 	if (defined $diff_algorithm) {
 		splice @diff_cmd, 1, 0, "--diff-algorithm=${diff_algorithm}";
 	}
-	if ($diff_indent_heuristic) {
-		splice @diff_cmd, 1, 0, "--indent-heuristic";
-	}
 	if (defined $patch_mode_revision) {
 		push @diff_cmd, get_diff_reference($patch_mode_revision);
 	}
-- 
2.13.0.rc1.407.g644207685

Re: [PATCHv2 0/3] Make diff plumbing commands respect the indentHeuristic.

[Michael Haggerty]

On 04/29/2017 02:40 PM, Jeff King wrote:
> On Fri, Apr 28, 2017 at 06:33:12PM -0400, Marc Branchaud wrote:
> 
>> v2: Fixed up the commit messages and added tests.
>>
>> Marc Branchaud (2):
>>   diff: make the indent heuristic part of diff's basic configuration
>>   diff: have the diff-* builtins configure diff before initializing
>>     revisions
>>
>> Stefan Beller (1):
>>   diff: enable indent heuristic by default
> 
> Thanks, these look fine to me. I'd like to get an ACK from Michael, in
> case he had some other reason for omitting them from git_diff_ui_config
> (from my recollection, it's probably just a mix of conservatism and
> following what the compaction heuristic had done).

That's exactly right. The only discussion I remember about broadening
the scope of diff options was with regards to `blame` [1]. I don't
really have enough overview of these configuration topics to have much
an opinion.

Michael

[1]
http://public-inbox.org/git/xmqqtwebwhbg.fsf@gitster.mtv.corp.google.com/