[PATCH] patch-id: Add support for mbox format

[PATCH] patch-id: Add support for mbox format

[Paolo Bonzini]

I have an alias that takes two arguments and compares their patch IDs.
I would like to use to make sure I've tested exactly what I submit
(patch by patch), like

   git patch-cmp origin/master.. file-being-sent

However, I cannot do that because git patch-id is fooled by the
"-- " trailer that git format-patch puts.

This patch adds a bit of lookahead to git patch-id in order to detect
the trailer and split the patch when it comes.  In addition, commit
ids in the "From " lines are considered and printed in the output.

Signed-off-by: Paolo Bonzini <bonzini@gnu.org>
---
	I couldn't resist testing it right away:

	$ git format-patch -1
	0001-patch-id-Add-support-for-mbox-format.patch

	### With the git I have installed:
	$ git patch-cmp HEAD^! 0001-patch-id-Add-support-for-mbox-format.patch
	HEAD^! and 0001-patch-id-Add-support-for-mbox-format.patch differ

	### With this patch:
	$ PATH=.:$PATH !!
	HEAD^! and 0001-patch-id-Add-support-for-mbox-format.patch are the same

 builtin/patch-id.c  |   23 ++++++++++++++++++++++-
 t/t4204-patch-id.sh |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 1 deletions(-)

diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index af0911e..895c526 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -34,17 +34,22 @@ static void generate_id_list(void)
 	static char line[1000];
 	git_SHA_CTX ctx;
 	int patchlen = 0;
+	int have_line = 0;
 
 	git_SHA1_Init(&ctx);
