[PATCH] diff: prefer indent heuristic over compaction heuristic

[PATCH] diff: prefer indent heuristic over compaction heuristic

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

The current configuration code for enabling experimental heuristics
prefers the last-set heuristic in the configuration. However, it is not
necessarily easy to see what order the configuration will be read. This
means that it is possible for a user to have accidentally enabled both
heuristics, and end up only enabling the older compaction heuristic.

Modify the code so that we do not clear the other heuristic when we set
each heuristic enabled. Then, during diff_setup() when we check the
configuration, we will first check the newer indent heuristic. This
ensures that we only enable the newer heuristic if both have been
enabled.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 diff.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/diff.c b/diff.c
index ec8728362dae..48a5b2797e3d 100644
--- a/diff.c
+++ b/diff.c
@@ -223,16 +223,10 @@ void init_diff_ui_defaults(void)
 
 int git_diff_heuristic_config(const char *var, const char *value, void *cb)
 {
-	if (!strcmp(var, "diff.indentheuristic")) {
+	if (!strcmp(var, "diff.indentheuristic"))
 		diff_indent_heuristic = git_config_bool(var, value);
-		if (diff_indent_heuristic)
-			diff_compaction_heuristic = 0;
-	}
-	if (!strcmp(var, "diff.compactionheuristic")) {
+	if (!strcmp(var, "diff.compactionheuristic"))
 		diff_compaction_heuristic = git_config_bool(var, value);
-		if (diff_compaction_heuristic)
-			diff_indent_heuristic = 0;
-	}
 	return 0;
 }
 
-- 
2.11.0.rc2.152.g4d04e67

Re: [PATCH] diff: prefer indent heuristic over compaction heuristic

[Junio C Hamano]

Jacob Keller <jacob.e.keller@intel.com> writes:

> From: Jacob Keller <jacob.keller@gmail.com>
>
> The current configuration code for enabling experimental heuristics
> prefers the last-set heuristic in the configuration. However, it is not
> necessarily easy to see what order the configuration will be read. This
> means that it is possible for a user to have accidentally enabled both
> heuristics, and end up only enabling the older compaction heuristic.
>
> Modify the code so that we do not clear the other heuristic when we set
> each heuristic enabled. Then, during diff_setup() when we check the
> configuration, we will first check the newer indent heuristic. This
> ensures that we only enable the newer heuristic if both have been
> enabled.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
>  diff.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)

Although I do not think we should spend too much braincycles on this
one (we should rather just removing the older one soonish), I think
this patch is going in a wrong direction.  I agree that "the last
one wins" is a bit hard to see (until you check with "git config -l"
perhaps) but it at least is predictable.  With this patch, you need
to KNOW that indent wins over compaction, perhaps by knowing the
order they were developed, which demands a lot more from the users.

We probably should just keep one and remove the other.

> diff --git a/diff.c b/diff.c
> index ec8728362dae..48a5b2797e3d 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -223,16 +223,10 @@ void init_diff_ui_defaults(void)
>  
>  int git_diff_heuristic_config(const char *var, const char *value, void *cb)
>  {
> +	if (!strcmp(var, "diff.indentheuristic"))
>  		diff_indent_heuristic = git_config_bool(var, value);
> +	if (!strcmp(var, "diff.compactionheuristic"))
>  		diff_compaction_heuristic = git_config_bool(var, value);
>  	return 0;
>  }

Re: [PATCH] diff: prefer indent heuristic over compaction heuristic

[Jacob Keller]

On Fri, Dec 16, 2016 at 5:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Although I do not think we should spend too much braincycles on this
> one (we should rather just removing the older one soonish), I think
> this patch is going in a wrong direction.  I agree that "the last
> one wins" is a bit hard to see (until you check with "git config -l"
> perhaps) but it at least is predictable.  With this patch, you need
> to KNOW that indent wins over compaction, perhaps by knowing the
> order they were developed, which demands a lot more from the users.
>

I don't think we have too many config options that interact in this
way, so I understand that "last writing of a particular configuration"
makes sense, but interactions between configs is something that would
have never occurred to me. I'll send a patch to drop the compaction
heuristic since I think we're all 100% in agreement that it is
superseded by the new configuration (as no case has been shown where
the new one is worse than compaction, and most show it to be better).

Thanks,
Jake

Re: [PATCH] diff: prefer indent heuristic over compaction heuristic

[Junio C Hamano]

Jacob Keller <jacob.keller@gmail.com> writes:

> I don't think we have too many config options that interact in this
> way, so I understand that "last writing of a particular configuration"
> makes sense, but interactions between configs is something that would
> have never occurred to me. I'll send a patch to drop the compaction
> heuristic since I think we're all 100% in agreement that it is
> superseded by the new configuration (as no case has been shown where
> the new one is worse than compaction, and most show it to be better).

If I recall correctly, we agreed that we'll drop the implementation
of compaction, but use the name --compaction-heuristics to trigger
the new and improved "indent heuristics":

    <20161101205916.d74n6lhgp2hexpzr@sigill.intra.peff.net>

So let's do this.

-- >8 --
Subject: [PATCH] diff: retire the original experimental "compaction" heuristics

This retires the experimental "compaction" heuristics but with a
twist.  It removes the mention of "indent" heuristics, which was a
competing experiment, from everywhere, guts the core logic of the
original "compaction" heuristics out and replaces it with the logic
used by the "indent" heuristics.

The externally visible effect of this change is that people who have
been experimenting by setting diff.compactionHeuristic configuration
or giving the command line option --compaction-heuristic will start
getting the behaviour based on the improved heuristics that used to
be called "indent" heuristics.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/diff-config.txt            |  6 ++---
 Documentation/diff-heuristic-options.txt |  2 --
 builtin/blame.c                          |  3 +--
 diff.c                                   | 25 ++++-----------------
 git-add--interactive.perl                |  5 +----
 t/t4061-diff-indent.sh                   | 38 ++++++++++++++++----------------
 xdiff/xdiff.h                            |  1 -
 xdiff/xdiffi.c                           | 37 +++----------------------------
 8 files changed, 30 insertions(+), 87 deletions(-)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 58f4bd6afa..39fff3aef9 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -171,11 +171,9 @@ diff.tool::
 
 include::mergetools-diff.txt[]
 
-diff.indentHeuristic::
 diff.compactionHeuristic::
-	Set one of these options to `true` to enable one of two
-	experimental heuristics that shift diff hunk boundaries to
-	make patches easier to read.
+	Set this option to `true` to enable experimental heuristics
+	that shift diff hunk boundaries to make patches easier to read.
 
 diff.algorithm::
 	Choose a diff algorithm.  The variants are as follows:
diff --git a/Documentation/diff-heuristic-options.txt b/Documentation/diff-heuristic-options.txt
index 36cb549df9..3cb024aa22 100644
--- a/Documentation/diff-heuristic-options.txt
+++ b/Documentation/diff-heuristic-options.txt
@@ -1,5 +1,3 @@
---indent-heuristic::
---no-indent-heuristic::
 --compaction-heuristic::
 --no-compaction-heuristic::
 	These are to help debugging and tuning experimental heuristics
diff --git a/builtin/blame.c b/builtin/blame.c
index 4ddfadb71f..395d4011fb 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2596,7 +2596,6 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		 * and are only included here to get included in the "-h"
 		 * output:
 		 */
-		{ OPTION_LOWLEVEL_CALLBACK, 0, "indent-heuristic", NULL, NULL, N_("Use an experimental indent-based heuristic to improve diffs"), PARSE_OPT_NOARG, parse_opt_unknown_cb },
 		{ OPTION_LOWLEVEL_CALLBACK, 0, "compaction-heuristic", NULL, NULL, N_("Use an experimental blank-line-based heuristic to improve diffs"), PARSE_OPT_NOARG, parse_opt_unknown_cb },
 
 		OPT_BIT(0, "minimal", &xdl_opts, N_("Spend extra cycles to find better match"), XDF_NEED_MINIMAL),
