Re: [PATCH 2/2] xdiff: implement empty line chunk heuristic

[Junio C Hamano <gitster@pobox.com>]

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

> On Fri, Apr 29, 2016 at 1:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jeff King <peff@peff.net> writes:
>>
>>> ... Having the two directly next to each other reads
>>> better to me. This is a pretty unusual diff, though, in that it did
>>> change the surrounding whitespace (and if you look further in the diff,
>>> the identical change is made elsewhere _without_ touching the
>>> whitespace). So this is kind of an anomaly. And IMHO the weirdness here
>>> is outweighed by the vast number of improvements elsewhere.
>>
>> So... is everybody happy with the result and now we can drop the
>> tweaking knob added to help experimentation before merging the
>> result to 'master'?
>>
>> I am pretty happy with the end result myself.
>
> I am very happy with it. I haven't had any issues, and I think we'll
> find better traction by enabling it at this point and seeing when/if
> someone complains.
>
> I think for most it won't be noticed and for those that do it will
> likely be positive.

I am doing this only to prepare in case we have a concensus,
i.e. this is not to declare that I do not care what other people
say.  Here is a patch to remove the experimentation knob.

Let's say we keep this patch out of tree for now and keep the topic
in 'next' so that people can further play with it for several more
weeks, and then apply this on top and merge the result to 'master'
early in the next cycle.

-- >8 --
diff: enable "compaction heuristics" and lose experimentation knob

It seems that the new "find a good hunk boundary by locating a blank
line" heuristics gives much more pleasant result without much
noticeable downsides.  Let's make it the new algorithm for real,
without the opt-out knob we added while experimenting with it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/diff-config.txt  |  5 -----
 Documentation/diff-options.txt |  6 ------
 diff.c                         | 11 -----------
 xdiff/xdiff.h                  |  2 --
 xdiff/xdiffi.c                 |  2 +-
 5 files changed, 1 insertion(+), 25 deletions(-)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 9bf3e92..6eaa452 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -166,11 +166,6 @@ diff.tool::
 
 include::mergetools-diff.txt[]
 
-diff.compactionHeuristic::
-	Set this option to enable an experimental heuristic that
-	shifts the hunk boundary in an attempt to make the resulting
-	patch easier to read.
-
 diff.algorithm::
 	Choose a diff algorithm.  The variants are as follows:
 +
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index b513023..3ad6404 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -63,12 +63,6 @@ ifndef::git-format-patch[]
 	Synonym for `-p --raw`.
 endif::git-format-patch[]
 
---compaction-heuristic::
---no-compaction-heuristic::
-	These are to help debugging and tuning an experimental
-	heuristic that shifts the hunk boundary in an attempt to
-	make the resulting patch easier to read.
-
 --minimal::
 	Spend extra time to make sure the smallest possible
 	diff is produced.
diff --git a/diff.c b/diff.c
index 05ca3ce..f62b7f7 100644
--- a/diff.c
+++ b/diff.c
@@ -25,7 +25,6 @@
 #endif
 
 static int diff_detect_rename_default;
-static int diff_compaction_heuristic = 1;
 static int diff_rename_limit_default = 400;
 static int diff_suppress_blank_empty;
 static int diff_use_color_default = -1;
@@ -184,10 +183,6 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
 		diff_detect_rename_default = git_config_rename(var, value);
 		return 0;
 	}
-	if (!strcmp(var, "diff.compactionheuristic")) {
-		diff_compaction_heuristic = git_config_bool(var, value);
-		return 0;
-	}
 	if (!strcmp(var, "diff.autorefreshindex")) {
 		diff_auto_refresh_index = git_config_bool(var, value);
 		return 0;
@@ -3240,8 +3235,6 @@ 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_compaction_heuristic)
-		DIFF_XDL_SET(options, COMPACTION_HEURISTIC);
 
 	options->orderfile = diff_order_file_cfg;
 
@@ -3719,10 +3712,6 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		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, "--compaction-heuristic"))
-		DIFF_XDL_SET(options, 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);
 	else if (!strcmp(arg, "--histogram"))
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index d1dbb27..c033991 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -41,8 +41,6 @@ extern "C" {
 
 #define XDF_IGNORE_BLANK_LINES (1 << 7)
 
-#define XDF_COMPACTION_HEURISTIC (1 << 8)
-
 #define XDL_EMIT_FUNCNAMES (1 << 0)
 #define XDL_EMIT_COMMON (1 << 1)
 #define XDL_EMIT_FUNCCONTEXT (1 << 2)
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index b3c6848..574f83c 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -522,7 +522,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
 		 * As we already shifted the group forward as far as possible
 		 * in the earlier loop, we need to shift it back only if at all.
 		 */
-		if ((flags & XDF_COMPACTION_HEURISTIC) && blank_lines) {
+		if (blank_lines) {
 			while (ixs > 0 &&
 			       !is_blank_line(recs, ix - 1, flags) &&
 			       recs_match(recs, ixs - 1, ix - 1, flags)) {