[PATCH v4 1/6] diff: add a config option to control orderfile

[PATCH v4 1/6] diff: add a config option to control orderfile

[Michael S. Tsirkin]

I always want my diffs to show header files first,
then .c files, then the rest. Make it possible to
set orderfile though a config option to achieve this.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 diff.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/diff.c b/diff.c
index b79432b..6bcb26b 100644
--- a/diff.c
+++ b/diff.c
@@ -264,6 +264,9 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(var, "diff.orderfile"))
+		return git_config_string(&default_diff_options.orderfile, var, value);
+
 	if (starts_with(var, "submodule."))
 		return parse_submodule_config_option(var, value);
 
-- 
MST

[PATCH v4 2/6] test: add test_write_lines helper

[Michael S. Tsirkin]

As suggested by Junio.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 t/test-lib-functions.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index aeae3ca..2fa6453 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -712,6 +712,13 @@ test_ln_s_add () {
 	fi
 }
 
+# This function writes out its parameters, one per line
+test_write_lines () {
+	for line in "$@"; do
+		echo "$line"
+	done
+}
+
 perl () {
 	command "$PERL_PATH" "$@"
 }
-- 
MST

Re: [PATCH v4 2/6] test: add test_write_lines helper

[Junio C Hamano]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> As suggested by Junio.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---

Ehh, I would probably not suggest such an implementation though.

	test_write_lines () {
		printf "%s\n" "$@"
	}

might be, but not with "echo" and semicolon on the same line as
"for" ;-).

>  t/test-lib-functions.sh | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index aeae3ca..2fa6453 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -712,6 +712,13 @@ test_ln_s_add () {
>  	fi
>  }
>  
> +# This function writes out its parameters, one per line
> +test_write_lines () {
> +	for line in "$@"; do
> +		echo "$line"
> +	done
> +}
> +
>  perl () {
>  	command "$PERL_PATH" "$@"
>  }

Re: [PATCH v4 2/6] test: add test_write_lines helper

[Michael S. Tsirkin]

On Wed, Apr 23, 2014 at 10:34:30AM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > As suggested by Junio.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> 
> Ehh, I would probably not suggest such an implementation though.
> 
> 	test_write_lines () {
> 		printf "%s\n" "$@"
> 	}
> 
> might be, but not with "echo" and semicolon on the same line as
> "for" ;-).

Okay I didn't know printf reuses format in bash, cute trick.

Do you want to rewrite it yourself or want me to post a
new version?


> >  t/test-lib-functions.sh | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> > index aeae3ca..2fa6453 100644
> > --- a/t/test-lib-functions.sh
> > +++ b/t/test-lib-functions.sh
> > @@ -712,6 +712,13 @@ test_ln_s_add () {
> >  	fi
> >  }
> >  
> > +# This function writes out its parameters, one per line
> > +test_write_lines () {
> > +	for line in "$@"; do
> > +		echo "$line"
> > +	done
> > +}
> > +
> >  perl () {
> >  	command "$PERL_PATH" "$@"
> >  }

[PATCH v4 3/6] tests: new test for orderfile options

[Michael S. Tsirkin]

The test is very basic and can be extended.
Couldn't find a good existing place to put it,
so created a new file.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 t/t4056-diff-order.sh | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)
 create mode 100755 t/t4056-diff-order.sh

diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh
new file mode 100755
index 0000000..a247b7a
--- /dev/null
+++ b/t/t4056-diff-order.sh
@@ -0,0 +1,63 @@
+#!/bin/sh
+
+test_description='diff orderfile'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+        as="a a a a a a a a" && # eight a
+        test_write_lines $as >foo &&
+        test_write_lines $as >bar &&
+	git add foo bar &&
+	git commit -a -m initial &&
+        test_write_lines $as b >foo &&
+        test_write_lines $as b >bar &&
+	git commit -a -m first &&
+	test_write_lines bar foo >bar-then-foo &&
+	test_write_lines foo bar >foo-then-bar &&
+	git diff -Ofoo-then-bar HEAD~1..HEAD >diff-foo-then-bar &&
+	git diff -Obar-then-foo HEAD~1..HEAD >diff-bar-then-foo
+'
+
+test_diff_well_formed () {
+	grep ^+b "$1" >added
+	grep ^-b "$1" >removed
+	grep ^+++ "$1" >oldfiles
+	grep ^--- "$1" >newfiles
+	test_line_count = 2 added &&
+	test_line_count = 0 removed &&
+	test_line_count = 2 oldfiles &&
+	test_line_count = 2 newfiles
+}
+
+test_expect_success 'diff output with -O is well-formed' '
+	test_diff_well_formed diff-foo-then-bar &&
+	test_diff_well_formed diff-bar-then-foo
+'
+
+test_expect_success 'flag -O affects diff output' '
+	! test_cmp diff-foo-then-bar diff-bar-then-foo
+'
+
+test_expect_success 'orderfile is same as -O' '
+	test_config diff.orderfile foo-then-bar &&
+	git diff HEAD~1..HEAD >diff-foo-then-bar-config &&
+	test_config diff.orderfile bar-then-foo &&
+	git diff HEAD~1..HEAD >diff-bar-then-foo-config &&
+	test_cmp diff-foo-then-bar diff-foo-then-bar-config &&
+	test_cmp diff-bar-then-foo diff-bar-then-foo-config
+'
+
+test_expect_success '-O overrides orderfile' '
+	test_config diff.orderfile foo-then-bar &&
+	git diff -Obar-then-foo HEAD~1..HEAD >diff-bar-then-foo-flag &&
+	test_cmp diff-bar-then-foo diff-bar-then-foo-flag
+'
+
+test_expect_success '/dev/null is same as no orderfile' '
+	git diff -O/dev/null HEAD~1..HEAD>diff-null-orderfile &&
+	git diff HEAD~1..HEAD >diff-default &&
+	test_cmp diff-null-orderfile diff-default
+'
+
+test_done
-- 
MST

Re: [PATCH v4 3/6] tests: new test for orderfile options

[Junio C Hamano]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> The test is very basic and can be extended.
> Couldn't find a good existing place to put it,
> so created a new file.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  t/t4056-diff-order.sh | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
>  create mode 100755 t/t4056-diff-order.sh
>
> diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh
> new file mode 100755
> index 0000000..a247b7a
> --- /dev/null
> +++ b/t/t4056-diff-order.sh
> @@ -0,0 +1,63 @@
> +#!/bin/sh
> +
> +test_description='diff orderfile'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> +        as="a a a a a a a a" && # eight a
> +        test_write_lines $as >foo &&
> +        test_write_lines $as >bar &&
> +	git add foo bar &&
> +	git commit -a -m initial &&
> +        test_write_lines $as b >foo &&
> +        test_write_lines $as b >bar &&

These lines are curiously jaggy; are you indenting with spaces?
Please don't.

My running "git am -s3c" may auto-fix these indentation issues, so
please wait before seeing what may be pushed out on 'pu'.  It may
turn out that there is no other need for rerolling this patch in the
series.

Thanks.

> +	git commit -a -m first &&
> +	test_write_lines bar foo >bar-then-foo &&
> +	test_write_lines foo bar >foo-then-bar &&
> +	git diff -Ofoo-then-bar HEAD~1..HEAD >diff-foo-then-bar &&
> +	git diff -Obar-then-foo HEAD~1..HEAD >diff-bar-then-foo
> +'
> +
> +test_diff_well_formed () {
> +	grep ^+b "$1" >added
> +	grep ^-b "$1" >removed
> +	grep ^+++ "$1" >oldfiles
> +	grep ^--- "$1" >newfiles
> +	test_line_count = 2 added &&
> +	test_line_count = 0 removed &&
> +	test_line_count = 2 oldfiles &&
> +	test_line_count = 2 newfiles
> +}
> +
> +test_expect_success 'diff output with -O is well-formed' '
> +	test_diff_well_formed diff-foo-then-bar &&
> +	test_diff_well_formed diff-bar-then-foo
> +'
> +
> +test_expect_success 'flag -O affects diff output' '
> +	! test_cmp diff-foo-then-bar diff-bar-then-foo
> +'
> +
> +test_expect_success 'orderfile is same as -O' '
> +	test_config diff.orderfile foo-then-bar &&
> +	git diff HEAD~1..HEAD >diff-foo-then-bar-config &&
> +	test_config diff.orderfile bar-then-foo &&
> +	git diff HEAD~1..HEAD >diff-bar-then-foo-config &&
> +	test_cmp diff-foo-then-bar diff-foo-then-bar-config &&
> +	test_cmp diff-bar-then-foo diff-bar-then-foo-config
> +'
> +
> +test_expect_success '-O overrides orderfile' '
> +	test_config diff.orderfile foo-then-bar &&
> +	git diff -Obar-then-foo HEAD~1..HEAD >diff-bar-then-foo-flag &&
> +	test_cmp diff-bar-then-foo diff-bar-then-foo-flag
> +'
> +
> +test_expect_success '/dev/null is same as no orderfile' '
> +	git diff -O/dev/null HEAD~1..HEAD>diff-null-orderfile &&
> +	git diff HEAD~1..HEAD >diff-default &&
> +	test_cmp diff-null-orderfile diff-default
> +'
> +
> +test_done

