threads / patch / 45821

patch, 2 partsHave the diff-* builtins configure diff before initializing revisions.

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

## tl;dr

35 messages between Apr 27, 2017 and May 9, 2017. Diffs are folded; open one to read it.

replies: 34people: 6as markdown or json

Marc Branchaud· Apr 27, 2017, 20:50 UTC · lore

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

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(-)
Marc Branchaud· Apr 27, 2017, 20:50 UTC · re: Marc Branchaud · lore

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(-)
Show changes to 3 files +3 −3

builtin/diff-files.c, builtin/diff-index.c, builtin/diff-tree.c

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
Jeff King· Apr 28, 2017, 08:06 UTC · re: Marc Branchaud · lore

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

On Thu, Apr 27, 2017 at 04:50:37PM -0400, Marc Branchaud wrote:
Show 6 quoted lines
> 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.

Show 14 quoted lines
> 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
Junio C Hamano· May 1, 2017, 01:01 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 21 quoted lines
> 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.

Show 9 quoted lines
> 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.
Jeff King· May 1, 2017, 05:17 UTC · re: Junio C Hamano · lore

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

On Sun, Apr 30, 2017 at 06:01:37PM -0700, Junio C Hamano wrote:
Show 14 quoted lines
> >> +	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
Jeff King· May 1, 2017, 05:29 UTC · re: Jeff King · lore

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

On Mon, May 01, 2017 at 01:17:58AM -0400, Jeff King wrote:
Show 10 quoted lines
> 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
Marc Branchaud· Apr 27, 2017, 20:50 UTC · re: Marc Branchaud · lore

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

Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
---
 diff.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Show changes to diff.c +3 −3
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
Jeff King· Apr 28, 2017, 07:59 UTC · re: Marc Branchaud · lore

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

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
Jeff King· Apr 28, 2017, 07:56 UTC · re: Marc Branchaud · lore

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

On Thu, Apr 27, 2017 at 04:50:35PM -0400, Marc Branchaud wrote:
Show 17 quoted lines
> 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.
Show 5 quoted lines
> 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
Stefan Beller· Apr 28, 2017, 17:34 UTC · re: Marc Branchaud · lore

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

On Thu, Apr 27, 2017 at 1:50 PM, Marc Branchaud <marcnarc@xiplink.com> wrote:
Show 28 quoted lines
> 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>
---
Show changes to diff.c +1 −1
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;
Jeff King· Apr 28, 2017, 22:04 UTC · re: Stefan Beller · lore

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

On Fri, Apr 28, 2017 at 10:34:15AM -0700, Stefan Beller wrote:
Show 9 quoted lines
> > 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
Stefan Beller· Apr 28, 2017, 22:13 UTC · re: Jeff King · lore

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

On Fri, Apr 28, 2017 at 3:04 PM, Jeff King <peff@peff.net> wrote:
Show 36 quoted lines
> 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. :)
Show 7 quoted lines
>> 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.
Show 13 quoted lines
> 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

Ævar Arnfjörð Bjarmason· May 1, 2017, 10:34 UTC · re: Jeff King · lore

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

On Sat, Apr 29, 2017 at 12:04 AM, Jeff King <peff@peff.net> wrote:
Show 56 quoted lines
> 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.

Jeff King· May 9, 2017, 03:16 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On Mon, May 01, 2017 at 12:34:38PM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 14 quoted lines
> > 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
Junio C Hamano· May 9, 2017, 04:06 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 6 quoted lines
> 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.

Ævar Arnfjörð Bjarmason· May 9, 2017, 07:58 UTC · re: Jeff King · lore

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

On Tue, May 9, 2017 at 5:16 AM, Jeff King <peff@peff.net> wrote:
Show 22 quoted lines
> 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.

Show 6 quoted lines
> 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.

Jeff King· May 9, 2017, 08:04 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On Tue, May 09, 2017 at 09:58:28AM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 8 quoted lines
> > 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
Marc Branchaud· Apr 28, 2017, 22:33 UTC · re: Marc Branchaud · lore

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

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
Marc Branchaud· Apr 28, 2017, 22:33 UTC · re: Marc Branchaud · lore

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

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(-)
Show changes to diff.c +3 −3
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
Marc Branchaud· Apr 28, 2017, 22:33 UTC · re: Marc Branchaud · lore

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

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(-)
Show changes to 4 files +69 −3

builtin/diff-files.c, builtin/diff-index.c, builtin/diff-tree.c, t/t4061-diff-indent.sh

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
Marc Branchaud· Apr 28, 2017, 22:33 UTC · re: Marc Branchaud · lore

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

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(-)
Show changes to diff.c +1 −1
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
Jeff King· Apr 29, 2017, 12:40 UTC · re: Marc Branchaud · lore

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

On Fri, Apr 28, 2017 at 06:33:12PM -0400, Marc Branchaud wrote:
Show 9 quoted lines
> 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
Jeff King· Apr 29, 2017, 13:14 UTC · re: Jeff King · lore

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

On Sat, Apr 29, 2017 at 08:40:52AM -0400, Jeff King wrote:
Show 16 quoted lines
> 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.

Show changes to 2 files +4 −4

t/t4051-diff-function-context.sh, t/t4061-diff-indent.sh

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
 '
Junio C Hamano· May 1, 2017, 01:11 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 10 quoted lines
> 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.
Jeff King· May 1, 2017, 05:15 UTC · re: Junio C Hamano · lore

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

On Sun, Apr 30, 2017 at 06:11:48PM -0700, Junio C Hamano wrote:
Show 17 quoted lines
> 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.
Junio C Hamano· May 1, 2017, 23:25 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 7 quoted lines
> 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.

Marc Branchaud· May 1, 2017, 22:13 UTC · re: Jeff King · lore

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

On 2017-04-29 09:14 AM, Jeff King wrote:
Show 21 quoted lines
> 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
Marc Branchaud· May 1, 2017, 22:13 UTC · re: Marc Branchaud · lore

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

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(-)
Show changes to 4 files +69 −3

builtin/diff-files.c, builtin/diff-index.c, builtin/diff-tree.c, t/t4061-diff-indent.sh

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
Marc Branchaud· May 1, 2017, 22:13 UTC · re: Marc Branchaud · lore

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

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(-)
Show changes to diff.c +3 −3
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
Marc Branchaud· May 1, 2017, 22:13 UTC · re: Marc Branchaud · lore

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

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(-)
Show changes to git-add--interactive.perl +0 −4
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
Marc Branchaud· May 1, 2017, 22:13 UTC · re: Marc Branchaud · lore

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

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(-)
Show changes to 3 files +6 −5

diff.c, t/t4051-diff-function-context.sh, t/t4061-diff-indent.sh

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
Jeff King· May 1, 2017, 22:20 UTC · re: Marc Branchaud · lore

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

On Mon, May 01, 2017 at 06:13:44PM -0400, Marc Branchaud wrote:
Show 23 quoted lines
> 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.

Show 12 quoted lines
> 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
Stefan Beller· May 1, 2017, 22:18 UTC · re: Marc Branchaud · lore

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

Show 7 quoted lines
> 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

Jeff King· Apr 29, 2017, 13:15 UTC · re: Jeff King · lore

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

On Sat, Apr 29, 2017 at 08:40:52AM -0400, Jeff King wrote:
Show 16 quoted lines
> 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(-)
Show changes to git-add--interactive.perl +0 −4
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
Michael Haggerty· Apr 30, 2017, 03:26 UTC · re: Jeff King · lore

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

On 04/29/2017 02:40 PM, Jeff King wrote:
Show 16 quoted lines
> 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/

← back to recent threads