-	while (fgets(line, sizeof(line), stdin) != NULL) {
+	while (!feof (stdin) &&
+	       (have_line || fgets(line, sizeof(line), stdin) != NULL)) {
 		unsigned char n[20];
 		char *p = line;
 		int len;
 
+		have_line = 0;
 		if (!memcmp(line, "diff-tree ", 10))
 			p += 10;
 		else if (!memcmp(line, "commit ", 7))
 			p += 7;
+		else if (!memcmp(line, "From ", 5))
+			p += 5;
 
 		if (!get_sha1_hex(p, n)) {
 			flush_current_id(patchlen, sha1, &ctx);
@@ -67,6 +72,22 @@ static void generate_id_list(void)
 
 		/* Compute the sha without whitespace */
 		len = remove_space(line);
+
+		/* Split at -- followed by version number.  */
+		if (len == 2 && line[0] == '-' && line[1] == '-') {
+			if (fgets(line, sizeof(line), stdin) != NULL &&
+			    isdigit (*line)) {
+				flush_current_id(patchlen, sha1, &ctx);
+				patchlen = 0;
+			} else {
+				/* Just a normal unified diff line.  */
+				patchlen += 2;
+				git_SHA1_Update(&ctx, "--", 2);
+				have_line = 1;
+			}
+			continue;
+		}
+
 		patchlen += len;
 		git_SHA1_Update(&ctx, line, len);
 	}
diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh
index 04f7bae..2110eac 100755
--- a/t/t4204-patch-id.sh
+++ b/t/t4204-patch-id.sh
@@ -18,6 +18,11 @@ test_expect_success 'patch-id output is well-formed' '
 	grep "^[a-f0-9]\{40\} $(git rev-parse HEAD)$" output
 '
 
+get_text_patch_id () {
+	tr % '\n' | git patch-id |
+		sed "s# .*##" > patch-id_"$1"
+}
+
 get_patch_id () {
 	git log -p -1 "$1" | git patch-id |
 		sed "s# .*##" > patch-id_"$1"
@@ -35,4 +40,31 @@ test_expect_success 'patch-id detects inequality' '
 	! test_cmp patch-id_master patch-id_notsame
 '
 
+test_expect_success 'patch-id does not discard meaningful -- line' '
+	echo "diff a b%--%-ab%-cd" | get_text_patch_id first &&
+	echo "diff a b%-ab%-cd" | get_text_patch_id second &&
+	! test_cmp patch-id_first patch-id_second
+'
+
+test_expect_success 'patch-id does not discard meaningful final -- line' '
+	echo "diff a b%-ab%-cd%--" | get_text_patch_id first &&
+	echo "diff a b%-ab%-cd" | get_text_patch_id second &&
+	! test_cmp patch-id_first patch-id_second
+'
+
+test_expect_success 'whitespace is irrelevant in footer' '
+	echo "diff a b%--%-ab%-cd%-- %1.2" | get_text_patch_id first &&
+	echo "diff a b%--%-ab%-cd%--%1.2" | get_text_patch_id second &&
+	test_cmp patch-id_first patch-id_second
+'
+
+test_expect_success 'patch-id supports git-format-patch output' '
+	get_patch_id master &&
+	git checkout same &&
+	git format-patch -1 --stdout | get_text_patch_id same &&
+	test_cmp patch-id_master patch-id_same &&
+	set `git format-patch -1 --stdout | git patch-id` &&
+	test "$2" = `git rev-parse HEAD`
+'
+
 test_done
-- 
1.6.6.1

Re: [PATCH] patch-id: Add support for mbox format

[Junio C Hamano]

Paolo Bonzini <bonzini@gnu.org> writes:

> I have an alias that takes two arguments and compares their patch IDs.
> I would like to use to make sure I've tested exactly what I submit
> (patch by patch), like
>
>    git patch-cmp origin/master.. file-being-sent
>
> However, I cannot do that because git patch-id is fooled by the
> "-- " trailer that git format-patch puts.
>
> This patch adds a bit of lookahead to git patch-id in order to detect
> the trailer and split the patch when it comes.  In addition, commit
> ids in the "From " lines are considered and printed in the output.

How well does this interact with mime encoded output?

I somehow have a feeling that this is solving a wrong problem.

Re: [PATCH] patch-id: Add support for mbox format

[Paolo Bonzini]

On 04/16/2010 08:30 PM, Junio C Hamano wrote:
>> >  I have an alias that takes two arguments and compares their patch IDs.
>> >  I would like to use to make sure I've tested exactly what I submit
>> >  (patch by patch), like
>> >
>> >      git patch-cmp origin/master.. file-being-sent
>> >
>> >  However, I cannot do that because git patch-id is fooled by the
>> >  "-- " trailer that git format-patch puts.
>> >
>> >  This patch adds a bit of lookahead to git patch-id in order to detect
>> >  the trailer and split the patch when it comes.  In addition, commit
>> >  ids in the "From " lines are considered and printed in the output.
>
> How well does this interact with mime encoded output?

It doesn't work.  I have a version of the patch that works with it now 
(by parsing hunk headers and looking for a "--" line outside the hunk, 
but I doubt that would satisfy your feeling.

> I somehow have a feeling that this is solving a wrong problem.

In what sense?

In the end the patch id is what matters for the usecase above.  I could 
obtain it in other ways for the "origin/master.." part, but the mbox 
file from "git format-patch --stdout" must be given unmodified to 
git-patch-cmp, otherwise it wouldn't be what I send on the next step.

Paolo

Re: [PATCH] patch-id: Add support for mbox format

[Junio C Hamano]

Paolo Bonzini <bonzini@gnu.org> writes:

> On 04/16/2010 08:30 PM, Junio C Hamano wrote:
>>> >  I have an alias that takes two arguments and compares their patch IDs.
>>> >  I would like to use to make sure I've tested exactly what I submit
>>> >  (patch by patch), like
>>> >
>>> >      git patch-cmp origin/master.. file-being-sent
>>> > ...
>>
>> I somehow have a feeling that this is solving a wrong problem.
>
> In what sense?

Why does file-being-sent have anything from origin/master to begin with?
Perhaps the --ignore-if-in-upstream mechanism needs to be updated so that
you won't have duplicates that patch-id could easily find in the first
place?

Re: [PATCH] patch-id: Add support for mbox format

[Paolo Bonzini]

On 04/16/2010 11:23 PM, Junio C Hamano wrote:
>>>>   I have an alias that takes two arguments and compares their patch IDs.
>>>>   I would like to use to make sure I've tested exactly what I submit
>>>>   (patch by patch), like
>>>>
>>>>       git patch-cmp origin/master.. file-being-sent
>>>> ...
>>>
>>> I somehow have a feeling that this is solving a wrong problem.
>>
>> In what sense?
>
> Why does file-being-sent have anything from origin/master to begin with?
> Perhaps the --ignore-if-in-upstream mechanism needs to be updated so that
> you won't have duplicates that patch-id could easily find in the first
> place?

I think we're speaking about different things, --ignore-if-in-upstream 
doesn't have anything to do with this.  The git patch-cmp alias is just 
checking that

   git show origin/master.. | git patch-id | tac | awk '{print $1}'

and

   git patch-id < file-being-sent | awk '{print $1}'

produce the exact same output.

I use this when I had to edit the file-being-sent, e.g. to add cover 
letters or an introduction to a patch series.  Since some time passes 
between format-patch and send-email, I want to test that the file I'm 
sending is exactly what I have in the repository, and that I'm not 
submitting the wrong series.

Alternatively, I could apply file-being-sent to a detached HEAD and 
compare the trees, like

   topic=`git rev-parse HEAD`
   git checkout origin/master
   git am file-being-sent || echo bad
   mine=HEAD
   for i in `git rev-list origin/master..$topic`; do
     git diff-tree $i $mine || echo bad
     mine=${mine}^
   done

However, comparing the patch-id is faster and perfect for this job. 
What were your intended uses for the git-patch-id utility?

Paolo

Re: [PATCH] patch-id: Add support for mbox format

[Junio C Hamano]

Paolo Bonzini <bonzini@gnu.org> writes:

> I use this when I had to edit the file-being-sent, e.g. to add cover
> letters or an introduction to a patch series.  Since some time passes
> between format-patch and send-email, I want to test that the file I'm
> sending is exactly what I have in the repository, and that I'm not
> submitting the wrong series.

Ok, that is what I missed when I mentioned ignore-if-in-upstream, iow, I
thought "patch-cmp" were merely to check for failed detection of
duplicates by format-patch.  If you are editing the mbox file and let the
patches sit there while origin/master may be progressing, then that is an
entirely different story.

[PATCH 1/2] patch-id: extract parsing one diff out of generate_id_list

[Paolo Bonzini]

This simplifies a bit the next patch, since it will have more than one
condition to exit the loop.

Signed-off-by: Paolo Bonzini <bonzini@gnu.org>
---
        On 04/17/2010 04:43 AM, Junio C Hamano wrote:
        > Paolo Bonzini<bonzini@gnu.org>  writes:
        > 
        >> I use this when I had to edit the file-being-sent, e.g. to add cover
        >> letters or an introduction to a patch series.  Since some time passes
        >> between format-patch and send-email, I want to test that the file I'm
        >> sending is exactly what I have in the repository, and that I'm not
        >> submitting the wrong series.
        > 
        > Ok, that is what I missed when I mentioned ignore-if-in-upstream, iow,
        > I thought "patch-cmp" were merely to check for failed detection of
        > duplicates by format-patch.  If you are editing the mbox file and let
        > the patches sit there while origin/master may be progressing, then
        > that is an entirely different story.

        Yes, that is exactly the point.  I sometimes have to edit the mbox file
        so I cannot use "git send-email origin/master..", and I want a double
        check that I'm not doing something stupid.

 builtin/patch-id.c |   35 +++++++++++++++++++++--------------
 1 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index af0911e..78d24dc 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -28,16 +28,11 @@ static int remove_space(char *line)
 	return dst - line;
 }
 
-static void generate_id_list(void)
+int get_one_patchid (unsigned char *next_sha1, git_SHA_CTX *ctx)
 {
-	static unsigned char sha1[20];
 	static char line[1000];
-	git_SHA_CTX ctx;
 	int patchlen = 0;
-
-	git_SHA1_Init(&ctx);
 	while (fgets(line, sizeof(line), stdin) != NULL) {
-		unsigned char n[20];
 		char *p = line;
 		int len;
 
@@ -46,12 +41,8 @@ static void generate_id_list(void)
 		else if (!memcmp(line, "commit ", 7))
 			p += 7;
 
-		if (!get_sha1_hex(p, n)) {
-			flush_current_id(patchlen, sha1, &ctx);
-			hashcpy(sha1, n);
-			patchlen = 0;
-			continue;
-		}
+		if (!get_sha1_hex(p, next_sha1))
+			break;
 
 		/* Ignore commit comments */
 		if (!patchlen && memcmp(line, "diff ", 5))
@@ -68,9 +59,25 @@ static void generate_id_list(void)
 		/* Compute the sha without whitespace */
 		len = remove_space(line);
 		patchlen += len;
-		git_SHA1_Update(&ctx, line, len);
+		git_SHA1_Update(ctx, line, len);
+	}
+	return patchlen;
+}
+
+
+static void generate_id_list(void)
+{
+	unsigned char sha1[20], n[20];
+	git_SHA_CTX ctx;
+	int patchlen;
+
+	git_SHA1_Init(&ctx);
+	while (!feof (stdin)) {
+		memset (n, 0, 20);
+		patchlen = get_one_patchid (n, &ctx);
+		flush_current_id(patchlen, sha1, &ctx);
+		hashcpy (sha1, n);
 	}
-	flush_current_id(patchlen, sha1, &ctx);
 }
 
 static const char patch_id_usage[] = "git patch-id < patch";
-- 
1.6.6.1

[PATCH 2/2] patch-id: Add support for mbox format

[Paolo Bonzini]

I have an alias that takes two arguments and compares their patch IDs.
I would like to use to make sure I've tested exactly what I submit
(patch by patch), like

   git patch-cmp origin/master.. file-being-sent

However, I cannot do that because git patch-id is fooled by the "-- "
trailer that git format-patch puts, or likely by the MIME boundary.

This patch adds hunk parsing logic to git patch-id in order to detect an
out of place "-" line and split the patch when it comes.  In addition,
commit ids in the "From " lines are considered and printed in the output.

Signed-off-by: Paolo Bonzini <bonzini@gnu.org>
---
 builtin/patch-id.c  |   70 +++++++++++++++++++++++++++++++++++++++++++++-----
 t/t4204-patch-id.sh |   28 ++++++++++++++++++++
 2 files changed, 91 insertions(+), 7 deletions(-)

diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 78d24dc..c3ecf8f 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -28,10 +28,40 @@ static int remove_space(char *line)
 	return dst - line;
 }
 
+static int scan_hunk_header(const char *p, int *p_before, int *p_after)
+{
+	static const char digits[] = "0123456789";
+	const char *q, *r;
+	int n;
+
+	q = p + 4;
+	n = strspn(q, digits);
+	if (q[n] == ',') {
+		q += n + 1;
+		n = strspn(q, digits);
+	}
+	if (n == 0 || q[n] != ' ' || q[n+1] != '+')
+		return 0;
+
+	r = q + n + 2;
+	n = strspn(r, digits);
+	if (r[n] == ',') {
+		r += n + 1;
+		n = strspn(r, digits);
+	}
+	if (n == 0)
+		return 0;
+
+	*p_before = atoi(q);
+	*p_after = atoi(r);
+	return 1;
+}
+
 int get_one_patchid (unsigned char *next_sha1, git_SHA_CTX *ctx)
 {
 	static char line[1000];
 	int patchlen = 0;
+	int before = -1, after = -1;
 	while (fgets(line, sizeof(line), stdin) != NULL) {
 		char *p = line;
 		int len;
@@ -40,6 +70,8 @@ int get_one_patchid (unsigned char *next_sha1, git_SHA_CTX *ctx)
 			p += 10;
 		else if (!memcmp(line, "commit ", 7))
 			p += 7;
+		else if (!memcmp(line, "From ", 5))
+			p += 5;
 
 		if (!get_sha1_hex(p, next_sha1))
 			break;
@@ -48,13 +80,37 @@ int get_one_patchid (unsigned char *next_sha1, git_SHA_CTX *ctx)
 		if (!patchlen && memcmp(line, "diff ", 5))
 			continue;
 
-		/* Ignore git-diff index header */
-		if (!memcmp(line, "index ", 6))
-			continue;
-
-		/* Ignore line numbers when computing the SHA1 of the patch */
-		if (!memcmp(line, "@@ -", 4))
-			continue;
+		/* Parsing diff header?  */
+		if (before == -1) {
+			if (!memcmp(line, "index ", 6))
+				continue;
+			else if (!memcmp(line, "--- ", 4))
+				before = after = 1;
+			else if (!isalpha(line[0]))
+				break;
+		}
+
+		/* Looking for a valid hunk header?  */
+		if (before == 0 && after == 0) {
+			if (!memcmp (line, "@@ -", 4)) {
+				/* Parse next hunk, but ignore line numbers.  */
+				scan_hunk_header (line, &before, &after);
+				continue;
+			}
+
+			/* Split at the end of the patch.  */
+			if (memcmp (line, "diff ", 5))
+				break;
+
+			/* Else we're parsing another header.  */
+			before = after = -1;
+		}
+
+		/* If we get here, we're inside a hunk.  */
+		if (line[0] == '-' || line[0] == ' ')
+			before--;
+		if (line[0] == '+' || line[0] == ' ')
+			after--;
 
 		/* Compute the sha without whitespace */
 		len = remove_space(line);
diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh
index 04f7bae..68e2652 100755
--- a/t/t4204-patch-id.sh
+++ b/t/t4204-patch-id.sh
@@ -18,6 +18,11 @@ test_expect_success 'patch-id output is well-formed' '
 	grep "^[a-f0-9]\{40\} $(git rev-parse HEAD)$" output
 '
 
+calc_patch_id () {
+	git patch-id |
+		sed "s# .*##" > patch-id_"$1"
+}
+
 get_patch_id () {
 	git log -p -1 "$1" | git patch-id |
 		sed "s# .*##" > patch-id_"$1"
@@ -35,4 +40,27 @@ test_expect_success 'patch-id detects inequality' '
 	! test_cmp patch-id_master patch-id_notsame
 '
 
+test_expect_success 'patch-id supports git-format-patch output' '
+	get_patch_id master &&
+	git checkout same &&
+	git format-patch -1 --stdout | calc_patch_id same &&
+	test_cmp patch-id_master patch-id_same &&
+	set `git format-patch -1 --stdout | git patch-id` &&
+	test "$2" = `git rev-parse HEAD`
+'
+
+test_expect_success 'whitespace is irrelevant in footer' '
+	get_patch_id master &&
+	git checkout same &&
+	git format-patch -1 --stdout | sed "s/ \$//" | calc_patch_id same &&
+	test_cmp patch-id_master patch-id_same
+'
+
+test_expect_success 'patch-id supports git-format-patch MIME output' '
+	get_patch_id master &&
+	git checkout same &&
+	git format-patch -1 --attach --stdout | calc_patch_id same &&
+	test_cmp patch-id_master patch-id_same
+'
+
 test_done
-- 
1.6.6.1

Re: [PATCH 2/2] patch-id: Add support for mbox format

[Junio C Hamano]

Paolo Bonzini <bonzini@gnu.org> writes:

> This patch adds hunk parsing logic to git patch-id in order to detect an
> out of place "-" line and split the patch when it comes.  In addition,
> commit ids in the "From " lines are considered and printed in the output.

I don't know how this patch will affect the difficulty of enhancing it to
handle MIME attachments later, but let's say we won't worry about it for
now, because we do not even know if we want to do so at this moment.

I fixed up some style issues locally before applying, but please be
careful next time.

 - One SP after a keyword used in syntactic constructs (e.g. "if (cond)",
   not "while(cond)"); your patches were fine wrt this;

 - No SP after a function (e.g. "memcpy(args...)", not "memcpy (args...)").

Also please be careful when calling get_sha1_hex(); an unsuccessful call
to this function is allowed to clobber the buffer.  In addition to the
style fixes, I squashed something like this (not exactly this).

Thanks.

 builtin/patch-id.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 40af5c7..4785411 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -61,7 +61,8 @@ int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx)
 {
 	static char line[1000];
 	int patchlen = 0;
-	int before = -1, after = -1;
+	int before = -1, after = -1, found_next = 0;
+
 	while (fgets(line, sizeof(line), stdin) != NULL) {
 		char *p = line;
 		int len;
@@ -73,8 +74,10 @@ int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx)
 		else if (!memcmp(line, "From ", 5))
 			p += 5;
 
-		if (!get_sha1_hex(p, next_sha1))
+		if (!get_sha1_hex(p, next_sha1)) {
+			found_next = 1;
 			break;
+		}
 
 		/* Ignore commit comments */
 		if (!patchlen && memcmp(line, "diff ", 5))
@@ -117,6 +120,8 @@ int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx)
 		patchlen += len;
 		git_SHA1_Update(ctx, line, len);
 	}
+	if (!found_next)
+		hashclr(next_sha1);
 	return patchlen;
 }
 
@@ -128,8 +133,8 @@ static void generate_id_list(void)
 	int patchlen;
 
 	git_SHA1_Init(&ctx);
+	hashclr(sha1);
 	while (!feof(stdin)) {
-		memset(n, 0, 20);
 		patchlen = get_one_patchid(n, &ctx);
 		flush_current_id(patchlen, sha1, &ctx);
 		hashcpy(sha1, n);

Re: [PATCH 2/2] patch-id: Add support for mbox format

[Paolo Bonzini]

On 04/19/2010 11:48 PM, Junio C Hamano wrote:
> I don't know how this patch will affect the difficulty of enhancing it to
> handle MIME attachments later, but let's say we won't worry about it for
> now, because we do not even know if we want to do so at this moment.

This v2 also handles MIME, see the tests.

Thanks for fixing up the patch.

Paolo

Re: [PATCH 2/2] patch-id: Add support for mbox format

[Junio C Hamano]

Paolo Bonzini <bonzini@gnu.org> writes:

> This v2 also handles MIME, see the tests.

I was more worried about quoted-printable (or base64) when I said MIME ;-)