threads / discuss / 30556

Add comment lines to patch format

Subject: Add comment lines to patch format

## tl;dr

5 messages between May 18, 2012 and May 19, 2012.

replies: 4people: 3as markdown or json

Nguyen Thai Ngoc Duy· May 18, 2012, 13:22 UTC · lore

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· May 18, 2012, 15:30 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: Add comment lines to patch format

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.

Nguyen Thai Ngoc Duy· May 19, 2012, 04:50 UTC · re: Junio C Hamano · lore

Re: Add comment lines to patch format

On Fri, May 18, 2012 at 10:30 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 11 quoted lines

> 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

Jeff King· May 19, 2012, 00:14 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: Add comment lines to patch format

On Fri, May 18, 2012 at 08:22:28PM +0700, Nguyen Thai Ngoc Duy wrote:

Show 8 quoted lines

> 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· May 19, 2012, 04:54 UTC · re: Jeff King · lore

Re: Add comment lines to patch format

On Sat, May 19, 2012 at 7:14 AM, Jeff King <peff@peff.net> wrote:

Show 12 quoted lines

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

Show 15 quoted lines

> 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