@@ -2645,7 +2644,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	}
 parse_done:
 	no_whole_file_rename = !DIFF_OPT_TST(&revs.diffopt, FOLLOW_RENAMES);
-	xdl_opts |= revs.diffopt.xdl_opts & (XDF_COMPACTION_HEURISTIC | XDF_INDENT_HEURISTIC);
+	xdl_opts |= revs.diffopt.xdl_opts & XDF_COMPACTION_HEURISTIC;
 	DIFF_OPT_CLR(&revs.diffopt, FOLLOW_RENAMES);
 	argc = parse_options_end(&ctx);
 
diff --git a/diff.c b/diff.c
index 8981477c43..f1b01f5b1e 100644
--- a/diff.c
+++ b/diff.c
@@ -27,7 +27,6 @@
 #endif
 
 static int diff_detect_rename_default;
-static int diff_indent_heuristic; /* experimental */
 static int diff_compaction_heuristic; /* experimental */
 static int diff_rename_limit_default = 400;
 static int diff_suppress_blank_empty;
@@ -223,16 +222,8 @@ void init_diff_ui_defaults(void)
 
 int git_diff_heuristic_config(const char *var, const char *value, void *cb)
 {
-	if (!strcmp(var, "diff.indentheuristic")) {
-		diff_indent_heuristic = git_config_bool(var, value);
-		if (diff_indent_heuristic)
-			diff_compaction_heuristic = 0;
-	}
-	if (!strcmp(var, "diff.compactionheuristic")) {
+	if (!strcmp(var, "diff.compactionheuristic"))
 		diff_compaction_heuristic = git_config_bool(var, value);
-		if (diff_compaction_heuristic)
-			diff_indent_heuristic = 0;
-	}
 	return 0;
 }
 
@@ -3378,9 +3369,7 @@ void diff_setup(struct diff_options *options)
 	options->use_color = diff_use_color_default;
 	options->detect_rename = diff_detect_rename_default;
 	options->xdl_opts |= diff_algorithm;
-	if (diff_indent_heuristic)
-		DIFF_XDL_SET(options, INDENT_HEURISTIC);
-	else if (diff_compaction_heuristic)
+	if (diff_compaction_heuristic)
 		DIFF_XDL_SET(options, COMPACTION_HEURISTIC);
 
 	options->orderfile = diff_order_file_cfg;
@@ -3876,15 +3865,9 @@ int diff_opt_parse(struct diff_options *options,
 		DIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);
 	else if (!strcmp(arg, "--ignore-blank-lines"))
 		DIFF_XDL_SET(options, IGNORE_BLANK_LINES);
-	else if (!strcmp(arg, "--indent-heuristic")) {
-		DIFF_XDL_SET(options, INDENT_HEURISTIC);
-		DIFF_XDL_CLR(options, COMPACTION_HEURISTIC);
-	} else if (!strcmp(arg, "--no-indent-heuristic"))
-		DIFF_XDL_CLR(options, INDENT_HEURISTIC);
-	else if (!strcmp(arg, "--compaction-heuristic")) {
+	else if (!strcmp(arg, "--compaction-heuristic"))
 		DIFF_XDL_SET(options, COMPACTION_HEURISTIC);
-		DIFF_XDL_CLR(options, INDENT_HEURISTIC);
-	} else if (!strcmp(arg, "--no-compaction-heuristic"))
+	else if (!strcmp(arg, "--no-compaction-heuristic"))
 		DIFF_XDL_CLR(options, COMPACTION_HEURISTIC);
 	else if (!strcmp(arg, "--patience"))
 		options->xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index ee3d812695..642cce1ac6 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -45,7 +45,6 @@
 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_compaction_heuristic = $repo->config_bool('diff.compactionheuristic');
 my $diff_filter = $repo->config('interactive.difffilter');
 
