{"thread":{"id":"23220","subject":"git diff too slow for a file","startedAt":"2010-03-29T01:42:11Z","lastAt":"2010-05-04T22:56:54Z","messageCount":12,"participants":["SungHyun Nam","René Scharfe","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"138040","messageId":"4BB00573.6040005@gmail.com","threadId":"23220","inReplyTo":null,"subject":"git diff too slow for a file","fromName":"SungHyun Nam","fromEmail":"goweol@gmail.com","sentAt":"2010-03-29T01:42:11Z","receivedAt":"2010-03-29T01:42:11Z","isPatch":false,"sender":{"key":"goweol@gmail.com","avatar":null},"body":"Hello,\n\nIf I run a attached script for bunzipped attached files, I get:\n(To reduce size, I removed many lines and bzipped.)\n\n     $ ./mk.sh\n     time diff -u x3 x4 >/dev/null 2>&1\n\n     real\t0m0.011s\n     user\t0m0.000s\n     sys\t0m0.010s\n\n     time git diff >/dev/null 2>&1\n\n     real\t0m0.193s\n     user\t0m0.190s\n     sys\t0m0.000s\n\n     $ git version\n     git version 1.7.0.2.273.gc2413\n\n     $ diff --version\n     diff (GNU diffutils) 2.8.1\n     ...\n\nWell, though the files are ascii file, they includes a random\nhexa-decimal datas, so that I don't interest the diff result at\nall.  But the real problem is 'rebasing took so long if the file\nwas changed'.  Because the git tree includes several such a file,\nif they changed, rebase took some miniutes for every branch.\nSuch a branch includes a few lines of changes for a C source file,\nthough.  Now I'm waiting an hour to finish rebasing all the\nbranches and yet a rebasing script is running... :-(\n\nPlease help!\nThanks,\nnamsh\n\nThe original file has more than 180000 lines and the result is:\n     $ ./mk.sh\n     time diff -u x3 x4 >/dev/null 2>&1\n\n     real\t0m0.759s\n     user\t0m0.740s\n     sys\t0m0.010s\n\n     time git diff >/dev/null 2>&1\n\n     real\t0m44.460s\n     user\t0m44.390s\n     sys\t0m0.030s\n\n\n\n#!/bin/bash\n\nrun_command()\n{\n    echo \"$@\"\n    eval \"$@\"\n}\n\nrun_command 'time diff -u x3 x4 >/dev/null 2>&1'\n\n{\n    rm -rf u\n    mkdir u\n    cd u\n    cp ../x3 x\n    git init\n    git add .\n    git ci -m x\n    cp ../x4 x\n} >/dev/null 2>&1\n\necho ''\nrun_command 'time git diff >/dev/null 2>&1'\n\nexit 0\n"},{"id":"139753","messageId":"4BC9D928.50909@lsrfire.ath.cx","threadId":"23220","inReplyTo":"4BB00573.6040005@gmail.com","subject":"Re: git diff too slow for a file","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-04-17T15:52:08Z","receivedAt":"2010-04-17T15:52:08Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 29.03.2010 03:42, schrieb SungHyun Nam:\n> Hello,\n> \n> If I run a attached script for bunzipped attached files, I get:\n> (To reduce size, I removed many lines and bzipped.)\n> \n>     $ ./mk.sh\n>     time diff -u x3 x4 >/dev/null 2>&1\n> \n>     real    0m0.011s\n>     user    0m0.000s\n>     sys    0m0.010s\n> \n>     time git diff >/dev/null 2>&1\n> \n>     real    0m0.193s\n>     user    0m0.190s\n>     sys    0m0.000s\n> \n>     $ git version\n>     git version 1.7.0.2.273.gc2413\n> \n>     $ diff --version\n>     diff (GNU diffutils) 2.8.1\n>     ...\n> \n> Well, though the files are ascii file, they includes a random\n> hexa-decimal datas, so that I don't interest the diff result at\n> all.  But the real problem is 'rebasing took so long if the file\n> was changed'.  Because the git tree includes several such a file,\n> if they changed, rebase took some miniutes for every branch.\n> Such a branch includes a few lines of changes for a C source file,\n> though.  Now I'm waiting an hour to finish rebasing all the\n> branches and yet a rebasing script is running... :-(\n\nI can reproduce it; I concatenated your example files five times to get\nmeaningful timings (x1 = five times x3, x2 = five times x4).\n\nThe difference between GNU diff and git diff is that the latter is trying\nhard to minimize the size of the diff.  Each user of the xdiff library in\ngit turns on the XDF_NEED_MINIMAL flag, which makes it very expensive\n(specifically the function xdl_split()).\n\nThe following patch is not meant for inclusion, but rather to start a\ndicussion.  Is XDF_NEED_MINIMAL a good default to have?\n\nThe patch removes XDF_NEED_MINIMAL and replaces it with XDF_QUICK, with\nreversed meaning.  XDF_QUICK is only set if the new option --quick is\ngiven, so without it the old behaviour is retained.  Some numbers:\n\n$ time diff ../x1 ../x2 | diffstat\n unknown |34208 ++++++++++++++++++++++++++++++++--------------------------------\n 1 file changed, 17104 insertions(+), 17104 deletions(-)\n\nreal\t0m0.043s\nuser\t0m0.030s\nsys\t0m0.010s\n\n$ time git diff --stat\n x |32082 ++++++++++++++++++++++++++++++++++----------------------------------\n 1 files changed, 16041 insertions(+), 16041 deletions(-)\n\nreal\t0m2.176s\nuser\t0m2.170s\nsys\t0m0.010s\n\n$ time git diff --quick --stat\n x |34208 ++++++++++++++++++++++++++++++++++----------------------------------\n 1 files changed, 17104 insertions(+), 17104 deletions(-)\n\nreal\t0m0.064s\nuser\t0m0.060s\nsys\t0m0.010s\n\n---\n builtin/blame.c      |    2 +-\n builtin/merge-file.c |    2 +-\n builtin/merge-tree.c |    2 +-\n builtin/rerere.c     |    2 +-\n combine-diff.c       |    2 +-\n diff.c               |   12 +++++++-----\n merge-file.c         |    2 +-\n xdiff/xdiff.h        |    2 +-\n xdiff/xdiffi.c       |    2 +-\n 9 files changed, 15 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex fc15863..8deeee1 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -39,7 +39,7 @@ static int show_root;\n static int reverse;\n static int blank_boundary;\n static int incremental;\n-static int xdl_opts = XDF_NEED_MINIMAL;\n+static int xdl_opts;\n \n static enum date_mode blame_date_mode = DATE_ISO8601;\n static size_t blame_date_width;\ndiff --git a/builtin/merge-file.c b/builtin/merge-file.c\nindex 610849a..b8e9e5b 100644\n--- a/builtin/merge-file.c\n+++ b/builtin/merge-file.c\n@@ -25,7 +25,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n \tconst char *names[3] = { NULL, NULL, NULL };\n \tmmfile_t mmfs[3];\n \tmmbuffer_t result = {NULL, 0};\n-\txmparam_t xmp = {{XDF_NEED_MINIMAL}};\n+\txmparam_t xmp = {{0}};\n \tint ret = 0, i = 0, to_stdout = 0;\n \tint quiet = 0;\n \tint nongit;\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex a4a4f2c..fc00d79 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -106,7 +106,7 @@ static void show_diff(struct merge_list *entry)\n \txdemitconf_t xecfg;\n \txdemitcb_t ecb;\n \n-\txpp.flags = XDF_NEED_MINIMAL;\n+\txpp.flags = 0;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \txecfg.ctxlen = 3;\n \tecb.outf = show_outf;\ndiff --git a/builtin/rerere.c b/builtin/rerere.c\nindex 34f9ace..0048f9e 100644\n--- a/builtin/rerere.c\n+++ b/builtin/rerere.c\n@@ -89,7 +89,7 @@ static int diff_two(const char *file1, const char *label1,\n \tprintf(\"--- a/%s\\n+++ b/%s\\n\", label1, label2);\n \tfflush(stdout);\n \tmemset(&xpp, 0, sizeof(xpp));\n-\txpp.flags = XDF_NEED_MINIMAL;\n+\txpp.flags = 0;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \txecfg.ctxlen = 3;\n \tecb.outf = outf;\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 6162691..29779be 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -221,7 +221,7 @@ static void combine_diff(const unsigned char *parent, unsigned int mode,\n \tparent_file.ptr = grab_blob(parent, mode, &sz);\n \tparent_file.size = sz;\n \tmemset(&xpp, 0, sizeof(xpp));\n-\txpp.flags = XDF_NEED_MINIMAL;\n+\txpp.flags = 0;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \tmemset(&state, 0, sizeof(state));\n \tstate.nmask = nmask;\ndiff --git a/diff.c b/diff.c\nindex a1bf1e9..e32b47b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -718,7 +718,7 @@ static void diff_words_show(struct diff_words_data *diff_words)\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \tdiff_words_fill(&diff_words->minus, &minus, diff_words->word_regex);\n \tdiff_words_fill(&diff_words->plus, &plus, diff_words->word_regex);\n-\txpp.flags = XDF_NEED_MINIMAL;\n+\txpp.flags = 0;\n \t/* as only the hunk header will be parsed, we need a 0-context */\n \txecfg.ctxlen = 0;\n \txdi_diff_outf(&minus, &plus, fn_out_diff_words_aux, diff_words,\n@@ -1744,7 +1744,7 @@ static void builtin_diff(const char *name_a,\n \t\t\tcheck_blank_at_eof(&mf1, &mf2, &ecbdata);\n \t\tecbdata.file = o->file;\n \t\tecbdata.header = header.len ? &header : NULL;\n-\t\txpp.flags = XDF_NEED_MINIMAL | o->xdl_opts;\n+\t\txpp.flags = o->xdl_opts;\n \t\txecfg.ctxlen = o->context;\n \t\txecfg.interhunkctxlen = o->interhunkcontext;\n \t\txecfg.flags = XDL_EMIT_FUNCNAMES;\n@@ -1834,7 +1834,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \n \t\tmemset(&xpp, 0, sizeof(xpp));\n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n-\t\txpp.flags = XDF_NEED_MINIMAL | o->xdl_opts;\n+\t\txpp.flags = o->xdl_opts;\n \t\txdi_diff_outf(&mf1, &mf2, diffstat_consume, diffstat,\n \t\t\t      &xpp, &xecfg, &ecb);\n \t}\n@@ -1883,7 +1883,7 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,\n \t\tmemset(&xpp, 0, sizeof(xpp));\n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n \t\txecfg.ctxlen = 1; /* at least one context line */\n-\t\txpp.flags = XDF_NEED_MINIMAL;\n+\t\txpp.flags = 0;\n \t\txdi_diff_outf(&mf1, &mf2, checkdiff_consume, &data,\n \t\t\t      &xpp, &xecfg, &ecb);\n \n@@ -2816,6 +2816,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \t\tDIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);\n \telse if (!strcmp(arg, \"--patience\"))\n \t\tDIFF_XDL_SET(options, PATIENCE_DIFF);\n+\telse if (!strcmp(arg, \"--quick\"))\n+\t\tDIFF_XDL_SET(options, QUICK);\n \n \t/* flags options */\n \telse if (!strcmp(arg, \"--binary\")) {\n@@ -3438,7 +3440,7 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)\n \t\t\t\t\tlen2, p->two->path);\n \t\tgit_SHA1_Update(&ctx, buffer, len1);\n \n-\t\txpp.flags = XDF_NEED_MINIMAL;\n+\t\txpp.flags = 0;\n \t\txecfg.ctxlen = 3;\n \t\txecfg.flags = XDL_EMIT_FUNCNAMES;\n \t\txdi_diff_outf(&mf1, &mf2, patch_id_consume, &data,\ndiff --git a/merge-file.c b/merge-file.c\nindex c336c93..db4d0d5 100644\n--- a/merge-file.c\n+++ b/merge-file.c\n@@ -66,7 +66,7 @@ static int generate_common_file(mmfile_t *res, mmfile_t *f1, mmfile_t *f2)\n \txdemitcb_t ecb;\n \n \tmemset(&xpp, 0, sizeof(xpp));\n-\txpp.flags = XDF_NEED_MINIMAL;\n+\txpp.flags = 0;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \txecfg.ctxlen = 3;\n \txecfg.flags = XDL_EMIT_COMMON;\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex 711048e..3b8a962 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -28,7 +28,7 @@ extern \"C\" {\n #endif /* #ifdef __cplusplus */\n \n \n-#define XDF_NEED_MINIMAL (1 << 1)\n+#define XDF_QUICK (1 << 1)\n #define XDF_IGNORE_WHITESPACE (1 << 2)\n #define XDF_IGNORE_WHITESPACE_CHANGE (1 << 3)\n #define XDF_IGNORE_WHITESPACE_AT_EOL (1 << 4)\ndiff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\nindex da67c04..1f688cf 100644\n--- a/xdiff/xdiffi.c\n+++ b/xdiff/xdiffi.c\n@@ -367,7 +367,7 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,\n \tdd2.rindex = xe->xdf2.rindex;\n \n \tif (xdl_recs_cmp(&dd1, 0, dd1.nrec, &dd2, 0, dd2.nrec,\n-\t\t\t kvdf, kvdb, (xpp->flags & XDF_NEED_MINIMAL) != 0, &xenv) < 0) {\n+\t\t\t kvdf, kvdb, (xpp->flags & XDF_QUICK) == 0, &xenv) < 0) {\n \n \t\txdl_free(kvd);\n \t\txdl_free_env(xe);\n"},{"id":"139761","messageId":"7vpr1y2eev.fsf@alter.siamese.dyndns.org","threadId":"23220","inReplyTo":"4BC9D928.50909@lsrfire.ath.cx","subject":"Re: git diff too slow for a file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-04-17T17:10:32Z","receivedAt":"2010-04-17T17:10:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\nRené Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> Am 29.03.2010 03:42, schrieb SungHyun Nam:\n>>  ...\n>> Well, though the files are ascii file, they includes a random\n>> hexa-decimal datas, so that I don't interest the diff result at\n>> all.\n> ...\n> The following patch is not meant for inclusion, but rather to start a\n> dicussion.  Is XDF_NEED_MINIMAL a good default to have?\n\nThis is a very valid question to ask.  The choice of the default was done\nwithout any benchmarking nor analysis on performance impact at all.\n\nWhat we should do next would be to:\n\n - see how much performance impact we have been getting from more normal\n   set of files (say, \"git log -p\" in the kernel archive) by our use of\n   MINIMAL;  I suspect that git.git itself is too small to observe any\n   meaningful difference.  We already _know_ that MINIMAL is more\n   expensive, so this is not very important, but it would be good to\n   know.\n\n - inspect the difference of the quality of output for not using MINIMAL,\n   again for more normal set of files.  We know that the quality does not\n   matter for pathological cases like the one in this thread --- the user\n   is not even \"interested in the diff result at all\".\n\nThanks for starting this.\n"},{"id":"139824","messageId":"4BCB48E5.9090303@lsrfire.ath.cx","threadId":"23220","inReplyTo":"7vpr1y2eev.fsf@alter.siamese.dyndns.org","subject":"Re: git diff too slow for a file","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-04-18T18:01:09Z","receivedAt":"2010-04-18T18:01:09Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 17.04.2010 19:10, schrieb Junio C Hamano:\n> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n> \n> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n> \n>> Am 29.03.2010 03:42, schrieb SungHyun Nam:\n>>>  ...\n>>> Well, though the files are ascii file, they includes a random\n>>> hexa-decimal datas, so that I don't interest the diff result at\n>>> all.\n>> ...\n>> The following patch is not meant for inclusion, but rather to start a\n>> dicussion.  Is XDF_NEED_MINIMAL a good default to have?\n> \n> This is a very valid question to ask.  The choice of the default was done\n> without any benchmarking nor analysis on performance impact at all.\n> \n> What we should do next would be to:\n> \n>  - see how much performance impact we have been getting from more normal\n>    set of files (say, \"git log -p\" in the kernel archive) by our use of\n>    MINIMAL;  I suspect that git.git itself is too small to observe any\n>    meaningful difference.  We already _know_ that MINIMAL is more\n>    expensive, so this is not very important, but it would be good to\n>    know.\n> \n>  - inspect the difference of the quality of output for not using MINIMAL,\n>    again for more normal set of files.  We know that the quality does not\n>    matter for pathological cases like the one in this thread --- the user\n>    is not even \"interested in the diff result at all\".\n\nTo see the what difference it makes to the output, I did the following:\n\n\tgit rev-list --no-merges HEAD |\n\twhile read commit\n\tdo\n\t\ta=$(git show $commit | md5sum)\n\t\tb=$(git show --quick $commit | md5sum)\n\t\ttest \"$a\" = \"$b\" || echo $commit\n\tdone\n\nFor git, it reports those 2 out of 18095 total commits as being\ndifferent with and without XDF_NEED_MINIMAL:\n\n  f522c9b5  717b8311\n\nFor Linux, these 161 out of 178107 commits are affected:\n\n  90d49b4f  83f3c715  3b5dd52a  e97bd974  4e092d11  96b3c83d  4c96e893\n  1210db95  ca97b838  52bbe3c7  a3a4f7e1  de0710aa  8e730c15  b54f78a8\n  71034ba8  120a5d0d  1532ecea  0a18d7b5  3d0f8bc7  35c1b462  0f29f587\n  2cf71d2e  4d295db0  4953550a  c0a5962f  5176fae4  51f94a7b  5d4f98a2\n  cb6492e4  4f774513  da0436e9  3772a991  20b09c29  dd4969a8  b4a4d568\n  a7fefd10  d93f87c8  9b7895ef  a72bdb1c  96b8e145  b960074f  56bec294\n  f74df6fb  89cb7e7f  e8324357  5ef3041e  833dfbe7  65f9f619  02227c28\n  56afef56  5db8dcc9  6261ab3a  deee7c81  e7594072  2b82032c  f1dc5600\n  060ae855  5503ac56  54168ed7  e2d1d6c0  1a40e23b  d1310b2e  9c6bd790\n  8feceb67  9ac19a90  60f8b39c  ef1a628d  95b86a9a  0391c828  a7707adf\n  3f038d80  f5b728a1  5915eb53  287ac01a  c41f8cbd  b497549a  6802e340\n  f8d79e79  34f80b04  c18487ee  c9df406f  03718234  d10c2e46  584fffc8\n  624d5c51  6446a860  f1410647  1c45607a  b21a15f6  0e078e2f  8e09f215\n  99ca4e58  8bf5e5ca  402aa76a  195a4ef6  a9de9248  20510f2f  ae0b78d0\n  51219358  1a4f550a  b2c258fb  e2ebc74d  27c868c2  c3a2f0df  8e18257d\n  05ffdd7b  328d5752  92d7f7b0  39279cc3  4b19fcc3  f23a06f0  7699acd1\n  a1005012  eea221ce  dcd0538f  48b4554a  56b6aeb0  e05d723f  3cee5a60\n  4d0b4af9  9e89dde2  483dfdb6  02f1175c  a8dea4ec  28a6d671  933a27d3\n  572d3138  1450e6be  4ac4360b  9c7f852e  16a53ecc  5bb0b55a  2722971c\n  6a2900b6  c752666c  5c04a7b8  c92f222e  a966f3e7  3ebc284d  a1a5ea70\n  df694daa  7a88488b  afbf30a2  b095c381  ea2b26e0  12d30d89  09af7b44\n  5e83d430  37448f7d  544393fe  d203a7ec  73a25462  bd4c625c  330a115a\n  22e2c507  e9edcee0  303b86d9  47b5d69c  2d7edb92  cb624029  f4f051eb\n\nI have briefly looked at a few of them.  They were big and not obvious\nwith or without XDF_NEED_MINIMAL, but the flag clearly helped to cut\nthem down a bit.\n\n\nXDF_NEED_MINIMAL doesn't seem to affect the overall runtime for the\nLinux repo:\n\n\ttime git log --stat HEAD >/dev/null\n\n\treal\t4m37.378s\n\tuser\t4m28.070s\n\tsys\t0m9.310s\n\n\ttime ../git/git log --stat --quick >/dev/null\n\n\treal\t4m37.239s\n\tuser\t4m26.590s\n\tsys\t0m10.620s\n\nThe difference between the times for git's own repo are in the noise, too.\n"},{"id":"139860","messageId":"4BCBA725.2020307@gmail.com","threadId":"23220","inReplyTo":"4BC9D928.50909@lsrfire.ath.cx","subject":"Re: git diff too slow for a file","fromName":"SungHyun Nam","fromEmail":"goweol@gmail.com","sentAt":"2010-04-19T00:43:17Z","receivedAt":"2010-04-19T00:43:17Z","isPatch":false,"sender":{"key":"goweol@gmail.com","avatar":null},"body":"René Scharfe wrote:\n> Am 29.03.2010 03:42, schrieb SungHyun Nam:\n>> Hello,\n>>\n>> If I run a attached script for bunzipped attached files, I get:\n>> (To reduce size, I removed many lines and bzipped.)\n>>\n>>      $ ./mk.sh\n>>      time diff -u x3 x4>/dev/null 2>&1\n>>\n>>      real    0m0.011s\n>>      user    0m0.000s\n>>      sys    0m0.010s\n>>\n>>      time git diff>/dev/null 2>&1\n>>\n>>      real    0m0.193s\n>>      user    0m0.190s\n>>      sys    0m0.000s\n>>\n>>      $ git version\n>>      git version 1.7.0.2.273.gc2413\n>>\n>>      $ diff --version\n>>      diff (GNU diffutils) 2.8.1\n>>      ...\n>>\n>> Well, though the files are ascii file, they includes a random\n>> hexa-decimal datas, so that I don't interest the diff result at\n>> all.  But the real problem is 'rebasing took so long if the file\n>> was changed'.  Because the git tree includes several such a file,\n>> if they changed, rebase took some miniutes for every branch.\n>> Such a branch includes a few lines of changes for a C source file,\n>> though.  Now I'm waiting an hour to finish rebasing all the\n>> branches and yet a rebasing script is running... :-(\n>\n> I can reproduce it; I concatenated your example files five times to get\n> meaningful timings (x1 = five times x3, x2 = five times x4).\n>\n> The difference between GNU diff and git diff is that the latter is trying\n> hard to minimize the size of the diff.  Each user of the xdiff library in\n> git turns on the XDF_NEED_MINIMAL flag, which makes it very expensive\n> (specifically the function xdl_split()).\n>\n> The following patch is not meant for inclusion, but rather to start a\n> dicussion.  Is XDF_NEED_MINIMAL a good default to have?\n>\n> The patch removes XDF_NEED_MINIMAL and replaces it with XDF_QUICK, with\n> reversed meaning.  XDF_QUICK is only set if the new option --quick is\n> given, so without it the old behaviour is retained.  Some numbers:\n\nThe patch is great for me.  Thanks!\n\nAdded 'time git diff --quick' to the mk.sh and ran with a\noriginal file (about 180000 lines):\n\n     $ ./mk.sh\n     time diff -u x3 x4 >/dev/null 2>&1\n\n     real\t0m0.794s\n     user\t0m0.720s\n     sys\t0m0.010s\n\n     time git diff >/dev/null 2>&1\n\n     real\t0m44.687s\n     user\t0m44.670s\n     sys\t0m0.020s\n\n     time git diff --quick >/dev/null 2>&1\n\n     real\t0m1.853s\n     user\t0m1.840s\n     sys\t0m0.010s\n\nThanks!\nnamsh\n"},{"id":"139954","messageId":"7vd3xuinbe.fsf@alter.siamese.dyndns.org","threadId":"23220","inReplyTo":"4BCB48E5.9090303@lsrfire.ath.cx","subject":"Re: git diff too slow for a file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-04-20T07:40:37Z","receivedAt":"2010-04-20T07:40:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> For Linux, these 161 out of 178107 commits are affected:\n>\n>   90d49b4f  83f3c715  3b5dd52a  e97bd974  4e092d11  96b3c83d  4c96e893\n>   ...\n>   22e2c507  e9edcee0  303b86d9  47b5d69c  2d7edb92  cb624029  f4f051eb\n>\n> I have briefly looked at a few of them.  They were big and not obvious\n> with or without XDF_NEED_MINIMAL, but the flag clearly helped to cut\n> them down a bit.\n\nThanks.\n\nI am getting the same impression after staring some output.\n\nProbably we should at least try to get rid of the use of MINIMAL\nimmediately after 1.7.1 and if nobody finds large discrepancies, aim to\nship 1.7.2 (and possibly 1.7.1.1) without even --quick/--slow options.\n\nI expect that there will also be some differences in the blame output.\n"},{"id":"139996","messageId":"4BCE1983.4020009@lsrfire.ath.cx","threadId":"23220","inReplyTo":"7vd3xuinbe.fsf@alter.siamese.dyndns.org","subject":"Re: git diff too slow for a file","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-04-20T21:15:47Z","receivedAt":"2010-04-20T21:15:47Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 20.04.2010 09:40, schrieb Junio C Hamano:\n> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n> \n>> For Linux, these 161 out of 178107 commits are affected:\n>>\n>>   90d49b4f  83f3c715  3b5dd52a  e97bd974  4e092d11  96b3c83d  4c96e893\n>>   ...\n>>   22e2c507  e9edcee0  303b86d9  47b5d69c  2d7edb92  cb624029  f4f051eb\n>>\n>> I have briefly looked at a few of them.  They were big and not obvious\n>> with or without XDF_NEED_MINIMAL, but the flag clearly helped to cut\n>> them down a bit.\n> \n> Thanks.\n> \n> I am getting the same impression after staring some output.\n> \n> Probably we should at least try to get rid of the use of MINIMAL\n> immediately after 1.7.1 and if nobody finds large discrepancies, aim to\n> ship 1.7.2 (and possibly 1.7.1.1) without even --quick/--slow options.\n\nTurning XDF_NEED_MINIMAL off by default looks like the sane thing to do\nin order to help the fringe cases without hurting the normal ones.\n\nA --slow/--minimal/--try-harder option for git diff could come in handy\nfor longer patches, though.  GNU diff has it, too (-d/--minimal).\n\n> I expect that there will also be some differences in the blame output.\n\nI haven't looked at the impact on blame, but additionally patch IDs are\ngoing to change (for those patches where XDF_NEED_MINIMAL makes a\ndifference).  Are they stored somewhere?  Do we need to worry about them?\n\nRené\n"},{"id":"140002","messageId":"7vzl0xcyf8.fsf@alter.siamese.dyndns.org","threadId":"23220","inReplyTo":"4BCE1983.4020009@lsrfire.ath.cx","subject":"Re: git diff too slow for a file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-04-21T02:49:31Z","receivedAt":"2010-04-21T02:49:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> Turning XDF_NEED_MINIMAL off by default looks like the sane thing to do\n> in order to help the fringe cases without hurting the normal ones.\n>\n> A --slow/--minimal/--try-harder option for git diff could come in handy\n> for longer patches, though.  GNU diff has it, too (-d/--minimal).\n>\n>> I expect that there will also be some differences in the blame output.\n>\n> I haven't looked at the impact on blame, but additionally patch IDs are\n> going to change (for those patches where XDF_NEED_MINIMAL makes a\n> difference).  Are they stored somewhere?  Do we need to worry about them?\n\nPatch-IDs are purely transient inside core-git as far as I know, but I\nwouldn't surprised if people keep database of patch-IDs and corresponding\ncommits to somehow speed up change look-ups.\n\nTheoretically, conflict IDs used to index the rerere database would also\nbe affected, but I don't think it is such a huge issue.\n"},{"id":"140765","messageId":"4BDD7869.10701@lsrfire.ath.cx","threadId":"23220","inReplyTo":"7vd3xuinbe.fsf@alter.siamese.dyndns.org","subject":"Re: git diff too slow for a file","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-05-02T13:04:41Z","receivedAt":"2010-05-02T13:04:41Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 20.04.2010 09:40, schrieb Junio C Hamano:\n> Probably we should at least try to get rid of the use of MINIMAL\n> immediately after 1.7.1 and if nobody finds large discrepancies, aim to\n> ship 1.7.2 (and possibly 1.7.1.1) without even --quick/--slow options.\n\n-- >8 --\nEver since the xdiff library had been introduced to git, all its callers\nhave used the flag XDF_NEED_MINIMAL.  It makes sure that the smallest\npossible diff is produced, but that takes quite some time if there are\nlots of differences that can be expressed in multiple ways.\n\nThis flag makes a difference for only 0.1% of the non-merge commits in\nthe git repo of Linux, both in terms of diff size and execution time.\nThe patches there are mostly nice and small.\n\nSungHyun Nam however reported a case in a different repo where a diff\ntook more than 20 times longer to generate with XDF_NEED_MINIMAL than\nwithout.  Rebasing became really slow.\n\nThis patch removes this flag from all callers.  The default of xdiff is\nsaner because it has minimal to no impact in the normal case of small\ndiffs and doesn't incur that much of a speed penalty for large ones.\n\nA follow-up patch may introduce a command line option to set the flag if\nthe user needs it, similar to GNU diff's -d/--minimal.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n builtin/blame.c      |    2 +-\n builtin/merge-file.c |    2 +-\n builtin/merge-tree.c |    2 +-\n builtin/rerere.c     |    2 +-\n combine-diff.c       |    2 +-\n diff.c               |   10 +++++-----\n merge-file.c         |    2 +-\n 7 files changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex fc15863..8deeee1 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -39,7 +39,7 @@ static int show_root;\n static int reverse;\n static int blank_boundary;\n static int incremental;\n-static int xdl_opts = XDF_NEED_MINIMAL;\n+static int xdl_opts;\n \n static enum date_mode blame_date_mode = DATE_ISO8601;\n static size_t blame_date_width;\ndiff --git a/builtin/merge-file.c b/builtin/merge-file.c\nindex 610849a..b8e9e5b 100644\n--- a/builtin/merge-file.c\n+++ b/builtin/merge-file.c\n@@ -25,7 +25,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n \tconst char *names[3] = { NULL, NULL, NULL };\n \tmmfile_t mmfs[3];\n \tmmbuffer_t result = {NULL, 0};\n-\txmparam_t xmp = {{XDF_NEED_MINIMAL}};\n+\txmparam_t xmp = {{0}};\n \tint ret = 0, i = 0, to_stdout = 0;\n \tint quiet = 0;\n \tint nongit;\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex a4a4f2c..fc00d79 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -106,7 +106,7 @@ static void show_diff(struct merge_list *entry)\n \txdemitconf_t xecfg;\n \txdemitcb_t ecb;\n \n-\txpp.flags = XDF_NEED_MINIMAL;\n+\txpp.flags = 0;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \txecfg.ctxlen = 3;\n \tecb.outf = show_outf;\ndiff --git a/builtin/rerere.c b/builtin/rerere.c\nindex 34f9ace..0048f9e 100644\n--- a/builtin/rerere.c\n+++ b/builtin/rerere.c\n@@ -89,7 +89,7 @@ static int diff_two(const char *file1, const char *label1,\n \tprintf(\"--- a/%s\\n+++ b/%s\\n\", label1, label2);\n \tfflush(stdout);\n \tmemset(&xpp, 0, sizeof(xpp));\n-\txpp.flags = XDF_NEED_MINIMAL;\n+\txpp.flags = 0;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \txecfg.ctxlen = 3;\n \tecb.outf = outf;\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 6162691..29779be 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -221,7 +221,7 @@ static void combine_diff(const unsigned char *parent, unsigned int mode,\n \tparent_file.ptr = grab_blob(parent, mode, &sz);\n \tparent_file.size = sz;\n \tmemset(&xpp, 0, sizeof(xpp));\n-\txpp.flags = XDF_NEED_MINIMAL;\n+\txpp.flags = 0;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \tmemset(&state, 0, sizeof(state));\n \tstate.nmask = nmask;\ndiff --git a/diff.c b/diff.c\nindex a1bf1e9..29e608b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -718,7 +718,7 @@ static void diff_words_show(struct diff_words_data *diff_words)\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \tdiff_words_fill(&diff_words->minus, &minus, diff_words->word_regex);\n \tdiff_words_fill(&diff_words->plus, &plus, diff_words->word_regex);\n-\txpp.flags = XDF_NEED_MINIMAL;\n+\txpp.flags = 0;\n \t/* as only the hunk header will be parsed, we need a 0-context */\n \txecfg.ctxlen = 0;\n \txdi_diff_outf(&minus, &plus, fn_out_diff_words_aux, diff_words,\n@@ -1744,7 +1744,7 @@ static void builtin_diff(const char *name_a,\n \t\t\tcheck_blank_at_eof(&mf1, &mf2, &ecbdata);\n \t\tecbdata.file = o->file;\n \t\tecbdata.header = header.len ? &header : NULL;\n-\t\txpp.flags = XDF_NEED_MINIMAL | o->xdl_opts;\n+\t\txpp.flags = o->xdl_opts;\n \t\txecfg.ctxlen = o->context;\n \t\txecfg.interhunkctxlen = o->interhunkcontext;\n \t\txecfg.flags = XDL_EMIT_FUNCNAMES;\n@@ -1834,7 +1834,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \n \t\tmemset(&xpp, 0, sizeof(xpp));\n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n-\t\txpp.flags = XDF_NEED_MINIMAL | o->xdl_opts;\n+\t\txpp.flags = o->xdl_opts;\n \t\txdi_diff_outf(&mf1, &mf2, diffstat_consume, diffstat,\n \t\t\t      &xpp, &xecfg, &ecb);\n \t}\n@@ -1883,7 +1883,7 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,\n \t\tmemset(&xpp, 0, sizeof(xpp));\n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n \t\txecfg.ctxlen = 1; /* at least one context line */\n-\t\txpp.flags = XDF_NEED_MINIMAL;\n+\t\txpp.flags = 0;\n \t\txdi_diff_outf(&mf1, &mf2, checkdiff_consume, &data,\n \t\t\t      &xpp, &xecfg, &ecb);\n \n@@ -3438,7 +3438,7 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)\n \t\t\t\t\tlen2, p->two->path);\n \t\tgit_SHA1_Update(&ctx, buffer, len1);\n \n-\t\txpp.flags = XDF_NEED_MINIMAL;\n+\t\txpp.flags = 0;\n \t\txecfg.ctxlen = 3;\n \t\txecfg.flags = XDL_EMIT_FUNCNAMES;\n \t\txdi_diff_outf(&mf1, &mf2, patch_id_consume, &data,\ndiff --git a/merge-file.c b/merge-file.c\nindex c336c93..db4d0d5 100644\n--- a/merge-file.c\n+++ b/merge-file.c\n@@ -66,7 +66,7 @@ static int generate_common_file(mmfile_t *res, mmfile_t *f1, mmfile_t *f2)\n \txdemitcb_t ecb;\n \n \tmemset(&xpp, 0, sizeof(xpp));\n-\txpp.flags = XDF_NEED_MINIMAL;\n+\txpp.flags = 0;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \txecfg.ctxlen = 3;\n \txecfg.flags = XDL_EMIT_COMMON;\n-- \n1.7.1\n"},{"id":"140775","messageId":"7v1vduwd8j.fsf@alter.siamese.dyndns.org","threadId":"23220","inReplyTo":"4BDD7869.10701@lsrfire.ath.cx","subject":"Re: git diff too slow for a file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-05-02T15:10:52Z","receivedAt":"2010-05-02T15:10:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> Ever since the xdiff library had been introduced to git, all its callers\n> have used the flag XDF_NEED_MINIMAL.  It makes sure that the smallest\n> possible diff is produced, but that takes quite some time if there are\n> lots of differences that can be expressed in multiple ways.\n>\n> This flag makes a difference for only 0.1% of the non-merge commits in\n> the git repo of Linux, both in terms of diff size and execution time.\n> The patches there are mostly nice and small.\n>\n> SungHyun Nam however reported a case in a different repo where a diff\n> took more than 20 times longer to generate with XDF_NEED_MINIMAL than\n> without.  Rebasing became really slow.\n>\n> This patch removes this flag from all callers.  The default of xdiff is\n> saner because it has minimal to no impact in the normal case of small\n> diffs and doesn't incur that much of a speed penalty for large ones.\n\nThanks, will queue.\n\n> diff --git a/merge-file.c b/merge-file.c\n> index c336c93..db4d0d5 100644\n> --- a/merge-file.c\n> +++ b/merge-file.c\n> @@ -66,7 +66,7 @@ static int generate_common_file(mmfile_t *res, mmfile_t *f1, mmfile_t *f2)\n>  \txdemitcb_t ecb;\n>  \n>  \tmemset(&xpp, 0, sizeof(xpp));\n> -\txpp.flags = XDF_NEED_MINIMAL;\n> +\txpp.flags = 0;\n>  \tmemset(&xecfg, 0, sizeof(xecfg));\n>  \txecfg.ctxlen = 3;\n>  \txecfg.flags = XDL_EMIT_COMMON;\n\nWhen I wrote the message you are responding to, I tried to decide which is\nbetter, to replace the assigned value like your patch does, or to remove\nthe assignment altogether as we have memset(&xpp, 0, sizeof(xpp)).  And I\nwas somewhat torn.\n\nWhile it expresses what the patch wants to do a lot clearer (and it also\nmarks the places the \"later patch\" needs to touch), the resulting code\nbecomes slightly harder to read, because future readers of the code are\nleft with an obvious \"why do we assign 0 after clearing the whole thing?\nis there anything subtle going on?\" unanswered.\n"},{"id":"140932","messageId":"4BE080AF.2030604@lsrfire.ath.cx","threadId":"23220","inReplyTo":"7v1vduwd8j.fsf@alter.siamese.dyndns.org","subject":"Re: git diff too slow for a file","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2010-05-04T20:16:47Z","receivedAt":"2010-05-04T20:16:47Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 02.05.2010 17:10, schrieb Junio C Hamano:\n> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n>> diff --git a/merge-file.c b/merge-file.c\n>> index c336c93..db4d0d5 100644\n>> --- a/merge-file.c\n>> +++ b/merge-file.c\n>> @@ -66,7 +66,7 @@ static int generate_common_file(mmfile_t *res, mmfile_t *f1, mmfile_t *f2)\n>>  \txdemitcb_t ecb;\n>>  \n>>  \tmemset(&xpp, 0, sizeof(xpp));\n>> -\txpp.flags = XDF_NEED_MINIMAL;\n>> +\txpp.flags = 0;\n>>  \tmemset(&xecfg, 0, sizeof(xecfg));\n>>  \txecfg.ctxlen = 3;\n>>  \txecfg.flags = XDL_EMIT_COMMON;\n> \n> When I wrote the message you are responding to, I tried to decide which is\n> better, to replace the assigned value like your patch does, or to remove\n> the assignment altogether as we have memset(&xpp, 0, sizeof(xpp)).  And I\n> was somewhat torn.\n> \n> While it expresses what the patch wants to do a lot clearer (and it also\n> marks the places the \"later patch\" needs to touch), the resulting code\n> becomes slightly harder to read, because future readers of the code are\n> left with an obvious \"why do we assign 0 after clearing the whole thing?\n> is there anything subtle going on?\" unanswered.\n\nWell, I didn't do that because of a mix of laziness and caution.  A\nmechanical replacement is much less likely to introduce bugs..\n\nBut when I take a closer look at the surrounding code, I can't help but\nask if the flags really have be passed in such a complicated way.\n\nHow about the following, which makes xdi_diff*() take a simple flag\nparameter instead, moving the code to handle xpparam_t into\nxdiff-interface.c, which seems to be the proper place for it?\n\nRené\n\n\n---\n builtin/blame.c      |   10 ++--------\n builtin/merge-tree.c |    4 +---\n builtin/rerere.c     |    5 +----\n combine-diff.c       |    5 +----\n diff.c               |   25 +++++--------------------\n merge-file.c         |    5 +----\n xdiff-interface.c    |   18 +++++++++++-------\n xdiff-interface.h    |    8 ++++----\n 8 files changed, 26 insertions(+), 54 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 8deeee1..1c1c9e4 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -734,7 +734,6 @@ static int pass_blame_to_parent(struct scoreboard *sb,\n \tint last_in_target;\n \tmmfile_t file_p, file_o;\n \tstruct blame_chunk_cb_data d = { sb, target, parent, 0, 0 };\n-\txpparam_t xpp;\n \txdemitconf_t xecfg;\n \n \tlast_in_target = find_last_in_target(sb, target);\n@@ -745,11 +744,9 @@ static int pass_blame_to_parent(struct scoreboard *sb,\n \tfill_origin_blob(target, &file_o);\n \tnum_get_patch++;\n \n-\tmemset(&xpp, 0, sizeof(xpp));\n-\txpp.flags = xdl_opts;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \txecfg.ctxlen = 0;\n-\txdi_diff_hunks(&file_p, &file_o, blame_chunk_cb, &d, &xpp, &xecfg);\n+\txdi_diff_hunks(&file_p, &file_o, blame_chunk_cb, &d, &xecfg, xdl_opts);\n \t/* The rest (i.e. anything after tlno) are the same as the parent */\n \tblame_chunk(sb, d.tlno, d.plno, last_in_target, target, parent);\n \n@@ -876,7 +873,6 @@ static void find_copy_in_blob(struct scoreboard *sb,\n \tint cnt;\n \tmmfile_t file_o;\n \tstruct handle_split_cb_data d = { sb, ent, parent, split, 0, 0 };\n-\txpparam_t xpp;\n \txdemitconf_t xecfg;\n \n \t/*\n@@ -896,12 +892,10 @@ static void find_copy_in_blob(struct scoreboard *sb,\n \t * file_o is a part of final image we are annotating.\n \t * file_p partially may match that image.\n \t */\n-\tmemset(&xpp, 0, sizeof(xpp));\n-\txpp.flags = xdl_opts;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \txecfg.ctxlen = 1;\n \tmemset(split, 0, sizeof(struct blame_entry [3]));\n-\txdi_diff_hunks(file_p, &file_o, handle_split_cb, &d, &xpp, &xecfg);\n+\txdi_diff_hunks(file_p, &file_o, handle_split_cb, &d, &xecfg, xdl_opts);\n \t/* remainder, if any, all match the preimage */\n \thandle_split(sb, ent, d.tlno, d.plno, ent->num_lines, parent, split);\n }\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex fc00d79..d95cd1c 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -102,11 +102,9 @@ static void show_diff(struct merge_list *entry)\n {\n \tunsigned long size;\n \tmmfile_t src, dst;\n-\txpparam_t xpp;\n \txdemitconf_t xecfg;\n \txdemitcb_t ecb;\n \n-\txpp.flags = 0;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \txecfg.ctxlen = 3;\n \tecb.outf = show_outf;\n@@ -120,7 +118,7 @@ static void show_diff(struct merge_list *entry)\n \tif (!dst.ptr)\n \t\tsize = 0;\n \tdst.size = size;\n-\txdi_diff(&src, &dst, &xpp, &xecfg, &ecb);\n+\txdi_diff(&src, &dst, &xecfg, &ecb, 0);\n \tfree(src.ptr);\n \tfree(dst.ptr);\n }\ndiff --git a/builtin/rerere.c b/builtin/rerere.c\nindex 0048f9e..43b75fa 100644\n--- a/builtin/rerere.c\n+++ b/builtin/rerere.c\n@@ -78,7 +78,6 @@ static int outf(void *dummy, mmbuffer_t *ptr, int nbuf)\n static int diff_two(const char *file1, const char *label1,\n \t\tconst char *file2, const char *label2)\n {\n-\txpparam_t xpp;\n \txdemitconf_t xecfg;\n \txdemitcb_t ecb;\n \tmmfile_t minus, plus;\n@@ -88,12 +87,10 @@ static int diff_two(const char *file1, const char *label1,\n \n \tprintf(\"--- a/%s\\n+++ b/%s\\n\", label1, label2);\n \tfflush(stdout);\n-\tmemset(&xpp, 0, sizeof(xpp));\n-\txpp.flags = 0;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \txecfg.ctxlen = 3;\n \tecb.outf = outf;\n-\txdi_diff(&minus, &plus, &xpp, &xecfg, &ecb);\n+\txdi_diff(&minus, &plus, &xecfg, &ecb, 0);\n \n \tfree(minus.ptr);\n \tfree(plus.ptr);\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 29779be..5b29226 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -208,7 +208,6 @@ static void combine_diff(const unsigned char *parent, unsigned int mode,\n {\n \tunsigned int p_lno, lno;\n \tunsigned long nmask = (1UL << n);\n-\txpparam_t xpp;\n \txdemitconf_t xecfg;\n \tmmfile_t parent_file;\n \txdemitcb_t ecb;\n@@ -220,8 +219,6 @@ static void combine_diff(const unsigned char *parent, unsigned int mode,\n \n \tparent_file.ptr = grab_blob(parent, mode, &sz);\n \tparent_file.size = sz;\n-\tmemset(&xpp, 0, sizeof(xpp));\n-\txpp.flags = 0;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \tmemset(&state, 0, sizeof(state));\n \tstate.nmask = nmask;\n@@ -231,7 +228,7 @@ static void combine_diff(const unsigned char *parent, unsigned int mode,\n \tstate.n = n;\n \n \txdi_diff_outf(&parent_file, result_file, consume_line, &state,\n-\t\t      &xpp, &xecfg, &ecb);\n+\t\t      &xecfg, &ecb, 0);\n \tfree(parent_file.ptr);\n \n \t/* Assign line numbers for this parent.\ndiff --git a/diff.c b/diff.c\nindex 29e608b..d95a928 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -698,7 +698,6 @@ static void diff_words_fill(struct diff_words_buffer *buffer, mmfile_t *out,\n /* this executes the word diff on the accumulated buffers */\n static void diff_words_show(struct diff_words_data *diff_words)\n {\n-\txpparam_t xpp;\n \txdemitconf_t xecfg;\n \txdemitcb_t ecb;\n \tmmfile_t minus, plus;\n@@ -714,15 +713,13 @@ static void diff_words_show(struct diff_words_data *diff_words)\n \n \tdiff_words->current_plus = diff_words->plus.text.ptr;\n \n-\tmemset(&xpp, 0, sizeof(xpp));\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \tdiff_words_fill(&diff_words->minus, &minus, diff_words->word_regex);\n \tdiff_words_fill(&diff_words->plus, &plus, diff_words->word_regex);\n-\txpp.flags = 0;\n \t/* as only the hunk header will be parsed, we need a 0-context */\n \txecfg.ctxlen = 0;\n \txdi_diff_outf(&minus, &plus, fn_out_diff_words_aux, diff_words,\n-\t\t      &xpp, &xecfg, &ecb);\n+\t\t      &xecfg, &ecb, 0);\n \tfree(minus.ptr);\n \tfree(plus.ptr);\n \tif (diff_words->current_plus != diff_words->plus.text.ptr +\n@@ -1703,7 +1700,6 @@ static void builtin_diff(const char *name_a,\n \telse {\n \t\t/* Crazy xdl interfaces.. */\n \t\tconst char *diffopts = getenv(\"GIT_DIFF_OPTS\");\n-\t\txpparam_t xpp;\n \t\txdemitconf_t xecfg;\n \t\txdemitcb_t ecb;\n \t\tstruct emit_callback ecbdata;\n@@ -1733,7 +1729,6 @@ static void builtin_diff(const char *name_a,\n \t\tif (!pe)\n \t\t\tpe = diff_funcname_pattern(two);\n \n-\t\tmemset(&xpp, 0, sizeof(xpp));\n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n \t\tmemset(&ecbdata, 0, sizeof(ecbdata));\n \t\tecbdata.label_path = lbl;\n@@ -1744,7 +1739,6 @@ static void builtin_diff(const char *name_a,\n \t\t\tcheck_blank_at_eof(&mf1, &mf2, &ecbdata);\n \t\tecbdata.file = o->file;\n \t\tecbdata.header = header.len ? &header : NULL;\n-\t\txpp.flags = o->xdl_opts;\n \t\txecfg.ctxlen = o->context;\n \t\txecfg.interhunkctxlen = o->interhunkcontext;\n \t\txecfg.flags = XDL_EMIT_FUNCNAMES;\n@@ -1777,7 +1771,7 @@ static void builtin_diff(const char *name_a,\n \t\t\t}\n \t\t}\n \t\txdi_diff_outf(&mf1, &mf2, fn_out_consume, &ecbdata,\n-\t\t\t      &xpp, &xecfg, &ecb);\n+\t\t\t      &xecfg, &ecb, o->xdl_opts);\n \t\tif (DIFF_OPT_TST(o, COLOR_DIFF_WORDS))\n \t\t\tfree_diff_words_data(&ecbdata);\n \t\tif (textconv_one)\n@@ -1828,15 +1822,12 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\tdata->deleted = mf1.size;\n \t} else {\n \t\t/* Crazy xdl interfaces.. */\n-\t\txpparam_t xpp;\n \t\txdemitconf_t xecfg;\n \t\txdemitcb_t ecb;\n \n-\t\tmemset(&xpp, 0, sizeof(xpp));\n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n-\t\txpp.flags = o->xdl_opts;\n \t\txdi_diff_outf(&mf1, &mf2, diffstat_consume, diffstat,\n-\t\t\t      &xpp, &xecfg, &ecb);\n+\t\t\t      &xecfg, &ecb, o->xdl_opts);\n \t}\n \n  free_and_return:\n@@ -1876,16 +1867,13 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,\n \t\tgoto free_and_return;\n \telse {\n \t\t/* Crazy xdl interfaces.. */\n-\t\txpparam_t xpp;\n \t\txdemitconf_t xecfg;\n \t\txdemitcb_t ecb;\n \n-\t\tmemset(&xpp, 0, sizeof(xpp));\n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n \t\txecfg.ctxlen = 1; /* at least one context line */\n-\t\txpp.flags = 0;\n \t\txdi_diff_outf(&mf1, &mf2, checkdiff_consume, &data,\n-\t\t\t      &xpp, &xecfg, &ecb);\n+\t\t\t      &xecfg, &ecb, 0);\n \n \t\tif (data.ws_rule & WS_BLANK_AT_EOF) {\n \t\t\tstruct emit_callback ecbdata;\n@@ -3378,14 +3366,12 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)\n \tdata.ctx = &ctx;\n \n \tfor (i = 0; i < q->nr; i++) {\n-\t\txpparam_t xpp;\n \t\txdemitconf_t xecfg;\n \t\txdemitcb_t ecb;\n \t\tmmfile_t mf1, mf2;\n \t\tstruct diff_filepair *p = q->queue[i];\n \t\tint len1, len2;\n \n-\t\tmemset(&xpp, 0, sizeof(xpp));\n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n \t\tif (p->status == 0)\n \t\t\treturn error(\"internal diff status error\");\n@@ -3438,11 +3424,10 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)\n \t\t\t\t\tlen2, p->two->path);\n \t\tgit_SHA1_Update(&ctx, buffer, len1);\n \n-\t\txpp.flags = 0;\n \t\txecfg.ctxlen = 3;\n \t\txecfg.flags = XDL_EMIT_FUNCNAMES;\n \t\txdi_diff_outf(&mf1, &mf2, patch_id_consume, &data,\n-\t\t\t      &xpp, &xecfg, &ecb);\n+\t\t\t      &xecfg, &ecb, 0);\n \t}\n \n \tgit_SHA1_Final(sha1, &ctx);\ndiff --git a/merge-file.c b/merge-file.c\nindex db4d0d5..6521561 100644\n--- a/merge-file.c\n+++ b/merge-file.c\n@@ -61,12 +61,9 @@ static int generate_common_file(mmfile_t *res, mmfile_t *f1, mmfile_t *f2)\n {\n \tunsigned long size = f1->size < f2->size ? f1->size : f2->size;\n \tvoid *ptr = xmalloc(size);\n-\txpparam_t xpp;\n \txdemitconf_t xecfg;\n \txdemitcb_t ecb;\n \n-\tmemset(&xpp, 0, sizeof(xpp));\n-\txpp.flags = 0;\n \tmemset(&xecfg, 0, sizeof(xecfg));\n \txecfg.ctxlen = 3;\n \txecfg.flags = XDL_EMIT_COMMON;\n@@ -76,7 +73,7 @@ static int generate_common_file(mmfile_t *res, mmfile_t *f1, mmfile_t *f2)\n \tres->size = 0;\n \n \tecb.priv = res;\n-\treturn xdi_diff(f1, f2, &xpp, &xecfg, &ecb);\n+\treturn xdi_diff(f1, f2, &xecfg, &ecb, 0);\n }\n \n void *merge_file(const char *path, struct blob *base, struct blob *our, struct blob *their, unsigned long *size)\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex ca5e3fb..6457a5b 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -126,20 +126,24 @@ static void trim_common_tail(mmfile_t *a, mmfile_t *b, long ctx)\n \tb->size -= trimmed - recovered;\n }\n \n-int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t const *xecfg, xdemitcb_t *xecb)\n+int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xdemitconf_t const *xecfg,\n+\t     xdemitcb_t *xecb, int xpp_flags)\n {\n+\txpparam_t xpp;\n \tmmfile_t a = *mf1;\n \tmmfile_t b = *mf2;\n \n+\tmemset(&xpp, 0, sizeof(xpp));\n+\txpp.flags = xpp_flags;\n+\n \ttrim_common_tail(&a, &b, xecfg->ctxlen);\n \n-\treturn xdl_diff(&a, &b, xpp, xecfg, xecb);\n+\treturn xdl_diff(&a, &b, &xpp, xecfg, xecb);\n }\n \n int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,\n \t\t  xdiff_emit_consume_fn fn, void *consume_callback_data,\n-\t\t  xpparam_t const *xpp,\n-\t\t  xdemitconf_t const *xecfg, xdemitcb_t *xecb)\n+\t\t  xdemitconf_t const *xecfg, xdemitcb_t *xecb, int xpp_flags)\n {\n \tint ret;\n \tstruct xdiff_emit_state state;\n@@ -150,7 +154,7 @@ int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,\n \txecb->outf = xdiff_outf;\n \txecb->priv = &state;\n \tstrbuf_init(&state.remainder, 0);\n-\tret = xdi_diff(mf1, mf2, xpp, xecfg, xecb);\n+\tret = xdi_diff(mf1, mf2, xecfg, xecb, xpp_flags);\n \tstrbuf_release(&state.remainder);\n \treturn ret;\n }\n@@ -185,7 +189,7 @@ static int process_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \n int xdi_diff_hunks(mmfile_t *mf1, mmfile_t *mf2,\n \t\t   xdiff_emit_hunk_consume_fn fn, void *consume_callback_data,\n-\t\t   xpparam_t const *xpp, xdemitconf_t *xecfg)\n+\t\t   xdemitconf_t *xecfg, int xpp_flags)\n {\n \tstruct xdiff_emit_hunk_state state;\n \txdemitcb_t ecb;\n@@ -196,7 +200,7 @@ int xdi_diff_hunks(mmfile_t *mf1, mmfile_t *mf2,\n \tstate.consume_callback_data = consume_callback_data;\n \txecfg->emit_func = (void (*)())process_diff;\n \tecb.priv = &state;\n-\treturn xdi_diff(mf1, mf2, xpp, xecfg, &ecb);\n+\treturn xdi_diff(mf1, mf2, xecfg, &ecb, xpp_flags);\n }\n \n int read_mmfile(mmfile_t *ptr, const char *filename)\ndiff --git a/xdiff-interface.h b/xdiff-interface.h\nindex abba70c..05adeb3 100644\n--- a/xdiff-interface.h\n+++ b/xdiff-interface.h\n@@ -6,14 +6,14 @@\n typedef void (*xdiff_emit_consume_fn)(void *, char *, unsigned long);\n typedef void (*xdiff_emit_hunk_consume_fn)(void *, long, long, long);\n \n-int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdemitconf_t const *xecfg, xdemitcb_t *ecb);\n+int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xdemitconf_t const *xecfg,\n+\t     xdemitcb_t *ecb, int xpp_flags);\n int xdi_diff_outf(mmfile_t *mf1, mmfile_t *mf2,\n \t\t  xdiff_emit_consume_fn fn, void *consume_callback_data,\n-\t\t  xpparam_t const *xpp,\n-\t\t  xdemitconf_t const *xecfg, xdemitcb_t *xecb);\n+\t\t  xdemitconf_t const *xecfg, xdemitcb_t *xecb, int xpp_flags);\n int xdi_diff_hunks(mmfile_t *mf1, mmfile_t *mf2,\n \t\t   xdiff_emit_hunk_consume_fn fn, void *consume_callback_data,\n-\t\t   xpparam_t const *xpp, xdemitconf_t *xecfg);\n+\t\t   xdemitconf_t *xecfg, int xpp_flags);\n int parse_hunk_header(char *line, int len,\n \t\t      int *ob, int *on,\n \t\t      int *nb, int *nn);\n-- \n1.7.1\n"},{"id":"140946","messageId":"7vy6fzl1hl.fsf@alter.siamese.dyndns.org","threadId":"23220","inReplyTo":"4BE080AF.2030604@lsrfire.ath.cx","subject":"Re: git diff too slow for a file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-05-04T22:56:54Z","receivedAt":"2010-05-04T22:56:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> But when I take a closer look at the surrounding code, I can't help but\n> ask if the flags really have be passed in such a complicated way.\n>\n> How about the following, which makes xdi_diff*() take a simple flag\n> parameter instead, moving the code to handle xpparam_t into\n> xdiff-interface.c, which seems to be the proper place for it?\n\nThis looks very sensible.  Your patch doesn't touch xdiff/ proper but only\nthe thin interface layer, so we don't have to worry about deviating from\nthe upstream even further.\n"}]}