[PATCH v5 1/9] diff: add a config option to control orderfile

[PATCH v5 1/9] 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 v5 2/9] test: add test_write_lines helper

[Michael S. Tsirkin]

API and implementation as suggested by Junio.

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

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index aeae3ca..0e21275 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -712,6 +712,11 @@ test_ln_s_add () {
 	fi
 }
 
+# This function writes out its parameters, one per line
+test_write_lines () {
+	printf "%s\n" "$@";
+}
+
 perl () {
 	command "$PERL_PATH" "$@"
 }
-- 
MST

Re: [PATCH v5 2/9] test: add test_write_lines helper

[Jonathan Nieder]

Michael S. Tsirkin wrote:

> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -712,6 +712,11 @@ test_ln_s_add () {
>  	fi
>  }
>  
> +# This function writes out its parameters, one per line
> +test_write_lines () {
> +	printf "%s\n" "$@";
> +}
> +

Thanks for fixing this.

Nits:

 * no need for the trailing semicolon
 * it's probably worth documenting this in t/README as well so people
   writing new test scripts know what it's about.

Thanks,
Jonathan

Re: [PATCH v5 2/9] test: add test_write_lines helper

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Michael S. Tsirkin wrote:
>
>> --- a/t/test-lib-functions.sh
>> +++ b/t/test-lib-functions.sh
>> @@ -712,6 +712,11 @@ test_ln_s_add () {
>>  	fi
>>  }
>>  
>> +# This function writes out its parameters, one per line
>> +test_write_lines () {
>> +	printf "%s\n" "$@";
>> +}
>> +
>
> Thanks for fixing this.
>
> Nits:
>
>  * no need for the trailing semicolon

Good eyes.  I was wondering if I wrote that (which I found very
unlikely).

>  * it's probably worth documenting this in t/README as well so people
>    writing new test scripts know what it's about.

Sounds like a good idea.

Thanks.

[PATCH v5 3/9] 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..0404f50
--- /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 v5 3/9] tests: new test for orderfile options

[Jonathan Nieder]

Michael S. Tsirkin wrote:

>  t/t4056-diff-order.sh | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
>  create mode 100755 t/t4056-diff-order.sh

I thought this file already existed since v1.9-rc0~8^2~3 (t4056: add
new tests for "git diff -O", 2013-12-18).

What commit is your series based against?

Puzzled,
Jonathan

Re: [PATCH v5 3/9] 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..0404f50

Huh?  What codebase is this based on?

I think we had t4056 since b5277730 (t4056: add new tests for "git
diff -O", 2013-12-18).

Puzzled...

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

Re: [PATCH v5 3/9] tests: new test for orderfile options

[Michael S. Tsirkin]

On Thu, Apr 24, 2014 at 11:45:35AM -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..0404f50
> 
> Huh?  What codebase is this based on?
> 
> I think we had t4056 since b5277730 (t4056: add new tests for "git
> diff -O", 2013-12-18).
> 
> Puzzled...


Yes I didn't rebase in a while: 7794a680e63a2a11b73cb1194653662f2769a792
was the tip.
I'll rebase, sorry.

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

[PATCH v5 4/9] 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 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 | 89 ++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 73 insertions(+), 16 deletions(-)

diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 3cfe02d..037cf2f 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,63 @@ 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;
+	}
+
+	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 v5 4/9] patch-id: make it stable against hunk reordering

[Jonathan Nieder]

Hi,

Michael S. Tsirkin wrote:

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

Hm.

If the goal is that functionally equivalent patches are guaranteed to
produce the same patch-id, I wonder if we should be doing something
like the following:

 1. apply the patch in memory
 2. generate a new diff
 3. use that new diff to produce a patch-id

Otherwise issues like --diff-algorithm=patience versus =myers will
create trouble too.  I don't think that avoiding false negatives for
patch comparison without doing something like that is really possible.

On the other hand if someone reorders file diffs within a patch, that
is a potentially very common thing to do and something worth fixing.
In other words, while your (1) makes perfect sense to me, case (2)
seems less convincing.