Re: [PATCH v4 3/6] tests: new test for orderfile options

[Michael S. Tsirkin]

On Wed, Apr 23, 2014 at 10:38:07AM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > The test is very basic and can be extended.
> > Couldn't find a good existing place to put it,
> > so created a new file.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  t/t4056-diff-order.sh | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 63 insertions(+)
> >  create mode 100755 t/t4056-diff-order.sh
> >
> > diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh
> > new file mode 100755
> > index 0000000..a247b7a
> > --- /dev/null
> > +++ b/t/t4056-diff-order.sh
> > @@ -0,0 +1,63 @@
> > +#!/bin/sh
> > +
> > +test_description='diff orderfile'
> > +
> > +. ./test-lib.sh
> > +
> > +test_expect_success 'setup' '
> > +        as="a a a a a a a a" && # eight a
> > +        test_write_lines $as >foo &&
> > +        test_write_lines $as >bar &&
> > +	git add foo bar &&
> > +	git commit -a -m initial &&
> > +        test_write_lines $as b >foo &&
> > +        test_write_lines $as b >bar &&
> 
> These lines are curiously jaggy; are you indenting with spaces?
> Please don't.
> 
> My running "git am -s3c" may auto-fix these indentation issues, so
> please wait before seeing what may be pushed out on 'pu'.  It may
> turn out that there is no other need for rerolling this patch in the
> series.
> 
> Thanks.

Not normally - but this did creep in here, somehow :(

> > +	git commit -a -m first &&
> > +	test_write_lines bar foo >bar-then-foo &&
> > +	test_write_lines foo bar >foo-then-bar &&
> > +	git diff -Ofoo-then-bar HEAD~1..HEAD >diff-foo-then-bar &&
> > +	git diff -Obar-then-foo HEAD~1..HEAD >diff-bar-then-foo
> > +'
> > +
> > +test_diff_well_formed () {
> > +	grep ^+b "$1" >added
> > +	grep ^-b "$1" >removed
> > +	grep ^+++ "$1" >oldfiles
> > +	grep ^--- "$1" >newfiles
> > +	test_line_count = 2 added &&
> > +	test_line_count = 0 removed &&
> > +	test_line_count = 2 oldfiles &&
> > +	test_line_count = 2 newfiles
> > +}
> > +
> > +test_expect_success 'diff output with -O is well-formed' '
> > +	test_diff_well_formed diff-foo-then-bar &&
> > +	test_diff_well_formed diff-bar-then-foo
> > +'
> > +
> > +test_expect_success 'flag -O affects diff output' '
> > +	! test_cmp diff-foo-then-bar diff-bar-then-foo
> > +'
> > +
> > +test_expect_success 'orderfile is same as -O' '
> > +	test_config diff.orderfile foo-then-bar &&
> > +	git diff HEAD~1..HEAD >diff-foo-then-bar-config &&
> > +	test_config diff.orderfile bar-then-foo &&
> > +	git diff HEAD~1..HEAD >diff-bar-then-foo-config &&
> > +	test_cmp diff-foo-then-bar diff-foo-then-bar-config &&
> > +	test_cmp diff-bar-then-foo diff-bar-then-foo-config
> > +'
> > +
> > +test_expect_success '-O overrides orderfile' '
> > +	test_config diff.orderfile foo-then-bar &&
> > +	git diff -Obar-then-foo HEAD~1..HEAD >diff-bar-then-foo-flag &&
> > +	test_cmp diff-bar-then-foo diff-bar-then-foo-flag
> > +'
> > +
> > +test_expect_success '/dev/null is same as no orderfile' '
> > +	git diff -O/dev/null HEAD~1..HEAD>diff-null-orderfile &&
> > +	git diff HEAD~1..HEAD >diff-default &&
> > +	test_cmp diff-null-orderfile diff-default
> > +'
> > +
> > +test_done

[PATCH v4 4/6] patch-id: make it stable against hunk reordering

[Michael S. Tsirkin]

Patch id changes if users
1. reorder file diffs that make up a patch
or
2. split a patch up to multiple diffs that touch the same path
(keeping hunks within a single diff ordered to make patch valid).

As the result is functionally equivalent, a different patch id is
surprising to many users.
In particular, reordering files using diff -O is helpful to make patches
more readable (e.g. API header diff before implementation diff).

Add an option to change patch-id behaviour making it stable against
these two kinds of patch change:
1. calculate SHA1 hash for each hunk separately and sum all hashes
(using a symmetrical sum) to get patch id
2. hash the file-level headers together with each hunk (not just the
first hunk)

We use a 20byte sum and not xor - since xor would give 0 output
for patches that have two identical diffs, which isn't all that
unlikely (e.g. append the same line in two places).

The new behaviour is enabled
- when diff.orderfile config option is present
  (as that is likely to reorder patches)
- when patchid.stable is true
- when --stable flag is present

Using a new flag --unstable or setting patchid.stable to false force
the historical behaviour.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 builtin/patch-id.c | 94 ++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 78 insertions(+), 16 deletions(-)

diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 3cfe02d..e154405 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -1,17 +1,14 @@
 #include "builtin.h"
 
-static void flush_current_id(int patchlen, unsigned char *id, git_SHA_CTX *c)
+static void flush_current_id(int patchlen, unsigned char *id, unsigned char *result)
 {
-	unsigned char result[20];
 	char name[50];
 
 	if (!patchlen)
 		return;
 
-	git_SHA1_Final(result, c);
 	memcpy(name, sha1_to_hex(id), 41);
 	printf("%s %s\n", sha1_to_hex(result), name);
-	git_SHA1_Init(c);
 }
 
 static int remove_space(char *line)
@@ -56,10 +53,31 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)
 	return 1;
 }
 
