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>

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 58f4bd6afa12240f9e416860ac12d82776815ee9..d8570f2a75096c07d7045ddfba7f1e2e6396d000 100644 (file)
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -172,10 +172,8 @@ diff.tool::
 include::mergetools-diff.txt[]
 
 diff.indentHeuristic::
 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.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 36cb549df97cef182d590683cf262ccc9690e3ff..d4f3d9550555cd0aaa7a35c27ee926b71bf01b41 100644 (file)
--- a/Documentation/diff-heuristic-options.txt
+++ b/Documentation/diff-heuristic-options.txt
@@ -1,7 +1,5 @@
 --indent-heuristic::
 --no-indent-heuristic::
 --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.
        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 4ddfadb71f7ef93958e2f236e166212e8b6bda2b..ab54a5c1f48ffcb87193eafb69d45a0f8d378c5e 100644 (file)
--- 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:
                 */
                 * 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")),
 
                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);
        }
 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_OPT_CLR(&revs.diffopt, FOLLOW_RENAMES);
        argc = parse_options_end(&ctx);
 

diff --git a/diff.c b/diff.c
index 8981477c436dd11d97714f0ad7e1b9363e328798..741ce8c68dc7f4e6e860db03391055b4ac589cc3 100644 (file)
--- 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_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;
 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)
 {
 
 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);
                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;
 }
 
        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);
        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;
 
 
        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);
                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_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);
                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"))
        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 ee3d812695fa232f30682b0e546105ca0eea5d49..5a55d80b9dd848d41daf84771e3b89d55a60ec23 100755 (executable)
--- 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_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;
 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";
        }
        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);
        }
        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 8db16d4ae6def5657bc51fa2b9b354a7c0267201..b090ad8eacfe6ed171aa14b39901a8042ab678d9 100644 (file)
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -41,8 +41,7 @@ extern "C" {
 
 #define XDF_IGNORE_BLANK_LINES (1 << 7)
 
 
 #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)
 
 #define XDL_EMIT_FUNCNAMES (1 << 0)
 #define XDL_EMIT_FUNCCONTEXT (1 << 2)

diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 760fbb6db7583f6d0ec5eed9b9c63ea6a0e7abe7..93a65680a18284041ce57f61191d20dcc763e022 100644 (file)
--- 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 &&
 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;
        struct xdlgroup g, go;
        long earliest_end, end_matching_other;
        long groupsize;

-       unsigned int blank_lines;

 
        group_init(xdf, &g);
        group_init(xdfo, &go);
 
        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;
 
                         */
                        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))
                        /* 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) {
 
                        /* 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))
                                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");
                        }
                                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
                } else if (flags & XDF_INDENT_HEURISTIC) {
                        /*
                         * Indent heuristic: a group of pure add/delete lines