Annotating patches inside diff

Jakub Narebski· Dec 10, 2008, 13:45 UTC · lore

I remember that long time ago on git mailing list there was discussed extending git-apply and friends (including git-am), to be able to ignore lines in patches with selected special prefix, different from '@' for chunks headers, ' ' for context, '+'/'-' for added/deleted lines. IIRC it was chose '|' for this purpose.

This way you could annotate patch

@@ -4667,7 +4667,6 @@ HTML
                                  hash_base => $parent_commit);
                print "<td class=\"linenr\">";
                print $cgi->a({ -href => "$blamed#l$orig_lineno",
| moved to <tr>
-                               -id => "l$lineno",
                                -class => "linenr" },
                              esc_html($lineno));
                print "</td>";


Was it accepted or dropped, or is this feature present but not 
documented?

-- 
Jakub Narebski
Poland

Johannes Schindelin· Dec 10, 2008, 16:58 UTC · re: Jakub Narebski · lore

Re: Annotating patches inside diff

Hi,

On Wed, 10 Dec 2008, Jakub Narebski wrote:

Show 21 quoted lines

> I remember that long time ago on git mailing list there was discussed 
> extending git-apply and friends (including git-am), to be able to 
> ignore lines in patches with selected special prefix, different from 
> '@' for chunks headers, ' ' for context, '+'/'-' for added/deleted 
> lines.  IIRC it was chose '|' for this purpose.
> 
> This way you could annotate patch
> 
> @@ -4667,7 +4667,6 @@ HTML
>                                   hash_base => $parent_commit);
>                 print "<td class=\"linenr\">";
>                 print $cgi->a({ -href => "$blamed#l$orig_lineno",
> | moved to <tr>
> -                               -id => "l$lineno",
>                                 -class => "linenr" },
>                               esc_html($lineno));
>                 print "</td>";
> 
> 
> Was it accepted or dropped, or is this feature present but not 
> documented?

As I said on IRC, I think that if you are too good in the hiding-comments business, you can just spare the time to write them, 'cause nobody will find them.

IOW such a comment needs to go either into the commit message (if it is an important API change), so that people who do not remember discussions on the mailing list still have a chance to find the comment, or between the message and the diffstat (if it is less important).

Ciao, Dscho

Junio C Hamano· Dec 10, 2008, 18:40 UTC · re: Jakub Narebski · lore

Re: Annotating patches inside diff

Jakub Narebski <jnareb@gmail.com> writes:

Show 5 quoted lines

> I remember that long time ago on git mailing list there was discussed 
> extending git-apply and friends (including git-am), to be able to 
> ignore lines in patches with selected special prefix, different from 
> '@' for chunks headers, ' ' for context, '+'/'-' for added/deleted 
> lines.  IIRC it was chose '|' for this purpose.

That's a blast from the past.

Show 7 quoted lines

> This way you could annotate patch
>
> @@ -4667,7 +4667,6 @@ HTML
> ...
>
> Was it accepted or dropped, or is this feature present but not 
> documented?

The code is simple, but I do not think it is a good idea to do so:

 * If it is about describing the state (what it does, how it does it and
   why), you should be writing that as in-code comments;

 * If it is about describing the change (what it used to do and how, and
   what it does after your change and how, and why you changed it that
   way), i.e. "we used to do X but now we do Y for such and such reasons"
   (your example is a good one, except in real life you would want to
   state "why" as well), you should be writing that in the commit log
   message; and

 * If describing the change in the commit log message feels insufficient,
   it probably is a sign that the patch is too big and covers too many
   topics.

In other words, I think the problem the old patch attempted to solve was this:

   When you do too many things in a patch touching many places of the
   code, and want to explain and justify all of them, your commit log
   message becomes a list of "we used to do X but 7th hunk changes it to Y
   because of Z" entries for different X, Y and Z.  When this list gets
   too big, it is easier to read through the patch if such explanation are
   done close to where the changes described are.

For your reference, this was the dropped patch.

From: Junio C Hamano <junkio@cox.net>
Date: Fri, 8 Dec 2006 21:03:52 -0800
Subject: [PATCH] mailinfo: hack to accept in-line annotations in patches.

Long before git-apply, when I wanted to talk about rationale of individual changes, I used to add annotation between hunks (delimited @@ -n,m, +l,k @@) as unindented plain text and rely on GNU patch to discard them as garbage. Because git-apply is much less forgiving than GNU patch, this is not possible.

This patch teaches mailinfo that lines that begin with a '|' would never appear in the patch text and can be discarded safely. Which means that we can generate a patch as usual using format-patch, and add annotations inline, prefixed with '|'.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 builtin-mailinfo.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index c95e477..2608260 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c
@@ -720,8 +720,10 @@ static int handle_commit_msg(char *line)
 
 static int handle_patch(char *line)
 {
-	fputs(line, patchfile);
-	patch_lines++;
+	if (line[0] != '|') {
+		fputs(line, patchfile);
+		patch_lines++;
+	}
 	return 0;
 }