Add comment lines to patch format

Add comment lines to patch format

[Nguyen Thai Ngoc Duy]

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

Re: Add comment lines to patch format

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> 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.

Re: Add comment lines to patch format

[Jeff King]

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

Re: Add comment lines to patch format

[Nguyen Thai Ngoc Duy]

On Fri, May 18, 2012 at 10:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> 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

Re: Add comment lines to patch format

[Nguyen Thai Ngoc Duy]

On Sat, May 19, 2012 at 7:14 AM, Jeff King <peff@peff.net> 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