-static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx, struct strbuf *line_buf)
+static void flush_one_hunk(unsigned char *result, git_SHA_CTX *ctx)
 {
-	int patchlen = 0, found_next = 0;
+	unsigned char hash[20];
+	unsigned short carry = 0;
+	int i;
+
+	git_SHA1_Final(hash, ctx);
+	git_SHA1_Init(ctx);
+	/* 20-byte sum, with carry */
+	for (i = 0; i < 20; ++i) {
+		carry += result[i] + hash[i];
+		result[i] = carry;
+		carry >>= 8;
+	}
+}
+
+static int get_one_patchid(unsigned char *next_sha1, unsigned char *result,
+			   struct strbuf *line_buf, int stable)
+{
+	int patchlen = 0, found_next = 0, hunks = 0;
 	int before = -1, after = -1;
+	git_SHA_CTX ctx, header_ctx;
+
+	git_SHA1_Init(&ctx);
+	hashclr(result);
 
 	while (strbuf_getwholeline(line_buf, stdin, '\n') != EOF) {
 		char *line = line_buf->buf;
@@ -98,7 +116,19 @@ static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx, struct st
 		if (before == 0 && after == 0) {
 			if (!memcmp(line, "@@ -", 4)) {
 				/* Parse next hunk, but ignore line numbers.  */
+				if (stable) {
+					/* Hash the file-level headers together with each hunk. */
+					if (hunks) {
+						flush_one_hunk(result, &ctx);
+						/* Prepend saved header ctx for next hunk.  */
+						memcpy(&ctx, &header_ctx, sizeof(ctx));
+					} else {
+						/* Save header ctx for next hunk.  */
+						memcpy(&header_ctx, &ctx, sizeof(ctx));
+					}
+				}
 				scan_hunk_header(line, &before, &after);
+				hunks++;
 				continue;
 			}
 
@@ -107,7 +137,10 @@ static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx, struct st
 				break;
 
 			/* Else we're parsing another header.  */
+			if (stable && hunks)
+				flush_one_hunk(result, &ctx);
 			before = after = -1;
+			hunks = 0;
 		}
 
 		/* If we get here, we're inside a hunk.  */
@@ -119,39 +152,68 @@ static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx, struct st
 		/* Compute the sha without whitespace */
 		len = remove_space(line);
 		patchlen += len;
-		git_SHA1_Update(ctx, line, len);
+		git_SHA1_Update(&ctx, line, len);
 	}
 
 	if (!found_next)
 		hashclr(next_sha1);
 
+	flush_one_hunk(result, &ctx);
+
 	return patchlen;
 }
 
-static void generate_id_list(void)
+static void generate_id_list(int stable)
 {
-	unsigned char sha1[20], n[20];
-	git_SHA_CTX ctx;
+	unsigned char sha1[20], n[20], result[20];
 	int patchlen;
 	struct strbuf line_buf = STRBUF_INIT;
 
-	git_SHA1_Init(&ctx);
 	hashclr(sha1);
 	while (!feof(stdin)) {
-		patchlen = get_one_patchid(n, &ctx, &line_buf);
-		flush_current_id(patchlen, sha1, &ctx);
+		patchlen = get_one_patchid(n, result, &line_buf, stable);
+		flush_current_id(patchlen, sha1, result);
 		hashcpy(sha1, n);
 	}
 	strbuf_release(&line_buf);
 }
 
