{"thread":{"id":"16665","subject":"Annotating patches inside diff","startedAt":"2008-12-10T13:45:47Z","lastAt":"2008-12-10T18:40:42Z","messageCount":3,"participants":["Jakub Narebski","Johannes Schindelin","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"97486","messageId":"200812101445.48034.jnareb@gmail.com","threadId":"16665","inReplyTo":null,"subject":"Annotating patches inside diff","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-10T13:45:47Z","receivedAt":"2008-12-10T13:45:47Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"I remember that long time ago on git mailing list there was discussed \nextending git-apply and friends (including git-am), to be able to \nignore lines in patches with selected special prefix, different from \n'@' for chunks headers, ' ' for context, '+'/'-' for added/deleted \nlines.  IIRC it was chose '|' for this purpose.\n\nThis way you could annotate patch\n\n@@ -4667,7 +4667,6 @@ HTML\n                                  hash_base => $parent_commit);\n                print \"<td class=\\\"linenr\\\">\";\n                print $cgi->a({ -href => \"$blamed#l$orig_lineno\",\n| moved to <tr>\n-                               -id => \"l$lineno\",\n                                -class => \"linenr\" },\n                              esc_html($lineno));\n                print \"</td>\";\n\n\nWas it accepted or dropped, or is this feature present but not \ndocumented?\n-- \nJakub Narebski\nPoland\n"},{"id":"97497","messageId":"alpine.DEB.1.00.0812101755480.25069@eeepc-johanness","threadId":"16665","inReplyTo":"200812101445.48034.jnareb@gmail.com","subject":"Re: Annotating patches inside diff","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-12-10T16:58:14Z","receivedAt":"2008-12-10T16:58:14Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 10 Dec 2008, Jakub Narebski wrote:\n\n> I remember that long time ago on git mailing list there was discussed \n> extending git-apply and friends (including git-am), to be able to \n> ignore lines in patches with selected special prefix, different from \n> '@' for chunks headers, ' ' for context, '+'/'-' for added/deleted \n> lines.  IIRC it was chose '|' for this purpose.\n> \n> This way you could annotate patch\n> \n> @@ -4667,7 +4667,6 @@ HTML\n>                                   hash_base => $parent_commit);\n>                 print \"<td class=\\\"linenr\\\">\";\n>                 print $cgi->a({ -href => \"$blamed#l$orig_lineno\",\n> | moved to <tr>\n> -                               -id => \"l$lineno\",\n>                                 -class => \"linenr\" },\n>                               esc_html($lineno));\n>                 print \"</td>\";\n> \n> \n> Was it accepted or dropped, or is this feature present but not \n> documented?\n\nAs I said on IRC, I think that if you are too good in the hiding-comments \nbusiness, you can just spare the time to write them, 'cause nobody will \nfind them.\n\nIOW such a comment needs to go either into the commit message (if it is an \nimportant API change), so that people who do not remember discussions on \nthe mailing list still have a chance to find the comment, or between the \nmessage and the diffstat (if it is less important).\n\nCiao,\nDscho\n"},{"id":"97499","messageId":"7vfxkv27rp.fsf@gitster.siamese.dyndns.org","threadId":"16665","inReplyTo":"200812101445.48034.jnareb@gmail.com","subject":"Re: Annotating patches inside diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-10T18:40:42Z","receivedAt":"2008-12-10T18:40:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> I remember that long time ago on git mailing list there was discussed \n> extending git-apply and friends (including git-am), to be able to \n> ignore lines in patches with selected special prefix, different from \n> '@' for chunks headers, ' ' for context, '+'/'-' for added/deleted \n> lines.  IIRC it was chose '|' for this purpose.\n\nThat's a blast from the past.\n\n> This way you could annotate patch\n>\n> @@ -4667,7 +4667,6 @@ HTML\n> ...\n>\n> Was it accepted or dropped, or is this feature present but not \n> documented?\n\nThe code is simple, but I do not think it is a good idea to do so:\n\n * If it is about describing the state (what it does, how it does it and\n   why), you should be writing that as in-code comments;\n\n * If it is about describing the change (what it used to do and how, and\n   what it does after your change and how, and why you changed it that\n   way), i.e. \"we used to do X but now we do Y for such and such reasons\"\n   (your example is a good one, except in real life you would want to\n   state \"why\" as well), you should be writing that in the commit log\n   message; and\n\n * If describing the change in the commit log message feels insufficient,\n   it probably is a sign that the patch is too big and covers too many\n   topics.\n\nIn other words, I think the problem the old patch attempted to solve was\nthis:\n\n   When you do too many things in a patch touching many places of the\n   code, and want to explain and justify all of them, your commit log\n   message becomes a list of \"we used to do X but 7th hunk changes it to Y\n   because of Z\" entries for different X, Y and Z.  When this list gets\n   too big, it is easier to read through the patch if such explanation are\n   done close to where the changes described are.\n\nFor your reference, this was the dropped patch.\n\nFrom: Junio C Hamano <junkio@cox.net>\nDate: Fri, 8 Dec 2006 21:03:52 -0800\nSubject: [PATCH] mailinfo: hack to accept in-line annotations in patches.\n\nLong before git-apply, when I wanted to talk about rationale of\nindividual changes, I used to add annotation between hunks\n(delimited @@ -n,m, +l,k @@) as unindented plain text and rely\non GNU patch to discard them as garbage.  Because git-apply is\nmuch less forgiving than GNU patch, this is not possible.\n\nThis patch teaches mailinfo that lines that begin with a '|' would\nnever appear in the patch text and can be discarded safely.\nWhich means that we can generate a patch as usual using format-patch,\nand add annotations inline, prefixed with '|'.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\n builtin-mailinfo.c |    6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-mailinfo.c b/builtin-mailinfo.c\nindex c95e477..2608260 100644\n--- a/builtin-mailinfo.c\n+++ b/builtin-mailinfo.c\n@@ -720,8 +720,10 @@ static int handle_commit_msg(char *line)\n \n static int handle_patch(char *line)\n {\n-\tfputs(line, patchfile);\n-\tpatch_lines++;\n+\tif (line[0] != '|') {\n+\t\tfputs(line, patchfile);\n+\t\tpatch_lines++;\n+\t}\n \treturn 0;\n }\n \n"}]}