The downside of allowing reordering hunks is that it can potentially
make different patches to be treated the same (for example if they
were making similar changes to different functions) when the ordering
previously caused them to be distinguished.  But that wasn't something
people could count on anyway, so I don't mind.

Should the internal patch-id computation used by commands like 'git
cherry' (see diff.c::diff_get_patch_id) get the same change?  (Not a
rhetorical question --- I don't know what the right choice would be
there.)

[...]
> The new behaviour is enabled
> - 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.

Which is the default?

[...]
>  builtin/patch-id.c | 89 ++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 73 insertions(+), 16 deletions(-)

Documentation?  Tests?

Thanks,
Jonathan

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Should the internal patch-id computation used by commands like 'git
> cherry' (see diff.c::diff_get_patch_id) get the same change?  (Not a
> rhetorical question --- I don't know what the right choice would be
> there.)

I thought about it but I did not think of a reason why.  If we do
not store the patch-id (it would be a misnomer especially after this
series, it is mor like patch signature), and we generate the patch
to be hashed internally without getting affected by any user input
given per-invocation, then nothing is externally observable even if
we used two completely different definition of patch id computation,
and I think these preconditions do hold.

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

[Michael S. Tsirkin]

On Thu, Apr 24, 2014 at 10:30:44AM -0700, Jonathan Nieder wrote:
> Hi,
> 
> Michael S. Tsirkin wrote:
> 
> > 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.
> 
> Hm.
> 
> If the goal is that functionally equivalent patches are guaranteed to
> produce the same patch-id, I wonder if we should be doing something
> like the following:
> 
>  1. apply the patch in memory
>  2. generate a new diff
>  3. use that new diff to produce a patch-id
> 
> Otherwise issues like --diff-algorithm=patience versus =myers will
> create trouble too.  I don't think that avoiding false negatives for
> patch comparison without doing something like that is really possible.
> 
> On the other hand if someone reorders file diffs within a patch, that
> is a potentially very common thing to do and something worth fixing.
> In other words, while your (1) makes perfect sense to me, case (2)
> seems less convincing.

I agree it's less convincing: one would have to edit patch
by hand (which I used to sometimes do to make important parts more prominent,
but stopped doing in favor of splitting a patch).
I'm not 100% sure whether it's worth supporting or not.


> The downside of allowing reordering hunks is that it can potentially
> make different patches to be treated the same (for example if they
> were making similar changes to different functions) when the ordering
> previously caused them to be distinguished.  But that wasn't something
> people could count on anyway, so I don't mind.

I think this example convinces me. I'll drop this support in the next version.

> Should the internal patch-id computation used by commands like 'git
> cherry' (see diff.c::diff_get_patch_id) get the same change?  (Not a
> rhetorical question --- I don't know what the right choice would be
> there.)
> 
> [...]
> > The new behaviour is enabled
> > - 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.
> 
> Which is the default?
> 
> [...]
> >  builtin/patch-id.c | 89 ++++++++++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 73 insertions(+), 16 deletions(-)
> 
> Documentation?  Tests?
> 
> Thanks,
> Jonathan

[PATCH v5 5/9] 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 | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-patch-id.txt b/Documentation/git-patch-id.txt
index 312c3b1..e21b79b 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,19 @@ 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.
+
+--unstable::
+	Use a non-symmetrical sum of hashes, such that reordering
+	or splitting the patch does affect the ID.
+	This is the default.
+
 <patch>::
 	The diff to create the ID of.
 
-- 
MST

Re: [PATCH v5 5/9] patch-id: document new behaviour

[Jonathan Nieder]

Michael S. Tsirkin wrote:

>  Documentation/git-patch-id.txt | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)

Ah, there's the documentation.  Please squash this with the patch that
introduces the new behavior so they can be reviewed together more
easily (both now and later when people do archeology).

[...]
> +--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.

This doesn't explain to me why I would want to use --stable versus
--unstable.  Maybe an EXAMPLES section would help?

The only reason I can think of to use --unstable is for compatibility
with historical patch-ids.  Is there any other reason?

At this point in the series there is no patchid.stable configuration.

> +--unstable::
> +	Use a non-symmetrical sum of hashes, such that reordering

What is a non-symmetrical sum?

Thanks,
Jonathan