-static const char patch_id_usage[] = "git patch-id < patch";
+static const char patch_id_usage[] = "git patch-id [--stable | --unstable] < patch";
+
+static int git_patch_id_config(const char *var, const char *value, void *cb)
+{
+	int *stable = cb;
+
+	if (!strcmp(var, "patchid.stable")) {
+		*stable = git_config_bool(var, value);
+		return 0;
+	}
+
+	if (!strcmp(var, "diff.orderfile") && *stable < 0) {
+		*stable = 1;
+		return 0;
+	}
+
+	return git_default_config(var, value, cb);
+}
 
 int cmd_patch_id(int argc, const char **argv, const char *prefix)
 {
-	if (argc != 1)
+	int stable = -1;
+
+	git_config(git_patch_id_config, &stable);
+
+	/* If nothing is set, default to unstable. */
+	if (stable < 0)
+		stable = 0;
+
+	if (argc == 2 && !strcmp(argv[1], "--stable"))
+		stable = 1;
+	else if (argc == 2 && !strcmp(argv[1], "--unstable"))
+		stable = 0;
+	else if (argc != 1)
 		usage(patch_id_usage);
 
-	generate_id_list();
+	generate_id_list(stable);
 	return 0;
 }
-- 
MST

Re: [PATCH v4 4/6] patch-id: make it stable against hunk reordering

[Junio C Hamano]

Are these three patches the same as what has been queued on
mt/patch-id-stable topic and cooking in 'next' for a few weeks?

Re: [PATCH v4 4/6] patch-id: make it stable against hunk reordering

[Michael S. Tsirkin]

On Wed, Apr 23, 2014 at 10:39:23AM -0700, Junio C Hamano wrote:
> Are these three patches the same as what has been queued on
> mt/patch-id-stable topic and cooking in 'next' for a few weeks?

Not exactly - at your request I implemented git config
options to control patch id behaviour.
Documentation and tests updated to match.

Re: [PATCH v4 4/6] patch-id: make it stable against hunk reordering

[Junio C Hamano]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Wed, Apr 23, 2014 at 10:39:23AM -0700, Junio C Hamano wrote:
>> Are these three patches the same as what has been queued on
>> mt/patch-id-stable topic and cooking in 'next' for a few weeks?
>
> Not exactly - at your request I implemented git config
> options to control patch id behaviour.
> Documentation and tests updated to match.

After comparing the patches 4-6 and the one that has been in 'next'
for a few weeks, I tried to like the new one, but I couldn't.

The new one, if I am reading the patch correctly, makes the stable
variant the default if

 - the configuration explicitly asks to use it; or

 - diff.orderfile, which is a new configuration that did not exist,
   is used.

At the first glance, the latter makes it look as if that will not
hurt any existing users/repositories, but the thing is, the producer
of the patches is different from the consumer of the patches.  There
needs to be a way for a consumer to say "I need stable variant" on
the patches when computing "patch-id" on a patch that arrived.  As
long as two different producers use two different orders, the
consumer of the patches from these two sources is forced to use the
stable variant if some sort of cache is kept keyed with the
patch-ids.

But "diff.orderfile" configuration is all and only about the
producer, and does not help the case at all.

Compared to it, the previous version forced people who do not have a
particular reason to choose between the variants to use the new
"stable" variant, which was a lot simpler solution.

The reason why I merged the previous one to 'next' was because I
wanted to see if the behaviour change is as grave as I thought, or I
was just being unnecessary cautious.  If there is no third-party
script that precomputes something and stores them under these hashes
that breaks with this change, I do not see any reason not to make
the new "stable" one the default.

I however suspect that the ideal "stable" implementation may be
different from what you implemented.  What if we compute a patch-id
on a reordered patch as if the files came in the "usual" order?
That would be another way to achieve the stable-ness for sane cases
(i.e. no funny "you could split one patch with two hunks into two
patches with one hunk each twice mentioning the same path" which is
totally an uninteresting use case---diff.orderfile would not even
produce such a patch) and a huge difference would be that it would
produce the same patch-id as existing versions of Git, if the patch
is not reordered.  Is that asking for a moon (I admit that I haven't
looked at the code involved too deeply)?

Re: [PATCH v4 4/6] patch-id: make it stable against hunk reordering

[Michael S. Tsirkin]

