{"thread":{"id":"51001","subject":"[PATCH 1/2] format-patch: Inform user that patch-id generation is unstable","startedAt":"2019-04-26T23:52:02Z","lastAt":"2019-05-08T02:56:48Z","messageCount":5,"participants":["Stephen Boyd","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"374585","messageId":"20190426235157.106350-1-sboyd@kernel.org","threadId":"51001","inReplyTo":null,"subject":"[PATCH 1/2] format-patch: Inform user that patch-id generation is unstable","fromName":"Stephen Boyd","fromEmail":"sboyd@kernel.org","sentAt":"2019-04-26T23:51:56Z","receivedAt":"2019-04-26T23:52:02Z","isPatch":true,"sender":{"key":"sboyd@kernel.org","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"I tried out 'git format-patch --base' with a set of commits that\nmodifies more than one file. It turns out that the way this command is\nimplemented it actually uses the unstable version of patch-id instead of\nthe stable version that's documented. When I tried to modify the\nexisting test to use 'git patch-id --stable' vs. 'git patch-id\n--unstable' I found that it didn't matter, the test still passed.\n\nLet's expand on the test here so it is a little more complicated and\nthen use that to show that the patch-id generation is actually unstable\nvs. stable. Update the documentation as well.\n\nCc: Xiaolong Ye <xiaolong.ye@intel.com>\nSigned-off-by: Stephen Boyd <sboyd@kernel.org>\n---\n Documentation/git-format-patch.txt |  2 +-\n t/t4014-format-patch.sh            | 36 +++++++++++++++++++++++++-----\n 2 files changed, 32 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\nindex 1af85d404f51..e8cc792e7f5d 100644\n--- a/Documentation/git-format-patch.txt\n+++ b/Documentation/git-format-patch.txt\n@@ -583,7 +583,7 @@ of 'base commit' in topological order before the patches can be applied.\n The 'base commit' is shown as \"base-commit: \" followed by the 40-hex of\n the commit object name.  A 'prerequisite patch' is shown as\n \"prerequisite-patch-id: \" followed by the 40-hex 'patch id', which can\n-be obtained by passing the patch through the `git patch-id --stable`\n+be obtained by passing the patch through the `git patch-id --unstable`\n command.\n \n Imagine that on top of the public commit P, you applied well-known\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex b6e2fdbc4410..e82c6c7d9177 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -36,8 +36,27 @@ test_expect_success setup '\n \tgit checkout master &&\n \tgit diff-tree -p C2 | git apply --index &&\n \ttest_tick &&\n-\tgit commit -m \"Master accepts moral equivalent of #2\"\n+\tgit commit -m \"Master accepts moral equivalent of #2\" &&\n \n+\tgit checkout side &&\n+\tgit checkout -b patchid &&\n+\tfor i in 5 6 1 2 3 A 4 B C 7 8 9 10 D E F; do echo \"$i\"; done >file2 &&\n+\tfor i in 1 2 3 A 4 B C 7 8 9 10 D E F 5 6; do echo \"$i\"; done >file3 &&\n+\tfor i in 8 9 10; do echo \"$i\"; done >file &&\n+\tgit add file file2 file3 &&\n+\ttest_tick &&\n+\tgit commit -m \"patchid 1\" &&\n+\tfor i in 4 A B 7 8 9 10; do echo \"$i\"; done >file2 &&\n+\tfor i in 8 9 10 5 6; do echo \"$i\"; done >file3 &&\n+\tgit add file2 file3 &&\n+\ttest_tick &&\n+\tgit commit -m \"patchid 2\" &&\n+\tfor i in 10 5 6; do echo \"$i\"; done >file &&\n+\tgit add file &&\n+\ttest_tick &&\n+\tgit commit -m \"patchid 3\" &&\n+\n+\tgit checkout master\n '\n \n test_expect_success \"format-patch --ignore-if-in-upstream\" '\n@@ -1559,16 +1578,23 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' '\n '\n \n test_expect_success 'format-patch --base' '\n-\tgit checkout side &&\n+\tgit checkout patchid &&\n \tgit format-patch --stdout --base=HEAD~3 -1 | tail -n 7 >actual1 &&\n \tgit format-patch --stdout --base=HEAD~3 HEAD~.. | tail -n 7 >actual2 &&\n \techo >expected &&\n \techo \"base-commit: $(git rev-parse HEAD~3)\" >>expected &&\n-\techo \"prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --stable | awk \"{print \\$1}\")\" >>expected &&\n-\techo \"prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --stable | awk \"{print \\$1}\")\" >>expected &&\n+\techo \"prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --unstable | awk \"{print \\$1}\")\" >>expected &&\n+\techo \"prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --unstable | awk \"{print \\$1}\")\" >>expected &&\n \tsignature >> expected &&\n \ttest_cmp expected actual1 &&\n-\ttest_cmp expected actual2\n+\ttest_cmp expected actual2 &&\n+\techo >fail &&\n+\techo \"base-commit: $(git rev-parse HEAD~3)\" >>fail &&\n+\techo \"prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --stable | awk \"{print \\$1}\")\" >>fail &&\n+\techo \"prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --stable | awk \"{print \\$1}\")\" >>fail &&\n+\tsignature >> fail &&\n+\t! test_cmp fail actual1 &&\n+\t! test_cmp fail actual2\n '\n \n test_expect_success 'format-patch --base errors out when base commit is in revision list' '\n-- \nSent by a computer through tubes\n\n"},{"id":"374586","messageId":"20190426235157.106350-2-sboyd@kernel.org","threadId":"51001","inReplyTo":"20190426235157.106350-1-sboyd@kernel.org","subject":"[PATCH 2/2] format-patch: Make --base patch-id output stable","fromName":"Stephen Boyd","fromEmail":"sboyd@kernel.org","sentAt":"2019-04-26T23:51:57Z","receivedAt":"2019-04-26T23:52:03Z","isPatch":true,"sender":{"key":"sboyd@kernel.org","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"We weren't flushing the context each time we processed a hunk in the\npatch-id generation code in diff.c, but we were doing that when we\ngenerated \"stable\" patch-ids with the 'patch-id' tool. Let's port that\nsimilar logic over from patch-id.c into diff.c so we can get the same\nhash when we're generating patch-ids for 'format-patch --base=' types of\ncommand invocations.\n\nCc: Xiaolong Ye <xiaolong.ye@intel.com>\nSigned-off-by: Stephen Boyd <sboyd@kernel.org>\n---\n\nI wonder if we need to make some other sort of form of\n\"prerequisite-patch-id:\" here and let that be a legacy form of the\npatch-id so that users know that they have a fixed version of this code?\nMaybe \"prerequisite-stable-patch-id:\"? Or we don't have to care because\nit's been broken for anything besides the most trivial type of patches\nand presumably users aren't able to use it with 'patch-id --stable'?\n\n Documentation/git-format-patch.txt |  2 +-\n builtin/log.c                      |  2 +-\n builtin/patch-id.c                 | 17 +---------------\n diff.c                             | 32 +++++++++++++++++++++++++-----\n diff.h                             |  3 ++-\n patch-ids.c                        | 10 +++++-----\n patch-ids.h                        |  2 +-\n t/t4014-format-patch.sh            |  8 ++++----\n 8 files changed, 42 insertions(+), 34 deletions(-)\n\ndiff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\nindex e8cc792e7f5d..1af85d404f51 100644\n--- a/Documentation/git-format-patch.txt\n+++ b/Documentation/git-format-patch.txt\n@@ -583,7 +583,7 @@ of 'base commit' in topological order before the patches can be applied.\n The 'base commit' is shown as \"base-commit: \" followed by the 40-hex of\n the commit object name.  A 'prerequisite patch' is shown as\n \"prerequisite-patch-id: \" followed by the 40-hex 'patch id', which can\n-be obtained by passing the patch through the `git patch-id --unstable`\n+be obtained by passing the patch through the `git patch-id --stable`\n command.\n \n Imagine that on top of the public commit P, you applied well-known\ndiff --git a/builtin/log.c b/builtin/log.c\nindex e43ee12fb1dd..147850dc7317 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1435,7 +1435,7 @@ static void prepare_bases(struct base_tree_info *bases,\n \t\tstruct object_id *patch_id;\n \t\tif (*commit_base_at(&commit_base, commit))\n \t\t\tcontinue;\n-\t\tif (commit_patch_id(commit, &diffopt, &oid, 0))\n+\t\tif (commit_patch_id(commit, &diffopt, &oid, 0, 1))\n \t\t\tdie(_(\"cannot get patch id\"));\n \t\tALLOC_GROW(bases->patch_id, bases->nr_patch_id + 1, bases->alloc_patch_id);\n \t\tpatch_id = bases->patch_id + bases->nr_patch_id;\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex 970d0d30b4f4..bd28b80b2d0f 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -1,5 +1,6 @@\n #include \"builtin.h\"\n #include \"config.h\"\n+#include \"diff.h\"\n \n static void flush_current_id(int patchlen, struct object_id *id, struct object_id *result)\n {\n@@ -54,22 +55,6 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)\n \treturn 1;\n }\n \n-static void flush_one_hunk(struct object_id *result, git_SHA_CTX *ctx)\n-{\n-\tunsigned char hash[GIT_MAX_RAWSZ];\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 < GIT_SHA1_RAWSZ; ++i) {\n-\t\tcarry += result->hash[i] + hash[i];\n-\t\tresult->hash[i] = carry;\n-\t\tcarry >>= 8;\n-\t}\n-}\n-\n static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \t\t\t   struct strbuf *line_buf, int stable)\n {\ndiff --git a/diff.c b/diff.c\nindex 4d3cf83a27e5..8e3675b416a2 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5988,6 +5988,22 @@ static int remove_space(char *line, int len)\n \treturn dst - line;\n }\n \n+void flush_one_hunk(struct object_id *result, git_SHA_CTX *ctx)\n+{\n+\tunsigned char hash[GIT_MAX_RAWSZ];\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 < GIT_SHA1_RAWSZ; ++i) {\n+\t\tcarry += result->hash[i] + hash[i];\n+\t\tresult->hash[i] = carry;\n+\t\tcarry >>= 8;\n+\t}\n+}\n+\n static void patch_id_consume(void *priv, char *line, unsigned long len)\n {\n \tstruct patch_id_t *data = priv;\n@@ -6012,8 +6028,8 @@ static void patch_id_add_mode(git_SHA_CTX *ctx, unsigned mode)\n \tgit_SHA1_Update(ctx, buf, len);\n }\n \n-/* returns 0 upon success, and writes result into sha1 */\n-static int diff_get_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only)\n+/* returns 0 upon success, and writes result into oid */\n+static int diff_get_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only, int stable)\n {\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n \tint i;\n@@ -6023,6 +6039,7 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid\n \tgit_SHA1_Init(&ctx);\n \tmemset(&data, 0, sizeof(struct patch_id_t));\n \tdata.ctx = &ctx;\n+\toidclr(oid);\n \n \tfor (i = 0; i < q->nr; i++) {\n \t\txpparam_t xpp;\n@@ -6098,17 +6115,22 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid\n \t\t\t\t  patch_id_consume, &data, &xpp, &xecfg))\n \t\t\treturn error(\"unable to generate patch-id diff for %s\",\n \t\t\t\t     p->one->path);\n+\n+\t\tif (stable)\n+\t\t\tflush_one_hunk(oid, &ctx);\n \t}\n \n-\tgit_SHA1_Final(oid->hash, &ctx);\n+\tif (!stable)\n+\t\tgit_SHA1_Final(oid->hash, &ctx);\n+\n \treturn 0;\n }\n \n-int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only)\n+int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only, int stable)\n {\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n \tint i;\n-\tint result = diff_get_patch_id(options, oid, diff_header_only);\n+\tint result = diff_get_patch_id(options, oid, diff_header_only, stable);\n \n \tfor (i = 0; i < q->nr; i++)\n \t\tdiff_free_filepair(q->queue[i]);\ndiff --git a/diff.h b/diff.h\nindex b20cbcc09142..de24a048649e 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -436,7 +436,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option);\n int run_diff_index(struct rev_info *revs, int cached);\n \n int do_diff_cache(const struct object_id *, struct diff_options *);\n-int diff_flush_patch_id(struct diff_options *, struct object_id *, int);\n+int diff_flush_patch_id(struct diff_options *, struct object_id *, int, int);\n+void flush_one_hunk(struct object_id *, git_SHA_CTX *);\n \n int diff_result_code(struct diff_options *, int);\n \ndiff --git a/patch-ids.c b/patch-ids.c\nindex c262e1be9c9c..f70d3966542d 100644\n--- a/patch-ids.c\n+++ b/patch-ids.c\n@@ -11,7 +11,7 @@ static int patch_id_defined(struct commit *commit)\n }\n \n int commit_patch_id(struct commit *commit, struct diff_options *options,\n-\t\t    struct object_id *oid, int diff_header_only)\n+\t\t    struct object_id *oid, int diff_header_only, int stable)\n {\n \tif (!patch_id_defined(commit))\n \t\treturn -1;\n@@ -22,7 +22,7 @@ int commit_patch_id(struct commit *commit, struct diff_options *options,\n \telse\n \t\tdiff_root_tree_oid(&commit->object.oid, \"\", options);\n \tdiffcore_std(options);\n-\treturn diff_flush_patch_id(options, oid, diff_header_only);\n+\treturn diff_flush_patch_id(options, oid, diff_header_only, stable);\n }\n \n /*\n@@ -46,11 +46,11 @@ static int patch_id_neq(const void *cmpfn_data,\n \tstruct patch_id *b = (void *)entry_or_key;\n \n \tif (is_null_oid(&a->patch_id) &&\n-\t    commit_patch_id(a->commit, opt, &a->patch_id, 0))\n+\t    commit_patch_id(a->commit, opt, &a->patch_id, 0, 0))\n \t\treturn error(\"Could not get patch ID for %s\",\n \t\t\toid_to_hex(&a->commit->object.oid));\n \tif (is_null_oid(&b->patch_id) &&\n-\t    commit_patch_id(b->commit, opt, &b->patch_id, 0))\n+\t    commit_patch_id(b->commit, opt, &b->patch_id, 0, 0))\n \t\treturn error(\"Could not get patch ID for %s\",\n \t\t\toid_to_hex(&b->commit->object.oid));\n \treturn !oideq(&a->patch_id, &b->patch_id);\n@@ -80,7 +80,7 @@ static int init_patch_id_entry(struct patch_id *patch,\n \tstruct object_id header_only_patch_id;\n \n \tpatch->commit = commit;\n-\tif (commit_patch_id(commit, &ids->diffopts, &header_only_patch_id, 1))\n+\tif (commit_patch_id(commit, &ids->diffopts, &header_only_patch_id, 1, 0))\n \t\treturn -1;\n \n \thashmap_entry_init(patch, sha1hash(header_only_patch_id.hash));\ndiff --git a/patch-ids.h b/patch-ids.h\nindex 82a12b66f889..03bb04e7071f 100644\n--- a/patch-ids.h\n+++ b/patch-ids.h\n@@ -20,7 +20,7 @@ struct patch_ids {\n };\n \n int commit_patch_id(struct commit *commit, struct diff_options *options,\n-\t\t    struct object_id *oid, int);\n+\t\t    struct object_id *oid, int, int);\n int init_patch_ids(struct repository *, struct patch_ids *);\n int free_patch_ids(struct patch_ids *);\n struct patch_id *add_commit_patch_id(struct commit *, struct patch_ids *);\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex e82c6c7d9177..7a5833a4b275 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1583,15 +1583,15 @@ test_expect_success 'format-patch --base' '\n \tgit format-patch --stdout --base=HEAD~3 HEAD~.. | tail -n 7 >actual2 &&\n \techo >expected &&\n \techo \"base-commit: $(git rev-parse HEAD~3)\" >>expected &&\n-\techo \"prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --unstable | awk \"{print \\$1}\")\" >>expected &&\n-\techo \"prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --unstable | awk \"{print \\$1}\")\" >>expected &&\n+\techo \"prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --stable | awk \"{print \\$1}\")\" >>expected &&\n+\techo \"prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --stable | awk \"{print \\$1}\")\" >>expected &&\n \tsignature >> expected &&\n \ttest_cmp expected actual1 &&\n \ttest_cmp expected actual2 &&\n \techo >fail &&\n \techo \"base-commit: $(git rev-parse HEAD~3)\" >>fail &&\n-\techo \"prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --stable | awk \"{print \\$1}\")\" >>fail &&\n-\techo \"prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --stable | awk \"{print \\$1}\")\" >>fail &&\n+\techo \"prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --unstable | awk \"{print \\$1}\")\" >>fail &&\n+\techo \"prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --unstable | awk \"{print \\$1}\")\" >>fail &&\n \tsignature >> fail &&\n \t! test_cmp fail actual1 &&\n \t! test_cmp fail actual2\n-- \nSent by a computer through tubes\n\n"},{"id":"374984","messageId":"xmqqpnounakf.fsf@gitster-ct.c.googlers.com","threadId":"51001","inReplyTo":"20190426235157.106350-2-sboyd@kernel.org","subject":"Re: [PATCH 2/2] format-patch: Make --base patch-id output stable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-07T04:38:24Z","receivedAt":"2019-05-07T04:38:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stephen Boyd <sboyd@kernel.org> writes:\n\n> I wonder if we need to make some other sort of form of\n> \"prerequisite-patch-id:\" here and let that be a legacy form of the\n> patch-id so that users know that they have a fixed version of this code?\n> Maybe \"prerequisite-stable-patch-id:\"? Or we don't have to care because\n> it's been broken for anything besides the most trivial type of patches\n> and presumably users aren't able to use it with 'patch-id --stable'?\n\nDo projects actively use -O<orderfile> when generating the patches?\nI had an impression that not many do, and without -O<orderfile> in\nthe picture, --unstable/--stable would not matter, no?\n\nSo, I am not sure if this matters very much in practice.\n\n>\n>  Documentation/git-format-patch.txt |  2 +-\n>  builtin/log.c                      |  2 +-\n>  builtin/patch-id.c                 | 17 +---------------\n>  diff.c                             | 32 +++++++++++++++++++++++++-----\n>  diff.h                             |  3 ++-\n>  patch-ids.c                        | 10 +++++-----\n>  patch-ids.h                        |  2 +-\n>  t/t4014-format-patch.sh            |  8 ++++----\n>  8 files changed, 42 insertions(+), 34 deletions(-)\n>\n> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\n> index e8cc792e7f5d..1af85d404f51 100644\n> --- a/Documentation/git-format-patch.txt\n> +++ b/Documentation/git-format-patch.txt\n> @@ -583,7 +583,7 @@ of 'base commit' in topological order before the patches can be applied.\n>  The 'base commit' is shown as \"base-commit: \" followed by the 40-hex of\n>  the commit object name.  A 'prerequisite patch' is shown as\n>  \"prerequisite-patch-id: \" followed by the 40-hex 'patch id', which can\n> -be obtained by passing the patch through the `git patch-id --unstable`\n> +be obtained by passing the patch through the `git patch-id --stable`\n>  command.\n>  \n>  Imagine that on top of the public commit P, you applied well-known\n> diff --git a/builtin/log.c b/builtin/log.c\n> index e43ee12fb1dd..147850dc7317 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -1435,7 +1435,7 @@ static void prepare_bases(struct base_tree_info *bases,\n>  \t\tstruct object_id *patch_id;\n>  \t\tif (*commit_base_at(&commit_base, commit))\n>  \t\t\tcontinue;\n> -\t\tif (commit_patch_id(commit, &diffopt, &oid, 0))\n> +\t\tif (commit_patch_id(commit, &diffopt, &oid, 0, 1))\n>  \t\t\tdie(_(\"cannot get patch id\"));\n>  \t\tALLOC_GROW(bases->patch_id, bases->nr_patch_id + 1, bases->alloc_patch_id);\n>  \t\tpatch_id = bases->patch_id + bases->nr_patch_id;\n> diff --git a/builtin/patch-id.c b/builtin/patch-id.c\n> index 970d0d30b4f4..bd28b80b2d0f 100644\n> --- a/builtin/patch-id.c\n> +++ b/builtin/patch-id.c\n> @@ -1,5 +1,6 @@\n>  #include \"builtin.h\"\n>  #include \"config.h\"\n> +#include \"diff.h\"\n>  \n>  static void flush_current_id(int patchlen, struct object_id *id, struct object_id *result)\n>  {\n> @@ -54,22 +55,6 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)\n>  \treturn 1;\n>  }\n>  \n> -static void flush_one_hunk(struct object_id *result, git_SHA_CTX *ctx)\n> -{\n> -\tunsigned char hash[GIT_MAX_RAWSZ];\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 < GIT_SHA1_RAWSZ; ++i) {\n> -\t\tcarry += result->hash[i] + hash[i];\n> -\t\tresult->hash[i] = carry;\n> -\t\tcarry >>= 8;\n> -\t}\n> -}\n> -\n>  static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n>  \t\t\t   struct strbuf *line_buf, int stable)\n>  {\n> diff --git a/diff.c b/diff.c\n> index 4d3cf83a27e5..8e3675b416a2 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -5988,6 +5988,22 @@ static int remove_space(char *line, int len)\n>  \treturn dst - line;\n>  }\n>  \n> +void flush_one_hunk(struct object_id *result, git_SHA_CTX *ctx)\n> +{\n> +\tunsigned char hash[GIT_MAX_RAWSZ];\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 < GIT_SHA1_RAWSZ; ++i) {\n> +\t\tcarry += result->hash[i] + hash[i];\n> +\t\tresult->hash[i] = carry;\n> +\t\tcarry >>= 8;\n> +\t}\n> +}\n> +\n>  static void patch_id_consume(void *priv, char *line, unsigned long len)\n>  {\n>  \tstruct patch_id_t *data = priv;\n> @@ -6012,8 +6028,8 @@ static void patch_id_add_mode(git_SHA_CTX *ctx, unsigned mode)\n>  \tgit_SHA1_Update(ctx, buf, len);\n>  }\n>  \n> -/* returns 0 upon success, and writes result into sha1 */\n> -static int diff_get_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only)\n> +/* returns 0 upon success, and writes result into oid */\n> +static int diff_get_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only, int stable)\n>  {\n>  \tstruct diff_queue_struct *q = &diff_queued_diff;\n>  \tint i;\n> @@ -6023,6 +6039,7 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid\n>  \tgit_SHA1_Init(&ctx);\n>  \tmemset(&data, 0, sizeof(struct patch_id_t));\n>  \tdata.ctx = &ctx;\n> +\toidclr(oid);\n>  \n>  \tfor (i = 0; i < q->nr; i++) {\n>  \t\txpparam_t xpp;\n> @@ -6098,17 +6115,22 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid\n>  \t\t\t\t  patch_id_consume, &data, &xpp, &xecfg))\n>  \t\t\treturn error(\"unable to generate patch-id diff for %s\",\n>  \t\t\t\t     p->one->path);\n> +\n> +\t\tif (stable)\n> +\t\t\tflush_one_hunk(oid, &ctx);\n>  \t}\n>  \n> -\tgit_SHA1_Final(oid->hash, &ctx);\n> +\tif (!stable)\n> +\t\tgit_SHA1_Final(oid->hash, &ctx);\n> +\n>  \treturn 0;\n>  }\n>  \n> -int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only)\n> +int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only, int stable)\n>  {\n>  \tstruct diff_queue_struct *q = &diff_queued_diff;\n>  \tint i;\n> -\tint result = diff_get_patch_id(options, oid, diff_header_only);\n> +\tint result = diff_get_patch_id(options, oid, diff_header_only, stable);\n>  \n>  \tfor (i = 0; i < q->nr; i++)\n>  \t\tdiff_free_filepair(q->queue[i]);\n> diff --git a/diff.h b/diff.h\n> index b20cbcc09142..de24a048649e 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -436,7 +436,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option);\n>  int run_diff_index(struct rev_info *revs, int cached);\n>  \n>  int do_diff_cache(const struct object_id *, struct diff_options *);\n> -int diff_flush_patch_id(struct diff_options *, struct object_id *, int);\n> +int diff_flush_patch_id(struct diff_options *, struct object_id *, int, int);\n> +void flush_one_hunk(struct object_id *, git_SHA_CTX *);\n>  \n>  int diff_result_code(struct diff_options *, int);\n>  \n> diff --git a/patch-ids.c b/patch-ids.c\n> index c262e1be9c9c..f70d3966542d 100644\n> --- a/patch-ids.c\n> +++ b/patch-ids.c\n> @@ -11,7 +11,7 @@ static int patch_id_defined(struct commit *commit)\n>  }\n>  \n>  int commit_patch_id(struct commit *commit, struct diff_options *options,\n> -\t\t    struct object_id *oid, int diff_header_only)\n> +\t\t    struct object_id *oid, int diff_header_only, int stable)\n>  {\n>  \tif (!patch_id_defined(commit))\n>  \t\treturn -1;\n> @@ -22,7 +22,7 @@ int commit_patch_id(struct commit *commit, struct diff_options *options,\n>  \telse\n>  \t\tdiff_root_tree_oid(&commit->object.oid, \"\", options);\n>  \tdiffcore_std(options);\n> -\treturn diff_flush_patch_id(options, oid, diff_header_only);\n> +\treturn diff_flush_patch_id(options, oid, diff_header_only, stable);\n>  }\n>  \n>  /*\n> @@ -46,11 +46,11 @@ static int patch_id_neq(const void *cmpfn_data,\n>  \tstruct patch_id *b = (void *)entry_or_key;\n>  \n>  \tif (is_null_oid(&a->patch_id) &&\n> -\t    commit_patch_id(a->commit, opt, &a->patch_id, 0))\n> +\t    commit_patch_id(a->commit, opt, &a->patch_id, 0, 0))\n>  \t\treturn error(\"Could not get patch ID for %s\",\n>  \t\t\toid_to_hex(&a->commit->object.oid));\n>  \tif (is_null_oid(&b->patch_id) &&\n> -\t    commit_patch_id(b->commit, opt, &b->patch_id, 0))\n> +\t    commit_patch_id(b->commit, opt, &b->patch_id, 0, 0))\n>  \t\treturn error(\"Could not get patch ID for %s\",\n>  \t\t\toid_to_hex(&b->commit->object.oid));\n>  \treturn !oideq(&a->patch_id, &b->patch_id);\n> @@ -80,7 +80,7 @@ static int init_patch_id_entry(struct patch_id *patch,\n>  \tstruct object_id header_only_patch_id;\n>  \n>  \tpatch->commit = commit;\n> -\tif (commit_patch_id(commit, &ids->diffopts, &header_only_patch_id, 1))\n> +\tif (commit_patch_id(commit, &ids->diffopts, &header_only_patch_id, 1, 0))\n>  \t\treturn -1;\n>  \n>  \thashmap_entry_init(patch, sha1hash(header_only_patch_id.hash));\n> diff --git a/patch-ids.h b/patch-ids.h\n> index 82a12b66f889..03bb04e7071f 100644\n> --- a/patch-ids.h\n> +++ b/patch-ids.h\n> @@ -20,7 +20,7 @@ struct patch_ids {\n>  };\n>  \n>  int commit_patch_id(struct commit *commit, struct diff_options *options,\n> -\t\t    struct object_id *oid, int);\n> +\t\t    struct object_id *oid, int, int);\n>  int init_patch_ids(struct repository *, struct patch_ids *);\n>  int free_patch_ids(struct patch_ids *);\n>  struct patch_id *add_commit_patch_id(struct commit *, struct patch_ids *);\n> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n> index e82c6c7d9177..7a5833a4b275 100755\n> --- a/t/t4014-format-patch.sh\n> +++ b/t/t4014-format-patch.sh\n> @@ -1583,15 +1583,15 @@ test_expect_success 'format-patch --base' '\n>  \tgit format-patch --stdout --base=HEAD~3 HEAD~.. | tail -n 7 >actual2 &&\n>  \techo >expected &&\n>  \techo \"base-commit: $(git rev-parse HEAD~3)\" >>expected &&\n> -\techo \"prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --unstable | awk \"{print \\$1}\")\" >>expected &&\n> -\techo \"prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --unstable | awk \"{print \\$1}\")\" >>expected &&\n> +\techo \"prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --stable | awk \"{print \\$1}\")\" >>expected &&\n> +\techo \"prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --stable | awk \"{print \\$1}\")\" >>expected &&\n>  \tsignature >> expected &&\n>  \ttest_cmp expected actual1 &&\n>  \ttest_cmp expected actual2 &&\n>  \techo >fail &&\n>  \techo \"base-commit: $(git rev-parse HEAD~3)\" >>fail &&\n> -\techo \"prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --stable | awk \"{print \\$1}\")\" >>fail &&\n> -\techo \"prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --stable | awk \"{print \\$1}\")\" >>fail &&\n> +\techo \"prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --unstable | awk \"{print \\$1}\")\" >>fail &&\n> +\techo \"prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --unstable | awk \"{print \\$1}\")\" >>fail &&\n>  \tsignature >> fail &&\n>  \t! test_cmp fail actual1 &&\n>  \t! test_cmp fail actual2\n"},{"id":"375046","messageId":"155725118838.14659.6891235578536334187@swboyd.mtv.corp.google.com","threadId":"51001","inReplyTo":"xmqqpnounakf.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] format-patch: Make --base patch-id output stable","fromName":"Stephen Boyd","fromEmail":"sboyd@kernel.org","sentAt":"2019-05-07T17:46:28Z","receivedAt":"2019-05-07T17:46:32Z","isPatch":true,"sender":{"key":"sboyd@kernel.org","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"Quoting Junio C Hamano (2019-05-06 21:38:24)\n> Stephen Boyd <sboyd@kernel.org> writes:\n> \n> > I wonder if we need to make some other sort of form of\n> > \"prerequisite-patch-id:\" here and let that be a legacy form of the\n> > patch-id so that users know that they have a fixed version of this code?\n> > Maybe \"prerequisite-stable-patch-id:\"? Or we don't have to care because\n> > it's been broken for anything besides the most trivial type of patches\n> > and presumably users aren't able to use it with 'patch-id --stable'?\n> \n> Do projects actively use -O<orderfile> when generating the patches?\n> I had an impression that not many do, and without -O<orderfile> in\n> the picture, --unstable/--stable would not matter, no?\n> \n> So, I am not sure if this matters very much in practice.\n> \n\nI'm not really concerned with projects using -O<orderfile> for patch\ngeneration. I'm concerned that the documentation told users to use\n--stable and that didn't work for me when there was more than one hunk\nin the patch. This leads me to believe that either I'm doing something\nwrong or the other users of this feature have been using --unstable\nsince this was added to git format-patch.\n\n"},{"id":"375086","messageId":"xmqq1s19k61g.fsf@gitster-ct.c.googlers.com","threadId":"51001","inReplyTo":"155725118838.14659.6891235578536334187@swboyd.mtv.corp.google.com","subject":"Re: [PATCH 2/2] format-patch: Make --base patch-id output stable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-08T02:56:43Z","receivedAt":"2019-05-08T02:56:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stephen Boyd <sboyd@kernel.org> writes:\n\n> Quoting Junio C Hamano (2019-05-06 21:38:24)\n>> Stephen Boyd <sboyd@kernel.org> writes:\n>> \n>> > I wonder if we need to make some other sort of form of\n>> > \"prerequisite-patch-id:\" here and let that be a legacy form of the\n>> > patch-id so that users know that they have a fixed version of this code?\n>> > Maybe \"prerequisite-stable-patch-id:\"? Or we don't have to care because\n>> > it's been broken for anything besides the most trivial type of patches\n>> > and presumably users aren't able to use it with 'patch-id --stable'?\n>> \n>> Do projects actively use -O<orderfile> when generating the patches?\n>> I had an impression that not many do, and without -O<orderfile> in\n>> the picture, --unstable/--stable would not matter, no?\n>> \n>> So, I am not sure if this matters very much in practice.\n>> \n>\n> I'm not really concerned with projects using -O<orderfile> for patch\n> generation. \n\nI think I misunderstood, then.  I have been assuming that the order\nof target file paths was the primary thing that contributes to the\ndifferences between --[un]stable modes, but apparently I forgot\nabout that 30e12b92 (\"patch-id: make it stable against hunk\nreordering\", 2014-04-27) affects even a patch that touches a single\npath.\n\nIf we advise \"--stable\" in the documentation to those who wants to\ninterpret \"--base\", then I agree with the goal of this series to\nmake sure that is what actually is happening.\n\nThanks for working on this.\n"}]}