Re: [PATCH v5 5/9] patch-id: document new behaviour

[Michael S. Tsirkin]

On Thu, Apr 24, 2014 at 10:33:25AM -0700, Jonathan Nieder wrote:
> Michael S. Tsirkin wrote:
> 
> >  Documentation/git-patch-id.txt | 23 ++++++++++++++++++-----
> >  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> Ah, there's the documentation.  Please squash this with the patch that
> introduces the new behavior so they can be reviewed together more
> easily (both now and later when people do archeology).
> 
> [...]
> > +--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.
> 
> This doesn't explain to me why I would want to use --stable versus
> --unstable.  Maybe an EXAMPLES section would help?
> 
> The only reason I can think of to use --unstable is for compatibility
> with historical patch-ids.  Is there any other reason?
> 
> At this point in the series there is no patchid.stable configuration.
> 
> > +--unstable::
> > +	Use a non-symmetrical sum of hashes, such that reordering
> 
> What is a non-symmetrical sum?

Non-symmetrical combination function is better?

> Thanks,
> Jonathan

Re: [PATCH v5 5/9] patch-id: document new behaviour

[Junio C Hamano]

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

>> > +--unstable::
>> > +	Use a non-symmetrical sum of hashes, such that reordering
>> 
>> What is a non-symmetrical sum?
>
> Non-symmetrical combination function is better?

I do not think either is very good X-<.

The primary points to convey for "--stable" are:

 - Two patches produced by comparing the same two trees with two
   different settings for "-O<orderfile>" will result in the same
   patchc signature, thereby allowing the computed result to be used
   as a key to index some metainformation about the change between
   the two trees;

 - It will produce a result different from the plain vanilla
   patch-id has always produced even when used on a diff output
   taken without any use of "-O<orderfile>", thereby making existing
   databases keyed by patch-ids unusable.

The fact that we happened to use a patch-id that catches that
somebody reordered the same patch into different file order and
declares that they are two different changes is a more historical
accident than a designed goal.

I would even say that we would have used the "stable" version from
the beginning if we thought that "-O<orderfile>" would be widely
used when these two features both appeared.  Even though I was the
guilty one who introduced it, I'd admit that "-O<orderfile>" has
merely been a curiosity from its inception and has been a failed
experiment, not in the sense that the feature does not work as
adverertised (it does), but in the sense that it is not widely used
(evidenced by the lack of complaints on missing diff.orderfile for a
long time) at all.  With "-O<orderfile>" being a failed experiment,
the "unstability" did not matter, so it has stuck.

The only two things worth mentioning about "--unstable", if our
future direction is to see diff.orderfile and "--stable" a lot more
widely used, are:

 (1) it keeps producing the same patch-id as existing versions of
     Git, so users with existing databases (who do not deal with
     reordered patches) may want to use it; and perhaps

 (2) it will not consider a patch taken with "-O<orderfile>" and
     another without it from the same source the same patches.

Mathmatically speaking, mentioning "non-symmetrial" might be one way
of expressing the latter point (2), but stressing on that alone
without mentioning (1) misses the point.  (2) is _not_ a designed
feature, so it is not very interesting.  Unless you have an existing
database, there is no reason to use "--unstable".

On the other hand (1) is a very relevant thing to mention, as we are
talking about a feature that, if unused, may break existing users'
data.

Re: [PATCH v5 5/9] patch-id: document new behaviour

[Michael S. Tsirkin]

