# Add comment lines to patch format 5 messages from 2012-05-18 to 2012-05-19. Participants: Nguyen Thai Ngoc Duy, Junio C Hamano, Jeff King. Thread: https://gitlist.dev/t/30556 ## Nguyen Thai Ngoc Duy, 2012-05-18 13:22 Subject: Add comment lines to patch format Message-ID: <20120518132228.GA27970@do> URL: https://gitlist.dev/e/20120518132228.GA27970%40do ``` Hi, This is just an idea to help spot subtle changes in patch form (e.g. visible over email). A line starting with '=' (or something else) is treated like a comment line and ignored by "git apply". We could use this line to mark special things in the previous line. One thing is trailing space, like demonstrated in the patch below, because trailing spaces may be intentional sometimes. But I'd like to incorporate some word-diff goodness in patch format using this comment line to spot few/single character addition/removal. For example, instead of showing - "this is a string" + N_("this is a string") we could show - "this is a string" = ..... .. . ^....... + N_("this is a string") = ^^^..... .. . .......^ If anyone knows a tool with similar feature, I'd greatly appreciate it (as the Internet taught me, everything I think of is already thought of/implemented by someone) -- 8< -- diff --git a/diff.c b/diff.c index 77edd50..09d8c19 100644 --- a/diff.c +++ b/diff.c @@ -1110,6 +1110,30 @@ static void find_lno(const char *line, struct emit_callback *ecbdata) ecbdata->lno_in_postimage = strtol(p + 1, NULL, 10); } +static void annotate_line(struct emit_callback *ecbdata, + const char *line, unsigned long len) +{ + char *buf, *s; + int trailing = 1; + if (!isspace(line[len-2])) = .. .......................^^^ + return; + buf = malloc(len); + memcpy(buf, line, len); + buf[0] = '='; + for (s = buf + len - 2; s > buf; s--) { + if (trailing && isspace(*s)) + *s = '^'; + else + trailing = 0; + if (!trailing && !isspace(*s)) + *s = '.'; + } + emit_line(ecbdata->opt, + diff_get_color(ecbdata->color_diff, DIFF_PLAIN), + diff_get_color(ecbdata->color_diff, DIFF_RESET), buf, len); + free(buf); +} + static void fn_out_consume(void *priv, char *line, unsigned long len) { struct emit_callback *ecbdata = priv; @@ -1208,17 +1232,26 @@ static void fn_out_consume(void *priv, char *line, unsigned long len) return; } - if (line[0] != '+') { - const char *color = - diff_get_color(ecbdata->color_diff, - line[0] == '-' ? DIFF_FILE_OLD : DIFF_PLAIN); + switch (line[0]) { + case '-': ecbdata->lno_in_preimage++; - if (line[0] == ' ') - ecbdata->lno_in_postimage++; - emit_line(ecbdata->opt, color, reset, line, len); - } else { + emit_line(ecbdata->opt, + diff_get_color(ecbdata->color_diff, DIFF_FILE_OLD), + reset, line, len); + annotate_line(ecbdata, line, len); + break; + case '+': ecbdata->lno_in_postimage++; emit_add_line(reset, ecbdata, line + 1, len - 1); + annotate_line(ecbdata, line, len); + break; + case ' ': + ecbdata->lno_in_preimage++; + ecbdata->lno_in_postimage++; + emit_line(ecbdata->opt, plain, reset, line, len); + break; + default: + die("huh? a diff line starting with '%c'??", line[0]); } } -- 8< -- -- Duy -- Duy ``` ## Junio C Hamano, 2012-05-18 15:30 Subject: Re: Add comment lines to patch format Message-ID: URL: https://gitlist.dev/e/xmqq62bt4ida.fsf%40junio.mtv.corp.google.com In-Reply-To: <20120518132228.GA27970@do> ``` Nguyen Thai Ngoc Duy writes: > If anyone knows a tool with similar feature, I'd greatly appreciate it > (as the Internet taught me, everything I think of is already thought > of/implemented by someone) Look in the archive and I think you will find a patch by me that implemented this not with '=' but with some other character, possibly with a matching patch to 'apply' without which its output is not usable. It was back when I was still a contributor, I think, so I do not remember the details. ``` ## Jeff King, 2012-05-19 00:14 Subject: Re: Add comment lines to patch format Message-ID: <20120519001410.GC765@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120519001410.GC765%40sigill.intra.peff.net In-Reply-To: <20120518132228.GA27970@do> ``` On Fri, May 18, 2012 at 08:22:28PM +0700, Nguyen Thai Ngoc Duy wrote: > One thing is trailing space, like demonstrated in the patch below, > because trailing spaces may be intentional sometimes. But I'd like to > incorporate some word-diff goodness in patch format using this comment > line to spot few/single character addition/removal. > [...] > If anyone knows a tool with similar feature, I'd greatly appreciate it > (as the Internet taught me, everything I think of is already thought > of/implemented by someone) Have you looked at contrib/diff-highlight? I think your approach is interesting because it can annotate much more than just "this part is interesting". Sometimes when I send a patch, I'd like to be able to comment in-line, like: + if (bar > 3) + foo(bar); = Yeah, this magic "3" is ugly, and we should handle it = through $whatever in the re-roll. + else + something_else(); I usually just write something in the cover letter, or reply to myself with comments inline. I don't know how much I would use it in practice, but it might be a neat thing for "git apply" to simply ignore comment lines in the middle of a hunk. Of course, this feature might not be worth breaking compatibility with every existing version of "git apply" and "patch" out there. -Peff ``` ## Nguyen Thai Ngoc Duy, 2012-05-19 04:50 Subject: Re: Add comment lines to patch format Message-ID: URL: https://gitlist.dev/e/CACsJy8BMKwyMhREe-VyKNu-G9V-hMhMjWdty3QHHppY329vTbA%40mail.gmail.com In-Reply-To: ``` On Fri, May 18, 2012 at 10:30 PM, Junio C Hamano wrote: > Nguyen Thai Ngoc Duy writes: > >> If anyone knows a tool with similar feature, I'd greatly appreciate it >> (as the Internet taught me, everything I think of is already thought >> of/implemented by someone) > > Look in the archive and I think you will find a patch by me that > implemented this not with '=' but with some other character, possibly > with a matching patch to 'apply' without which its output is not usable. > It was back when I was still a contributor, I think, so I do not > remember the details. Nothing stands out from gmane interface up until you became maintainer. But I might overlook because you exceeded 1000 mails just in three months. That might be Linus' secret criteria for a new maintainer: pick the first one that passes 1000 mails limit ;) Anyway don't waste your time looking back in your mail archive. -- Duy ``` ## Nguyen Thai Ngoc Duy, 2012-05-19 04:54 Subject: Re: Add comment lines to patch format Message-ID: URL: https://gitlist.dev/e/CACsJy8C-%2BVwpR4XEBqxhpwhtfZ%3Dcxo6uKSp6Ov7K6aLGxqR%3DSw%40mail.gmail.com In-Reply-To: <20120519001410.GC765@sigill.intra.peff.net> ``` On Sat, May 19, 2012 at 7:14 AM, Jeff King wrote: > On Fri, May 18, 2012 at 08:22:28PM +0700, Nguyen Thai Ngoc Duy wrote: > >> One thing is trailing space, like demonstrated in the patch below, >> because trailing spaces may be intentional sometimes. But I'd like to >> incorporate some word-diff goodness in patch format using this comment >> line to spot few/single character addition/removal. >> [...] >> If anyone knows a tool with similar feature, I'd greatly appreciate it >> (as the Internet taught me, everything I think of is already thought >> of/implemented by someone) > > Have you looked at contrib/diff-highlight? I didn't. Its output looks too packed though. > I think your approach is interesting because it can annotate much more > than just "this part is interesting". Sometimes when I send a patch, I'd > like to be able to comment in-line, like: > >  +       if (bar > 3) >  +               foo(bar); >  = Yeah, this magic "3" is ugly, and we should handle it >  = through $whatever in the re-roll. >  +       else >  +               something_else(); > > I usually just write something in the cover letter, or reply to myself > with comments inline. I don't know how much I would use it in practice, > but it might be a neat thing for "git apply" to simply ignore comment > lines in the middle of a hunk. Yeah. That's a good thing about in-patch comments. One thing that's still bugging me is that we don't have a way to send machine-readable conflict resolutions in text form. But that's probably something else. > Of course, this feature might not be worth breaking compatibility with > every existing version of "git apply" and "patch" out there. It's opt-in. And it's quite easy to convert back. A simple "sed -i '/^=/d'" would do. -- Duy ```