On Wed, Apr 23, 2014 at 03:05:42PM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Wed, Apr 23, 2014 at 10:39:23AM -0700, Junio C Hamano wrote:
> >> Are these three patches the same as what has been queued on
> >> mt/patch-id-stable topic and cooking in 'next' for a few weeks?
> >
> > Not exactly - at your request I implemented git config
> > options to control patch id behaviour.
> > Documentation and tests updated to match.
> 
> After comparing the patches 4-6 and the one that has been in 'next'
> for a few weeks, I tried to like the new one, but I couldn't.

I'm fine with the one in next too.
I was under the impression that you wanted me to change
the behaviour so I worked on this, but previous version was sufficient
for my purposes (which is really all about putting diff.orderfile
upstream).

> The new one, if I am reading the patch correctly, makes the stable
> variant the default if
> 
>  - the configuration explicitly asks to use it; or
> 
>  - diff.orderfile, which is a new configuration that did not exist,
>    is used.
> 
> At the first glance, the latter makes it look as if that will not
> hurt any existing users/repositories, but the thing is, the producer
> of the patches is different from the consumer of the patches.  There
> needs to be a way for a consumer to say "I need stable variant" on
> the patches when computing "patch-id" on a patch that arrived.  As
> long as two different producers use two different orders, the
> consumer of the patches from these two sources is forced to use the
> stable variant if some sort of cache is kept keyed with the
> patch-ids.
> 
> But "diff.orderfile" configuration is all and only about the
> producer, and does not help the case at all.

OK, we can just drop that, that's easy.

> Compared to it, the previous version forced people who do not have a
> particular reason to choose between the variants to use the new
> "stable" variant, which was a lot simpler solution.
> 
> The reason why I merged the previous one to 'next' was because I
> wanted to see if the behaviour change is as grave as I thought, or I
> was just being unnecessary cautious.  If there is no third-party
> script that precomputes something and stores them under these hashes
> that breaks with this change, I do not see any reason not to make
> the new "stable" one the default.

Ah okay, so we just wait a bit and see and if all is quiet the
existing patch will graduate to master with time?
Totally cool.
I thought you wanted me to add the config option, but if everyone
is happy as is, I don't need it personally at all.

> I however suspect that the ideal "stable" implementation may be
> different from what you implemented.  What if we compute a patch-id
> on a reordered patch as if the files came in the "usual" order?

ATM patch id does not have any concept of the usual order,
so that's one problem - how does one figure out what the order would be?
I have no idea - is this documented anywhere?
Also I'm guessing this would depend on the state of the git tree which
would be another behaviour change: previously patch-id worked
fine outside any git tree.

>
> That would be another way to achieve the stable-ness for sane cases
> (i.e. no funny "you could split one patch with two hunks into two
> patches with one hunk each twice mentioning the same path" which is
> totally an uninteresting use case---diff.orderfile would not even
> produce such a patch)

Yes I'm thinking we should drop this hunk in the patch:
let's support reordering files but not splitting them.
This makes the change even smaller, so I now think we should
just go for it.

> and a huge difference would be that it would
> produce the same patch-id as existing versions of Git, if the patch
> is not reordered.  Is that asking for a moon (I admit that I haven't
> looked at the code involved too deeply)?

Yes this would be a bunch of code to write - certainly much more complex than
the existing small patch which just tweaks the checksum slightly to make
it stable.

-- 
MST

Re: [PATCH v4 4/6] patch-id: make it stable against hunk reordering

[Junio C Hamano]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Wed, Apr 23, 2014 at 03:05:42PM -0700, Junio C Hamano wrote:
>
>> After comparing the patches 4-6 and the one that has been in 'next'
>> for a few weeks, I tried to like the new one, but I couldn't.
>
> I'm fine with the one in next too.
> I was under the impression that you wanted me to change
> the behaviour so I worked on this,...

What I wanted to see was to make sure this would not kick in unless
the user explicitly asks.  "patchid.stable" configuration variable
is very much welcome, and if it defaulted to false with or without
"diff.orderfile", that would have been very much welcome.  Then
nobody will be surprised.

>> But "diff.orderfile" configuration is all and only about the
>> producer, and does not help the case at all.
>
> OK, we can just drop that, that's easy.
>
>> Compared to it, the previous version forced people who do not have a
>> particular reason to choose between the variants to use the new
>> "stable" variant, which was a lot simpler solution.
>> 
>> The reason why I merged the previous one to 'next' was because I
>> wanted to see if the behaviour change is as grave as I thought, or I
>> was just being unnecessary cautious.  If there is no third-party
>> script that precomputes something and stores them under these hashes
>> that breaks with this change, I do not see any reason not to make
>> the new "stable" one the default.
>
> Ah okay, so we just wait a bit and see and if all is quiet the
> existing patch will graduate to master with time?
> Totally cool.
> I thought you wanted me to add the config option, but if everyone
> is happy as is, I don't need it personally at all.