@@ -751,9 +750,7 @@ 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";
-	} elsif ($diff_compaction_heuristic) {
+	if ($diff_compaction_heuristic) {
 		splice @diff_cmd, 1, 0, "--compaction-heuristic";
 	}
 	if (defined $patch_mode_revision) {
diff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh
index 556450609b..30f809d0d3 100755
--- a/t/t4061-diff-indent.sh
+++ b/t/t4061-diff-indent.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-test_description='Test diff indent heuristic.
+test_description='Test diff compaction heuristic.
 
 '
 . ./test-lib.sh
@@ -157,28 +157,28 @@ test_expect_success 'diff: ugly spaces' '
 	compare_diff spaces-expect out
 '
 
-test_expect_success 'diff: nice spaces with --indent-heuristic' '
-	git diff --indent-heuristic old new -- spaces.txt >out-compacted &&
+test_expect_success 'diff: nice spaces with --compaction-heuristic' '
+	git diff --compaction-heuristic old new -- spaces.txt >out-compacted &&
 	compare_diff spaces-compacted-expect out-compacted
 '
 
-test_expect_success 'diff: nice spaces with diff.indentHeuristic' '
-	git -c diff.indentHeuristic=true diff old new -- spaces.txt >out-compacted2 &&
+test_expect_success 'diff: nice spaces with diff.compactionHeuristic' '
+	git -c diff.compactionHeuristic=true diff old new -- spaces.txt >out-compacted2 &&
 	compare_diff spaces-compacted-expect out-compacted2
 '
 
-test_expect_success 'diff: --no-indent-heuristic overrides config' '
-	git -c diff.indentHeuristic=true diff --no-indent-heuristic old new -- spaces.txt >out2 &&
+test_expect_success 'diff: --no-compaction-heuristic overrides config' '
+	git -c diff.compactionHeuristic=true diff --no-compaction-heuristic old new -- spaces.txt >out2 &&
 	compare_diff spaces-expect out2
 '
 
-test_expect_success 'diff: --indent-heuristic with --patience' '
-	git diff --indent-heuristic --patience old new -- spaces.txt >out-compacted3 &&
+test_expect_success 'diff: --compaction-heuristic with --patience' '
+	git diff --compaction-heuristic --patience old new -- spaces.txt >out-compacted3 &&
 	compare_diff spaces-compacted-expect out-compacted3
 '
 
-test_expect_success 'diff: --indent-heuristic with --histogram' '
-	git diff --indent-heuristic --histogram old new -- spaces.txt >out-compacted4 &&
+test_expect_success 'diff: --compaction-heuristic with --histogram' '
+	git diff --compaction-heuristic --histogram old new -- spaces.txt >out-compacted4 &&
 	compare_diff spaces-compacted-expect out-compacted4
 '
 
@@ -187,8 +187,8 @@ test_expect_success 'diff: ugly functions' '
 	compare_diff functions-expect out
 '
 
-test_expect_success 'diff: nice functions with --indent-heuristic' '
-	git diff --indent-heuristic old new -- functions.c >out-compacted &&
+test_expect_success 'diff: nice functions with --compaction-heuristic' '
+	git diff --compaction-heuristic old new -- functions.c >out-compacted &&
 	compare_diff functions-compacted-expect out-compacted
 '
 
@@ -197,18 +197,18 @@ test_expect_success 'blame: ugly spaces' '
 	compare_blame spaces-expect out-blame
 '
 
-test_expect_success 'blame: nice spaces with --indent-heuristic' '
-	git blame --indent-heuristic old..new -- spaces.txt >out-blame-compacted &&
+test_expect_success 'blame: nice spaces with --compaction-heuristic' '
+	git blame --compaction-heuristic old..new -- spaces.txt >out-blame-compacted &&
 	compare_blame spaces-compacted-expect out-blame-compacted
 '
 
-test_expect_success 'blame: nice spaces with diff.indentHeuristic' '
-	git -c diff.indentHeuristic=true blame old..new -- spaces.txt >out-blame-compacted2 &&
+test_expect_success 'blame: nice spaces with diff.compactionHeuristic' '
+	git -c diff.compactionHeuristic=true blame old..new -- spaces.txt >out-blame-compacted2 &&
 	compare_blame spaces-compacted-expect out-blame-compacted2
 '
 
-test_expect_success 'blame: --no-indent-heuristic overrides config' '
-	git -c diff.indentHeuristic=true blame --no-indent-heuristic old..new -- spaces.txt >out-blame2 &&
+test_expect_success 'blame: --no-compaction-heuristic overrides config' '
+	git -c diff.compactionHeuristic=true blame --no-compaction-heuristic old..new -- spaces.txt >out-blame2 &&
 	git blame old..new -- spaces.txt >out-blame &&
 	compare_blame spaces-expect out-blame2
 '
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index 8db16d4ae6..7423f77fc8 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -42,7 +42,6 @@ extern "C" {
 #define XDF_IGNORE_BLANK_LINES (1 << 7)
 
 #define XDF_COMPACTION_HEURISTIC (1 << 8)
-#define XDF_INDENT_HEURISTIC (1 << 9)
 
 #define XDL_EMIT_FUNCNAMES (1 << 0)
 #define XDL_EMIT_FUNCCONTEXT (1 << 2)
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 760fbb6db7..2131ea4920 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -400,11 +400,6 @@ static xdchange_t *xdl_add_change(xdchange_t *xscr, long i1, long i2, long chg1,
 }
 
 
-static int is_blank_line(xrecord_t *rec, long flags)
-{
-	return xdl_blankline(rec->ptr, rec->size, flags);
-}
-
 static int recs_match(xrecord_t *rec1, xrecord_t *rec2, long flags)
 {
 	return (rec1->ha == rec2->ha &&
@@ -821,7 +816,6 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
 	struct xdlgroup g, go;
 	long earliest_end, end_matching_other;
 	long groupsize;
-	unsigned int blank_lines;
 
 	group_init(xdf, &g);
 	group_init(xdfo, &go);
@@ -846,13 +840,6 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
 			 */
 			end_matching_other = -1;
 
-			/*
-			 * Boolean value that records whether there are any blank
-			 * lines that could be made to be the last line of this
-			 * group.
-			 */
-			blank_lines = 0;
-
 			/* Shift the group backward as much as possible: */
 			while (!group_slide_up(xdf, &g, flags))
 				if (group_previous(xdfo, &go))
@@ -869,11 +856,6 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
 
 			/* Now shift the group forward as far as possible: */
 			while (1) {
-				if (!blank_lines)
-					blank_lines = is_blank_line(
-							xdf->recs[g.end - 1],
-							flags);
-
 				if (group_slide_down(xdf, &g, flags))
 					break;
 				if (group_next(xdfo, &go))
@@ -906,24 +888,11 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
 				if (group_previous(xdfo, &go))
 					xdl_bug("group sync broken sliding to match");
 			}
-		} else if ((flags & XDF_COMPACTION_HEURISTIC) && blank_lines) {
+		} else if (flags & XDF_COMPACTION_HEURISTIC) {
 			/*
-			 * Compaction heuristic: if it is possible to shift the
-			 * group to make its bottom line a blank line, do so.
+			 * Heuristic based on the indentation level.
 			 *
-			 * As we already shifted the group forward as far as
-			 * possible in the earlier loop, we only need to handle
-			 * backward shifts, not forward ones.
-			 */
-			while (!is_blank_line(xdf->recs[g.end - 1], flags)) {
-				if (group_slide_up(xdf, &g, flags))
-					xdl_bug("blank line disappeared");
-				if (group_previous(xdfo, &go))
-					xdl_bug("group sync broken sliding to blank line");
-			}
-		} else if (flags & XDF_INDENT_HEURISTIC) {
-			/*
-			 * Indent heuristic: a group of pure add/delete lines
+			 * A group of pure add/delete lines
 			 * implies two splits, one between the end of the "before"
 			 * context and the start of the group, and another between
 			 * the end of the group and the beginning of the "after"
-- 
2.11.0-448-g9a11f8a62b

Re: [PATCH] diff: prefer indent heuristic over compaction heuristic

[Jacob Keller]

On Thu, Dec 22, 2016 at 1:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> I don't think we have too many config options that interact in this
>> way, so I understand that "last writing of a particular configuration"
>> makes sense, but interactions between configs is something that would
>> have never occurred to me. I'll send a patch to drop the compaction
>> heuristic since I think we're all 100% in agreement that it is
>> superseded by the new configuration (as no case has been shown where
>> the new one is worse than compaction, and most show it to be better).
>
> If I recall correctly, we agreed that we'll drop the implementation
> of compaction, but use the name --compaction-heuristics to trigger
> the new and improved "indent heuristics":
>
>     <20161101205916.d74n6lhgp2hexpzr@sigill.intra.peff.net>
>

That sounds familiar.

> So let's do this.
>
> -- >8 --
> Subject: [PATCH] diff: retire the original experimental "compaction" heuristics
>
> This retires the experimental "compaction" heuristics but with a
> twist.  It removes the mention of "indent" heuristics, which was a
> competing experiment, from everywhere, guts the core logic of the
> original "compaction" heuristics out and replaces it with the logic
> used by the "indent" heuristics.
>
> The externally visible effect of this change is that people who have
> been experimenting by setting diff.compactionHeuristic configuration
> or giving the command line option --compaction-heuristic will start
> getting the behaviour based on the improved heuristics that used to
> be called "indent" heuristics.
>
> Suggested-by: Jeff King <peff@peff.net>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/diff-config.txt            |  6 ++---
>  Documentation/diff-heuristic-options.txt |  2 --
>  builtin/blame.c                          |  3 +--
>  diff.c                                   | 25 ++++-----------------
>  git-add--interactive.perl                |  5 +----
>  t/t4061-diff-indent.sh                   | 38 ++++++++++++++++----------------
>  xdiff/xdiff.h                            |  1 -
>  xdiff/xdiffi.c                           | 37 +++----------------------------
>  8 files changed, 30 insertions(+), 87 deletions(-)
>
> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
> index 58f4bd6afa..39fff3aef9 100644
> --- a/Documentation/diff-config.txt
> +++ b/Documentation/diff-config.txt
> @@ -171,11 +171,9 @@ diff.tool::
>
>  include::mergetools-diff.txt[]
>
> -diff.indentHeuristic::
>  diff.compactionHeuristic::
> -       Set one of these options to `true` to enable one of two
> -       experimental heuristics that shift diff hunk boundaries to
> -       make patches easier to read.
> +       Set this option to `true` to enable experimental heuristics
> +       that shift diff hunk boundaries to make patches easier to read.
>
>  diff.algorithm::
>         Choose a diff algorithm.  The variants are as follows:
> diff --git a/Documentation/diff-heuristic-options.txt b/Documentation/diff-heuristic-options.txt
> index 36cb549df9..3cb024aa22 100644
> --- a/Documentation/diff-heuristic-options.txt
> +++ b/Documentation/diff-heuristic-options.txt
> @@ -1,5 +1,3 @@
> ---indent-heuristic::
> ---no-indent-heuristic::
>  --compaction-heuristic::
>  --no-compaction-heuristic::
>         These are to help debugging and tuning experimental heuristics
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 4ddfadb71f..395d4011fb 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2596,7 +2596,6 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>                  * and are only included here to get included in the "-h"
>                  * output:
>                  */
> -               { OPTION_LOWLEVEL_CALLBACK, 0, "indent-heuristic", NULL, NULL, N_("Use an experimental indent-based heuristic to improve diffs"), PARSE_OPT_NOARG, parse_opt_unknown_cb },
>                 { OPTION_LOWLEVEL_CALLBACK, 0, "compaction-heuristic", NULL, NULL, N_("Use an experimental blank-line-based heuristic to improve diffs"), PARSE_OPT_NOARG, parse_opt_unknown_cb },
>

The unchanged context line should have its description re-worded to
something like "Use an experimental heuristic to improve diffs" as it
no longer uses only blank lines.

Everything else looked correct.

Thanks,
Jake

Re: [PATCH] diff: prefer indent heuristic over compaction heuristic

[Junio C Hamano]

Jacob Keller <jacob.keller@gmail.com> writes:

>>                 { OPTION_LOWLEVEL_CALLBACK, 0, "compaction-heuristic", NULL, NULL, N_("Use an experimental blank-line-based heuristic to improve diffs"), PARSE_OPT_NOARG, parse_opt_unknown_cb },
>>
>
> The unchanged context line should have its description re-worded to
> something like "Use an experimental heuristic to improve diffs" as it
> no longer uses only blank lines.

Thanks.  The final copy I pushed out has that change.

Re: [PATCH] diff: prefer indent heuristic over compaction heuristic

[Jeff King]

On Thu, Dec 22, 2016 at 01:12:12PM -0800, Junio C Hamano wrote:

> Jacob Keller <jacob.keller@gmail.com> writes:
> 
> > I don't think we have too many config options that interact in this
> > way, so I understand that "last writing of a particular configuration"
> > makes sense, but interactions between configs is something that would
> > have never occurred to me. I'll send a patch to drop the compaction
> > heuristic since I think we're all 100% in agreement that it is
> > superseded by the new configuration (as no case has been shown where
> > the new one is worse than compaction, and most show it to be better).
> 
> If I recall correctly, we agreed that we'll drop the implementation
> of compaction, but use the name --compaction-heuristics to trigger
> the new and improved "indent heuristics":
> 
>     <20161101205916.d74n6lhgp2hexpzr@sigill.intra.peff.net>

FWIW, I was swayed in the other direction by later messages in the
thread. Especially your noting that the "compaction" name has always
been labeled experimental, and Michael's argument in:

  http://public-inbox.org/git/8dbbd28b-af60-5e66-ae27-d7cddca233dc@alum.mit.edu/

I.e., we could keep calling it "--indent-heuristic", and probably drop
the other heuristic entirely as a failed experiment.

I can live with it either way, but since I am being quoted as the source
of the suggestion, I feel like that's an invitation to add my 2 cents. :)

Liberal quoting below since I am adding Michael to the cc list.

-Peff

> So let's do this.
> 
> -- >8 --
> Subject: [PATCH] diff: retire the original experimental "compaction" heuristics
> 
> This retires the experimental "compaction" heuristics but with a
> twist.  It removes the mention of "indent" heuristics, which was a
> competing experiment, from everywhere, guts the core logic of the
> original "compaction" heuristics out and replaces it with the logic
> used by the "indent" heuristics.
> 
> The externally visible effect of this change is that people who have
> been experimenting by setting diff.compactionHeuristic configuration
> or giving the command line option --compaction-heuristic will start
> getting the behaviour based on the improved heuristics that used to
> be called "indent" heuristics.
> 
> Suggested-by: Jeff King <peff@peff.net>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/diff-config.txt            |  6 ++---
>  Documentation/diff-heuristic-options.txt |  2 --
>  builtin/blame.c                          |  3 +--
>  diff.c                                   | 25 ++++-----------------
>  git-add--interactive.perl                |  5 +----
>  t/t4061-diff-indent.sh                   | 38 ++++++++++++++++----------------
>  xdiff/xdiff.h                            |  1 -
>  xdiff/xdiffi.c                           | 37 +++----------------------------
>  8 files changed, 30 insertions(+), 87 deletions(-)
> 
> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
> index 58f4bd6afa..39fff3aef9 100644
> --- a/Documentation/diff-config.txt
> +++ b/Documentation/diff-config.txt
> @@ -171,11 +171,9 @@ diff.tool::
>  
>  include::mergetools-diff.txt[]
>  
> -diff.indentHeuristic::
>  diff.compactionHeuristic::
> -	Set one of these options to `true` to enable one of two
> -	experimental heuristics that shift diff hunk boundaries to
> -	make patches easier to read.
> +	Set this option to `true` to enable experimental heuristics
> +	that shift diff hunk boundaries to make patches easier to read.
>  
>  diff.algorithm::
>  	Choose a diff algorithm.  The variants are as follows:
> diff --git a/Documentation/diff-heuristic-options.txt b/Documentation/diff-heuristic-options.txt
> index 36cb549df9..3cb024aa22 100644
> --- a/Documentation/diff-heuristic-options.txt
> +++ b/Documentation/diff-heuristic-options.txt
> @@ -1,5 +1,3 @@
> ---indent-heuristic::
> ---no-indent-heuristic::
>  --compaction-heuristic::
>  --no-compaction-heuristic::
>  	These are to help debugging and tuning experimental heuristics
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 4ddfadb71f..395d4011fb 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2596,7 +2596,6 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>  		 * and are only included here to get included in the "-h"
>  		 * output:
>  		 */
> -		{ OPTION_LOWLEVEL_CALLBACK, 0, "indent-heuristic", NULL, NULL, N_("Use an experimental indent-based heuristic to improve diffs"), PARSE_OPT_NOARG, parse_opt_unknown_cb },
>  		{ OPTION_LOWLEVEL_CALLBACK, 0, "compaction-heuristic", NULL, NULL, N_("Use an experimental blank-line-based heuristic to improve diffs"), PARSE_OPT_NOARG, parse_opt_unknown_cb },
>  
>  		OPT_BIT(0, "minimal", &xdl_opts, N_("Spend extra cycles to find better match"), XDF_NEED_MINIMAL),
> @@ -2645,7 +2644,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>  	}
>  parse_done:
>  	no_whole_file_rename = !DIFF_OPT_TST(&revs.diffopt, FOLLOW_RENAMES);
> -	xdl_opts |= revs.diffopt.xdl_opts & (XDF_COMPACTION_HEURISTIC | XDF_INDENT_HEURISTIC);
> +	xdl_opts |= revs.diffopt.xdl_opts & XDF_COMPACTION_HEURISTIC;
>  	DIFF_OPT_CLR(&revs.diffopt, FOLLOW_RENAMES);
>  	argc = parse_options_end(&ctx);
>  
> diff --git a/diff.c b/diff.c
> index 8981477c43..f1b01f5b1e 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -27,7 +27,6 @@
>  #endif
>  
>  static int diff_detect_rename_default;
> -static int diff_indent_heuristic; /* experimental */
>  static int diff_compaction_heuristic; /* experimental */
>  static int diff_rename_limit_default = 400;
>  static int diff_suppress_blank_empty;
> @@ -223,16 +222,8 @@ void init_diff_ui_defaults(void)
>  
>  int git_diff_heuristic_config(const char *var, const char *value, void *cb)
>  {
> -	if (!strcmp(var, "diff.indentheuristic")) {
> -		diff_indent_heuristic = git_config_bool(var, value);
> -		if (diff_indent_heuristic)
> -			diff_compaction_heuristic = 0;
> -	}
> -	if (!strcmp(var, "diff.compactionheuristic")) {
> +	if (!strcmp(var, "diff.compactionheuristic"))
>  		diff_compaction_heuristic = git_config_bool(var, value);
> -		if (diff_compaction_heuristic)
> -			diff_indent_heuristic = 0;
> -	}
>  	return 0;
>  }
>  
> @@ -3378,9 +3369,7 @@ void diff_setup(struct diff_options *options)
>  	options->use_color = diff_use_color_default;
>  	options->detect_rename = diff_detect_rename_default;
>  	options->xdl_opts |= diff_algorithm;
> -	if (diff_indent_heuristic)
> -		DIFF_XDL_SET(options, INDENT_HEURISTIC);
> -	else if (diff_compaction_heuristic)
> +	if (diff_compaction_heuristic)
>  		DIFF_XDL_SET(options, COMPACTION_HEURISTIC);
>  
>  	options->orderfile = diff_order_file_cfg;
> @@ -3876,15 +3865,9 @@ int diff_opt_parse(struct diff_options *options,
>  		DIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);
>  	else if (!strcmp(arg, "--ignore-blank-lines"))
>  		DIFF_XDL_SET(options, IGNORE_BLANK_LINES);
> -	else if (!strcmp(arg, "--indent-heuristic")) {
> -		DIFF_XDL_SET(options, INDENT_HEURISTIC);
> -		DIFF_XDL_CLR(options, COMPACTION_HEURISTIC);
> -	} else if (!strcmp(arg, "--no-indent-heuristic"))
> -		DIFF_XDL_CLR(options, INDENT_HEURISTIC);
> -	else if (!strcmp(arg, "--compaction-heuristic")) {
> +	else if (!strcmp(arg, "--compaction-heuristic"))
>  		DIFF_XDL_SET(options, COMPACTION_HEURISTIC);
> -		DIFF_XDL_CLR(options, INDENT_HEURISTIC);
> -	} else if (!strcmp(arg, "--no-compaction-heuristic"))
> +	else if (!strcmp(arg, "--no-compaction-heuristic"))
>  		DIFF_XDL_CLR(options, COMPACTION_HEURISTIC);
>  	else if (!strcmp(arg, "--patience"))
>  		options->xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index ee3d812695..642cce1ac6 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -45,7 +45,6 @@
>  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_compaction_heuristic = $repo->config_bool('diff.compactionheuristic');
>  my $diff_filter = $repo->config('interactive.difffilter');
>  
> @@ -751,9 +750,7 @@ 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";
> -	} elsif ($diff_compaction_heuristic) {
> +	if ($diff_compaction_heuristic) {
>  		splice @diff_cmd, 1, 0, "--compaction-heuristic";
>  	}
>  	if (defined $patch_mode_revision) {
> diff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh
> index 556450609b..30f809d0d3 100755
> --- a/t/t4061-diff-indent.sh
> +++ b/t/t4061-diff-indent.sh
> @@ -1,6 +1,6 @@
>  #!/bin/sh
>  
> -test_description='Test diff indent heuristic.
> +test_description='Test diff compaction heuristic.
>  
>  '
>  . ./test-lib.sh
> @@ -157,28 +157,28 @@ test_expect_success 'diff: ugly spaces' '
>  	compare_diff spaces-expect out
>  '
>  
> -test_expect_success 'diff: nice spaces with --indent-heuristic' '
> -	git diff --indent-heuristic old new -- spaces.txt >out-compacted &&
> +test_expect_success 'diff: nice spaces with --compaction-heuristic' '
> +	git diff --compaction-heuristic old new -- spaces.txt >out-compacted &&
>  	compare_diff spaces-compacted-expect out-compacted
>  '
>  
> -test_expect_success 'diff: nice spaces with diff.indentHeuristic' '
> -	git -c diff.indentHeuristic=true diff old new -- spaces.txt >out-compacted2 &&
> +test_expect_success 'diff: nice spaces with diff.compactionHeuristic' '
> +	git -c diff.compactionHeuristic=true diff old new -- spaces.txt >out-compacted2 &&
>  	compare_diff spaces-compacted-expect out-compacted2
>  '
>  
> -test_expect_success 'diff: --no-indent-heuristic overrides config' '
> -	git -c diff.indentHeuristic=true diff --no-indent-heuristic old new -- spaces.txt >out2 &&
> +test_expect_success 'diff: --no-compaction-heuristic overrides config' '
> +	git -c diff.compactionHeuristic=true diff --no-compaction-heuristic old new -- spaces.txt >out2 &&
>  	compare_diff spaces-expect out2
>  '
>  
> -test_expect_success 'diff: --indent-heuristic with --patience' '
> -	git diff --indent-heuristic --patience old new -- spaces.txt >out-compacted3 &&
> +test_expect_success 'diff: --compaction-heuristic with --patience' '
> +	git diff --compaction-heuristic --patience old new -- spaces.txt >out-compacted3 &&
>  	compare_diff spaces-compacted-expect out-compacted3
>  '
>  
> -test_expect_success 'diff: --indent-heuristic with --histogram' '
> -	git diff --indent-heuristic --histogram old new -- spaces.txt >out-compacted4 &&
> +test_expect_success 'diff: --compaction-heuristic with --histogram' '
> +	git diff --compaction-heuristic --histogram old new -- spaces.txt >out-compacted4 &&
>  	compare_diff spaces-compacted-expect out-compacted4
>  '
>  
> @@ -187,8 +187,8 @@ test_expect_success 'diff: ugly functions' '
>  	compare_diff functions-expect out
>  '
>  
> -test_expect_success 'diff: nice functions with --indent-heuristic' '
> -	git diff --indent-heuristic old new -- functions.c >out-compacted &&
> +test_expect_success 'diff: nice functions with --compaction-heuristic' '
> +	git diff --compaction-heuristic old new -- functions.c >out-compacted &&
>  	compare_diff functions-compacted-expect out-compacted
>  '
>  
> @@ -197,18 +197,18 @@ test_expect_success 'blame: ugly spaces' '
>  	compare_blame spaces-expect out-blame
>  '
>  
> -test_expect_success 'blame: nice spaces with --indent-heuristic' '
> -	git blame --indent-heuristic old..new -- spaces.txt >out-blame-compacted &&
> +test_expect_success 'blame: nice spaces with --compaction-heuristic' '
> +	git blame --compaction-heuristic old..new -- spaces.txt >out-blame-compacted &&
>  	compare_blame spaces-compacted-expect out-blame-compacted
>  '
>  
> -test_expect_success 'blame: nice spaces with diff.indentHeuristic' '
> -	git -c diff.indentHeuristic=true blame old..new -- spaces.txt >out-blame-compacted2 &&
> +test_expect_success 'blame: nice spaces with diff.compactionHeuristic' '
> +	git -c diff.compactionHeuristic=true blame old..new -- spaces.txt >out-blame-compacted2 &&
>  	compare_blame spaces-compacted-expect out-blame-compacted2
>  '
>  
> -test_expect_success 'blame: --no-indent-heuristic overrides config' '
> -	git -c diff.indentHeuristic=true blame --no-indent-heuristic old..new -- spaces.txt >out-blame2 &&
> +test_expect_success 'blame: --no-compaction-heuristic overrides config' '
> +	git -c diff.compactionHeuristic=true blame --no-compaction-heuristic old..new -- spaces.txt >out-blame2 &&
>  	git blame old..new -- spaces.txt >out-blame &&
>  	compare_blame spaces-expect out-blame2
>  '
> diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
> index 8db16d4ae6..7423f77fc8 100644
> --- a/xdiff/xdiff.h
> +++ b/xdiff/xdiff.h
> @@ -42,7 +42,6 @@ extern "C" {
>  #define XDF_IGNORE_BLANK_LINES (1 << 7)
>  
>  #define XDF_COMPACTION_HEURISTIC (1 << 8)
> -#define XDF_INDENT_HEURISTIC (1 << 9)
>  
>  #define XDL_EMIT_FUNCNAMES (1 << 0)
>  #define XDL_EMIT_FUNCCONTEXT (1 << 2)
> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
> index 760fbb6db7..2131ea4920 100644
> --- a/xdiff/xdiffi.c
> +++ b/xdiff/xdiffi.c
> @@ -400,11 +400,6 @@ static xdchange_t *xdl_add_change(xdchange_t *xscr, long i1, long i2, long chg1,
>  }
>  
>  
> -static int is_blank_line(xrecord_t *rec, long flags)
> -{
> -	return xdl_blankline(rec->ptr, rec->size, flags);
> -}
> -
>  static int recs_match(xrecord_t *rec1, xrecord_t *rec2, long flags)
>  {
>  	return (rec1->ha == rec2->ha &&
> @@ -821,7 +816,6 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
>  	struct xdlgroup g, go;
>  	long earliest_end, end_matching_other;
>  	long groupsize;
> -	unsigned int blank_lines;
>  
>  	group_init(xdf, &g);
>  	group_init(xdfo, &go);
> @@ -846,13 +840,6 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
>  			 */
>  			end_matching_other = -1;
>  
> -			/*
> -			 * Boolean value that records whether there are any blank
> -			 * lines that could be made to be the last line of this
> -			 * group.
> -			 */
> -			blank_lines = 0;
> -
>  			/* Shift the group backward as much as possible: */
>  			while (!group_slide_up(xdf, &g, flags))
>  				if (group_previous(xdfo, &go))
> @@ -869,11 +856,6 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
>  
>  			/* Now shift the group forward as far as possible: */
>  			while (1) {
> -				if (!blank_lines)
> -					blank_lines = is_blank_line(
> -							xdf->recs[g.end - 1],
> -							flags);
> -
>  				if (group_slide_down(xdf, &g, flags))
>  					break;
>  				if (group_next(xdfo, &go))
> @@ -906,24 +888,11 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
>  				if (group_previous(xdfo, &go))
>  					xdl_bug("group sync broken sliding to match");
>  			}
> -		} else if ((flags & XDF_COMPACTION_HEURISTIC) && blank_lines) {
> +		} else if (flags & XDF_COMPACTION_HEURISTIC) {
>  			/*
> -			 * Compaction heuristic: if it is possible to shift the
> -			 * group to make its bottom line a blank line, do so.
> +			 * Heuristic based on the indentation level.
>  			 *
> -			 * As we already shifted the group forward as far as
> -			 * possible in the earlier loop, we only need to handle
> -			 * backward shifts, not forward ones.
> -			 */
> -			while (!is_blank_line(xdf->recs[g.end - 1], flags)) {
> -				if (group_slide_up(xdf, &g, flags))
> -					xdl_bug("blank line disappeared");
> -				if (group_previous(xdfo, &go))
> -					xdl_bug("group sync broken sliding to blank line");
> -			}
> -		} else if (flags & XDF_INDENT_HEURISTIC) {
> -			/*
> -			 * Indent heuristic: a group of pure add/delete lines
> +			 * A group of pure add/delete lines
>  			 * implies two splits, one between the end of the "before"
>  			 * context and the start of the group, and another between
>  			 * the end of the group and the beginning of the "after"
> -- 
> 2.11.0-448-g9a11f8a62b
> 

Re: [PATCH] diff: prefer indent heuristic over compaction heuristic

[Jacob Keller]

On Thu, Dec 22, 2016 at 11:22 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Dec 22, 2016 at 01:12:12PM -0800, Junio C Hamano wrote:
>
>> Jacob Keller <jacob.keller@gmail.com> writes:
>>
>> > I don't think we have too many config options that interact in this
>> > way, so I understand that "last writing of a particular configuration"
>> > makes sense, but interactions between configs is something that would
>> > have never occurred to me. I'll send a patch to drop the compaction
>> > heuristic since I think we're all 100% in agreement that it is
>> > superseded by the new configuration (as no case has been shown where
>> > the new one is worse than compaction, and most show it to be better).
>>
>> If I recall correctly, we agreed that we'll drop the implementation
>> of compaction, but use the name --compaction-heuristics to trigger
>> the new and improved "indent heuristics":
>>
>>     <20161101205916.d74n6lhgp2hexpzr@sigill.intra.peff.net>
>
> FWIW, I was swayed in the other direction by later messages in the
> thread. Especially your noting that the "compaction" name has always
> been labeled experimental, and Michael's argument in:
>
>   http://public-inbox.org/git/8dbbd28b-af60-5e66-ae27-d7cddca233dc@alum.mit.edu/
>
> I.e., we could keep calling it "--indent-heuristic", and probably drop
> the other heuristic entirely as a failed experiment.
>
> I can live with it either way, but since I am being quoted as the source
> of the suggestion, I feel like that's an invitation to add my 2 cents. :)
>
> Liberal quoting below since I am adding Michael to the cc list.
>
> -Peff
>

I actually would prefer that we just say "this is the default now" and
provide some knob "no-indent-heuristic" or "no-compaction-heuristic"
and go with that, I think, since I am pretty sure we're all in
agreement that the heuristic is an improvement in almost every case,
certainly all the ones we've found. It's at least not worse in any
case I've seen, and is usually better.

Thoughts? I don't have a super strong opinion about which name we went
with for the knob.

Thanks,
Jake

Re: [PATCH] diff: prefer indent heuristic over compaction heuristic

[Jeff King]

On Fri, Dec 23, 2016 at 12:12:13AM -0800, Jacob Keller wrote:

> I actually would prefer that we just say "this is the default now" and
> provide some knob "no-indent-heuristic" or "no-compaction-heuristic"
> and go with that, I think, since I am pretty sure we're all in
> agreement that the heuristic is an improvement in almost every case,
> certainly all the ones we've found. It's at least not worse in any
> case I've seen, and is usually better.
> 
> Thoughts? I don't have a super strong opinion about which name we went
> with for the knob.

Yes, I think we should also make --indent-heuristic the default. That's
technically orthogonal to the name, but I agree the name becomes a lot
less important when it is just on by default.

-Peff

Re: [PATCH] diff: prefer indent heuristic over compaction heuristic

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Dec 23, 2016 at 12:12:13AM -0800, Jacob Keller wrote:
>
>> I actually would prefer that we just say "this is the default now" and
>> provide some knob "no-indent-heuristic" or "no-compaction-heuristic"
>> and go with that, I think, since I am pretty sure we're all in
>> agreement that the heuristic is an improvement in almost every case,
>> certainly all the ones we've found. It's at least not worse in any
>> case I've seen, and is usually better.
>> 
>> Thoughts? I don't have a super strong opinion about which name we went
>> with for the knob.
>
> Yes, I think we should also make --indent-heuristic the default. That's
> technically orthogonal to the name, but I agree the name becomes a lot
> less important when it is just on by default.

Yes, I agree that will be the endgame, but one step at the time, and
also removing one of them is orthogonal that we would want to do
sooner rather than later.  So the step represented by the patch
under discussion is the first one among the two towards the final
state.

I guess both you and Michael are in favor of just removing compaction
variant without any renames, so let me prepare a reroll and queue
that instead.  We can flip the default perhaps one release later.

Thanks.

Re: [PATCH] diff: prefer indent heuristic over compaction heuristic

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I guess both you and Michael are in favor of just removing compaction
> variant without any renames, so let me prepare a reroll and queue
> that instead.  We can flip the default perhaps one release later.

-- >8 --
Subject: [PATCH] diff: retire "compaction" heuristics

When a patch inserts a block of lines, whose last lines are the
same as the existing lines that appear before the inserted block,
"git diff" can choose any place between these existing lines as the
boundary between the pre-context and the added lines (adjusting the
end of the inserted block as appropriate) to come up with variants
of the same patch, and some variants are easier to read than others.

We have been trying to improve the choice of this boundary, and Git
2.11 shipped with an experimental "compaction-heuristic".  Since
then another attempt to improve the logic further resulted in a new
"indent-heuristic" logic.  It is agreed that the latter gives better
result overall, and the former outlived its usefulness.

Retire "compaction", and keep "indent" as an experimental feature.
The latter hopefully will be turned on by default in a future
release, but that should be done as a separate step.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/diff-config.txt            |  6 ++----
 Documentation/diff-heuristic-options.txt |  2 --
 builtin/blame.c                          |  5 ++---
 diff.c                                   | 23 +++-------------------
 git-add--interactive.perl                |  3 ---
 xdiff/xdiff.h                            |  3 +--
 xdiff/xdiffi.c                           | 33 --------------------------------
 7 files changed, 8 insertions(+), 67 deletions(-)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 58f4bd6afa..d8570f2a75 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -172,10 +172,8 @@ diff.tool::
 include::mergetools-diff.txt[]
 
 diff.indentHeuristic::
-diff.compactionHeuristic::
-	Set one of these options to `true` to enable one of two
-	experimental heuristics that shift diff hunk boundaries to
-	make patches easier to read.
+	Set this option to `true` to enable experimental heuristics
+	that shift diff hunk boundaries to make patches easier to read.
 
 diff.algorithm::
 	Choose a diff algorithm.  The variants are as follows:
diff --git a/Documentation/diff-heuristic-options.txt b/Documentation/diff-heuristic-options.txt
index 36cb549df9..d4f3d95505 100644
--- a/Documentation/diff-heuristic-options.txt
+++ b/Documentation/diff-heuristic-options.txt
@@ -1,7 +1,5 @@
 --indent-heuristic::
 --no-indent-heuristic::
---compaction-heuristic::
---no-compaction-heuristic::
 	These are to help debugging and tuning experimental heuristics
 	(which are off by default) that shift diff hunk boundaries to
 	make patches easier to read.
diff --git a/builtin/blame.c b/builtin/blame.c
index 4ddfadb71f..ab54a5c1f4 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2596,8 +2596,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		 * and are only included here to get included in the "-h"
 		 * output:
 		 */
-		{ OPTION_LOWLEVEL_CALLBACK, 0, "indent-heuristic", NULL, NULL, N_("Use an experimental indent-based heuristic to improve diffs"), PARSE_OPT_NOARG, parse_opt_unknown_cb },
-		{ OPTION_LOWLEVEL_CALLBACK, 0, "compaction-heuristic", NULL, NULL, N_("Use an experimental blank-line-based heuristic to improve diffs"), PARSE_OPT_NOARG, parse_opt_unknown_cb },
+		{ OPTION_LOWLEVEL_CALLBACK, 0, "indent-heuristic", NULL, NULL, N_("Use an experimental heuristic to improve diffs"), PARSE_OPT_NOARG, parse_opt_unknown_cb },
 
 		OPT_BIT(0, "minimal", &xdl_opts, N_("Spend extra cycles to find better match"), XDF_NEED_MINIMAL),
 		OPT_STRING('S', NULL, &revs_file, N_("file"), N_("Use revisions from <file> instead of calling git-rev-list")),
@@ -2645,7 +2644,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	}
 parse_done:
 	no_whole_file_rename = !DIFF_OPT_TST(&revs.diffopt, FOLLOW_RENAMES);
-	xdl_opts |= revs.diffopt.xdl_opts & (XDF_COMPACTION_HEURISTIC | XDF_INDENT_HEURISTIC);
+	xdl_opts |= revs.diffopt.xdl_opts & XDF_INDENT_HEURISTIC;
 	DIFF_OPT_CLR(&revs.diffopt, FOLLOW_RENAMES);
 	argc = parse_options_end(&ctx);
 
diff --git a/diff.c b/diff.c
index 8981477c43..741ce8c68d 100644
--- a/diff.c
+++ b/diff.c
@@ -28,7 +28,6 @@
 
 static int diff_detect_rename_default;
 static int diff_indent_heuristic; /* experimental */
-static int diff_compaction_heuristic; /* experimental */
 static int diff_rename_limit_default = 400;
 static int diff_suppress_blank_empty;
 static int diff_use_color_default = -1;
@@ -223,16 +222,8 @@ void init_diff_ui_defaults(void)
 
 int git_diff_heuristic_config(const char *var, const char *value, void *cb)
 {
-	if (!strcmp(var, "diff.indentheuristic")) {
+	if (!strcmp(var, "diff.indentheuristic"))
 		diff_indent_heuristic = git_config_bool(var, value);
-		if (diff_indent_heuristic)
-			diff_compaction_heuristic = 0;
-	}
-	if (!strcmp(var, "diff.compactionheuristic")) {
-		diff_compaction_heuristic = git_config_bool(var, value);
-		if (diff_compaction_heuristic)
-			diff_indent_heuristic = 0;
-	}
 	return 0;
 }
 
@@ -3380,8 +3371,6 @@ void diff_setup(struct diff_options *options)
 	options->xdl_opts |= diff_algorithm;
 	if (diff_indent_heuristic)
 		DIFF_XDL_SET(options, INDENT_HEURISTIC);
-	else if (diff_compaction_heuristic)
-		DIFF_XDL_SET(options, COMPACTION_HEURISTIC);
 
 	options->orderfile = diff_order_file_cfg;
 
@@ -3876,16 +3865,10 @@ int diff_opt_parse(struct diff_options *options,
 		DIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);
 	else if (!strcmp(arg, "--ignore-blank-lines"))
 		DIFF_XDL_SET(options, IGNORE_BLANK_LINES);
-	else if (!strcmp(arg, "--indent-heuristic")) {
+	else if (!strcmp(arg, "--indent-heuristic"))
 		DIFF_XDL_SET(options, INDENT_HEURISTIC);
-		DIFF_XDL_CLR(options, COMPACTION_HEURISTIC);
-	} else if (!strcmp(arg, "--no-indent-heuristic"))
-		DIFF_XDL_CLR(options, INDENT_HEURISTIC);
-	else if (!strcmp(arg, "--compaction-heuristic")) {
-		DIFF_XDL_SET(options, COMPACTION_HEURISTIC);
+	else if (!strcmp(arg, "--no-indent-heuristic"))
 		DIFF_XDL_CLR(options, INDENT_HEURISTIC);
-	} else if (!strcmp(arg, "--no-compaction-heuristic"))
-		DIFF_XDL_CLR(options, COMPACTION_HEURISTIC);
 	else if (!strcmp(arg, "--patience"))
 		options->xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);
 	else if (!strcmp(arg, "--histogram"))
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index ee3d812695..5a55d80b9d 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -46,7 +46,6 @@
 
 my $diff_algorithm = $repo->config('diff.algorithm');
 my $diff_indent_heuristic = $repo->config_bool('diff.indentheuristic');
-my $diff_compaction_heuristic = $repo->config_bool('diff.compactionheuristic');
 my $diff_filter = $repo->config('interactive.difffilter');
 
 my $use_readkey = 0;
@@ -753,8 +752,6 @@ sub parse_diff {
 	}
 	if ($diff_indent_heuristic) {
 		splice @diff_cmd, 1, 0, "--indent-heuristic";
-	} elsif ($diff_compaction_heuristic) {
-		splice @diff_cmd, 1, 0, "--compaction-heuristic";
 	}
 	if (defined $patch_mode_revision) {
 		push @diff_cmd, get_diff_reference($patch_mode_revision);
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index 8db16d4ae6..b090ad8eac 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -41,8 +41,7 @@ extern "C" {
 
 #define XDF_IGNORE_BLANK_LINES (1 << 7)
 
-#define XDF_COMPACTION_HEURISTIC (1 << 8)
-#define XDF_INDENT_HEURISTIC (1 << 9)
+#define XDF_INDENT_HEURISTIC (1 << 8)
 
 #define XDL_EMIT_FUNCNAMES (1 << 0)
 #define XDL_EMIT_FUNCCONTEXT (1 << 2)
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 760fbb6db7..93a65680a1 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -400,11 +400,6 @@ static xdchange_t *xdl_add_change(xdchange_t *xscr, long i1, long i2, long chg1,
 }
 
 
-static int is_blank_line(xrecord_t *rec, long flags)
-{
-	return xdl_blankline(rec->ptr, rec->size, flags);
-}
-
 static int recs_match(xrecord_t *rec1, xrecord_t *rec2, long flags)
 {
 	return (rec1->ha == rec2->ha &&
@@ -821,7 +816,6 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
 	struct xdlgroup g, go;
 	long earliest_end, end_matching_other;
 	long groupsize;
-	unsigned int blank_lines;
 
 	group_init(xdf, &g);
 	group_init(xdfo, &go);
@@ -846,13 +840,6 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
 			 */
 			end_matching_other = -1;
 
-			/*
-			 * Boolean value that records whether there are any blank
-			 * lines that could be made to be the last line of this
-			 * group.
-			 */
-			blank_lines = 0;
-
 			/* Shift the group backward as much as possible: */
 			while (!group_slide_up(xdf, &g, flags))
 				if (group_previous(xdfo, &go))
@@ -869,11 +856,6 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
 
 			/* Now shift the group forward as far as possible: */
 			while (1) {
-				if (!blank_lines)
-					blank_lines = is_blank_line(
-							xdf->recs[g.end - 1],
-							flags);
-
 				if (group_slide_down(xdf, &g, flags))
 					break;
 				if (group_next(xdfo, &go))
@@ -906,21 +888,6 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
 				if (group_previous(xdfo, &go))
 					xdl_bug("group sync broken sliding to match");
 			}
-		} else if ((flags & XDF_COMPACTION_HEURISTIC) && blank_lines) {
-			/*
-			 * Compaction heuristic: if it is possible to shift the
-			 * group to make its bottom line a blank line, do so.
-			 *
-			 * As we already shifted the group forward as far as
-			 * possible in the earlier loop, we only need to handle
-			 * backward shifts, not forward ones.
-			 */
-			while (!is_blank_line(xdf->recs[g.end - 1], flags)) {
-				if (group_slide_up(xdf, &g, flags))
-					xdl_bug("blank line disappeared");
-				if (group_previous(xdfo, &go))
-					xdl_bug("group sync broken sliding to blank line");
-			}
 		} else if (flags & XDF_INDENT_HEURISTIC) {
 			/*
 			 * Indent heuristic: a group of pure add/delete lines
-- 
2.11.0-448-g09546ed716

Re: [PATCH] diff: prefer indent heuristic over compaction heuristic

[Jeff King]

On Fri, Dec 23, 2016 at 01:17:03PM -0800, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > I guess both you and Michael are in favor of just removing compaction
> > variant without any renames, so let me prepare a reroll and queue
> > that instead.  We can flip the default perhaps one release later.
> 
> -- >8 --
> Subject: [PATCH] diff: retire "compaction" heuristics

Looks good to me from a cursory read.

Thanks.

-Peff

Re: [PATCH] diff: prefer indent heuristic over compaction heuristic

[Jacob Keller]

On Fri, Dec 23, 2016 at 2:21 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Dec 23, 2016 at 01:17:03PM -0800, Junio C Hamano wrote:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> > I guess both you and Michael are in favor of just removing compaction
>> > variant without any renames, so let me prepare a reroll and queue
>> > that instead.  We can flip the default perhaps one release later.
>>
>> -- >8 --
>> Subject: [PATCH] diff: retire "compaction" heuristics
>
> Looks good to me from a cursory read.
>
> Thanks.
>
> -Peff

Same. This is more obviously correct since we didn't have to change a
bunch of references to INDENT_HEURISTIC. I agree that the name does
not make sense now, but if our goal is to make it default with a
disable option, I think that we shouldn't worry too much about the
naming.

Thanks,
Jake

Re: [PATCH] diff: prefer indent heuristic over compaction heuristic

[Michael Haggerty]

On 12/23/2016 10:17 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> I guess both you and Michael are in favor of just removing compaction
>> variant without any renames, so let me prepare a reroll and queue
>> that instead.  We can flip the default perhaps one release later.
> 
> -- >8 --
> Subject: [PATCH] diff: retire "compaction" heuristics
> 
> When a patch inserts a block of lines, whose last lines are the
> same as the existing lines that appear before the inserted block,
> "git diff" can choose any place between these existing lines as the
> boundary between the pre-context and the added lines (adjusting the
> end of the inserted block as appropriate) to come up with variants
> of the same patch, and some variants are easier to read than others.
> 
> We have been trying to improve the choice of this boundary, and Git
> 2.11 shipped with an experimental "compaction-heuristic".  Since
> then another attempt to improve the logic further resulted in a new
> "indent-heuristic" logic.  It is agreed that the latter gives better
> result overall, and the former outlived its usefulness.
> 
> Retire "compaction", and keep "indent" as an experimental feature.
> The latter hopefully will be turned on by default in a future
> release, but that should be done as a separate step.

The whole patch looks good to me. Thanks for taking care of this.

Michael

Re: [PATCH] diff: prefer indent heuristic over compaction heuristic

[Stefan Beller]

picking up this old topic,
with Martin, Marc and SZEDER cc'd,
as we got patch proposals regarding the indent heuristic.

On Sat, Dec 24, 2016 at 4:55 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 12/23/2016 10:17 PM, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> I guess both you and Michael are in favor of just removing compaction
>>> variant without any renames, so let me prepare a reroll and queue
>>> that instead.  We can flip the default perhaps one release later.
>>
>> -- >8 --
>> Subject: [PATCH] diff: retire "compaction" heuristics
>>
>> When a patch inserts a block of lines, whose last lines are the
>> same as the existing lines that appear before the inserted block,
>> "git diff" can choose any place between these existing lines as the
>> boundary between the pre-context and the added lines (adjusting the
>> end of the inserted block as appropriate) to come up with variants
>> of the same patch, and some variants are easier to read than others.
>>
>> We have been trying to improve the choice of this boundary, and Git
>> 2.11 shipped with an experimental "compaction-heuristic".  Since
>> then another attempt to improve the logic further resulted in a new
>> "indent-heuristic" logic.  It is agreed that the latter gives better
>> result overall, and the former outlived its usefulness.
>>
>> Retire "compaction", and keep "indent" as an experimental feature.
>> The latter hopefully will be turned on by default in a future
>> release, but that should be done as a separate step.

Maybe turning on this feature by default is the next step instead of
adding them to bash competition or making them available in plumbing
commands for the upcoming release.

Then in a later release we could even remove the knobs to turn it off.

Thanks,
Stefan

Re: [PATCH] diff: prefer indent heuristic over compaction heuristic

[Jeff King]

On Thu, Apr 27, 2017 at 02:17:55PM -0700, Stefan Beller wrote:

> >> Retire "compaction", and keep "indent" as an experimental feature.
> >> The latter hopefully will be turned on by default in a future
> >> release, but that should be done as a separate step.
> 
> Maybe turning on this feature by default is the next step instead of
> adding them to bash competition or making them available in plumbing
> commands for the upcoming release.
> 
> Then in a later release we could even remove the knobs to turn it off.

Yeah, if we are going to proceed with making it the default, I'd rather
do that than worry about adding frills that will eventually not matter.

We could take Marc's patch to move the config to git_diff_basic_config()
as an interim step, though. It gives people an escape hatch if they find
that they need to disable the feature for some plumbing command.

In that discussion, the only argument I could come up with against
making it the default (or respecting it for plumbing) is that it may
have an impact on patch-ids. I'm not sure I'm swayed by that, but I
wanted to mention it to make sure we're deciding consciously not to care
about it.

-Peff

Re: [PATCH] diff: prefer indent heuristic over compaction heuristic

[Martin Liška]

On 04/27/2017 11:17 PM, Stefan Beller wrote:
> Maybe turning on this feature by default is the next step instead of
> adding them to bash competition or making them available in plumbing
> commands for the upcoming release.

Hello.

Works for me, please ignore the patch I've sent ;)

Anyhow, nice option (functionality), I really like it.
Martin