On Thu, Apr 24, 2014 at 03:12:14PM -0700, Junio C Hamano wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> >> > +--unstable::
> >> > +	Use a non-symmetrical sum of hashes, such that reordering
> >> 
> >> What is a non-symmetrical sum?
> >
> > Non-symmetrical combination function is better?
> 
> I do not think either is very good X-<.
> 
> The primary points to convey for "--stable" are:
> 
>  - Two patches produced by comparing the same two trees with two
>    different settings for "-O<orderfile>" will result in the same
>    patchc signature, thereby allowing the computed result to be used
>    as a key to index some metainformation about the change between
>    the two trees;
> 
>  - It will produce a result different from the plain vanilla
>    patch-id has always produced even when used on a diff output
>    taken without any use of "-O<orderfile>", thereby making existing
>    databases keyed by patch-ids unusable.
> 
> The fact that we happened to use a patch-id that catches that
> somebody reordered the same patch into different file order and
> declares that they are two different changes is a more historical
> accident than a designed goal.
> 
> I would even say that we would have used the "stable" version from
> the beginning if we thought that "-O<orderfile>" would be widely
> used when these two features both appeared.  Even though I was the
> guilty one who introduced it, I'd admit that "-O<orderfile>" has
> merely been a curiosity from its inception and has been a failed
> experiment, not in the sense that the feature does not work as
> adverertised (it does), but in the sense that it is not widely used
> (evidenced by the lack of complaints on missing diff.orderfile for a
> long time) at all.  With "-O<orderfile>" being a failed experiment,
> the "unstability" did not matter, so it has stuck.
> 
> The only two things worth mentioning about "--unstable", if our
> future direction is to see diff.orderfile and "--stable" a lot more
> widely used, are:
> 
>  (1) it keeps producing the same patch-id as existing versions of
>      Git, so users with existing databases (who do not deal with
>      reordered patches) may want to use it; and perhaps
> 
>  (2) it will not consider a patch taken with "-O<orderfile>" and
>      another without it from the same source the same patches.
> 
> Mathmatically speaking, mentioning "non-symmetrial" might be one way
> of expressing the latter point (2), but stressing on that alone
> without mentioning (1) misses the point.  (2) is _not_ a designed
> feature, so it is not very interesting.  Unless you have an existing
> database, there is no reason to use "--unstable".
> 
> On the other hand (1) is a very relevant thing to mention, as we are
> talking about a feature that, if unused, may break existing users'
> data.

OK I did just that, pls take a look.

[PATCH v5 6/9] 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 | 128 +++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 117 insertions(+), 11 deletions(-)

diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh
index d2c930d..cd13e8e 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,88 @@ 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 '--unstable overrides patchid.stable = true' '
+	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

[PATCH v5 7/9] patch-id: change default to stable

[Michael S. Tsirkin]

--stable has been the default in 'next' for a few weeks with no ill
effects.
Change the default to that so that users don't have to remember to
enable it.

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

diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 037cf2f..0b267af 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -198,9 +198,9 @@ int cmd_patch_id(int argc, const char **argv, const char *prefix)
 
 	git_config(git_patch_id_config, &stable);
 
-	/* If nothing is set, default to unstable. */
+	/* If nothing is set, default to stable. */
 	if (stable < 0)
-		stable = 0;
+		stable = 1;
 
 	if (argc == 2 && !strcmp(argv[1], "--stable"))
 		stable = 1;
-- 
MST

[PATCH v5 8/9] t4204-patch-id.sh: default is now stable

[Michael S. Tsirkin]

update test to match behaviour change

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

diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh
index cd13e8e..03f91ce 100755
--- a/t/t4204-patch-id.sh
+++ b/t/t4204-patch-id.sh
@@ -138,8 +138,8 @@ test_expect_success 'splitting patch affects id with --unstable' '
 '
 
 #Now test various option combinations.
-test_expect_success 'default is unstable' '
-	test_patch_id relevant default
+test_expect_success 'default is stable' '
+	test_patch_id irrelevant default
 '
 
 test_expect_success 'patchid.stable = true is stable' '
-- 
MST

[PATCH v5 9/9] Documentation/git-patch-id.txt: default is stable

[Michael S. Tsirkin]

Update documentation to match behaviour change.

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

diff --git a/Documentation/git-patch-id.txt b/Documentation/git-patch-id.txt
index e21b79b..9299b90 100644
--- a/Documentation/git-patch-id.txt
+++ b/Documentation/git-patch-id.txt
@@ -33,12 +33,13 @@ OPTIONS
 	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.
+	This is the default.
 
 --unstable::
 	Use a non-symmetrical sum of hashes, such that reordering
 	or splitting the patch does affect the ID.
-	This is the default.
+	This is the default if patchid.stable is set to false.
+	This was the default value for git 1.9 and older.
 
 <patch>::
 	The diff to create the ID of.
-- 
MST