... or if we see problems, then build a fix on top to introduce
"patchid.stable" that defaults to false and not linking the feature
with "diff.ordefile".

Adding a new feature turned-off by default is the safer thing to do.
When nothing changes, by defintion there won't be a new breakage.
That is the default stance this project has taken for a long time,
and what made us trusted by projects that manage their source files
using our product.

It however is to favour stability and "no surprise" over progress,
and it may not be an optimal thing to do if the new feature is
compellingly good.  In some cases like this one, we may need to
experiment to find out the need of the users, and introducing the
feature with a configuration that is explicitly opt-in is one way to
do so.  We may or may not see many people using that feature without
disrupting existing users that way.

Cooking in 'next' with the feature turned-on by default is another
way that is riskier, but in this particular case, the population
that is likely to be affected are power users, which would match the
audience of 'next'---those who still want stability but want to be
closer to the cutting edge.

[PATCH v4 5/6] patch-id: document new behaviour

[Michael S. Tsirkin]

Clarify that patch ID can now be a sum of hashes, not a hash.
Document how command line and config options affect the
behaviour.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 Documentation/git-patch-id.txt | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-patch-id.txt b/Documentation/git-patch-id.txt
index 312c3b1..61d4a67 100644
--- a/Documentation/git-patch-id.txt
+++ b/Documentation/git-patch-id.txt
@@ -8,14 +8,14 @@ git-patch-id - Compute unique ID for a patch
 SYNOPSIS
 --------
 [verse]
-'git patch-id' < <patch>
+'git patch-id' [--stable | --unstable] < <patch>
 
 DESCRIPTION
 -----------
-A "patch ID" is nothing but a SHA-1 of the diff associated with a patch, with
-whitespace and line numbers ignored.  As such, it's "reasonably stable", but at
-the same time also reasonably unique, i.e., two patches that have the same "patch
-ID" are almost guaranteed to be the same thing.
+A "patch ID" is nothing but a sum of SHA-1 of the diff hunks associated with a
+patch, with whitespace and line numbers ignored.  As such, it's "reasonably
+stable", but at the same time also reasonably unique, i.e., two patches that
+have the same "patch ID" are almost guaranteed to be the same thing.
 
 IOW, you can use this thing to look for likely duplicate commits.
 
@@ -27,6 +27,21 @@ This can be used to make a mapping from patch ID to commit ID.
 
 OPTIONS
 -------
+
+--stable::
+	Use a symmetrical sum of hashes as the patch ID.
+	With this option, reordering file diffs that make up a patch or
+	splitting a diff up to multiple diffs that touch the same path
+	does not affect the ID.
+	This is the default if patchid.stable is set to true, or if patchid.stable
+	is not set and diff.orderfile is set to some value.
+
+--unstable::
+	Use a non-symmetrical sum of hashes, such that reordering
+	or splitting the patch does affect the ID.
+	This is the default if patchid.stable is set to false, or if neither patchid.stable
+	nor diff.orderfile are set.
+
 <patch>::
 	The diff to create the ID of.
 
-- 
MST

[PATCH v4 6/6] patch-id-test: test stable and unstable behaviour

[Michael S. Tsirkin]

Verify that patch ID supports an algorithm
that is stable against diff split and reordering.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 t/t4204-patch-id.sh | 140 +++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 129 insertions(+), 11 deletions(-)

diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh
index d2c930d..54f2fb8 100755
--- a/t/t4204-patch-id.sh
+++ b/t/t4204-patch-id.sh
@@ -5,27 +5,51 @@ test_description='git patch-id'
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-	test_commit initial foo a &&
-	test_commit first foo b &&
-	git checkout -b same HEAD^ &&
-	test_commit same-msg foo b &&
-	git checkout -b notsame HEAD^ &&
-	test_commit notsame-msg foo c
+        as="a a a a a a a a" && # eight a
+	test_write_lines $as >foo &&
+	test_write_lines $as >bar &&
+	git add foo bar &&
+	git commit -a -m initial &&
+	test_write_lines $as b >foo &&
+	test_write_lines $as b >bar &&
+	git commit -a -m first &&
+	git checkout -b same master &&
+	git commit --amend -m same-msg &&
+	git checkout -b notsame master &&
+	echo c >foo &&
+	echo c >bar &&
+	git commit --amend -a -m notsame-msg &&
+	git checkout -b split master &&
+	test_write_lines d $as b >foo &&
+	test_write_lines d $as b >bar &&
+	git commit -a -m split &&
+	git checkout -b merged master &&
+	git checkout split -- foo bar &&
+	git commit --amend -a -m merged &&
+	test_write_lines bar foo >bar-then-foo &&
+	test_write_lines foo bar >foo-then-bar
 '
 
 test_expect_success 'patch-id output is well-formed' '
