{"thread":{"id":"36490","subject":"[PATCH v5 8/9] t4204-patch-id.sh: default is now stable","startedAt":"2014-04-24T09:30:51Z","lastAt":"2014-04-27T18:26:23Z","messageCount":21,"participants":["Michael S. Tsirkin","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":5,"patchTotal":9},"messages":[{"id":"239557","messageId":"1398331809-11309-1-git-send-email-mst@redhat.com","threadId":"36490","inReplyTo":null,"subject":"[PATCH v5 1/9] diff: add a config option to control orderfile","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2014-04-24T09:30:51Z","receivedAt":"2014-04-24T09:30:51Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"I always want my diffs to show header files first,\nthen .c files, then the rest. Make it possible to\nset orderfile though a config option to achieve this.\n\nSigned-off-by: Michael S. Tsirkin \n---\n diff.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex b79432b..6bcb26b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -264,6 +264,9 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"diff.orderfile\"))\n+\t\treturn git_config_string(&default_diff_options.orderfile, var, value);\n+\n \tif (starts_with(var, \"submodule.\"))\n \t\treturn parse_submodule_config_option(var, value);\n \n-- \nMST\n"},{"id":"239555","messageId":"1398331809-11309-2-git-send-email-mst@redhat.com","threadId":"36490","inReplyTo":"1398331809-11309-1-git-send-email-mst@redhat.com","subject":"[PATCH v5 2/9] test: add test_write_lines helper","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2014-04-24T09:30:54Z","receivedAt":"2014-04-24T09:30:54Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"API and implementation as suggested by Junio.\n\nSigned-off-by: Michael S. Tsirkin \n---\n t/test-lib-functions.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex aeae3ca..0e21275 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -712,6 +712,11 @@ test_ln_s_add () {\n \tfi\n }\n \n+# This function writes out its parameters, one per line\n+test_write_lines () {\n+\tprintf \"%s\\n\" \"$@\";\n+}\n+\n perl () {\n \tcommand \"$PERL_PATH\" \"$@\"\n }\n-- \nMST\n"},{"id":"239558","messageId":"1398331809-11309-3-git-send-email-mst@redhat.com","threadId":"36490","inReplyTo":"1398331809-11309-1-git-send-email-mst@redhat.com","subject":"[PATCH v5 3/9] tests: new test for orderfile options","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2014-04-24T09:30:57Z","receivedAt":"2014-04-24T09:30:57Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"The test is very basic and can be extended.\nCouldn't find a good existing place to put it,\nso created a new file.\n\nSigned-off-by: Michael S. Tsirkin \n---\n t/t4056-diff-order.sh | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 63 insertions(+)\n create mode 100755 t/t4056-diff-order.sh\n\ndiff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh\nnew file mode 100755\nindex 0000000..0404f50\n--- /dev/null\n+++ b/t/t4056-diff-order.sh\n@@ -0,0 +1,63 @@\n+#!/bin/sh\n+\n+test_description='diff orderfile'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tas=\"a a a a a a a a\" && # eight a\n+\ttest_write_lines $as >foo &&\n+\ttest_write_lines $as >bar &&\n+\tgit add foo bar &&\n+\tgit commit -a -m initial &&\n+\ttest_write_lines $as b >foo &&\n+\ttest_write_lines $as b >bar &&\n+\tgit commit -a -m first &&\n+\ttest_write_lines bar foo >bar-then-foo &&\n+\ttest_write_lines foo bar >foo-then-bar &&\n+\tgit diff -Ofoo-then-bar HEAD~1..HEAD >diff-foo-then-bar &&\n+\tgit diff -Obar-then-foo HEAD~1..HEAD >diff-bar-then-foo\n+'\n+\n+test_diff_well_formed () {\n+\tgrep ^+b \"$1\" >added\n+\tgrep ^-b \"$1\" >removed\n+\tgrep ^+++ \"$1\" >oldfiles\n+\tgrep ^--- \"$1\" >newfiles\n+\ttest_line_count = 2 added &&\n+\ttest_line_count = 0 removed &&\n+\ttest_line_count = 2 oldfiles &&\n+\ttest_line_count = 2 newfiles\n+}\n+\n+test_expect_success 'diff output with -O is well-formed' '\n+\ttest_diff_well_formed diff-foo-then-bar &&\n+\ttest_diff_well_formed diff-bar-then-foo\n+'\n+\n+test_expect_success 'flag -O affects diff output' '\n+\t! test_cmp diff-foo-then-bar diff-bar-then-foo\n+'\n+\n+test_expect_success 'orderfile is same as -O' '\n+\ttest_config diff.orderfile foo-then-bar &&\n+\tgit diff HEAD~1..HEAD >diff-foo-then-bar-config &&\n+\ttest_config diff.orderfile bar-then-foo &&\n+\tgit diff HEAD~1..HEAD >diff-bar-then-foo-config &&\n+\ttest_cmp diff-foo-then-bar diff-foo-then-bar-config &&\n+\ttest_cmp diff-bar-then-foo diff-bar-then-foo-config\n+'\n+\n+test_expect_success '-O overrides orderfile' '\n+\ttest_config diff.orderfile foo-then-bar &&\n+\tgit diff -Obar-then-foo HEAD~1..HEAD >diff-bar-then-foo-flag &&\n+\ttest_cmp diff-bar-then-foo diff-bar-then-foo-flag\n+'\n+\n+test_expect_success '/dev/null is same as no orderfile' '\n+\tgit diff -O/dev/null HEAD~1..HEAD>diff-null-orderfile &&\n+\tgit diff HEAD~1..HEAD >diff-default &&\n+\ttest_cmp diff-null-orderfile diff-default\n+'\n+\n+test_done\n-- \nMST\n"},{"id":"239554","messageId":"1398331809-11309-4-git-send-email-mst@redhat.com","threadId":"36490","inReplyTo":"1398331809-11309-1-git-send-email-mst@redhat.com","subject":"[PATCH v5 4/9] patch-id: make it stable against hunk reordering","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2014-04-24T09:31:00Z","receivedAt":"2014-04-24T09:31:00Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"Patch id changes if users\n1. reorder file diffs that make up a patch\nor\n2. split a patch up to multiple diffs that touch the same path\n(keeping hunks within a single diff ordered to make patch valid).\n\nAs the result is functionally equivalent, a different patch id is\nsurprising to many users.\nIn particular, reordering files using diff -O is helpful to make patches\nmore readable (e.g. API header diff before implementation diff).\n\nAdd an option to change patch-id behaviour making it stable against\nthese two kinds of patch change:\n1. calculate SHA1 hash for each hunk separately and sum all hashes\n(using a symmetrical sum) to get patch id\n2. hash the file-level headers together with each hunk (not just the\nfirst hunk)\n\nWe use a 20byte sum and not xor - since xor would give 0 output\nfor patches that have two identical diffs, which isn't all that\nunlikely (e.g. append the same line in two places).\n\nThe new behaviour is enabled\n- when patchid.stable is true\n- when --stable flag is present\n\nUsing a new flag --unstable or setting patchid.stable to false force\nthe historical behaviour.\n\nSigned-off-by: Michael S. Tsirkin \n---\n builtin/patch-id.c | 89 ++++++++++++++++++++++++++++++++++++++++++++----------\n 1 file changed, 73 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex 3cfe02d..037cf2f 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -1,17 +1,14 @@\n #include \"builtin.h\"\n \n-static void flush_current_id(int patchlen, unsigned char *id, git_SHA_CTX *c)\n+static void flush_current_id(int patchlen, unsigned char *id, unsigned char *result)\n {\n-\tunsigned char result[20];\n \tchar name[50];\n \n \tif (!patchlen)\n \t\treturn;\n \n-\tgit_SHA1_Final(result, c);\n \tmemcpy(name, sha1_to_hex(id), 41);\n \tprintf(\"%s %s\\n\", sha1_to_hex(result), name);\n-\tgit_SHA1_Init(c);\n }\n \n static int remove_space(char *line)\n@@ -56,10 +53,31 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)\n \treturn 1;\n }\n \n-static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx, struct strbuf *line_buf)\n+static void flush_one_hunk(unsigned char *result, git_SHA_CTX *ctx)\n {\n-\tint patchlen = 0, found_next = 0;\n+\tunsigned char hash[20];\n+\tunsigned short carry = 0;\n+\tint i;\n+\n+\tgit_SHA1_Final(hash, ctx);\n+\tgit_SHA1_Init(ctx);\n+\t/* 20-byte sum, with carry */\n+\tfor (i = 0; i < 20; ++i) {\n+\t\tcarry += result[i] + hash[i];\n+\t\tresult[i] = carry;\n+\t\tcarry >>= 8;\n+\t}\n+}\n+\n+static int get_one_patchid(unsigned char *next_sha1, unsigned char *result,\n+\t\t\t struct strbuf *line_buf, int stable)\n+{\n+\tint patchlen = 0, found_next = 0, hunks = 0;\n \tint before = -1, after = -1;\n+\tgit_SHA_CTX ctx, header_ctx;\n+\n+\tgit_SHA1_Init(&ctx);\n+\thashclr(result);\n \n \twhile (strbuf_getwholeline(line_buf, stdin, '\\n') != EOF) {\n \t\tchar *line = line_buf->buf;\n@@ -98,7 +116,19 @@ static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx, struct st\n \t\tif (before == 0 && after == 0) {\n \t\t\tif (!memcmp(line, \"@@ -\", 4)) {\n \t\t\t\t/* Parse next hunk, but ignore line numbers. */\n+\t\t\t\tif (stable) {\n+\t\t\t\t\t/* Hash the file-level headers together with each hunk. */\n+\t\t\t\t\tif (hunks) {\n+\t\t\t\t\t\tflush_one_hunk(result, &ctx);\n+\t\t\t\t\t\t/* Prepend saved header ctx for next hunk. */\n+\t\t\t\t\t\tmemcpy(&ctx, &header_ctx, sizeof(ctx));\n+\t\t\t\t\t} else {\n+\t\t\t\t\t\t/* Save header ctx for next hunk. */\n+\t\t\t\t\t\tmemcpy(&header_ctx, &ctx, sizeof(ctx));\n+\t\t\t\t\t}\n+\t\t\t\t}\n \t\t\t\tscan_hunk_header(line, &before, &after);\n+\t\t\t\thunks++;\n \t\t\t\tcontinue;\n \t\t\t}\n \n@@ -107,7 +137,10 @@ static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx, struct st\n \t\t\t\tbreak;\n \n \t\t\t/* Else we're parsing another header. */\n+\t\t\tif (stable && hunks)\n+\t\t\t\tflush_one_hunk(result, &ctx);\n \t\t\tbefore = after = -1;\n+\t\t\thunks = 0;\n \t\t}\n \n \t\t/* If we get here, we're inside a hunk. */\n@@ -119,39 +152,63 @@ static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx, struct st\n \t\t/* Compute the sha without whitespace */\n \t\tlen = remove_space(line);\n \t\tpatchlen += len;\n-\t\tgit_SHA1_Update(ctx, line, len);\n+\t\tgit_SHA1_Update(&ctx, line, len);\n \t}\n \n \tif (!found_next)\n \t\thashclr(next_sha1);\n \n+\tflush_one_hunk(result, &ctx);\n+\n \treturn patchlen;\n }\n \n-static void generate_id_list(void)\n+static void generate_id_list(int stable)\n {\n-\tunsigned char sha1[20], n[20];\n-\tgit_SHA_CTX ctx;\n+\tunsigned char sha1[20], n[20], result[20];\n \tint patchlen;\n \tstruct strbuf line_buf = STRBUF_INIT;\n \n-\tgit_SHA1_Init(&ctx);\n \thashclr(sha1);\n \twhile (!feof(stdin)) {\n-\t\tpatchlen = get_one_patchid(n, &ctx, &line_buf);\n-\t\tflush_current_id(patchlen, sha1, &ctx);\n+\t\tpatchlen = get_one_patchid(n, result, &line_buf, stable);\n+\t\tflush_current_id(patchlen, sha1, result);\n \t\thashcpy(sha1, n);\n \t}\n \tstrbuf_release(&line_buf);\n }\n \n-static const char patch_id_usage[] = \"git patch-id < patch\";\n+static const char patch_id_usage[] = \"git patch-id [--stable | --unstable] < patch\";\n+\n+static int git_patch_id_config(const char *var, const char *value, void *cb)\n+{\n+\tint *stable = cb;\n+\n+\tif (!strcmp(var, \"patchid.stable\")) {\n+\t\t*stable = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n+\treturn git_default_config(var, value, cb);\n+}\n \n int cmd_patch_id(int argc, const char **argv, const char *prefix)\n {\n-\tif (argc != 1)\n+\tint stable = -1;\n+\n+\tgit_config(git_patch_id_config, &stable);\n+\n+\t/* If nothing is set, default to unstable. */\n+\tif (stable < 0)\n+\t\tstable = 0;\n+\n+\tif (argc == 2 && !strcmp(argv[1], \"--stable\"))\n+\t\tstable = 1;\n+\telse if (argc == 2 && !strcmp(argv[1], \"--unstable\"))\n+\t\tstable = 0;\n+\telse if (argc != 1)\n \t\tusage(patch_id_usage);\n \n-\tgenerate_id_list();\n+\tgenerate_id_list(stable);\n \treturn 0;\n }\n-- \nMST\n"},{"id":"239552","messageId":"1398331809-11309-5-git-send-email-mst@redhat.com","threadId":"36490","inReplyTo":"1398331809-11309-1-git-send-email-mst@redhat.com","subject":"[PATCH v5 5/9] patch-id: document new behaviour","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2014-04-24T09:31:04Z","receivedAt":"2014-04-24T09:31:04Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"Clarify that patch ID can now be a sum of hashes, not a hash.\nDocument how command line and config options affect the\nbehaviour.\n\nSigned-off-by: Michael S. Tsirkin \n---\n Documentation/git-patch-id.txt | 23 ++++++++++++++++++-----\n 1 file changed, 18 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-patch-id.txt b/Documentation/git-patch-id.txt\nindex 312c3b1..e21b79b 100644\n--- a/Documentation/git-patch-id.txt\n+++ b/Documentation/git-patch-id.txt\n@@ -8,14 +8,14 @@ git-patch-id - Compute unique ID for a patch\n SYNOPSIS\n --------\n [verse]\n-'git patch-id' < \n+'git patch-id' [--stable | --unstable] < \n \n DESCRIPTION\n -----------\n-A \"patch ID\" is nothing but a SHA-1 of the diff associated with a patch, with\n-whitespace and line numbers ignored. As such, it's \"reasonably stable\", but at\n-the same time also reasonably unique, i.e., two patches that have the same \"patch\n-ID\" are almost guaranteed to be the same thing.\n+A \"patch ID\" is nothing but a sum of SHA-1 of the diff hunks associated with a\n+patch, with whitespace and line numbers ignored. As such, it's \"reasonably\n+stable\", but at the same time also reasonably unique, i.e., two patches that\n+have the same \"patch ID\" are almost guaranteed to be the same thing.\n \n IOW, you can use this thing to look for likely duplicate commits.\n \n@@ -27,6 +27,19 @@ This can be used to make a mapping from patch ID to commit ID.\n \n OPTIONS\n -------\n+\n+--stable::\n+\tUse a symmetrical sum of hashes as the patch ID.\n+\tWith this option, reordering file diffs that make up a patch or\n+\tsplitting a diff up to multiple diffs that touch the same path\n+\tdoes not affect the ID.\n+\tThis is the default if patchid.stable is set to true.\n+\n+--unstable::\n+\tUse a non-symmetrical sum of hashes, such that reordering\n+\tor splitting the patch does affect the ID.\n+\tThis is the default.\n+\n ::\n \tThe diff to create the ID of.\n \n-- \nMST\n"},{"id":"239553","messageId":"1398331809-11309-6-git-send-email-mst@redhat.com","threadId":"36490","inReplyTo":"1398331809-11309-1-git-send-email-mst@redhat.com","subject":"[PATCH v5 6/9] patch-id-test: test stable and unstable behaviour","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2014-04-24T09:31:07Z","receivedAt":"2014-04-24T09:31:07Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"Verify that patch ID supports an algorithm\nthat is stable against diff split and reordering.\n\nSigned-off-by: Michael S. Tsirkin \n---\n t/t4204-patch-id.sh | 128 +++++++++++++++++++++++++++++++++++++++++++++++-----\n 1 file changed, 117 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\nindex d2c930d..cd13e8e 100755\n--- a/t/t4204-patch-id.sh\n+++ b/t/t4204-patch-id.sh\n@@ -5,27 +5,51 @@ test_description='git patch-id'\n . ./test-lib.sh\n \n test_expect_success 'setup' '\n-\ttest_commit initial foo a &&\n-\ttest_commit first foo b &&\n-\tgit checkout -b same HEAD^ &&\n-\ttest_commit same-msg foo b &&\n-\tgit checkout -b notsame HEAD^ &&\n-\ttest_commit notsame-msg foo c\n+\tas=\"a a a a a a a a\" && # eight a\n+\ttest_write_lines $as >foo &&\n+\ttest_write_lines $as >bar &&\n+\tgit add foo bar &&\n+\tgit commit -a -m initial &&\n+\ttest_write_lines $as b >foo &&\n+\ttest_write_lines $as b >bar &&\n+\tgit commit -a -m first &&\n+\tgit checkout -b same master &&\n+\tgit commit --amend -m same-msg &&\n+\tgit checkout -b notsame master &&\n+\techo c >foo &&\n+\techo c >bar &&\n+\tgit commit --amend -a -m notsame-msg &&\n+\tgit checkout -b split master &&\n+\ttest_write_lines d $as b >foo &&\n+\ttest_write_lines d $as b >bar &&\n+\tgit commit -a -m split &&\n+\tgit checkout -b merged master &&\n+\tgit checkout split -- foo bar &&\n+\tgit commit --amend -a -m merged &&\n+\ttest_write_lines bar foo >bar-then-foo &&\n+\ttest_write_lines foo bar >foo-then-bar\n '\n \n test_expect_success 'patch-id output is well-formed' '\n-\tgit log -p -1 | git patch-id > output &&\n+\tgit log -p -1 | git patch-id >output &&\n \tgrep \"^[a-f0-9]\\{40\\} $(git rev-parse HEAD)$\" output\n '\n \n+#calculate patch id. Make sure output is not empty.\n calc_patch_id () {\n-\tgit patch-id |\n-\t\tsed \"s# .*##\" > patch-id_\"$1\"\n+\tname=\"$1\"\n+\tshift\n+\tgit patch-id \"$@\" |\n+\tsed \"s/ .*//\" >patch-id_\"$name\" &&\n+\ttest_line_count -gt 0 patch-id_\"$name\"\n+}\n+\n+get_top_diff () {\n+\tgit log -p -1 \"$@\" -O bar-then-foo --\n }\n \n get_patch_id () {\n-\tgit log -p -1 \"$1\" | git patch-id |\n-\t\tsed \"s# .*##\" > patch-id_\"$1\"\n+\tget_top_diff \"$1\" | calc_patch_id \"$@\"\n }\n \n test_expect_success 'patch-id detects equality' '\n@@ -56,6 +80,88 @@ test_expect_success 'whitespace is irrelevant in footer' '\n \ttest_cmp patch-id_master patch-id_same\n '\n \n+cmp_patch_id () {\n+\tif\n+\t\ttest \"$1\" = \"relevant\"\n+\tthen\n+\t\t! test_cmp patch-id_\"$2\" patch-id_\"$3\"\n+\telse\n+\t\ttest_cmp patch-id_\"$2\" patch-id_\"$3\"\n+\tfi\n+}\n+\n+test_patch_id_file_order () {\n+\trelevant=\"$1\"\n+\tshift\n+\tname=\"order-${1}-$relevant\"\n+\tshift\n+\tget_top_diff \"master\" | calc_patch_id \"$name\" \"$@\" &&\n+\tgit checkout same &&\n+\tgit format-patch -1 --stdout -O foo-then-bar |\n+\t\tcalc_patch_id \"ordered-$name\" \"$@\" &&\n+\tcmp_patch_id $relevant \"$name\" \"ordered-$name\"\n+\t\t\n+}\n+\n+test_patch_id_split () {\n+\trelevant=\"$1\"\n+\tshift\n+\tname=\"split-${1}-$relevant\"\n+\tshift\n+\tget_top_diff merged | calc_patch_id \"$name\" \"$@\" &&\n+\t(git log -p -1 -O foo-then-bar split~1; git diff split~1..split) |\n+\t\tcalc_patch_id \"split-$name\" \"$@\" &&\n+\tcmp_patch_id \"$relevant\" \"$name\" \"split-$name\"\n+}\n+\n+# combined test for options\n+test_patch_id () {\n+\ttest_patch_id_file_order \"$@\" &&\n+\ttest_patch_id_split \"$@\"\n+}\n+\n+# small tests with detailed diagnostic for basic options.\n+test_expect_success 'file order is irrelevant with --stable' '\n+\ttest_patch_id_file_order irrelevant --stable --stable\n+'\n+\n+test_expect_success 'file order is relevant with --unstable' '\n+\ttest_patch_id_file_order relevant --unstable --unstable\n+'\n+\n+test_expect_success 'splitting patch is irrelevant with --stable' '\n+\ttest_patch_id_split irrelevant --stable --stable\n+'\n+\n+test_expect_success 'splitting patch affects id with --unstable' '\n+\ttest_patch_id_split relevant --unstable --unstable\n+'\n+\n+#Now test various option combinations.\n+test_expect_success 'default is unstable' '\n+\ttest_patch_id relevant default\n+'\n+\n+test_expect_success 'patchid.stable = true is stable' '\n+\ttest_config patchid.stable true &&\n+\ttest_patch_id irrelevant patchid.stable=true\n+'\n+\n+test_expect_success 'patchid.stable = false is unstable' '\n+\ttest_config patchid.stable false &&\n+\ttest_patch_id relevant patchid.stable=false\n+'\n+\n+test_expect_success '--unstable overrides patchid.stable = true' '\n+\ttest_config patchid.stable true &&\n+\ttest_patch_id relevant patchid.stable=true--unstable --unstable\n+'\n+\n+test_expect_success '--stable overrides patchid.stable = false' '\n+\ttest_config patchid.stable false &&\n+\ttest_patch_id irrelevant patchid.stable=false--stable --stable\n+'\n+\n test_expect_success 'patch-id supports git-format-patch MIME output' '\n \tget_patch_id master &&\n \tgit checkout same &&\n-- \nMST\n"},{"id":"239556","messageId":"1398331809-11309-7-git-send-email-mst@redhat.com","threadId":"36490","inReplyTo":"1398331809-11309-1-git-send-email-mst@redhat.com","subject":"[PATCH v5 7/9] patch-id: change default to stable","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2014-04-24T09:31:10Z","receivedAt":"2014-04-24T09:31:10Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"--stable has been the default in 'next' for a few weeks with no ill\neffects.\nChange the default to that so that users don't have to remember to\nenable it.\n\nSigned-off-by: Michael S. Tsirkin \n---\n builtin/patch-id.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex 037cf2f..0b267af 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -198,9 +198,9 @@ int cmd_patch_id(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_patch_id_config, &stable);\n \n-\t/* If nothing is set, default to unstable. */\n+\t/* If nothing is set, default to stable. */\n \tif (stable < 0)\n-\t\tstable = 0;\n+\t\tstable = 1;\n \n \tif (argc == 2 && !strcmp(argv[1], \"--stable\"))\n \t\tstable = 1;\n-- \nMST\n"},{"id":"239550","messageId":"1398331809-11309-8-git-send-email-mst@redhat.com","threadId":"36490","inReplyTo":"1398331809-11309-1-git-send-email-mst@redhat.com","subject":"[PATCH v5 8/9] t4204-patch-id.sh: default is now stable","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2014-04-24T09:31:13Z","receivedAt":"2014-04-24T09:31:13Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"update test to match behaviour change\n\nSigned-off-by: Michael S. Tsirkin \n---\n t/t4204-patch-id.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\nindex cd13e8e..03f91ce 100755\n--- a/t/t4204-patch-id.sh\n+++ b/t/t4204-patch-id.sh\n@@ -138,8 +138,8 @@ test_expect_success 'splitting patch affects id with --unstable' '\n '\n \n #Now test various option combinations.\n-test_expect_success 'default is unstable' '\n-\ttest_patch_id relevant default\n+test_expect_success 'default is stable' '\n+\ttest_patch_id irrelevant default\n '\n \n test_expect_success 'patchid.stable = true is stable' '\n-- \nMST\n"},{"id":"239551","messageId":"1398331809-11309-9-git-send-email-mst@redhat.com","threadId":"36490","inReplyTo":"1398331809-11309-1-git-send-email-mst@redhat.com","subject":"[PATCH v5 9/9] Documentation/git-patch-id.txt: default is stable","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2014-04-24T09:31:16Z","receivedAt":"2014-04-24T09:31:16Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"Update documentation to match behaviour change.\n\nSigned-off-by: Michael S. Tsirkin \n---\n Documentation/git-patch-id.txt | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-patch-id.txt b/Documentation/git-patch-id.txt\nindex e21b79b..9299b90 100644\n--- a/Documentation/git-patch-id.txt\n+++ b/Documentation/git-patch-id.txt\n@@ -33,12 +33,13 @@ OPTIONS\n \tWith this option, reordering file diffs that make up a patch or\n \tsplitting a diff up to multiple diffs that touch the same path\n \tdoes not affect the ID.\n-\tThis is the default if patchid.stable is set to true.\n+\tThis is the default.\n \n --unstable::\n \tUse a non-symmetrical sum of hashes, such that reordering\n \tor splitting the patch does affect the ID.\n-\tThis is the default.\n+\tThis is the default if patchid.stable is set to false.\n+\tThis was the default value for git 1.9 and older.\n \n ::\n \tThe diff to create the ID of.\n-- \nMST\n"},{"id":"239573","messageId":"20140424170826.GH15516@google.com","threadId":"36490","inReplyTo":"1398331809-11309-2-git-send-email-mst@redhat.com","subject":"Re: [PATCH v5 2/9] test: add test_write_lines helper","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-04-24T17:08:26Z","receivedAt":"2014-04-24T17:08:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael S. Tsirkin wrote:\n\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -712,6 +712,11 @@ test_ln_s_add () {\n> \tfi\n> }\n> \n> +# This function writes out its parameters, one per line\n> +test_write_lines () {\n> +\tprintf \"%s\\n\" \"$@\";\n> +}\n> +\n\nThanks for fixing this.\n\nNits:\n\n * no need for the trailing semicolon\n * it's probably worth documenting this in t/README as well so people\n writing new test scripts know what it's about.\n\nThanks,\nJonathan\n"},{"id":"239574","messageId":"20140424171128.GI15516@google.com","threadId":"36490","inReplyTo":"1398331809-11309-3-git-send-email-mst@redhat.com","subject":"Re: [PATCH v5 3/9] tests: new test for orderfile options","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-04-24T17:11:29Z","receivedAt":"2014-04-24T17:11:29Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael S. Tsirkin wrote:\n\n> t/t4056-diff-order.sh | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 63 insertions(+)\n> create mode 100755 t/t4056-diff-order.sh\n\nI thought this file already existed since v1.9-rc0~8^2~3 (t4056: add\nnew tests for \"git diff -O\", 2013-12-18).\n\nWhat commit is your series based against?\n\nPuzzled,\nJonathan\n"},{"id":"239576","messageId":"20140424173043.GJ15516@google.com","threadId":"36490","inReplyTo":"1398331809-11309-4-git-send-email-mst@redhat.com","subject":"Re: [PATCH v5 4/9] patch-id: make it stable against hunk reordering","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-04-24T17:30:44Z","receivedAt":"2014-04-24T17:30:44Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nMichael S. Tsirkin wrote:\n\n> Patch id changes if users\n> 1. reorder file diffs that make up a patch\n> or\n> 2. split a patch up to multiple diffs that touch the same path\n> (keeping hunks within a single diff ordered to make patch valid).\n>\n> As the result is functionally equivalent, a different patch id is\n> surprising to many users.\n\nHm.\n\nIf the goal is that functionally equivalent patches are guaranteed to\nproduce the same patch-id, I wonder if we should be doing something\nlike the following:\n\n 1. apply the patch in memory\n 2. generate a new diff\n 3. use that new diff to produce a patch-id\n\nOtherwise issues like --diff-algorithm=patience versus =myers will\ncreate trouble too. I don't think that avoiding false negatives for\npatch comparison without doing something like that is really possible.\n\nOn the other hand if someone reorders file diffs within a patch, that\nis a potentially very common thing to do and something worth fixing.\nIn other words, while your (1) makes perfect sense to me, case (2)\nseems less convincing.\n\nThe downside of allowing reordering hunks is that it can potentially\nmake different patches to be treated the same (for example if they\nwere making similar changes to different functions) when the ordering\npreviously caused them to be distinguished. But that wasn't something\npeople could count on anyway, so I don't mind.\n\nShould the internal patch-id computation used by commands like 'git\ncherry' (see diff.c::diff_get_patch_id) get the same change? (Not a\nrhetorical question --- I don't know what the right choice would be\nthere.)\n\n[...]\n> The new behaviour is enabled\n> - when patchid.stable is true\n> - when --stable flag is present\n>\n> Using a new flag --unstable or setting patchid.stable to false force\n> the historical behaviour.\n\nWhich is the default?\n\n[...]\n> builtin/patch-id.c | 89 ++++++++++++++++++++++++++++++++++++++++++++----------\n> 1 file changed, 73 insertions(+), 16 deletions(-)\n\nDocumentation? Tests?\n\nThanks,\nJonathan\n"},{"id":"239577","messageId":"20140424173325.GK15516@google.com","threadId":"36490","inReplyTo":"1398331809-11309-5-git-send-email-mst@redhat.com","subject":"Re: [PATCH v5 5/9] patch-id: document new behaviour","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-04-24T17:33:25Z","receivedAt":"2014-04-24T17:33:25Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael S. Tsirkin wrote:\n\n> Documentation/git-patch-id.txt | 23 ++++++++++++++++++-----\n> 1 file changed, 18 insertions(+), 5 deletions(-)\n\nAh, there's the documentation. Please squash this with the patch that\nintroduces the new behavior so they can be reviewed together more\neasily (both now and later when people do archeology).\n\n[...]\n> +--stable::\n> +\tUse a symmetrical sum of hashes as the patch ID.\n> +\tWith this option, reordering file diffs that make up a patch or\n> +\tsplitting a diff up to multiple diffs that touch the same path\n> +\tdoes not affect the ID.\n> +\tThis is the default if patchid.stable is set to true.\n\nThis doesn't explain to me why I would want to use --stable versus\n--unstable. Maybe an EXAMPLES section would help?\n\nThe only reason I can think of to use --unstable is for compatibility\nwith historical patch-ids. Is there any other reason?\n\nAt this point in the series there is no patchid.stable configuration.\n\n> +--unstable::\n> +\tUse a non-symmetrical sum of hashes, such that reordering\n\nWhat is a non-symmetrical sum?\n\nThanks,\nJonathan\n"},{"id":"239585","messageId":"xmqqwqeevatc.fsf@gitster.dls.corp.google.com","threadId":"36490","inReplyTo":"20140424170826.GH15516@google.com","subject":"Re: [PATCH v5 2/9] test: add test_write_lines helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-24T18:31:27Z","receivedAt":"2014-04-24T18:31:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Michael S. Tsirkin wrote:\n>\n>> --- a/t/test-lib-functions.sh\n>> +++ b/t/test-lib-functions.sh\n>> @@ -712,6 +712,11 @@ test_ln_s_add () {\n>> \tfi\n>> }\n>> \n>> +# This function writes out its parameters, one per line\n>> +test_write_lines () {\n>> +\tprintf \"%s\\n\" \"$@\";\n>> +}\n>> +\n>\n> Thanks for fixing this.\n>\n> Nits:\n>\n> * no need for the trailing semicolon\n\nGood eyes. I was wondering if I wrote that (which I found very\nunlikely).\n\n> * it's probably worth documenting this in t/README as well so people\n> writing new test scripts know what it's about.\n\nSounds like a good idea.\n\nThanks.\n"},{"id":"239592","messageId":"xmqqoazqva5s.fsf@gitster.dls.corp.google.com","threadId":"36490","inReplyTo":"1398331809-11309-3-git-send-email-mst@redhat.com","subject":"Re: [PATCH v5 3/9] tests: new test for orderfile options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-24T18:45:35Z","receivedAt":"2014-04-24T18:45:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Michael S. Tsirkin\" writes:\n\n> The test is very basic and can be extended.\n> Couldn't find a good existing place to put it,\n> so created a new file.\n>\n> Signed-off-by: Michael S. Tsirkin \n> ---\n> t/t4056-diff-order.sh | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 63 insertions(+)\n> create mode 100755 t/t4056-diff-order.sh\n>\n> diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh\n> new file mode 100755\n> index 0000000..0404f50\n\nHuh? What codebase is this based on?\n\nI think we had t4056 since b5277730 (t4056: add new tests for \"git\ndiff -O\", 2013-12-18).\n\nPuzzled...\n\n> --- /dev/null\n> +++ b/t/t4056-diff-order.sh\n> @@ -0,0 +1,63 @@\n> +#!/bin/sh\n> +\n> +test_description='diff orderfile'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup' '\n> +\tas=\"a a a a a a a a\" && # eight a\n> +\ttest_write_lines $as >foo &&\n> +\ttest_write_lines $as >bar &&\n> +\tgit add foo bar &&\n> +\tgit commit -a -m initial &&\n> +\ttest_write_lines $as b >foo &&\n> +\ttest_write_lines $as b >bar &&\n> +\tgit commit -a -m first &&\n> +\ttest_write_lines bar foo >bar-then-foo &&\n> +\ttest_write_lines foo bar >foo-then-bar &&\n> +\tgit diff -Ofoo-then-bar HEAD~1..HEAD >diff-foo-then-bar &&\n> +\tgit diff -Obar-then-foo HEAD~1..HEAD >diff-bar-then-foo\n> +'\n> +\n> +test_diff_well_formed () {\n> +\tgrep ^+b \"$1\" >added\n> +\tgrep ^-b \"$1\" >removed\n> +\tgrep ^+++ \"$1\" >oldfiles\n> +\tgrep ^--- \"$1\" >newfiles\n> +\ttest_line_count = 2 added &&\n> +\ttest_line_count = 0 removed &&\n> +\ttest_line_count = 2 oldfiles &&\n> +\ttest_line_count = 2 newfiles\n> +}\n> +\n> +test_expect_success 'diff output with -O is well-formed' '\n> +\ttest_diff_well_formed diff-foo-then-bar &&\n> +\ttest_diff_well_formed diff-bar-then-foo\n> +'\n> +\n> +test_expect_success 'flag -O affects diff output' '\n> +\t! test_cmp diff-foo-then-bar diff-bar-then-foo\n> +'\n> +\n> +test_expect_success 'orderfile is same as -O' '\n> +\ttest_config diff.orderfile foo-then-bar &&\n> +\tgit diff HEAD~1..HEAD >diff-foo-then-bar-config &&\n> +\ttest_config diff.orderfile bar-then-foo &&\n> +\tgit diff HEAD~1..HEAD >diff-bar-then-foo-config &&\n> +\ttest_cmp diff-foo-then-bar diff-foo-then-bar-config &&\n> +\ttest_cmp diff-bar-then-foo diff-bar-then-foo-config\n> +'\n> +\n> +test_expect_success '-O overrides orderfile' '\n> +\ttest_config diff.orderfile foo-then-bar &&\n> +\tgit diff -Obar-then-foo HEAD~1..HEAD >diff-bar-then-foo-flag &&\n> +\ttest_cmp diff-bar-then-foo diff-bar-then-foo-flag\n> +'\n> +\n> +test_expect_success '/dev/null is same as no orderfile' '\n> +\tgit diff -O/dev/null HEAD~1..HEAD>diff-null-orderfile &&\n> +\tgit diff HEAD~1..HEAD >diff-default &&\n> +\ttest_cmp diff-null-orderfile diff-default\n> +'\n> +\n> +test_done\n"},{"id":"239595","messageId":"xmqqd2g6v8we.fsf@gitster.dls.corp.google.com","threadId":"36490","inReplyTo":"20140424173043.GJ15516@google.com","subject":"Re: [PATCH v5 4/9] patch-id: make it stable against hunk reordering","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-24T19:12:49Z","receivedAt":"2014-04-24T19:12:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Should the internal patch-id computation used by commands like 'git\n> cherry' (see diff.c::diff_get_patch_id) get the same change? (Not a\n> rhetorical question --- I don't know what the right choice would be\n> there.)\n\nI thought about it but I did not think of a reason why. If we do\nnot store the patch-id (it would be a misnomer especially after this\nseries, it is mor like patch signature), and we generate the patch\nto be hashed internally without getting affected by any user input\ngiven per-invocation, then nothing is externally observable even if\nwe used two completely different definition of patch id computation,\nand I think these preconditions do hold.\n"},{"id":"239605","messageId":"20140424212639.GA9129@redhat.com","threadId":"36490","inReplyTo":"20140424173325.GK15516@google.com","subject":"Re: [PATCH v5 5/9] patch-id: document new behaviour","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2014-04-24T21:26:39Z","receivedAt":"2014-04-24T21:26:39Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Thu, Apr 24, 2014 at 10:33:25AM -0700, Jonathan Nieder wrote:\n> Michael S. Tsirkin wrote:\n> \n> > Documentation/git-patch-id.txt | 23 ++++++++++++++++++-----\n> > 1 file changed, 18 insertions(+), 5 deletions(-)\n> \n> Ah, there's the documentation. Please squash this with the patch that\n> introduces the new behavior so they can be reviewed together more\n> easily (both now and later when people do archeology).\n> \n> [...]\n> > +--stable::\n> > +\tUse a symmetrical sum of hashes as the patch ID.\n> > +\tWith this option, reordering file diffs that make up a patch or\n> > +\tsplitting a diff up to multiple diffs that touch the same path\n> > +\tdoes not affect the ID.\n> > +\tThis is the default if patchid.stable is set to true.\n> \n> This doesn't explain to me why I would want to use --stable versus\n> --unstable. Maybe an EXAMPLES section would help?\n> \n> The only reason I can think of to use --unstable is for compatibility\n> with historical patch-ids. Is there any other reason?\n> \n> At this point in the series there is no patchid.stable configuration.\n> \n> > +--unstable::\n> > +\tUse a non-symmetrical sum of hashes, such that reordering\n> \n> What is a non-symmetrical sum?\n\nNon-symmetrical combination function is better?\n\n> Thanks,\n> Jonathan\n"},{"id":"239606","messageId":"20140424213238.GB9129@redhat.com","threadId":"36490","inReplyTo":"20140424173043.GJ15516@google.com","subject":"Re: [PATCH v5 4/9] patch-id: make it stable against hunk reordering","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2014-04-24T21:32:38Z","receivedAt":"2014-04-24T21:32:38Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Thu, Apr 24, 2014 at 10:30:44AM -0700, Jonathan Nieder wrote:\n> Hi,\n> \n> Michael S. Tsirkin wrote:\n> \n> > Patch id changes if users\n> > 1. reorder file diffs that make up a patch\n> > or\n> > 2. split a patch up to multiple diffs that touch the same path\n> > (keeping hunks within a single diff ordered to make patch valid).\n> >\n> > As the result is functionally equivalent, a different patch id is\n> > surprising to many users.\n> \n> Hm.\n> \n> If the goal is that functionally equivalent patches are guaranteed to\n> produce the same patch-id, I wonder if we should be doing something\n> like the following:\n> \n> 1. apply the patch in memory\n> 2. generate a new diff\n> 3. use that new diff to produce a patch-id\n> \n> Otherwise issues like --diff-algorithm=patience versus =myers will\n> create trouble too. I don't think that avoiding false negatives for\n> patch comparison without doing something like that is really possible.\n> \n> On the other hand if someone reorders file diffs within a patch, that\n> is a potentially very common thing to do and something worth fixing.\n> In other words, while your (1) makes perfect sense to me, case (2)\n> seems less convincing.\n\nI agree it's less convincing: one would have to edit patch\nby hand (which I used to sometimes do to make important parts more prominent,\nbut stopped doing in favor of splitting a patch).\nI'm not 100% sure whether it's worth supporting or not.\n\n\n> The downside of allowing reordering hunks is that it can potentially\n> make different patches to be treated the same (for example if they\n> were making similar changes to different functions) when the ordering\n> previously caused them to be distinguished. But that wasn't something\n> people could count on anyway, so I don't mind.\n\nI think this example convinces me. I'll drop this support in the next version.\n\n> Should the internal patch-id computation used by commands like 'git\n> cherry' (see diff.c::diff_get_patch_id) get the same change? (Not a\n> rhetorical question --- I don't know what the right choice would be\n> there.)\n> \n> [...]\n> > The new behaviour is enabled\n> > - when patchid.stable is true\n> > - when --stable flag is present\n> >\n> > Using a new flag --unstable or setting patchid.stable to false force\n> > the historical behaviour.\n> \n> Which is the default?\n> \n> [...]\n> > builtin/patch-id.c | 89 ++++++++++++++++++++++++++++++++++++++++++++----------\n> > 1 file changed, 73 insertions(+), 16 deletions(-)\n> \n> Documentation? Tests?\n> \n> Thanks,\n> Jonathan\n"},{"id":"239608","messageId":"20140424213912.GD9129@redhat.com","threadId":"36490","inReplyTo":"xmqqoazqva5s.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 3/9] tests: new test for orderfile options","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2014-04-24T21:39:12Z","receivedAt":"2014-04-24T21:39:12Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Thu, Apr 24, 2014 at 11:45:35AM -0700, Junio C Hamano wrote:\n> \"Michael S. Tsirkin\" writes:\n> \n> > The test is very basic and can be extended.\n> > Couldn't find a good existing place to put it,\n> > so created a new file.\n> >\n> > Signed-off-by: Michael S. Tsirkin \n> > ---\n> > t/t4056-diff-order.sh | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++\n> > 1 file changed, 63 insertions(+)\n> > create mode 100755 t/t4056-diff-order.sh\n> >\n> > diff --git a/t/t4056-diff-order.sh b/t/t4056-diff-order.sh\n> > new file mode 100755\n> > index 0000000..0404f50\n> \n> Huh? What codebase is this based on?\n> \n> I think we had t4056 since b5277730 (t4056: add new tests for \"git\n> diff -O\", 2013-12-18).\n> \n> Puzzled...\n\n\nYes I didn't rebase in a while: 7794a680e63a2a11b73cb1194653662f2769a792\nwas the tip.\nI'll rebase, sorry.\n\n> > --- /dev/null\n> > +++ b/t/t4056-diff-order.sh\n> > @@ -0,0 +1,63 @@\n> > +#!/bin/sh\n> > +\n> > +test_description='diff orderfile'\n> > +\n> > +. ./test-lib.sh\n> > +\n> > +test_expect_success 'setup' '\n> > +\tas=\"a a a a a a a a\" && # eight a\n> > +\ttest_write_lines $as >foo &&\n> > +\ttest_write_lines $as >bar &&\n> > +\tgit add foo bar &&\n> > +\tgit commit -a -m initial &&\n> > +\ttest_write_lines $as b >foo &&\n> > +\ttest_write_lines $as b >bar &&\n> > +\tgit commit -a -m first &&\n> > +\ttest_write_lines bar foo >bar-then-foo &&\n> > +\ttest_write_lines foo bar >foo-then-bar &&\n> > +\tgit diff -Ofoo-then-bar HEAD~1..HEAD >diff-foo-then-bar &&\n> > +\tgit diff -Obar-then-foo HEAD~1..HEAD >diff-bar-then-foo\n> > +'\n> > +\n> > +test_diff_well_formed () {\n> > +\tgrep ^+b \"$1\" >added\n> > +\tgrep ^-b \"$1\" >removed\n> > +\tgrep ^+++ \"$1\" >oldfiles\n> > +\tgrep ^--- \"$1\" >newfiles\n> > +\ttest_line_count = 2 added &&\n> > +\ttest_line_count = 0 removed &&\n> > +\ttest_line_count = 2 oldfiles &&\n> > +\ttest_line_count = 2 newfiles\n> > +}\n> > +\n> > +test_expect_success 'diff output with -O is well-formed' '\n> > +\ttest_diff_well_formed diff-foo-then-bar &&\n> > +\ttest_diff_well_formed diff-bar-then-foo\n> > +'\n> > +\n> > +test_expect_success 'flag -O affects diff output' '\n> > +\t! test_cmp diff-foo-then-bar diff-bar-then-foo\n> > +'\n> > +\n> > +test_expect_success 'orderfile is same as -O' '\n> > +\ttest_config diff.orderfile foo-then-bar &&\n> > +\tgit diff HEAD~1..HEAD >diff-foo-then-bar-config &&\n> > +\ttest_config diff.orderfile bar-then-foo &&\n> > +\tgit diff HEAD~1..HEAD >diff-bar-then-foo-config &&\n> > +\ttest_cmp diff-foo-then-bar diff-foo-then-bar-config &&\n> > +\ttest_cmp diff-bar-then-foo diff-bar-then-foo-config\n> > +'\n> > +\n> > +test_expect_success '-O overrides orderfile' '\n> > +\ttest_config diff.orderfile foo-then-bar &&\n> > +\tgit diff -Obar-then-foo HEAD~1..HEAD >diff-bar-then-foo-flag &&\n> > +\ttest_cmp diff-bar-then-foo diff-bar-then-foo-flag\n> > +'\n> > +\n> > +test_expect_success '/dev/null is same as no orderfile' '\n> > +\tgit diff -O/dev/null HEAD~1..HEAD>diff-null-orderfile &&\n> > +\tgit diff HEAD~1..HEAD >diff-default &&\n> > +\ttest_cmp diff-null-orderfile diff-default\n> > +'\n> > +\n> > +test_done\n"},{"id":"239612","messageId":"xmqqzjjatm0x.fsf@gitster.dls.corp.google.com","threadId":"36490","inReplyTo":"20140424212639.GA9129@redhat.com","subject":"Re: [PATCH v5 5/9] patch-id: document new behaviour","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-24T22:12:14Z","receivedAt":"2014-04-24T22:12:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Michael S. Tsirkin\" writes:\n\n>> > +--unstable::\n>> > +\tUse a non-symmetrical sum of hashes, such that reordering\n>> \n>> What is a non-symmetrical sum?\n>\n> Non-symmetrical combination function is better?\n\nI do not think either is very good X-<.\n\nThe primary points to convey for \"--stable\" are:\n\n - Two patches produced by comparing the same two trees with two\n different settings for \"-O\" will result in the same\n patchc signature, thereby allowing the computed result to be used\n as a key to index some metainformation about the change between\n the two trees;\n\n - It will produce a result different from the plain vanilla\n patch-id has always produced even when used on a diff output\n taken without any use of \"-O\", thereby making existing\n databases keyed by patch-ids unusable.\n\nThe fact that we happened to use a patch-id that catches that\nsomebody reordered the same patch into different file order and\ndeclares that they are two different changes is a more historical\naccident than a designed goal.\n\nI would even say that we would have used the \"stable\" version from\nthe beginning if we thought that \"-O\" would be widely\nused when these two features both appeared. Even though I was the\nguilty one who introduced it, I'd admit that \"-O\" has\nmerely been a curiosity from its inception and has been a failed\nexperiment, not in the sense that the feature does not work as\nadverertised (it does), but in the sense that it is not widely used\n(evidenced by the lack of complaints on missing diff.orderfile for a\nlong time) at all. With \"-O\" being a failed experiment,\nthe \"unstability\" did not matter, so it has stuck.\n\nThe only two things worth mentioning about \"--unstable\", if our\nfuture direction is to see diff.orderfile and \"--stable\" a lot more\nwidely used, are:\n\n (1) it keeps producing the same patch-id as existing versions of\n Git, so users with existing databases (who do not deal with\n reordered patches) may want to use it; and perhaps\n\n (2) it will not consider a patch taken with \"-O\" and\n another without it from the same source the same patches.\n\nMathmatically speaking, mentioning \"non-symmetrial\" might be one way\nof expressing the latter point (2), but stressing on that alone\nwithout mentioning (1) misses the point. (2) is _not_ a designed\nfeature, so it is not very interesting. Unless you have an existing\ndatabase, there is no reason to use \"--unstable\".\n\nOn the other hand (1) is a very relevant thing to mention, as we are\ntalking about a feature that, if unused, may break existing users'\ndata.\n"},{"id":"239814","messageId":"20140427182623.GA28551@redhat.com","threadId":"36490","inReplyTo":"xmqqzjjatm0x.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 5/9] patch-id: document new behaviour","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2014-04-27T18:26:23Z","receivedAt":"2014-04-27T18:26:23Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Thu, Apr 24, 2014 at 03:12:14PM -0700, Junio C Hamano wrote:\n> \"Michael S. Tsirkin\" writes:\n> \n> >> > +--unstable::\n> >> > +\tUse a non-symmetrical sum of hashes, such that reordering\n> >> \n> >> What is a non-symmetrical sum?\n> >\n> > Non-symmetrical combination function is better?\n> \n> I do not think either is very good X-<.\n> \n> The primary points to convey for \"--stable\" are:\n> \n> - Two patches produced by comparing the same two trees with two\n> different settings for \"-O\" will result in the same\n> patchc signature, thereby allowing the computed result to be used\n> as a key to index some metainformation about the change between\n> the two trees;\n> \n> - It will produce a result different from the plain vanilla\n> patch-id has always produced even when used on a diff output\n> taken without any use of \"-O\", thereby making existing\n> databases keyed by patch-ids unusable.\n> \n> The fact that we happened to use a patch-id that catches that\n> somebody reordered the same patch into different file order and\n> declares that they are two different changes is a more historical\n> accident than a designed goal.\n> \n> I would even say that we would have used the \"stable\" version from\n> the beginning if we thought that \"-O\" would be widely\n> used when these two features both appeared. Even though I was the\n> guilty one who introduced it, I'd admit that \"-O\" has\n> merely been a curiosity from its inception and has been a failed\n> experiment, not in the sense that the feature does not work as\n> adverertised (it does), but in the sense that it is not widely used\n> (evidenced by the lack of complaints on missing diff.orderfile for a\n> long time) at all. With \"-O\" being a failed experiment,\n> the \"unstability\" did not matter, so it has stuck.\n> \n> The only two things worth mentioning about \"--unstable\", if our\n> future direction is to see diff.orderfile and \"--stable\" a lot more\n> widely used, are:\n> \n> (1) it keeps producing the same patch-id as existing versions of\n> Git, so users with existing databases (who do not deal with\n> reordered patches) may want to use it; and perhaps\n> \n> (2) it will not consider a patch taken with \"-O\" and\n> another without it from the same source the same patches.\n> \n> Mathmatically speaking, mentioning \"non-symmetrial\" might be one way\n> of expressing the latter point (2), but stressing on that alone\n> without mentioning (1) misses the point. (2) is _not_ a designed\n> feature, so it is not very interesting. Unless you have an existing\n> database, there is no reason to use \"--unstable\".\n> \n> On the other hand (1) is a very relevant thing to mention, as we are\n> talking about a feature that, if unused, may break existing users'\n> data.\n\nOK I did just that, pls take a look.\n"}]}