{"thread":{"id":"21404","subject":"[PATCH 3/3] diff: fix the location of hunk headers for \"git diff --color-words -U0\"","startedAt":"2009-10-28T12:24:29Z","lastAt":"2009-10-31T11:48:57Z","messageCount":14,"participants":["Markus Heidelberg","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"126140","messageId":"1256732672-11817-1-git-send-email-markus.heidelberg@web.de","threadId":"21404","inReplyTo":null,"subject":"[PATCH 0/3] fix \"git diff --color-words -U0\"","fromName":"Markus Heidelberg","fromEmail":"markus.heidelberg@web.de","sentAt":"2009-10-28T12:24:29Z","receivedAt":"2009-10-28T12:24:29Z","isPatch":true,"sender":{"key":"markus.heidelberg@web.de","avatar":"https://avatars.githubusercontent.com/u/6334512?v=4"},"body":"The wrong output roughly is as follows:\n\n@@ -a,b +c,d @@\n@@ -e,f +g,h @@\nsome red, green and black text\nmore red, green and black text\n\nWhen it should be:\n\n@@ -a,b +c,d @@\nsome red, green and black text\n@@ -e,f +g,h @@\nmore red, green and black text\n\n\nMarkus Heidelberg (3):\n t4034-diff-words: add a test for word diff without context\n diff: move the handling of the hunk header after the changed lines\n diff: fix the location of hunk headers for \"git diff --color-words\n -U0\"\n\n diff.c | 40 +++++++++++++++++++++++-----------------\n t/t4034-diff-words.sh | 20 ++++++++++++++++++++\n 2 files changed, 43 insertions(+), 17 deletions(-)\n"},{"id":"126139","messageId":"1256732672-11817-2-git-send-email-markus.heidelberg@web.de","threadId":"21404","inReplyTo":"1256732672-11817-1-git-send-email-markus.heidelberg@web.de","subject":"[PATCH 1/3] t4034-diff-words: add a test for word diff without context","fromName":"Markus Heidelberg","fromEmail":"markus.heidelberg@web.de","sentAt":"2009-10-28T12:24:30Z","receivedAt":"2009-10-28T12:24:30Z","isPatch":true,"sender":{"key":"markus.heidelberg@web.de","avatar":"https://avatars.githubusercontent.com/u/6334512?v=4"},"body":"\nSigned-off-by: Markus Heidelberg \n---\n t/t4034-diff-words.sh | 20 ++++++++++++++++++++\n 1 files changed, 20 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh\nindex 4508eff..82240cf 100755\n--- a/t/t4034-diff-words.sh\n+++ b/t/t4034-diff-words.sh\n@@ -68,6 +68,26 @@ cat > expect <<\\EOF\n index 330b04f..5ed8eff 100644\n --- a/pre\n +++ b/post\n+@@ -1 +1 @@\n+h(4)h(4),hh[44]\n+@@ -3,0 +4,4 @@ a = b + c\n+\n+aa = a\n+\n+aeff = aeff * ( aaa )\n+EOF\n+\n+test_expect_failure 'word diff without context' '\n+\n+\tword_diff --color-words --unified=0\n+\n+'\n+\n+cat > expect <<\\EOF\n+diff --git a/pre b/post\n+index 330b04f..5ed8eff 100644\n+--- a/pre\n++++ b/post\n @@ -1,3 +1,7 @@\n h(4),hh[44]\n \n-- \n1.6.5.2.86.g61663\n"},{"id":"126138","messageId":"1256732672-11817-3-git-send-email-markus.heidelberg@web.de","threadId":"21404","inReplyTo":"1256732672-11817-1-git-send-email-markus.heidelberg@web.de","subject":"[PATCH 2/3] diff: move the handling of the hunk header after the changed lines","fromName":"Markus Heidelberg","fromEmail":"markus.heidelberg@web.de","sentAt":"2009-10-28T12:24:31Z","receivedAt":"2009-10-28T12:24:31Z","isPatch":true,"sender":{"key":"markus.heidelberg@web.de","avatar":"https://avatars.githubusercontent.com/u/6334512?v=4"},"body":"In preparation for a special case handling of colored word diff without\ncontext.\n\nSigned-off-by: Markus Heidelberg \n---\n diff.c | 41 +++++++++++++++++++++--------------------\n 1 files changed, 21 insertions(+), 20 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex b0c7e61..067e5a0 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -771,17 +771,6 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)\n \t\tlen = 1;\n \t}\n \n-\tif (line[0] == '@') {\n-\t\tlen = sane_truncate_line(ecbdata, line, len);\n-\t\tfind_lno(line, ecbdata);\n-\t\temit_line(ecbdata->file,\n-\t\t\t diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO),\n-\t\t\t reset, line, len);\n-\t\tif (line[len-1] != '\\n')\n-\t\t\tputc('\\n', ecbdata->file);\n-\t\treturn;\n-\t}\n-\n \tif (len < 1) {\n \t\temit_line(ecbdata->file, reset, reset, line, len);\n \t\treturn;\n@@ -796,17 +785,18 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)\n \t\t\tdiff_words_append(line, len,\n \t\t\t\t\t &ecbdata->diff_words->plus);\n \t\t\treturn;\n+\t\t} else if (line[0] == ' ') {\n+\t\t\tif (ecbdata->diff_words->minus.text.size ||\n+\t\t\t ecbdata->diff_words->plus.text.size)\n+\t\t\t\tdiff_words_show(ecbdata->diff_words);\n+\t\t\tline++;\n+\t\t\tlen--;\n+\t\t\temit_line(ecbdata->file, plain, reset, line, len);\n+\t\t\treturn;\n \t\t}\n-\t\tif (ecbdata->diff_words->minus.text.size ||\n-\t\t ecbdata->diff_words->plus.text.size)\n-\t\t\tdiff_words_show(ecbdata->diff_words);\n-\t\tline++;\n-\t\tlen--;\n-\t\temit_line(ecbdata->file, plain, reset, line, len);\n-\t\treturn;\n \t}\n \n-\tif (line[0] != '+') {\n+\tif (line[0] == ' ' || line[0] == '-') {\n \t\tconst char *color =\n \t\t\tdiff_get_color(ecbdata->color_diff,\n \t\t\t\t line[0] == '-' ? DIFF_FILE_OLD : DIFF_PLAIN);\n@@ -814,10 +804,21 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)\n \t\tif (line[0] == ' ')\n \t\t\tecbdata->lno_in_postimage++;\n \t\temit_line(ecbdata->file, color, reset, line, len);\n-\t} else {\n+\t\treturn;\n+\t} else if (line[0] == '+') {\n \t\tecbdata->lno_in_postimage++;\n \t\temit_add_line(reset, ecbdata, line + 1, len - 1);\n+\t\treturn;\n \t}\n+\n+\t/* line[0] == '@' */\n+\tlen = sane_truncate_line(ecbdata, line, len);\n+\tfind_lno(line, ecbdata);\n+\temit_line(ecbdata->file,\n+\t\t diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO),\n+\t\t reset, line, len);\n+\tif (line[len-1] != '\\n')\n+\t\tputc('\\n', ecbdata->file);\n }\n \n static char *pprint_rename(const char *a, const char *b)\n-- \n1.6.5.2.86.g61663\n"},{"id":"126137","messageId":"1256732672-11817-4-git-send-email-markus.heidelberg@web.de","threadId":"21404","inReplyTo":"1256732672-11817-1-git-send-email-markus.heidelberg@web.de","subject":"[PATCH 3/3] diff: fix the location of hunk headers for \"git diff --color-words -U0\"","fromName":"Markus Heidelberg","fromEmail":"markus.heidelberg@web.de","sentAt":"2009-10-28T12:24:32Z","receivedAt":"2009-10-28T12:24:32Z","isPatch":true,"sender":{"key":"markus.heidelberg@web.de","avatar":"https://avatars.githubusercontent.com/u/6334512?v=4"},"body":"Colored word diff without context lines firstly printed all the hunk\nheaders among each other and then printed the diff.\n\nBecause the word diff cannot be calculated before the end of the diff\n(added/removed lines) hunk, it was calculated directly before first line\nof context after the diff. But this didn't work if there was no context.\nIn this case the diff wasn't printed in fn_out_consume(), but entirely\nin free_diff_words_data(). This also led to calculate the colored diff\nfrom the whole diff in one swoop instead of calculating it in several\nindependent steps (one step per hunk).\n\nWe now calculate and print the word diff directly before the next hunk\nheader. The word diff of the last hunk is still printed in\nfree_diff_words_data().\n\nSigned-off-by: Markus Heidelberg \n---\n diff.c | 13 +++++++++----\n t/t4034-diff-words.sh | 2 +-\n 2 files changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 067e5a0..e95fe9b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -785,10 +785,15 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)\n \t\t\tdiff_words_append(line, len,\n \t\t\t\t\t &ecbdata->diff_words->plus);\n \t\t\treturn;\n-\t\t} else if (line[0] == ' ') {\n-\t\t\tif (ecbdata->diff_words->minus.text.size ||\n-\t\t\t ecbdata->diff_words->plus.text.size)\n-\t\t\t\tdiff_words_show(ecbdata->diff_words);\n+\t\t}\n+\t\t/*\n+\t\t * If line[0] == '@' then this prints the content of the\n+\t\t * previous hunk, necessary for 0-context.\n+\t\t */\n+\t\tif (ecbdata->diff_words->minus.text.size ||\n+\t\t ecbdata->diff_words->plus.text.size)\n+\t\t\tdiff_words_show(ecbdata->diff_words);\n+\t\tif (line[0] == ' ') {\n \t\t\tline++;\n \t\t\tlen--;\n \t\t\temit_line(ecbdata->file, plain, reset, line, len);\ndiff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh\nindex 82240cf..21db6e9 100755\n--- a/t/t4034-diff-words.sh\n+++ b/t/t4034-diff-words.sh\n@@ -77,7 +77,7 @@ cat > expect <<\\EOF\n aeff = aeff * ( aaa )\n EOF\n \n-test_expect_failure 'word diff without context' '\n+test_expect_success 'word diff without context' '\n \n \tword_diff --color-words --unified=0\n \n-- \n1.6.5.2.86.g61663\n"},{"id":"126171","messageId":"7v7hufo07i.fsf@alter.siamese.dyndns.org","threadId":"21404","inReplyTo":"1256732672-11817-1-git-send-email-markus.heidelberg@web.de","subject":"Re: [PATCH 0/3] fix \"git diff --color-words -U0\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-28T18:14:09Z","receivedAt":"2009-10-28T18:14:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Is this a serious enough breakage that deserves to be fixed in the\nmaintenance track (1.6.5.X)?\n"},{"id":"126183","messageId":"200910282321.46585.markus.heidelberg@web.de","threadId":"21404","inReplyTo":"7v7hufo07i.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/3] fix \"git diff --color-words -U0\"","fromName":"Markus Heidelberg","fromEmail":"markus.heidelberg@web.de","sentAt":"2009-10-28T22:21:45Z","receivedAt":"2009-10-28T22:21:45Z","isPatch":true,"sender":{"key":"markus.heidelberg@web.de","avatar":"https://avatars.githubusercontent.com/u/6334512?v=4"},"body":"Junio C Hamano, 28.10.2009:\n> Is this a serious enough breakage that deserves to be fixed in the\n> maintenance track (1.6.5.X)?\n\nThis problem exists since the introduction of this feature over three\nyears ago and apparently nobody complained so far. So I don't think it's\noverly serious.\n\nOTOH, depending on the change the produced diff can be totally wrong.\nFound a good example: c5022f576aa583429c245054d8600564b788ff33\nCompare \"--color-words -U0\" with \"--color-words -U1\".\n"},{"id":"126259","messageId":"alpine.DEB.1.00.0910291144170.3687@felix-maschine","threadId":"21404","inReplyTo":"1256732672-11817-4-git-send-email-markus.heidelberg@web.de","subject":"[PATCH] diff --color-words -U0: fix the location of hunk headers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-10-29T10:45:03Z","receivedAt":"2009-10-29T10:45:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nColored word diff without context lines firstly printed all the hunk\nheaders among each other and then printed the diff.\n\nThis was due to the code relying on getting at least one context line at\nthe end of each hunk, where the colored words would be flushed (it is\ndone that way to be able to ignore rewrapped lines).\n\nNoticed by Markus Heidelberg.\n\nSigned-off-by: Johannes Schindelin \n---\n\n\tI would strongly prefer this fix instead of your 2/3 and 3/3.\n\n diff.c | 6 ++++++\n t/t4034-diff-words.sh | 2 +-\n 2 files changed, 7 insertions(+), 1 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 51b5dbb..4eafaf5 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -656,6 +656,12 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)\n \tfor (i = 0; i < len && line[i] == '@'; i++)\n \t\t;\n \tif (2 <= i && i < len && line[i] == ' ') {\n+\t\t/* flush --color-words even for --unified=0 */\n+\t\tif (ecbdata->diff_words &&\n+\t\t (ecbdata->diff_words->minus.text.size ||\n+\t\t ecbdata->diff_words->plus.text.size))\n+\t\t\tdiff_words_show(ecbdata->diff_words);\n+\n \t\tecbdata->nparents = i - 1;\n \t\tlen = sane_truncate_line(ecbdata, line, len);\n \t\temit_line(ecbdata->file,\ndiff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh\nindex 82240cf..21db6e9 100755\n--- a/t/t4034-diff-words.sh\n+++ b/t/t4034-diff-words.sh\n@@ -77,7 +77,7 @@ cat > expect <<\\EOF\n aeff = aeff * ( aaa )\n EOF\n \n-test_expect_failure 'word diff without context' '\n+test_expect_success 'word diff without context' '\n \n \tword_diff --color-words --unified=0\n \n-- \n1.6.4.GIT\n"},{"id":"126262","messageId":"200910291222.42598.markus.heidelberg@web.de","threadId":"21404","inReplyTo":"alpine.DEB.1.00.0910291144170.3687@felix-maschine","subject":"Re: [PATCH] diff --color-words -U0: fix the location of hunk headers","fromName":"Markus Heidelberg","fromEmail":"markus.heidelberg@web.de","sentAt":"2009-10-29T11:22:41Z","receivedAt":"2009-10-29T11:22:41Z","isPatch":true,"sender":{"key":"markus.heidelberg@web.de","avatar":"https://avatars.githubusercontent.com/u/6334512?v=4"},"body":"Johannes Schindelin, 29.10.2009:\n> \n> \tI would strongly prefer this fix instead of your 2/3 and 3/3.\n> \n> diff.c | 6 ++++++\n> t/t4034-diff-words.sh | 2 +-\n> 2 files changed, 7 insertions(+), 1 deletions(-)\n> \n> diff --git a/diff.c b/diff.c\n> index 51b5dbb..4eafaf5 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -656,6 +656,12 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)\n> \tfor (i = 0; i < len && line[i] == '@'; i++)\n> \t\t;\n> \tif (2 <= i && i < len && line[i] == ' ') {\n> +\t\t/* flush --color-words even for --unified=0 */\n> +\t\tif (ecbdata->diff_words &&\n> +\t\t (ecbdata->diff_words->minus.text.size ||\n> +\t\t ecbdata->diff_words->plus.text.size))\n> +\t\t\tdiff_words_show(ecbdata->diff_words);\n> +\n> \t\tecbdata->nparents = i - 1;\n> \t\tlen = sane_truncate_line(ecbdata, line, len);\n> \t\temit_line(ecbdata->file,\n\nThis seems to apply before commit b8d9c1a (diff.c: the builtin_diff()\ndeals with only two-file comparison, 2009-09-03).\n\nIndeed my initial fix was in the same fashion:\n\n@@ -772,6 +772,15 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)\n }\n\n if (line[0] == '@') {\n+ if (ecbdata->diff_words) {\n+ /*\n+ * The content of the previous hunk, necessary for\n+ * 0-context.\n+ */\n+ if (ecbdata->diff_words->minus.text.size ||\n+ ecbdata->diff_words->plus.text.size)\n+ diff_words_show(ecbdata->diff_words);\n+ }\n len = sane_truncate_line(ecbdata, line, len);\n find_lno(line, ecbdata);\n emit_line(ecbdata->file,\n\nBut then I thought I should not put the diff output from --color-words\ninto the block that deals with the hunk header, but save another place\nwhere diff_words_show() is called.\n\nMarkus\n"},{"id":"126270","messageId":"alpine.DEB.1.00.0910291425010.3687@felix-maschine","threadId":"21404","inReplyTo":"200910291222.42598.markus.heidelberg@web.de","subject":"Re: [PATCH] diff --color-words -U0: fix the location of hunk headers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-10-29T13:27:18Z","receivedAt":"2009-10-29T13:27:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 29 Oct 2009, Markus Heidelberg wrote:\n\n> Johannes Schindelin, 29.10.2009:\n> > \n> > \tI would strongly prefer this fix instead of your 2/3 and 3/3.\n> > \n> > diff.c | 6 ++++++\n> > t/t4034-diff-words.sh | 2 +-\n> > 2 files changed, 7 insertions(+), 1 deletions(-)\n> > \n> > diff --git a/diff.c b/diff.c\n> > index 51b5dbb..4eafaf5 100644\n> > --- a/diff.c\n> > +++ b/diff.c\n> > @@ -656,6 +656,12 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)\n> > \tfor (i = 0; i < len && line[i] == '@'; i++)\n> > \t\t;\n> > \tif (2 <= i && i < len && line[i] == ' ') {\n> > +\t\t/* flush --color-words even for --unified=0 */\n> > +\t\tif (ecbdata->diff_words &&\n> > +\t\t (ecbdata->diff_words->minus.text.size ||\n> > +\t\t ecbdata->diff_words->plus.text.size))\n> > +\t\t\tdiff_words_show(ecbdata->diff_words);\n> > +\n> > \t\tecbdata->nparents = i - 1;\n> > \t\tlen = sane_truncate_line(ecbdata, line, len);\n> > \t\temit_line(ecbdata->file,\n> \n> This seems to apply before commit b8d9c1a (diff.c: the builtin_diff()\n> deals with only two-file comparison, 2009-09-03).\n\nYes, sorry, for some reason I worked on a machine where I do not work from \njunio's next, but my own fork (which is outdated due to lack of time).\n\n> Indeed my initial fix was in the same fashion:\n> \n> @@ -772,6 +772,15 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)\n> }\n> \n> if (line[0] == '@') {\n> + if (ecbdata->diff_words) {\n> + /*\n> + * The content of the previous hunk, necessary for\n> + * 0-context.\n> + */\n> + if (ecbdata->diff_words->minus.text.size ||\n> + ecbdata->diff_words->plus.text.size)\n> + diff_words_show(ecbdata->diff_words);\n> + }\n> len = sane_truncate_line(ecbdata, line, len);\n> find_lno(line, ecbdata);\n> emit_line(ecbdata->file,\n> \n> But then I thought I should not put the diff output from --color-words\n> into the block that deals with the hunk header, but save another place\n> where diff_words_show() is called.\n\nI found this paragraph, as well as the patches 2/3 and 3/3, hard to \nfollow.\n\nAnd besides, flushing in that block is the correct thing to do. The \nfunction diff_words_show() is a function for that exact purpose.\n\nCiao,\nDscho\n"},{"id":"126289","messageId":"200910291729.23562.markus.heidelberg@web.de","threadId":"21404","inReplyTo":"alpine.DEB.1.00.0910291425010.3687@felix-maschine","subject":"Re: [PATCH] diff --color-words -U0: fix the location of hunk headers","fromName":"Markus Heidelberg","fromEmail":"markus.heidelberg@web.de","sentAt":"2009-10-29T16:29:23Z","receivedAt":"2009-10-29T16:29:23Z","isPatch":true,"sender":{"key":"markus.heidelberg@web.de","avatar":"https://avatars.githubusercontent.com/u/6334512?v=4"},"body":"Johannes Schindelin, 29.10.2009:\n> Hi,\n> \n> On Thu, 29 Oct 2009, Markus Heidelberg wrote:\n> \n> > Indeed my initial fix was in the same fashion:\n> > \n> > @@ -772,6 +772,15 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)\n> > }\n> > \n> > if (line[0] == '@') {\n> > + if (ecbdata->diff_words) {\n> > + /*\n> > + * The content of the previous hunk, necessary for\n> > + * 0-context.\n> > + */\n> > + if (ecbdata->diff_words->minus.text.size ||\n> > + ecbdata->diff_words->plus.text.size)\n> > + diff_words_show(ecbdata->diff_words);\n> > + }\n> > len = sane_truncate_line(ecbdata, line, len);\n> > find_lno(line, ecbdata);\n> > emit_line(ecbdata->file,\n> > \n> > But then I thought I should not put the diff output from --color-words\n> > into the block that deals with the hunk header, but save another place\n> > where diff_words_show() is called.\n> \n> I found this paragraph, as well as the patches 2/3 and 3/3, hard to \n> follow.\n\nI try to reword:\nWith 2/3 and 3/3 I wanted to keep --color-words specific code in the\nblock starting with\n\n\tif (ecbdata->diff_words) {\n\nand didn't want to contaminate the block starting with\n\n\tif (line[0] == '@') {\n\nwith non-hunk-header content.\n\nBut I'm not sure what's the better way and am content with either.\n\n> And besides, flushing in that block is the correct thing to do. The \n> function diff_words_show() is a function for that exact purpose.\n\nYes, 2/3 and 3/3 just don't introduce a new invocation of this function\nat another place in the code.\n\nMarkus\n"},{"id":"126395","messageId":"7v3a50n6hw.fsf@alter.siamese.dyndns.org","threadId":"21404","inReplyTo":"200910291729.23562.markus.heidelberg@web.de","subject":"Re: [PATCH] diff --color-words -U0: fix the location of hunk headers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-30T17:20:27Z","receivedAt":"2009-10-30T17:20:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Markus Heidelberg writes:\n\n> I try to reword:\n> With 2/3 and 3/3 I wanted to keep --color-words specific code in the\n> block starting with\n>\n> \tif (ecbdata->diff_words) {\n>\n> and didn't want to contaminate the block starting with\n>\n> \tif (line[0] == '@') {\n>\n> with non-hunk-header content.\n\nThe contamination was already done long time ago. The way \"color words\"\nmode piggybacks into the rest of the code is to let the line oriented diff\ncodepath run, capture the lines to its buffer so that it can split them\ninto words and compare, and hijack the control flow not to let the line\noriented diff to be output, and in the context of the original design,\nDscho's patch makes sense.\n\nI do think that the way the \"color words\" logic has to touch line-oriented\ncodepaths unnecessarily makes it look intrusive; one reason is because it\nexposes the state that is only interesting to the \"color words\" mode to\nthese places too much.\n\nWith a small helper function on top of Dscho's patch, I think the code can\nbe made a lot more readable. This way, \"consume\" codepath is made more\nabout \"here is what we do when we get a line from the line-oriented diff\nengine\", and we can keep the knowledge of \"color words\" mode to the\nminimum (no more than \"here is where we may need to flush the buffer\").\nThe helper hides the implementation details such as how we decide if we\nhave accumulated words or what we do when we do need to flush.\n\nThere is another large-ish \"if (ecbdata->diff_words)\" codeblock in\nfn_out_consume() that peeks into *(ecbdata->diff_words); I think it should\nbe ejected from the main codepath for the same reason (i.e. readability).\n.\nProbably we can make a helper function that has only a single caller, like\nthis:\n\n static void diff_words_use_line(char *line, unsigned long len,\n struct emit_callback *ecbdata,\n const char *plain, const char *reset)\n {\n if (line[0] == '-') {\n diff_words_append(line, len,\n &ecbdata->diff_words->minus);\n return;\n } else if (line[0] == '+') {\n diff_words_append(line, len,\n &ecbdata->diff_words->plus);\n return;\n }\n diff_words_flush(ecbdata);\n line++;\n len--;\n emit_line(ecbdata->file, plain, reset, line, len);\n }\n\nIt would even be cleaner to change \"diff_words_append()\" function to do\nall of the above.\n\nBut that is outside the scope of this comment.\n\n\n diff.c | 26 ++++++++++++--------------\n 1 files changed, 12 insertions(+), 14 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex b7ecfe3..8c66e4a 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -541,14 +541,18 @@ struct emit_callback {\n \tFILE *file;\n };\n \n+/* In \"color-words\" mode, show word-diff of words accumulated in the buffer */\n+static void diff_words_flush(struct emit_callback *ecbdata)\n+{\n+\tif (ecbdata->diff_words->minus.text.size ||\n+\t ecbdata->diff_words->plus.text.size)\n+\t\tdiff_words_show(ecbdata->diff_words);\n+}\n+\n static void free_diff_words_data(struct emit_callback *ecbdata)\n {\n \tif (ecbdata->diff_words) {\n-\t\t/* flush buffers */\n-\t\tif (ecbdata->diff_words->minus.text.size ||\n-\t\t\t\tecbdata->diff_words->plus.text.size)\n-\t\t\tdiff_words_show(ecbdata->diff_words);\n-\n+\t\tdiff_words_flush(ecbdata);\n \t\tfree (ecbdata->diff_words->minus.text.ptr);\n \t\tfree (ecbdata->diff_words->minus.orig);\n \t\tfree (ecbdata->diff_words->plus.text.ptr);\n@@ -656,12 +660,8 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)\n \tfor (i = 0; i < len && line[i] == '@'; i++)\n \t\t;\n \tif (2 <= i && i < len && line[i] == ' ') {\n-\t\t/* flush --color-words even for --unified=0 */\n-\t\tif (ecbdata->diff_words &&\n-\t\t (ecbdata->diff_words->minus.text.size ||\n-\t\t ecbdata->diff_words->plus.text.size))\n-\t\t\tdiff_words_show(ecbdata->diff_words);\n-\n+\t\tif (ecbdata->diff_words)\n+\t\t\tdiff_words_flush(ecbdata);\n \t\tecbdata->nparents = i - 1;\n \t\tlen = sane_truncate_line(ecbdata, line, len);\n \t\temit_line(ecbdata->file,\n@@ -691,9 +691,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)\n \t\t\t\t\t &ecbdata->diff_words->plus);\n \t\t\treturn;\n \t\t}\n-\t\tif (ecbdata->diff_words->minus.text.size ||\n-\t\t ecbdata->diff_words->plus.text.size)\n-\t\t\tdiff_words_show(ecbdata->diff_words);\n+\t\tdiff_words_flush(ecbdata);\n \t\tline++;\n \t\tlen--;\n \t\temit_line(ecbdata->file, plain, reset, line, len);\n"},{"id":"126397","messageId":"alpine.DEB.1.00.0910301831190.5383@felix-maschine","threadId":"21404","inReplyTo":"7v3a50n6hw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] diff --color-words -U0: fix the location of hunk headers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-10-30T17:32:21Z","receivedAt":"2009-10-30T17:32:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 30 Oct 2009, Junio C Hamano wrote:\n\n> Markus Heidelberg writes:\n> \n> > I try to reword:\n> > With 2/3 and 3/3 I wanted to keep --color-words specific code in the\n> > block starting with\n> >\n> > \tif (ecbdata->diff_words) {\n> >\n> > and didn't want to contaminate the block starting with\n> >\n> > \tif (line[0] == '@') {\n> >\n> > with non-hunk-header content.\n> \n> The contamination was already done long time ago.\n\nActually, it was don on purpose.\n\n> diff --git a/diff.c b/diff.c\n> index b7ecfe3..8c66e4a 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -541,14 +541,18 @@ struct emit_callback {\n> \tFILE *file;\n> };\n> \n> +/* In \"color-words\" mode, show word-diff of words accumulated in the buffer */\n> +static void diff_words_flush(struct emit_callback *ecbdata)\n> +{\n> +\tif (ecbdata->diff_words->minus.text.size ||\n> +\t ecbdata->diff_words->plus.text.size)\n> +\t\tdiff_words_show(ecbdata->diff_words);\n> +}\n\nInstead of this function, you can check the same at the beginning of \ndiff_words_show().\n\nThe reason I did not do that was to avoid a full subroutine call, as I \nexpected this code path to be very expensive.\n\nCiao,\nDscho\n"},{"id":"126404","messageId":"7vr5sklo7c.fsf@alter.siamese.dyndns.org","threadId":"21404","inReplyTo":"alpine.DEB.1.00.0910301831190.5383@felix-maschine","subject":"Re: [PATCH] diff --color-words -U0: fix the location of hunk headers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-30T18:40:55Z","receivedAt":"2009-10-30T18:40:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> The reason I did not do that was to avoid a full subroutine call, as I \n> expected this code path to be very expensive.\n\nThis is only done for the \"word diff\" mode, and my gut feeling is that it\nis not such a big issue. But you can always inline it if it is\nperformance critical.\n\nThe current structure shows how this code evolved. fn_out_consume() used\nto be \"this is called every time a line worth of diff becomes ready from\nthe lower-level diff routine, and here is what we do to output line\noriented diff using that line.\"\n\nWhen we introduced the \"word diff\" mode, we could have done three things.\n\n * change fn_out_consume() to \"this is called every time a line worth of\n diff becomes ready from the lower-level diff routine. This function\n knows two sets of helpers (one for line-oriented diff, another for word\n diff), and each set has various functions to be called at certain\n places (e.g. hunk header, context, ...). The function's role is to\n inspect the incoming line, and dispatch appropriate helpers to produce\n either line- or word- oriented diff output.\"\n\n * introduce fn_out_consume_word_diff() that is \"this is called every time\n a line worth of diff becomes ready from the lower-level diff routine,\n and here is what we do to prepare word oriented diff using that line.\"\n without touching fn_out_consume() at all.\n\n * Do neither of the above, and keep fn_out_consume() to \"this is called\n every time a line worth of diff becomes ready from the lower-level diff\n routine, and here is what we do to output line oriented diff using that\n line.\" but sprinkle a handful of 'are we in word-diff mode? if so do\n this totally different thing' at random places.\n\nMy clean-up \"something like this\" patch was to at least abstract the\ndetails of \"this totally different thing\" out from the main codepath, in\norder to improve readability.\n\nWe can of course refactor this by introducing fn_out_consume_word_diff(),\ni.e. the second route above. Then we do not have to check \"are we in word\ndiff mode\" for every line of diff and that would give you even better\nperformance.\n"},{"id":"126519","messageId":"alpine.DEB.1.00.0910311247590.4985@pacific.mpi-cbg.de","threadId":"21404","inReplyTo":"7vr5sklo7c.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] diff --color-words -U0: fix the location of hunk headers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-10-31T11:48:57Z","receivedAt":"2009-10-31T11:48:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 30 Oct 2009, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > The reason I did not do that was to avoid a full subroutine call, as I \n> > expected this code path to be very expensive.\n> \n> This is only done for the \"word diff\" mode, and my gut feeling is that it\n> is not such a big issue.\n\nYeah, sorry, I should have stated explicitely that I no longer think that \nthere is a performance issue.\n\nCiao,\nDscho\n"}]}