{"thread":{"id":"42037","subject":"[RFC PATCH, WAS: \"weird diff output?\" 0/2] implement better chunk heuristics","startedAt":"2016-04-15T16:51:39Z","lastAt":"2016-04-15T21:56:30Z","messageCount":22,"participants":["Jacob Keller","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"283537","messageId":"20160415165141.4712-1-jacob.e.keller@intel.com","threadId":"42037","inReplyTo":null,"subject":"[RFC PATCH, WAS: \"weird diff output?\" 0/2] implement better chunk heuristics","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2016-04-15T16:51:39Z","receivedAt":"2016-04-15T16:51:39Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nI took up Stefan's patch, and modified it to resolve a couple issues. I\nalso tried to implement the suggestions from Junio's review, as well as\nthe suggestions I had. It appears to produce equivalent output as Jeff's\nscript. This version is still missing a few things, and it is possible\nStefan is working on a v2 of his own, but I thought I'd submit this.\n\nThere's still several TODOs:\n* Add some tests\n* better name for the heuristic(?)\n* better patch description plus documentation\n* is_emptyline should be more generic and handle CRLF\n\nChanges since Stefan's v1:\n* Added a patch to implement xdl_hash_and_recmatch as Junio suggested.\n* Fixed a segfault in Stefan's patch\n* Added XDL flag to configure the behavior\n* Used an int and counted empty lines via += instead of |=\n* Renamed starts_with_emptyline to is_emptyline\n* Added diff command line and config options\n\nFor reviewer convenience, the interdiff between this and Stefan's version:\n\ndiff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\nindex edba56522bce..cebf82702d2a 100644\n--- a/Documentation/diff-config.txt\n+++ b/Documentation/diff-config.txt\n@@ -170,6 +170,12 @@ diff.tool::\n \n include::mergetools-diff.txt[]\n \n+diff.emptyLineHeuristic::\n+\tSet this option to true to enable the empty line chunk heuristic when\n+\tproducing diff output. This heuristic will attempt to shift hunks such\n+\tthat a common empty line occurs below the hunk with the rest of the\n+\tcontext above it.\n+\n diff.algorithm::\n \tChoose a diff algorithm. The variants are as follows:\n +\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 4b0318e2ac15..6c1cd8b35584 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -63,6 +63,12 @@ ifndef::git-format-patch[]\n \tSynonym for `-p --raw`.\n endif::git-format-patch[]\n \n+--empty-line-heuristic::\n+--no-empty-line-heuristic::\n+\tWhen possible, shift common empty lines in diff hunks below the hunk\n+\tsuch that the last common empty line for each hunk is below, with the\n+\trest of the context above the hunk.\n+\n --minimal::\n \tSpend extra time to make sure the smallest possible\n \tdiff is produced.\ndiff --git a/diff.c b/diff.c\nindex 4dfe6609d059..8ab9a492928d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -26,6 +26,7 @@\n #endif\n \n static int diff_detect_rename_default;\n+static int diff_empty_line_heuristic = 0;\n static int diff_rename_limit_default = 400;\n static int diff_suppress_blank_empty;\n static int diff_use_color_default = -1;\n@@ -189,6 +190,10 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \t\tdiff_detect_rename_default = git_config_rename(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"diff.emptylineheuristic\")) {\n+\t\tdiff_empty_line_heuristic = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n \tif (!strcmp(var, \"diff.autorefreshindex\")) {\n \t\tdiff_auto_refresh_index = git_config_bool(var, value);\n \t\treturn 0;\n@@ -3278,6 +3283,8 @@ void diff_setup(struct diff_options *options)\n \toptions->use_color = diff_use_color_default;\n \toptions->detect_rename = diff_detect_rename_default;\n \toptions->xdl_opts |= diff_algorithm;\n+\tif (diff_empty_line_heuristic)\n+\t\tDIFF_XDL_SET(options, EMPTY_LINE_HEURISTIC);\n \n \toptions->orderfile = diff_order_file_cfg;\n \n@@ -3798,6 +3805,10 @@ int diff_opt_parse(struct diff_options *options,\n \t\tDIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);\n \telse if (!strcmp(arg, \"--ignore-blank-lines\"))\n \t\tDIFF_XDL_SET(options, IGNORE_BLANK_LINES);\n+\telse if (!strcmp(arg, \"--empty-line-heuristic\"))\n+\t\tDIFF_XDL_SET(options, EMPTY_LINE_HEURISTIC);\n+\telse if (!strcmp(arg, \"--no-empty-line-heuristic\"))\n+\t\tDIFF_XDL_CLR(options, EMPTY_LINE_HEURISTIC);\n \telse if (!strcmp(arg, \"--patience\"))\n \t\toptions->xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);\n \telse if (!strcmp(arg, \"--histogram\"))\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex 4fb7e79410c2..9195a5c0e958 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -41,6 +41,8 @@ extern \"C\" {\n \n #define XDF_IGNORE_BLANK_LINES (1 << 7)\n \n+#define XDF_EMPTY_LINE_HEURISTIC (1 << 8)\n+\n #define XDL_EMIT_FUNCNAMES (1 << 0)\n #define XDL_EMIT_FUNCCONTEXT (1 << 2)\n \ndiff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\nindex 5f14beb98049..83984b90f82f 100644\n--- a/xdiff/xdiffi.c\n+++ b/xdiff/xdiffi.c\n@@ -400,7 +400,7 @@ static xdchange_t *xdl_add_change(xdchange_t *xscr, long i1, long i2, long chg1,\n }\n \n \n-static int starts_with_emptyline(const char *recs)\n+static int is_emptyline(const char *recs)\n {\n \treturn recs[0] == '\\n'; /* CRLF not covered here */\n }\n@@ -416,7 +416,7 @@ static int xdl_hash_and_recmatch(xrecord_t **recs, long ixs, long ix, long flags\n int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n \tlong ix, ixo, ixs, ixref, grpsiz, nrec = xdf->nrec;\n \tchar *rchg = xdf->rchg, *rchgo = xdfo->rchg;\n-\tunsigned char has_emptyline;\n+\tunsigned int emptylines;\n \txrecord_t **recs = xdf->recs;\n \n \t/*\n@@ -450,7 +450,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n \n \t\tdo {\n \t\t\tgrpsiz = ix - ixs;\n-\t\t\thas_emptyline = 0;\n+\t\t\temptylines = 0;\n \n \t\t\t/*\n \t\t\t * If the line before the current change group, is equal to\n@@ -458,11 +458,11 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n \t\t\t * the group.\n \t\t\t */\n \t\t\twhile (ixs > 0 && xdl_hash_and_recmatch(recs, ixs - 1, ix - 1, flags)) {\n+\t\t\t\temptylines += is_emptyline(recs[ix - 1]->ptr);\n+\n \t\t\t\trchg[--ixs] = 1;\n \t\t\t\trchg[--ix] = 0;\n \n-\t\t\t\thas_emptyline |=\n-\t\t\t\t\tstarts_with_emptyline(recs[ix]->ptr);\n \t\t\t\t/*\n \t\t\t\t * This change might have joined two change groups,\n \t\t\t\t * so we try to take this scenario in account by moving\n@@ -492,9 +492,6 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n \t\t\t\trchg[ixs++] = 0;\n \t\t\t\trchg[ix++] = 1;\n \n-\t\t\t\thas_emptyline |=\n-\t\t\t\t\tstarts_with_emptyline(recs[ix]->ptr);\n-\n \t\t\t\t/*\n \t\t\t\t * This change might have joined two change groups,\n \t\t\t\t * so we try to take this scenario in account by moving\n@@ -527,21 +524,12 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n \t\t * As we shifted the group forward as far as possible, we only\n \t\t * need to shift it back if at all.\n \t\t */\n-\t\tif (has_emptyline) {\n-\t\t\twhile (ixs > 0 && recs[ixs - 1]->ha == recs[ix - 1]->ha &&\n-\t\t\t xdl_recmatch(recs[ixs - 1]->ptr, recs[ixs - 1]->size, recs[ix - 1]->ptr, recs[ix - 1]->size, flags) &&\n-\t\t\t !starts_with_emptyline(recs[ix - 1]->ptr)) {\n+\t\tif ((flags & XDF_EMPTY_LINE_HEURISTIC) && emptylines) {\n+\t\t\twhile (ixs > 0 &&\n+\t\t\t !is_emptyline(recs[ix - 1]->ptr) &&\n+\t\t\t xdl_hash_and_recmatch(recs, ixs - 1, ix - 1, flags)) {\n \t\t\t\trchg[--ixs] = 1;\n \t\t\t\trchg[--ix] = 0;\n-\n-\t\t\t\t/*\n-\t\t\t\t * This change did not join two change groups,\n-\t\t\t\t * as we did that before already, so there is no\n-\t\t\t\t * need to adapt the other-file, i.e.\n-\t\t\t\t * running\n-\t\t\t\t * for (; rchg[ixs - 1]; ixs--);\n-\t\t\t\t * while (rchgo[--ixo]);\n-\t\t\t\t */\n \t\t\t}\n \t\t}\n \t}\n\nJacob Keller (1):\n xdiff: add xdl_hash_and_recmatch helper function\n\nStefan Beller (1):\n xdiff: implement empty line chunk heuristic\n\n Documentation/diff-config.txt | 6 ++++++\n Documentation/diff-options.txt | 6 ++++++\n diff.c | 11 +++++++++++\n xdiff/xdiff.h | 2 ++\n xdiff/xdiffi.c | 42 ++++++++++++++++++++++++++++++++++++++----\n 5 files changed, 63 insertions(+), 4 deletions(-)\n\n-- \n2.8.1.369.geae769a\n"},{"id":"283538","messageId":"20160415165141.4712-2-jacob.e.keller@intel.com","threadId":"42037","inReplyTo":"20160415165141.4712-1-jacob.e.keller@intel.com","subject":"[RFC PATCH, WAS: \"weird diff output?\" 1/2] xdiff: add xdl_hash_and_recmatch helper function","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2016-04-15T16:51:40Z","receivedAt":"2016-04-15T16:51:40Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nIt is a common pattern in xdl_change_compact to check that hashes and\nstrings match. The resulting code to perform this change causes very\nlong lines and makes it hard to follow the intention. Introduce a helper\nfunction xdl_hash_and_recmatch which performs both checks to increase\ncode readability.\n\nSigned-off-by: Jacob Keller \n---\n xdiff/xdiffi.c | 16 ++++++++++++----\n 1 file changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\nindex 2358a2d6326e..fff97ac3e3c7 100644\n--- a/xdiff/xdiffi.c\n+++ b/xdiff/xdiffi.c\n@@ -400,6 +400,14 @@ static xdchange_t *xdl_add_change(xdchange_t *xscr, long i1, long i2, long chg1,\n }\n \n \n+static int xdl_hash_and_recmatch(xrecord_t **recs, long ixs, long ix, long flags)\n+{\n+\treturn (recs[ixs]->ha == recs[ix]->ha &&\n+\t\txdl_recmatch(recs[ixs]->ptr, recs[ixs]->size,\n+\t\t\t recs[ix]->ptr, recs[ix]->size,\n+\t\t\t flags));\n+}\n+\n int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n \tlong ix, ixo, ixs, ixref, grpsiz, nrec = xdf->nrec;\n \tchar *rchg = xdf->rchg, *rchgo = xdfo->rchg;\n@@ -442,8 +450,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n \t\t\t * the last line of the current change group, shift backward\n \t\t\t * the group.\n \t\t\t */\n-\t\t\twhile (ixs > 0 && recs[ixs - 1]->ha == recs[ix - 1]->ha &&\n-\t\t\t xdl_recmatch(recs[ixs - 1]->ptr, recs[ixs - 1]->size, recs[ix - 1]->ptr, recs[ix - 1]->size, flags)) {\n+\t\t\twhile (ixs > 0 && xdl_hash_and_recmatch(recs, ixs - 1, ix - 1, flags)) {\n \t\t\t\trchg[--ixs] = 1;\n \t\t\t\trchg[--ix] = 0;\n \n@@ -470,8 +477,9 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n \t\t\t * the line next of the current change group, shift forward\n \t\t\t * the group.\n \t\t\t */\n-\t\t\twhile (ix < nrec && recs[ixs]->ha == recs[ix]->ha &&\n-\t\t\t xdl_recmatch(recs[ixs]->ptr, recs[ixs]->size, recs[ix]->ptr, recs[ix]->size, flags)) {\n+\t\t\twhile (ix < nrec && xdl_hash_and_recmatch(recs, ixs, ix, flags)) {\n+\t\t\t\temptylines += is_emptyline(recs[ix]->ptr);\n+\n \t\t\t\trchg[ixs++] = 0;\n \t\t\t\trchg[ix++] = 1;\n \n-- \n2.8.1.369.geae769a\n"},{"id":"283539","messageId":"20160415165141.4712-3-jacob.e.keller@intel.com","threadId":"42037","inReplyTo":"20160415165141.4712-1-jacob.e.keller@intel.com","subject":"[RFC PATCH, WAS: \"weird diff output?\" 2/2] xdiff: implement empty line chunk heuristic","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2016-04-15T16:51:41Z","receivedAt":"2016-04-15T16:51:41Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Stefan Beller \n\nIn order to produce the smallest possible diff and combine several diff\nhunks together, we implement a heuristic from GNU Diff which moves diff\nhunks forward as far as possible when we find common context above and\nbelow a diff hunk. This sometimes produces less readable diffs when\nwriting C, Shell, or other programming languages, ie:\n\n...\n /*\n+ *\n+ *\n+ */\n+\n+/*\n...\n\ninstead of the more readable equivalent of\n\n...\n+/*\n+ *\n+ *\n+ */\n+\n /*\n...\n\nImplement the following heuristic to (optionally) produce the desired\noutput.\n\n If there are diff chunks which can be shifted around, shift each hunk\n such that the last common empty line is below the chunk with the rest\n of the context above.\n\nThis heuristic appears to resolve the above example and several other\ncommon issues without producing significantly weird results. However, as\nwith any heuristic it is not really known whether this will always be\nmore optimal. Thus, leave the heuristic disabled by default.\n\nAdd an XDIFF flag to enable this heuristic only conditionally. Add\na diff command line option and diff configuration option to allow users\nto enable this option when desired.\n\nTODO:\n* Add tests\n* Add better/more documentation explaining the heuristic, possibly with\n examples(?)\n* better name(?)\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Jacob Keller \n---\n Documentation/diff-config.txt | 6 ++++++\n Documentation/diff-options.txt | 6 ++++++\n diff.c | 11 +++++++++++\n xdiff/xdiff.h | 2 ++\n xdiff/xdiffi.c | 26 ++++++++++++++++++++++++++\n 5 files changed, 51 insertions(+)\n\ndiff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\nindex edba56522bce..cebf82702d2a 100644\n--- a/Documentation/diff-config.txt\n+++ b/Documentation/diff-config.txt\n@@ -170,6 +170,12 @@ diff.tool::\n \n include::mergetools-diff.txt[]\n \n+diff.emptyLineHeuristic::\n+\tSet this option to true to enable the empty line chunk heuristic when\n+\tproducing diff output. This heuristic will attempt to shift hunks such\n+\tthat a common empty line occurs below the hunk with the rest of the\n+\tcontext above it.\n+\n diff.algorithm::\n \tChoose a diff algorithm. The variants are as follows:\n +\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 4b0318e2ac15..6c1cd8b35584 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -63,6 +63,12 @@ ifndef::git-format-patch[]\n \tSynonym for `-p --raw`.\n endif::git-format-patch[]\n \n+--empty-line-heuristic::\n+--no-empty-line-heuristic::\n+\tWhen possible, shift common empty lines in diff hunks below the hunk\n+\tsuch that the last common empty line for each hunk is below, with the\n+\trest of the context above the hunk.\n+\n --minimal::\n \tSpend extra time to make sure the smallest possible\n \tdiff is produced.\ndiff --git a/diff.c b/diff.c\nindex 4dfe6609d059..8ab9a492928d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -26,6 +26,7 @@\n #endif\n \n static int diff_detect_rename_default;\n+static int diff_empty_line_heuristic = 0;\n static int diff_rename_limit_default = 400;\n static int diff_suppress_blank_empty;\n static int diff_use_color_default = -1;\n@@ -189,6 +190,10 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \t\tdiff_detect_rename_default = git_config_rename(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"diff.emptylineheuristic\")) {\n+\t\tdiff_empty_line_heuristic = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n \tif (!strcmp(var, \"diff.autorefreshindex\")) {\n \t\tdiff_auto_refresh_index = git_config_bool(var, value);\n \t\treturn 0;\n@@ -3278,6 +3283,8 @@ void diff_setup(struct diff_options *options)\n \toptions->use_color = diff_use_color_default;\n \toptions->detect_rename = diff_detect_rename_default;\n \toptions->xdl_opts |= diff_algorithm;\n+\tif (diff_empty_line_heuristic)\n+\t\tDIFF_XDL_SET(options, EMPTY_LINE_HEURISTIC);\n \n \toptions->orderfile = diff_order_file_cfg;\n \n@@ -3798,6 +3805,10 @@ int diff_opt_parse(struct diff_options *options,\n \t\tDIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);\n \telse if (!strcmp(arg, \"--ignore-blank-lines\"))\n \t\tDIFF_XDL_SET(options, IGNORE_BLANK_LINES);\n+\telse if (!strcmp(arg, \"--empty-line-heuristic\"))\n+\t\tDIFF_XDL_SET(options, EMPTY_LINE_HEURISTIC);\n+\telse if (!strcmp(arg, \"--no-empty-line-heuristic\"))\n+\t\tDIFF_XDL_CLR(options, EMPTY_LINE_HEURISTIC);\n \telse if (!strcmp(arg, \"--patience\"))\n \t\toptions->xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);\n \telse if (!strcmp(arg, \"--histogram\"))\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex 4fb7e79410c2..9195a5c0e958 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -41,6 +41,8 @@ extern \"C\" {\n \n #define XDF_IGNORE_BLANK_LINES (1 << 7)\n \n+#define XDF_EMPTY_LINE_HEURISTIC (1 << 8)\n+\n #define XDL_EMIT_FUNCNAMES (1 << 0)\n #define XDL_EMIT_FUNCCONTEXT (1 << 2)\n \ndiff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\nindex fff97ac3e3c7..83984b90f82f 100644\n--- a/xdiff/xdiffi.c\n+++ b/xdiff/xdiffi.c\n@@ -400,6 +400,11 @@ static xdchange_t *xdl_add_change(xdchange_t *xscr, long i1, long i2, long chg1,\n }\n \n \n+static int is_emptyline(const char *recs)\n+{\n+\treturn recs[0] == '\\n'; /* CRLF not covered here */\n+}\n+\n static int xdl_hash_and_recmatch(xrecord_t **recs, long ixs, long ix, long flags)\n {\n \treturn (recs[ixs]->ha == recs[ix]->ha &&\n@@ -411,6 +416,7 @@ static int xdl_hash_and_recmatch(xrecord_t **recs, long ixs, long ix, long flags\n int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n \tlong ix, ixo, ixs, ixref, grpsiz, nrec = xdf->nrec;\n \tchar *rchg = xdf->rchg, *rchgo = xdfo->rchg;\n+\tunsigned int emptylines;\n \txrecord_t **recs = xdf->recs;\n \n \t/*\n@@ -444,6 +450,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n \n \t\tdo {\n \t\t\tgrpsiz = ix - ixs;\n+\t\t\temptylines = 0;\n \n \t\t\t/*\n \t\t\t * If the line before the current change group, is equal to\n@@ -451,6 +458,8 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n \t\t\t * the group.\n \t\t\t */\n \t\t\twhile (ixs > 0 && xdl_hash_and_recmatch(recs, ixs - 1, ix - 1, flags)) {\n+\t\t\t\temptylines += is_emptyline(recs[ix - 1]->ptr);\n+\n \t\t\t\trchg[--ixs] = 1;\n \t\t\t\trchg[--ix] = 0;\n \n@@ -506,6 +515,23 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n \t\t\trchg[--ix] = 0;\n \t\t\twhile (rchgo[--ixo]);\n \t\t}\n+\n+\t\t/*\n+\t\t * If a group can be moved back and forth, see if there is an\n+\t\t * empty line in the moving space. If there is an empty line,\n+\t\t * make sure the last empty line is the end of the group.\n+\t\t *\n+\t\t * As we shifted the group forward as far as possible, we only\n+\t\t * need to shift it back if at all.\n+\t\t */\n+\t\tif ((flags & XDF_EMPTY_LINE_HEURISTIC) && emptylines) {\n+\t\t\twhile (ixs > 0 &&\n+\t\t\t !is_emptyline(recs[ix - 1]->ptr) &&\n+\t\t\t xdl_hash_and_recmatch(recs, ixs - 1, ix - 1, flags)) {\n+\t\t\t\trchg[--ixs] = 1;\n+\t\t\t\trchg[--ix] = 0;\n+\t\t\t}\n+\t\t}\n \t}\n \n \treturn 0;\n-- \n2.8.1.369.geae769a\n"},{"id":"283542","messageId":"CAGZ79kbCHA3L6mUfYn6OfVXLDEyhv70PwxXo-YHP_QZXXAB8ig@mail.gmail.com","threadId":"42037","inReplyTo":"20160415165141.4712-1-jacob.e.keller@intel.com","subject":"Re: [RFC PATCH, WAS: \"weird diff output?\" 0/2] implement better chunk heuristics","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-15T17:02:39Z","receivedAt":"2016-04-15T17:02:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Apr 15, 2016 at 9:51 AM, Jacob Keller wrote:\n> From: Jacob Keller \n>\n> I took up Stefan's patch, and modified it to resolve a couple issues. I\n> also tried to implement the suggestions from Junio's review, as well as\n> the suggestions I had. It appears to produce equivalent output as Jeff's\n> script. This version is still missing a few things, and it is possible\n> Stefan is working on a v2 of his own, but I thought I'd submit this.\n>\n> There's still several TODOs:\n> * Add some tests\n> * better name for the heuristic(?)\n> * better patch description plus documentation\n> * is_emptyline should be more generic and handle CRLF\n>\n> Changes since Stefan's v1:\n> * Added a patch to implement xdl_hash_and_recmatch as Junio suggested.\n> * Fixed a segfault in Stefan's patch\n> * Added XDL flag to configure the behavior\n> * Used an int and counted empty lines via += instead of |=\n> * Renamed starts_with_emptyline to is_emptyline\n> * Added diff command line and config options\n>\n> For reviewer convenience, the interdiff between this and Stefan's version:\n>\n> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\n> index edba56522bce..cebf82702d2a 100644\n> --- a/Documentation/diff-config.txt\n> +++ b/Documentation/diff-config.txt\n> @@ -170,6 +170,12 @@ diff.tool::\n>\n> include::mergetools-diff.txt[]\n>\n> +diff.emptyLineHeuristic::\n> + Set this option to true to enable the empty line chunk heuristic when\n> + producing diff output. This heuristic will attempt to shift hunks such\n> + that a common empty line occurs below the hunk with the rest of the\n> + context above it.\n> +\n\n This heuristic will attempt to shift hunks such that *the last* common\n empty line occurs below the hunk with the rest of the context above it.\n\nmaybe?\n\n\n> diff.algorithm::\n> Choose a diff algorithm. The variants are as follows:\n> +\n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index 4b0318e2ac15..6c1cd8b35584 100644\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -63,6 +63,12 @@ ifndef::git-format-patch[]\n> Synonym for `-p --raw`.\n> endif::git-format-patch[]\n>\n> +--empty-line-heuristic::\n> +--no-empty-line-heuristic::\n> + When possible, shift common empty lines in diff hunks below the hunk\n> + such that the last common empty line for each hunk is below, with the\n> + rest of the context above the hunk.\n> +\n> --minimal::\n> Spend extra time to make sure the smallest possible\n> diff is produced.\n> diff --git a/diff.c b/diff.c\n> index 4dfe6609d059..8ab9a492928d 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -26,6 +26,7 @@\n> #endif\n>\n> static int diff_detect_rename_default;\n> +static int diff_empty_line_heuristic = 0;\n> static int diff_rename_limit_default = 400;\n> static int diff_suppress_blank_empty;\n> static int diff_use_color_default = -1;\n> @@ -189,6 +190,10 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n> diff_detect_rename_default = git_config_rename(var, value);\n> return 0;\n> }\n> + if (!strcmp(var, \"diff.emptylineheuristic\")) {\n> + diff_empty_line_heuristic = git_config_bool(var, value);\n> + return 0;\n> + }\n> if (!strcmp(var, \"diff.autorefreshindex\")) {\n> diff_auto_refresh_index = git_config_bool(var, value);\n> return 0;\n> @@ -3278,6 +3283,8 @@ void diff_setup(struct diff_options *options)\n> options->use_color = diff_use_color_default;\n> options->detect_rename = diff_detect_rename_default;\n> options->xdl_opts |= diff_algorithm;\n> + if (diff_empty_line_heuristic)\n> + DIFF_XDL_SET(options, EMPTY_LINE_HEURISTIC);\n>\n> options->orderfile = diff_order_file_cfg;\n>\n> @@ -3798,6 +3805,10 @@ int diff_opt_parse(struct diff_options *options,\n> DIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);\n> else if (!strcmp(arg, \"--ignore-blank-lines\"))\n> DIFF_XDL_SET(options, IGNORE_BLANK_LINES);\n> + else if (!strcmp(arg, \"--empty-line-heuristic\"))\n> + DIFF_XDL_SET(options, EMPTY_LINE_HEURISTIC);\n> + else if (!strcmp(arg, \"--no-empty-line-heuristic\"))\n> + DIFF_XDL_CLR(options, EMPTY_LINE_HEURISTIC);\n> else if (!strcmp(arg, \"--patience\"))\n> options->xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);\n> else if (!strcmp(arg, \"--histogram\"))\n> diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\n> index 4fb7e79410c2..9195a5c0e958 100644\n> --- a/xdiff/xdiff.h\n> +++ b/xdiff/xdiff.h\n> @@ -41,6 +41,8 @@ extern \"C\" {\n>\n> #define XDF_IGNORE_BLANK_LINES (1 << 7)\n>\n> +#define XDF_EMPTY_LINE_HEURISTIC (1 << 8)\n> +\n> #define XDL_EMIT_FUNCNAMES (1 << 0)\n> #define XDL_EMIT_FUNCCONTEXT (1 << 2)\n>\n> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\n> index 5f14beb98049..83984b90f82f 100644\n> --- a/xdiff/xdiffi.c\n> +++ b/xdiff/xdiffi.c\n> @@ -400,7 +400,7 @@ static xdchange_t *xdl_add_change(xdchange_t *xscr, long i1, long i2, long chg1,\n> }\n>\n>\n> -static int starts_with_emptyline(const char *recs)\n> +static int is_emptyline(const char *recs)\n> {\n> return recs[0] == '\\n'; /* CRLF not covered here */\n> }\n> @@ -416,7 +416,7 @@ static int xdl_hash_and_recmatch(xrecord_t **recs, long ixs, long ix, long flags\n> int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n> long ix, ixo, ixs, ixref, grpsiz, nrec = xdf->nrec;\n> char *rchg = xdf->rchg, *rchgo = xdfo->rchg;\n> - unsigned char has_emptyline;\n> + unsigned int emptylines;\n> xrecord_t **recs = xdf->recs;\n>\n> /*\n> @@ -450,7 +450,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n>\n> do {\n> grpsiz = ix - ixs;\n> - has_emptyline = 0;\n> + emptylines = 0;\n>\n> /*\n> * If the line before the current change group, is equal to\n> @@ -458,11 +458,11 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n> * the group.\n> */\n> while (ixs > 0 && xdl_hash_and_recmatch(recs, ixs - 1, ix - 1, flags)) {\n> + emptylines += is_emptyline(recs[ix - 1]->ptr);\n> +\n> rchg[--ixs] = 1;\n> rchg[--ix] = 0;\n>\n> - has_emptyline |=\n> - starts_with_emptyline(recs[ix]->ptr);\n\nHow is this fixing the segfault bug?\nFrom my understanding ix should have the same value here as ix-1 above.\n\n> /*\n> * This change might have joined two change groups,\n> * so we try to take this scenario in account by moving\n> @@ -492,9 +492,6 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n> rchg[ixs++] = 0;\n> rchg[ix++] = 1;\n>\n> - has_emptyline |=\n> - starts_with_emptyline(recs[ix]->ptr);\n> -\n\nOr would this fix the segfault bug?\nI think we would need to have the check for empty lines\nin both loops to be sure to cover the whole movable range.\n\n\n> /*\n> * This change might have joined two change groups,\n> * so we try to take this scenario in account by moving\n> @@ -527,21 +524,12 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n> * As we shifted the group forward as far as possible, we only\n> * need to shift it back if at all.\n> */\n> - if (has_emptyline) {\n> - while (ixs > 0 && recs[ixs - 1]->ha == recs[ix - 1]->ha &&\n> - xdl_recmatch(recs[ixs - 1]->ptr, recs[ixs - 1]->size, recs[ix - 1]->ptr, recs[ix - 1]->size, flags) &&\n> - !starts_with_emptyline(recs[ix - 1]->ptr)) {\n> + if ((flags & XDF_EMPTY_LINE_HEURISTIC) && emptylines) {\n> + while (ixs > 0 &&\n> + !is_emptyline(recs[ix - 1]->ptr) &&\n> + xdl_hash_and_recmatch(recs, ixs - 1, ix - 1, flags)) {\n> rchg[--ixs] = 1;\n> rchg[--ix] = 0;\n> -\n> - /*\n> - * This change did not join two change groups,\n> - * as we did that before already, so there is no\n> - * need to adapt the other-file, i.e.\n> - * running\n> - * for (; rchg[ixs - 1]; ixs--);\n> - * while (rchgo[--ixo]);\n> - */\n> }\n> }\n> }\n>\n> Jacob Keller (1):\n> xdiff: add xdl_hash_and_recmatch helper function\n>\n> Stefan Beller (1):\n> xdiff: implement empty line chunk heuristic\n>\n> Documentation/diff-config.txt | 6 ++++++\n> Documentation/diff-options.txt | 6 ++++++\n> diff.c | 11 +++++++++++\n> xdiff/xdiff.h | 2 ++\n> xdiff/xdiffi.c | 42 ++++++++++++++++++++++++++++++++++++++----\n> 5 files changed, 63 insertions(+), 4 deletions(-)\n>\n> --\n> 2.8.1.369.geae769a\n>\n"},{"id":"283545","messageId":"CAGZ79kZs33sJj+DPSS4FPoJTLqbCLpvSe_h9UUQM-dBe=8ExKw@mail.gmail.com","threadId":"42037","inReplyTo":"CAGZ79kbCHA3L6mUfYn6OfVXLDEyhv70PwxXo-YHP_QZXXAB8ig@mail.gmail.com","subject":"Re: [RFC PATCH, WAS: \"weird diff output?\" 0/2] implement better chunk heuristics","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-15T17:10:31Z","receivedAt":"2016-04-15T17:10:31Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Apr 15, 2016 at 10:02 AM, Stefan Beller wrote:\n>> @@ -458,11 +458,11 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n>> * the group.\n>> */\n>> while (ixs > 0 && xdl_hash_and_recmatch(recs, ixs - 1, ix - 1, flags)) {\n>> + emptylines += is_emptyline(recs[ix - 1]->ptr);\n>> +\n>> rchg[--ixs] = 1;\n>> rchg[--ix] = 0;\n>>\n>> - has_emptyline |=\n>> - starts_with_emptyline(recs[ix]->ptr);\n>\n> How is this fixing the segfault bug?\n> From my understanding ix should have the same value here as ix-1 above.\n>\n>> /*\n>> * This change might have joined two change groups,\n>> * so we try to take this scenario in account by moving\n>> @@ -492,9 +492,6 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n>> rchg[ixs++] = 0;\n>> rchg[ix++] = 1;\n>>\n>> - has_emptyline |=\n>> - starts_with_emptyline(recs[ix]->ptr);\n>> -\n>\n> Or would this fix the segfault bug?\n> I think we would need to have the check for empty lines\n> in both loops to be sure to cover the whole movable range.\n\nActually we would only need to have the empty line count in the second loop as\nthe first loop shifted it as much up(backwards) as possible, such that\nthe hunk sits on one\nend of the movable range. The second loop would iterate over the whole\nrange, so that\nwould be the only place needed to count.\n"},{"id":"283547","messageId":"xmqq60vidbsu.fsf@gitster.mtv.corp.google.com","threadId":"42037","inReplyTo":"20160415165141.4712-2-jacob.e.keller@intel.com","subject":"Re: [RFC PATCH, WAS: \"weird diff output?\" 1/2] xdiff: add xdl_hash_and_recmatch helper function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-15T17:25:21Z","receivedAt":"2016-04-15T17:25:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> From: Jacob Keller \n>\n> It is a common pattern in xdl_change_compact to check that hashes and\n> strings match. The resulting code to perform this change causes very\n> long lines and makes it hard to follow the intention. Introduce a helper\n> function xdl_hash_and_recmatch which performs both checks to increase\n> code readability.\n\nThink _why_ it is common to check hash and then do recmatch(). What\nis the combination of two trying to compute?\n\nHow about calling it after \"what\" it computes, not after \"how\" it\ncomputes it? E.g.\n\n static int recs_match(xrecord_t **recs, long x, long y, long flags)\n\nif we answer the above question \"they try to see if two records match\".\nWe could also go s/recs/lines/.\n\nThe xdl_recmatch() function appears in xutils.c, and over there the\nfunctions do not use arrays of (xrecord_t *), so I think we are\nbetter off without xdl_ prefix to avoid confusion.\n\n> Signed-off-by: Jacob Keller \n> ---\n> xdiff/xdiffi.c | 16 ++++++++++++----\n> 1 file changed, 12 insertions(+), 4 deletions(-)\n>\n> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\n> index 2358a2d6326e..fff97ac3e3c7 100644\n> --- a/xdiff/xdiffi.c\n> +++ b/xdiff/xdiffi.c\n> @@ -400,6 +400,14 @@ static xdchange_t *xdl_add_change(xdchange_t *xscr, long i1, long i2, long chg1,\n> }\n> \n> \n> +static int xdl_hash_and_recmatch(xrecord_t **recs, long ixs, long ix, long flags)\n> +{\n> +\treturn (recs[ixs]->ha == recs[ix]->ha &&\n> +\t\txdl_recmatch(recs[ixs]->ptr, recs[ixs]->size,\n> +\t\t\t recs[ix]->ptr, recs[ix]->size,\n> +\t\t\t flags));\n> +}\n> +\n> int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n> \tlong ix, ixo, ixs, ixref, grpsiz, nrec = xdf->nrec;\n> \tchar *rchg = xdf->rchg, *rchgo = xdfo->rchg;\n> @@ -442,8 +450,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n> \t\t\t * the last line of the current change group, shift backward\n> \t\t\t * the group.\n> \t\t\t */\n> -\t\t\twhile (ixs > 0 && recs[ixs - 1]->ha == recs[ix - 1]->ha &&\n> -\t\t\t xdl_recmatch(recs[ixs - 1]->ptr, recs[ixs - 1]->size, recs[ix - 1]->ptr, recs[ix - 1]->size, flags)) {\n> +\t\t\twhile (ixs > 0 && xdl_hash_and_recmatch(recs, ixs - 1, ix - 1, flags)) {\n> \t\t\t\trchg[--ixs] = 1;\n> \t\t\t\trchg[--ix] = 0;\n> \n> @@ -470,8 +477,9 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n> \t\t\t * the line next of the current change group, shift forward\n> \t\t\t * the group.\n> \t\t\t */\n> -\t\t\twhile (ix < nrec && recs[ixs]->ha == recs[ix]->ha &&\n> -\t\t\t xdl_recmatch(recs[ixs]->ptr, recs[ixs]->size, recs[ix]->ptr, recs[ix]->size, flags)) {\n> +\t\t\twhile (ix < nrec && xdl_hash_and_recmatch(recs, ixs, ix, flags)) {\n> +\t\t\t\temptylines += is_emptyline(recs[ix]->ptr);\n> +\n> \t\t\t\trchg[ixs++] = 0;\n> \t\t\t\trchg[ix++] = 1;\n"},{"id":"283548","messageId":"xmqq1t66dbp8.fsf@gitster.mtv.corp.google.com","threadId":"42037","inReplyTo":"CAGZ79kZs33sJj+DPSS4FPoJTLqbCLpvSe_h9UUQM-dBe=8ExKw@mail.gmail.com","subject":"Re: [RFC PATCH, WAS: \"weird diff output?\" 0/2] implement better chunk heuristics","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-15T17:27:31Z","receivedAt":"2016-04-15T17:27:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Actually we would only need to have the empty line count in the\n> second loop as the first loop shifted it as much up(backwards) as\n> possible, such that the hunk sits on one end of the movable\n> range. The second loop would iterate over the whole range, so that\n> would be the only place needed to count.\n\nThe description makes me wonder if we can do without two loops ;-)\n\nThat is, can we teach the first loop not to be so aggressive to\nshift \"as much as possible\" but notice there is an empty line we\nwould want to treat specially?\n"},{"id":"283550","messageId":"CAGZ79kZPsGuimv3pFAFdwuhD1ps74qx7Q6d1kqxxRX6VPwNYGQ@mail.gmail.com","threadId":"42037","inReplyTo":"xmqq1t66dbp8.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH, WAS: \"weird diff output?\" 0/2] implement better chunk heuristics","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-15T17:33:41Z","receivedAt":"2016-04-15T17:33:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Apr 15, 2016 at 10:27 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> Actually we would only need to have the empty line count in the\n>> second loop as the first loop shifted it as much up(backwards) as\n>> possible, such that the hunk sits on one end of the movable\n>> range. The second loop would iterate over the whole range, so that\n>> would be the only place needed to count.\n>\n> The description makes me wonder if we can do without two loops ;-)\n\nI think the existing 2 loops are needed to move \"as much as possible\"\nto either boundary to see if there is overlap to another group and combine\nthe groups if so. (This whole construction prior to these patches remind\nme of shaker sort)\n\n>\n> That is, can we teach the first loop not to be so aggressive to\n> shift \"as much as possible\" but notice there is an empty line we\n> would want to treat specially?\n\nI think we need to be aggressive to find adjacent groups and only after\nthat is done we should think about optimizing look&feel? That is why I\neven proposed to not touch the current 2 loops at all and have our own\nloop to find out if there is at least one empty line.\n"},{"id":"283552","messageId":"xmqqshymbw6g.fsf@gitster.mtv.corp.google.com","threadId":"42037","inReplyTo":"CAGZ79kZPsGuimv3pFAFdwuhD1ps74qx7Q6d1kqxxRX6VPwNYGQ@mail.gmail.com","subject":"Re: [RFC PATCH, WAS: \"weird diff output?\" 0/2] implement better chunk heuristics","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-15T17:48:07Z","receivedAt":"2016-04-15T17:48:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> I think we need to be aggressive to find adjacent groups and only after\n> that is done we should think about optimizing look&feel?\n\nOK. I was just gauging to see if those involved in the codepath\nthought things through, and apparently you did, so I'm happy ;-)\n\nThanks.\n"},{"id":"283557","messageId":"CA+P7+xr4uEwOaLWCNvhQ2gRxn7YA04CJ3xT8mTB2wWrpPbJ5PQ@mail.gmail.com","threadId":"42037","inReplyTo":"CAGZ79kbCHA3L6mUfYn6OfVXLDEyhv70PwxXo-YHP_QZXXAB8ig@mail.gmail.com","subject":"Re: [RFC PATCH, WAS: \"weird diff output?\" 0/2] implement better chunk heuristics","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-04-15T19:07:29Z","receivedAt":"2016-04-15T19:07:29Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Apr 15, 2016 at 10:02 AM, Stefan Beller wrote:\n> On Fri, Apr 15, 2016 at 9:51 AM, Jacob Keller wrote:\n>> From: Jacob Keller \n>>\n>> I took up Stefan's patch, and modified it to resolve a couple issues. I\n>> also tried to implement the suggestions from Junio's review, as well as\n>> the suggestions I had. It appears to produce equivalent output as Jeff's\n>> script. This version is still missing a few things, and it is possible\n>> Stefan is working on a v2 of his own, but I thought I'd submit this.\n>>\n>> There's still several TODOs:\n>> * Add some tests\n>> * better name for the heuristic(?)\n>> * better patch description plus documentation\n>> * is_emptyline should be more generic and handle CRLF\n>>\n>> Changes since Stefan's v1:\n>> * Added a patch to implement xdl_hash_and_recmatch as Junio suggested.\n>> * Fixed a segfault in Stefan's patch\n>> * Added XDL flag to configure the behavior\n>> * Used an int and counted empty lines via += instead of |=\n>> * Renamed starts_with_emptyline to is_emptyline\n>> * Added diff command line and config options\n>>\n>> For reviewer convenience, the interdiff between this and Stefan's version:\n>>\n>> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\n>> index edba56522bce..cebf82702d2a 100644\n>> --- a/Documentation/diff-config.txt\n>> +++ b/Documentation/diff-config.txt\n>> @@ -170,6 +170,12 @@ diff.tool::\n>>\n>> include::mergetools-diff.txt[]\n>>\n>> +diff.emptyLineHeuristic::\n>> + Set this option to true to enable the empty line chunk heuristic when\n>> + producing diff output. This heuristic will attempt to shift hunks such\n>> + that a common empty line occurs below the hunk with the rest of the\n>> + context above it.\n>> +\n>\n> This heuristic will attempt to shift hunks such that *the last* common\n> empty line occurs below the hunk with the rest of the context above it.\n>\n> maybe?\n>\n\nThat sounds reasonable.\n\n>\n>> diff.algorithm::\n>> Choose a diff algorithm. The variants are as follows:\n>> +\n>> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n>> index 4b0318e2ac15..6c1cd8b35584 100644\n>> --- a/Documentation/diff-options.txt\n>> +++ b/Documentation/diff-options.txt\n>> @@ -63,6 +63,12 @@ ifndef::git-format-patch[]\n>> Synonym for `-p --raw`.\n>> endif::git-format-patch[]\n>>\n>> +--empty-line-heuristic::\n>> +--no-empty-line-heuristic::\n>> + When possible, shift common empty lines in diff hunks below the hunk\n>> + such that the last common empty line for each hunk is below, with the\n>> + rest of the context above the hunk.\n>> +\n>> --minimal::\n>> Spend extra time to make sure the smallest possible\n>> diff is produced.\n>> diff --git a/diff.c b/diff.c\n>> index 4dfe6609d059..8ab9a492928d 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -26,6 +26,7 @@\n>> #endif\n>>\n>> static int diff_detect_rename_default;\n>> +static int diff_empty_line_heuristic = 0;\n>> static int diff_rename_limit_default = 400;\n>> static int diff_suppress_blank_empty;\n>> static int diff_use_color_default = -1;\n>> @@ -189,6 +190,10 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n>> diff_detect_rename_default = git_config_rename(var, value);\n>> return 0;\n>> }\n>> + if (!strcmp(var, \"diff.emptylineheuristic\")) {\n>> + diff_empty_line_heuristic = git_config_bool(var, value);\n>> + return 0;\n>> + }\n>> if (!strcmp(var, \"diff.autorefreshindex\")) {\n>> diff_auto_refresh_index = git_config_bool(var, value);\n>> return 0;\n>> @@ -3278,6 +3283,8 @@ void diff_setup(struct diff_options *options)\n>> options->use_color = diff_use_color_default;\n>> options->detect_rename = diff_detect_rename_default;\n>> options->xdl_opts |= diff_algorithm;\n>> + if (diff_empty_line_heuristic)\n>> + DIFF_XDL_SET(options, EMPTY_LINE_HEURISTIC);\n>>\n>> options->orderfile = diff_order_file_cfg;\n>>\n>> @@ -3798,6 +3805,10 @@ int diff_opt_parse(struct diff_options *options,\n>> DIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);\n>> else if (!strcmp(arg, \"--ignore-blank-lines\"))\n>> DIFF_XDL_SET(options, IGNORE_BLANK_LINES);\n>> + else if (!strcmp(arg, \"--empty-line-heuristic\"))\n>> + DIFF_XDL_SET(options, EMPTY_LINE_HEURISTIC);\n>> + else if (!strcmp(arg, \"--no-empty-line-heuristic\"))\n>> + DIFF_XDL_CLR(options, EMPTY_LINE_HEURISTIC);\n>> else if (!strcmp(arg, \"--patience\"))\n>> options->xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);\n>> else if (!strcmp(arg, \"--histogram\"))\n>> diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\n>> index 4fb7e79410c2..9195a5c0e958 100644\n>> --- a/xdiff/xdiff.h\n>> +++ b/xdiff/xdiff.h\n>> @@ -41,6 +41,8 @@ extern \"C\" {\n>>\n>> #define XDF_IGNORE_BLANK_LINES (1 << 7)\n>>\n>> +#define XDF_EMPTY_LINE_HEURISTIC (1 << 8)\n>> +\n>> #define XDL_EMIT_FUNCNAMES (1 << 0)\n>> #define XDL_EMIT_FUNCCONTEXT (1 << 2)\n>>\n>> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\n>> index 5f14beb98049..83984b90f82f 100644\n>> --- a/xdiff/xdiffi.c\n>> +++ b/xdiff/xdiffi.c\n>> @@ -400,7 +400,7 @@ static xdchange_t *xdl_add_change(xdchange_t *xscr, long i1, long i2, long chg1,\n>> }\n>>\n>>\n>> -static int starts_with_emptyline(const char *recs)\n>> +static int is_emptyline(const char *recs)\n>> {\n>> return recs[0] == '\\n'; /* CRLF not covered here */\n>> }\n>> @@ -416,7 +416,7 @@ static int xdl_hash_and_recmatch(xrecord_t **recs, long ixs, long ix, long flags\n>> int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n>> long ix, ixo, ixs, ixref, grpsiz, nrec = xdf->nrec;\n>> char *rchg = xdf->rchg, *rchgo = xdfo->rchg;\n>> - unsigned char has_emptyline;\n>> + unsigned int emptylines;\n>> xrecord_t **recs = xdf->recs;\n>>\n>> /*\n>> @@ -450,7 +450,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n>>\n>> do {\n>> grpsiz = ix - ixs;\n>> - has_emptyline = 0;\n>> + emptylines = 0;\n>>\n>> /*\n>> * If the line before the current change group, is equal to\n>> @@ -458,11 +458,11 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n>> * the group.\n>> */\n>> while (ixs > 0 && xdl_hash_and_recmatch(recs, ixs - 1, ix - 1, flags)) {\n>> + emptylines += is_emptyline(recs[ix - 1]->ptr);\n>> +\n>> rchg[--ixs] = 1;\n>> rchg[--ix] = 0;\n>>\n>> - has_emptyline |=\n>> - starts_with_emptyline(recs[ix]->ptr);\n>\n> How is this fixing the segfault bug?\n> From my understanding ix should have the same value here as ix-1 above.\n>\n\nThis change was for style reasons to check for empty lines first\nthing, and it matches how we do \"ix - 1\" into xdl_hash_and_recmatch.\nBut as per your following comment, I think you're right and we don't\nneed this.\n\n>> /*\n>> * This change might have joined two change groups,\n>> * so we try to take this scenario in account by moving\n>> @@ -492,9 +492,6 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n>> rchg[ixs++] = 0;\n>> rchg[ix++] = 1;\n>>\n>> - has_emptyline |=\n>> - starts_with_emptyline(recs[ix]->ptr);\n>> -\n>\n> Or would this fix the segfault bug?\n> I think we would need to have the check for empty lines\n> in both loops to be sure to cover the whole movable range.\n>\n\nThis is the real segfault fix, I only changed the other in order to\nmake it appear consistent.\n\n>\n>> /*\n>> * This change might have joined two change groups,\n>> * so we try to take this scenario in account by moving\n>> @@ -527,21 +524,12 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n>> * As we shifted the group forward as far as possible, we only\n>> * need to shift it back if at all.\n>> */\n>> - if (has_emptyline) {\n>> - while (ixs > 0 && recs[ixs - 1]->ha == recs[ix - 1]->ha &&\n>> - xdl_recmatch(recs[ixs - 1]->ptr, recs[ixs - 1]->size, recs[ix - 1]->ptr, recs[ix - 1]->size, flags) &&\n>> - !starts_with_emptyline(recs[ix - 1]->ptr)) {\n>> + if ((flags & XDF_EMPTY_LINE_HEURISTIC) && emptylines) {\n>> + while (ixs > 0 &&\n>> + !is_emptyline(recs[ix - 1]->ptr) &&\n>> + xdl_hash_and_recmatch(recs, ixs - 1, ix - 1, flags)) {\n>> rchg[--ixs] = 1;\n>> rchg[--ix] = 0;\n>> -\n>> - /*\n>> - * This change did not join two change groups,\n>> - * as we did that before already, so there is no\n>> - * need to adapt the other-file, i.e.\n>> - * running\n>> - * for (; rchg[ixs - 1]; ixs--);\n>> - * while (rchgo[--ixo]);\n>> - */\n>> }\n>> }\n>> }\n>>\n>> Jacob Keller (1):\n>> xdiff: add xdl_hash_and_recmatch helper function\n>>\n>> Stefan Beller (1):\n>> xdiff: implement empty line chunk heuristic\n>>\n>> Documentation/diff-config.txt | 6 ++++++\n>> Documentation/diff-options.txt | 6 ++++++\n>> diff.c | 11 +++++++++++\n>> xdiff/xdiff.h | 2 ++\n>> xdiff/xdiffi.c | 42 ++++++++++++++++++++++++++++++++++++++----\n>> 5 files changed, 63 insertions(+), 4 deletions(-)\n>>\n>> --\n>> 2.8.1.369.geae769a\n>>\n"},{"id":"283558","messageId":"CA+P7+xq7JqerLizg_5RLNGuWPaQL8QWUisjZm=ENcdaE7ZPMDg@mail.gmail.com","threadId":"42037","inReplyTo":"CAGZ79kZs33sJj+DPSS4FPoJTLqbCLpvSe_h9UUQM-dBe=8ExKw@mail.gmail.com","subject":"Re: [RFC PATCH, WAS: \"weird diff output?\" 0/2] implement better chunk heuristics","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-04-15T19:08:10Z","receivedAt":"2016-04-15T19:08:10Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Apr 15, 2016 at 10:10 AM, Stefan Beller wrote:\n> On Fri, Apr 15, 2016 at 10:02 AM, Stefan Beller wrote:\n>>> @@ -458,11 +458,11 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n>>> * the group.\n>>> */\n>>> while (ixs > 0 && xdl_hash_and_recmatch(recs, ixs - 1, ix - 1, flags)) {\n>>> + emptylines += is_emptyline(recs[ix - 1]->ptr);\n>>> +\n>>> rchg[--ixs] = 1;\n>>> rchg[--ix] = 0;\n>>>\n>>> - has_emptyline |=\n>>> - starts_with_emptyline(recs[ix]->ptr);\n>>\n>> How is this fixing the segfault bug?\n>> From my understanding ix should have the same value here as ix-1 above.\n>>\n>>> /*\n>>> * This change might have joined two change groups,\n>>> * so we try to take this scenario in account by moving\n>>> @@ -492,9 +492,6 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {\n>>> rchg[ixs++] = 0;\n>>> rchg[ix++] = 1;\n>>>\n>>> - has_emptyline |=\n>>> - starts_with_emptyline(recs[ix]->ptr);\n>>> -\n>>\n>> Or would this fix the segfault bug?\n>> I think we would need to have the check for empty lines\n>> in both loops to be sure to cover the whole movable range.\n>\n> Actually we would only need to have the empty line count in the second loop as\n> the first loop shifted it as much up(backwards) as possible, such that\n> the hunk sits on one\n> end of the movable range. The second loop would iterate over the whole\n> range, so that\n> would be the only place needed to count.\n\nI agree that we can drop the first part and I am testing it now.\n\nThanks,\nJake\n"},{"id":"283559","messageId":"CA+P7+xpr1nVhKpMCPdUdd8nTJ5aj+Mcxr0ML2Qz4PRqwoerK=Q@mail.gmail.com","threadId":"42037","inReplyTo":"CAGZ79kZPsGuimv3pFAFdwuhD1ps74qx7Q6d1kqxxRX6VPwNYGQ@mail.gmail.com","subject":"Re: [RFC PATCH, WAS: \"weird diff output?\" 0/2] implement better chunk heuristics","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-04-15T19:09:09Z","receivedAt":"2016-04-15T19:09:09Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Apr 15, 2016 at 10:33 AM, Stefan Beller wrote:\n> On Fri, Apr 15, 2016 at 10:27 AM, Junio C Hamano wrote:\n>> Stefan Beller writes:\n>>\n>>> Actually we would only need to have the empty line count in the\n>>> second loop as the first loop shifted it as much up(backwards) as\n>>> possible, such that the hunk sits on one end of the movable\n>>> range. The second loop would iterate over the whole range, so that\n>>> would be the only place needed to count.\n>>\n>> The description makes me wonder if we can do without two loops ;-)\n>\n> I think the existing 2 loops are needed to move \"as much as possible\"\n> to either boundary to see if there is overlap to another group and combine\n> the groups if so. (This whole construction prior to these patches remind\n> me of shaker sort)\n>\n>>\n>> That is, can we teach the first loop not to be so aggressive to\n>> shift \"as much as possible\" but notice there is an empty line we\n>> would want to treat specially?\n>\n> I think we need to be aggressive to find adjacent groups and only after\n> that is done we should think about optimizing look&feel? That is why I\n> even proposed to not touch the current 2 loops at all and have our own\n> loop to find out if there is at least one empty line.\n\nAgreed, we need the two loops in order to aggressively merge all the\nchanges together first.\n\nThanks,\nJake\n"},{"id":"283560","messageId":"CA+P7+xquEuu0aLb9vFaFhzun4YYW4PmiXycAt01HUxxUXqUuyQ@mail.gmail.com","threadId":"42037","inReplyTo":"xmqq60vidbsu.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH, WAS: \"weird diff output?\" 1/2] xdiff: add xdl_hash_and_recmatch helper function","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-04-15T19:09:46Z","receivedAt":"2016-04-15T19:09:46Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Apr 15, 2016 at 10:25 AM, Junio C Hamano wrote:\n> Jacob Keller writes:\n>\n>> From: Jacob Keller \n>>\n>> It is a common pattern in xdl_change_compact to check that hashes and\n>> strings match. The resulting code to perform this change causes very\n>> long lines and makes it hard to follow the intention. Introduce a helper\n>> function xdl_hash_and_recmatch which performs both checks to increase\n>> code readability.\n>\n> Think _why_ it is common to check hash and then do recmatch(). What\n> is the combination of two trying to compute?\n>\n> How about calling it after \"what\" it computes, not after \"how\" it\n> computes it? E.g.\n>\n> static int recs_match(xrecord_t **recs, long x, long y, long flags)\n>\n> if we answer the above question \"they try to see if two records match\".\n> We could also go s/recs/lines/.\n>\n> The xdl_recmatch() function appears in xutils.c, and over there the\n> functions do not use arrays of (xrecord_t *), so I think we are\n> better off without xdl_ prefix to avoid confusion.\n>\n\nThat makes sense. I like the sound of recs_match, and it's shorter too\nwhich is nice.\n\nRegards,\nJake\n"},{"id":"283572","messageId":"CAGZ79ka7h25=rHun_hPv1qjqeghXt1UwUU3Q6xT0aj4+OW87fg@mail.gmail.com","threadId":"42037","inReplyTo":"20160415165141.4712-3-jacob.e.keller@intel.com","subject":"Re: [RFC PATCH, WAS: \"weird diff output?\" 2/2] xdiff: implement empty line chunk heuristic","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-15T19:57:05Z","receivedAt":"2016-04-15T19:57:05Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Apr 15, 2016 at 9:51 AM, Jacob Keller wrote:\n> From: Stefan Beller \n>\n> In order to produce the smallest possible diff and combine several diff\n> hunks together, we implement a heuristic from GNU Diff which moves diff\n> hunks forward as far as possible when we find common context above and\n> below a diff hunk. This sometimes produces less readable diffs when\n> writing C, Shell, or other programming languages, ie:\n>\n> ...\n> /*\n> + *\n> + *\n> + */\n> +\n> +/*\n> ...\n>\n> instead of the more readable equivalent of\n>\n> ...\n> +/*\n> + *\n> + *\n> + */\n> +\n> /*\n> ...\n>\n> Implement the following heuristic to (optionally) produce the desired\n> output.\n>\n> If there are diff chunks which can be shifted around, shift each hunk\n> such that the last common empty line is below the chunk with the rest\n> of the context above.\n>\n> This heuristic appears to resolve the above example and several other\n> common issues without producing significantly weird results. However, as\n> with any heuristic it is not really known whether this will always be\n> more optimal. Thus, leave the heuristic disabled by default.\n>\n> Add an XDIFF flag to enable this heuristic only conditionally. Add\n> a diff command line option and diff configuration option to allow users\n> to enable this option when desired.\n>\n> TODO:\n> * Add tests\n> * Add better/more documentation explaining the heuristic, possibly with\n> examples(?)\n> * better name(?)\n>\n> Signed-off-by: Stefan Beller \n> Signed-off-by: Jacob Keller \n> ---\n> Documentation/diff-config.txt | 6 ++++++\n> Documentation/diff-options.txt | 6 ++++++\n> diff.c | 11 +++++++++++\n> xdiff/xdiff.h | 2 ++\n> xdiff/xdiffi.c | 26 ++++++++++++++++++++++++++\n> 5 files changed, 51 insertions(+)\n>\n> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\n> index edba56522bce..cebf82702d2a 100644\n> --- a/Documentation/diff-config.txt\n> +++ b/Documentation/diff-config.txt\n> @@ -170,6 +170,12 @@ diff.tool::\n>\n> include::mergetools-diff.txt[]\n>\n> +diff.emptyLineHeuristic::\n\nI was looking at the TODO here and thought about the name:\nIt should not encode the `emptyLine` into the config option as\nit is only one of many heuristics.\n\nIt should be something like `diff.heuristic=lastEmptyLine`\nThe we could add firstEmptyLine, aggressiveUp, aggressiveDown,\nbreakAtShortestLineLength or other styles as well later on.\n\nI do not quite understand the difference between diff.algorithm\nand the newly proposed heuristic as the heuristic is part of\nthe algorithm? So I guess we'd need to have some documentation\nsaying how these differ. (fundamental algorithm vs last minute\nstyle fixup?)\n"},{"id":"283574","messageId":"xmqq8u0ebpru.fsf@gitster.mtv.corp.google.com","threadId":"42037","inReplyTo":"CAGZ79ka7h25=rHun_hPv1qjqeghXt1UwUU3Q6xT0aj4+OW87fg@mail.gmail.com","subject":"Re: [RFC PATCH, WAS: \"weird diff output?\" 2/2] xdiff: implement empty line chunk heuristic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-15T20:06:29Z","receivedAt":"2016-04-15T20:06:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> +diff.emptyLineHeuristic::\n>\n> I was looking at the TODO here and thought about the name:\n> It should not encode the `emptyLine` into the config option as\n> it is only one of many heuristics.\n>\n> It should be something like `diff.heuristic=lastEmptyLine`\n> The we could add firstEmptyLine, aggressiveUp, aggressiveDown,\n> breakAtShortestLineLength or other styles as well later on.\n>\n> I do not quite understand the difference between diff.algorithm\n> and the newly proposed heuristic as the heuristic is part of\n> the algorithm? So I guess we'd need to have some documentation\n> saying how these differ. (fundamental algorithm vs last minute\n> style fixup?)\n\nI actually do not think these knobs should exist when the code is\nmature enough to be shipped to the end users.\n\nUse \"diff.compactionHeuristics = \" as an opaque set of bits to\nhelp the developers while they compare notes and reach consensus on\na single tweak that they can agree on being good enough, and then\nremove that variable before the code hits 'next'.\n\nThanks.\n"},{"id":"283575","messageId":"CA+P7+xopb-kz4ipvoH6q14F+Sc5vqqHG5z93NdxJJzKwg_TSWA@mail.gmail.com","threadId":"42037","inReplyTo":"CAGZ79ka7h25=rHun_hPv1qjqeghXt1UwUU3Q6xT0aj4+OW87fg@mail.gmail.com","subject":"Re: [RFC PATCH, WAS: \"weird diff output?\" 2/2] xdiff: implement empty line chunk heuristic","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-04-15T20:06:31Z","receivedAt":"2016-04-15T20:06:31Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Apr 15, 2016 at 12:57 PM, Stefan Beller wrote:\n> I was looking at the TODO here and thought about the name:\n> It should not encode the `emptyLine` into the config option as\n> it is only one of many heuristics.\n>\n> It should be something like `diff.heuristic=lastEmptyLine`\n> The we could add firstEmptyLine, aggressiveUp, aggressiveDown,\n> breakAtShortestLineLength or other styles as well later on.\n>\n\nThis sounds better, but how does this handle multiple heuristics?\n\n> I do not quite understand the difference between diff.algorithm\n> and the newly proposed heuristic as the heuristic is part of\n> the algorithm? So I guess we'd need to have some documentation\n> saying how these differ. (fundamental algorithm vs last minute\n> style fixup?)\n\nIt is not part of the algorithm. It's applied after the algorithm.\nxdl_change_compact is run after the algorithm and run for all\nalgorithms.\n\nThese are last minute style changes, and should probably not use the\nterm heuristic, but somehow capture \"last minute style fixup\"\n\nThanks,\nJake\n\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"283576","messageId":"CA+P7+xoWbrSaONH5xq=w5W190Jknk0Qsc5brS4UKFAs2_dTceg@mail.gmail.com","threadId":"42037","inReplyTo":"xmqq8u0ebpru.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH, WAS: \"weird diff output?\" 2/2] xdiff: implement empty line chunk heuristic","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-04-15T20:17:59Z","receivedAt":"2016-04-15T20:17:59Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Apr 15, 2016 at 1:06 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>>> +diff.emptyLineHeuristic::\n>>\n>> I was looking at the TODO here and thought about the name:\n>> It should not encode the `emptyLine` into the config option as\n>> it is only one of many heuristics.\n>>\n>> It should be something like `diff.heuristic=lastEmptyLine`\n>> The we could add firstEmptyLine, aggressiveUp, aggressiveDown,\n>> breakAtShortestLineLength or other styles as well later on.\n>>\n>> I do not quite understand the difference between diff.algorithm\n>> and the newly proposed heuristic as the heuristic is part of\n>> the algorithm? So I guess we'd need to have some documentation\n>> saying how these differ. (fundamental algorithm vs last minute\n>> style fixup?)\n>\n> I actually do not think these knobs should exist when the code is\n> mature enough to be shipped to the end users.\n>\n> Use \"diff.compactionHeuristics = \" as an opaque set of bits to\n> help the developers while they compare notes and reach consensus on\n> a single tweak that they can agree on being good enough, and then\n> remove that variable before the code hits 'next'.\n>\n> Thanks.\n\nI was under the impression that we would want a longer lived\nconfiguration until we had enough data to say whether it was helpful\nto make it default. I guess i had thought it would need to be longer\nlived since there may be cases where it's not optimal and being able\nto turn it off would be good?\n\nI'd rather keep it semi-human readable vs a uint since it would help\nkeep me sane when looking at it in the interim.\n\nThanks,\nJake\n"},{"id":"283581","messageId":"xmqqzisuaa3t.fsf@gitster.mtv.corp.google.com","threadId":"42037","inReplyTo":"CA+P7+xoWbrSaONH5xq=w5W190Jknk0Qsc5brS4UKFAs2_dTceg@mail.gmail.com","subject":"Re: [RFC PATCH, WAS: \"weird diff output?\" 2/2] xdiff: implement empty line chunk heuristic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-15T20:30:14Z","receivedAt":"2016-04-15T20:30:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> On Fri, Apr 15, 2016 at 1:06 PM, Junio C Hamano wrote:\n>\n>> I actually do not think these knobs should exist when the code is\n>> mature enough to be shipped to the end users.\n>>\n>> Use \"diff.compactionHeuristics = \" as an opaque set of bits to\n>> help the developers while they compare notes and reach consensus on\n>> a single tweak that they can agree on being good enough, and then\n>> remove that variable before the code hits 'next'.\n>>\n>> Thanks.\n>\n> I was under the impression that we would want a longer lived\n> configuration until we had enough data to say whether it was\n> helpful to make it default. I guess i had thought it would need to\n> be longer lived since there may be cases where it's not optimal\n> and being able to turn it off would be good?\n\nOnce you start worrying about \"some cases this may misbehave\", a\nconfiguration variable is a wrong mechanism to do so anyway. You\nwould need to tie this to attributes, so the users can say \"use this\nheuristics for my C code, but do not apply it for my AsciiDoc\ninput\", etc.\n\nWhat you have is a pure developer support; aim to come up with \"good\nenough\" way, giving developers an easier way to experiment with, and\nremove it before the feature is shipped to the end user.\n"},{"id":"283587","messageId":"CA+P7+xqe4ng9-gn1DRqhjebRXuHXbqZ6f3QsJ798k6DRo3bYyQ@mail.gmail.com","threadId":"42037","inReplyTo":"xmqqzisuaa3t.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH, WAS: \"weird diff output?\" 2/2] xdiff: implement empty line chunk heuristic","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-04-15T21:15:17Z","receivedAt":"2016-04-15T21:15:17Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Apr 15, 2016 at 1:30 PM, Junio C Hamano wrote:\n> Jacob Keller writes:\n>\n>> On Fri, Apr 15, 2016 at 1:06 PM, Junio C Hamano wrote:\n>>\n>>> I actually do not think these knobs should exist when the code is\n>>> mature enough to be shipped to the end users.\n>>>\n>>> Use \"diff.compactionHeuristics = \" as an opaque set of bits to\n>>> help the developers while they compare notes and reach consensus on\n>>> a single tweak that they can agree on being good enough, and then\n>>> remove that variable before the code hits 'next'.\n>>>\n>>> Thanks.\n>>\n>> I was under the impression that we would want a longer lived\n>> configuration until we had enough data to say whether it was\n>> helpful to make it default. I guess i had thought it would need to\n>> be longer lived since there may be cases where it's not optimal\n>> and being able to turn it off would be good?\n>\n> Once you start worrying about \"some cases this may misbehave\", a\n> configuration variable is a wrong mechanism to do so anyway. You\n> would need to tie this to attributes, so the users can say \"use this\n> heuristics for my C code, but do not apply it for my AsciiDoc\n> input\", etc.\n>\n\nI think this makes perfect sense to apply this as an attribute,\nhowever.. why isn't the current diff algorithm done this way?\n\nThanks,\nJake\n\n> What you have is a pure developer support; aim to come up with \"good\n> enough\" way, giving developers an easier way to experiment with, and\n> remove it before the feature is shipped to the end user.\n"},{"id":"283590","messageId":"CA+P7+xp7oJoOXBhexe9zhrG1dMkz8jA3yQLzyTiqMwNQ1AQVdg@mail.gmail.com","threadId":"42037","inReplyTo":"CA+P7+xqe4ng9-gn1DRqhjebRXuHXbqZ6f3QsJ798k6DRo3bYyQ@mail.gmail.com","subject":"Re: [RFC PATCH, WAS: \"weird diff output?\" 2/2] xdiff: implement empty line chunk heuristic","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-04-15T21:22:48Z","receivedAt":"2016-04-15T21:22:48Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Apr 15, 2016 at 2:15 PM, Jacob Keller wrote:\n> On Fri, Apr 15, 2016 at 1:30 PM, Junio C Hamano wrote:\n>> Jacob Keller writes:\n>>\n>>> On Fri, Apr 15, 2016 at 1:06 PM, Junio C Hamano wrote:\n>>>\n>>>> I actually do not think these knobs should exist when the code is\n>>>> mature enough to be shipped to the end users.\n>>>>\n>>>> Use \"diff.compactionHeuristics = \" as an opaque set of bits to\n>>>> help the developers while they compare notes and reach consensus on\n>>>> a single tweak that they can agree on being good enough, and then\n>>>> remove that variable before the code hits 'next'.\n>>>>\n>>>> Thanks.\n>>>\n>>> I was under the impression that we would want a longer lived\n>>> configuration until we had enough data to say whether it was\n>>> helpful to make it default. I guess i had thought it would need to\n>>> be longer lived since there may be cases where it's not optimal\n>>> and being able to turn it off would be good?\n>>\n>> Once you start worrying about \"some cases this may misbehave\", a\n>> configuration variable is a wrong mechanism to do so anyway. You\n>> would need to tie this to attributes, so the users can say \"use this\n>> heuristics for my C code, but do not apply it for my AsciiDoc\n>> input\", etc.\n>>\n>\n> I think this makes perfect sense to apply this as an attribute,\n> however.. why isn't the current diff algorithm done this way?\n>\n> Thanks,\n> Jake\n>\n>> What you have is a pure developer support; aim to come up with \"good\n>> enough\" way, giving developers an easier way to experiment with, and\n>> remove it before the feature is shipped to the end user.\n\n\nWhat are your thoughts on adding this do the gitattributes diff\nsection? Ie: modifications to the diff driver.\n\nThanks,\nJake\n"},{"id":"283593","messageId":"xmqqzisu8s30.fsf@gitster.mtv.corp.google.com","threadId":"42037","inReplyTo":"CA+P7+xp7oJoOXBhexe9zhrG1dMkz8jA3yQLzyTiqMwNQ1AQVdg@mail.gmail.com","subject":"Re: [RFC PATCH, WAS: \"weird diff output?\" 2/2] xdiff: implement empty line chunk heuristic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-15T21:44:51Z","receivedAt":"2016-04-15T21:44:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n>>> What you have is a pure developer support; aim to come up with \"good\n>>> enough\" way, giving developers an easier way to experiment with, and\n>>> remove it before the feature is shipped to the end user.\n>\n> What are your thoughts on adding this do the gitattributes diff\n> section? Ie: modifications to the diff driver.\n\nI do try to foresee the future needs but I try to limit the forecast\nto \"just enough so that we won't waste engineering effort on a wrong\nthing\". \"It may need to be turned off conditionally\" suggests we\nwould want attributes added per path, and that is sufficient to make\nme say \"don't waste time on bikeshedding configuration variable\nnames, it will not be in the final product\".\n\nWe do not need yet to know what the final name of the attributes\nare, or how exactly the bit to affect the low level mechanism will\nbe set by the attribute mechanism. I do not think this topic is\nthere yet, and it is a waste of engineering effort to prematurely\ntrying to make things too flexible and customizable, when the thing\nthat will eventually become flexible by conditionally enabled is not\neven there yet.\n\nAs long as the low-level thing has a knob, set of internal bits, to\nenable and disable it, that is all that is necessary to know at this\npoint.\n\nHaving said all that, I'd expect we'd compute the right bit to use\nin the same place where we currently pick the custom textconv\ndriver, diff backend, etc., by consulting the attribute system\nbefore running the diff.\n\nBut again, I'd think it would be waste of time to think beyond that\nat this point, identifying exactly at which line of which source\nfile the new code would go and what that new code would look like,\nuntil we are ready to start integrating it.\n"},{"id":"283598","messageId":"CA+P7+xpqVVXNRnXNJfq7moWkuwqn8HN2iqDvd+5gLsyNc9_BgQ@mail.gmail.com","threadId":"42037","inReplyTo":"xmqqzisu8s30.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH, WAS: \"weird diff output?\" 2/2] xdiff: implement empty line chunk heuristic","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-04-15T21:56:30Z","receivedAt":"2016-04-15T21:56:30Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Apr 15, 2016 at 2:44 PM, Junio C Hamano wrote:\n> Jacob Keller writes:\n>\n>>>> What you have is a pure developer support; aim to come up with \"good\n>>>> enough\" way, giving developers an easier way to experiment with, and\n>>>> remove it before the feature is shipped to the end user.\n>>\n>> What are your thoughts on adding this do the gitattributes diff\n>> section? Ie: modifications to the diff driver.\n>\n> I do try to foresee the future needs but I try to limit the forecast\n> to \"just enough so that we won't waste engineering effort on a wrong\n> thing\". \"It may need to be turned off conditionally\" suggests we\n> would want attributes added per path, and that is sufficient to make\n> me say \"don't waste time on bikeshedding configuration variable\n> names, it will not be in the final product\".\n>\n> We do not need yet to know what the final name of the attributes\n> are, or how exactly the bit to affect the low level mechanism will\n> be set by the attribute mechanism. I do not think this topic is\n> there yet, and it is a waste of engineering effort to prematurely\n> trying to make things too flexible and customizable, when the thing\n> that will eventually become flexible by conditionally enabled is not\n> even there yet.\n>\n> As long as the low-level thing has a knob, set of internal bits, to\n> enable and disable it, that is all that is necessary to know at this\n> point.\n>\n> Having said all that, I'd expect we'd compute the right bit to use\n> in the same place where we currently pick the custom textconv\n> driver, diff backend, etc., by consulting the attribute system\n> before running the diff.\n>\n> But again, I'd think it would be waste of time to think beyond that\n> at this point, identifying exactly at which line of which source\n> file the new code would go and what that new code would look like,\n> until we are ready to start integrating it.\n\nOk, for now I'll leave this as is then.\n\nThanks,\nJake\n"}]}