{"thread":{"id":"44071","subject":"[RFC/PATCH 00/17]","startedAt":"2016-09-13T04:46:24Z","lastAt":"2016-09-13T23:42:53Z","messageCount":28,"participants":["Stefan Beller","René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":17},"messages":[{"id":"301817","messageId":"20160913044613.1037-1-sbeller@google.com","threadId":"44071","inReplyTo":null,"subject":"[RFC/PATCH 00/17]","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-13T04:45:56Z","receivedAt":"2016-09-13T04:46:24Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"About a week ago, I started experimenting to add a new functionality to the \ndiff machinery: We want to color moved lines differently! This will aid developers\nto review patches such as the patch \"Move libified code from builtin/apply.c to apply.{c,h}\"\nOr a smaller example:\n\n git show 11979b9\n \n\nIn the original patch series I just used the xdl diff machinery multiple times\nto obtain the output for a multi pass algorithm I need for this coloring.\nThis was one of Junios main concerns (We have to rely on the xdl stuff to \nyield the same output in the two passes).\n\nThis brings in another approach:\n * run the xdl stuff only once\n * In case we do not ask for coloring moves, put it out immediately\n (small detour: we do not use fprintf/fputs directly but we channel\n all output through the emit_line_* interface)\n * When asking for coloring moves: buffer all its output for now.\n Feed the line by line into the move detection algorithm.\n * then output it all at once using the information from the move detection\n to recolor moves while outputting.\n \nWhile viewing the output of 11979b9 again, I notice a small bug in the proposal,\nbut I'd rather be asking for review of the whole design approach.\n \nThanks,\nStefan\n\nStefan Beller (17):\n diff: move line ending check into emit_hunk_header\n diff: emit_{add, del, context}_line to increase {pre,post}image line\n count\n diff.c: drop tautologous condition in emit_line_0\n diff.c: factor out diff_flush_patch_all_file_pairs\n diff.c: emit_line_0 can handle no color setting\n diff.c: convert fn_out_consume to use emit_line_*\n diff.c: convert builtin_diff to use emit_line_*\n diff.c: convert emit_rewrite_diff to use emit_line_*\n diff.c: convert emit_rewrite_lines to use emit_line_*\n submodule.c: convert show_submodule_summary to use emit_line_fmt\n diff.c: convert emit_binary_diff_body to use emit_line_*\n diff.c: convert show_stats to use emit_line_*\n diff.c: convert word diffing to use emit_line_*\n diff.c: convert diff_flush to use emit_line_*\n diff.c: convert diff_summary to use emit_line_*\n diff: buffer output in emit_line_0\n diff.c: color moved lines differently\n\n Documentation/config.txt | 12 +-\n Documentation/diff-options.txt | 7 +\n contrib/completion/git-completion.bash | 2 +\n diff.c | 666 +++++++++++++++++++++++----------\n diff.h | 35 +-\n submodule.c | 42 +--\n submodule.h | 3 +-\n t/t4015-diff-whitespace.sh | 229 ++++++++++++\n 8 files changed, 760 insertions(+), 236 deletions(-)\n\n-- \n2.10.0.21.g1da280f.dirty\n\n"},{"id":"301818","messageId":"20160913044613.1037-2-sbeller@google.com","threadId":"44071","inReplyTo":"20160913044613.1037-1-sbeller@google.com","subject":"[RFC/PATCH 01/17] diff: move line ending check into emit_hunk_header","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-13T04:45:57Z","receivedAt":"2016-09-13T04:46:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In a later patch, I want to propose an option to detect&color\nmoved lines in a diff, which cannot be done in a one-pass over\nthe diff. Instead we need to go over the whole diff twice,\nbecause we cannot detect the first line of the two corresponding\nlines (+ and -) that got moved.\n\nSo to prepare the diff machinery for two pass algorithms\n(i.e. buffer it all up and then operate on the result),\nmove all emissions to places, such that the only emitting\nfunction is emit_line_0.\n\nThis patch moves code that is conceptually part of\nemit_hunk_header, but was using output in fn_out_consume,\nback to emit_hunk_header.\n\nSigned-off-by: Stefan Beller \n---\n diff.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex cc8e812..aa50b2d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -610,6 +610,9 @@ static void emit_hunk_header(struct emit_callback *ecbdata,\n \t}\n \n \tstrbuf_add(&msgbuf, line + len, org_len - len);\n+\tif (line[org_len - 1] != '\\n')\n+\t\tstrbuf_addch(&msgbuf, '\\n');\n+\n \temit_line(ecbdata->opt, \"\", \"\", msgbuf.buf, msgbuf.len);\n \tstrbuf_release(&msgbuf);\n }\n@@ -1247,8 +1250,6 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)\n \t\tlen = sane_truncate_line(ecbdata, line, len);\n \t\tfind_lno(line, ecbdata);\n \t\temit_hunk_header(ecbdata, line, len);\n-\t\tif (line[len-1] != '\\n')\n-\t\t\tputc('\\n', o->file);\n \t\treturn;\n \t}\n \n-- \n2.10.0.21.g1da280f.dirty\n\n"},{"id":"301819","messageId":"20160913044613.1037-3-sbeller@google.com","threadId":"44071","inReplyTo":"20160913044613.1037-1-sbeller@google.com","subject":"[RFC/PATCH 02/17] diff: emit_{add, del, context}_line to increase {pre,post}image line count","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-13T04:45:58Z","receivedAt":"2016-09-13T04:46:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"At all call sites of emit_{add, del, context}_line we increment the line\ncount, so move it into the respective functions to make the code at the\ncalling site a bit clearer.\n\nSigned-off-by: Stefan Beller \n---\n diff.c | 15 ++++++---------\n 1 file changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex aa50b2d..156c2aa 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -536,6 +536,7 @@ static void emit_add_line(const char *reset,\n \t\t\t struct emit_callback *ecbdata,\n \t\t\t const char *line, int len)\n {\n+\tecbdata->lno_in_postimage++;\n \temit_line_checked(reset, ecbdata, line, len,\n \t\t\t DIFF_FILE_NEW, WSEH_NEW, '+');\n }\n@@ -544,6 +545,7 @@ static void emit_del_line(const char *reset,\n \t\t\t struct emit_callback *ecbdata,\n \t\t\t const char *line, int len)\n {\n+\tecbdata->lno_in_preimage++;\n \temit_line_checked(reset, ecbdata, line, len,\n \t\t\t DIFF_FILE_OLD, WSEH_OLD, '-');\n }\n@@ -552,6 +554,8 @@ static void emit_context_line(const char *reset,\n \t\t\t struct emit_callback *ecbdata,\n \t\t\t const char *line, int len)\n {\n+\tecbdata->lno_in_postimage++;\n+\tecbdata->lno_in_preimage++;\n \temit_line_checked(reset, ecbdata, line, len,\n \t\t\t DIFF_CONTEXT, WSEH_CONTEXT, ' ');\n }\n@@ -662,13 +666,10 @@ static void emit_rewrite_lines(struct emit_callback *ecb,\n \n \t\tendp = memchr(data, '\\n', size);\n \t\tlen = endp ? (endp - data + 1) : size;\n-\t\tif (prefix != '+') {\n-\t\t\tecb->lno_in_preimage++;\n+\t\tif (prefix != '+')\n \t\t\temit_del_line(reset, ecb, data, len);\n-\t\t} else {\n-\t\t\tecb->lno_in_postimage++;\n+\t\telse\n \t\t\temit_add_line(reset, ecb, data, len);\n-\t\t}\n \t\tsize -= len;\n \t\tdata += len;\n \t}\n@@ -1293,16 +1294,12 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)\n \n \tswitch (line[0]) {\n \tcase '+':\n-\t\tecbdata->lno_in_postimage++;\n \t\temit_add_line(reset, ecbdata, line + 1, len - 1);\n \t\tbreak;\n \tcase '-':\n-\t\tecbdata->lno_in_preimage++;\n \t\temit_del_line(reset, ecbdata, line + 1, len - 1);\n \t\tbreak;\n \tcase ' ':\n-\t\tecbdata->lno_in_postimage++;\n-\t\tecbdata->lno_in_preimage++;\n \t\temit_context_line(reset, ecbdata, line + 1, len - 1);\n \t\tbreak;\n \tdefault:\n-- \n2.10.0.21.g1da280f.dirty\n\n"},{"id":"301820","messageId":"20160913044613.1037-4-sbeller@google.com","threadId":"44071","inReplyTo":"20160913044613.1037-1-sbeller@google.com","subject":"[RFC/PATCH 03/17] diff.c: drop tautologous condition in emit_line_0","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-13T04:45:59Z","receivedAt":"2016-09-13T04:46:33Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When `first` equals '\\r', then it cannot equal '\\n', which makes\n`!has_trailing_newline` always true if `first` is '\\r'.\nDrop that check.\n\nSigned-off-by: Stefan Beller \n---\n diff.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 156c2aa..9d2e704 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -460,8 +460,7 @@ static void emit_line_0(struct diff_options *o, const char *set, const char *res\n \n \tif (len == 0) {\n \t\thas_trailing_newline = (first == '\\n');\n-\t\thas_trailing_carriage_return = (!has_trailing_newline &&\n-\t\t\t\t\t\t(first == '\\r'));\n+\t\thas_trailing_carriage_return = (first == '\\r');\n \t\tnofirst = has_trailing_newline || has_trailing_carriage_return;\n \t} else {\n \t\thas_trailing_newline = (len > 0 && line[len-1] == '\\n');\n-- \n2.10.0.21.g1da280f.dirty\n\n"},{"id":"301821","messageId":"20160913044613.1037-5-sbeller@google.com","threadId":"44071","inReplyTo":"20160913044613.1037-1-sbeller@google.com","subject":"[RFC/PATCH 04/17] diff.c: factor out diff_flush_patch_all_file_pairs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-13T04:46:00Z","receivedAt":"2016-09-13T04:46:36Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In a later patch we want to do more things before and after all filepairs\nare flushed. So factor flushing out all file pairs into its own function\nthat the new code can be plugged in easily.\n\nSigned-off-by: Stefan Beller \n---\n diff.c | 17 ++++++++++++-----\n 1 file changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 9d2e704..b6a40ae 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4608,6 +4608,17 @@ void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc)\n \t\twarning(rename_limit_advice, varname, needed);\n }\n \n+static void diff_flush_patch_all_file_pairs(struct diff_options *o)\n+{\n+\tint i;\n+\tstruct diff_queue_struct *q = &diff_queued_diff;\n+\tfor (i = 0; i < q->nr; i++) {\n+\t\tstruct diff_filepair *p = q->queue[i];\n+\t\tif (check_pair_status(p))\n+\t\t\tdiff_flush_patch(p, o);\n+\t}\n+}\n+\n void diff_flush(struct diff_options *options)\n {\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n@@ -4702,11 +4713,7 @@ void diff_flush(struct diff_options *options)\n \t\t\t}\n \t\t}\n \n-\t\tfor (i = 0; i < q->nr; i++) {\n-\t\t\tstruct diff_filepair *p = q->queue[i];\n-\t\t\tif (check_pair_status(p))\n-\t\t\t\tdiff_flush_patch(p, options);\n-\t\t}\n+\t\tdiff_flush_patch_all_file_pairs(options);\n \t}\n \n \tif (output_format & DIFF_FORMAT_CALLBACK)\n-- \n2.10.0.21.g1da280f.dirty\n\n"},{"id":"301822","messageId":"20160913044613.1037-6-sbeller@google.com","threadId":"44071","inReplyTo":"20160913044613.1037-1-sbeller@google.com","subject":"[RFC/PATCH 05/17] diff.c: emit_line_0 can handle no color setting","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-13T04:46:01Z","receivedAt":"2016-09-13T04:46:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In a later patch, I want to propose an option to detect&color\nmoved lines in a diff, which cannot be done in a one-pass over\nthe diff. Instead we need to go over the whole diff twice,\nbecause we cannot detect the first line of the two corresponding\nlines (+ and -) that got moved.\n\nSo to prepare the diff machinery for two pass algorithms\n(i.e. buffer it all up and then operate on the result),\nmove all emissions to places, such that the only emitting\nfunction is emit_line_0.\n\nIn later patches we may pass lines that are not colored to\nthe central function emit_line_0, so we\nneed to emit the color only when it is non-NULL.\n\nSigned-off-by: Stefan Beller \n---\n diff.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex b6a40ae..5d57130 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -473,11 +473,13 @@ static void emit_line_0(struct diff_options *o, const char *set, const char *res\n \t}\n \n \tif (len || !nofirst) {\n-\t\tfputs(set, file);\n+\t\tif (set)\n+\t\t\tfputs(set, file);\n \t\tif (!nofirst)\n \t\t\tfputc(first, file);\n \t\tfwrite(line, len, 1, file);\n-\t\tfputs(reset, file);\n+\t\tif (reset)\n+\t\t\tfputs(reset, file);\n \t}\n \tif (has_trailing_carriage_return)\n \t\tfputc('\\r', file);\n-- \n2.10.0.21.g1da280f.dirty\n\n"},{"id":"301823","messageId":"20160913044613.1037-9-sbeller@google.com","threadId":"44071","inReplyTo":"20160913044613.1037-1-sbeller@google.com","subject":"[RFC/PATCH 08/17] diff.c: convert emit_rewrite_diff to use emit_line_*","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-13T04:46:04Z","receivedAt":"2016-09-13T04:46:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In a later patch, I want to propose an option to detect&color\nmoved lines in a diff, which cannot be done in a one-pass over\nthe diff. Instead we need to go over the whole diff twice,\nbecause we cannot detect the first line of the two corresponding\nlines (+ and -) that got moved.\n\nSo to prepare the diff machinery for two pass algorithms\n(i.e. buffer it all up and then operate on the result),\nmove all emissions to places, such that the only emitting\nfunction is emit_line_0.\n\nThis covers emit_rewrite_diff.\n\nSigned-off-by: Stefan Beller \n---\n diff.c | 33 ++++++++++++++++++---------------\n 1 file changed, 18 insertions(+), 15 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex cbd4ba7..ee4c9d4 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -653,17 +653,17 @@ static void remove_tempfile(void)\n \t}\n }\n \n-static void print_line_count(FILE *file, int count)\n+static void add_line_count(struct strbuf *out, int count)\n {\n \tswitch (count) {\n \tcase 0:\n-\t\tfprintf(file, \"0,0\");\n+\t\tstrbuf_addstr(out, \"0,0\");\n \t\tbreak;\n \tcase 1:\n-\t\tfprintf(file, \"1\");\n+\t\tstrbuf_addstr(out, \"1\");\n \t\tbreak;\n \tdefault:\n-\t\tfprintf(file, \"1,%d\", count);\n+\t\tstrbuf_addf(out, \"1,%d\", count);\n \t\tbreak;\n \t}\n }\n@@ -714,7 +714,7 @@ static void emit_rewrite_diff(const char *name_a,\n \tchar *data_one, *data_two;\n \tsize_t size_one, size_two;\n \tstruct emit_callback ecbdata;\n-\tconst char *line_prefix = diff_line_prefix(o);\n+\tstruct strbuf out = STRBUF_INIT;\n \n \tif (diff_mnemonic_prefix && DIFF_OPT_TST(o, REVERSE_DIFF)) {\n \t\ta_prefix = o->b_prefix;\n@@ -752,20 +752,23 @@ static void emit_rewrite_diff(const char *name_a,\n \tecbdata.lno_in_preimage = 1;\n \tecbdata.lno_in_postimage = 1;\n \n+\temit_line_fmt(o, metainfo, reset, \"--- %s%s\\n\", a_name.buf, name_a_tab);\n+\temit_line_fmt(o, metainfo, reset, \"+++ %s%s\\n\", b_name.buf, name_b_tab);\n+\n \tlc_a = count_lines(data_one, size_one);\n \tlc_b = count_lines(data_two, size_two);\n-\tfprintf(o->file,\n-\t\t\"%s%s--- %s%s%s\\n%s%s+++ %s%s%s\\n%s%s@@ -\",\n-\t\tline_prefix, metainfo, a_name.buf, name_a_tab, reset,\n-\t\tline_prefix, metainfo, b_name.buf, name_b_tab, reset,\n-\t\tline_prefix, fraginfo);\n+\n+\tstrbuf_addstr(&out, \"@@ -\");\n \tif (!o->irreversible_delete)\n-\t\tprint_line_count(o->file, lc_a);\n+\t\tadd_line_count(&out, lc_a);\n \telse\n-\t\tfprintf(o->file, \"?,?\");\n-\tfprintf(o->file, \" +\");\n-\tprint_line_count(o->file, lc_b);\n-\tfprintf(o->file, \" @@%s\\n\", reset);\n+\t\tstrbuf_addstr(&out, \"?,?\");\n+\tstrbuf_addstr(&out, \" +\");\n+\tadd_line_count(&out, lc_b);\n+\tstrbuf_addstr(&out, \" @@\\n\");\n+\temit_line(o, fraginfo, reset, out.buf, out.len);\n+\tstrbuf_release(&out);\n+\n \tif (lc_a && !o->irreversible_delete)\n \t\temit_rewrite_lines(&ecbdata, '-', data_one, size_one);\n \tif (lc_b)\n-- \n2.10.0.21.g1da280f.dirty\n\n"},{"id":"301824","messageId":"20160913044613.1037-8-sbeller@google.com","threadId":"44071","inReplyTo":"20160913044613.1037-1-sbeller@google.com","subject":"[RFC/PATCH 07/17] diff.c: convert builtin_diff to use emit_line_*","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-13T04:46:03Z","receivedAt":"2016-09-13T04:46:43Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In a later patch, I want to propose an option to detect&color\nmoved lines in a diff, which cannot be done in a one-pass over\nthe diff. Instead we need to go over the whole diff twice,\nbecause we cannot detect the first line of the two corresponding\nlines (+ and -) that got moved.\n\nSo to prepare the diff machinery for two pass algorithms\n(i.e. buffer it all up and then operate on the result),\nmove all emissions to places, such that the only emitting\nfunction is emit_line_0.\n\nThis covers builtin_diff.\n\nSigned-off-by: Stefan Beller \n---\n diff.c | 37 ++++++++++++++++++++-----------------\n 1 file changed, 20 insertions(+), 17 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex b4f4b75..cbd4ba7 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1234,8 +1234,8 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)\n \to->found_changes = 1;\n \n \tif (ecbdata->header) {\n-\t\tfprintf(o->file, \"%s\", ecbdata->header->buf);\n-\t\tstrbuf_reset(ecbdata->header);\n+\t\temit_line(o, 0, 0, ecbdata->header->buf, ecbdata->header->len);\n+\t\tstrbuf_release(ecbdata->header);\n \t\tecbdata->header = NULL;\n \t}\n \n@@ -2333,7 +2333,7 @@ static void builtin_diff(const char *name_a,\n \tb_two = quote_two(b_prefix, name_b + (*name_b == '/'));\n \tlbl[0] = DIFF_FILE_VALID(one) ? a_one : \"/dev/null\";\n \tlbl[1] = DIFF_FILE_VALID(two) ? b_two : \"/dev/null\";\n-\tstrbuf_addf(&header, \"%s%sdiff --git %s %s%s\\n\", line_prefix, meta, a_one, b_two, reset);\n+\tstrbuf_addf(&header, \"%sdiff --git %s %s%s\\n\", meta, a_one, b_two, reset);\n \tif (lbl[0][0] == '/') {\n \t\t/* /dev/null */\n \t\tstrbuf_addf(&header, \"%s%snew file mode %06o%s\\n\", line_prefix, meta, two->mode, reset);\n@@ -2365,7 +2365,7 @@ static void builtin_diff(const char *name_a,\n \t\tif (complete_rewrite &&\n \t\t (textconv_one || !diff_filespec_is_binary(one)) &&\n \t\t (textconv_two || !diff_filespec_is_binary(two))) {\n-\t\t\tfprintf(o->file, \"%s\", header.buf);\n+\t\t\temit_line(o, 0, 0, header.buf, header.len);\n \t\t\tstrbuf_reset(&header);\n \t\t\temit_rewrite_diff(name_a, name_b, one, two,\n \t\t\t\t\t\ttextconv_one, textconv_two, o);\n@@ -2375,7 +2375,8 @@ static void builtin_diff(const char *name_a,\n \t}\n \n \tif (o->irreversible_delete && lbl[1][0] == '/') {\n-\t\tfprintf(o->file, \"%s\", header.buf);\n+\t\tif (header.len)\n+\t\t\temit_line(o, 0, 0, header.buf, header.len);\n \t\tstrbuf_reset(&header);\n \t\tgoto free_ab_and_return;\n \t} else if (!DIFF_OPT_TST(o, TEXT) &&\n@@ -2385,13 +2386,14 @@ static void builtin_diff(const char *name_a,\n \t\t S_ISREG(one->mode) && S_ISREG(two->mode) &&\n \t\t !DIFF_OPT_TST(o, BINARY)) {\n \t\t\tif (!oidcmp(&one->oid, &two->oid)) {\n-\t\t\t\tif (must_show_header)\n-\t\t\t\t\tfprintf(o->file, \"%s\", header.buf);\n+\t\t\t\tif (must_show_header && header.len)\n+\t\t\t\t\temit_line(o, 0, 0, header.buf, header.len);\n \t\t\t\tgoto free_ab_and_return;\n \t\t\t}\n-\t\t\tfprintf(o->file, \"%s\", header.buf);\n-\t\t\tfprintf(o->file, \"%sBinary files %s and %s differ\\n\",\n-\t\t\t\tline_prefix, lbl[0], lbl[1]);\n+\t\t\tif (header.len)\n+\t\t\t\temit_line(o, 0, 0, header.buf, header.len);\n+\t\t\temit_line_fmt(o, 0, 0, \"Binary files %s and %s differ\\n\",\n+\t\t\t\t lbl[0], lbl[1]);\n \t\t\tgoto free_ab_and_return;\n \t\t}\n \t\tif (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)\n@@ -2399,17 +2401,18 @@ static void builtin_diff(const char *name_a,\n \t\t/* Quite common confusing case */\n \t\tif (mf1.size == mf2.size &&\n \t\t !memcmp(mf1.ptr, mf2.ptr, mf1.size)) {\n-\t\t\tif (must_show_header)\n-\t\t\t\tfprintf(o->file, \"%s\", header.buf);\n+\t\t\tif (must_show_header && header.len)\n+\t\t\t\temit_line(o, 0, 0, header.buf, header.len);\n \t\t\tgoto free_ab_and_return;\n \t\t}\n-\t\tfprintf(o->file, \"%s\", header.buf);\n+\t\tif (header.len)\n+\t\t\temit_line(o, 0, 0, header.buf, header.len);\n \t\tstrbuf_reset(&header);\n \t\tif (DIFF_OPT_TST(o, BINARY))\n \t\t\temit_binary_diff(o->file, &mf1, &mf2, line_prefix);\n \t\telse\n-\t\t\tfprintf(o->file, \"%sBinary files %s and %s differ\\n\",\n-\t\t\t\tline_prefix, lbl[0], lbl[1]);\n+\t\t\temit_line_fmt(o, 0, 0, \"Binary files %s and %s differ\\n\",\n+\t\t\t\t lbl[0], lbl[1]);\n \t\to->found_changes = 1;\n \t} else {\n \t\t/* Crazy xdl interfaces.. */\n@@ -2420,8 +2423,8 @@ static void builtin_diff(const char *name_a,\n \t\tstruct emit_callback ecbdata;\n \t\tconst struct userdiff_funcname *pe;\n \n-\t\tif (must_show_header) {\n-\t\t\tfprintf(o->file, \"%s\", header.buf);\n+\t\tif (must_show_header && header.len) {\n+\t\t\temit_line(o, 0, 0, header.buf, header.len);\n \t\t\tstrbuf_reset(&header);\n \t\t}\n \n-- \n2.10.0.21.g1da280f.dirty\n\n"},{"id":"301825","messageId":"20160913044613.1037-7-sbeller@google.com","threadId":"44071","inReplyTo":"20160913044613.1037-1-sbeller@google.com","subject":"[RFC/PATCH 06/17] diff.c: convert fn_out_consume to use emit_line_*","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-13T04:46:02Z","receivedAt":"2016-09-13T04:46:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In a later patch, I want to propose an option to detect&color\nmoved lines in a diff, which cannot be done in a one-pass over\nthe diff. Instead we need to go over the whole diff twice,\nbecause we cannot detect the first line of the two corresponding\nlines (+ and -) that got moved.\n\nSo to prepare the diff machinery for two pass algorithms\n(i.e. buffer it all up and then operate on the result),\nmove all emissions to places, such that the only emitting\nfunction is emit_line_0.\n\nThis covers the remaining parts of fn_out_consume.\n\nSigned-off-by: Stefan Beller \n---\n diff.c | 26 ++++++++++++++++++--------\n 1 file changed, 18 insertions(+), 8 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 5d57130..b4f4b75 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -493,6 +493,19 @@ static void emit_line(struct diff_options *o, const char *set, const char *reset\n \temit_line_0(o, set, reset, line[0], line+1, len-1);\n }\n \n+static void emit_line_fmt(struct diff_options *o,\n+\t\t\t const char *set, const char *reset,\n+\t\t\t const char *fmt, ...)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tva_list ap;\n+\tva_start(ap, fmt);\n+\tstrbuf_vaddf(&sb, fmt, ap);\n+\tva_end(ap);\n+\n+\temit_line(o, set, reset, sb.buf, sb.len);\n+}\n+\n static int new_blank_line_at_eof(struct emit_callback *ecbdata, const char *line, int len)\n {\n \tif (!((ecbdata->ws_rule & WS_BLANK_AT_EOF) &&\n@@ -1217,7 +1230,6 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)\n \tconst char *context = diff_get_color(ecbdata->color_diff, DIFF_CONTEXT);\n \tconst char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);\n \tstruct diff_options *o = ecbdata->opt;\n-\tconst char *line_prefix = diff_line_prefix(o);\n \n \to->found_changes = 1;\n \n@@ -1229,14 +1241,12 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)\n \n \tif (ecbdata->label_path[0]) {\n \t\tconst char *name_a_tab, *name_b_tab;\n-\n \t\tname_a_tab = strchr(ecbdata->label_path[0], ' ') ? \"\\t\" : \"\";\n \t\tname_b_tab = strchr(ecbdata->label_path[1], ' ') ? \"\\t\" : \"\";\n-\n-\t\tfprintf(o->file, \"%s%s--- %s%s%s\\n\",\n-\t\t\tline_prefix, meta, ecbdata->label_path[0], reset, name_a_tab);\n-\t\tfprintf(o->file, \"%s%s+++ %s%s%s\\n\",\n-\t\t\tline_prefix, meta, ecbdata->label_path[1], reset, name_b_tab);\n+\t\temit_line_fmt(o, meta, reset, \"--- %s%s\\n\",\n+\t\t\t ecbdata->label_path[0], name_a_tab);\n+\t\temit_line_fmt(o, meta, reset, \"+++ %s%s\\n\",\n+\t\t\t ecbdata->label_path[1], name_b_tab);\n \t\tecbdata->label_path[0] = ecbdata->label_path[1] = NULL;\n \t}\n \n@@ -1277,7 +1287,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)\n \t\tdiff_words_flush(ecbdata);\n \t\tif (ecbdata->diff_words->type == DIFF_WORDS_PORCELAIN) {\n \t\t\temit_line(o, context, reset, line, len);\n-\t\t\tfputs(\"~\\n\", o->file);\n+\t\t\temit_line(o, 0, 0, \"~\\n\", 2);\n \t\t} else {\n \t\t\t/*\n \t\t\t * Skip the prefix character, if any. With\n-- \n2.10.0.21.g1da280f.dirty\n\n"},{"id":"301826","messageId":"20160913044613.1037-10-sbeller@google.com","threadId":"44071","inReplyTo":"20160913044613.1037-1-sbeller@google.com","subject":"[RFC/PATCH 09/17] diff.c: convert emit_rewrite_lines to use emit_line_*","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-13T04:46:05Z","receivedAt":"2016-09-13T04:46:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In a later patch, I want to propose an option to detect&color\nmoved lines in a diff, which cannot be done in a one-pass over\nthe diff. Instead we need to go over the whole diff twice,\nbecause we cannot detect the first line of the two corresponding\nlines (+ and -) that got moved.\n\nSo to prepare the diff machinery for two pass algorithms\n(i.e. buffer it all up and then operate on the result),\nmove all emissions to places, such that the only emitting\nfunction is emit_line_0.\n\nThis covers emit_rewrite_lines.\n\nSigned-off-by: Stefan Beller \n---\n diff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex ee4c9d4..3aa99d1 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -690,7 +690,7 @@ static void emit_rewrite_lines(struct emit_callback *ecb,\n \tif (!endp) {\n \t\tconst char *context = diff_get_color(ecb->color_diff,\n \t\t\t\t\t\t DIFF_CONTEXT);\n-\t\tputc('\\n', ecb->opt->file);\n+\t\temit_line_0(ecb->opt, 0, 0, 0, \"\\n\", 1);\n \t\temit_line_0(ecb->opt, context, reset, '\\\\',\n \t\t\t nneof, strlen(nneof));\n \t}\n-- \n2.10.0.21.g1da280f.dirty\n\n"},{"id":"301827","messageId":"20160913044613.1037-14-sbeller@google.com","threadId":"44071","inReplyTo":"20160913044613.1037-1-sbeller@google.com","subject":"[RFC/PATCH 13/17] diff.c: convert word diffing to use emit_line_*","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-13T04:46:09Z","receivedAt":"2016-09-13T04:46:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In a later patch, I want to propose an option to detect&color\nmoved lines in a diff, which cannot be done in a one-pass over\nthe diff. Instead we need to go over the whole diff twice,\nbecause we cannot detect the first line of the two corresponding\nlines (+ and -) that got moved.\n\nSo to prepare the diff machinery for two pass algorithms\n(i.e. buffer it all up and then operate on the result),\nmove all emissions to places, such that the only emitting\nfunction is emit_line_0.\n\nThis covers all code related to diffing words.\n\nSigned-off-by: Stefan Beller \n---\n diff.c | 56 ++++++++++++++++++++++++++++----------------------------\n 1 file changed, 28 insertions(+), 28 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 00a5a4e..3940f79 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -828,37 +828,38 @@ struct diff_words_data {\n \tstruct diff_words_style *style;\n };\n \n-static int fn_out_diff_words_write_helper(FILE *fp,\n+static int fn_out_diff_words_write_helper(struct diff_options *o,\n \t\t\t\t\t struct diff_words_style_elem *st_el,\n \t\t\t\t\t const char *newline,\n \t\t\t\t\t size_t count, const char *buf,\n \t\t\t\t\t const char *line_prefix)\n {\n-\tint print = 0;\n+\tstruct strbuf sb = STRBUF_INIT;\n \n \twhile (count) {\n \t\tchar *p = memchr(buf, '\\n', count);\n-\t\tif (print)\n-\t\t\tfputs(line_prefix, fp);\n+\n \t\tif (p != buf) {\n-\t\t\tif (st_el->color && fputs(st_el->color, fp) < 0)\n-\t\t\t\treturn -1;\n-\t\t\tif (fputs(st_el->prefix, fp) < 0 ||\n-\t\t\t fwrite(buf, p ? p - buf : count, 1, fp) != 1 ||\n-\t\t\t fputs(st_el->suffix, fp) < 0)\n-\t\t\t\treturn -1;\n-\t\t\tif (st_el->color && *st_el->color\n-\t\t\t && fputs(GIT_COLOR_RESET, fp) < 0)\n-\t\t\t\treturn -1;\n+\t\t\tif (st_el->color)\n+\t\t\t\tstrbuf_addstr(&sb, st_el->color);\n+\t\t\tstrbuf_addstr(&sb, st_el->prefix);\n+\t\t\tstrbuf_add(&sb, buf, p ? p - buf : count);\n+\t\t\tstrbuf_addstr(&sb, st_el->suffix);\n+\t\t\tif (st_el->color && *st_el->color)\n+\t\t\t strbuf_addstr(&sb, GIT_COLOR_RESET);\n \t\t}\n \t\tif (!p)\n-\t\t\treturn 0;\n-\t\tif (fputs(newline, fp) < 0)\n-\t\t\treturn -1;\n+\t\t\tgoto out;\n+\t\tstrbuf_addstr(&sb, newline);\n+\t\temit_line_0(o, 0, 0, 0, sb.buf, sb.len);\n+\t\tstrbuf_reset(&sb);\n \t\tcount -= p + 1 - buf;\n \t\tbuf = p + 1;\n-\t\tprint = 1;\n \t}\n+out:\n+\tif (sb.len)\n+\t\temit_line_0(o, 0, 0, 0, sb.buf, sb.len);\n+\tstrbuf_release(&sb);\n \treturn 0;\n }\n \n@@ -936,25 +937,25 @@ static void fn_out_diff_words_aux(void *priv, char *line, unsigned long len)\n \t} else\n \t\tplus_begin = plus_end = diff_words->plus.orig[plus_first].end;\n \n-\tif (color_words_output_graph_prefix(diff_words)) {\n-\t\tfputs(line_prefix, diff_words->opt->file);\n-\t}\n+\tif (color_words_output_graph_prefix(diff_words))\n+\t\temit_line_0(diff_words->opt, 0, 0, 0, \"\", 0);\n+\n \tif (diff_words->current_plus != plus_begin) {\n-\t\tfn_out_diff_words_write_helper(diff_words->opt->file,\n+\t\tfn_out_diff_words_write_helper(diff_words->opt,\n \t\t\t\t&style->ctx, style->newline,\n \t\t\t\tplus_begin - diff_words->current_plus,\n \t\t\t\tdiff_words->current_plus, line_prefix);\n \t\tif (*(plus_begin - 1) == '\\n')\n-\t\t\tfputs(line_prefix, diff_words->opt->file);\n+\t\t\temit_line_0(diff_words->opt, 0, 0, 0, \"\", 0);\n \t}\n \tif (minus_begin != minus_end) {\n-\t\tfn_out_diff_words_write_helper(diff_words->opt->file,\n+\t\tfn_out_diff_words_write_helper(diff_words->opt,\n \t\t\t\t&style->old, style->newline,\n \t\t\t\tminus_end - minus_begin, minus_begin,\n \t\t\t\tline_prefix);\n \t}\n \tif (plus_begin != plus_end) {\n-\t\tfn_out_diff_words_write_helper(diff_words->opt->file,\n+\t\tfn_out_diff_words_write_helper(diff_words->opt,\n \t\t\t\t&style->new, style->newline,\n \t\t\t\tplus_end - plus_begin, plus_begin,\n \t\t\t\tline_prefix);\n@@ -1050,8 +1051,7 @@ static void diff_words_show(struct diff_words_data *diff_words)\n \n \t/* special case: only removal */\n \tif (!diff_words->plus.text.size) {\n-\t\tfputs(line_prefix, diff_words->opt->file);\n-\t\tfn_out_diff_words_write_helper(diff_words->opt->file,\n+\t\tfn_out_diff_words_write_helper(diff_words->opt,\n \t\t\t&style->old, style->newline,\n \t\t\tdiff_words->minus.text.size,\n \t\t\tdiff_words->minus.text.ptr, line_prefix);\n@@ -1077,8 +1077,8 @@ static void diff_words_show(struct diff_words_data *diff_words)\n \tif (diff_words->current_plus != diff_words->plus.text.ptr +\n \t\t\tdiff_words->plus.text.size) {\n \t\tif (color_words_output_graph_prefix(diff_words))\n-\t\t\tfputs(line_prefix, diff_words->opt->file);\n-\t\tfn_out_diff_words_write_helper(diff_words->opt->file,\n+\t\t\temit_line_0(diff_words->opt, 0, 0, 0, \"\", 0);\n+\t\tfn_out_diff_words_write_helper(diff_words->opt,\n \t\t\t&style->ctx, style->newline,\n \t\t\tdiff_words->plus.text.ptr + diff_words->plus.text.size\n \t\t\t- diff_words->current_plus, diff_words->current_plus,\n-- \n2.10.0.21.g1da280f.dirty\n\n"},{"id":"301828","messageId":"20160913044613.1037-13-sbeller@google.com","threadId":"44071","inReplyTo":"20160913044613.1037-1-sbeller@google.com","subject":"[RFC/PATCH 12/17] diff.c: convert show_stats to use emit_line_*","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-13T04:46:08Z","receivedAt":"2016-09-13T04:46:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In a later patch, I want to propose an option to detect&color\nmoved lines in a diff, which cannot be done in a one-pass over\nthe diff. Instead we need to go over the whole diff twice,\nbecause we cannot detect the first line of the two corresponding\nlines (+ and -) that got moved.\n\nSo to prepare the diff machinery for two pass algorithms\n(i.e. buffer it all up and then operate on the result),\nmove all emissions to places, such that the only emitting\nfunction is emit_line_0.\n\nWe call print_stat_summary from builtin/apply, so we still\nneed the version with a file pointer, so introduce\nprint_stat_summary_0 that uses emit_line_* machinery and\nkeep print_stat_summary with the same arguments around.\n\nSigned-off-by: Stefan Beller \n---\n diff.c | 87 +++++++++++++++++++++++++++++++++++++-----------------------------\n diff.h | 4 +--\n 2 files changed, 51 insertions(+), 40 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex ff1e829..00a5a4e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1465,20 +1465,19 @@ static int scale_linear(int it, int width, int max_change)\n \treturn 1 + (it * (width - 1) / max_change);\n }\n \n-static void show_name(FILE *file,\n+static void show_name(struct strbuf *out,\n \t\t const char *prefix, const char *name, int len)\n {\n-\tfprintf(file, \" %s%-*s |\", prefix, len, name);\n+\tstrbuf_addf(out, \" %s%-*s |\", prefix, len, name);\n }\n \n-static void show_graph(FILE *file, char ch, int cnt, const char *set, const char *reset)\n+static void show_graph(struct strbuf *out, char ch, int cnt, const char *set, const char *reset)\n {\n \tif (cnt <= 0)\n \t\treturn;\n-\tfprintf(file, \"%s\", set);\n-\twhile (cnt--)\n-\t\tputc(ch, file);\n-\tfprintf(file, \"%s\", reset);\n+\tstrbuf_addstr(out, set);\n+\tstrbuf_addchars(out, ch, cnt);\n+\tstrbuf_addstr(out, reset);\n }\n \n static void fill_print_name(struct diffstat_file *file)\n@@ -1502,14 +1501,16 @@ static void fill_print_name(struct diffstat_file *file)\n \tfile->print_name = pname;\n }\n \n-int print_stat_summary(FILE *fp, int files, int insertions, int deletions)\n+void print_stat_summary_0(struct diff_options *options, int files,\n+\t\t\t int insertions, int deletions)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n-\tint ret;\n \n \tif (!files) {\n \t\tassert(insertions == 0 && deletions == 0);\n-\t\treturn fprintf(fp, \"%s\\n\", \" 0 files changed\");\n+\t\tstrbuf_addstr(&sb, \" 0 files changed\");\n+\t\temit_line_0(options, 0, 0, 0, sb.buf, sb.len);\n+\t\treturn;\n \t}\n \n \tstrbuf_addf(&sb,\n@@ -1536,9 +1537,17 @@ int print_stat_summary(FILE *fp, int files, int insertions, int deletions)\n \t\t\t deletions);\n \t}\n \tstrbuf_addch(&sb, '\\n');\n-\tret = fputs(sb.buf, fp);\n+\temit_line_0(options, 0, 0, 0, sb.buf, sb.len);\n \tstrbuf_release(&sb);\n-\treturn ret;\n+}\n+\n+void print_stat_summary(FILE *fp, int files,\n+\t\t\tint insertions, int deletions)\n+{\n+\tstruct diff_options o;\n+\tmemset(&o, 0, sizeof(o));\n+\to.file = fp;\n+\tprint_stat_summary_0(&o, files, insertions, deletions);\n }\n \n static void show_stats(struct diffstat_t *data, struct diff_options *options)\n@@ -1548,13 +1557,12 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \tint total_files = data->nr, count;\n \tint width, name_width, graph_width, number_width = 0, bin_width = 0;\n \tconst char *reset, *add_c, *del_c;\n-\tconst char *line_prefix = \"\";\n \tint extra_shown = 0;\n+\tstruct strbuf out = STRBUF_INIT;\n \n \tif (data->nr == 0)\n \t\treturn;\n \n-\tline_prefix = diff_line_prefix(options);\n \tcount = options->stat_count ? options->stat_count : data->nr;\n \n \treset = diff_get_color_opt(options, DIFF_RESET);\n@@ -1708,26 +1716,29 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t}\n \n \t\tif (file->is_binary) {\n-\t\t\tfprintf(options->file, \"%s\", line_prefix);\n-\t\t\tshow_name(options->file, prefix, name, len);\n-\t\t\tfprintf(options->file, \" %*s\", number_width, \"Bin\");\n+\t\t\tshow_name(&out, prefix, name, len);\n+\t\t\tstrbuf_addf(&out, \" %*s\", number_width, \"Bin\");\n \t\t\tif (!added && !deleted) {\n-\t\t\t\tputc('\\n', options->file);\n+\t\t\t\tstrbuf_addch(&out, '\\n');\n+\t\t\t\temit_line(options, 0, 0, out.buf, out.len);\n+\t\t\t\tstrbuf_reset(&out);\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tfprintf(options->file, \" %s%\"PRIuMAX\"%s\",\n+\t\t\tstrbuf_addf(&out, \" %s%\"PRIuMAX\"%s\",\n \t\t\t\tdel_c, deleted, reset);\n-\t\t\tfprintf(options->file, \" -> \");\n-\t\t\tfprintf(options->file, \"%s%\"PRIuMAX\"%s\",\n+\t\t\tstrbuf_addstr(&out, \" -> \");\n+\t\t\tstrbuf_addf(&out, \"%s%\"PRIuMAX\"%s\",\n \t\t\t\tadd_c, added, reset);\n-\t\t\tfprintf(options->file, \" bytes\");\n-\t\t\tfprintf(options->file, \"\\n\");\n+\t\t\tstrbuf_addstr(&out, \" bytes\\n\");\n+\t\t\temit_line(options, 0, 0, out.buf, out.len);\n+\t\t\tstrbuf_reset(&out);\n \t\t\tcontinue;\n \t\t}\n \t\telse if (file->is_unmerged) {\n-\t\t\tfprintf(options->file, \"%s\", line_prefix);\n-\t\t\tshow_name(options->file, prefix, name, len);\n-\t\t\tfprintf(options->file, \" Unmerged\\n\");\n+\t\t\tshow_name(&out, prefix, name, len);\n+\t\t\tstrbuf_addstr(&out, \" Unmerged\\n\");\n+\t\t\temit_line(options, 0, 0, out.buf, out.len);\n+\t\t\tstrbuf_reset(&out);\n \t\t\tcontinue;\n \t\t}\n \n@@ -1750,14 +1761,15 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t\t\tadd = total - del;\n \t\t\t}\n \t\t}\n-\t\tfprintf(options->file, \"%s\", line_prefix);\n-\t\tshow_name(options->file, prefix, name, len);\n-\t\tfprintf(options->file, \" %*\"PRIuMAX\"%s\",\n+\t\tshow_name(&out, prefix, name, len);\n+\t\tstrbuf_addf(&out, \" %*\"PRIuMAX\"%s\",\n \t\t\tnumber_width, added + deleted,\n \t\t\tadded + deleted ? \" \" : \"\");\n-\t\tshow_graph(options->file, '+', add, add_c, reset);\n-\t\tshow_graph(options->file, '-', del, del_c, reset);\n-\t\tfprintf(options->file, \"\\n\");\n+\t\tshow_graph(&out, '+', add, add_c, reset);\n+\t\tshow_graph(&out, '-', del, del_c, reset);\n+\t\tstrbuf_addch(&out, '\\n');\n+\t\temit_line(options, 0, 0, out.buf, out.len);\n+\t\tstrbuf_reset(&out);\n \t}\n \n \tfor (i = 0; i < data->nr; i++) {\n@@ -1778,11 +1790,11 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\tif (i < count)\n \t\t\tcontinue;\n \t\tif (!extra_shown)\n-\t\t\tfprintf(options->file, \"%s ...\\n\", line_prefix);\n+\t\t\temit_line_0(options, 0, 0, 0, \" ...\\n\", strlen(\" ...\\n\"));\n \t\textra_shown = 1;\n \t}\n-\tfprintf(options->file, \"%s\", line_prefix);\n-\tprint_stat_summary(options->file, total_files, adds, dels);\n+\n+\tprint_stat_summary_0(options, total_files, adds, dels);\n }\n \n static void show_shortstats(struct diffstat_t *data, struct diff_options *options)\n@@ -1794,7 +1806,7 @@ static void show_shortstats(struct diffstat_t *data, struct diff_options *option\n \n \tfor (i = 0; i < data->nr; i++) {\n \t\tint added = data->files[i]->added;\n-\t\tint deleted= data->files[i]->deleted;\n+\t\tint deleted = data->files[i]->deleted;\n \n \t\tif (data->files[i]->is_unmerged ||\n \t\t (!data->files[i]->is_interesting && (added + deleted == 0))) {\n@@ -1804,8 +1816,7 @@ static void show_shortstats(struct diffstat_t *data, struct diff_options *option\n \t\t\tdels += deleted;\n \t\t}\n \t}\n-\tfprintf(options->file, \"%s\", diff_line_prefix(options));\n-\tprint_stat_summary(options->file, total_files, adds, dels);\n+\tprint_stat_summary_0(options, total_files, adds, dels);\n }\n \n static void show_numstat(struct diffstat_t *data, struct diff_options *options)\ndiff --git a/diff.h b/diff.h\nindex 1699d9c..cc5d038 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -379,8 +379,8 @@ extern int parse_rename_score(const char **cp_p);\n \n extern long parse_algorithm_value(const char *value);\n \n-extern int print_stat_summary(FILE *fp, int files,\n-\t\t\t int insertions, int deletions);\n+extern void print_stat_summary(FILE *fp, int files,\n+\t\t\t int insertions, int deletions);\n extern void setup_diff_pager(struct diff_options *);\n \n #endif /* DIFF_H */\n-- \n2.10.0.21.g1da280f.dirty\n\n"},{"id":"301829","messageId":"20160913044613.1037-11-sbeller@google.com","threadId":"44071","inReplyTo":"20160913044613.1037-1-sbeller@google.com","subject":"[RFC/PATCH 10/17] submodule.c: convert show_submodule_summary to use emit_line_fmt","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-13T04:46:06Z","receivedAt":"2016-09-13T04:46:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In a later patch, I want to propose an option to detect&color\nmoved lines in a diff, which cannot be done in a one-pass over\nthe diff. Instead we need to go over the whole diff twice,\nbecause we cannot detect the first line of the two corresponding\nlines (+ and -) that got moved.\n\nSo to prepare the diff machinery for two pass algorithms\n(i.e. buffer it all up and then operate on the result),\nmove all emissions to places, such that the only emitting\nfunction is emit_line_0.\n\nThis prepares the code for submodules to go through the\nemit_line_0 function.\n\nSigned-off-by: Stefan Beller \n---\n diff.c | 5 ++---\n diff.h | 3 +++\n submodule.c | 42 +++++++++++++++++-------------------------\n submodule.h | 3 +--\n 4 files changed, 23 insertions(+), 30 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 3aa99d1..1892c2b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -493,7 +493,7 @@ static void emit_line(struct diff_options *o, const char *set, const char *reset\n \temit_line_0(o, set, reset, line[0], line+1, len-1);\n }\n \n-static void emit_line_fmt(struct diff_options *o,\n+void emit_line_fmt(struct diff_options *o,\n \t\t\t const char *set, const char *reset,\n \t\t\t const char *fmt, ...)\n {\n@@ -2306,8 +2306,7 @@ static void builtin_diff(const char *name_a,\n \t\t\t(!two->mode || S_ISGITLINK(two->mode))) {\n \t\tconst char *del = diff_get_color_opt(o, DIFF_FILE_OLD);\n \t\tconst char *add = diff_get_color_opt(o, DIFF_FILE_NEW);\n-\t\tshow_submodule_summary(o->file, one->path ? one->path : two->path,\n-\t\t\t\tline_prefix,\n+\t\tshow_submodule_summary(o, one->path ? one->path : two->path,\n \t\t\t\tone->oid.hash, two->oid.hash,\n \t\t\t\ttwo->dirty_submodule,\n \t\t\t\tmeta, del, add, reset);\ndiff --git a/diff.h b/diff.h\nindex 7883729..1699d9c 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -180,6 +180,9 @@ struct diff_options {\n \tint diff_path_counter;\n };\n \n+void emit_line_fmt(struct diff_options *o, const char *set, const char *reset,\n+\t\t const char *fmt, ...);\n+\n enum color_diff {\n \tDIFF_RESET = 0,\n \tDIFF_CONTEXT = 1,\ndiff --git a/submodule.c b/submodule.c\nindex 1b5cdfb..c817b46 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -304,8 +304,8 @@ static int prepare_submodule_summary(struct rev_info *rev, const char *path,\n \treturn prepare_revision_walk(rev);\n }\n \n-static void print_submodule_summary(struct rev_info *rev, FILE *f,\n-\t\tconst char *line_prefix,\n+static void print_submodule_summary(struct rev_info *rev,\n+\t\tstruct diff_options *o,\n \t\tconst char *del, const char *add, const char *reset)\n {\n \tstatic const char format[] = \" %m %s\";\n@@ -317,24 +317,16 @@ static void print_submodule_summary(struct rev_info *rev, FILE *f,\n \t\tctx.date_mode = rev->date_mode;\n \t\tctx.output_encoding = get_log_output_encoding();\n \t\tstrbuf_setlen(&sb, 0);\n-\t\tstrbuf_addstr(&sb, line_prefix);\n-\t\tif (commit->object.flags & SYMMETRIC_LEFT) {\n-\t\t\tif (del)\n-\t\t\t\tstrbuf_addstr(&sb, del);\n-\t\t}\n-\t\telse if (add)\n-\t\t\tstrbuf_addstr(&sb, add);\n \t\tformat_commit_message(commit, format, &sb, &ctx);\n-\t\tif (reset)\n-\t\t\tstrbuf_addstr(&sb, reset);\n-\t\tstrbuf_addch(&sb, '\\n');\n-\t\tfprintf(f, \"%s\", sb.buf);\n+\t\tif (commit->object.flags & SYMMETRIC_LEFT)\n+\t\t\temit_line_fmt(o, del, reset, \"%s\\n\", sb.buf);\n+\t\telse if (add)\n+\t\t\temit_line_fmt(o, add, reset, \"%s\\n\", sb.buf);\n \t}\n \tstrbuf_release(&sb);\n }\n \n-void show_submodule_summary(FILE *f, const char *path,\n-\t\tconst char *line_prefix,\n+void show_submodule_summary(struct diff_options *o, const char *path,\n \t\tunsigned char one[20], unsigned char two[20],\n \t\tunsigned dirty_submodule, const char *meta,\n \t\tconst char *del, const char *add, const char *reset)\n@@ -359,30 +351,30 @@ void show_submodule_summary(FILE *f, const char *path,\n \t\tmessage = \"(revision walker failed)\";\n \n \tif (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)\n-\t\tfprintf(f, \"%sSubmodule %s contains untracked content\\n\",\n-\t\t\tline_prefix, path);\n+\t\temit_line_fmt(o, 0, 0,\n+\t\t\t \"Submodule %s contains untracked content\\n\", path);\n \tif (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n-\t\tfprintf(f, \"%sSubmodule %s contains modified content\\n\",\n-\t\t\tline_prefix, path);\n+\t\temit_line_fmt(o, 0, 0,\n+\t\t\t \"Submodule %s contains modified content\\n\", path);\n \n \tif (!hashcmp(one, two)) {\n \t\tstrbuf_release(&sb);\n \t\treturn;\n \t}\n \n-\tstrbuf_addf(&sb, \"%s%sSubmodule %s %s..\", line_prefix, meta, path,\n-\t\t\tfind_unique_abbrev(one, DEFAULT_ABBREV));\n+\tstrbuf_addf(&sb, \"Submodule %s %s..\", path,\n+\t\t find_unique_abbrev(one, DEFAULT_ABBREV));\n \tif (!fast_backward && !fast_forward)\n \t\tstrbuf_addch(&sb, '.');\n \tstrbuf_addf(&sb, \"%s\", find_unique_abbrev(two, DEFAULT_ABBREV));\n \tif (message)\n-\t\tstrbuf_addf(&sb, \" %s%s\\n\", message, reset);\n+\t\tstrbuf_addf(&sb, \" %s\\n\", message);\n \telse\n-\t\tstrbuf_addf(&sb, \"%s:%s\\n\", fast_backward ? \" (rewind)\" : \"\", reset);\n-\tfwrite(sb.buf, sb.len, 1, f);\n+\t\tstrbuf_addf(&sb, \"%s:\\n\", fast_backward ? \" (rewind)\" : \"\");\n+\temit_line_fmt(o, meta, reset, \"%s\", sb.buf);\n \n \tif (!message) /* only NULL if we succeeded in setting up the walk */\n-\t\tprint_submodule_summary(&rev, f, line_prefix, del, add, reset);\n+\t\tprint_submodule_summary(&rev, o, del, add, reset);\n \tif (left)\n \t\tclear_commit_marks(left, ~0);\n \tif (right)\ndiff --git a/submodule.h b/submodule.h\nindex 2af9390..fefc0ab 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -41,8 +41,7 @@ int parse_submodule_update_strategy(const char *value,\n \t\tstruct submodule_update_strategy *dst);\n const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);\n void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);\n-void show_submodule_summary(FILE *f, const char *path,\n-\t\tconst char *line_prefix,\n+void show_submodule_summary(struct diff_options *o, const char *path,\n \t\tunsigned char one[20], unsigned char two[20],\n \t\tunsigned dirty_submodule, const char *meta,\n \t\tconst char *del, const char *add, const char *reset);\n-- \n2.10.0.21.g1da280f.dirty\n\n"},{"id":"301830","messageId":"20160913044613.1037-15-sbeller@google.com","threadId":"44071","inReplyTo":"20160913044613.1037-1-sbeller@google.com","subject":"[RFC/PATCH 14/17] diff.c: convert diff_flush to use emit_line_*","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-13T04:46:10Z","receivedAt":"2016-09-13T04:47:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In a later patch, I want to propose an option to detect&color\nmoved lines in a diff, which cannot be done in a one-pass over\nthe diff. Instead we need to go over the whole diff twice,\nbecause we cannot detect the first line of the two corresponding\nlines (+ and -) that got moved.\n\nSo to prepare the diff machinery for two pass algorithms\n(i.e. buffer it all up and then operate on the result),\nmove all emissions to places, such that the only emitting\nfunction is emit_line_0.\n\nThis covers diff_flush.\n\nSigned-off-by: Stefan Beller \n---\n diff.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 3940f79..2b1ebcc 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4737,12 +4737,13 @@ void diff_flush(struct diff_options *options)\n \n \tif (output_format & DIFF_FORMAT_PATCH) {\n \t\tif (separator) {\n-\t\t\tfprintf(options->file, \"%s%c\",\n-\t\t\t\tdiff_line_prefix(options),\n-\t\t\t\toptions->line_termination);\n+\t\t\temit_line_0(options, NULL, NULL,\n+\t\t\t\t options->line_termination, \"\", 0);\n \t\t\tif (options->stat_sep) {\n \t\t\t\t/* attach patch instead of inline */\n-\t\t\t\tfputs(options->stat_sep, options->file);\n+\t\t\t\temit_line_0(options, NULL, NULL, 0,\n+\t\t\t\t\t options->stat_sep,\n+\t\t\t\t\t strlen(options->stat_sep));\n \t\t\t}\n \t\t}\n \n-- \n2.10.0.21.g1da280f.dirty\n\n"},{"id":"301831","messageId":"20160913044613.1037-18-sbeller@google.com","threadId":"44071","inReplyTo":"20160913044613.1037-1-sbeller@google.com","subject":"[RFC/PATCH 17/17] diff.c: color moved lines differently","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-13T04:46:13Z","receivedAt":"2016-09-13T04:47:04Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When there is a lot of code moved around such as in 11979b9 (2005-11-18,\n\"http.c: reorder to avoid compilation failure.\") for example, the review\nprocess is quite hard, as it is not mentally challenging. It is rather a\ntedious process, that gets boring quickly. However you still need to read\nthrough all of the code to make sure the moved lines are there as supposed.\n\nWhile it is trivial to color up a patch like the following\n\n $ git show -p\n commit 95b1fc60907fb9224bd785111ccd16e7e0aec4d1\n Author: Stefan Beller \n Date: Mon Sep 12 20:02:46 2016 -0700\n\n move secure_foo from test.c to file2.c\n\n diff --git a/file2.c b/file2.c\n index 9163a0f..8e66dc0 100644\n --- a/file2.c\n +++ b/file2.c\n @@ -3,13 +3,6 @@ void *xmemdupz(const void *data, size_t len)\n return memcpy(xmallocz(len), data, len);\n }\n\n -int secure_foo(struct user *u)\n -{\n - if (!u->is_allowed_foo)\n - return;\n - foo(u);\n -}\n -\n char *xstrndup(const char *str, size_t len)\n {\n char *p = memchr(str, '\\0', len);\n diff --git a/test.c b/test.c\n index a95e6fe..81eb0eb 100644\n --- a/test.c\n +++ b/test.c\n @@ -18,6 +18,13 @@ ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset)\n return total;\n }\n\n +int secure_foo(struct user *u)\n +{\n + if (!u->is_allowed_foo)\n + return;\n + foo(u);\n +}\n +\n int xdup(int fd)\n {\n int ret = dup(fd);\n\nas in this patch all lines that add or remove lines\nshould be colored in the new color that indicates moved\nlines.\n\nHowever the intention of this patch is to aid reviewers\nto spotting permutations in the moved code. So consider the\nfollowing malicious move:\n\n diff --git a/file2.c b/file2.c\n index 9163a0f..8e66dc0 100644\n --- a/file2.c\n +++ b/file2.c\n @@ -3,13 +3,6 @@ void *xmemdupz(const void *data, size_t len)\n return memcpy(xmallocz(len), data, len);\n }\n\n -int secure_foo(struct user *u)\n -{\n - if (!u->is_allowed_foo)\n - return;\n - foo(u);\n -}\n -\n char *xstrndup(const char *str, size_t len)\n {\n char *p = memchr(str, '\\0', len);\n diff --git a/test.c b/test.c\n index a95e6fe..a679c40 100644\n --- a/test.c\n +++ b/test.c\n @@ -18,6 +18,13 @@ ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset)\n return total;\n }\n\n +int secure_foo(struct user *u)\n +{\n + foo(u);\n + if (!u->is_allowed_foo)\n + return;\n +}\n +\n int xdup(int fd)\n {\n int ret = dup(fd);\n\nIf the moved code is larger, it is easier to hide some permutation in the\ncode, which is why we would not want to color all lines as \"moved\" in this\ncase. So additionally to coloring lines differently that are added and\nremoved in the same diff, we need to tweak the algorithm a bit more.\n\nAs the reviewers attention should be brought to the places, where the\ndifference is introduced to the moved code, we will not color a moved\nline if the previous line is a different moved line, i.e. in the code\nabove the line `foo()` is still colored as a regular added line, because\nthe line before the call to `foo` changed from `return` to `{`. The line\nwith the condition is also not colored as moved but as a new addition.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/config.txt | 12 +-\n Documentation/diff-options.txt | 7 +\n contrib/completion/git-completion.bash | 2 +\n diff.c | 171 ++++++++++++++++++++----\n diff.h | 10 +-\n t/t4015-diff-whitespace.sh | 229 +++++++++++++++++++++++++++++++++\n 6 files changed, 402 insertions(+), 29 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 0bcb679..e0c7b91 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -974,14 +974,22 @@ This does not affect linkgit:git-format-patch[1] or the\n 'git-diff-{asterisk}' plumbing commands. Can be overridden on the\n command line with the `--color[=]` option.\n \n+color.moved::\n+\tA boolean value, whether a diff should color moved lines\n+\tdifferently. The moved lines are searched for in the diff only.\n+\tDuplicated lines from somewhere in the project that are not\n+\tpart of the diff are not colored as moved.\n+\tDefaults to true.\n+\n color.diff.::\n \tUse customized color for diff colorization. `` specifies\n \twhich part of the patch to use the specified color, and is one\n \tof `context` (context text - `plain` is a historical synonym),\n \t`meta` (metainformation), `frag`\n \t(hunk header), 'func' (function in hunk header), `old` (removed lines),\n-\t`new` (added lines), `commit` (commit headers), or `whitespace`\n-\t(highlighting whitespace errors).\n+\t`new` (added lines), `commit` (commit headers), `whitespace`\n+\t(highlighting whitespace errors), `movedFrom` (removed lines that\n+\treappear), `movedTo` (added lines that were removed elsewhere).\n \n color.decorate.::\n \tUse customized color for 'git log --decorate' output. `` is one\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 705a873..13b6a2a 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -234,6 +234,13 @@ ifdef::git-diff[]\n endif::git-diff[]\n \tIt is the same as `--color=never`.\n \n+--[no-]color-moved::\n+\tShow moved blocks in a different color.\n+ifdef::git-diff[]\n+\tIt can be changed by the `diff.ui` and `color.diff`\n+\tconfiguration settings.\n+endif::git-diff[]\n+\n --word-diff[=]::\n \tShow a word diff, using the to delimit changed words.\n \tBy default, words are delimited by whitespace; see\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 9c8f738..85032dd 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2115,6 +2115,8 @@ _git_config ()\n \t\tcolor.diff.old\n \t\tcolor.diff.plain\n \t\tcolor.diff.whitespace\n+\t\tcolor.diff.movedFrom\n+\t\tcolor.diff.movedTo\n \t\tcolor.grep\n \t\tcolor.grep.context\n \t\tcolor.grep.filename\ndiff --git a/diff.c b/diff.c\nindex e240e89..f41cdcf 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -30,6 +30,7 @@ static int diff_compaction_heuristic; /* experimental */\n static int diff_rename_limit_default = 400;\n static int diff_suppress_blank_empty;\n static int diff_use_color_default = -1;\n+static int diff_color_moved_default;\n static int diff_context_default = 3;\n static const char *diff_word_regex_cfg;\n static const char *external_diff_cmd_cfg;\n@@ -52,6 +53,8 @@ static char diff_colors[][COLOR_MAXLEN] = {\n \tGIT_COLOR_YELLOW,\t/* COMMIT */\n \tGIT_COLOR_BG_RED,\t/* WHITESPACE */\n \tGIT_COLOR_NORMAL,\t/* FUNCINFO */\n+\tGIT_COLOR_BLUE,\t\t/* MOVED FROM */\n+\tGIT_COLOR_MAGENTA,\t/* MOVED TO */\n };\n \n static int parse_diff_color_slot(const char *var)\n@@ -72,6 +75,10 @@ static int parse_diff_color_slot(const char *var)\n \t\treturn DIFF_WHITESPACE;\n \tif (!strcasecmp(var, \"func\"))\n \t\treturn DIFF_FUNCINFO;\n+\tif (!strcasecmp(var, \"movedfrom\"))\n+\t\treturn DIFF_FILE_MOVED_FROM;\n+\tif (!strcasecmp(var, \"movedto\"))\n+\t\treturn DIFF_FILE_MOVED_TO;\n \treturn -1;\n }\n \n@@ -180,6 +187,10 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \t\tdiff_use_color_default = git_config_colorbool(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"color.moved\")) {\n+\t\tdiff_color_moved_default = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n \tif (!strcmp(var, \"diff.context\")) {\n \t\tdiff_context_default = git_config_int(var, value);\n \t\tif (diff_context_default < 0)\n@@ -287,6 +298,41 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \treturn git_default_config(var, value, cb);\n }\n \n+struct moved_entry {\n+\tstruct hashmap_entry ent;\n+\tchar *line;\n+\tint hash_prev;\n+};\n+\n+static int moved_entry_cmp(const struct moved_entry *a,\n+\t\t\t const struct moved_entry *b,\n+\t\t\t const void *unused)\n+{\n+\treturn strcmp(a->line, b->line) &&\n+\t a->hash_prev == b->hash_prev;\n+}\n+\n+static struct moved_entry *prepare_entry(const char *line,\n+\t\t\t\t\t unsigned long len,\n+\t\t\t\t\t int ws_rule,\n+\t\t\t\t\t int hash_prev)\n+{\n+\tint i;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct moved_entry *ret = xmalloc(sizeof(*ret));\n+\n+\tfor (i = 0; i < len; i++) {\n+\t\tif (ws_rule && isspace(line[i]))\n+\t\t\tcontinue;\n+\t\tstrbuf_addch(&sb, line[i]);\n+\t}\n+\n+\tret->ent.hash = memhash(sb.buf, sb.len);\n+\tret->line = xmemdupz(line, len);\n+\tret->hash_prev = hash_prev;\n+\treturn ret;\n+}\n+\n static char *quote_two(const char *one, const char *two)\n {\n \tint need_one = quote_c_style(one, NULL, NULL, 1);\n@@ -449,12 +495,49 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,\n \tecbdata->blank_at_eof_in_postimage = (at - l2) + 1;\n }\n \n+static int should_color_as_moved(struct diff_options *o,\n+\t\t\t\t struct line_emission *e,\n+\t\t\t\t struct moved_entry *keydata,\n+\t\t\t\t struct hashmap *lookup)\n+{\n+\tstruct moved_entry *me;\n+\n+\tif (!o->color_moved)\n+\t\treturn 0;\n+\tif (e->first != '+' && e->first != '-')\n+\t\treturn 0;\n+\n+\tme = hashmap_get(lookup, keydata, keydata);\n+\n+\tif (!me)\n+\t\treturn 0;\n+\n+\tif (me->hash_prev == 0 || o->hash_prev == 0)\n+\t\treturn 1;\n+\n+\treturn (me->hash_prev == o->hash_prev);\n+}\n+\n static void emit_line_emission(struct diff_options *o, struct line_emission *e)\n {\n \tFILE *file = o->file;\n \n \tfputs(diff_line_prefix(o), file);\n \n+\tif (o->color_moved && (e->first == '+' || e->first == '-')) {\n+\t\tstruct moved_entry *keydata;\n+\t\tkeydata = prepare_entry(e->line, e->len,\n+\t\t\t\t\tDIFF_XDL_TST(o, IGNORE_WHITESPACE), 0);\n+\t\tif (e->first == '-' &&\n+\t\t should_color_as_moved(o, e, keydata, o->added_lines))\n+\t\t\te->set = diff_get_color_opt(o, DIFF_FILE_MOVED_FROM);\n+\t\tif (e->first == '+' &&\n+\t\t should_color_as_moved(o, e, keydata, o->deleted_lines))\n+\t\t\te->set = diff_get_color_opt(o, DIFF_FILE_MOVED_TO);\n+\n+\t\to->hash_prev = keydata->ent.hash;\n+\t}\n+\n \tif (e->len || e->first) {\n \t\tif (e->set)\n \t\t\tfputs(e->set, file);\n@@ -568,67 +651,95 @@ static void emit_line_checked(const char *reset,\n \t\t\t unsigned ws_error_highlight,\n \t\t\t char sign)\n {\n+\tint i;\n+\tstruct line_emission *e;\n \tconst char *set = diff_get_color(ecbdata->color_diff, color);\n \tconst char *ws = NULL;\n+\tstruct diff_options *o = ecbdata->opt;\n \n-\tif (ecbdata->opt->ws_error_highlight & ws_error_highlight) {\n+\tif (o->ws_error_highlight & ws_error_highlight) {\n \t\tws = diff_get_color(ecbdata->color_diff, DIFF_WHITESPACE);\n \t\tif (!*ws)\n \t\t\tws = NULL;\n \t}\n \n \tif (!ws)\n-\t\temit_line_0(ecbdata->opt, set, reset, sign, line, len);\n+\t\temit_line_0(o, set, reset, sign, line, len);\n \telse if (sign == '+' && new_blank_line_at_eof(ecbdata, line, len))\n \t\t/* Blank line at EOF - paint '+' as well */\n-\t\temit_line_0(ecbdata->opt, ws, reset, sign, line, len);\n+\t\temit_line_0(o, ws, reset, sign, line, len);\n \telse {\n \t\t/* Emit just the prefix, then the rest. */\n-\t\tif (ecbdata->opt->use_buffer) {\n-\t\t\tstruct line_emission e;\n-\t\t\te.line = line;\n-\t\t\te.len = len;\n-\t\t\te.ws_rule = ecbdata->ws_rule;\n-\t\t\te.set = set;\n-\t\t\te.reset = reset;\n-\t\t\te.ws = ws;\n-\t\t\te.whitespace_check = 1;\n-\t\t\te.first = sign;\n-\t\t\tappend_line_emission_to_buffer(ecbdata->opt, &e);\n+\t\tif (o->use_buffer) {\n+\t\t\temit_line_0(o, set, reset, sign, line, len);\n+\t\t\ti = o->line_buffer_nr - 1;\n+\t\t\te = &o->line_buffer[i];\n+\t\t\te->whitespace_check = 1;\n+\t\t\te->ws_rule = ecbdata->ws_rule;\n+\t\t\te->ws = ws;\n \t\t} else {\n-\t\t\temit_line_0(ecbdata->opt, set, reset, sign, \"\", 0);\n+\t\t\temit_line_0(o, set, reset, sign, \"\", 0);\n \t\t\tws_check_emit(line, len, ecbdata->ws_rule,\n-\t\t\t\t ecbdata->opt->file, set, reset, ws);\n+\t\t\t\t o->file, set, reset, ws);\n \t\t}\n \t}\n+\tif (o->color_moved) {\n+\t\ti = o->line_buffer_nr - 1;\n+\t\te = &o->line_buffer[i];\n+\t}\n }\n \n static void emit_add_line(const char *reset,\n \t\t\t struct emit_callback *ecbdata,\n \t\t\t const char *line, int len)\n {\n+\tstruct diff_options *o = ecbdata->opt;\n \tecbdata->lno_in_postimage++;\n \temit_line_checked(reset, ecbdata, line, len,\n \t\t\t DIFF_FILE_NEW, WSEH_NEW, '+');\n+\tif (o->color_moved) {\n+\t\tstruct line_emission *l = &o->line_buffer[o->line_buffer_nr - 1];\n+\t\tstruct moved_entry *keydata = prepare_entry(l->line, l->len,\n+\t\t\t\t\tDIFF_XDL_TST(o, IGNORE_WHITESPACE),\n+\t\t\t\t\to->hash_prev);\n+\t\tl = &o->line_buffer[o->line_buffer_nr - 2];\n+\t\tif (l->first == '+')\n+\t\t\tkeydata->hash_prev = o->hash_prev;\n+\t\thashmap_add(o->added_lines, keydata);\n+\t\to->hash_prev = keydata->ent.hash;\n+\t}\n }\n \n static void emit_del_line(const char *reset,\n \t\t\t struct emit_callback *ecbdata,\n \t\t\t const char *line, int len)\n {\n+\tstruct diff_options *o = ecbdata->opt;\n \tecbdata->lno_in_preimage++;\n \temit_line_checked(reset, ecbdata, line, len,\n \t\t\t DIFF_FILE_OLD, WSEH_OLD, '-');\n+\tif (o->color_moved) {\n+\t\tstruct line_emission *l = &o->line_buffer[o->line_buffer_nr - 1];\n+\t\tstruct moved_entry *keydata = prepare_entry(l->line, l->len,\n+\t\t\t\t\tDIFF_XDL_TST(o, IGNORE_WHITESPACE), 0);\n+\t\tl = &o->line_buffer[o->line_buffer_nr - 2];\n+\t\tif (l->first == '-')\n+\t\t\tkeydata->hash_prev = o->hash_prev;\n+\t\thashmap_add(o->deleted_lines, keydata);\n+\t\to->hash_prev = keydata->ent.hash;\n+\t}\n }\n \n static void emit_context_line(const char *reset,\n \t\t\t struct emit_callback *ecbdata,\n \t\t\t const char *line, int len)\n {\n+\tstruct diff_options *o = ecbdata->opt;\n \tecbdata->lno_in_postimage++;\n \tecbdata->lno_in_preimage++;\n \temit_line_checked(reset, ecbdata, line, len,\n \t\t\t DIFF_CONTEXT, WSEH_CONTEXT, ' ');\n+\to->hash_prev = 0;\n }\n \n static void emit_hunk_header(struct emit_callback *ecbdata,\n@@ -3389,6 +3500,7 @@ void diff_setup(struct diff_options *options)\n \toptions->change = diff_change;\n \toptions->add_remove = diff_addremove;\n \toptions->use_color = diff_use_color_default;\n+\toptions->color_moved = diff_color_moved_default;\n \toptions->detect_rename = diff_detect_rename_default;\n \toptions->xdl_opts |= diff_algorithm;\n \tif (diff_compaction_heuristic)\n@@ -3511,6 +3623,9 @@ void diff_setup_done(struct diff_options *options)\n \n \tif (DIFF_OPT_TST(options, FOLLOW_RENAMES) && options->pathspec.nr != 1)\n \t\tdie(_(\"--follow requires exactly one pathspec\"));\n+\n+\tif (!options->use_color || external_diff())\n+\t\toptions->color_moved = 0;\n }\n \n static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *val)\n@@ -3961,6 +4076,10 @@ int diff_opt_parse(struct diff_options *options,\n \t}\n \telse if (!strcmp(arg, \"--no-color\"))\n \t\toptions->use_color = 0;\n+\telse if (!strcmp(arg, \"--color-moved\"))\n+\t\toptions->color_moved = 1;\n+\telse if (!strcmp(arg, \"--no-color-moved\"))\n+\t\toptions->color_moved = 0;\n \telse if (!strcmp(arg, \"--color-words\")) {\n \t\toptions->use_color = 1;\n \t\toptions->word_diff = DIFF_WORDS_COLOR;\n@@ -4728,16 +4847,15 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)\n {\n \tint i;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n-\t/*\n-\t * TODO:\n-\t * For testing purposes we want to make sure the diff machinery\n-\t * works with the buffer. If there is anything emitted outside the\n-\t * emit_line_emission, then the order is screwed up and the tests\n-\t * will fail.\n-\t *\n-\t * We'll unset this flag in a later patch.\n-\t */\n-\to->use_buffer = 1;\n+\n+\tif (o->color_moved) {\n+\t\to->use_buffer = 1;\n+\t\to->deleted_lines = xmallocz(sizeof(*o->deleted_lines));\n+\t\to->added_lines = xmallocz(sizeof(*o->added_lines));\n+\t\thashmap_init(o->deleted_lines, (hashmap_cmp_fn)moved_entry_cmp, 0);\n+\t\thashmap_init(o->added_lines, (hashmap_cmp_fn)moved_entry_cmp, 0);\n+\t}\n+\n \tfor (i = 0; i < q->nr; i++) {\n \t\tstruct diff_filepair *p = q->queue[i];\n \t\tif (check_pair_status(p))\n@@ -4827,6 +4945,7 @@ void diff_flush(struct diff_options *options)\n \t\tif (!options->file)\n \t\t\tdie_errno(\"Could not open /dev/null\");\n \t\toptions->close_file = 1;\n+\t\toptions->color_moved = 0;\n \t\tfor (i = 0; i < q->nr; i++) {\n \t\t\tstruct diff_filepair *p = q->queue[i];\n \t\t\tif (check_pair_status(p))\ndiff --git a/diff.h b/diff.h\nindex 4df6aa5..8c3f8f7 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -7,6 +7,7 @@\n #include \"tree-walk.h\"\n #include \"pathspec.h\"\n #include \"object.h\"\n+#include \"hashmap.h\"\n \n struct rev_info;\n struct diff_options;\n@@ -193,9 +194,14 @@ struct diff_options {\n \n \tint diff_path_counter;\n \n+\tint color_moved;\n \tint use_buffer;\n \tstruct line_emission *line_buffer;\n \tint line_buffer_nr, line_buffer_alloc;\n+\n+\tstruct hashmap *deleted_lines;\n+\tstruct hashmap *added_lines;\n+\tint hash_prev;\n };\n \n void emit_line_fmt(struct diff_options *o, const char *set, const char *reset,\n@@ -210,7 +216,9 @@ enum color_diff {\n \tDIFF_FILE_NEW = 5,\n \tDIFF_COMMIT = 6,\n \tDIFF_WHITESPACE = 7,\n-\tDIFF_FUNCINFO = 8\n+\tDIFF_FUNCINFO = 8,\n+\tDIFF_FILE_MOVED_TO = 9,\n+\tDIFF_FILE_MOVED_FROM = 10\n };\n const char *diff_get_color(int diff_use_color, enum color_diff ix);\n #define diff_get_color_opt(o, ix) \\\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 2434157..2e3d7c9 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -934,4 +934,233 @@ test_expect_success 'the same with --ws-error-highlight' '\n \ttest_cmp expected current\n '\n \n+test_expect_success 'detect moved code, complete file' '\n+\tgit reset --hard &&\n+\tcat <<-\\EOF >test.c &&\n+\t#include\n+\tmain()\n+\t{\n+\tprintf(\"Hello World\");\n+\t}\n+\tEOF\n+\tgit add test.c &&\n+\tgit commit -m \"add main function\" &&\n+\tgit mv test.c main.c &&\n+\tgit diff HEAD --color-moved --no-renames | test_decode_color >actual &&\n+\tcat >expected <<-\\EOF &&\n+\tdiff --git a/main.c b/main.c\n+\tnew file mode 100644\n+\tindex 0000000..a986c57\n+\t--- /dev/null\n+\t+++ b/main.c\n+\t@@ -0,0 +1,5 @@\n+\t+#include\n+\t+main()\n+\t+{\n+\t+printf(\"Hello World\");\n+\t+}\n+\tdiff --git a/test.c b/test.c\n+\tdeleted file mode 100644\n+\tindex a986c57..0000000\n+\t--- a/test.c\n+\t+++ /dev/null\n+\t@@ -1,5 +0,0 @@\n+\t-#include\n+\t-main()\n+\t-{\n+\t-printf(\"Hello World\");\n+\t-}\n+\tEOF\n+\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'detect moved code, inside file' '\n+\tgit reset --hard &&\n+\tcat <<-\\EOF >main.c &&\n+\t\t#include\n+\t\tint stuff()\n+\t\t{\n+\t\t\tprintf(\"Hello \");\n+\t\t\tprintf(\"World\\n\");\n+\t\t}\n+\n+\t\tint secure_foo(struct user *u)\n+\t\t{\n+\t\t\tif (!u->is_allowed_foo)\n+\t\t\t\treturn;\n+\t\t\tfoo(u);\n+\t\t}\n+\n+\t\tint main()\n+\t\t{\n+\t\t\tfoo();\n+\t\t}\n+\tEOF\n+\tcat <<-\\EOF >test.c &&\n+\t\t#include\n+\t\tint bar()\n+\t\t{\n+\t\t\tprintf(\"Hello World, but different\\n\");\n+\t\t}\n+\n+\t\tint another_function()\n+\t\t{\n+\t\t\tbar();\n+\t\t}\n+\tEOF\n+\tgit add main.c test.c &&\n+\tgit commit -m \"add main and test file\" &&\n+\tcat <<-\\EOF >main.c &&\n+\t\t#include\n+\t\tint stuff()\n+\t\t{\n+\t\t\tprintf(\"Hello \");\n+\t\t\tprintf(\"World\\n\");\n+\t\t}\n+\n+\t\tint main()\n+\t\t{\n+\t\t\tfoo();\n+\t\t}\n+\tEOF\n+\tcat <<-\\EOF >test.c &&\n+\t\t#include\n+\t\tint bar()\n+\t\t{\n+\t\t\tprintf(\"Hello World, but different\\n\");\n+\t\t}\n+\n+\t\tint secure_foo(struct user *u)\n+\t\t{\n+\t\t\tif (!u->is_allowed_foo)\n+\t\t\t\treturn;\n+\t\t\tfoo(u);\n+\t\t}\n+\n+\t\tint another_function()\n+\t\t{\n+\t\t\tbar();\n+\t\t}\n+\tEOF\n+\tgit diff HEAD --no-renames --color-moved| test_decode_color >actual &&\n+\tcat <<-\\EOF >expected &&\n+\tdiff --git a/main.c b/main.c\n+\tindex 27a619c..7cf9336 100644\n+\t--- a/main.c\n+\t+++ b/main.c\n+\t@@ -5,13 +5,6 @@ printf(\"Hello \");\n+\t printf(\"World\\n\");\n+\t }\n+\t \n+\t-int secure_foo(struct user *u)\n+\t-{\n+\t-if (!u->is_allowed_foo)\n+\t-return;\n+\t-foo(u);\n+\t-}\n+\t-\n+\t int main()\n+\t {\n+\t foo();\n+\tdiff --git a/test.c b/test.c\n+\tindex 1dc1d85..e34eb69 100644\n+\t--- a/test.c\n+\t+++ b/test.c\n+\t@@ -4,6 +4,13 @@ int bar()\n+\t printf(\"Hello World, but different\\n\");\n+\t }\n+\t \n+\t+int secure_foo(struct user *u)\n+\t+{\n+\t+if (!u->is_allowed_foo)\n+\t+return;\n+\t+foo(u);\n+\t+}\n+\t+\n+\t int another_function()\n+\t {\n+\t bar();\n+\tEOF\n+\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'detect permutations inside moved code, ' '\n+\t# reusing the move example from last test:\n+\tcat <<-\\EOF >main.c &&\n+\t\t#include\n+\t\tint stuff()\n+\t\t{\n+\t\t\tprintf(\"Hello \");\n+\t\t\tprintf(\"World\\n\");\n+\t\t}\n+\n+\t\tint main()\n+\t\t{\n+\t\t\tfoo();\n+\t\t}\n+\tEOF\n+\tcat <<-\\EOF >test.c &&\n+\t\t#include\n+\t\tint bar()\n+\t\t{\n+\t\t\tprintf(\"Hello World, but different\\n\");\n+\t\t}\n+\n+\t\tint secure_foo(struct user *u)\n+\t\t{\n+\t\t\tfoo(u);\n+\t\t\tif (!u->is_allowed_foo)\n+\t\t\t\treturn;\n+\t\t}\n+\n+\t\tint another_function()\n+\t\t{\n+\t\t\tbar();\n+\t\t}\n+\tEOF\n+\tgit diff HEAD --no-renames --color-moved| test_decode_color >actual &&\n+\tcat <<-\\EOF >expected &&\n+\tdiff --git a/main.c b/main.c\n+\tindex 27a619c..7cf9336 100644\n+\t--- a/main.c\n+\t+++ b/main.c\n+\t@@ -5,13 +5,6 @@ printf(\"Hello \");\n+\t printf(\"World\\n\");\n+\t }\n+\t \n+\t-int secure_foo(struct user *u)\n+\t-{\n+\t-if (!u->is_allowed_foo)\n+\t-return;\n+\t-foo(u);\n+\t-}\n+\t-\n+\t int main()\n+\t {\n+\t foo();\n+\tdiff --git a/test.c b/test.c\n+\tindex 1dc1d85..2bedec9 100644\n+\t--- a/test.c\n+\t+++ b/test.c\n+\t@@ -4,6 +4,13 @@ int bar()\n+\t printf(\"Hello World, but different\\n\");\n+\t }\n+\t \n+\t+int secure_foo(struct user *u)\n+\t+{\n+\t+foo(u);\n+\t+if (!u->is_allowed_foo)\n+\t+return;\n+\t+}\n+\t+\n+\t int another_function()\n+\t {\n+\t bar();\n+\tEOF\n+\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n2.10.0.21.g1da280f.dirty\n\n"},{"id":"301832","messageId":"20160913044613.1037-12-sbeller@google.com","threadId":"44071","inReplyTo":"20160913044613.1037-1-sbeller@google.com","subject":"[RFC/PATCH 11/17] diff.c: convert emit_binary_diff_body to use emit_line_*","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-13T04:46:07Z","receivedAt":"2016-09-13T04:47:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In a later patch, I want to propose an option to detect&color\nmoved lines in a diff, which cannot be done in a one-pass over\nthe diff. Instead we need to go over the whole diff twice,\nbecause we cannot detect the first line of the two corresponding\nlines (+ and -) that got moved.\n\nSo to prepare the diff machinery for two pass algorithms\n(i.e. buffer it all up and then operate on the result),\nmove all emissions to places, such that the only emitting\nfunction is emit_line_0.\n\nThis covers emit_binary_diff_body.\n\nSigned-off-by: Stefan Beller \n---\n diff.c | 39 ++++++++++++++++++++++-----------------\n 1 file changed, 22 insertions(+), 17 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 1892c2b..ff1e829 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2169,8 +2169,8 @@ static unsigned char *deflate_it(char *data,\n \treturn deflated;\n }\n \n-static void emit_binary_diff_body(FILE *file, mmfile_t *one, mmfile_t *two,\n-\t\t\t\t const char *prefix)\n+static void emit_binary_diff_body(struct diff_options *o,\n+\t\t\t\t mmfile_t *one, mmfile_t *two)\n {\n \tvoid *cp;\n \tvoid *delta;\n@@ -2199,13 +2199,12 @@ static void emit_binary_diff_body(FILE *file, mmfile_t *one, mmfile_t *two,\n \t}\n \n \tif (delta && delta_size < deflate_size) {\n-\t\tfprintf(file, \"%sdelta %lu\\n\", prefix, orig_size);\n+\t\temit_line_fmt(o, 0, 0, \"delta %lu\\n\", orig_size);\n \t\tfree(deflated);\n \t\tdata = delta;\n \t\tdata_size = delta_size;\n-\t}\n-\telse {\n-\t\tfprintf(file, \"%sliteral %lu\\n\", prefix, two->size);\n+\t} else {\n+\t\temit_line_fmt(o, 0, 0, \"literal %lu\\n\", two->size);\n \t\tfree(delta);\n \t\tdata = deflated;\n \t\tdata_size = deflate_size;\n@@ -2214,8 +2213,9 @@ static void emit_binary_diff_body(FILE *file, mmfile_t *one, mmfile_t *two,\n \t/* emit data encoded in base85 */\n \tcp = data;\n \twhile (data_size) {\n+\t\tint len;\n \t\tint bytes = (52 < data_size) ? 52 : data_size;\n-\t\tchar line[70];\n+\t\tchar line[71];\n \t\tdata_size -= bytes;\n \t\tif (bytes <= 26)\n \t\t\tline[0] = bytes + 'A' - 1;\n@@ -2223,20 +2223,25 @@ static void emit_binary_diff_body(FILE *file, mmfile_t *one, mmfile_t *two,\n \t\t\tline[0] = bytes - 26 + 'a' - 1;\n \t\tencode_85(line + 1, cp, bytes);\n \t\tcp = (char *) cp + bytes;\n-\t\tfprintf(file, \"%s\", prefix);\n-\t\tfputs(line, file);\n-\t\tfputc('\\n', file);\n+\n+\t\tlen = strlen(line);\n+\t\tline[len++] = '\\n';\n+\t\tline[len] = '\\0';\n+\n+\t\temit_line(o, 0, 0, line, len);\n \t}\n-\tfprintf(file, \"%s\\n\", prefix);\n+\temit_line(o, 0, 0, \"\\n\", 1);\n \tfree(data);\n }\n \n-static void emit_binary_diff(FILE *file, mmfile_t *one, mmfile_t *two,\n-\t\t\t const char *prefix)\n+static void emit_binary_diff(struct diff_options *o,\n+\t\t\t mmfile_t *one, mmfile_t *two)\n {\n-\tfprintf(file, \"%sGIT binary patch\\n\", prefix);\n-\temit_binary_diff_body(file, one, two, prefix);\n-\temit_binary_diff_body(file, two, one, prefix);\n+\tconst char *s = \"GIT binary patch\\n\";\n+\tconst int len = strlen(s);\n+\temit_line(o, 0, 0, s, len);\n+\temit_binary_diff_body(o, one, two);\n+\temit_binary_diff_body(o, two, one);\n }\n \n int diff_filespec_is_binary(struct diff_filespec *one)\n@@ -2411,7 +2416,7 @@ static void builtin_diff(const char *name_a,\n \t\t\temit_line(o, 0, 0, header.buf, header.len);\n \t\tstrbuf_reset(&header);\n \t\tif (DIFF_OPT_TST(o, BINARY))\n-\t\t\temit_binary_diff(o->file, &mf1, &mf2, line_prefix);\n+\t\t\temit_binary_diff(o, &mf1, &mf2);\n \t\telse\n \t\t\temit_line_fmt(o, 0, 0, \"Binary files %s and %s differ\\n\",\n \t\t\t\t lbl[0], lbl[1]);\n-- \n2.10.0.21.g1da280f.dirty\n\n"},{"id":"301833","messageId":"20160913044613.1037-17-sbeller@google.com","threadId":"44071","inReplyTo":"20160913044613.1037-1-sbeller@google.com","subject":"[RFC/PATCH 16/17] diff: buffer output in emit_line_0","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-13T04:46:12Z","receivedAt":"2016-09-13T04:47:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"emit_line_0 factors out the emission part into emit_line_emission,\nand depending on the diff_options->use_buffer the emission\nwill be performed directly when calling emit_line_0 or after the\nwhole process is done, i.e. by buffering we have add the possibility\nfor a second pass over the whole output before doing the actual\noutput.\n\nIn 6440d34 (2012-03-14, diff: tweak a _copy_ of diff_options with\nword-diff) we introduced a duplicate diff options struct for word\nemissions as we may have different regex settings in there.\n\nWhen buffering the output, we need to operate on just one buffer,\nso we have to copy back the emissions of the word buffer into the main\nbuffer.\n\nUnconditionally enable output via buffer in this patch as it yields\na great opportunity for testing, i.e. all the diff tests from the\ntest suite pass without having reordering issues (i.e. only parts\nof the output got buffered, and we forgot to buffer other parts).\nThe test suite passes, which gives confidence that we converted all\nfunctions to use emit_line_* for output.\n\nSigned-off-by: Stefan Beller \n---\n diff.c | 151 +++++++++++++++++++++++++++++++++++++++++++++++++++++------------\n diff.h | 18 ++++++++\n 2 files changed, 142 insertions(+), 27 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 68da135..e240e89 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -449,42 +449,86 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,\n \tecbdata->blank_at_eof_in_postimage = (at - l2) + 1;\n }\n \n-static void emit_line_0(struct diff_options *o, const char *set, const char *reset,\n-\t\t\tint first, const char *line, int len)\n+static void emit_line_emission(struct diff_options *o, struct line_emission *e)\n {\n-\tint has_trailing_newline, has_trailing_carriage_return;\n-\tint nofirst;\n \tFILE *file = o->file;\n \n \tfputs(diff_line_prefix(o), file);\n \n+\tif (e->len || e->first) {\n+\t\tif (e->set)\n+\t\t\tfputs(e->set, file);\n+\t\tif (e->first)\n+\t\t\tfputc(e->first, file);\n+\t\tif (e->whitespace_check) {\n+\t\t\tif (e->reset)\n+\t\t\t\tfputs(e->reset, file);\n+\t\t\tws_check_emit(e->line, e->len, e->ws_rule,\n+\t\t\t\t file, e->set, e->reset, e->ws);\n+\t\t} else {\n+\t\t\tfwrite(e->line, e->len, 1, file);\n+\t\t\tif (e->reset)\n+\t\t\t\tfputs(e->reset, file);\n+\t\t}\n+\t}\n+\tif (e->has_trailing_carriage_return)\n+\t\tfputc('\\r', file);\n+\tif (e->has_trailing_newline)\n+\t\tfputc('\\n', file);\n+}\n+\n+static void free_line_emission(struct line_emission *e)\n+{\n+\t/*\n+\t * No need to free set, reset, ws as they always point to the\n+\t * diff_colors[] static array. We don't own that memory.\n+\t */\n+\tfree((char*)e->line);\n+}\n+\n+static void append_line_emission_to_buffer(struct diff_options *o,\n+\t\t\t struct line_emission *e)\n+{\n+\tstruct line_emission *f;\n+\tALLOC_GROW(o->line_buffer,\n+\t\t o->line_buffer_nr + 1,\n+\t\t o->line_buffer_alloc);\n+\tf = &o->line_buffer[o->line_buffer_nr++];\n+\tmemcpy(f, e, sizeof(*e));\n+\n+\t/* duplicate the line for now as it is not a stable pointer */\n+\tf->line = xmemdupz(e->line, e->len);\n+}\n+\n+static void emit_line_0(struct diff_options *o, const char *set,\n+\t\t\tconst char *reset, int first, const char *line, int len)\n+{\n+\tint nofirst;\n+\tstruct line_emission e = LINE_EMISSION_INIT;\n+\n \tif (len == 0) {\n-\t\thas_trailing_newline = (first == '\\n');\n-\t\thas_trailing_carriage_return = (first == '\\r');\n-\t\tnofirst = has_trailing_newline || has_trailing_carriage_return;\n+\t\te.has_trailing_newline = (first == '\\n');\n+\t\te.has_trailing_carriage_return = (first == '\\r');\n+\t\tnofirst = e.has_trailing_newline || e.has_trailing_carriage_return;\n \t} else {\n-\t\thas_trailing_newline = (len > 0 && line[len-1] == '\\n');\n-\t\tif (has_trailing_newline)\n+\t\te.has_trailing_newline = (len > 0 && line[len-1] == '\\n');\n+\t\tif (e.has_trailing_newline)\n \t\t\tlen--;\n-\t\thas_trailing_carriage_return = (len > 0 && line[len-1] == '\\r');\n-\t\tif (has_trailing_carriage_return)\n+\t\te.has_trailing_carriage_return = (len > 0 && line[len-1] == '\\r');\n+\t\tif (e.has_trailing_carriage_return)\n \t\t\tlen--;\n \t\tnofirst = 0;\n \t}\n \n-\tif (len || !nofirst) {\n-\t\tif (set)\n-\t\t\tfputs(set, file);\n-\t\tif (!nofirst)\n-\t\t\tfputc(first, file);\n-\t\tfwrite(line, len, 1, file);\n-\t\tif (reset)\n-\t\t\tfputs(reset, file);\n-\t}\n-\tif (has_trailing_carriage_return)\n-\t\tfputc('\\r', file);\n-\tif (has_trailing_newline)\n-\t\tfputc('\\n', file);\n+\te.set = set;\n+\te.first = !nofirst ? first : 0;\n+\te.line = line;\n+\te.len = len;\n+\te.reset = reset;\n+\tif (!o->use_buffer)\n+\t\temit_line_emission(o, &e);\n+\telse\n+\t\tappend_line_emission_to_buffer(o, &e);\n }\n \n static void emit_line(struct diff_options *o, const char *set, const char *reset,\n@@ -540,9 +584,22 @@ static void emit_line_checked(const char *reset,\n \t\temit_line_0(ecbdata->opt, ws, reset, sign, line, len);\n \telse {\n \t\t/* Emit just the prefix, then the rest. */\n-\t\temit_line_0(ecbdata->opt, set, reset, sign, \"\", 0);\n-\t\tws_check_emit(line, len, ecbdata->ws_rule,\n-\t\t\t ecbdata->opt->file, set, reset, ws);\n+\t\tif (ecbdata->opt->use_buffer) {\n+\t\t\tstruct line_emission e;\n+\t\t\te.line = line;\n+\t\t\te.len = len;\n+\t\t\te.ws_rule = ecbdata->ws_rule;\n+\t\t\te.set = set;\n+\t\t\te.reset = reset;\n+\t\t\te.ws = ws;\n+\t\t\te.whitespace_check = 1;\n+\t\t\te.first = sign;\n+\t\t\tappend_line_emission_to_buffer(ecbdata->opt, &e);\n+\t\t} else {\n+\t\t\temit_line_0(ecbdata->opt, set, reset, sign, \"\", 0);\n+\t\t\tws_check_emit(line, len, ecbdata->ws_rule,\n+\t\t\t\t ecbdata->opt->file, set, reset, ws);\n+\t\t}\n \t}\n }\n \n@@ -1093,6 +1150,22 @@ static void diff_words_flush(struct emit_callback *ecbdata)\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+\tif (ecbdata->diff_words->opt->line_buffer_nr) {\n+\t\tint i;\n+\t\tfor (i = 0; i < ecbdata->diff_words->opt->line_buffer_nr; i++) {\n+\t\t\tstruct line_emission *e =\n+\t\t\t\t&ecbdata->diff_words->opt->line_buffer[i];\n+\t\t\tALLOC_GROW(ecbdata->opt->line_buffer,\n+\t\t\t\t ecbdata->opt->line_buffer_nr + 1,\n+\t\t\t\t ecbdata->opt->line_buffer_alloc);\n+\t\t\tmemcpy(&ecbdata->opt->line_buffer\n+\t\t\t\t\t[ecbdata->opt->line_buffer_nr],\n+\t\t\t\te, sizeof(*e));\n+\t\t\tecbdata->opt->line_buffer_nr++;\n+\t\t}\n+\t\tecbdata->diff_words->opt->line_buffer_nr = 0;\n+\t}\n }\n \n static void diff_filespec_load_driver(struct diff_filespec *one)\n@@ -1128,6 +1201,12 @@ static void init_diff_words_data(struct emit_callback *ecbdata,\n \t\txcalloc(1, sizeof(struct diff_words_data));\n \tecbdata->diff_words->type = o->word_diff;\n \tecbdata->diff_words->opt = o;\n+\n+\t/* Create our own buffer if needed. */\n+\to->line_buffer = NULL;\n+\to->line_buffer_nr = 0;\n+\to->line_buffer_alloc = 0;\n+\n \tif (!o->word_regex)\n \t\to->word_regex = userdiff_word_regex(one);\n \tif (!o->word_regex)\n@@ -4649,11 +4728,29 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)\n {\n \tint i;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n+\t/*\n+\t * TODO:\n+\t * For testing purposes we want to make sure the diff machinery\n+\t * works with the buffer. If there is anything emitted outside the\n+\t * emit_line_emission, then the order is screwed up and the tests\n+\t * will fail.\n+\t *\n+\t * We'll unset this flag in a later patch.\n+\t */\n+\to->use_buffer = 1;\n \tfor (i = 0; i < q->nr; i++) {\n \t\tstruct diff_filepair *p = q->queue[i];\n \t\tif (check_pair_status(p))\n \t\t\tdiff_flush_patch(p, o);\n \t}\n+\n+\tif (o->use_buffer) {\n+\t\tfor (i = 0; i < o->line_buffer_nr; i++) {\n+\t\t\temit_line_emission(o, &o->line_buffer[i]);\n+\t\t\tfree_line_emission(&o->line_buffer[i]);\n+\t\t}\n+\t\to->line_buffer_nr = 0;\n+\t}\n }\n \n void diff_flush(struct diff_options *options)\ndiff --git a/diff.h b/diff.h\nindex cc5d038..4df6aa5 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -110,6 +110,20 @@ enum diff_words_type {\n \tDIFF_WORDS_COLOR\n };\n \n+struct line_emission {\n+\tconst char *set;\n+\tconst char *line;\n+\tconst char *ws;\n+\tconst char *reset;\n+\tint first;\n+\tint len;\n+\tint whitespace_check;\n+\tunsigned ws_rule;\n+\tint has_trailing_carriage_return;\n+\tint has_trailing_newline;\n+};\n+#define LINE_EMISSION_INIT {NULL, NULL, NULL, NULL, 0, 0, 0, 0, 0, 0 }\n+\n struct diff_options {\n \tconst char *orderfile;\n \tconst char *pickaxe;\n@@ -178,6 +192,10 @@ struct diff_options {\n \tvoid *output_prefix_data;\n \n \tint diff_path_counter;\n+\n+\tint use_buffer;\n+\tstruct line_emission *line_buffer;\n+\tint line_buffer_nr, line_buffer_alloc;\n };\n \n void emit_line_fmt(struct diff_options *o, const char *set, const char *reset,\n-- \n2.10.0.21.g1da280f.dirty\n\n"},{"id":"301834","messageId":"20160913044613.1037-16-sbeller@google.com","threadId":"44071","inReplyTo":"20160913044613.1037-1-sbeller@google.com","subject":"[RFC/PATCH 15/17] diff.c: convert diff_summary to use emit_line_*","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-13T04:46:11Z","receivedAt":"2016-09-13T04:47:11Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In a later patch, I want to propose an option to detect&color\nmoved lines in a diff, which cannot be done in a one-pass over\nthe diff. Instead we need to go over the whole diff twice,\nbecause we cannot detect the first line of the two corresponding\nlines (+ and -) that got moved.\n\nSo to prepare the diff machinery for two pass algorithms\n(i.e. buffer it all up and then operate on the result),\nmove all emissions to places, such that the only emitting\nfunction is emit_line_0.\n\nThis covers diff_summary.\n\nSigned-off-by: Stefan Beller \n---\n diff.c | 62 +++++++++++++++++++++++++++++++++-----------------------------\n 1 file changed, 33 insertions(+), 29 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 2b1ebcc..68da135 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4382,67 +4382,71 @@ static void flush_one_pair(struct diff_filepair *p, struct diff_options *opt)\n \t}\n }\n \n-static void show_file_mode_name(FILE *file, const char *newdelete, struct diff_filespec *fs)\n+static void show_file_mode_name(struct diff_options *opt, const char *newdelete, struct diff_filespec *fs)\n {\n+\tstruct strbuf sb = STRBUF_INIT;\n \tif (fs->mode)\n-\t\tfprintf(file, \" %s mode %06o \", newdelete, fs->mode);\n+\t\tstrbuf_addf(&sb, \" %s mode %06o \", newdelete, fs->mode);\n \telse\n-\t\tfprintf(file, \" %s \", newdelete);\n-\twrite_name_quoted(fs->path, file, '\\n');\n+\t\tstrbuf_addf(&sb, \" %s \", newdelete);\n+\n+\tquote_c_style(fs->path, &sb, NULL, 0);\n+\tstrbuf_addch(&sb, '\\n');\n+\temit_line(opt, NULL, NULL, sb.buf, sb.len);\n+\tstrbuf_release(&sb);\n }\n \n \n-static void show_mode_change(FILE *file, struct diff_filepair *p, int show_name,\n-\t\tconst char *line_prefix)\n+static void show_mode_change(struct diff_options *opt, struct diff_filepair *p,\n+\t\tint show_name)\n {\n \tif (p->one->mode && p->two->mode && p->one->mode != p->two->mode) {\n-\t\tfprintf(file, \"%s mode change %06o => %06o%c\", line_prefix, p->one->mode,\n-\t\t\tp->two->mode, show_name ? ' ' : '\\n');\n+\t\tstruct strbuf sb = STRBUF_INIT;\n \t\tif (show_name) {\n-\t\t\twrite_name_quoted(p->two->path, file, '\\n');\n+\t\t\tstrbuf_addch(&sb, ' ');\n+\t\t\tquote_c_style(p->two->path, &sb, NULL, 0);\n \t\t}\n+\t\temit_line_fmt(opt, NULL, NULL,\n+\t\t\t \" mode change %06o => %06o%s\\n\",\n+\t\t\t p->one->mode, p->two->mode,\n+\t\t\t show_name ? sb.buf : \"\");\n+\t\tstrbuf_release(&sb);\n \t}\n }\n \n-static void show_rename_copy(FILE *file, const char *renamecopy, struct diff_filepair *p,\n-\t\t\tconst char *line_prefix)\n+static void show_rename_copy(struct diff_options *opt, const char *renamecopy,\n+\t\tstruct diff_filepair *p)\n {\n \tchar *names = pprint_rename(p->one->path, p->two->path);\n-\n-\tfprintf(file, \" %s %s (%d%%)\\n\", renamecopy, names, similarity_index(p));\n+\temit_line_fmt(opt, NULL, NULL, \" %s %s (%d%%)\\n\", renamecopy, names, similarity_index(p));\n \tfree(names);\n-\tshow_mode_change(file, p, 0, line_prefix);\n+\tshow_mode_change(opt, p, 0);\n }\n \n static void diff_summary(struct diff_options *opt, struct diff_filepair *p)\n {\n-\tFILE *file = opt->file;\n-\tconst char *line_prefix = diff_line_prefix(opt);\n-\n \tswitch(p->status) {\n \tcase DIFF_STATUS_DELETED:\n-\t\tfputs(line_prefix, file);\n-\t\tshow_file_mode_name(file, \"delete\", p->one);\n+\t\tshow_file_mode_name(opt, \"delete\", p->one);\n \t\tbreak;\n \tcase DIFF_STATUS_ADDED:\n-\t\tfputs(line_prefix, file);\n-\t\tshow_file_mode_name(file, \"create\", p->two);\n+\t\tshow_file_mode_name(opt, \"create\", p->two);\n \t\tbreak;\n \tcase DIFF_STATUS_COPIED:\n-\t\tfputs(line_prefix, file);\n-\t\tshow_rename_copy(file, \"copy\", p, line_prefix);\n+\t\tshow_rename_copy(opt, \"copy\", p);\n \t\tbreak;\n \tcase DIFF_STATUS_RENAMED:\n-\t\tfputs(line_prefix, file);\n-\t\tshow_rename_copy(file, \"rename\", p, line_prefix);\n+\t\tshow_rename_copy(opt, \"rename\", p);\n \t\tbreak;\n \tdefault:\n \t\tif (p->score) {\n-\t\t\tfprintf(file, \"%s rewrite \", line_prefix);\n-\t\t\twrite_name_quoted(p->two->path, file, ' ');\n-\t\t\tfprintf(file, \"(%d%%)\\n\", similarity_index(p));\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\t\tstrbuf_addstr(&sb, \" rewrite \");\n+\t\t\tquote_c_style(p->two->path, &sb, NULL, 0);\n+\t\t\tstrbuf_addf(&sb, \" (%d%%)\\n\", similarity_index(p));\n+\t\t\temit_line(opt, NULL, NULL, sb.buf, sb.len);\n \t\t}\n-\t\tshow_mode_change(file, p, !p->score, line_prefix);\n+\t\tshow_mode_change(opt, p, !p->score);\n \t\tbreak;\n \t}\n }\n-- \n2.10.0.21.g1da280f.dirty\n\n"},{"id":"301843","messageId":"848f36c9-4ac9-5316-1143-c47b48790847@web.de","threadId":"44071","inReplyTo":"20160913044613.1037-2-sbeller@google.com","subject":"Re: [RFC/PATCH 01/17] diff: move line ending check into emit_hunk_header","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-09-13T14:42:52Z","receivedAt":"2016-09-13T14:43:33Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 13.09.2016 um 06:45 schrieb Stefan Beller:\n> In a later patch, I want to propose an option to detect&color\n> moved lines in a diff, which cannot be done in a one-pass over\n> the diff. Instead we need to go over the whole diff twice,\n> because we cannot detect the first line of the two corresponding\n> lines (+ and -) that got moved.\n>\n> So to prepare the diff machinery for two pass algorithms\n> (i.e. buffer it all up and then operate on the result),\n> move all emissions to places, such that the only emitting\n> function is emit_line_0.\n>\n> This patch moves code that is conceptually part of\n> emit_hunk_header, but was using output in fn_out_consume,\n> back to emit_hunk_header.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> diff.c | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index cc8e812..aa50b2d 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -610,6 +610,9 @@ static void emit_hunk_header(struct emit_callback *ecbdata,\n> \t}\n>\n> \tstrbuf_add(&msgbuf, line + len, org_len - len);\n> +\tif (line[org_len - 1] != '\\n')\n> +\t\tstrbuf_addch(&msgbuf, '\\n');\n> +\n\nUsing strbuf_complete_line() would be nicer.\n\n> \temit_line(ecbdata->opt, \"\", \"\", msgbuf.buf, msgbuf.len);\n> \tstrbuf_release(&msgbuf);\n> }\n> @@ -1247,8 +1250,6 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)\n> \t\tlen = sane_truncate_line(ecbdata, line, len);\n> \t\tfind_lno(line, ecbdata);\n> \t\temit_hunk_header(ecbdata, line, len);\n> -\t\tif (line[len-1] != '\\n')\n> -\t\t\tputc('\\n', o->file);\n> \t\treturn;\n> \t}\n>\n>\n\n"},{"id":"301879","messageId":"CAGZ79kYriQpGd+V00c+90iTfpFo8tR2Os=rYBZR28p07mySeXQ@mail.gmail.com","threadId":"44071","inReplyTo":"848f36c9-4ac9-5316-1143-c47b48790847@web.de","subject":"Re: [RFC/PATCH 01/17] diff: move line ending check into emit_hunk_header","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-13T22:40:24Z","receivedAt":"2016-09-13T22:40:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Sep 13, 2016 at 7:42 AM, René Scharfe wrote:\n\n>>\n>> strbuf_add(&msgbuf, line + len, org_len - len);\n>> + if (line[org_len - 1] != '\\n')\n>> + strbuf_addch(&msgbuf, '\\n');\n>> +\n>\n>\n> Using strbuf_complete_line() would be nicer.\n\nThat makes sense!\n\nThanks,\nStefan\n"},{"id":"301881","messageId":"xmqqy42vtnub.fsf@gitster.mtv.corp.google.com","threadId":"44071","inReplyTo":"20160913044613.1037-6-sbeller@google.com","subject":"Re: [RFC/PATCH 05/17] diff.c: emit_line_0 can handle no color setting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-13T22:51:56Z","receivedAt":"2016-09-13T22:52:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> In a later patch, I want to propose an option to detect&color\n> moved lines in a diff, which cannot be done in a one-pass over\n> the diff. Instead we need to go over the whole diff twice,\n> because we cannot detect the first line of the two corresponding\n> lines (+ and -) that got moved.\n>\n> So to prepare the diff machinery for two pass algorithms\n> (i.e. buffer it all up and then operate on the result),\n> move all emissions to places, such that the only emitting\n> function is emit_line_0.\n>\n> In later patches we may pass lines that are not colored to\n> the central function emit_line_0, so we\n> need to emit the color only when it is non-NULL.\n\nExplained this way, a reader would find that this step is here\nbefore the underlying code is ready--we are not even buffering\nat this step yet.\n\nBut that is OK. It used to be that passing \"\" as set/reset was the\nway to get a --no-color output. Now you can pass NULL instead of\nempty strings. That would be an alternative explanation why this is\nan acceptable change (as your later step probably has a good reason\nwhy it cannot pass \"\" instead of NULL).\n\n>\n> Signed-off-by: Stefan Beller \n> ---\n> diff.c | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index b6a40ae..5d57130 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -473,11 +473,13 @@ static void emit_line_0(struct diff_options *o, const char *set, const char *res\n> \t}\n> \n> \tif (len || !nofirst) {\n> -\t\tfputs(set, file);\n> +\t\tif (set)\n> +\t\t\tfputs(set, file);\n> \t\tif (!nofirst)\n> \t\t\tfputc(first, file);\n> \t\tfwrite(line, len, 1, file);\n> -\t\tfputs(reset, file);\n> +\t\tif (reset)\n> +\t\t\tfputs(reset, file);\n> \t}\n> \tif (has_trailing_carriage_return)\n> \t\tfputc('\\r', file);\n"},{"id":"301882","messageId":"xmqqtwdjtnn4.fsf@gitster.mtv.corp.google.com","threadId":"44071","inReplyTo":"20160913044613.1037-7-sbeller@google.com","subject":"Re: [RFC/PATCH 06/17] diff.c: convert fn_out_consume to use emit_line_*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-13T22:56:15Z","receivedAt":"2016-09-13T22:56:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> In a later patch, I want to propose an option to detect&color\n> moved lines in a diff, which cannot be done in a one-pass over\n> the diff. Instead we need to go over the whole diff twice,\n> because we cannot detect the first line of the two corresponding\n> lines (+ and -) that got moved.\n>\n> So to prepare the diff machinery for two pass algorithms\n> (i.e. buffer it all up and then operate on the result),\n> move all emissions to places, such that the only emitting\n> function is emit_line_0.\n>\n> This covers the remaining parts of fn_out_consume.\n\nname_x_tab are colored as before, which you are already aware of and\nwe'd need to find a way to handle, but other than that, this is a\nno-op conversion, getting us closer to the goal of making everything\ngo through a single funnel.\n\n> \t\tname_a_tab = strchr(ecbdata->label_path[0], ' ') ? \"\\t\" : \"\";\n> \t\tname_b_tab = strchr(ecbdata->label_path[1], ' ') ? \"\\t\" : \"\";\n> -\n> -\t\tfprintf(o->file, \"%s%s--- %s%s%s\\n\",\n> -\t\t\tline_prefix, meta, ecbdata->label_path[0], reset, name_a_tab);\n> -\t\tfprintf(o->file, \"%s%s+++ %s%s%s\\n\",\n> -\t\t\tline_prefix, meta, ecbdata->label_path[1], reset, name_b_tab);\n> +\t\temit_line_fmt(o, meta, reset, \"--- %s%s\\n\",\n> +\t\t\t ecbdata->label_path[0], name_a_tab);\n> +\t\temit_line_fmt(o, meta, reset, \"+++ %s%s\\n\",\n> +\t\t\t ecbdata->label_path[1], name_b_tab);\n> \t\tecbdata->label_path[0] = ecbdata->label_path[1] = NULL;\n> \t}\n"},{"id":"301883","messageId":"xmqqpoo7tnco.fsf@gitster.mtv.corp.google.com","threadId":"44071","inReplyTo":"20160913044613.1037-11-sbeller@google.com","subject":"Re: [RFC/PATCH 10/17] submodule.c: convert show_submodule_summary to use emit_line_fmt","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-13T23:02:31Z","receivedAt":"2016-09-13T23:02:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> In a later patch, I want to propose an option to detect&color\n> moved lines in a diff, which cannot be done in a one-pass over\n> the diff. Instead we need to go over the whole diff twice,\n> because we cannot detect the first line of the two corresponding\n> lines (+ and -) that got moved.\n>\n> So to prepare the diff machinery for two pass algorithms\n> (i.e. buffer it all up and then operate on the result),\n> move all emissions to places, such that the only emitting\n> function is emit_line_0.\n>\n> This prepares the code for submodules to go through the\n> emit_line_0 function.\n>\n> Signed-off-by: Stefan Beller \n> ---\n\nI wonder how this interacts with the jk/diff-submodule-diff-inline\ntopic by Jacob that has graduated recently to 'master'. IIRC, it\njust lets a separate \"git diff\" instance that is spawned in the\nsubmodule directory emit its findings to the output of the driving\n\"git diff\" in the superproject.\n\n"},{"id":"301884","messageId":"xmqqlgyvtn6l.fsf@gitster.mtv.corp.google.com","threadId":"44071","inReplyTo":"20160913044613.1037-17-sbeller@google.com","subject":"Re: [RFC/PATCH 16/17] diff: buffer output in emit_line_0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-13T23:06:10Z","receivedAt":"2016-09-13T23:06:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> +struct line_emission {\n> +\tconst char *set;\n> +\tconst char *line;\n> +\tconst char *ws;\n> +\tconst char *reset;\n> +\tint first;\n> +\tint len;\n> +\tint whitespace_check;\n> +\tunsigned ws_rule;\n> +\tint has_trailing_carriage_return;\n> +\tint has_trailing_newline;\n> +};\n\nIt is somewhat strange to see whitespace things are per-line here.\nI'd understand it if it were per-path, though.\n"},{"id":"301886","messageId":"CAGZ79kbYNkvLC2A5oQ5FahrPQGe5syaF5_LXi-ddCRzC4HJH=Q@mail.gmail.com","threadId":"44071","inReplyTo":"xmqqpoo7tnco.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH 10/17] submodule.c: convert show_submodule_summary to use emit_line_fmt","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-13T23:09:41Z","receivedAt":"2016-09-13T23:09:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Sep 13, 2016 at 4:02 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> In a later patch, I want to propose an option to detect&color\n>> moved lines in a diff, which cannot be done in a one-pass over\n>> the diff. Instead we need to go over the whole diff twice,\n>> because we cannot detect the first line of the two corresponding\n>> lines (+ and -) that got moved.\n>>\n>> So to prepare the diff machinery for two pass algorithms\n>> (i.e. buffer it all up and then operate on the result),\n>> move all emissions to places, such that the only emitting\n>> function is emit_line_0.\n>>\n>> This prepares the code for submodules to go through the\n>> emit_line_0 function.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>\n> I wonder how this interacts with the jk/diff-submodule-diff-inline\n> topic by Jacob that has graduated recently to 'master'. IIRC, it\n> just lets a separate \"git diff\" instance that is spawned in the\n> submodule directory emit its findings to the output of the driving\n> \"git diff\" in the superproject.\n>\n\nThe easiest way to find out is to merge HEAD^ of this patch series\n(i.e. \"diff: buffer output in emit_line_0\") with whatever we suspect can\ncause breakage for the goal of channeling everything though emit_line_*\nfunctions. Looking at that series, I think I'll have to redo\nthis (maybe even including sb/diff-cleanup, to have it all in one series)\nto capture all output there.\n\nI suspected a breakage, but as the patch series grew larger and larger,\nI first wanted to get into a working state before paying attention to solving\nconflicts as resolving conflicts is easier when I know where this series is\nheaded.\n\nThanks!\nStefan\n"},{"id":"301888","messageId":"CAGZ79kb4Rm=AT-hFutXW5jhZRL0ThDFr789=AbxKC-o+jQ341g@mail.gmail.com","threadId":"44071","inReplyTo":"xmqqlgyvtn6l.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH 16/17] diff: buffer output in emit_line_0","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-13T23:28:06Z","receivedAt":"2016-09-13T23:28:12Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Sep 13, 2016 at 4:06 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> +struct line_emission {\n>> + const char *set;\n>> + const char *line;\n>> + const char *ws;\n>> + const char *reset;\n>> + int first;\n>> + int len;\n>> + int whitespace_check;\n>> + unsigned ws_rule;\n>> + int has_trailing_carriage_return;\n>> + int has_trailing_newline;\n>> +};\n>\n> It is somewhat strange to see whitespace things are per-line here.\n> I'd understand it if it were per-path, though.\n\nYeah we have to have it at least per path as that is the granularity\nthe user can configure it.\n\nSo would we rather want to keep the ecbdata around for each file pair and\njust reference that? I thought we deliberately want to avoid ecbdata, so maybe\nwe rather want to have another struct that keeps path related information\naround (pointer to the blob and white space information).\n\nThanks,\nStefan\n"},{"id":"301889","messageId":"xmqq60pztlz2.fsf@gitster.mtv.corp.google.com","threadId":"44071","inReplyTo":"CAGZ79kb4Rm=AT-hFutXW5jhZRL0ThDFr789=AbxKC-o+jQ341g@mail.gmail.com","subject":"Re: [RFC/PATCH 16/17] diff: buffer output in emit_line_0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-13T23:32:17Z","receivedAt":"2016-09-13T23:32:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> So would we rather want to keep the ecbdata around for each file pair and\n> just reference that? I thought we deliberately want to avoid ecbdata, so maybe\n> we rather want to have another struct that keeps path related information\n> around (pointer to the blob and white space information).\n\nI would expect that there would be two structs, one per path\n\"struct buffered_patch\" that has the per-path thing, and another per\nline \"struct buffered_patch_line\" that describes what each line is,\nand has a pointer to the former.\n\n\n\t\n"},{"id":"301890","messageId":"CAGZ79kaFofpU0TMd1ZcJa9UrzdFytg9UjqqtPf8WPXe0zjWihA@mail.gmail.com","threadId":"44071","inReplyTo":"xmqq60pztlz2.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH 16/17] diff: buffer output in emit_line_0","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-13T23:42:47Z","receivedAt":"2016-09-13T23:42:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Sep 13, 2016 at 4:32 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> So would we rather want to keep the ecbdata around for each file pair and\n>> just reference that? I thought we deliberately want to avoid ecbdata, so maybe\n>> we rather want to have another struct that keeps path related information\n>> around (pointer to the blob and white space information).\n>\n> I would expect that there would be two structs, one per path\n> \"struct buffered_patch\" that has the per-path thing, and another per\n> line \"struct buffered_patch_line\" that describes what each line is,\n> and has a pointer to the former.\n>\n\nHeh, I was trying to come up with a clever thing to save that pointer,\nas we would need to have that pointer once per line, so in large patches\nthat would save a bit of space, but probably I should not try to be too\nsmart about it.\n\nSo I'd split up the struct line_emission into the two proposed\nbuffered_patch_line as well as buffered_patch.\n\nHowever the naming is a bit off than I would expect. Historically you\nhad one patch per file, so it was natural to name a change of multiple\nfiles a \"patchset\" (c.f. a commit in Gerrit is called \"patchset\"/revision)\n\nToday as Git is quite successful, one \"patch\" is easily understood\nas the equivalent of one patch, i.e. what format-patch produced.\n\nSo I'd prefer to go with buffer_filepair and buffer_line maybe?\n"}]}