{"thread":{"id":"11302","subject":"trim_common_tail bug?","startedAt":"2007-12-15T11:16:22Z","lastAt":"2007-12-20T09:40:24Z","messageCount":30,"participants":["Jeff King","Junio C Hamano","Wincent Colaiuta","Johannes Schindelin","Johannes Sixt","Charles Bailey"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"63246","messageId":"20071215111621.GA8139@coredump.intra.peff.net","threadId":"11302","inReplyTo":null,"subject":"trim_common_tail bug?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-15T11:16:22Z","receivedAt":"2007-12-15T11:16:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Something seems to be not quite right with the trim_common_tail code in\nxdiff-interface.c. The diff I just sent for contrib/completion looks\nfine (as I sent it) when I comment out trim_common_tail. But using\nthe current master, it looks like this:\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 58e0e53..2fd32db 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -291,7 +291,7 @@ __git_commands ()\n \tfor i in $(git help -a|egrep '^ ')\n \tdo\n \t\tcase $i in\n-\t\tadd--interactive) : plumbing;;\n+\t\t*--*)             : plumbing pattern;;\n \t\tapplymbox)        : ask gittus;;\n \t\tapplypatch)       : ask gittus;;\n \t\tarchimport)       : import;;\n@@ -307,5 +307,4 @@ __git_commands ()\n \t\tdiff-index)       : plumbing;;\n \t\tdiff-tree)        : plumbing;;\n \t\tfast-import)      : import;;\n-\t\tfsck-objects)     : plumbing;;\n-\t\tfetch--tool)      : plumbing;;\n\\ No newline at end of file\n+\t\tfsck-objects)     : plumbing;;\n\\ No newline at end of file\n\n-Peff\n"},{"id":"63253","messageId":"20071215155150.GA24810@coredump.intra.peff.net","threadId":"11302","inReplyTo":"20071215111621.GA8139@coredump.intra.peff.net","subject":"Re: trim_common_tail bug?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-15T15:51:50Z","receivedAt":"2007-12-15T15:51:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 15, 2007 at 06:16:21AM -0500, Jeff King wrote:\n\n> Something seems to be not quite right with the trim_common_tail code in\n> xdiff-interface.c. The diff I just sent for contrib/completion looks\n> fine (as I sent it) when I comment out trim_common_tail. But using\n> the current master, it looks like this:\n\nHrm. It feels like there is an off-by-one when recovering the context\nlines; we end the inner loop at the newline, but then we never skip past\nit. So we end up counting only one line total, and even that ends up\nwith a missing newline. Something like this fixes it for me, but\nsomebody please double check.\n\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 700def2..eb60e88 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -124,6 +124,8 @@ static void trim_common_tail(mmfile_t *a, mmfile_t *b, long ctx)\n \tfor (i = 0, recovered = 0; recovered < trimmed && i <= ctx; i++) {\n \t\twhile (recovered < trimmed && ap[recovered] != '\\n')\n \t\t\trecovered++;\n+\t\tif (recovered < trimmed && ap[recovered] == '\\n')\n+\t\t\trecovered++;\n \t}\n \ta->size -= (trimmed - recovered);\n \tb->size -= (trimmed - recovered);\n"},{"id":"63263","messageId":"7vprx7n90t.fsf@gitster.siamese.dyndns.org","threadId":"11302","inReplyTo":"20071215155150.GA24810@coredump.intra.peff.net","subject":"Re: trim_common_tail bug?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-15T17:49:06Z","receivedAt":"2007-12-15T17:49:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> diff --git a/xdiff-interface.c b/xdiff-interface.c\n> index 700def2..eb60e88 100644\n> --- a/xdiff-interface.c\n> +++ b/xdiff-interface.c\n> @@ -124,6 +124,8 @@ static void trim_common_tail(mmfile_t *a, mmfile_t *b, long ctx)\n>  \tfor (i = 0, recovered = 0; recovered < trimmed && i <= ctx; i++) {\n>  \t\twhile (recovered < trimmed && ap[recovered] != '\\n')\n>  \t\t\trecovered++;\n> +\t\tif (recovered < trimmed && ap[recovered] == '\\n')\n> +\t\t\trecovered++;\n>  \t}\n>  \ta->size -= (trimmed - recovered);\n>  \tb->size -= (trimmed - recovered);\n\nShoot.  Thanks for spotting.\n\nWouldn't it be enough to do:\n\n  \tfor (i = 0, recovered = 0; recovered < trimmed && i <= ctx; i++) {\n\t\twhile (recovered < trimmed && ap[recovered++] != '\\n')\n\t        \t; /* nothing */\n\t}\n\nthen (warning: I haven't had my coffee yet)?\n"},{"id":"63272","messageId":"20071215200202.GA3334@sigill.intra.peff.net","threadId":"11302","inReplyTo":"7vprx7n90t.fsf@gitster.siamese.dyndns.org","subject":"Re: trim_common_tail bug?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-15T20:02:02Z","receivedAt":"2007-12-15T20:02:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 15, 2007 at 09:49:06AM -0800, Junio C Hamano wrote:\n\n> Shoot.  Thanks for spotting.\n> \n> Wouldn't it be enough to do:\n> \n>   \tfor (i = 0, recovered = 0; recovered < trimmed && i <= ctx; i++) {\n> \t\twhile (recovered < trimmed && ap[recovered++] != '\\n')\n> \t        \t; /* nothing */\n> \t}\n> \n> then (warning: I haven't had my coffee yet)?\n\nYes, I think that is equivalent. My sleep-deprived brain keeps thinking\nthere must be a more clear way of writing this whole loop, but it\nescapes me at the moment.\n\n-Peff\n"},{"id":"63302","messageId":"20071216070614.GA5072@sigill.intra.peff.net","threadId":"11302","inReplyTo":"20071215200202.GA3334@sigill.intra.peff.net","subject":"Re: trim_common_tail bug?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-16T07:06:14Z","receivedAt":"2007-12-16T07:06:14Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 15, 2007 at 03:02:02PM -0500, Jeff King wrote:\n\n> >   \tfor (i = 0, recovered = 0; recovered < trimmed && i <= ctx; i++) {\n> > \t\twhile (recovered < trimmed && ap[recovered++] != '\\n')\n> > \t        \t; /* nothing */\n> > \t}\n> > \n> > then (warning: I haven't had my coffee yet)?\n> \n> Yes, I think that is equivalent. My sleep-deprived brain keeps thinking\n> there must be a more clear way of writing this whole loop, but it\n> escapes me at the moment.\n\nAnd this came to me in a dream. :) It fixes the bug, and I think it is a\nbit simpler to see the termination conditions in a single loop. But\nplease double-check correctness, and that you agree that it is more\nreadable.\n\n---\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 700def2..98b02ed 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -110,7 +110,7 @@ int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)\n static void trim_common_tail(mmfile_t *a, mmfile_t *b, long ctx)\n {\n \tconst int blk = 1024;\n-\tlong trimmed = 0, recovered = 0, i;\n+\tlong trimmed = 0, recovered = 0;\n \tchar *ap = a->ptr + a->size;\n \tchar *bp = b->ptr + b->size;\n \tlong smaller = (a->size < b->size) ? a->size : b->size;\n@@ -121,10 +121,9 @@ static void trim_common_tail(mmfile_t *a, mmfile_t *b, long ctx)\n \t\tbp -= blk;\n \t}\n \n-\tfor (i = 0, recovered = 0; recovered < trimmed && i <= ctx; i++) {\n-\t\twhile (recovered < trimmed && ap[recovered] != '\\n')\n-\t\t\trecovered++;\n-\t}\n+\twhile (recovered < trimmed && ctx)\n+\t\tif (ap[recovered++] == '\\n')\n+\t\t\tctx--;\n \ta->size -= (trimmed - recovered);\n \tb->size -= (trimmed - recovered);\n }\n"},{"id":"63340","messageId":"7v8x3ul927.fsf@gitster.siamese.dyndns.org","threadId":"11302","inReplyTo":"20071216070614.GA5072@sigill.intra.peff.net","subject":"Re: trim_common_tail bug?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-16T19:43:28Z","receivedAt":"2007-12-16T19:43:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sat, Dec 15, 2007 at 03:02:02PM -0500, Jeff King wrote:\n>\n>> >   \tfor (i = 0, recovered = 0; recovered < trimmed && i <= ctx; i++) {\n>> > \t\twhile (recovered < trimmed && ap[recovered++] != '\\n')\n>> > \t        \t; /* nothing */\n>> > \t}\n>> > \n>> > then (warning: I haven't had my coffee yet)?\n>> \n>> Yes, I think that is equivalent. My sleep-deprived brain keeps thinking\n>> there must be a more clear way of writing this whole loop, but it\n>> escapes me at the moment.\n>\n> And this came to me in a dream. :) It fixes the bug, and I think it is a\n> bit simpler to see the termination conditions in a single loop. But\n> please double-check correctness, and that you agree that it is more\n> readable.\n\nI wanted to recover to the end of the line that includes the cut-off\npoint, even when (ctx == 0), to be extra safer, but I do not think it\nwas necessary.\n\n> ...\n>  \t\tbp -= blk;\n>  \t}\n>  \n> +\twhile (recovered < trimmed && ctx)\n> +\t\tif (ap[recovered++] == '\\n')\n> +\t\t\tctx--;\n>  \ta->size -= (trimmed - recovered);\n>  \tb->size -= (trimmed - recovered);\n>  }\n"},{"id":"63349","messageId":"7v7ijejq6j.fsf@gitster.siamese.dyndns.org","threadId":"11302","inReplyTo":"7v8x3ul927.fsf@gitster.siamese.dyndns.org","subject":"Re: trim_common_tail bug?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-16T21:16:36Z","receivedAt":"2007-12-16T21:16:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n>\n>> On Sat, Dec 15, 2007 at 03:02:02PM -0500, Jeff King wrote:\n>>\n>>> >   \tfor (i = 0, recovered = 0; recovered < trimmed && i <= ctx; i++) {\n>>> > \t\twhile (recovered < trimmed && ap[recovered++] != '\\n')\n>>> > \t        \t; /* nothing */\n>>> > \t}\n>>> > \n>>> > then (warning: I haven't had my coffee yet)?\n>>> \n>>> Yes, I think that is equivalent. My sleep-deprived brain keeps thinking\n>>> there must be a more clear way of writing this whole loop, but it\n>>> escapes me at the moment.\n>>\n>> And this came to me in a dream. :) It fixes the bug, and I think it is a\n>> bit simpler to see the termination conditions in a single loop. But\n>> please double-check correctness, and that you agree that it is more\n>> readable.\n>\n> I wanted to recover to the end of the line that includes the cut-off\n> point, even when (ctx == 0), to be extra safer, but I do not think it\n> was necessary.\n\nI have to wonder what happens if the last line which is incomplete has\nmore than block bytes that are identical at the end?  We need to recover\nthe whole thing to show the correct diff, don't we?\n"},{"id":"63351","messageId":"20071216212104.GA32307@coredump.intra.peff.net","threadId":"11302","inReplyTo":"7v7ijejq6j.fsf@gitster.siamese.dyndns.org","subject":"Re: trim_common_tail bug?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-16T21:21:04Z","receivedAt":"2007-12-16T21:21:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 16, 2007 at 01:16:36PM -0800, Junio C Hamano wrote:\n\n> > I wanted to recover to the end of the line that includes the cut-off\n> > point, even when (ctx == 0), to be extra safer, but I do not think it\n> > was necessary.\n> \n> I have to wonder what happens if the last line which is incomplete has\n> more than block bytes that are identical at the end?  We need to recover\n> the whole thing to show the correct diff, don't we?\n\nNo, I think it's right as-is. We forget about the blocks during the\nrecovery section of the code. IOW, we just keep reading forward until we\nfind all of the context lines, or we run out of trimmed content. In the\nfirst case, we are fine (we restored the right number of context lines).\nIn the latter case, we are also fine, because we end up trimming nothing\n(IOW, there _weren't_ enough context lines in the first place).\n\n-Peff\n"},{"id":"63353","messageId":"7v3au2joo2.fsf_-_@gitster.siamese.dyndns.org","threadId":"11302","inReplyTo":"20071216212104.GA32307@coredump.intra.peff.net","subject":"[PATCH] Re-re-re-fix common tail optimization","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-16T21:49:17Z","receivedAt":"2007-12-16T21:49:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> No, I think it's right as-is. We forget about the blocks during the\n> recovery section of the code. IOW, we just keep reading forward until we\n> find all of the context lines, or we run out of trimmed content. In the\n> first case, we are fine (we restored the right number of context lines).\n> In the latter case, we are also fine, because we end up trimming nothing\n> (IOW, there _weren't_ enough context lines in the first place).\n\nKind'a embarrassing that both of us cannot get this right without so\nmany rounds, isn't it?\n\n-- >8 --\nSubject: [PATCH] Re-re-re-fix common tail optimization\n\nWe need to be extra careful recovering the removed common section, so\nthat we do not break context nor the changed incomplete line (i.e. the\nlast line that does not end with LF).\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n t/t4024-diff-optimize-common.sh |   69 +++++++++++++++++++++++++++++++++++++++\n xdiff-interface.c               |    2 +-\n 2 files changed, 70 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t4024-diff-optimize-common.sh b/t/t4024-diff-optimize-common.sh\nnew file mode 100755\nindex 0000000..10405f1\n--- /dev/null\n+++ b/t/t4024-diff-optimize-common.sh\n@@ -0,0 +1,69 @@\n+#!/bin/sh\n+\n+test_description='common tail optimization'\n+\n+. ./test-lib.sh\n+\n+z=zzzzzzzz ;# 8\n+z=\"$z$z$z$z$z$z$z$z\" ;# 64\n+z=\"$z$z$z$z$z$z$z$z\" ;# 512\n+z=\"$z$z$z$z\" ;# 2048\n+z2047=$(expr \"$z\" : '.\\(.*\\)') ; #2047\n+\n+test_expect_success setup '\n+\n+\techo \"a$z2047\" >file-a &&\n+\techo \"b\" >file-b &&\n+\techo \"$z2047\" >>file-b &&\n+\techo \"c$z2047\" | tr -d \"\\012\" >file-c &&\n+\techo \"d\" >file-d &&\n+\techo \"$z2047\" | tr -d \"\\012\" >>file-d &&\n+\n+\tgit add file-a file-b file-c file-d &&\n+\n+\techo \"A$z2047\" >file-a &&\n+\techo \"B\" >file-b &&\n+\techo \"$z2047\" >>file-b &&\n+\techo \"C$z2047\" | tr -d \"\\012\" >file-c &&\n+\techo \"D\" >file-d &&\n+\techo \"$z2047\" | tr -d \"\\012\" >>file-d\n+\n+'\n+\n+echo >expect <<\\EOF\n+diff --git a/file-a b/file-a\n+--- a/file-a\n++++ b/file-a\n+@@ -1 +1 @@\n+-aZ\n++AZ\n+diff --git a/file-b b/file-b\n+--- a/file-b\n++++ b/file-b\n+@@ -1 +1 @@\n+-b\n++B\n+diff --git a/file-c b/file-c\n+--- a/file-c\n++++ b/file-c\n+@@ -1 +1 @@\n+-cZ\n+\\ No newline at end of file\n++CZ\n+\\ No newline at end of file\n+diff --git a/file-d b/file-d\n+--- a/file-d\n++++ b/file-d\n+@@ -1 +1 @@\n+-d\n++D\n+EOF\n+\n+test_expect_success 'diff -U0' '\n+\n+\tgit diff -U0 | sed -e \"/^index/d\" -e \"s/$z2047/Z/g\" >actual &&\n+\tdiff -u expect actual\n+\n+'\n+\n+test_done\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 98b02ed..9ee877c 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -121,7 +121,7 @@ static void trim_common_tail(mmfile_t *a, mmfile_t *b, long ctx)\n \t\tbp -= blk;\n \t}\n \n-\twhile (recovered < trimmed && ctx)\n+\twhile (recovered < trimmed && 0 <= ctx)\n \t\tif (ap[recovered++] == '\\n')\n \t\t\tctx--;\n \ta->size -= (trimmed - recovered);\n"},{"id":"63355","messageId":"20071216221545.GA32596@coredump.intra.peff.net","threadId":"11302","inReplyTo":"7v3au2joo2.fsf_-_@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Re-re-re-fix common tail optimization","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-16T22:15:45Z","receivedAt":"2007-12-16T22:15:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 16, 2007 at 01:49:17PM -0800, Junio C Hamano wrote:\n\n> Kind'a embarrassing that both of us cannot get this right without so\n> many rounds, isn't it?\n\nGood thing we can just delete the emails and nobody will be the wiser...\n\n> +echo >expect <<\\EOF\n\nThis would probably work better as 'cat'.\n\n> +test_expect_success 'diff -U0' '\n> +\n> +\tgit diff -U0 | sed -e \"/^index/d\" -e \"s/$z2047/Z/g\" >actual &&\n> +\tdiff -u expect actual\n\nAren't we using \"git diff\" for the second diff there nowadays?\n\n> -\twhile (recovered < trimmed && ctx)\n> +\twhile (recovered < trimmed && 0 <= ctx)\n>  \t\tif (ap[recovered++] == '\\n')\n>  \t\t\tctx--;\n>  \ta->size -= (trimmed - recovered);\n\nOops (I think maybe I misunderstood what you were asking in the last\nemail). This fix is correct, though the code is now kind of subtle. I\nthink it would be more obvious as:\n\n  /* finish off any changed line we are in */\n  while (recovered < trimmed && ap[recovered++] != '\\n')\n    /* nothing */;\n  /* recover context lines */\n  while (recovered < trimmed && ctx)\n    if (ap[recovered++] == '\\n')\n      ctx--;\n\nYour loop does both actions in the same loop, which is correct, but took\nme 10 minutes of thinking and staring to realize what was going on.\n\n-Peff\n"},{"id":"63356","messageId":"7vtzmii8io.fsf@gitster.siamese.dyndns.org","threadId":"11302","inReplyTo":"20071216221545.GA32596@coredump.intra.peff.net","subject":"Re: [PATCH] Re-re-re-fix common tail optimization","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-16T22:23:27Z","receivedAt":"2007-12-16T22:23:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> This would probably work better as 'cat'.\n\nYeah, I amended it without adding another \"re-\" to the title ;-)  The\nresult has been already pushed out.\n\n>> +test_expect_success 'diff -U0' '\n>> +\n>> +\tgit diff -U0 | sed -e \"/^index/d\" -e \"s/$z2047/Z/g\" >actual &&\n>> +\tdiff -u expect actual\n>\n> Aren't we using \"git diff\" for the second diff there nowadays?\n\nSome people seem to think that is a good idea, but I generally do not\nlike using \"git diff\" between expect and actual (both untracked) inside\ntests.  The last \"diff\" is about validating what git does and using \"git\ndiff\" there would make the test meaningless when \"git diff\" itself is\nbroken.\n\nThis is especially so because comparison between untracked files is a\nbolted-on afterthought and I am least confident about among the\ncodepaths in the whole \"git diff\" (it is not even my nor Linus's code).\n"},{"id":"63358","messageId":"7vodcqi8aq.fsf@gitster.siamese.dyndns.org","threadId":"11302","inReplyTo":"7vtzmii8io.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Re-re-re-fix common tail optimization","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-16T22:28:13Z","receivedAt":"2007-12-16T22:28:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>>> +test_expect_success 'diff -U0' '\n>>> +\n>>> +\tgit diff -U0 | sed -e \"/^index/d\" -e \"s/$z2047/Z/g\" >actual &&\n>>> +\tdiff -u expect actual\n>>\n>> Aren't we using \"git diff\" for the second diff there nowadays?\n>\n> Some people seem to think that is a good idea, but I generally do not\n> like using \"git diff\" between expect and actual (both untracked) inside\n> tests.  The last \"diff\" is about validating what git does and using \"git\n> diff\" there would make the test meaningless when \"git diff\" itself is\n> broken.\n>\n> This is especially so because comparison between untracked files is a\n> bolted-on afterthought and I am least confident about among the\n> codepaths in the whole \"git diff\" (it is not even my nor Linus's code).\n\nSide note.  The \"confidence\" I am talking about the above is not about\nthe correct-working of the current code.  It seems to work fine.\n\nIt is about the fact it was bolted on rather than designed in from the\nbeginning---it is much likely to subtly break than other parts that are\nmuch more integrated when you change seemingly unrelated thing like\ngit-dir discovery and rename detection.\n\nIOW, the confidence is about the fixability/maintainability when\nsomebody breaks it.\n"},{"id":"63359","messageId":"20071216222919.GA2260@coredump.intra.peff.net","threadId":"11302","inReplyTo":"7vtzmii8io.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Re-re-re-fix common tail optimization","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-16T22:29:19Z","receivedAt":"2007-12-16T22:29:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 16, 2007 at 02:23:27PM -0800, Junio C Hamano wrote:\n\n> Yeah, I amended it without adding another \"re-\" to the title ;-)  The\n> result has been already pushed out.\n\nOK. Too late, but it has my ack. ;)\n\n> > Aren't we using \"git diff\" for the second diff there nowadays?\n> \n> Some people seem to think that is a good idea, but I generally do not\n> like using \"git diff\" between expect and actual (both untracked) inside\n> tests.  The last \"diff\" is about validating what git does and using \"git\n> diff\" there would make the test meaningless when \"git diff\" itself is\n> broken.\n\nI think that is a valid concern. But ISTR that were some issues with\nusing GNU diff. Commit 5bd74506 mentions getting rid of the dependency\nin all existing tests, but gives no reason.\n\n-Peff\n"},{"id":"63380","messageId":"EBD73F46-810F-4605-972C-54EED0EF9A63@wincent.com","threadId":"11302","inReplyTo":"20071216222919.GA2260@coredump.intra.peff.net","subject":"Re: [PATCH] Re-re-re-fix common tail optimization","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-12-17T08:42:20Z","receivedAt":"2007-12-17T08:42:20Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"El 16/12/2007, a las 23:29, Jeff King escribió:\n\n> On Sun, Dec 16, 2007 at 02:23:27PM -0800, Junio C Hamano wrote:\n>\n>> Yeah, I amended it without adding another \"re-\" to the title ;-)  The\n>> result has been already pushed out.\n>\n> OK. Too late, but it has my ack. ;)\n>\n>>> Aren't we using \"git diff\" for the second diff there nowadays?\n>>\n>> Some people seem to think that is a good idea, but I generally do not\n>> like using \"git diff\" between expect and actual (both untracked)  \n>> inside\n>> tests.  The last \"diff\" is about validating what git does and using  \n>> \"git\n>> diff\" there would make the test meaningless when \"git diff\" itself is\n>> broken.\n>\n> I think that is a valid concern. But ISTR that were some issues with\n> using GNU diff. Commit 5bd74506 mentions getting rid of the dependency\n> in all existing tests, but gives no reason.\n\nI'd say it's safe and sensible to use \"git diff\" in all tests *except*  \nfor tests of \"git diff\" itself.\n\nWincent\n"},{"id":"63393","messageId":"Pine.LNX.4.64.0712171038130.9446@racer.site","threadId":"11302","inReplyTo":"EBD73F46-810F-4605-972C-54EED0EF9A63@wincent.com","subject":"Re: [PATCH] Re-re-re-fix common tail optimization","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-12-17T10:39:55Z","receivedAt":"2007-12-17T10:39:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 17 Dec 2007, Wincent Colaiuta wrote:\n\n> El 16/12/2007, a las 23:29, Jeff King escribi?:\n> \n> > On Sun, Dec 16, 2007 at 02:23:27PM -0800, Junio C Hamano wrote:\n> > \n> > > > Aren't we using \"git diff\" for the second diff there nowadays?\n> > > \n> > > Some people seem to think that is a good idea, but I generally do \n> > > not like using \"git diff\" between expect and actual (both untracked) \n> > > inside tests.  The last \"diff\" is about validating what git does and \n> > > using \"git diff\" there would make the test meaningless when \"git \n> > > diff\" itself is broken.\n> > \n> > I think that is a valid concern. But ISTR that were some issues with \n> > using GNU diff. Commit 5bd74506 mentions getting rid of the dependency \n> > in all existing tests, but gives no reason.\n> \n> I'd say it's safe and sensible to use \"git diff\" in all tests *except* \n> for tests of \"git diff\" itself.\n\nTo the contrary.  It has to test \"git diff\", so it must use \"git diff\".  \nAs for the reference output: we include the expected diffs as texts, and \ntherefore do not really have to rely on having GNU diff installed.\n\nBesides, we cannot even test the goodies like \"rename from\" by comparing \nto GNU diff's output.\n\nCiao,\nDscho\n"},{"id":"63398","messageId":"36E62F9B-26FF-4DC0-99B8-D6DC2B960E67@wincent.com","threadId":"11302","inReplyTo":"Pine.LNX.4.64.0712171038130.9446@racer.site","subject":"Re: [PATCH] Re-re-re-fix common tail optimization","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-12-17T10:59:22Z","receivedAt":"2007-12-17T10:59:22Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"El 17/12/2007, a las 11:39, Johannes Schindelin escribió:\n\n> Hi,\n>\n> On Mon, 17 Dec 2007, Wincent Colaiuta wrote:\n>\n>> El 16/12/2007, a las 23:29, Jeff King escribi?:\n>>\n>>> On Sun, Dec 16, 2007 at 02:23:27PM -0800, Junio C Hamano wrote:\n>>>\n>>>>> Aren't we using \"git diff\" for the second diff there nowadays?\n>>>>\n>>>> Some people seem to think that is a good idea, but I generally do\n>>>> not like using \"git diff\" between expect and actual (both  \n>>>> untracked)\n>>>> inside tests.  The last \"diff\" is about validating what git does  \n>>>> and\n>>>> using \"git diff\" there would make the test meaningless when \"git\n>>>> diff\" itself is broken.\n>>>\n>>> I think that is a valid concern. But ISTR that were some issues with\n>>> using GNU diff. Commit 5bd74506 mentions getting rid of the  \n>>> dependency\n>>> in all existing tests, but gives no reason.\n>>\n>> I'd say it's safe and sensible to use \"git diff\" in all tests  \n>> *except*\n>> for tests of \"git diff\" itself.\n>\n> To the contrary.  It has to test \"git diff\", so it must use \"git  \n> diff\".\n\nObviously, you can only test \"git diff\" by actually running it.\n\n> As for the reference output: we include the expected diffs as texts,  \n> and\n> therefore do not really have to rely on having GNU diff installed.\n>\n> Besides, we cannot even test the goodies like \"rename from\" by  \n> comparing\n> to GNU diff's output.\n\nSorry, I didn't make myself clear. That's not what I was proposing at  \nall. I was talking about this kind of example:\n\n> +\tgit diff -U0 | sed -e \"/^index/d\" -e \"s/$z2047/Z/g\" >actual &&\n> +\tdiff -u expect actual\n\n\nFirst line uses \"git diff\", if the second line uses \"git diff\" as well  \nand \"git diff\" happens to be broken then you're using a broken tool to  \ntest a broken tool, as Junio already pointed out. I presumed that if  \nyou had read the whole thread then that would be obvious (look at the  \nquoted section from Junio above).\n\nIn the example you're not interested in the details of the output  \nformat, only in the exit status, so it is appropriate to use diff  \ninstead of \"git diff\".\n\nWincent\n"},{"id":"63408","messageId":"Pine.LNX.4.64.0712171151490.9446@racer.site","threadId":"11302","inReplyTo":"36E62F9B-26FF-4DC0-99B8-D6DC2B960E67@wincent.com","subject":"Re: [PATCH] Re-re-re-fix common tail optimization","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-12-17T11:57:29Z","receivedAt":"2007-12-17T11:57:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 17 Dec 2007, Wincent Colaiuta wrote:\n\n> El 17/12/2007, a las 11:39, Johannes Schindelin escribi?:\n> \n> > On Mon, 17 Dec 2007, Wincent Colaiuta wrote:\n> > \n> > > El 16/12/2007, a las 23:29, Jeff King escribi?:\n> > > \n> > > > On Sun, Dec 16, 2007 at 02:23:27PM -0800, Junio C Hamano wrote:\n> > > > \n> > > > > > Aren't we using \"git diff\" for the second diff there nowadays?\n> > > > > \n> > > > > Some people seem to think that is a good idea, but I generally \n> > > > > do not like using \"git diff\" between expect and actual (both \n> > > > > untracked) inside tests.  The last \"diff\" is about validating \n> > > > > what git does and using \"git diff\" there would make the test \n> > > > > meaningless when \"git diff\" itself is broken.\n> > > > \n> > > > I think that is a valid concern. But ISTR that were some issues \n> > > > with using GNU diff. Commit 5bd74506 mentions getting rid of the \n> > > > dependency in all existing tests, but gives no reason.\n> > > \n> > > I'd say it's safe and sensible to use \"git diff\" in all tests \n> > > *except* for tests of \"git diff\" itself.\n> > \n> > To the contrary.  It has to test \"git diff\", so it must use \"git \n> > diff\".\n> \n> Obviously, you can only test \"git diff\" by actually running it.\n\nSorry, I should have made clear that I meant this as funny:\n\n\t;-)\n\n> > As for the reference output: we include the expected diffs as texts, \n> > and therefore do not really have to rely on having GNU diff installed.\n> > \n> > Besides, we cannot even test the goodies like \"rename from\" by \n> > comparing to GNU diff's output.\n> \n> Sorry, I didn't make myself clear. That's not what I was proposing at \n> all. I was talking about this kind of example:\n> \n> > + git diff -U0 | sed -e \"/^index/d\" -e \"s/$z2047/Z/g\" >actual &&\n> > + diff -u expect actual\n> \n> First line uses \"git diff\", if the second line uses \"git diff\" as well \n> and \"git diff\" happens to be broken then you're using a broken tool to \n> test a broken tool, as Junio already pointed out.\n\nHmm.  There is some chicken-and-egg problem here (I read the thread, but \ndid not really see a problem, as I assumed that _other_ tests would assure \nthat \"git diff --no-index\" works as expected).\n\nBut as at least one released version of GNU diff has a pretty serious bug, \nI would rather not rely too much on diff.  (BTW this was the reason I \nwanted --no-index so badly.)\n\nSo yeah, the second \"diff\" cannot be \"git diff\".  Maybe \"cmp\", but not \n\"git diff\".\n\nCiao,\nDscho\n"},{"id":"63411","messageId":"57245FA1-361B-4333-B490-A2CC99ED4F9C@wincent.com","threadId":"11302","inReplyTo":"Pine.LNX.4.64.0712171151490.9446@racer.site","subject":"Re: [PATCH] Re-re-re-fix common tail optimization","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-12-17T12:08:45Z","receivedAt":"2007-12-17T12:08:45Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"El 17/12/2007, a las 12:57, Johannes Schindelin escribió:\n\n> Hmm.  There is some chicken-and-egg problem here (I read the thread,  \n> but\n> did not really see a problem, as I assumed that _other_ tests would  \n> assure\n> that \"git diff --no-index\" works as expected).\n>\n> But as at least one released version of GNU diff has a pretty  \n> serious bug,\n> I would rather not rely too much on diff.  (BTW this was the reason I\n> wanted --no-index so badly.)\n>\n> So yeah, the second \"diff\" cannot be \"git diff\".  Maybe \"cmp\", but not\n> \"git diff\".\n\nWell cmp would be fine as well, seeing all we want is a boolean \"is  \nthis the same or not\" answer. (I'm not familiar with the GNU diff bug  \nyou speak of, but was it so bad that it couldn't even get *that*  \nanswer right?)\n\nCheers,\nWincent\n"},{"id":"63412","messageId":"20071217121209.GA15368@coredump.intra.peff.net","threadId":"11302","inReplyTo":"57245FA1-361B-4333-B490-A2CC99ED4F9C@wincent.com","subject":"Re: [PATCH] Re-re-re-fix common tail optimization","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-17T12:12:10Z","receivedAt":"2007-12-17T12:12:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 17, 2007 at 01:08:45PM +0100, Wincent Colaiuta wrote:\n\n>> So yeah, the second \"diff\" cannot be \"git diff\".  Maybe \"cmp\", but not\n>> \"git diff\".\n>\n> Well cmp would be fine as well, seeing all we want is a boolean \"is this the \n> same or not\" answer. (I'm not familiar with the GNU diff bug you speak of, \n> but was it so bad that it couldn't even get *that* answer right?)\n\nPersonally I find it useful to generate the diff output when viewing\nwith \"-v\". But if there is a real reason to use cmp over diff in the\nscript, one can always manually go into the trash directory and run\ndiff.\n\n-Peff\n"},{"id":"63414","messageId":"476669A7.1050407@viscovery.net","threadId":"11302","inReplyTo":"57245FA1-361B-4333-B490-A2CC99ED4F9C@wincent.com","subject":"Re: [PATCH] Re-re-re-fix common tail optimization","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2007-12-17T12:20:55Z","receivedAt":"2007-12-17T12:20:55Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Wincent Colaiuta schrieb:\n> El 17/12/2007, a las 12:57, Johannes Schindelin escribió:\n> \n>> Hmm.  There is some chicken-and-egg problem here (I read the thread, but\n>> did not really see a problem, as I assumed that _other_ tests would\n>> assure\n>> that \"git diff --no-index\" works as expected).\n>>\n>> But as at least one released version of GNU diff has a pretty serious\n>> bug,\n>> I would rather not rely too much on diff.  (BTW this was the reason I\n>> wanted --no-index so badly.)\n>>\n>> So yeah, the second \"diff\" cannot be \"git diff\".  Maybe \"cmp\", but not\n>> \"git diff\".\n> \n> Well cmp would be fine as well, seeing all we want is a boolean \"is this\n> the same or not\" answer. (I'm not familiar with the GNU diff bug you\n> speak of, but was it so bad that it couldn't even get *that* answer right?)\n\nHeh, there's at least one distribution out there (Suse 10.1) that comes with\na *cmp* that doesn't get that answer right if its output is connected to\n/dev/null, which is the case when you simply 'make test'.\n\n-- Hannes\n"},{"id":"63420","messageId":"Pine.LNX.4.64.0712171250550.9446@racer.site","threadId":"11302","inReplyTo":"476669A7.1050407@viscovery.net","subject":"Re: [PATCH] Re-re-re-fix common tail optimization","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-12-17T12:51:29Z","receivedAt":"2007-12-17T12:51:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 17 Dec 2007, Johannes Sixt wrote:\n\n> Wincent Colaiuta schrieb:\n> > El 17/12/2007, a las 12:57, Johannes Schindelin escribi?:\n> > \n> >> Hmm.  There is some chicken-and-egg problem here (I read the thread, but\n> >> did not really see a problem, as I assumed that _other_ tests would\n> >> assure\n> >> that \"git diff --no-index\" works as expected).\n> >>\n> >> But as at least one released version of GNU diff has a pretty serious\n> >> bug,\n> >> I would rather not rely too much on diff.  (BTW this was the reason I\n> >> wanted --no-index so badly.)\n> >>\n> >> So yeah, the second \"diff\" cannot be \"git diff\".  Maybe \"cmp\", but not\n> >> \"git diff\".\n> > \n> > Well cmp would be fine as well, seeing all we want is a boolean \"is \n> > this the same or not\" answer. (I'm not familiar with the GNU diff bug \n> > you speak of, but was it so bad that it couldn't even get *that* \n> > answer right?)\n> \n> Heh, there's at least one distribution out there (Suse 10.1) that comes \n> with a *cmp* that doesn't get that answer right if its output is \n> connected to /dev/null, which is the case when you simply 'make test'.\n\nYeah.  That's what it was.  I even posted a patch to GNU diff, only to \nfind out that it was already fixed in CVS.  Sigh.\n\nCiao,\nDscho\n"},{"id":"63444","messageId":"7vsl218aqd.fsf@gitster.siamese.dyndns.org","threadId":"11302","inReplyTo":"Pine.LNX.4.64.0712171250550.9446@racer.site","subject":"Re: [PATCH] Re-re-re-fix common tail optimization","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-17T17:58:02Z","receivedAt":"2007-12-17T17:58:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> >> But as at least one released version of GNU diff has a pretty serious\n>> >> bug,\n>> >> I would rather not rely too much on diff.  (BTW this was the reason I\n>> >> wanted --no-index so badly.)\n>> >>\n>> >> So yeah, the second \"diff\" cannot be \"git diff\".  Maybe \"cmp\", but not\n>> >> \"git diff\".\n>> > \n>> > Well cmp would be fine as well, seeing all we want is a boolean \"is \n>> > this the same or not\" answer. (I'm not familiar with the GNU diff bug \n>> > you speak of, but was it so bad that it couldn't even get *that* \n>> > answer right?)\n>> \n>> Heh, there's at least one distribution out there (Suse 10.1) that comes \n>> with a *cmp* that doesn't get that answer right if its output is \n>> connected to /dev/null, which is the case when you simply 'make test'.\n>\n> Yeah.  That's what it was.  I even posted a patch to GNU diff, only to \n> find out that it was already fixed in CVS.  Sigh.\n\nWait.  Are you still talking about diff or cmp, or are you saying that\nyour earlier statement about avoiding GNU diff due to its bugs is\nunfounded?\n"},{"id":"63447","messageId":"Pine.LNX.4.64.0712171804040.9446@racer.site","threadId":"11302","inReplyTo":"7vsl218aqd.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Re-re-re-fix common tail optimization","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-12-17T18:05:48Z","receivedAt":"2007-12-17T18:05:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 17 Dec 2007, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> >> >> But as at least one released version of GNU diff has a pretty \n> >> >> serious bug, I would rather not rely too much on diff.  (BTW this \n> >> >> was the reason I wanted --no-index so badly.)\n> >> >>\n> >> >> So yeah, the second \"diff\" cannot be \"git diff\".  Maybe \"cmp\", but \n> >> >> not \"git diff\".\n> >> > \n> >> > Well cmp would be fine as well, seeing all we want is a boolean \"is \n> >> > this the same or not\" answer. (I'm not familiar with the GNU diff \n> >> > bug you speak of, but was it so bad that it couldn't even get \n> >> > *that* answer right?)\n> >> \n> >> Heh, there's at least one distribution out there (Suse 10.1) that \n> >> comes with a *cmp* that doesn't get that answer right if its output \n> >> is connected to /dev/null, which is the case when you simply 'make \n> >> test'.\n> >\n> > Yeah.  That's what it was.  I even posted a patch to GNU diff, only to \n> > find out that it was already fixed in CVS.  Sigh.\n> \n> Wait.  Are you still talking about diff or cmp, or are you saying that \n> your earlier statement about avoiding GNU diff due to its bugs is \n> unfounded?\n\nI do not remember offhand.  I only remembered that it was the GNU diff \npackage.  But maybe it was only the \"cmp\" tool.  Symptoms were that tests \nwere failing without \"-i\", but succeeding with \"-i\".\n\nOkay, I found the mail:\n\nhttp://article.gmane.org/gmane.comp.version-control.git/25107/match=cmp\n\nSeems it was only \"cmp\".\n\nSorry for the noise,\nDscho\n"},{"id":"63768","messageId":"20071219141845.GA2146@hashpling.org","threadId":"11302","inReplyTo":"7v3au2joo2.fsf_-_@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Re-re-re-fix common tail optimization","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2007-12-19T14:18:45Z","receivedAt":"2007-12-19T14:18:45Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Sun, Dec 16, 2007 at 01:49:17PM -0800, Junio C Hamano wrote:\n> \n> Kind'a embarrassing that both of us cannot get this right without so\n> many rounds, isn't it?\n> \n> -- >8 --\n> Subject: [PATCH] Re-re-re-fix common tail optimization\n> \n> We need to be extra careful recovering the removed common section, so\n> that we do not break context nor the changed incomplete line (i.e. the\n> last line that does not end with LF).\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\nJust to add to the woe on this one, this test breaks on MacOS X due to\nthe pattern length limitations of the default sed on that platform.\n\nInterested in a patch?\n\nCharles.\n"},{"id":"63769","messageId":"20071219142715.GB14187@coredump.intra.peff.net","threadId":"11302","inReplyTo":"20071219141845.GA2146@hashpling.org","subject":"Re: [PATCH] Re-re-re-fix common tail optimization","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-19T14:27:15Z","receivedAt":"2007-12-19T14:27:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 19, 2007 at 02:18:45PM +0000, Charles Bailey wrote:\n\n> Just to add to the woe on this one, this test breaks on MacOS X due to\n> the pattern length limitations of the default sed on that platform.\n> \n> Interested in a patch?\n\nSomebody beat you to it. :) Can you confirm that the fix in\n\n  <1198007158-27576-1-git-send-email-win@wincent.com>\n\nworks for you?\n\n-Peff\n"},{"id":"63772","messageId":"20071219143712.GA3483@hashpling.org","threadId":"11302","inReplyTo":"20071219142715.GB14187@coredump.intra.peff.net","subject":"Re: [PATCH] Re-re-re-fix common tail optimization","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2007-12-19T14:37:12Z","receivedAt":"2007-12-19T14:37:12Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Wed, Dec 19, 2007 at 09:27:15AM -0500, Jeff King wrote:\n> On Wed, Dec 19, 2007 at 02:18:45PM +0000, Charles Bailey wrote:\n> \n> > Just to add to the woe on this one, this test breaks on MacOS X due to\n> > the pattern length limitations of the default sed on that platform.\n> > \n> > Interested in a patch?\n> \n> Somebody beat you to it. :) Can you confirm that the fix in\n> \n>   <1198007158-27576-1-git-send-email-win@wincent.com>\n> \n> works for you?\n> \n> -Peff\n\n\nOoh, the excitement, I've never had the opportunity to \"git am\"\nbefore.\n\nYes, I can confirm.  It works for me.\n\nFor reference I had the following, which is fewer lines but not\ninherently better in any other way.\n\nCharles.\n\n\n\ndiff --git a/t/t4024-diff-optimize-common.sh b/t/t4024-diff-optimize-common.sh\nindex 20fe87b..ffb2c8f 100755\n--- a/t/t4024-diff-optimize-common.sh\n+++ b/t/t4024-diff-optimize-common.sh\n@@ -7,8 +7,9 @@ test_description='common tail optimization'\n z=zzzzzzzz ;# 8\n z=\"$z$z$z$z$z$z$z$z\" ;# 64\n z=\"$z$z$z$z$z$z$z$z\" ;# 512\n-z=\"$z$z$z$z\" ;# 2048\n-z2047=$(expr \"$z\" : '.\\(.*\\)') ; #2047\n+z=\"$z$z\" ;# 1024\n+z1023=$(expr \"$z\" : '.\\(.*\\)') ; #1023\n+z2047=$z$z1023\n \n test_expect_success setup '\n \n@@ -35,8 +36,8 @@ diff --git a/file-a b/file-a\n --- a/file-a\n +++ b/file-a\n @@ -1 +1 @@\n--aZ\n-+AZ\n+-aZZz\n++AZZz\n diff --git a/file-b b/file-b\n --- a/file-b\n +++ b/file-b\n@@ -47,9 +48,9 @@ diff --git a/file-c b/file-c\n --- a/file-c\n +++ b/file-c\n @@ -1 +1 @@\n--cZ\n+-cZZz\n \\ No newline at end of file\n-+CZ\n++CZZz\n \\ No newline at end of file\n diff --git a/file-d b/file-d\n --- a/file-d\n@@ -61,7 +62,7 @@ EOF\n \n test_expect_success 'diff -U0' '\n \n-\tgit diff -U0 | sed -e \"/^index/d\" -e \"s/$z2047/Z/g\" >actual &&\n+\tgit diff -U0 | sed -e \"/^index/d\" -e \"s/$z1023/Z/g\" >actual &&\n \tdiff -u expect actual\n \n '\n-- \n1.5.3.7.11.ga3d7\n"},{"id":"63822","messageId":"7vy7bqrzat.fsf@gitster.siamese.dyndns.org","threadId":"11302","inReplyTo":"20071219143712.GA3483@hashpling.org","subject":"Re: [PATCH] Re-re-re-fix common tail optimization","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-20T00:21:30Z","receivedAt":"2007-12-20T00:21:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charles Bailey <charles@hashpling.org> writes:\n\n> On Wed, Dec 19, 2007 at 09:27:15AM -0500, Jeff King wrote:\n> ...\n>> Somebody beat you to it. :) Can you confirm that the fix in\n>> \n>>   <1198007158-27576-1-git-send-email-win@wincent.com>\n>> \n>> works for you?\n>\n> Ooh, the excitement, I've never had the opportunity to \"git am\"\n> before.\n\nWell, if Wincent had sent the patch as plain text without MIME\nquoted-printable, you did not have to even have the excitement of\nrunning \"git am\", but a simple \"git apply\" would have sufficed.\n\n    ...\n    How about the following? This swaps in perl in place of sed, which we c=\n    an\n    hopefully rely upon to work across platforms.\n\n    Cheers,\n    Wincent\n\n    -------- 8< --------\n    =46ix tests for broken sed on Leopard\n\n    The newly-added common-tail-optimization test fails on Leopard because\n    the broken sed implementation bails with a spurious \"unterminated\n    substitute pattern\" error because of the length of one of the\n    arguments.\n\nJust for future reference, I do not mind avoiding \"top-post\" style patch\nusing \"-- >8 --\" cut markers, but do not have three or more dashes in\nthe cut marker.  \"mailinfo\" will split the message at that line and the\nportion intended as the proposed commit log message will be taken as\npart of garbage in front of the patch.  Using \"-- >8 --\" (two dashes,\nspace, scissors, ...) keeps both the front matter and the log text\ntogether so that \"am -i\" or \"commit --amend\" can work on both parts.\n\n\nBy the way, how does this rewrite look?\n\n-- >8 --\nt4024: fix test script to use simpler sed pattern\n\nThe earlier test stripped away expected number of 'z' but the output\nwould have been very hard to read once somebody broke the common tail\noptimization.  Instead, count the number of 'z' and show it, to help\ndiagnosing the problem better in the future.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n t/t4024-diff-optimize-common.sh |  158 ++++++++++++++++++++++++++++++---------\n 1 files changed, 123 insertions(+), 35 deletions(-)\n\ndiff --git a/t/t4024-diff-optimize-common.sh b/t/t4024-diff-optimize-common.sh\nindex 20fe87b..3c66102 100755\n--- a/t/t4024-diff-optimize-common.sh\n+++ b/t/t4024-diff-optimize-common.sh\n@@ -10,58 +10,146 @@ z=\"$z$z$z$z$z$z$z$z\" ;# 512\n z=\"$z$z$z$z\" ;# 2048\n z2047=$(expr \"$z\" : '.\\(.*\\)') ; #2047\n \n-test_expect_success setup '\n-\n-\techo \"a$z2047\" >file-a &&\n-\techo \"b\" >file-b &&\n-\techo \"$z2047\" >>file-b &&\n-\techo \"c$z2047\" | tr -d \"\\012\" >file-c &&\n-\techo \"d\" >file-d &&\n-\techo \"$z2047\" | tr -d \"\\012\" >>file-d &&\n+x=zzzzzzzzzz\t\t\t;# 10\n+y=\"$x$x$x$x$x$x$x$x$x$x\"\t;# 100\n+z=\"$y$y$y$y$y$y$y$y$y$y\"\t;# 1000\n+z1000=$z\n+z100=$y\n+z10=$x\n \n-\tgit add file-a file-b file-c file-d &&\n+zs() {\n+\tcount=\"$1\"\n+\twhile test \"$count\" -ge 1000\n+\tdo\n+\t\tcount=$(($count - 1000))\n+\t\tprintf \"%s\" $z1000\n+\tdone\n+\twhile test \"$count\" -ge 100\n+\tdo\n+\t\tcount=$(($count - 100))\n+\t\tprintf \"%s\" $z100\n+\tdone\n+\twhile test \"$count\" -ge 10\n+\tdo\n+\t\tcount=$(($count - 10))\n+\t\tprintf \"%s\" $z10\n+\tdone\n+\twhile test \"$count\" -ge 1\n+\tdo\n+\t\tcount=$(($count - 1))\n+\t\tprintf \"z\"\n+\tdone\n+}\n \n-\techo \"A$z2047\" >file-a &&\n-\techo \"B\" >file-b &&\n-\techo \"$z2047\" >>file-b &&\n-\techo \"C$z2047\" | tr -d \"\\012\" >file-c &&\n-\techo \"D\" >file-d &&\n-\techo \"$z2047\" | tr -d \"\\012\" >>file-d\n-\n-'\n+zc () {\n+\tsed -e \"/^index/d\" \\\n+\t\t-e \"s/$z1000/Q/g\" \\\n+\t\t-e \"s/QQQQQQQQQ/Z9000/g\" \\\n+\t\t-e \"s/QQQQQQQQ/Z8000/g\" \\\n+\t\t-e \"s/QQQQQQQ/Z7000/g\" \\\n+\t\t-e \"s/QQQQQQ/Z6000/g\" \\\n+\t\t-e \"s/QQQQQ/Z5000/g\" \\\n+\t\t-e \"s/QQQQ/Z4000/g\" \\\n+\t\t-e \"s/QQQ/Z3000/g\" \\\n+\t\t-e \"s/QQ/Z2000/g\" \\\n+\t\t-e \"s/Q/Z1000/g\" \\\n+\t\t-e \"s/$z100/Q/g\" \\\n+\t\t-e \"s/QQQQQQQQQ/Z900/g\" \\\n+\t\t-e \"s/QQQQQQQQ/Z800/g\" \\\n+\t\t-e \"s/QQQQQQQ/Z700/g\" \\\n+\t\t-e \"s/QQQQQQ/Z600/g\" \\\n+\t\t-e \"s/QQQQQ/Z500/g\" \\\n+\t\t-e \"s/QQQQ/Z400/g\" \\\n+\t\t-e \"s/QQQ/Z300/g\" \\\n+\t\t-e \"s/QQ/Z200/g\" \\\n+\t\t-e \"s/Q/Z100/g\" \\\n+\t\t-e \"s/000Z//g\" \\\n+\t\t-e \"s/$z10/Q/g\" \\\n+\t\t-e \"s/QQQQQQQQQ/Z90/g\" \\\n+\t\t-e \"s/QQQQQQQQ/Z80/g\" \\\n+\t\t-e \"s/QQQQQQQ/Z70/g\" \\\n+\t\t-e \"s/QQQQQQ/Z60/g\" \\\n+\t\t-e \"s/QQQQQ/Z50/g\" \\\n+\t\t-e \"s/QQQQ/Z40/g\" \\\n+\t\t-e \"s/QQQ/Z30/g\" \\\n+\t\t-e \"s/QQ/Z20/g\" \\\n+\t\t-e \"s/Q/Z10/g\" \\\n+\t\t-e \"s/00Z//g\" \\\n+\t\t-e \"s/z/Q/g\" \\\n+\t\t-e \"s/QQQQQQQQQ/Z9/g\" \\\n+\t\t-e \"s/QQQQQQQQ/Z8/g\" \\\n+\t\t-e \"s/QQQQQQQ/Z7/g\" \\\n+\t\t-e \"s/QQQQQQ/Z6/g\" \\\n+\t\t-e \"s/QQQQQ/Z5/g\" \\\n+\t\t-e \"s/QQQQ/Z4/g\" \\\n+\t\t-e \"s/QQQ/Z3/g\" \\\n+\t\t-e \"s/QQ/Z2/g\" \\\n+\t\t-e \"s/Q/Z1/g\" \\\n+\t\t-e \"s/0Z//g\" \\\n+\t;\n+}\n \n-cat >expect <<\\EOF\n-diff --git a/file-a b/file-a\n---- a/file-a\n-+++ b/file-a\n+expect_pattern () {\n+\tcnt=\"$1\"\n+\tcat <<EOF\n+diff --git a/file-a$cnt b/file-a$cnt\n+--- a/file-a$cnt\n++++ b/file-a$cnt\n @@ -1 +1 @@\n--aZ\n-+AZ\n-diff --git a/file-b b/file-b\n---- a/file-b\n-+++ b/file-b\n+-Z${cnt}a\n++Z${cnt}A\n+diff --git a/file-b$cnt b/file-b$cnt\n+--- a/file-b$cnt\n++++ b/file-b$cnt\n @@ -1 +1 @@\n -b\n +B\n-diff --git a/file-c b/file-c\n---- a/file-c\n-+++ b/file-c\n+diff --git a/file-c$cnt b/file-c$cnt\n+--- a/file-c$cnt\n++++ b/file-c$cnt\n @@ -1 +1 @@\n--cZ\n+-cZ$cnt\n \\ No newline at end of file\n-+CZ\n++CZ$cnt\n \\ No newline at end of file\n-diff --git a/file-d b/file-d\n---- a/file-d\n-+++ b/file-d\n+diff --git a/file-d$cnt b/file-d$cnt\n+--- a/file-d$cnt\n++++ b/file-d$cnt\n @@ -1 +1 @@\n -d\n +D\n EOF\n+}\n+\n+sample='1023 1024 1025 2047 4095'\n+\n+test_expect_success setup '\n+\n+\tfor n in $sample\n+\tdo\n+\t\t( zs $n ; echo a ) >file-a$n &&\n+\t\t( echo b; zs $n; echo ) >file-b$n &&\n+\t\t( printf c; zs $n ) >file-c$n &&\n+\t\t( echo d; zs $n ) >file-d$n &&\n+\n+\t\tgit add file-a$n file-b$n file-c$n file-d$n &&\n+\n+\t\t( zs $n ; echo A ) >file-a$n &&\n+\t\t( echo B; zs $n; echo ) >file-b$n &&\n+\t\t( printf C; zs $n ) >file-c$n &&\n+\t\t( echo D; zs $n ) >file-d$n &&\n+\n+\t\texpect_pattern $n || break\n+\n+\tdone >expect\n+'\n \n test_expect_success 'diff -U0' '\n \n-\tgit diff -U0 | sed -e \"/^index/d\" -e \"s/$z2047/Z/g\" >actual &&\n+\tfor n in $sample\n+\tdo\n+\t\tgit diff -U0 file-?$n\n+\tdone | zc >actual &&\n \tdiff -u expect actual\n \n '\n"},{"id":"63825","messageId":"C152371B-7BFE-4EDB-93C3-78805E58A773@wincent.com","threadId":"11302","inReplyTo":"7vy7bqrzat.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Re-re-re-fix common tail optimization","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-12-20T01:38:24Z","receivedAt":"2007-12-20T01:38:24Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"El 20/12/2007, a las 1:21, Junio C Hamano escribió:\n\n> Charles Bailey <charles@hashpling.org> writes:\n>\n>> On Wed, Dec 19, 2007 at 09:27:15AM -0500, Jeff King wrote:\n>> ...\n>>> Somebody beat you to it. :) Can you confirm that the fix in\n>>>\n>>>  <1198007158-27576-1-git-send-email-win@wincent.com>\n>>>\n>>> works for you?\n>>\n>> Ooh, the excitement, I've never had the opportunity to \"git am\"\n>> before.\n>\n> Well, if Wincent had sent the patch as plain text without MIME\n> quoted-printable, you did not have to even have the excitement of\n> running \"git am\", but a simple \"git apply\" would have sufficed.\n\nWell, I sent it using \"git send-email\", so I guess it didn't have MIME  \nquoted-printable.\n\nCheers,\nWincent\n"},{"id":"63843","messageId":"20071220092315.GA31337@hashpling.org","threadId":"11302","inReplyTo":"7vy7bqrzat.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Re-re-re-fix common tail optimization","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2007-12-20T09:23:15Z","receivedAt":"2007-12-20T09:23:15Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Wed, Dec 19, 2007 at 04:21:30PM -0800, Junio C Hamano wrote:\n> \n> By the way, how does this rewrite look?\n> \n\nIt looks good and the test works on my fedora and Mac OS X boxes.\n(Just for reference I'm on 10.4.3, not 'Leopard'.)\n\nI've snipped everything except zc that I wanted to comment on.\n\nThe only two really minor nits I have is that zc does bizarre things\nwithout warning for more than 9999 \"z\" ( e.g. 17000 = 9000 + 8000 =\n98000 (!) ) and that, from a function responsibility point of view,\nthe /^index/d looks rather out of place.\n\nPerhaps a comment saying tha zc is designed for <= 9999 z's?  Given\nthis, a lot of the /g are redundant.\n\nBut hey, it's a test script and it works and I don't have any better\nsuggestions. :)\n\nCharles.\n\n> +zc () {\n> +\tsed -e \"/^index/d\" \\\n> +\t\t-e \"s/$z1000/Q/g\" \\\n> +\t\t-e \"s/QQQQQQQQQ/Z9000/g\" \\\n> +\t\t-e \"s/QQQQQQQQ/Z8000/g\" \\\n> +\t\t-e \"s/QQQQQQQ/Z7000/g\" \\\n> +\t\t-e \"s/QQQQQQ/Z6000/g\" \\\n> +\t\t-e \"s/QQQQQ/Z5000/g\" \\\n> +\t\t-e \"s/QQQQ/Z4000/g\" \\\n> +\t\t-e \"s/QQQ/Z3000/g\" \\\n> +\t\t-e \"s/QQ/Z2000/g\" \\\n> +\t\t-e \"s/Q/Z1000/g\" \\\n> +\t\t-e \"s/$z100/Q/g\" \\\n> +\t\t-e \"s/QQQQQQQQQ/Z900/g\" \\\n> +\t\t-e \"s/QQQQQQQQ/Z800/g\" \\\n> +\t\t-e \"s/QQQQQQQ/Z700/g\" \\\n> +\t\t-e \"s/QQQQQQ/Z600/g\" \\\n> +\t\t-e \"s/QQQQQ/Z500/g\" \\\n> +\t\t-e \"s/QQQQ/Z400/g\" \\\n> +\t\t-e \"s/QQQ/Z300/g\" \\\n> +\t\t-e \"s/QQ/Z200/g\" \\\n> +\t\t-e \"s/Q/Z100/g\" \\\n> +\t\t-e \"s/000Z//g\" \\\n> +\t\t-e \"s/$z10/Q/g\" \\\n> +\t\t-e \"s/QQQQQQQQQ/Z90/g\" \\\n> +\t\t-e \"s/QQQQQQQQ/Z80/g\" \\\n> +\t\t-e \"s/QQQQQQQ/Z70/g\" \\\n> +\t\t-e \"s/QQQQQQ/Z60/g\" \\\n> +\t\t-e \"s/QQQQQ/Z50/g\" \\\n> +\t\t-e \"s/QQQQ/Z40/g\" \\\n> +\t\t-e \"s/QQQ/Z30/g\" \\\n> +\t\t-e \"s/QQ/Z20/g\" \\\n> +\t\t-e \"s/Q/Z10/g\" \\\n> +\t\t-e \"s/00Z//g\" \\\n> +\t\t-e \"s/z/Q/g\" \\\n> +\t\t-e \"s/QQQQQQQQQ/Z9/g\" \\\n> +\t\t-e \"s/QQQQQQQQ/Z8/g\" \\\n> +\t\t-e \"s/QQQQQQQ/Z7/g\" \\\n> +\t\t-e \"s/QQQQQQ/Z6/g\" \\\n> +\t\t-e \"s/QQQQQ/Z5/g\" \\\n> +\t\t-e \"s/QQQQ/Z4/g\" \\\n> +\t\t-e \"s/QQQ/Z3/g\" \\\n> +\t\t-e \"s/QQ/Z2/g\" \\\n> +\t\t-e \"s/Q/Z1/g\" \\\n> +\t\t-e \"s/0Z//g\" \\\n> +\t;\n> +}\n"},{"id":"63845","messageId":"7vtzmdpuuv.fsf@gitster.siamese.dyndns.org","threadId":"11302","inReplyTo":"20071220092315.GA31337@hashpling.org","subject":"Re: [PATCH] Re-re-re-fix common tail optimization","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-20T09:40:24Z","receivedAt":"2007-12-20T09:40:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charles Bailey <charles@hashpling.org> writes:\n\n> Perhaps a comment saying tha zc is designed for <= 9999 z's?  Given\n> this, a lot of the /g are redundant.\n\nYeah, /g redundancy is a leftover from the time when it did not have\nthese sawtooth patterns.\n"}]}