Re: [PATCH 2/2] xdiff: implement empty line chunk heuristic
From: Stefan Beller <hidden>
Date: 2016-06-16 02:19:05
Possibly related (same subject, not in this thread)
- 2016-06-16 · Re: [PATCH 2/2] xdiff: implement empty line chunk heuristic · Jacob Keller <hidden>
- 2016-06-16 · Re: [PATCH 2/2] xdiff: implement empty line chunk heuristic · Junio C Hamano <hidden>
- 2016-06-16 · Re: [PATCH 2/2] xdiff: implement empty line chunk heuristic · Stefan Beller <hidden>
- 2016-06-16 · Re: [PATCH 2/2] xdiff: implement empty line chunk heuristic · Jeff King <hidden>
- 2016-06-16 · Re: [PATCH 2/2] xdiff: implement empty line chunk heuristic · Junio C Hamano <hidden>
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?
quoted hunk ↗ jump to hunk
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)) {