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

Jacob Keller [off-list ref] writes:

On Fri, Apr 29, 2016 at 1:29 PM, Junio C Hamano [off-list ref] wrote:

quoted

Jeff King [off-list ref] writes:

quoted

... 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 <redacted>
---
 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)) {

On Fri, Apr 29, 2016 at 3:18 PM, Junio C Hamano [off-list ref] wrote:

Jacob Keller [off-list ref] writes:

quoted

On Fri, Apr 29, 2016 at 1:29 PM, Junio C Hamano [off-list ref] wrote:

quoted

Jeff King [off-list ref] writes:

quoted

... 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.

I would remove the opt-out knob much later in the game, i.e.

    1) make a patch that removes the documentation only
       before the next release (i.e. before 2.9)
    2) make a patch to remove the actual (unlabeled) knobs,
        merge into master before 2.10 (i.e. just after the 2.9 release)

Then we get the most of the community to test it with the 2.9 release
and still have an emergency knob in case some major headaches
show up. After one release cycle we'll be much more confident
about its usage and its short comings and do not need the
emergency turn off. If the community doesn't like it for some reason
we can document it and debate the default setting?

I agree we want the knob gone eventually.
Making it an undocumented feature is as good as that from
a users point of view?

On Fri, 2016-04-29 at 15:35 -0700, Stefan Beller wrote:

On Fri, Apr 29, 2016 at 3:18 PM, Junio C Hamano [off-list ref]
wrote:

quoted

Jacob Keller [off-list ref] writes:

quoted

On Fri, Apr 29, 2016 at 1:29 PM, Junio C Hamano <gitster@pobox.co
m> wrote:

quoted

Jeff King [off-list ref] writes:

quoted

... 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.

I would remove the opt-out knob much later in the game, i.e.

    1) make a patch that removes the documentation only
       before the next release (i.e. before 2.9)
    2) make a patch to remove the actual (unlabeled) knobs,
        merge into master before 2.10 (i.e. just after the 2.9
release)

Then we get the most of the community to test it with the 2.9 release
and still have an emergency knob in case some major headaches
show up. After one release cycle we'll be much more confident
about its usage and its short comings and do not need the
emergency turn off. If the community doesn't like it for some reason
we can document it and debate the default setting?

I agree we want the knob gone eventually.
Making it an undocumented feature is as good as that from
a users point of view?

Currently it's an "opt in" knob, so this doesn't make sense to me. If
we remove the entire knob as is, we can always (fairly easily) add it
back. I would keep the code inside xdiff as a knob, but set it to
enable default so that the user config has no knob at the top level but
the xdiff machinery does (this making a "disable" be relatively small
patch).

Thanks,
Jake

Currently it's an "opt in" knob, so this doesn't make sense to me.

+static int diff_compaction_heuristic = 1;

It's rather an opt-out knob going by the current
origin/jk/diff-compact-heuristic

If
we remove the entire knob as is, we can always (fairly easily) add it
back. I would keep the code inside xdiff as a knob, but set it to
enable default so that the user config has no knob at the top level but
the xdiff machinery does (this making a "disable" be relatively small
patch).

When writing my reply, I thought about people using Git from a binary
distribution with little to no admin rights. They want to have an emergency
knob to disable this thing, but cannot patch/recompile Git.

If you can patch and compile your version of Git, then reverting is easy, so
in that case Junios patch looks good to me.

Thanks,
Stefan

Thanks,
Jake

On Fri, 2016-04-29 at 15:44 -0700, Stefan Beller wrote:

quoted

Currently it's an "opt in" knob, so this doesn't make sense to me.

+static int diff_compaction_heuristic = 1;

Oops didn't know we'd made it default at some point. (all my versions
had it disabled by default)

It's rather an opt-out knob going by the current
origin/jk/diff-compact-heuristic

Yea in that case, we could keep it.

quoted

If
we remove the entire knob as is, we can always (fairly easily) add
it
back. I would keep the code inside xdiff as a knob, but set it to
enable default so that the user config has no knob at the top level
but
the xdiff machinery does (this making a "disable" be relatively
small
patch).

When writing my reply, I thought about people using Git from a binary
distribution with little to no admin rights. They want to have an
emergency
knob to disable this thing, but cannot patch/recompile Git.

If you can patch and compile your version of Git, then reverting is
easy, so
in that case Junios patch looks good to me.

Thanks,
Stefan

True. I think the chances that it needs such a thing are quite minor,
and if an undocumented knob gets exposed it would have to become
documented and maintained, so I'd prefer to avoid it. Given that the
risk is pretty small I think that's ok.

Thanks,
Jake

On Fri, Apr 29, 2016 at 03:35:54PM -0700, Stefan Beller wrote:

quoted

-- >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.

I would remove the opt-out knob much later in the game, i.e.

    1) make a patch that removes the documentation only
       before the next release (i.e. before 2.9)
    2) make a patch to remove the actual (unlabeled) knobs,
        merge into master before 2.10 (i.e. just after the 2.9 release)

Yeah, I think it might be a good idea to keep some sort of undocumented
safety valve in the release, at least for a cycle or two. The heuristic
won't _really_ see wide use until it is in a released version of git, as
much as we would like it to be otherwise.

So I am anticipating a possible conversation where somebody reports that
the new output looks bad, and it would be nice to say "try it with this
flag (or environment variable, or whatever) and see if that looks
better".  And then based on that conversation we can decide what the
right next is (a real user-visible flag, or reversion, or deciding the
user's case isn't worth it). Or maybe if we're lucky that conversation
never happens.

The "whatever" in the instructions can obviously be "build with this
patch" or "try with an older version of git", but we're a lot more
likely to get a good response if it's easy for the user to do.

-Peff