-	git log -p -1 | git patch-id > output &&
+	git log -p -1 | git patch-id >output &&
 	grep "^[a-f0-9]\{40\} $(git rev-parse HEAD)$" output
 '
 
+#calculate patch id. Make sure output is not empty.
 calc_patch_id () {
-	git patch-id |
-		sed "s# .*##" > patch-id_"$1"
+	name="$1"
+	shift
+	git patch-id "$@" |
+	sed "s/ .*//" >patch-id_"$name" &&
+	test_line_count -gt 0 patch-id_"$name"
+}
+
+get_top_diff () {
+	git log -p -1 "$@" -O bar-then-foo --
 }
 
 get_patch_id () {
-	git log -p -1 "$1" | git patch-id |
-		sed "s# .*##" > patch-id_"$1"
+	get_top_diff "$1" | calc_patch_id "$@"
 }
 
 test_expect_success 'patch-id detects equality' '
@@ -56,6 +80,100 @@ test_expect_success 'whitespace is irrelevant in footer' '
 	test_cmp patch-id_master patch-id_same
 '
 
+cmp_patch_id () {
+	if
+		test "$1" = "relevant"
+	then
+		! test_cmp patch-id_"$2" patch-id_"$3"
+	else
+		test_cmp patch-id_"$2" patch-id_"$3"
+	fi
+}
+
+test_patch_id_file_order () {
+	relevant="$1"
+	shift
+	name="order-${1}-$relevant"
+	shift
+	get_top_diff "master" | calc_patch_id "$name" "$@" &&
+	git checkout same &&
+	git format-patch -1 --stdout -O foo-then-bar |
+		calc_patch_id "ordered-$name" "$@" &&
+	cmp_patch_id $relevant "$name" "ordered-$name"
+		
+}
+
+test_patch_id_split () {
+	relevant="$1"
+	shift
+	name="split-${1}-$relevant"
+	shift
+	get_top_diff merged | calc_patch_id "$name" "$@" &&
+	(git log -p -1 -O foo-then-bar split~1; git diff split~1..split) |
+		calc_patch_id "split-$name" "$@" &&
+	cmp_patch_id "$relevant" "$name" "split-$name"
+}
+
+# combined test for options
+test_patch_id () {
+	test_patch_id_file_order "$@" &&
+	test_patch_id_split "$@"
+}
+
+# small tests with detailed diagnostic for basic options.
+test_expect_success 'file order is irrelevant with --stable' '
+	test_patch_id_file_order irrelevant --stable --stable
+'
+
+test_expect_success 'file order is relevant with --unstable' '
+	test_patch_id_file_order relevant --unstable --unstable
+'
+
+test_expect_success 'splitting patch is irrelevant with --stable' '
+	test_patch_id_split irrelevant --stable --stable
+'
+
+test_expect_success 'splitting patch affects id with --unstable' '
+	test_patch_id_split relevant --unstable --unstable
+'
+
+#Now test various option combinations.
+test_expect_success 'default is unstable' '
+	test_patch_id relevant default
+'
+
+test_expect_success 'patchid.stable = true is stable' '
+	test_config patchid.stable true &&
+	test_patch_id irrelevant patchid.stable=true
+'
+
+test_expect_success 'patchid.stable = false is unstable' '
+	test_config patchid.stable false &&
+	test_patch_id relevant patchid.stable=false
+'
+
+test_expect_success 'diff.orderfile implies stable' '
+	test_config diff.orderfile /dev/null &&
+	test_patch_id irrelevant diff.orderfile
+'
+
+test_expect_success 'patchid.stable = false overrides diff.orderfile' '
+	test_config patchid.stable false &&
+	test_config diff.orderfile /dev/null &&
+	test_patch_id relevant patchid.stable=false-diff.orderfile
+'
+
+test_expect_success '--unstable overrides both orderfile and patchid.stable = true' '
+	test_config diff.orderfile /dev/null &&
+	test_config patchid.stable true &&
+	test_patch_id relevant patchid.stable=true--unstable --unstable
+'
+
+test_expect_success '--stable overrides patchid.stable = false' '
+	test_config patchid.stable false &&
+	test_patch_id irrelevant patchid.stable=false--stable --stable
+'
+
 test_expect_success 'patch-id supports git-format-patch MIME output' '
 	get_patch_id master &&
 	git checkout same &&
-- 
MST