Annotating patches inside diff

Annotating patches inside diff

[Jakub Narebski]

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

Re: Annotating patches inside diff

[Johannes Schindelin]

Hi,

On Wed, 10 Dec 2008, Jakub Narebski wrote:

> 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

Re: Annotating patches inside diff

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> 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.

> 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;
 }