{"thread":{"id":"58470","subject":"[PATCH 0/2] update internal patch-id to use \"stable\" algorithm","startedAt":"2022-09-20T05:59:00Z","lastAt":"2022-10-25T00:32:04Z","messageCount":46,"participants":["Jerry Zhang via GitGitGadget","Junio C Hamano","Jerry Zhang"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"463276","messageId":"pull.1359.git.1663653505.gitgitgadget@gmail.com","threadId":"58470","inReplyTo":null,"subject":"[PATCH 0/2] update internal patch-id to use \"stable\" algorithm","fromName":"Jerry Zhang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-20T05:58:23Z","receivedAt":"2022-09-20T05:59:00Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"Internal usage of patch-id in rebase / cherry-pick doesn't persist\npatch-ids, so there's no need to specifically invoke the unstable variant.\n\nThis allows the unstable logic to be cleaned up.\n\nIn the process, fixed a bug in the combination of --stable with binary files\nand header-only, and expanded the test to cover both binary and non-binary\nfiles.\n\nSigned-off-by: Jerry Zhang jerry@skydio.com\n\nJerry Zhang (2):\n patch-id: fix stable patch id for binary / header-only\n patch-id: use stable patch-id for rebases\n\n builtin/log.c | 2 +-\n diff.c | 44 ++++++++++++++++----------------------\n diff.h | 2 +-\n patch-ids.c | 10 ++++-----\n patch-ids.h | 2 +-\n t/t3419-rebase-patch-id.sh | 19 ++++++++++------\n 6 files changed, 39 insertions(+), 40 deletions(-)\n\n\nbase-commit: e188ec3a735ae52a0d0d3c22f9df6b29fa613b1e\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1359%2Fjerry-skydio%2Fjerry%2Frevup%2Fmaster%2Fpatch_ids-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1359/jerry-skydio/jerry/revup/master/patch_ids-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1359\n-- \ngitgitgadget\n"},{"id":"463277","messageId":"6abb1ced1bde21098502342fcb776fe6805a7873.1663653506.git.gitgitgadget@gmail.com","threadId":"58470","inReplyTo":"pull.1359.git.1663653505.gitgitgadget@gmail.com","subject":"[PATCH 2/2] patch-id: use stable patch-id for rebases","fromName":"Jerry Zhang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-20T05:58:25Z","receivedAt":"2022-09-20T05:59:04Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"From: Jerry Zhang \n\nGit doesn't persist patch-ids during the rebase process, so there is\nno need to specifically invoke the unstable variant.\n\nThis allows the legacy unstable id logic to be cleaned up.\n\nSigned-off-by: Jerry Zhang \n---\n builtin/log.c | 2 +-\n diff.c | 12 ++++--------\n diff.h | 2 +-\n patch-ids.c | 10 +++++-----\n patch-ids.h | 2 +-\n 5 files changed, 12 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 047f9e5278d..3bb49fd7406 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1762,7 +1762,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, 1))\n+\t\tif (commit_patch_id(commit, &diffopt, &oid, 0))\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/diff.c b/diff.c\nindex 2f8f0c2e4f4..8c46531cd44 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6185,7 +6185,7 @@ static void patch_id_add_mode(git_hash_ctx *ctx, unsigned mode)\n }\n \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+static int diff_get_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only)\n {\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n \tint i;\n@@ -6268,21 +6268,17 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid\n \t\t\t\treturn error(\"unable to generate patch-id diff for %s\",\n \t\t\t\t\t p->one->path);\n \t\t}\n-\t\tif (stable)\n-\t\t\tflush_one_hunk(oid, &ctx);\n+\t\tflush_one_hunk(oid, &ctx);\n \t}\n \n-\tif (!stable)\n-\t\tthe_hash_algo->final_oid_fn(oid, &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, int stable)\n+int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only)\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, stable);\n+\tint result = diff_get_patch_id(options, oid, diff_header_only);\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 8ae18e5ab1e..fd33caeb25d 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -634,7 +634,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option);\n int run_diff_index(struct rev_info *revs, unsigned int option);\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, int);\n+int diff_flush_patch_id(struct diff_options *, struct object_id *, int);\n void flush_one_hunk(struct object_id *result, git_hash_ctx *ctx);\n \n int diff_result_code(struct diff_options *, int);\ndiff --git a/patch-ids.c b/patch-ids.c\nindex 8bf425555de..fefddc487e9 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, int stable)\n+\t\t struct object_id *oid, int diff_header_only)\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, stable);\n+\treturn diff_flush_patch_id(options, oid, diff_header_only);\n }\n \n /*\n@@ -48,11 +48,11 @@ static int patch_id_neq(const void *cmpfn_data,\n \tb = container_of(entry_or_key, struct patch_id, ent);\n \n \tif (is_null_oid(&a->patch_id) &&\n-\t commit_patch_id(a->commit, opt, &a->patch_id, 0, 0))\n+\t commit_patch_id(a->commit, opt, &a->patch_id, 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, 0))\n+\t commit_patch_id(b->commit, opt, &b->patch_id, 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@@ -82,7 +82,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, 0))\n+\tif (commit_patch_id(commit, &ids->diffopts, &header_only_patch_id, 1))\n \t\treturn -1;\n \n \thashmap_entry_init(&patch->ent, oidhash(&header_only_patch_id));\ndiff --git a/patch-ids.h b/patch-ids.h\nindex ab6c6a68047..490d7393716 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, int);\n+\t\t struct object_id *oid, int);\n int init_patch_ids(struct repository *, struct patch_ids *);\n int free_patch_ids(struct patch_ids *);\n \n-- \ngitgitgadget\n"},{"id":"463278","messageId":"82fe77c1ce0122423c6bd67ccbf472a3fc7f5c3e.1663653506.git.gitgitgadget@gmail.com","threadId":"58470","inReplyTo":"pull.1359.git.1663653505.gitgitgadget@gmail.com","subject":"[PATCH 1/2] patch-id: fix stable patch id for binary / header-only","fromName":"Jerry Zhang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-20T05:58:24Z","receivedAt":"2022-09-20T05:59:07Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"From: Jerry Zhang \n\nPrevious logic here skipped flushing the hunks for binary\nand header-only patch ids, which would always result in a\npatch-id of 0000.\n\nReorder the logic to branch into 3 cases for populating the\npatch body: header-only which populates nothing, binary which\npopulates the object ids, and normal which populates the text\ndiff. All branches will end up flushing the hunk.\n\nUpdate the test to run on both binary and normal files.\n\nSigned-off-by: Jerry Zhang \n---\n diff.c | 32 ++++++++++++++------------------\n t/t3419-rebase-patch-id.sh | 19 +++++++++++++------\n 2 files changed, 27 insertions(+), 24 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex dd68281ba44..2f8f0c2e4f4 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6248,30 +6248,26 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid\n \t\t\tthe_hash_algo->update_fn(&ctx, p->two->path, len2);\n \t\t}\n \n-\t\tif (diff_header_only)\n-\t\t\tcontinue;\n-\n-\t\tif (fill_mmfile(options->repo, &mf1, p->one) < 0 ||\n-\t\t fill_mmfile(options->repo, &mf2, p->two) < 0)\n-\t\t\treturn error(\"unable to read files to diff\");\n-\n-\t\tif (diff_filespec_is_binary(options->repo, p->one) ||\n+\t\tif (diff_header_only) {\n+\t\t\t// Don't do anything since we're only populating header info\n+\t\t} else if (diff_filespec_is_binary(options->repo, p->one) ||\n \t\t diff_filespec_is_binary(options->repo, p->two)) {\n \t\t\tthe_hash_algo->update_fn(&ctx, oid_to_hex(&p->one->oid),\n \t\t\t\t\tthe_hash_algo->hexsz);\n \t\t\tthe_hash_algo->update_fn(&ctx, oid_to_hex(&p->two->oid),\n \t\t\t\t\tthe_hash_algo->hexsz);\n-\t\t\tcontinue;\n+\t\t} else {\n+\t\t\tif (fill_mmfile(options->repo, &mf1, p->one) < 0 ||\n+\t\t\t fill_mmfile(options->repo, &mf2, p->two) < 0)\n+\t\t\t\treturn error(\"unable to read files to diff\");\n+\t\t\txpp.flags = 0;\n+\t\t\txecfg.ctxlen = 3;\n+\t\t\txecfg.flags = XDL_EMIT_NO_HUNK_HDR;\n+\t\t\tif (xdi_diff_outf(&mf1, &mf2, NULL,\n+\t\t\t\t\t patch_id_consume, &data, &xpp, &xecfg))\n+\t\t\t\treturn error(\"unable to generate patch-id diff for %s\",\n+\t\t\t\t\t p->one->path);\n \t\t}\n-\n-\t\txpp.flags = 0;\n-\t\txecfg.ctxlen = 3;\n-\t\txecfg.flags = XDL_EMIT_NO_HUNK_HDR;\n-\t\tif (xdi_diff_outf(&mf1, &mf2, NULL,\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}\ndiff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh\nindex 295040f2fe3..f7b7e9e5b7c 100755\n--- a/t/t3419-rebase-patch-id.sh\n+++ b/t/t3419-rebase-patch-id.sh\n@@ -46,10 +46,6 @@ test_expect_success 'setup: 500 lines' '\n \tgit cherry-pick main >/dev/null 2>&1\n '\n \n-test_expect_success 'setup attributes' '\n-\techo \"file binary\" >.gitattributes\n-'\n-\n test_expect_success 'detect upstream patch' '\n \tgit checkout -q main &&\n \tscramble file &&\n@@ -58,7 +54,13 @@ test_expect_success 'detect upstream patch' '\n \tgit checkout -q other^{} &&\n \tgit rebase main &&\n \tgit rev-list main...HEAD~ >revs &&\n-\ttest_must_be_empty revs\n+\ttest_must_be_empty revs &&\n+\techo \"file binary\" >.gitattributes &&\n+\tgit checkout -q other^{} &&\n+\tgit rebase main &&\n+\tgit rev-list main...HEAD~ >revs &&\n+\ttest_must_be_empty revs &&\n+\trm .gitattributes\n '\n \n test_expect_success 'do not drop patch' '\n@@ -68,7 +70,12 @@ test_expect_success 'do not drop patch' '\n \tgit commit -q -m squashed &&\n \tgit checkout -q other^{} &&\n \ttest_must_fail git rebase squashed &&\n-\tgit rebase --quit\n+\tgit rebase --abort &&\n+\techo \"file binary\" >.gitattributes &&\n+\tgit checkout -q other^{} &&\n+\ttest_must_fail git rebase squashed &&\n+\tgit rebase --abort &&\n+\trm .gitattributes\n '\n \n test_done\n-- \ngitgitgadget\n\n"},{"id":"463279","messageId":"pull.1359.v2.git.1663654859.gitgitgadget@gmail.com","threadId":"58470","inReplyTo":"pull.1359.git.1663653505.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] update internal patch-id to use \"stable\" algorithm","fromName":"Jerry Zhang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-20T06:20:57Z","receivedAt":"2022-09-20T06:21:08Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"Internal usage of patch-id in rebase / cherry-pick doesn't persist\npatch-ids, so there's no need to specifically invoke the unstable variant.\n\nThis allows the unstable logic to be cleaned up.\n\nIn the process, fixed a bug in the combination of --stable with binary files\nand header-only, and expanded the test to cover both binary and non-binary\nfiles.\n\nSigned-off-by: Jerry Zhang jerry@skydio.com\n\nJerry Zhang (2):\n patch-id: fix stable patch id for binary / header-only\n patch-id: use stable patch-id for rebases\n\n builtin/log.c | 2 +-\n diff.c | 44 ++++++++++++++++----------------------\n diff.h | 2 +-\n patch-ids.c | 10 ++++-----\n patch-ids.h | 2 +-\n t/t3419-rebase-patch-id.sh | 19 ++++++++++------\n 6 files changed, 39 insertions(+), 40 deletions(-)\n\n\nbase-commit: e188ec3a735ae52a0d0d3c22f9df6b29fa613b1e\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1359%2Fjerry-skydio%2Fjerry%2Frevup%2Fmaster%2Fpatch_ids-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1359/jerry-skydio/jerry/revup/master/patch_ids-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1359\n\nRange-diff vs v1:\n\n 1: 82fe77c1ce0 ! 1: 945508df7b6 patch-id: fix stable patch id for binary / header-only\n @@ diff.c: static int diff_get_patch_id(struct diff_options *options, struct object\n -\n -\t\tif (diff_filespec_is_binary(options->repo, p->one) ||\n +\t\tif (diff_header_only) {\n -+\t\t\t// Don't do anything since we're only populating header info\n ++\t\t\t/* don't do anything since we're only populating header info */\n +\t\t} else if (diff_filespec_is_binary(options->repo, p->one) ||\n \t\t diff_filespec_is_binary(options->repo, p->two)) {\n \t\t\tthe_hash_algo->update_fn(&ctx, oid_to_hex(&p->one->oid),\n 2: 6abb1ced1bd = 2: 30ec43cd129 patch-id: use stable patch-id for rebases\n\n-- \ngitgitgadget\n"},{"id":"463280","messageId":"945508df7b6335cb419b2769755c484236538c8e.1663654859.git.gitgitgadget@gmail.com","threadId":"58470","inReplyTo":"pull.1359.v2.git.1663654859.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] patch-id: fix stable patch id for binary / header-only","fromName":"Jerry Zhang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-20T06:20:58Z","receivedAt":"2022-09-20T06:21:10Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"From: Jerry Zhang \n\nPrevious logic here skipped flushing the hunks for binary\nand header-only patch ids, which would always result in a\npatch-id of 0000.\n\nReorder the logic to branch into 3 cases for populating the\npatch body: header-only which populates nothing, binary which\npopulates the object ids, and normal which populates the text\ndiff. All branches will end up flushing the hunk.\n\nUpdate the test to run on both binary and normal files.\n\nSigned-off-by: Jerry Zhang \n---\n diff.c | 32 ++++++++++++++------------------\n t/t3419-rebase-patch-id.sh | 19 +++++++++++++------\n 2 files changed, 27 insertions(+), 24 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex dd68281ba44..70bc1902e11 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6248,30 +6248,26 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid\n \t\t\tthe_hash_algo->update_fn(&ctx, p->two->path, len2);\n \t\t}\n \n-\t\tif (diff_header_only)\n-\t\t\tcontinue;\n-\n-\t\tif (fill_mmfile(options->repo, &mf1, p->one) < 0 ||\n-\t\t fill_mmfile(options->repo, &mf2, p->two) < 0)\n-\t\t\treturn error(\"unable to read files to diff\");\n-\n-\t\tif (diff_filespec_is_binary(options->repo, p->one) ||\n+\t\tif (diff_header_only) {\n+\t\t\t/* don't do anything since we're only populating header info */\n+\t\t} else if (diff_filespec_is_binary(options->repo, p->one) ||\n \t\t diff_filespec_is_binary(options->repo, p->two)) {\n \t\t\tthe_hash_algo->update_fn(&ctx, oid_to_hex(&p->one->oid),\n \t\t\t\t\tthe_hash_algo->hexsz);\n \t\t\tthe_hash_algo->update_fn(&ctx, oid_to_hex(&p->two->oid),\n \t\t\t\t\tthe_hash_algo->hexsz);\n-\t\t\tcontinue;\n+\t\t} else {\n+\t\t\tif (fill_mmfile(options->repo, &mf1, p->one) < 0 ||\n+\t\t\t fill_mmfile(options->repo, &mf2, p->two) < 0)\n+\t\t\t\treturn error(\"unable to read files to diff\");\n+\t\t\txpp.flags = 0;\n+\t\t\txecfg.ctxlen = 3;\n+\t\t\txecfg.flags = XDL_EMIT_NO_HUNK_HDR;\n+\t\t\tif (xdi_diff_outf(&mf1, &mf2, NULL,\n+\t\t\t\t\t patch_id_consume, &data, &xpp, &xecfg))\n+\t\t\t\treturn error(\"unable to generate patch-id diff for %s\",\n+\t\t\t\t\t p->one->path);\n \t\t}\n-\n-\t\txpp.flags = 0;\n-\t\txecfg.ctxlen = 3;\n-\t\txecfg.flags = XDL_EMIT_NO_HUNK_HDR;\n-\t\tif (xdi_diff_outf(&mf1, &mf2, NULL,\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}\ndiff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh\nindex 295040f2fe3..f7b7e9e5b7c 100755\n--- a/t/t3419-rebase-patch-id.sh\n+++ b/t/t3419-rebase-patch-id.sh\n@@ -46,10 +46,6 @@ test_expect_success 'setup: 500 lines' '\n \tgit cherry-pick main >/dev/null 2>&1\n '\n \n-test_expect_success 'setup attributes' '\n-\techo \"file binary\" >.gitattributes\n-'\n-\n test_expect_success 'detect upstream patch' '\n \tgit checkout -q main &&\n \tscramble file &&\n@@ -58,7 +54,13 @@ test_expect_success 'detect upstream patch' '\n \tgit checkout -q other^{} &&\n \tgit rebase main &&\n \tgit rev-list main...HEAD~ >revs &&\n-\ttest_must_be_empty revs\n+\ttest_must_be_empty revs &&\n+\techo \"file binary\" >.gitattributes &&\n+\tgit checkout -q other^{} &&\n+\tgit rebase main &&\n+\tgit rev-list main...HEAD~ >revs &&\n+\ttest_must_be_empty revs &&\n+\trm .gitattributes\n '\n \n test_expect_success 'do not drop patch' '\n@@ -68,7 +70,12 @@ test_expect_success 'do not drop patch' '\n \tgit commit -q -m squashed &&\n \tgit checkout -q other^{} &&\n \ttest_must_fail git rebase squashed &&\n-\tgit rebase --quit\n+\tgit rebase --abort &&\n+\techo \"file binary\" >.gitattributes &&\n+\tgit checkout -q other^{} &&\n+\ttest_must_fail git rebase squashed &&\n+\tgit rebase --abort &&\n+\trm .gitattributes\n '\n \n test_done\n-- \ngitgitgadget\n\n"},{"id":"463281","messageId":"30ec43cd129b626373f1886592f0040027586da6.1663654859.git.gitgitgadget@gmail.com","threadId":"58470","inReplyTo":"pull.1359.v2.git.1663654859.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] patch-id: use stable patch-id for rebases","fromName":"Jerry Zhang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-20T06:20:59Z","receivedAt":"2022-09-20T06:21:11Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"From: Jerry Zhang \n\nGit doesn't persist patch-ids during the rebase process, so there is\nno need to specifically invoke the unstable variant.\n\nThis allows the legacy unstable id logic to be cleaned up.\n\nSigned-off-by: Jerry Zhang \n---\n builtin/log.c | 2 +-\n diff.c | 12 ++++--------\n diff.h | 2 +-\n patch-ids.c | 10 +++++-----\n patch-ids.h | 2 +-\n 5 files changed, 12 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 047f9e5278d..3bb49fd7406 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1762,7 +1762,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, 1))\n+\t\tif (commit_patch_id(commit, &diffopt, &oid, 0))\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/diff.c b/diff.c\nindex 70bc1902e11..f00522d9354 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6185,7 +6185,7 @@ static void patch_id_add_mode(git_hash_ctx *ctx, unsigned mode)\n }\n \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+static int diff_get_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only)\n {\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n \tint i;\n@@ -6268,21 +6268,17 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid\n \t\t\t\treturn error(\"unable to generate patch-id diff for %s\",\n \t\t\t\t\t p->one->path);\n \t\t}\n-\t\tif (stable)\n-\t\t\tflush_one_hunk(oid, &ctx);\n+\t\tflush_one_hunk(oid, &ctx);\n \t}\n \n-\tif (!stable)\n-\t\tthe_hash_algo->final_oid_fn(oid, &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, int stable)\n+int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only)\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, stable);\n+\tint result = diff_get_patch_id(options, oid, diff_header_only);\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 8ae18e5ab1e..fd33caeb25d 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -634,7 +634,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option);\n int run_diff_index(struct rev_info *revs, unsigned int option);\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, int);\n+int diff_flush_patch_id(struct diff_options *, struct object_id *, int);\n void flush_one_hunk(struct object_id *result, git_hash_ctx *ctx);\n \n int diff_result_code(struct diff_options *, int);\ndiff --git a/patch-ids.c b/patch-ids.c\nindex 8bf425555de..fefddc487e9 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, int stable)\n+\t\t struct object_id *oid, int diff_header_only)\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, stable);\n+\treturn diff_flush_patch_id(options, oid, diff_header_only);\n }\n \n /*\n@@ -48,11 +48,11 @@ static int patch_id_neq(const void *cmpfn_data,\n \tb = container_of(entry_or_key, struct patch_id, ent);\n \n \tif (is_null_oid(&a->patch_id) &&\n-\t commit_patch_id(a->commit, opt, &a->patch_id, 0, 0))\n+\t commit_patch_id(a->commit, opt, &a->patch_id, 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, 0))\n+\t commit_patch_id(b->commit, opt, &b->patch_id, 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@@ -82,7 +82,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, 0))\n+\tif (commit_patch_id(commit, &ids->diffopts, &header_only_patch_id, 1))\n \t\treturn -1;\n \n \thashmap_entry_init(&patch->ent, oidhash(&header_only_patch_id));\ndiff --git a/patch-ids.h b/patch-ids.h\nindex ab6c6a68047..490d7393716 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, int);\n+\t\t struct object_id *oid, int);\n int init_patch_ids(struct repository *, struct patch_ids *);\n int free_patch_ids(struct patch_ids *);\n \n-- \ngitgitgadget\n"},{"id":"463386","messageId":"xmqqo7v81qsn.fsf@gitster.g","threadId":"58470","inReplyTo":"pull.1359.git.1663653505.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] update internal patch-id to use \"stable\" algorithm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-21T19:16:40Z","receivedAt":"2022-09-21T19:16:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Jerry Zhang via GitGitGadget\" writes:\n\n> Internal usage of patch-id in rebase / cherry-pick doesn't persist\n> patch-ids, so there's no need to specifically invoke the unstable variant.\n>\n> This allows the unstable logic to be cleaned up.\n\nWhile all of that may be true, two things are not explained. \n\n * Why does \"unstable\" need to be \"cleaned up\"? Is that too dirty\n in what way?\n\n * If internal usage does not persist patch-ids generated by the\n machinery, why is it bad to be using the unstable variant? A\n naïve expectation would be to make sure you use stable one if you\n want a future recomputation to give you the same result, but the\n opposite does not have to be always true.\n\n"},{"id":"463390","messageId":"CAMKO5CtS5c-Cd518hzoLszzX5bPDws-bByEcqfVpn7iJTvvOUw@mail.gmail.com","threadId":"58470","inReplyTo":"xmqqo7v81qsn.fsf@gitster.g","subject":"Re: [PATCH 0/2] update internal patch-id to use \"stable\" algorithm","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2022-09-21T20:59:26Z","receivedAt":"2022-09-21T20:59:55Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"On Wed, Sep 21, 2022 at 12:16 PM Junio C Hamano wrote:\n>\n> \"Jerry Zhang via GitGitGadget\" writes:\n>\n> > Internal usage of patch-id in rebase / cherry-pick doesn't persist\n> > patch-ids, so there's no need to specifically invoke the unstable variant.\n> >\n> > This allows the unstable logic to be cleaned up.\n>\n> While all of that may be true, two things are not explained.\n>\n> * Why does \"unstable\" need to be \"cleaned up\"? Is that too dirty\n> in what way?\n>\n> * If internal usage does not persist patch-ids generated by the\n> machinery, why is it bad to be using the unstable variant? A\n> naïve expectation would be to make sure you use stable one if you\n> want a future recomputation to give you the same result, but the\n> opposite does not have to be always true.\n>\nFair questions. My broad view is that less code and fewer code paths\nis better for readability and testing. This isn't a massive impact but\nit's not theoretical either -- as seen in patch 1 in this series I\ncaught a bug in stable + binary files because of this change.\nPreviously stable patch ids were only used in \"git format-patch\" and\nso this corner case was missed, but this becomes less likely if rebase\n+ cherry-pick + format-patch were all on the same scheme.\n"},{"id":"464918","messageId":"pull.1359.v3.git.1665737804.gitgitgadget@gmail.com","threadId":"58470","inReplyTo":"pull.1359.v2.git.1663654859.gitgitgadget@gmail.com","subject":"[PATCH v3 0/7] patch-id fixes and improvements","fromName":"Jerry Zhang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-14T08:56:37Z","receivedAt":"2022-10-14T08:56:51Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"These patches add fixes and features to the \"git patch-id\" command, mostly\ndiscovered through our usage of patch-id in the revup project\n(https://github.com/Skydio/revup). On top of that I've tried to make general\ncleanup changes where I can.\n\nSummary:\n\n1: Fixed a bug in the combination of --stable with binary files and\nheader-only, and expanded the test to cover both binary and non-binary\nfiles.\n\n2: Switch internal usage of patch-id in rebase / cherry-pick to use the\nstable variant to reduce the number of code paths and improve testing for\nbugs like above.\n\n3: Fixed bugs with patch-id and binary diffs. Previously patch-id did not\nbehave correctly for binary diffs regardless of whether \"--binary\" was given\nto \"diff\".\n\n4: Fixed bugs with patch-id and mode changes. Previously mode changes were\nincorrectly excluded from the patch-id.\n\n5: Add a new \"--include-whitespace\" mode to patch-id that prevents\nwhitespace from being stripped during id calculation. Also add a config\noption for the same behavior.\n\n6: Remove unused prefix from patch-id logic.\n\n7: Update format-patch doc to specify when patch-ids are going to be equal\nto those generated by \"git patch-id\".\n\nV1->V2: Fixed comment style V2->V3: The ---/+++ lines no longer get added to\nthe patch-id of binary diffs. Also added patches 3-7 in the series.\n\nSigned-off-by: Jerry Zhang jerry@skydio.com\n\nJerry Zhang (7):\n patch-id: fix stable patch id for binary / header-only\n patch-id: use stable patch-id for rebases\n builtin: patch-id: fix patch-id with binary diffs\n patch-id: fix patch-id for mode changes\n builtin: patch-id: add --include-whitespace as a command mode\n builtin: patch-id: remove unused diff-tree prefix\n documentation: format-patch: clarify requirements for patch-ids to\n match\n\n Documentation/git-format-patch.txt | 4 +-\n Documentation/git-patch-id.txt | 25 +++++--\n builtin/log.c | 2 +-\n builtin/patch-id.c | 114 +++++++++++++++++++++--------\n diff.c | 75 +++++++++----------\n diff.h | 2 +-\n patch-ids.c | 10 +--\n patch-ids.h | 2 +-\n t/t3419-rebase-patch-id.sh | 63 +++++++++++++---\n t/t4204-patch-id.sh | 95 ++++++++++++++++++++++--\n 10 files changed, 291 insertions(+), 101 deletions(-)\n\n\nbase-commit: d420dda0576340909c3faff364cfbd1485f70376\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1359%2Fjerry-skydio%2Fjerry%2Frevup%2Fmaster%2Fpatch_ids-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1359/jerry-skydio/jerry/revup/master/patch_ids-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1359\n\nRange-diff vs v2:\n\n 1: 945508df7b6 ! 1: 7d4c2e91ce0 patch-id: fix stable patch id for binary / header-only\n @@ Commit message\n populates the object ids, and normal which populates the text\n diff. All branches will end up flushing the hunk.\n \n + Don't populate the ---a/ and +++b/ lines for binary diffs, to correspond\n + to those lines not being present in the \"git diff\" text output.\n + This is necessary because we advertise that the patch-id calculated\n + internally and used in format-patch is the same that what the\n + builtin \"git patch-id\" would produce when piped from a diff.\n +\n Update the test to run on both binary and normal files.\n \n Signed-off-by: Jerry Zhang \n \n ## diff.c ##\n @@ diff.c: static int diff_get_patch_id(struct diff_options *options, struct object_id *oid\n - \t\t\tthe_hash_algo->update_fn(&ctx, p->two->path, len2);\n + \t\tif (p->one->mode == 0) {\n + \t\t\tpatch_id_add_string(&ctx, \"newfilemode\");\n + \t\t\tpatch_id_add_mode(&ctx, p->two->mode);\n +-\t\t\tpatch_id_add_string(&ctx, \"---/dev/null\");\n +-\t\t\tpatch_id_add_string(&ctx, \"+++b/\");\n +-\t\t\tthe_hash_algo->update_fn(&ctx, p->two->path, len2);\n + \t\t} else if (p->two->mode == 0) {\n + \t\t\tpatch_id_add_string(&ctx, \"deletedfilemode\");\n + \t\t\tpatch_id_add_mode(&ctx, p->one->mode);\n +-\t\t\tpatch_id_add_string(&ctx, \"---a/\");\n +-\t\t\tthe_hash_algo->update_fn(&ctx, p->one->path, len1);\n +-\t\t\tpatch_id_add_string(&ctx, \"+++/dev/null\");\n +-\t\t} else {\n +-\t\t\tpatch_id_add_string(&ctx, \"---a/\");\n +-\t\t\tthe_hash_algo->update_fn(&ctx, p->one->path, len1);\n +-\t\t\tpatch_id_add_string(&ctx, \"+++b/\");\n +-\t\t\tthe_hash_algo->update_fn(&ctx, p->two->path, len2);\n \t\t}\n \n -\t\tif (diff_header_only)\n @@ diff.c: static int diff_get_patch_id(struct diff_options *options, struct object\n \t\t\tthe_hash_algo->update_fn(&ctx, oid_to_hex(&p->two->oid),\n \t\t\t\t\tthe_hash_algo->hexsz);\n -\t\t\tcontinue;\n +-\t\t}\n +-\n +-\t\txpp.flags = 0;\n +-\t\txecfg.ctxlen = 3;\n +-\t\txecfg.flags = XDL_EMIT_NO_HUNK_HDR;\n +-\t\tif (xdi_diff_outf(&mf1, &mf2, NULL,\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 +\t\t} else {\n ++\t\t\tif (p->one->mode == 0) {\n ++\t\t\t\tpatch_id_add_string(&ctx, \"---/dev/null\");\n ++\t\t\t\tpatch_id_add_string(&ctx, \"+++b/\");\n ++\t\t\t\tthe_hash_algo->update_fn(&ctx, p->two->path, len2);\n ++\t\t\t} else if (p->two->mode == 0) {\n ++\t\t\t\tpatch_id_add_string(&ctx, \"---a/\");\n ++\t\t\t\tthe_hash_algo->update_fn(&ctx, p->one->path, len1);\n ++\t\t\t\tpatch_id_add_string(&ctx, \"+++/dev/null\");\n ++\t\t\t} else {\n ++\t\t\t\tpatch_id_add_string(&ctx, \"---a/\");\n ++\t\t\t\tthe_hash_algo->update_fn(&ctx, p->one->path, len1);\n ++\t\t\t\tpatch_id_add_string(&ctx, \"+++b/\");\n ++\t\t\t\tthe_hash_algo->update_fn(&ctx, p->two->path, len2);\n ++\t\t\t}\n + \n +\t\t\tif (fill_mmfile(options->repo, &mf1, p->one) < 0 ||\n +\t\t\t fill_mmfile(options->repo, &mf2, p->two) < 0)\n +\t\t\t\treturn error(\"unable to read files to diff\");\n @@ diff.c: static int diff_get_patch_id(struct diff_options *options, struct object\n +\t\t\t\t\t patch_id_consume, &data, &xpp, &xecfg))\n +\t\t\t\treturn error(\"unable to generate patch-id diff for %s\",\n +\t\t\t\t\t p->one->path);\n - \t\t}\n --\n --\t\txpp.flags = 0;\n --\t\txecfg.ctxlen = 3;\n --\t\txecfg.flags = XDL_EMIT_NO_HUNK_HDR;\n --\t\tif (xdi_diff_outf(&mf1, &mf2, NULL,\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\t}\n \t\tif (stable)\n \t\t\tflush_one_hunk(oid, &ctx);\n \t}\n \n ## t/t3419-rebase-patch-id.sh ##\n @@ t/t3419-rebase-patch-id.sh: test_expect_success 'setup: 500 lines' '\n - \tgit cherry-pick main >/dev/null 2>&1\n - '\n + \tgit add newfile &&\n + \tgit commit -q -m \"add small file\" &&\n + \n +-\tgit cherry-pick main >/dev/null 2>&1\n +-'\n ++\tgit cherry-pick main >/dev/null 2>&1 &&\n \n -test_expect_success 'setup attributes' '\n -\techo \"file binary\" >.gitattributes\n --'\n --\n ++\tgit branch -f squashed main &&\n ++\tgit checkout -q -f squashed &&\n ++\tgit reset -q --soft HEAD~2 &&\n ++\tgit commit -q -m squashed\n + '\n + \n test_expect_success 'detect upstream patch' '\n - \tgit checkout -q main &&\n +-\tgit checkout -q main &&\n ++\tgit checkout -q main^{} &&\n \tscramble file &&\n + \tgit add file &&\n + \tgit commit -q -m \"change big file again\" &&\n @@ t/t3419-rebase-patch-id.sh: test_expect_success 'detect upstream patch' '\n - \tgit checkout -q other^{} &&\n - \tgit rebase main &&\n - \tgit rev-list main...HEAD~ >revs &&\n --\ttest_must_be_empty revs\n -+\ttest_must_be_empty revs &&\n + \ttest_must_be_empty revs\n + '\n + \n ++test_expect_success 'detect upstream patch binary' '\n +\techo \"file binary\" >.gitattributes &&\n +\tgit checkout -q other^{} &&\n +\tgit rebase main &&\n +\tgit rev-list main...HEAD~ >revs &&\n +\ttest_must_be_empty revs &&\n -+\trm .gitattributes\n - '\n - \n ++\ttest_when_finished \"rm .gitattributes\"\n ++'\n ++\n test_expect_success 'do not drop patch' '\n -@@ t/t3419-rebase-patch-id.sh: test_expect_success 'do not drop patch' '\n - \tgit commit -q -m squashed &&\n +-\tgit branch -f squashed main &&\n +-\tgit checkout -q -f squashed &&\n +-\tgit reset -q --soft HEAD~2 &&\n +-\tgit commit -q -m squashed &&\n \tgit checkout -q other^{} &&\n \ttest_must_fail git rebase squashed &&\n -\tgit rebase --quit\n -+\tgit rebase --abort &&\n ++\ttest_when_finished \"git rebase --abort\"\n ++'\n ++\n ++test_expect_success 'do not drop patch binary' '\n +\techo \"file binary\" >.gitattributes &&\n +\tgit checkout -q other^{} &&\n +\ttest_must_fail git rebase squashed &&\n -+\tgit rebase --abort &&\n -+\trm .gitattributes\n ++\ttest_when_finished \"git rebase --abort\" &&\n ++\ttest_when_finished \"rm .gitattributes\"\n '\n \n test_done\n 2: 30ec43cd129 ! 2: 25e28b7dab3 patch-id: use stable patch-id for rebases\n @@ Commit message\n patch-id: use stable patch-id for rebases\n \n Git doesn't persist patch-ids during the rebase process, so there is\n - no need to specifically invoke the unstable variant.\n -\n - This allows the legacy unstable id logic to be cleaned up.\n + no need to specifically invoke the unstable variant. Use the stable\n + logic for all internal patch-id calculations to minimize the number of\n + code paths and improve test coverage.\n \n Signed-off-by: Jerry Zhang \n \n -: ----------- > 3: 21642128927 builtin: patch-id: fix patch-id with binary diffs\n -: ----------- > 4: 6e07cfd5691 patch-id: fix patch-id for mode changes\n -: ----------- > 5: bbaa2425ad0 builtin: patch-id: add --include-whitespace as a command mode\n -: ----------- > 6: a1f6f36d487 builtin: patch-id: remove unused diff-tree prefix\n -: ----------- > 7: 69440797f30 documentation: format-patch: clarify requirements for patch-ids to match\n\n-- \ngitgitgadget\n"},{"id":"464919","messageId":"7d4c2e91ce0c71610840168a157146f980b86497.1665737804.git.gitgitgadget@gmail.com","threadId":"58470","inReplyTo":"pull.1359.v3.git.1665737804.gitgitgadget@gmail.com","subject":"[PATCH v3 1/7] patch-id: fix stable patch id for binary / header-only","fromName":"Jerry Zhang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-14T08:56:38Z","receivedAt":"2022-10-14T08:56:55Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"From: Jerry Zhang \n\nPrevious logic here skipped flushing the hunks for binary\nand header-only patch ids, which would always result in a\npatch-id of 0000.\n\nReorder the logic to branch into 3 cases for populating the\npatch body: header-only which populates nothing, binary which\npopulates the object ids, and normal which populates the text\ndiff. All branches will end up flushing the hunk.\n\nDon't populate the ---a/ and +++b/ lines for binary diffs, to correspond\nto those lines not being present in the \"git diff\" text output.\nThis is necessary because we advertise that the patch-id calculated\ninternally and used in format-patch is the same that what the\nbuiltin \"git patch-id\" would produce when piped from a diff.\n\nUpdate the test to run on both binary and normal files.\n\nSigned-off-by: Jerry Zhang \n---\n diff.c | 58 +++++++++++++++++++-------------------\n t/t3419-rebase-patch-id.sh | 34 +++++++++++++++-------\n 2 files changed, 53 insertions(+), 39 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 648f6717a55..c15169e4b06 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6253,46 +6253,46 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid\n \t\tif (p->one->mode == 0) {\n \t\t\tpatch_id_add_string(&ctx, \"newfilemode\");\n \t\t\tpatch_id_add_mode(&ctx, p->two->mode);\n-\t\t\tpatch_id_add_string(&ctx, \"---/dev/null\");\n-\t\t\tpatch_id_add_string(&ctx, \"+++b/\");\n-\t\t\tthe_hash_algo->update_fn(&ctx, p->two->path, len2);\n \t\t} else if (p->two->mode == 0) {\n \t\t\tpatch_id_add_string(&ctx, \"deletedfilemode\");\n \t\t\tpatch_id_add_mode(&ctx, p->one->mode);\n-\t\t\tpatch_id_add_string(&ctx, \"---a/\");\n-\t\t\tthe_hash_algo->update_fn(&ctx, p->one->path, len1);\n-\t\t\tpatch_id_add_string(&ctx, \"+++/dev/null\");\n-\t\t} else {\n-\t\t\tpatch_id_add_string(&ctx, \"---a/\");\n-\t\t\tthe_hash_algo->update_fn(&ctx, p->one->path, len1);\n-\t\t\tpatch_id_add_string(&ctx, \"+++b/\");\n-\t\t\tthe_hash_algo->update_fn(&ctx, p->two->path, len2);\n \t\t}\n \n-\t\tif (diff_header_only)\n-\t\t\tcontinue;\n-\n-\t\tif (fill_mmfile(options->repo, &mf1, p->one) < 0 ||\n-\t\t fill_mmfile(options->repo, &mf2, p->two) < 0)\n-\t\t\treturn error(\"unable to read files to diff\");\n-\n-\t\tif (diff_filespec_is_binary(options->repo, p->one) ||\n+\t\tif (diff_header_only) {\n+\t\t\t/* don't do anything since we're only populating header info */\n+\t\t} else if (diff_filespec_is_binary(options->repo, p->one) ||\n \t\t diff_filespec_is_binary(options->repo, p->two)) {\n \t\t\tthe_hash_algo->update_fn(&ctx, oid_to_hex(&p->one->oid),\n \t\t\t\t\tthe_hash_algo->hexsz);\n \t\t\tthe_hash_algo->update_fn(&ctx, oid_to_hex(&p->two->oid),\n \t\t\t\t\tthe_hash_algo->hexsz);\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\txpp.flags = 0;\n-\t\txecfg.ctxlen = 3;\n-\t\txecfg.flags = XDL_EMIT_NO_HUNK_HDR;\n-\t\tif (xdi_diff_outf(&mf1, &mf2, NULL,\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+\t\t} else {\n+\t\t\tif (p->one->mode == 0) {\n+\t\t\t\tpatch_id_add_string(&ctx, \"---/dev/null\");\n+\t\t\t\tpatch_id_add_string(&ctx, \"+++b/\");\n+\t\t\t\tthe_hash_algo->update_fn(&ctx, p->two->path, len2);\n+\t\t\t} else if (p->two->mode == 0) {\n+\t\t\t\tpatch_id_add_string(&ctx, \"---a/\");\n+\t\t\t\tthe_hash_algo->update_fn(&ctx, p->one->path, len1);\n+\t\t\t\tpatch_id_add_string(&ctx, \"+++/dev/null\");\n+\t\t\t} else {\n+\t\t\t\tpatch_id_add_string(&ctx, \"---a/\");\n+\t\t\t\tthe_hash_algo->update_fn(&ctx, p->one->path, len1);\n+\t\t\t\tpatch_id_add_string(&ctx, \"+++b/\");\n+\t\t\t\tthe_hash_algo->update_fn(&ctx, p->two->path, len2);\n+\t\t\t}\n \n+\t\t\tif (fill_mmfile(options->repo, &mf1, p->one) < 0 ||\n+\t\t\t fill_mmfile(options->repo, &mf2, p->two) < 0)\n+\t\t\t\treturn error(\"unable to read files to diff\");\n+\t\t\txpp.flags = 0;\n+\t\t\txecfg.ctxlen = 3;\n+\t\t\txecfg.flags = XDL_EMIT_NO_HUNK_HDR;\n+\t\t\tif (xdi_diff_outf(&mf1, &mf2, NULL,\n+\t\t\t\t\t patch_id_consume, &data, &xpp, &xecfg))\n+\t\t\t\treturn error(\"unable to generate patch-id diff for %s\",\n+\t\t\t\t\t p->one->path);\n+\t\t}\n \t\tif (stable)\n \t\t\tflush_one_hunk(oid, &ctx);\n \t}\ndiff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh\nindex 295040f2fe3..d24e55aac8d 100755\n--- a/t/t3419-rebase-patch-id.sh\n+++ b/t/t3419-rebase-patch-id.sh\n@@ -43,15 +43,16 @@ test_expect_success 'setup: 500 lines' '\n \tgit add newfile &&\n \tgit commit -q -m \"add small file\" &&\n \n-\tgit cherry-pick main >/dev/null 2>&1\n-'\n+\tgit cherry-pick main >/dev/null 2>&1 &&\n \n-test_expect_success 'setup attributes' '\n-\techo \"file binary\" >.gitattributes\n+\tgit branch -f squashed main &&\n+\tgit checkout -q -f squashed &&\n+\tgit reset -q --soft HEAD~2 &&\n+\tgit commit -q -m squashed\n '\n \n test_expect_success 'detect upstream patch' '\n-\tgit checkout -q main &&\n+\tgit checkout -q main^{} &&\n \tscramble file &&\n \tgit add file &&\n \tgit commit -q -m \"change big file again\" &&\n@@ -61,14 +62,27 @@ test_expect_success 'detect upstream patch' '\n \ttest_must_be_empty revs\n '\n \n+test_expect_success 'detect upstream patch binary' '\n+\techo \"file binary\" >.gitattributes &&\n+\tgit checkout -q other^{} &&\n+\tgit rebase main &&\n+\tgit rev-list main...HEAD~ >revs &&\n+\ttest_must_be_empty revs &&\n+\ttest_when_finished \"rm .gitattributes\"\n+'\n+\n test_expect_success 'do not drop patch' '\n-\tgit branch -f squashed main &&\n-\tgit checkout -q -f squashed &&\n-\tgit reset -q --soft HEAD~2 &&\n-\tgit commit -q -m squashed &&\n \tgit checkout -q other^{} &&\n \ttest_must_fail git rebase squashed &&\n-\tgit rebase --quit\n+\ttest_when_finished \"git rebase --abort\"\n+'\n+\n+test_expect_success 'do not drop patch binary' '\n+\techo \"file binary\" >.gitattributes &&\n+\tgit checkout -q other^{} &&\n+\ttest_must_fail git rebase squashed &&\n+\ttest_when_finished \"git rebase --abort\" &&\n+\ttest_when_finished \"rm .gitattributes\"\n '\n \n test_done\n-- \ngitgitgadget\n\n"},{"id":"464920","messageId":"25e28b7dab3f89039667c5317090510754b80964.1665737804.git.gitgitgadget@gmail.com","threadId":"58470","inReplyTo":"pull.1359.v3.git.1665737804.gitgitgadget@gmail.com","subject":"[PATCH v3 2/7] patch-id: use stable patch-id for rebases","fromName":"Jerry Zhang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-14T08:56:39Z","receivedAt":"2022-10-14T08:57:01Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"From: Jerry Zhang \n\nGit doesn't persist patch-ids during the rebase process, so there is\nno need to specifically invoke the unstable variant. Use the stable\nlogic for all internal patch-id calculations to minimize the number of\ncode paths and improve test coverage.\n\nSigned-off-by: Jerry Zhang \n---\n builtin/log.c | 2 +-\n diff.c | 12 ++++--------\n diff.h | 2 +-\n patch-ids.c | 10 +++++-----\n patch-ids.h | 2 +-\n 5 files changed, 12 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex ee19dc5d450..e72869afb36 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1763,7 +1763,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, 1))\n+\t\tif (commit_patch_id(commit, &diffopt, &oid, 0))\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/diff.c b/diff.c\nindex c15169e4b06..199b63dbcc3 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6206,7 +6206,7 @@ static void patch_id_add_mode(git_hash_ctx *ctx, unsigned mode)\n }\n \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+static int diff_get_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only)\n {\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n \tint i;\n@@ -6293,21 +6293,17 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid\n \t\t\t\treturn error(\"unable to generate patch-id diff for %s\",\n \t\t\t\t\t p->one->path);\n \t\t}\n-\t\tif (stable)\n-\t\t\tflush_one_hunk(oid, &ctx);\n+\t\tflush_one_hunk(oid, &ctx);\n \t}\n \n-\tif (!stable)\n-\t\tthe_hash_algo->final_oid_fn(oid, &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, int stable)\n+int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only)\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, stable);\n+\tint result = diff_get_patch_id(options, oid, diff_header_only);\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 8ae18e5ab1e..fd33caeb25d 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -634,7 +634,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option);\n int run_diff_index(struct rev_info *revs, unsigned int option);\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, int);\n+int diff_flush_patch_id(struct diff_options *, struct object_id *, int);\n void flush_one_hunk(struct object_id *result, git_hash_ctx *ctx);\n \n int diff_result_code(struct diff_options *, int);\ndiff --git a/patch-ids.c b/patch-ids.c\nindex 46c6a8f3eab..31534466266 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, int stable)\n+\t\t struct object_id *oid, int diff_header_only)\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, stable);\n+\treturn diff_flush_patch_id(options, oid, diff_header_only);\n }\n \n /*\n@@ -48,11 +48,11 @@ static int patch_id_neq(const void *cmpfn_data,\n \tb = container_of(entry_or_key, struct patch_id, ent);\n \n \tif (is_null_oid(&a->patch_id) &&\n-\t commit_patch_id(a->commit, opt, &a->patch_id, 0, 0))\n+\t commit_patch_id(a->commit, opt, &a->patch_id, 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, 0))\n+\t commit_patch_id(b->commit, opt, &b->patch_id, 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@@ -82,7 +82,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, 0))\n+\tif (commit_patch_id(commit, &ids->diffopts, &header_only_patch_id, 1))\n \t\treturn -1;\n \n \thashmap_entry_init(&patch->ent, oidhash(&header_only_patch_id));\ndiff --git a/patch-ids.h b/patch-ids.h\nindex ab6c6a68047..490d7393716 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, int);\n+\t\t struct object_id *oid, int);\n int init_patch_ids(struct repository *, struct patch_ids *);\n int free_patch_ids(struct patch_ids *);\n \n-- \ngitgitgadget\n\n"},{"id":"464921","messageId":"2164212892712930cb34223499bb3e03bf2c2392.1665737804.git.gitgitgadget@gmail.com","threadId":"58470","inReplyTo":"pull.1359.v3.git.1665737804.gitgitgadget@gmail.com","subject":"[PATCH v3 3/7] builtin: patch-id: fix patch-id with binary diffs","fromName":"Jerry Zhang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-14T08:56:40Z","receivedAt":"2022-10-14T08:57:02Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"From: Jerry Zhang \n\n\"git patch-id\" currently doesn't produce correct output if the\nincoming diff has any binary files. Add logic to\nget_one_patchid to handle the different possible styles of binary\ndiff. This attempts to keep resulting patch-ids identical to what\nwould be produced by the counterpart logic in diff.c, that is it\nproduces the id by hashing the a and b oids in succession.\n\nIn general we handle binary diffs by first caching the object ids from\nthe \"index\" line and using those if we then find an indication\nthat the diff is binary.\n\nThe input could contain patches generated with \"git diff --binary\". This\ncurrently breaks the parse logic and results in multiple patch-ids\noutput for a single commit. Here we have to skip the contents of the\npatch itself since those do not go into the patch id. --binary\nimplies --full-index so the object ids are always available.\n\nWhen the diff is generated with --full-index there is no patch content\nto skip over.\n\nWhen a diff is generated without --full-index or --binary, it will\ncontain abbreviated object ids. This will still result in a sufficiently\nunique patch-id when hashed, but does not match internal patch id\noutput. We'll call this ok for now as we already need specialized\narguments to diff in order to match internal patch id (namely -U3).\n\nSigned-off-by: Jerry Zhang \n---\n builtin/patch-id.c | 36 ++++++++++++++++++++++++++++++++++--\n t/t4204-patch-id.sh | 29 ++++++++++++++++++++++++++++-\n 2 files changed, 62 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex 881fcf32732..e7a31123142 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -61,6 +61,8 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n {\n \tint patchlen = 0, found_next = 0;\n \tint before = -1, after = -1;\n+\tint diff_is_binary = 0;\n+\tchar pre_oid_str[GIT_MAX_HEXSZ + 1], post_oid_str[GIT_MAX_HEXSZ + 1];\n \tgit_hash_ctx ctx;\n \n \tthe_hash_algo->init_fn(&ctx);\n@@ -88,14 +90,44 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \n \t\t/* Parsing diff header? */\n \t\tif (before == -1) {\n-\t\t\tif (starts_with(line, \"index \"))\n+\t\t\tif (starts_with(line, \"GIT binary patch\") ||\n+\t\t\t starts_with(line, \"Binary files\")) {\n+\t\t\t\tdiff_is_binary = 1;\n+\t\t\t\tbefore = 0;\n+\t\t\t\tthe_hash_algo->update_fn(&ctx, pre_oid_str,\n+\t\t\t\t\t\t\t strlen(pre_oid_str));\n+\t\t\t\tthe_hash_algo->update_fn(&ctx, post_oid_str,\n+\t\t\t\t\t\t\t strlen(post_oid_str));\n+\t\t\t\tif (stable)\n+\t\t\t\t\tflush_one_hunk(result, &ctx);\n \t\t\t\tcontinue;\n-\t\t\telse if (starts_with(line, \"--- \"))\n+\t\t\t} else if (skip_prefix(line, \"index \", &p)) {\n+\t\t\t\tchar *oid1_end = strstr(line, \"..\");\n+\t\t\t\tchar *oid2_end = NULL;\n+\t\t\t\tif (oid1_end)\n+\t\t\t\t\toid2_end = strstr(oid1_end, \" \");\n+\t\t\t\tif (!oid2_end)\n+\t\t\t\t\toid2_end = line + strlen(line) - 1;\n+\t\t\t\tif (oid1_end != NULL && oid2_end != NULL) {\n+\t\t\t\t\t*oid1_end = *oid2_end = '\\0';\n+\t\t\t\t\tstrlcpy(pre_oid_str, p, GIT_MAX_HEXSZ + 1);\n+\t\t\t\t\tstrlcpy(post_oid_str, oid1_end + 2, GIT_MAX_HEXSZ + 1);\n+\t\t\t\t}\n+\t\t\t\tcontinue;\n+\t\t\t} else if (starts_with(line, \"--- \"))\n \t\t\t\tbefore = after = 1;\n \t\t\telse if (!isalpha(line[0]))\n \t\t\t\tbreak;\n \t\t}\n \n+\t\tif (diff_is_binary) {\n+\t\t\tif (starts_with(line, \"diff \")) {\n+\t\t\t\tdiff_is_binary = 0;\n+\t\t\t\tbefore = -1;\n+\t\t\t}\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\t/* Looking for a valid hunk header? */\n \t\tif (before == 0 && after == 0) {\n \t\t\tif (starts_with(line, \"@@ -\")) {\ndiff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\nindex a730c0db985..cdc5191aa8d 100755\n--- a/t/t4204-patch-id.sh\n+++ b/t/t4204-patch-id.sh\n@@ -42,7 +42,7 @@ calc_patch_id () {\n }\n \n get_top_diff () {\n-\tgit log -p -1 \"$@\" -O bar-then-foo --\n+\tgit log -p -1 \"$@\" -O bar-then-foo --full-index --\n }\n \n get_patch_id () {\n@@ -61,6 +61,33 @@ test_expect_success 'patch-id detects inequality' '\n \tget_patch_id notsame &&\n \t! test_cmp patch-id_main patch-id_notsame\n '\n+test_expect_success 'patch-id detects equality binary' '\n+\tcat >.gitattributes <<-\\EOF &&\n+\tfoo binary\n+\tbar binary\n+\tEOF\n+\tget_patch_id main &&\n+\tget_patch_id same &&\n+\tgit log -p -1 --binary main >top-diff.output &&\n+\tcalc_patch_id top-diff.output &&\n+\tcalc_patch_id .gitattributes <<-\\EOF &&\n+\tfoo binary\n+\tbar binary\n+\tEOF\n+\tget_patch_id main &&\n+\tget_patch_id notsame &&\n+\t! test_cmp patch-id_main patch-id_notsame &&\n+\ttest_when_finished \"rm .gitattributes\"\n+'\n \n test_expect_success 'patch-id supports git-format-patch output' '\n \tget_patch_id main &&\n-- \ngitgitgadget\n\n"},{"id":"464922","messageId":"69440797f302729d59f19c0994916e193c9dbf58.1665737804.git.gitgitgadget@gmail.com","threadId":"58470","inReplyTo":"pull.1359.v3.git.1665737804.gitgitgadget@gmail.com","subject":"[PATCH v3 7/7] documentation: format-patch: clarify requirements for patch-ids to match","fromName":"Jerry Zhang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-14T08:56:44Z","receivedAt":"2022-10-14T08:57:04Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"From: Jerry Zhang \n\nThe documentation for format-patch advertises that the ids\nit generates for prerequisite patches are the same as piping\nthe patch into \"git patch-id\". Clarify here that this is only\ntrue if the patch was generated with -U3, and for binary patches,\nwith --full-index.\n\nNote that the actual equivalence isn't currently tested. Aside\nfrom a few cases fixed in this patch series, I've seen some\nuncommon situations where \"git diff\" and the internal diff api\nactually generate equally valid diffs of the same length, but\nwith lines reordered, which results in different patch-ids.\n\nSigned-off-by: Jerry Zhang \n---\n Documentation/git-format-patch.txt | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\nindex dfcc7da4c21..566d4b486dd 100644\n--- a/Documentation/git-format-patch.txt\n+++ b/Documentation/git-format-patch.txt\n@@ -668,8 +668,8 @@ 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-command.\n+be obtained by passing the patch (generated with -U3 --full-index) through\n+the `git patch-id --stable` command.\n \n Imagine that on top of the public commit P, you applied well-known\n patches X, Y and Z from somebody else, and then built your three-patch\n-- \ngitgitgadget\n"},{"id":"464923","messageId":"6e07cfd56917db16a281e06118cce312eb39a488.1665737804.git.gitgitgadget@gmail.com","threadId":"58470","inReplyTo":"pull.1359.v3.git.1665737804.gitgitgadget@gmail.com","subject":"[PATCH v3 4/7] patch-id: fix patch-id for mode changes","fromName":"Jerry Zhang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-14T08:56:41Z","receivedAt":"2022-10-14T08:57:18Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"From: Jerry Zhang \n\nCurrently patch-id as used in rebase and cherry-pick does not account\nfor file modes if the file is modified. One consequence of this is\nthat if you have a local patch that changes modes, but upstream\nhas applied an outdated version of the patch that doesn't include\nthat mode change, \"git rebase\" will drop your local version of the\npatch along with your mode changes. It also means that internal\npatch-id doesn't produce the same output as the builtin, which does\naccount for mode changes due to them being part of diff output.\n\nFix by adding mode to the patch-id if it has changed, in the same\nformat that would be produced by diff, so that it is compatible\nwith builtin patch-id.\n\nSigned-off-by: Jerry Zhang \n---\n diff.c | 5 +++++\n t/t3419-rebase-patch-id.sh | 31 ++++++++++++++++++++++++++++++-\n 2 files changed, 35 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 199b63dbcc3..0e336c48560 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6256,6 +6256,11 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid\n \t\t} else if (p->two->mode == 0) {\n \t\t\tpatch_id_add_string(&ctx, \"deletedfilemode\");\n \t\t\tpatch_id_add_mode(&ctx, p->one->mode);\n+\t\t} else if (p->one->mode != p->two->mode) {\n+\t\t\tpatch_id_add_string(&ctx, \"oldmode\");\n+\t\t\tpatch_id_add_mode(&ctx, p->one->mode);\n+\t\t\tpatch_id_add_string(&ctx, \"newmode\");\n+\t\t\tpatch_id_add_mode(&ctx, p->two->mode);\n \t\t}\n \n \t\tif (diff_header_only) {\ndiff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh\nindex d24e55aac8d..7181f176b81 100755\n--- a/t/t3419-rebase-patch-id.sh\n+++ b/t/t3419-rebase-patch-id.sh\n@@ -48,7 +48,17 @@ test_expect_success 'setup: 500 lines' '\n \tgit branch -f squashed main &&\n \tgit checkout -q -f squashed &&\n \tgit reset -q --soft HEAD~2 &&\n-\tgit commit -q -m squashed\n+\tgit commit -q -m squashed &&\n+\n+\tgit branch -f mode main &&\n+\tgit checkout -q -f mode &&\n+\ttest_chmod +x file &&\n+\tgit commit -q -a --amend &&\n+\n+\tgit branch -f modeother other &&\n+\tgit checkout -q -f modeother &&\n+\ttest_chmod +x file &&\n+\tgit commit -q -a --amend\n '\n \n test_expect_success 'detect upstream patch' '\n@@ -71,6 +81,13 @@ test_expect_success 'detect upstream patch binary' '\n \ttest_when_finished \"rm .gitattributes\"\n '\n \n+test_expect_success 'detect upstream patch modechange' '\n+\tgit checkout -q modeother^{} &&\n+\tgit rebase mode &&\n+\tgit rev-list mode...HEAD~ >revs &&\n+\ttest_must_be_empty revs\n+'\n+\n test_expect_success 'do not drop patch' '\n \tgit checkout -q other^{} &&\n \ttest_must_fail git rebase squashed &&\n@@ -85,4 +102,16 @@ test_expect_success 'do not drop patch binary' '\n \ttest_when_finished \"rm .gitattributes\"\n '\n \n+test_expect_success 'do not drop patch modechange' '\n+\tgit checkout -q modeother^{} &&\n+\tgit rebase other &&\n+\tcat >expected <<-\\EOF &&\n+\tdiff --git a/file b/file\n+\told mode 100644\n+\tnew mode 100755\n+\tEOF\n+\tgit diff HEAD~ >modediff &&\n+\ttest_cmp expected modediff\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"464924","messageId":"a1f6f36d4878ade4fae1142f03e53d0cc42dfb2b.1665737804.git.gitgitgadget@gmail.com","threadId":"58470","inReplyTo":"pull.1359.v3.git.1665737804.gitgitgadget@gmail.com","subject":"[PATCH v3 6/7] builtin: patch-id: remove unused diff-tree prefix","fromName":"Jerry Zhang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-14T08:56:43Z","receivedAt":"2022-10-14T08:57:20Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"From: Jerry Zhang \n\nFrom a \"git grep\" of the repo, no command, including diff-tree itself,\nproduces diff output with \"diff-tree \" prefixed in the header.\n\nThus remove its handling in \"patch-id\".\n\nSigned-off-by: Jerry Zhang \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 745fe193a71..c37b8f573b7 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -74,8 +74,8 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \t\tconst char *p = line;\n \t\tint len;\n \n-\t\tif (!skip_prefix(line, \"diff-tree \", &p) &&\n-\t\t !skip_prefix(line, \"commit \", &p) &&\n+\t\t/* Possibly skip over the prefix added by \"log\" or \"format-patch\" */\n+\t\tif (!skip_prefix(line, \"commit \", &p) &&\n \t\t !skip_prefix(line, \"From \", &p) &&\n \t\t starts_with(line, \"\\\\ \") && 12 < strlen(line)) {\n \t\t\tif (include_whitespace)\n-- \ngitgitgadget\n\n"},{"id":"464925","messageId":"bbaa2425ad0cbb4b945cdce3402c6ed5fab381ec.1665737804.git.gitgitgadget@gmail.com","threadId":"58470","inReplyTo":"pull.1359.v3.git.1665737804.gitgitgadget@gmail.com","subject":"[PATCH v3 5/7] builtin: patch-id: add --include-whitespace as a command mode","fromName":"Jerry Zhang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-14T08:56:42Z","receivedAt":"2022-10-14T08:57:23Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"From: Jerry Zhang \n\nThere are situations where the user might not want the default setting\nwhere patch-id strips all whitespace. They might be working in a\nlanguage where white space is syntactically important, or they might\nhave CI testing that enforces strict whitespace linting. In these cases,\na whitespace change would result in the patch fundamentally changing,\nand thus deserving of a different id.\n\nAdd a new mode that is exclusive of --stable and --unstable called\n--include-whitespace. It also corresponds to the config\npatchid.include_whitespace = true. In this mode, the stable algorithm\nis used and whitespace is not stripped from the patch text.\n\nSigned-off-by: Jerry Zhang \nfixes https://github.com/Skydio/revup/issues/2\n---\n Documentation/git-patch-id.txt | 25 ++++++++----\n builtin/patch-id.c | 74 ++++++++++++++++++++++------------\n t/t4204-patch-id.sh | 66 +++++++++++++++++++++++++++---\n 3 files changed, 126 insertions(+), 39 deletions(-)\n\ndiff --git a/Documentation/git-patch-id.txt b/Documentation/git-patch-id.txt\nindex 442caff8a9c..8eab4cdfe1d 100644\n--- a/Documentation/git-patch-id.txt\n+++ b/Documentation/git-patch-id.txt\n@@ -8,18 +8,18 @@ git-patch-id - Compute unique ID for a patch\n SYNOPSIS\n --------\n [verse]\n-'git patch-id' [--stable | --unstable]\n+'git patch-id' [--stable | --unstable | --include-whitespace]\n \n DESCRIPTION\n -----------\n Read a patch from the standard input and compute the patch ID for it.\n \n A \"patch ID\" is nothing but a sum of SHA-1 of the file diffs 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+patch, with 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\n+\"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+The main usecase for this command is to look for likely duplicate commits.\n \n When dealing with 'git diff-tree' output, it takes advantage of\n the fact that the patch is prefixed with the object name of the\n@@ -30,6 +30,13 @@ This can be used to make a mapping from patch ID to commit ID.\n OPTIONS\n -------\n \n+--include-whitespace::\n+\tUse the \"stable\" algorithm described below and also don't strip whitespace\n+\tfrom lines when calculating the patch-id.\n+\n+\tThis is the default if patchid.includeWhitespace is true and implies\n+\tpatchid.stable.\n+\n --stable::\n \tUse a \"stable\" sum of hashes as the patch ID. With this option:\n \t - Reordering file diffs that make up a patch does not affect the ID.\n@@ -45,14 +52,16 @@ OPTIONS\n \t of \"-O\", thereby making existing databases storing such\n \t \"unstable\" or historical patch-ids unusable.\n \n+\t - All whitespace within the patch is ignored and does not affect the id.\n+\n \tThis is the default if patchid.stable is set to true.\n \n --unstable::\n \tUse an \"unstable\" hash as the patch ID. With this option,\n \tthe result produced is compatible with the patch-id value produced\n-\tby git 1.9 and older. Users with pre-existing databases storing\n-\tpatch-ids produced by git 1.9 and older (who do not deal with reordered\n-\tpatches) may want to use this option.\n+\tby git 1.9 and older and whitespace is ignored. Users with pre-existing\n+\tdatabases storing patch-ids produced by git 1.9 and older (who do not deal\n+\twith reordered patches) may want to use this option.\n \n \tThis is the default.\n \ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex e7a31123142..745fe193a71 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -2,6 +2,7 @@\n #include \"builtin.h\"\n #include \"config.h\"\n #include \"diff.h\"\n+#include \"parse-options.h\"\n \n static void flush_current_id(int patchlen, struct object_id *id, struct object_id *result)\n {\n@@ -57,7 +58,7 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)\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+\t\t\t struct strbuf *line_buf, int stable, int include_whitespace)\n {\n \tint patchlen = 0, found_next = 0;\n \tint before = -1, after = -1;\n@@ -76,8 +77,11 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \t\tif (!skip_prefix(line, \"diff-tree \", &p) &&\n \t\t !skip_prefix(line, \"commit \", &p) &&\n \t\t !skip_prefix(line, \"From \", &p) &&\n-\t\t starts_with(line, \"\\\\ \") && 12 < strlen(line))\n+\t\t starts_with(line, \"\\\\ \") && 12 < strlen(line)) {\n+\t\t\tif (include_whitespace)\n+\t\t\t\tthe_hash_algo->update_fn(&ctx, line, strlen(line));\n \t\t\tcontinue;\n+\t\t}\n \n \t\tif (!get_oid_hex(p, next_oid)) {\n \t\t\tfound_next = 1;\n@@ -152,8 +156,8 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \t\tif (line[0] == '+' || line[0] == ' ')\n \t\t\tafter--;\n \n-\t\t/* Compute the sha without whitespace */\n-\t\tlen = remove_space(line);\n+\t\t/* Add line to hash algo (possibly removing whitespace) */\n+\t\tlen = include_whitespace ? strlen(line) : remove_space(line);\n \t\tpatchlen += len;\n \t\tthe_hash_algo->update_fn(&ctx, line, len);\n \t}\n@@ -166,7 +170,7 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \treturn patchlen;\n }\n \n-static void generate_id_list(int stable)\n+static void generate_id_list(int stable, int include_whitespace)\n {\n \tstruct object_id oid, n, result;\n \tint patchlen;\n@@ -174,21 +178,33 @@ static void generate_id_list(int stable)\n \n \toidclr(&oid);\n \twhile (!feof(stdin)) {\n-\t\tpatchlen = get_one_patchid(&n, &result, &line_buf, stable);\n+\t\tpatchlen = get_one_patchid(&n, &result, &line_buf, stable, include_whitespace);\n \t\tflush_current_id(patchlen, &oid, &result);\n \t\toidcpy(&oid, &n);\n \t}\n \tstrbuf_release(&line_buf);\n }\n \n-static const char patch_id_usage[] = \"git patch-id [--stable | --unstable]\";\n+static const char * const patch_id_usage[] = {\n+\tN_(\"git patch-id [--stable | --unstable | --include-whitespace]\"),\n+\tNULL\n+};\n+\n+struct patch_id_opts {\n+\tint stable;\n+\tint include_whitespace;\n+};\n \n static int git_patch_id_config(const char *var, const char *value, void *cb)\n {\n-\tint *stable = cb;\n+\tstruct patch_id_opts *opts = cb;\n \n \tif (!strcmp(var, \"patchid.stable\")) {\n-\t\t*stable = git_config_bool(var, value);\n+\t\topts->stable = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\tif (!strcmp(var, \"patchid.includewhitespace\")) {\n+\t\topts->include_whitespace = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n \n@@ -197,21 +213,29 @@ static int git_patch_id_config(const char *var, const char *value, void *cb)\n \n int cmd_patch_id(int argc, const char **argv, const char *prefix)\n {\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(stable);\n+\t/* if nothing is set, default to unstable */\n+\tstruct patch_id_opts config = {0, 0};\n+\tint opts = 0;\n+\tstruct option builtin_patch_id_options[] = {\n+\t\tOPT_CMDMODE(0, \"unstable\", &opts,\n+\t\t\tN_(\"use the unstable patch-id algorithm\"), 1),\n+\t\tOPT_CMDMODE(0, \"stable\", &opts,\n+\t\t\tN_(\"use the stable patch-id algorithm\"), 2),\n+\t\tOPT_CMDMODE(0, \"include-whitespace\", &opts,\n+\t\t\tN_(\"use the stable algorithm and don't strip whitespace\"), 3),\n+\t\tOPT_END()\n+\t};\n+\n+\tgit_config(git_patch_id_config, &config);\n+\n+\t/* includeWhitespace implies stable */\n+\tif (config.include_whitespace)\n+\t\tconfig.stable = 1;\n+\n+\targc = parse_options(argc, argv, prefix, builtin_patch_id_options,\n+\t\t\t patch_id_usage, 0);\n+\n+\tgenerate_id_list(opts ? opts > 1 : config.stable,\n+\t\t\t opts ? opts == 3 : config.include_whitespace);\n \treturn 0;\n }\ndiff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\nindex cdc5191aa8d..107e5a59fee 100755\n--- a/t/t4204-patch-id.sh\n+++ b/t/t4204-patch-id.sh\n@@ -8,13 +8,13 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\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+\tstr=\"ab cd ef gh ij kl mn op\" &&\n+\ttest_write_lines $str >foo &&\n+\ttest_write_lines $str >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+\ttest_write_lines $str b >foo &&\n+\ttest_write_lines $str b >bar &&\n \tgit commit -a -m first &&\n \tgit checkout -b same main &&\n \tgit commit --amend -m same-msg &&\n@@ -22,8 +22,23 @@ test_expect_success 'setup' '\n \techo c >foo &&\n \techo c >bar &&\n \tgit commit --amend -a -m notsame-msg &&\n+\tgit checkout -b with_space main~ &&\n+\tcat >foo <<-\\EOF &&\n+\ta b\n+\tc d\n+\te f\n+\t g h\n+\t i j\n+\tk l\n+\tm n\n+\top\n+\tEOF\n+\tcp foo bar &&\n+\tgit add foo bar &&\n+\tgit commit --amend -m \"with spaces\" &&\n \ttest_write_lines bar foo >bar-then-foo &&\n \ttest_write_lines foo bar >foo-then-bar\n+\n '\n \n test_expect_success 'patch-id output is well-formed' '\n@@ -128,9 +143,21 @@ test_patch_id_file_order () {\n \tgit format-patch -1 --stdout -O foo-then-bar >format-patch.output &&\n \tcalc_patch_id top-diff.output &&\n+\tcalc_patch_id top-diff.output &&\n+\tcalc_patch_id writes:\n\n> From: Jerry Zhang \n>\n> \"git patch-id\" currently doesn't produce correct output if the\n> incoming diff has any binary files. Add logic to\n> get_one_patchid to handle the different possible styles of binary\n> diff. This attempts to keep resulting patch-ids identical to what\n> would be produced by the counterpart logic in diff.c, that is it\n> produces the id by hashing the a and b oids in succession.\n\nIt is sad that we have two separate implementations in the first\nplace. Do you see if it is feasible to unify the implementation\nby reusing one from the other (answering this is not a requirement\nfor this patch to be looked at)?\n\n"},{"id":"464990","messageId":"xmqqmt9y6rem.fsf@gitster.g","threadId":"58470","inReplyTo":"2164212892712930cb34223499bb3e03bf2c2392.1665737804.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 3/7] builtin: patch-id: fix patch-id with binary diffs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-14T21:12:33Z","receivedAt":"2022-10-14T21:12:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Jerry Zhang via GitGitGadget\" writes:\n\n> From: Jerry Zhang \n>\n> \"git patch-id\" currently doesn't produce correct output if the\n> incoming diff has any binary files. Add logic to\n> get_one_patchid to handle the different possible styles of binary\n> diff. This attempts to keep resulting patch-ids identical to what\n> would be produced by the counterpart logic in diff.c, that is it\n> produces the id by hashing the a and b oids in succession.\n\nI thought I saw that a previous step touched diff.c to change how\npatch ID for a binary diff is computed to match what patch-id\ncommand computes? Now we also have to change patch-id? In the end\noutput from both may match, but which one between diff and patch-id\nhave we standardised on?\n\nPuzzled...\n"},{"id":"464991","messageId":"xmqqilkm6r6r.fsf@gitster.g","threadId":"58470","inReplyTo":"6e07cfd56917db16a281e06118cce312eb39a488.1665737804.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 4/7] patch-id: fix patch-id for mode changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-14T21:17:16Z","receivedAt":"2022-10-14T21:17:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Jerry Zhang via GitGitGadget\" writes:\n\n> Currently patch-id as used in rebase and cherry-pick does not account\n> for file modes if the file is modified. One consequence of this is\n> that if you have a local patch that changes modes, but upstream\n> has applied an outdated version of the patch that doesn't include\n> that mode change, \"git rebase\" will drop your local version of the\n> patch along with your mode changes.\n\nHmph, it may be a feature and a curse depending on the phase of the\nmoon and what you are using the patch-id computation to see if you\nalready have an identical change. But attempting to apply a patch\nafter applying the same patch with different mode bits will likely\ndo either the right thing (i.e. taking the mode changes only) or\nresult in conflict to draw human attention, so this change is a\ndefinite improvement over possibly dropping a change silently.\n\nGood.\n\n"},{"id":"464992","messageId":"xmqqbkqe6qv4.fsf@gitster.g","threadId":"58470","inReplyTo":"bbaa2425ad0cbb4b945cdce3402c6ed5fab381ec.1665737804.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 5/7] builtin: patch-id: add --include-whitespace as a command mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-14T21:24:15Z","receivedAt":"2022-10-14T21:25:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Jerry Zhang via GitGitGadget\" writes:\n\n> +--include-whitespace::\n> +\tUse the \"stable\" algorithm described below and also don't strip whitespace\n> +\tfrom lines when calculating the patch-id.\n> +\n> +\tThis is the default if patchid.includeWhitespace is true and implies\n> +\tpatchid.stable.\n\nThis seems very much orthogonal to \"--stable/--unstable. \n\nBecause the \"--stable\" variant is more expensive than \"--unstable\",\nI am not sure why such an implication is a good thing to have. Why\ncan we not have\n\n --include-whitespace --stable\n --include-whitespace --unstable\n\nboth combinations valid?\n"},{"id":"465006","messageId":"xmqq1qra6p1o.fsf@gitster.g","threadId":"58470","inReplyTo":"a1f6f36d4878ade4fae1142f03e53d0cc42dfb2b.1665737804.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 6/7] builtin: patch-id: remove unused diff-tree prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-14T22:03:31Z","receivedAt":"2022-10-14T22:03:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Jerry Zhang via GitGitGadget\" writes:\n\n> From: Jerry Zhang \n>\n> From a \"git grep\" of the repo, no command, including diff-tree itself,\n> produces diff output with \"diff-tree \" prefixed in the header.\n\nI think you are lucky and it is OK in this case, but the \"grep\" only\ntells about the current source, and a bit more due dilligence is in\ngeneral needed.\n\n * f9767222 (Add \"git-patch-id\" program to generate patch ID's.,\n 2005-06-23) introduced the line that prepares us to see\n \"diff-tree\" prefix. In that version, \"git diff-tree --pretty\n --stdin\" did use the prefix (it comes from 'diff-tree.c\").\n\n * 5f1c3f07 (log-tree: separate major part of diff-tree.,\n 2006-04-09) moved things around from \"diff-tree.c\" to\n \"log-tree.c\", without changing the behaviour.\n\n * cd2bdc53 (Common option parsing for \"git log --diff\" and friends,\n 2006-04-14) further moved revs->header_prefix set-up to\n revision.c::setup_revisions(), without changing the behaviour.\n\n * 91539833 (Log message printout cleanups, 2006-04-17) did change\n the things drastically. Its log message says:\n\n This does change \"git whatchanged\" from using \"diff-tree\" as\n the commit descriptor to \"commit\", and I changed one of the\n tests to reflect that new reality. Otherwise everything still\n passes, and my other tests look fine too.\n\nAs long as nobody keeps output from version of Git before v1.3.0\nthis change is safe to do ;-)\n\nThere may be third-party tools that was written in 2005-2006 that\nstill emit diff-tree prefix, but I somehow do not think it is likely\nanobody would feed such an output to us. Given how widely Git is\nused, I might be overly optimistic, though.\n\n> Thus remove its handling in \"patch-id\".\n>\n> Signed-off-by: Jerry Zhang \n> ---\n> builtin/patch-id.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/patch-id.c b/builtin/patch-id.c\n> index 745fe193a71..c37b8f573b7 100644\n> --- a/builtin/patch-id.c\n> +++ b/builtin/patch-id.c\n> @@ -74,8 +74,8 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n> \t\tconst char *p = line;\n> \t\tint len;\n> \n> -\t\tif (!skip_prefix(line, \"diff-tree \", &p) &&\n> -\t\t !skip_prefix(line, \"commit \", &p) &&\n> +\t\t/* Possibly skip over the prefix added by \"log\" or \"format-patch\" */\n> +\t\tif (!skip_prefix(line, \"commit \", &p) &&\n> \t\t !skip_prefix(line, \"From \", &p) &&\n> \t\t starts_with(line, \"\\\\ \") && 12 < strlen(line)) {\n> \t\t\tif (include_whitespace)\n"},{"id":"465007","messageId":"CAMKO5CtKorXv+SO821V6s4COp4MZxHbNCCY_-MR=YUjC_vgH4g@mail.gmail.com","threadId":"58470","inReplyTo":"xmqqa65y9vm0.fsf@gitster.g","subject":"Re: [PATCH v3 3/7] builtin: patch-id: fix patch-id with binary diffs","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2022-10-14T22:33:40Z","receivedAt":"2022-10-14T22:33:56Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"On Fri, Oct 14, 2022 at 10:13 AM Junio C Hamano wrote:\n>\n> \"Jerry Zhang via GitGitGadget\" writes:\n>\n> > From: Jerry Zhang \n> >\n> > \"git patch-id\" currently doesn't produce correct output if the\n> > incoming diff has any binary files. Add logic to\n> > get_one_patchid to handle the different possible styles of binary\n> > diff. This attempts to keep resulting patch-ids identical to what\n> > would be produced by the counterpart logic in diff.c, that is it\n> > produces the id by hashing the a and b oids in succession.\n>\n> It is sad that we have two separate implementations in the first\n> place. Do you see if it is feasible to unify the implementation\n> by reusing one from the other (answering this is not a requirement\n> for this patch to be looked at)?\nYeah I wondered this myself, it's tricky because they are actually doing\nopposite things: the diff.c logic is adding diff metadata before doing the\npatch-id, while the patch-id logic is parsing out the diff metadata. We could\nrefactor it, but would have to be careful not to accidentally change the output\nsemantics.\n\nAnother possible path to \"unifying\" the logic would be to add a\n\"--patch-id\" mode\nto \"git diff' that produces the patch-id of what would be the diff,\nrather than the diff\nitself. For the usecases that involve piping \"git diff\" into \"git\npatch-id\", this would\nrequire not needing the separate patch-id tool at all. Of course\npeople also like\nto run \"patch-id\" on the output of \"format-patch\" after the fact so\nthis isn't a perfect\nsolution either.\n\nSpeaking of which, do you have some context as to why we promise that\n\"git patch-id\"\noutput will remain the same across git versions? Were there cases in\nthe past where\npeople actually made persistent databases of patch-ids, or complained\nabout the output\nchanging? I ask because this requirement makes it difficult to make\nbig changes, and\nthere aren't any tests to verify consistent output between git\nversions. Also git itself\nis already a persistent database of patches, so I'm not sure why\nsomeone would choose\nto implement a new system for this.\n>\n"},{"id":"465008","messageId":"CAMKO5CvdDWEd6HPbkg7DP9bZMKNzcvmK+c1UPpuTk7vM1D8i9g@mail.gmail.com","threadId":"58470","inReplyTo":"xmqqmt9y6rem.fsf@gitster.g","subject":"Re: [PATCH v3 3/7] builtin: patch-id: fix patch-id with binary diffs","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2022-10-14T22:34:05Z","receivedAt":"2022-10-14T22:34:18Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"On Fri, Oct 14, 2022 at 2:12 PM Junio C Hamano wrote:\n>\n> \"Jerry Zhang via GitGitGadget\" writes:\n>\n> > From: Jerry Zhang \n> >\n> > \"git patch-id\" currently doesn't produce correct output if the\n> > incoming diff has any binary files. Add logic to\n> > get_one_patchid to handle the different possible styles of binary\n> > diff. This attempts to keep resulting patch-ids identical to what\n> > would be produced by the counterpart logic in diff.c, that is it\n> > produces the id by hashing the a and b oids in succession.\n>\n> I thought I saw that a previous step touched diff.c to change how\n> patch ID for a binary diff is computed to match what patch-id\n> command computes? Now we also have to change patch-id? In the end\n> output from both may match, but which one between diff and patch-id\n> have we standardised on?\nEr yeah let me see if I can simplify.\n\nBefore:\nInternal patch-id w/ unstable + binary was correct\nInternal patch-id w/ stable + binary was broken\nbuiltin patch-id w/ binary was broken\n\nAfter:\nInternal patch-id w/ unstable + binary is correct\nInternal patch-id w/ stable + binary is now correct\nbuiltin patch-id w/ binary is now correct\n\nSo the \"standard\" actually came from the one working case from\n\"before\", which was the diff.c logic + unstable. I based all new logic\non that because it seemed reasonable and correct. Since \"internal\nw/unstable\" is never exposed externally, it's perhaps true that i\ncould have invented a totally new format and standardized on that.\nHashing the oids in succession is pretty much representative of a\nbinary patch though, so I don't think there's much to be improved on.\n>\n> Puzzled...\n"},{"id":"465009","messageId":"CAMKO5CuCbyFt739GOzcvFn92i8vNqK6vgJqvT8E5zs=kJ1+H=A@mail.gmail.com","threadId":"58470","inReplyTo":"xmqqbkqe6qv4.fsf@gitster.g","subject":"Re: [PATCH v3 5/7] builtin: patch-id: add --include-whitespace as a command mode","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2022-10-14T22:55:22Z","receivedAt":"2022-10-14T22:55:38Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"On Fri, Oct 14, 2022 at 2:24 PM Junio C Hamano wrote:\n>\n> \"Jerry Zhang via GitGitGadget\" writes:\n>\n> > +--include-whitespace::\n> > + Use the \"stable\" algorithm described below and also don't strip whitespace\n> > + from lines when calculating the patch-id.\n> > +\n> > + This is the default if patchid.includeWhitespace is true and implies\n> > + patchid.stable.\n>\n> This seems very much orthogonal to \"--stable/--unstable.\n>\n> Because the \"--stable\" variant is more expensive than \"--unstable\",\nI didn't realize it was more expensive, I'm assuming you mean in terms\nof time, maybe it does\nslightly more hashing operations under the hood? I tried timing some\nruns locally\nand they were a wash:\n\ntime /bin/sh -c \"git show | git patch-id --stable\"\ndddea79ee68d62a32cf8c0d7bb6691bcd0445628\n4677fe858366a51ff3c5a0c0893418e32e934262\n\nreal 0m0.011s\nuser 0m0.003s\nsys 0m0.012s\ntime /bin/sh -c \"git show | git patch-id\"\n6602a3b2fe8b17d5bc295c2703901ad3e18eee18\n4677fe858366a51ff3c5a0c0893418e32e934262\n\nreal 0m0.012s\nuser 0m0.009s\nsys 0m0.007s\n\nThe operation is probably bound by process / disk overhead quite a bit\nand a small\namount of cpu use wouldn't really be user-visible. Based on these\nresults I don't think\na user would choose --unstable just for the speed gain (if any).\n\n> I am not sure why such an implication is a good thing to have. Why\n> can we not have\n>\n> --include-whitespace --stable\n> --include-whitespace --unstable\n>\n> both combinations valid?\nIf you accept my point above, then a user would only choose\n\"--unstable\" if they actually\nhad a need for backwards compatibility, such as for a persistent\ndatabase. Trying to include\nwhitespace on top of that would break the compatibility they're\nrelying on. So my conclusion was\nthat there isn't any usecase for the combination \"--include-whitespace\n--unstable\", and it's better for\nusability and not needing to always maintain compatibility if we don't\nexpose it to users at all.\n"},{"id":"465106","messageId":"xmqqo7ua1nrz.fsf@gitster.g","threadId":"58470","inReplyTo":"69440797f302729d59f19c0994916e193c9dbf58.1665737804.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 7/7] documentation: format-patch: clarify requirements for patch-ids to match","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-17T15:18:56Z","receivedAt":"2022-10-17T15:19:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Jerry Zhang via GitGitGadget\" writes:\n\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> -command.\n> +be obtained by passing the patch (generated with -U3 --full-index) through\n> +the `git patch-id --stable` command.\n\nThis is not incorrect per-se, but I wonder how much it would help or\nmislead people in practice.\n\nI understand that the update means well to help those who complain\n\"'patch-id' produces wrong result when I feed the output of 'git\ndiff -U1'\" by making them suspect that their -U1 may be the culprit.\nBut the new description does not cover everything that can affect\nthe resulting patch ID (the choice of --diff-algorithm affects how\ncommon lines are matched up between the preimage and the postimage,\nfor example).\n\nSo, I dunno.\n"},{"id":"465107","messageId":"xmqqk04y1nks.fsf@gitster.g","threadId":"58470","inReplyTo":"CAMKO5CvdDWEd6HPbkg7DP9bZMKNzcvmK+c1UPpuTk7vM1D8i9g@mail.gmail.com","subject":"Re: [PATCH v3 3/7] builtin: patch-id: fix patch-id with binary diffs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-17T15:23:15Z","receivedAt":"2022-10-17T15:23:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jerry Zhang writes:\n\n>> I thought I saw that a previous step touched diff.c to change how\n>> patch ID for a binary diff is computed to match what patch-id\n>> command computes? Now we also have to change patch-id? In the end\n>> output from both may match, but which one between diff and patch-id\n>> have we standardised on?\n> Er yeah let me see if I can simplify.\n>\n> Before:\n> Internal patch-id w/ unstable + binary was correct\n> Internal patch-id w/ stable + binary was broken\n> builtin patch-id w/ binary was broken\n>\n> After:\n> Internal patch-id w/ unstable + binary is correct\n> Internal patch-id w/ stable + binary is now correct\n> builtin patch-id w/ binary is now correct\n>\n> So the \"standard\" actually came from the one working case from\n> \"before\", which was the diff.c logic + unstable.\n\nOK.\n\nThe question was meant to help you improve the log message, as it is\nsomething a future reader of \"git log\" would wonder after reading\nthem. I think including something that makes it easy for readers to\narrive at the summary above themselves by reading the log message\nwould be a very much welcome change.\n\nThanks.\n"},{"id":"465108","messageId":"xmqq7d0y1mw7.fsf@gitster.g","threadId":"58470","inReplyTo":"xmqqbkqe6qv4.fsf@gitster.g","subject":"Re: [PATCH v3 5/7] builtin: patch-id: add --include-whitespace as a command mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-17T15:38:00Z","receivedAt":"2022-10-17T15:38:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"Jerry Zhang via GitGitGadget\" writes:\n>\n>> +--include-whitespace::\n>> +\tUse the \"stable\" algorithm described below and also don't strip whitespace\n>> +\tfrom lines when calculating the patch-id.\n>> +\n>> +\tThis is the default if patchid.includeWhitespace is true and implies\n>> +\tpatchid.stable.\n>\n> This seems very much orthogonal to \"--stable/--unstable. \n>\n> Because the \"--stable\" variant is more expensive than \"--unstable\",\n\nSorry, I misspoke. The way we make the result stable is *not* by\nenforcing a fixed order to the input of the hash (which would have\nbeen more expensive), but by hashing each file separately and\nsumming up the hashes, and it shouldn't be noticeably more expensive\nthan the unstable variant.\n\nSo, I do not think I mind if we introduced --include-whitespace as a\nthird option in addition to --stable and --unstable, instead of allowing\nit to be combined with both --stable and --unstable.\n\nBut I wonder:\n\n * (minor) Would \"--verbatim\" work as a better option name? The\n name \"--include-whitespace\" can apply even to an implementation\n that squashes multiple consecutive spaces and tabs into a single\n space, i.e. we keep words on a single line as separate words,\n instead of squishing them together, when hashing.\n\n * Do users even care about the internal reliance on the \"stable\"\n algorithm? Wouldn't it be better to leave such an implementation\n detail unsaid? After all, \"--verbatim --unstable\" would not work\n as they expect.\n\nSo I would suggest dropping \"and implies patchid.stable\" from the\nabove description.\n\n\n"},{"id":"465227","messageId":"CAMKO5CuqLSowSo3fhOux-fY8ek-CL4zudgA0fBjXAt+9CBhs9g@mail.gmail.com","threadId":"58470","inReplyTo":"xmqqo7ua1nrz.fsf@gitster.g","subject":"Re: [PATCH v3 7/7] documentation: format-patch: clarify requirements for patch-ids to match","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2022-10-18T21:57:27Z","receivedAt":"2022-10-18T21:57:43Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"On Mon, Oct 17, 2022 at 8:19 AM Junio C Hamano wrote:\n>\n> \"Jerry Zhang via GitGitGadget\" writes:\n>\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> > -command.\n> > +be obtained by passing the patch (generated with -U3 --full-index) through\n> > +the `git patch-id --stable` command.\n>\n> This is not incorrect per-se, but I wonder how much it would help or\n> mislead people in practice.\n>\n> I understand that the update means well to help those who complain\n> \"'patch-id' produces wrong result when I feed the output of 'git\n> diff -U1'\" by making them suspect that their -U1 may be the culprit.\n> But the new description does not cover everything that can affect\n> the resulting patch ID (the choice of --diff-algorithm affects how\n> common lines are matched up between the preimage and the postimage,\n> for example).\nI can add a note about diff algorithm as well. Its slightly different\nfrom the other two options\nin that there should be fewer cases where diff algorithm is a problem.\nU3 and full-index aren't\nthe default options to \"git diff\" so people are more likely to have\nthe wrong options,\nwhereas they would explicitly have to run the two diffs with different\nalgorithms to\nrun into a problem. But there's no harm in being more specific.\n>\n> So, I dunno.\nKeeping patch-ids the same is difficult especially between different\ngit versions, as\nchanges could be made to \"diff\" that seem like improvements but would\nslightly change\npatch-id even with the same args. I expect that people generally aren't keeping\nthe compatibility of patch-id in mind when changing diff. Nevertheless\nsince we've\nalready advertised that they would match, we ought to give users the best advice\npossible to minimize if not eliminate confusion.\n"},{"id":"465228","messageId":"CAMKO5CuEaFwe5WgAC=whz1ouXs1PiD6GW5Qay82w4723EHsEfQ@mail.gmail.com","threadId":"58470","inReplyTo":"xmqq7d0y1mw7.fsf@gitster.g","subject":"Re: [PATCH v3 5/7] builtin: patch-id: add --include-whitespace as a command mode","fromName":"Jerry Zhang","fromEmail":"jerry@skydio.com","sentAt":"2022-10-18T22:12:30Z","receivedAt":"2022-10-18T22:12:47Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"On Mon, Oct 17, 2022 at 8:38 AM Junio C Hamano wrote:\n>\n> Junio C Hamano writes:\n>\n> > \"Jerry Zhang via GitGitGadget\" writes:\n> >\n> >> +--include-whitespace::\n> >> + Use the \"stable\" algorithm described below and also don't strip whitespace\n> >> + from lines when calculating the patch-id.\n> >> +\n> >> + This is the default if patchid.includeWhitespace is true and implies\n> >> + patchid.stable.\n> >\n> > This seems very much orthogonal to \"--stable/--unstable.\n> >\n> > Because the \"--stable\" variant is more expensive than \"--unstable\",\n>\n> Sorry, I misspoke. The way we make the result stable is *not* by\n> enforcing a fixed order to the input of the hash (which would have\n> been more expensive), but by hashing each file separately and\n> summing up the hashes, and it shouldn't be noticeably more expensive\n> than the unstable variant.\n>\n> So, I do not think I mind if we introduced --include-whitespace as a\n> third option in addition to --stable and --unstable, instead of allowing\n> it to be combined with both --stable and --unstable.\n>\n> But I wonder:\n>\n> * (minor) Would \"--verbatim\" work as a better option name? The\n> name \"--include-whitespace\" can apply even to an implementation\n> that squashes multiple consecutive spaces and tabs into a single\n> space, i.e. we keep words on a single line as separate words,\n> instead of squishing them together, when hashing.\n>\n> * Do users even care about the internal reliance on the \"stable\"\n> algorithm? Wouldn't it be better to leave such an implementation\n> detail unsaid? After all, \"--verbatim --unstable\" would not work\n> as they expect.\n>\n> So I would suggest dropping \"and implies patchid.stable\" from the\n> above description.\nSure I can spin a new series with these changes\n>\n>\n"},{"id":"465254","messageId":"xmqqtu3zss47.fsf@gitster.g","threadId":"58470","inReplyTo":"CAMKO5CuqLSowSo3fhOux-fY8ek-CL4zudgA0fBjXAt+9CBhs9g@mail.gmail.com","subject":"Re: [PATCH v3 7/7] documentation: format-patch: clarify requirements for patch-ids to match","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-19T16:19:52Z","receivedAt":"2022-10-19T16:20:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jerry Zhang writes:\n\n> I can add a note about diff algorithm as well.\n\nThat's totally different from what I had in mind. We _could_ be\nmore specific, but I do not think that helps users, as we cannot\npromise we will keep using the same diff algorithm and parameters,\nand the implementation would change to give \"better\" output for\nhuman consumption that is not byte-for-byte identical to older one.\n\n>> So, I dunno.\n\nSo, I do not know if more description is a good idea to begin with.\n"},{"id":"465372","messageId":"321757ef919bc75e58108d6e6bef4aaeeb4b326a.1666307815.git.gitgitgadget@gmail.com","threadId":"58470","inReplyTo":"pull.1359.v4.git.1666307815.gitgitgadget@gmail.com","subject":"[PATCH v4 1/6] patch-id: fix stable patch id for binary / header-only","fromName":"Jerry Zhang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-20T23:16:50Z","receivedAt":"2022-10-20T23:17:08Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"From: Jerry Zhang \n\nPatch-ids for binary patches are found by hashing the object\nids of the before and after objects in succession. However in\nthe --stable case, there is a bug where hunks are not flushed\nfor binary and header-only patch ids, which would always result\nin a patch-id of 0000. The --unstable case is currently correct.\n\nReorder the logic to branch into 3 cases for populating the\npatch body: header-only which populates nothing, binary which\npopulates the object ids, and normal which populates the text\ndiff. All branches will end up flushing the hunk.\n\nDon't populate the ---a/ and +++b/ lines for binary diffs, to correspond\nto those lines not being present in the \"git diff\" text output.\nThis is necessary because we advertise that the patch-id calculated\ninternally and used in format-patch is the same that what the\nbuiltin \"git patch-id\" would produce when piped from a diff.\n\nUpdate the test to run on both binary and normal files.\n\nSigned-off-by: Jerry Zhang \n---\n diff.c | 58 +++++++++++++++++++-------------------\n t/t3419-rebase-patch-id.sh | 34 +++++++++++++++-------\n 2 files changed, 53 insertions(+), 39 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 648f6717a55..c15169e4b06 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6253,46 +6253,46 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid\n \t\tif (p->one->mode == 0) {\n \t\t\tpatch_id_add_string(&ctx, \"newfilemode\");\n \t\t\tpatch_id_add_mode(&ctx, p->two->mode);\n-\t\t\tpatch_id_add_string(&ctx, \"---/dev/null\");\n-\t\t\tpatch_id_add_string(&ctx, \"+++b/\");\n-\t\t\tthe_hash_algo->update_fn(&ctx, p->two->path, len2);\n \t\t} else if (p->two->mode == 0) {\n \t\t\tpatch_id_add_string(&ctx, \"deletedfilemode\");\n \t\t\tpatch_id_add_mode(&ctx, p->one->mode);\n-\t\t\tpatch_id_add_string(&ctx, \"---a/\");\n-\t\t\tthe_hash_algo->update_fn(&ctx, p->one->path, len1);\n-\t\t\tpatch_id_add_string(&ctx, \"+++/dev/null\");\n-\t\t} else {\n-\t\t\tpatch_id_add_string(&ctx, \"---a/\");\n-\t\t\tthe_hash_algo->update_fn(&ctx, p->one->path, len1);\n-\t\t\tpatch_id_add_string(&ctx, \"+++b/\");\n-\t\t\tthe_hash_algo->update_fn(&ctx, p->two->path, len2);\n \t\t}\n \n-\t\tif (diff_header_only)\n-\t\t\tcontinue;\n-\n-\t\tif (fill_mmfile(options->repo, &mf1, p->one) < 0 ||\n-\t\t fill_mmfile(options->repo, &mf2, p->two) < 0)\n-\t\t\treturn error(\"unable to read files to diff\");\n-\n-\t\tif (diff_filespec_is_binary(options->repo, p->one) ||\n+\t\tif (diff_header_only) {\n+\t\t\t/* don't do anything since we're only populating header info */\n+\t\t} else if (diff_filespec_is_binary(options->repo, p->one) ||\n \t\t diff_filespec_is_binary(options->repo, p->two)) {\n \t\t\tthe_hash_algo->update_fn(&ctx, oid_to_hex(&p->one->oid),\n \t\t\t\t\tthe_hash_algo->hexsz);\n \t\t\tthe_hash_algo->update_fn(&ctx, oid_to_hex(&p->two->oid),\n \t\t\t\t\tthe_hash_algo->hexsz);\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\txpp.flags = 0;\n-\t\txecfg.ctxlen = 3;\n-\t\txecfg.flags = XDL_EMIT_NO_HUNK_HDR;\n-\t\tif (xdi_diff_outf(&mf1, &mf2, NULL,\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+\t\t} else {\n+\t\t\tif (p->one->mode == 0) {\n+\t\t\t\tpatch_id_add_string(&ctx, \"---/dev/null\");\n+\t\t\t\tpatch_id_add_string(&ctx, \"+++b/\");\n+\t\t\t\tthe_hash_algo->update_fn(&ctx, p->two->path, len2);\n+\t\t\t} else if (p->two->mode == 0) {\n+\t\t\t\tpatch_id_add_string(&ctx, \"---a/\");\n+\t\t\t\tthe_hash_algo->update_fn(&ctx, p->one->path, len1);\n+\t\t\t\tpatch_id_add_string(&ctx, \"+++/dev/null\");\n+\t\t\t} else {\n+\t\t\t\tpatch_id_add_string(&ctx, \"---a/\");\n+\t\t\t\tthe_hash_algo->update_fn(&ctx, p->one->path, len1);\n+\t\t\t\tpatch_id_add_string(&ctx, \"+++b/\");\n+\t\t\t\tthe_hash_algo->update_fn(&ctx, p->two->path, len2);\n+\t\t\t}\n \n+\t\t\tif (fill_mmfile(options->repo, &mf1, p->one) < 0 ||\n+\t\t\t fill_mmfile(options->repo, &mf2, p->two) < 0)\n+\t\t\t\treturn error(\"unable to read files to diff\");\n+\t\t\txpp.flags = 0;\n+\t\t\txecfg.ctxlen = 3;\n+\t\t\txecfg.flags = XDL_EMIT_NO_HUNK_HDR;\n+\t\t\tif (xdi_diff_outf(&mf1, &mf2, NULL,\n+\t\t\t\t\t patch_id_consume, &data, &xpp, &xecfg))\n+\t\t\t\treturn error(\"unable to generate patch-id diff for %s\",\n+\t\t\t\t\t p->one->path);\n+\t\t}\n \t\tif (stable)\n \t\t\tflush_one_hunk(oid, &ctx);\n \t}\ndiff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh\nindex 295040f2fe3..d24e55aac8d 100755\n--- a/t/t3419-rebase-patch-id.sh\n+++ b/t/t3419-rebase-patch-id.sh\n@@ -43,15 +43,16 @@ test_expect_success 'setup: 500 lines' '\n \tgit add newfile &&\n \tgit commit -q -m \"add small file\" &&\n \n-\tgit cherry-pick main >/dev/null 2>&1\n-'\n+\tgit cherry-pick main >/dev/null 2>&1 &&\n \n-test_expect_success 'setup attributes' '\n-\techo \"file binary\" >.gitattributes\n+\tgit branch -f squashed main &&\n+\tgit checkout -q -f squashed &&\n+\tgit reset -q --soft HEAD~2 &&\n+\tgit commit -q -m squashed\n '\n \n test_expect_success 'detect upstream patch' '\n-\tgit checkout -q main &&\n+\tgit checkout -q main^{} &&\n \tscramble file &&\n \tgit add file &&\n \tgit commit -q -m \"change big file again\" &&\n@@ -61,14 +62,27 @@ test_expect_success 'detect upstream patch' '\n \ttest_must_be_empty revs\n '\n \n+test_expect_success 'detect upstream patch binary' '\n+\techo \"file binary\" >.gitattributes &&\n+\tgit checkout -q other^{} &&\n+\tgit rebase main &&\n+\tgit rev-list main...HEAD~ >revs &&\n+\ttest_must_be_empty revs &&\n+\ttest_when_finished \"rm .gitattributes\"\n+'\n+\n test_expect_success 'do not drop patch' '\n-\tgit branch -f squashed main &&\n-\tgit checkout -q -f squashed &&\n-\tgit reset -q --soft HEAD~2 &&\n-\tgit commit -q -m squashed &&\n \tgit checkout -q other^{} &&\n \ttest_must_fail git rebase squashed &&\n-\tgit rebase --quit\n+\ttest_when_finished \"git rebase --abort\"\n+'\n+\n+test_expect_success 'do not drop patch binary' '\n+\techo \"file binary\" >.gitattributes &&\n+\tgit checkout -q other^{} &&\n+\ttest_must_fail git rebase squashed &&\n+\ttest_when_finished \"git rebase --abort\" &&\n+\ttest_when_finished \"rm .gitattributes\"\n '\n \n test_done\n-- \ngitgitgadget\n\n"},{"id":"465373","messageId":"pull.1359.v4.git.1666307815.gitgitgadget@gmail.com","threadId":"58470","inReplyTo":"pull.1359.v3.git.1665737804.gitgitgadget@gmail.com","subject":"[PATCH v4 0/6] patch-id fixes and improvements","fromName":"Jerry Zhang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-20T23:16:49Z","receivedAt":"2022-10-20T23:17:09Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"These patches add fixes and features to the \"git patch-id\" command, mostly\ndiscovered through our usage of patch-id in the revup project\n(https://github.com/Skydio/revup). On top of that I've tried to make general\ncleanup changes where I can.\n\nSummary:\n\n1: Fixed a bug in the combination of --stable with binary files and\nheader-only, and expanded the test to cover both binary and non-binary\nfiles.\n\n2: Switch internal usage of patch-id in rebase / cherry-pick to use the\nstable variant to reduce the number of code paths and improve testing for\nbugs like above.\n\n3: Fixed bugs with patch-id and binary diffs. Previously patch-id did not\nbehave correctly for binary diffs regardless of whether \"--binary\" was given\nto \"diff\".\n\n4: Fixed bugs with patch-id and mode changes. Previously mode changes were\nincorrectly excluded from the patch-id.\n\n5: Add a new \"--include-whitespace\" mode to patch-id that prevents\nwhitespace from being stripped during id calculation. Also add a config\noption for the same behavior.\n\n6: Remove unused prefix from patch-id logic.\n\nV1->V2: Fixed comment style V2->V3: The ---/+++ lines no longer get added to\nthe patch-id of binary diffs. Also added patches 3-7 in the series. V3->V3:\nDropped patch7. Updated flag name to --verbatim. Updated commit message\ndescriptions.\n\nSigned-off-by: Jerry Zhang jerry@skydio.com\n\nJerry Zhang (6):\n patch-id: fix stable patch id for binary / header-only\n patch-id: use stable patch-id for rebases\n builtin: patch-id: fix patch-id with binary diffs\n patch-id: fix patch-id for mode changes\n builtin: patch-id: add --verbatim as a command mode\n builtin: patch-id: remove unused diff-tree prefix\n\n Documentation/git-patch-id.txt | 24 ++++---\n builtin/log.c | 2 +-\n builtin/patch-id.c | 113 ++++++++++++++++++++++++---------\n diff.c | 75 +++++++++++-----------\n diff.h | 2 +-\n patch-ids.c | 10 +--\n patch-ids.h | 2 +-\n t/t3419-rebase-patch-id.sh | 63 +++++++++++++++---\n t/t4204-patch-id.sh | 95 +++++++++++++++++++++++++--\n 9 files changed, 287 insertions(+), 99 deletions(-)\n\n\nbase-commit: 45c9f05c44b1cb6bd2d6cb95a22cf5e3d21d5b63\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1359%2Fjerry-skydio%2Fjerry%2Frevup%2Fmaster%2Fpatch_ids-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1359/jerry-skydio/jerry/revup/master/patch_ids-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1359\n\nRange-diff vs v3:\n\n 1: 7d4c2e91ce0 ! 1: 321757ef919 patch-id: fix stable patch id for binary / header-only\n @@ Metadata\n ## Commit message ##\n patch-id: fix stable patch id for binary / header-only\n \n - Previous logic here skipped flushing the hunks for binary\n - and header-only patch ids, which would always result in a\n - patch-id of 0000.\n + Patch-ids for binary patches are found by hashing the object\n + ids of the before and after objects in succession. However in\n + the --stable case, there is a bug where hunks are not flushed\n + for binary and header-only patch ids, which would always result\n + in a patch-id of 0000. The --unstable case is currently correct.\n \n Reorder the logic to branch into 3 cases for populating the\n patch body: header-only which populates nothing, binary which\n 2: 25e28b7dab3 = 2: ec4a2422d5b patch-id: use stable patch-id for rebases\n 3: 21642128927 ! 3: 81501355313 builtin: patch-id: fix patch-id with binary diffs\n @@ Commit message\n builtin: patch-id: fix patch-id with binary diffs\n \n \"git patch-id\" currently doesn't produce correct output if the\n - incoming diff has any binary files. Add logic to\n - get_one_patchid to handle the different possible styles of binary\n - diff. This attempts to keep resulting patch-ids identical to what\n - would be produced by the counterpart logic in diff.c, that is it\n - produces the id by hashing the a and b oids in succession.\n + incoming diff has any binary files. Add logic to get_one_patchid\n + to handle the different possible styles of binary diff. This\n + attempts to keep resulting patch-ids identical to what would be\n + produced by the counterpart logic in diff.c, that is it produces\n + the id by hashing the a and b oids in succession.\n \n In general we handle binary diffs by first caching the object ids from\n the \"index\" line and using those if we then find an indication\n 4: 6e07cfd5691 = 4: bb0b4add03c patch-id: fix patch-id for mode changes\n 5: bbaa2425ad0 ! 5: b160f2ae49f builtin: patch-id: add --include-whitespace as a command mode\n @@ Metadata\n Author: Jerry Zhang \n \n ## Commit message ##\n - builtin: patch-id: add --include-whitespace as a command mode\n + builtin: patch-id: add --verbatim as a command mode\n \n - There are situations where the user might not want the default setting\n - where patch-id strips all whitespace. They might be working in a\n - language where white space is syntactically important, or they might\n - have CI testing that enforces strict whitespace linting. In these cases,\n - a whitespace change would result in the patch fundamentally changing,\n - and thus deserving of a different id.\n + There are situations where the user might not want the default\n + setting where patch-id strips all whitespace. They might be working\n + in a language where white space is syntactically important, or they\n + might have CI testing that enforces strict whitespace linting. In\n + these cases, a whitespace change would result in the patch\n + fundamentally changing, and thus deserving of a different id.\n \n Add a new mode that is exclusive of --stable and --unstable called\n - --include-whitespace. It also corresponds to the config\n - patchid.include_whitespace = true. In this mode, the stable algorithm\n - is used and whitespace is not stripped from the patch text.\n + --verbatim. It also corresponds to the config\n + patchid.verbatim = true. In this mode, the stable algorithm is\n + used and whitespace is not stripped from the patch text.\n +\n + Users of --unstable mainly care about compatibility with old git\n + versions, which unstripping the whitespace would break. Thus there\n + isn't a usecase for the combination of --verbatim and --unstable,\n + and we don't expose this so as to not add maintainence burden.\n \n Signed-off-by: Jerry Zhang \n fixes https://github.com/Skydio/revup/issues/2\n @@ Documentation/git-patch-id.txt: git-patch-id - Compute unique ID for a patch\n --------\n [verse]\n -'git patch-id' [--stable | --unstable]\n -+'git patch-id' [--stable | --unstable | --include-whitespace]\n ++'git patch-id' [--stable | --unstable | --verbatim]\n \n DESCRIPTION\n -----------\n @@ Documentation/git-patch-id.txt: This can be used to make a mapping from patch ID\n OPTIONS\n -------\n \n -+--include-whitespace::\n -+\tUse the \"stable\" algorithm described below and also don't strip whitespace\n -+\tfrom lines when calculating the patch-id.\n ++--verbatim::\n ++\tCalculate the patch-id of the input as it is given, do not strip\n ++\tany whitespace.\n +\n -+\tThis is the default if patchid.includeWhitespace is true and implies\n -+\tpatchid.stable.\n ++\tThis is the default if patchid.verbatim is true.\n +\n --stable::\n \tUse a \"stable\" sum of hashes as the patch ID. With this option:\n @@ builtin/patch-id.c: static int scan_hunk_header(const char *p, int *p_before, in\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 -+\t\t\t struct strbuf *line_buf, int stable, int include_whitespace)\n ++\t\t\t struct strbuf *line_buf, int stable, int verbatim)\n {\n \tint patchlen = 0, found_next = 0;\n \tint before = -1, after = -1;\n @@ builtin/patch-id.c: static int get_one_patchid(struct object_id *next_oid, struc\n \t\t !skip_prefix(line, \"From \", &p) &&\n -\t\t starts_with(line, \"\\\\ \") && 12 < strlen(line))\n +\t\t starts_with(line, \"\\\\ \") && 12 < strlen(line)) {\n -+\t\t\tif (include_whitespace)\n ++\t\t\tif (verbatim)\n +\t\t\t\tthe_hash_algo->update_fn(&ctx, line, strlen(line));\n \t\t\tcontinue;\n +\t\t}\n @@ builtin/patch-id.c: static int get_one_patchid(struct object_id *next_oid, struc\n -\t\t/* Compute the sha without whitespace */\n -\t\tlen = remove_space(line);\n +\t\t/* Add line to hash algo (possibly removing whitespace) */\n -+\t\tlen = include_whitespace ? strlen(line) : remove_space(line);\n ++\t\tlen = verbatim ? strlen(line) : remove_space(line);\n \t\tpatchlen += len;\n \t\tthe_hash_algo->update_fn(&ctx, line, len);\n \t}\n @@ builtin/patch-id.c: static int get_one_patchid(struct object_id *next_oid, struc\n }\n \n -static void generate_id_list(int stable)\n -+static void generate_id_list(int stable, int include_whitespace)\n ++static void generate_id_list(int stable, int verbatim)\n {\n \tstruct object_id oid, n, result;\n \tint patchlen;\n @@ builtin/patch-id.c: static void generate_id_list(int stable)\n \toidclr(&oid);\n \twhile (!feof(stdin)) {\n -\t\tpatchlen = get_one_patchid(&n, &result, &line_buf, stable);\n -+\t\tpatchlen = get_one_patchid(&n, &result, &line_buf, stable, include_whitespace);\n ++\t\tpatchlen = get_one_patchid(&n, &result, &line_buf, stable, verbatim);\n \t\tflush_current_id(patchlen, &oid, &result);\n \t\toidcpy(&oid, &n);\n \t}\n @@ builtin/patch-id.c: static void generate_id_list(int stable)\n }\n \n -static const char patch_id_usage[] = \"git patch-id [--stable | --unstable]\";\n -+static const char * const patch_id_usage[] = {\n -+\tN_(\"git patch-id [--stable | --unstable | --include-whitespace]\"),\n -+\tNULL\n ++static const char *const patch_id_usage[] = {\n ++\tN_(\"git patch-id [--stable | --unstable | --verbatim]\"), NULL\n +};\n +\n +struct patch_id_opts {\n +\tint stable;\n -+\tint include_whitespace;\n ++\tint verbatim;\n +};\n \n static int git_patch_id_config(const char *var, const char *value, void *cb)\n @@ builtin/patch-id.c: static void generate_id_list(int stable)\n +\t\topts->stable = git_config_bool(var, value);\n +\t\treturn 0;\n +\t}\n -+\tif (!strcmp(var, \"patchid.includewhitespace\")) {\n -+\t\topts->include_whitespace = git_config_bool(var, value);\n ++\tif (!strcmp(var, \"patchid.verbatim\")) {\n ++\t\topts->verbatim = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n \n @@ builtin/patch-id.c: static int git_patch_id_config(const char *var, const char *\n +\tint opts = 0;\n +\tstruct option builtin_patch_id_options[] = {\n +\t\tOPT_CMDMODE(0, \"unstable\", &opts,\n -+\t\t\tN_(\"use the unstable patch-id algorithm\"), 1),\n ++\t\t N_(\"use the unstable patch-id algorithm\"), 1),\n +\t\tOPT_CMDMODE(0, \"stable\", &opts,\n -+\t\t\tN_(\"use the stable patch-id algorithm\"), 2),\n -+\t\tOPT_CMDMODE(0, \"include-whitespace\", &opts,\n -+\t\t\tN_(\"use the stable algorithm and don't strip whitespace\"), 3),\n ++\t\t N_(\"use the stable patch-id algorithm\"), 2),\n ++\t\tOPT_CMDMODE(0, \"verbatim\", &opts,\n ++\t\t\tN_(\"don't strip whitespace from the patch\"), 3),\n +\t\tOPT_END()\n +\t};\n +\n +\tgit_config(git_patch_id_config, &config);\n +\n -+\t/* includeWhitespace implies stable */\n -+\tif (config.include_whitespace)\n ++\t/* verbatim implies stable */\n ++\tif (config.verbatim)\n +\t\tconfig.stable = 1;\n +\n +\targc = parse_options(argc, argv, prefix, builtin_patch_id_options,\n +\t\t\t patch_id_usage, 0);\n +\n +\tgenerate_id_list(opts ? opts > 1 : config.stable,\n -+\t\t\t opts ? opts == 3 : config.include_whitespace);\n ++\t\t\t opts ? opts == 3 : config.verbatim);\n \treturn 0;\n }\n \n @@ t/t4204-patch-id.sh: test_expect_success 'file order is relevant with --unstable\n \ttest_patch_id_file_order relevant --unstable --unstable\n '\n \n -+test_expect_success 'whitespace is relevant with --include-whitespace' '\n -+\ttest_patch_id_whitespace relevant --include-whitespace --include-whitespace\n ++test_expect_success 'whitespace is relevant with --verbatim' '\n ++\ttest_patch_id_whitespace relevant --verbatim --verbatim\n +'\n +\n -+test_expect_success 'whitespace is irrelevant without --include-whitespace' '\n ++test_expect_success 'whitespace is irrelevant without --verbatim' '\n +\ttest_patch_id_whitespace irrelevant --stable --stable\n +'\n +\n @@ t/t4204-patch-id.sh: test_expect_success 'patchid.stable = false is unstable' '\n \ttest_patch_id relevant patchid.stable=false\n '\n \n -+test_expect_success 'patchid.includeWhitespace = true is correct and stable' '\n -+\ttest_config patchid.includeWhitespace true &&\n -+\ttest_patch_id_whitespace relevant patchid.includeWhitespace=true &&\n -+\ttest_patch_id irrelevant patchid.includeWhitespace=true\n ++test_expect_success 'patchid.verbatim = true is correct and stable' '\n ++\ttest_config patchid.verbatim true &&\n ++\ttest_patch_id_whitespace relevant patchid.verbatim=true &&\n ++\ttest_patch_id irrelevant patchid.verbatim=true\n +'\n +\n -+test_expect_success 'patchid.includeWhitespace = false is unstable' '\n -+\ttest_config patchid.includeWhitespace false &&\n -+\ttest_patch_id relevant patchid.includeWhitespace=false\n ++test_expect_success 'patchid.verbatim = false is unstable' '\n ++\ttest_config patchid.verbatim false &&\n ++\ttest_patch_id relevant patchid.verbatim=false\n +'\n +\n test_expect_success '--unstable overrides patchid.stable = true' '\n @@ t/t4204-patch-id.sh: test_expect_success '--stable overrides patchid.stable = fa\n \ttest_patch_id irrelevant patchid.stable=false--stable --stable\n '\n \n -+test_expect_success '--include-whitespace overrides patchid.stable = false' '\n ++test_expect_success '--verbatim overrides patchid.stable = false' '\n +\ttest_config patchid.stable false &&\n -+\ttest_patch_id_whitespace relevant stable=false--include-whitespace --include-whitespace\n ++\ttest_patch_id_whitespace relevant stable=false--verbatim --verbatim\n +'\n +\n test_expect_success 'patch-id supports git-format-patch MIME output' '\n @@ t/t4204-patch-id.sh: test_expect_success 'patch-id handles no-nl-at-eof markers'\n \tcalc_patch_id withnl \n\nGit doesn't persist patch-ids during the rebase process, so there is\nno need to specifically invoke the unstable variant. Use the stable\nlogic for all internal patch-id calculations to minimize the number of\ncode paths and improve test coverage.\n\nSigned-off-by: Jerry Zhang \n---\n builtin/log.c | 2 +-\n diff.c | 12 ++++--------\n diff.h | 2 +-\n patch-ids.c | 10 +++++-----\n patch-ids.h | 2 +-\n 5 files changed, 12 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex ee19dc5d450..e72869afb36 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1763,7 +1763,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, 1))\n+\t\tif (commit_patch_id(commit, &diffopt, &oid, 0))\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/diff.c b/diff.c\nindex c15169e4b06..199b63dbcc3 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6206,7 +6206,7 @@ static void patch_id_add_mode(git_hash_ctx *ctx, unsigned mode)\n }\n \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+static int diff_get_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only)\n {\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n \tint i;\n@@ -6293,21 +6293,17 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid\n \t\t\t\treturn error(\"unable to generate patch-id diff for %s\",\n \t\t\t\t\t p->one->path);\n \t\t}\n-\t\tif (stable)\n-\t\t\tflush_one_hunk(oid, &ctx);\n+\t\tflush_one_hunk(oid, &ctx);\n \t}\n \n-\tif (!stable)\n-\t\tthe_hash_algo->final_oid_fn(oid, &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, int stable)\n+int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only)\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, stable);\n+\tint result = diff_get_patch_id(options, oid, diff_header_only);\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 8ae18e5ab1e..fd33caeb25d 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -634,7 +634,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option);\n int run_diff_index(struct rev_info *revs, unsigned int option);\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, int);\n+int diff_flush_patch_id(struct diff_options *, struct object_id *, int);\n void flush_one_hunk(struct object_id *result, git_hash_ctx *ctx);\n \n int diff_result_code(struct diff_options *, int);\ndiff --git a/patch-ids.c b/patch-ids.c\nindex 46c6a8f3eab..31534466266 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, int stable)\n+\t\t struct object_id *oid, int diff_header_only)\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, stable);\n+\treturn diff_flush_patch_id(options, oid, diff_header_only);\n }\n \n /*\n@@ -48,11 +48,11 @@ static int patch_id_neq(const void *cmpfn_data,\n \tb = container_of(entry_or_key, struct patch_id, ent);\n \n \tif (is_null_oid(&a->patch_id) &&\n-\t commit_patch_id(a->commit, opt, &a->patch_id, 0, 0))\n+\t commit_patch_id(a->commit, opt, &a->patch_id, 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, 0))\n+\t commit_patch_id(b->commit, opt, &b->patch_id, 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@@ -82,7 +82,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, 0))\n+\tif (commit_patch_id(commit, &ids->diffopts, &header_only_patch_id, 1))\n \t\treturn -1;\n \n \thashmap_entry_init(&patch->ent, oidhash(&header_only_patch_id));\ndiff --git a/patch-ids.h b/patch-ids.h\nindex ab6c6a68047..490d7393716 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, int);\n+\t\t struct object_id *oid, int);\n int init_patch_ids(struct repository *, struct patch_ids *);\n int free_patch_ids(struct patch_ids *);\n \n-- \ngitgitgadget\n\n"},{"id":"465375","messageId":"815013553133cddae5baf9d3dca00f8318e250f7.1666307815.git.gitgitgadget@gmail.com","threadId":"58470","inReplyTo":"pull.1359.v4.git.1666307815.gitgitgadget@gmail.com","subject":"[PATCH v4 3/6] builtin: patch-id: fix patch-id with binary diffs","fromName":"Jerry Zhang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-20T23:16:52Z","receivedAt":"2022-10-20T23:17:14Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"From: Jerry Zhang \n\n\"git patch-id\" currently doesn't produce correct output if the\nincoming diff has any binary files. Add logic to get_one_patchid\nto handle the different possible styles of binary diff. This\nattempts to keep resulting patch-ids identical to what would be\nproduced by the counterpart logic in diff.c, that is it produces\nthe id by hashing the a and b oids in succession.\n\nIn general we handle binary diffs by first caching the object ids from\nthe \"index\" line and using those if we then find an indication\nthat the diff is binary.\n\nThe input could contain patches generated with \"git diff --binary\". This\ncurrently breaks the parse logic and results in multiple patch-ids\noutput for a single commit. Here we have to skip the contents of the\npatch itself since those do not go into the patch id. --binary\nimplies --full-index so the object ids are always available.\n\nWhen the diff is generated with --full-index there is no patch content\nto skip over.\n\nWhen a diff is generated without --full-index or --binary, it will\ncontain abbreviated object ids. This will still result in a sufficiently\nunique patch-id when hashed, but does not match internal patch id\noutput. We'll call this ok for now as we already need specialized\narguments to diff in order to match internal patch id (namely -U3).\n\nSigned-off-by: Jerry Zhang \n---\n builtin/patch-id.c | 36 ++++++++++++++++++++++++++++++++++--\n t/t4204-patch-id.sh | 29 ++++++++++++++++++++++++++++-\n 2 files changed, 62 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex 881fcf32732..e7a31123142 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -61,6 +61,8 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n {\n \tint patchlen = 0, found_next = 0;\n \tint before = -1, after = -1;\n+\tint diff_is_binary = 0;\n+\tchar pre_oid_str[GIT_MAX_HEXSZ + 1], post_oid_str[GIT_MAX_HEXSZ + 1];\n \tgit_hash_ctx ctx;\n \n \tthe_hash_algo->init_fn(&ctx);\n@@ -88,14 +90,44 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \n \t\t/* Parsing diff header? */\n \t\tif (before == -1) {\n-\t\t\tif (starts_with(line, \"index \"))\n+\t\t\tif (starts_with(line, \"GIT binary patch\") ||\n+\t\t\t starts_with(line, \"Binary files\")) {\n+\t\t\t\tdiff_is_binary = 1;\n+\t\t\t\tbefore = 0;\n+\t\t\t\tthe_hash_algo->update_fn(&ctx, pre_oid_str,\n+\t\t\t\t\t\t\t strlen(pre_oid_str));\n+\t\t\t\tthe_hash_algo->update_fn(&ctx, post_oid_str,\n+\t\t\t\t\t\t\t strlen(post_oid_str));\n+\t\t\t\tif (stable)\n+\t\t\t\t\tflush_one_hunk(result, &ctx);\n \t\t\t\tcontinue;\n-\t\t\telse if (starts_with(line, \"--- \"))\n+\t\t\t} else if (skip_prefix(line, \"index \", &p)) {\n+\t\t\t\tchar *oid1_end = strstr(line, \"..\");\n+\t\t\t\tchar *oid2_end = NULL;\n+\t\t\t\tif (oid1_end)\n+\t\t\t\t\toid2_end = strstr(oid1_end, \" \");\n+\t\t\t\tif (!oid2_end)\n+\t\t\t\t\toid2_end = line + strlen(line) - 1;\n+\t\t\t\tif (oid1_end != NULL && oid2_end != NULL) {\n+\t\t\t\t\t*oid1_end = *oid2_end = '\\0';\n+\t\t\t\t\tstrlcpy(pre_oid_str, p, GIT_MAX_HEXSZ + 1);\n+\t\t\t\t\tstrlcpy(post_oid_str, oid1_end + 2, GIT_MAX_HEXSZ + 1);\n+\t\t\t\t}\n+\t\t\t\tcontinue;\n+\t\t\t} else if (starts_with(line, \"--- \"))\n \t\t\t\tbefore = after = 1;\n \t\t\telse if (!isalpha(line[0]))\n \t\t\t\tbreak;\n \t\t}\n \n+\t\tif (diff_is_binary) {\n+\t\t\tif (starts_with(line, \"diff \")) {\n+\t\t\t\tdiff_is_binary = 0;\n+\t\t\t\tbefore = -1;\n+\t\t\t}\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\t/* Looking for a valid hunk header? */\n \t\tif (before == 0 && after == 0) {\n \t\t\tif (starts_with(line, \"@@ -\")) {\ndiff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\nindex a730c0db985..cdc5191aa8d 100755\n--- a/t/t4204-patch-id.sh\n+++ b/t/t4204-patch-id.sh\n@@ -42,7 +42,7 @@ calc_patch_id () {\n }\n \n get_top_diff () {\n-\tgit log -p -1 \"$@\" -O bar-then-foo --\n+\tgit log -p -1 \"$@\" -O bar-then-foo --full-index --\n }\n \n get_patch_id () {\n@@ -61,6 +61,33 @@ test_expect_success 'patch-id detects inequality' '\n \tget_patch_id notsame &&\n \t! test_cmp patch-id_main patch-id_notsame\n '\n+test_expect_success 'patch-id detects equality binary' '\n+\tcat >.gitattributes <<-\\EOF &&\n+\tfoo binary\n+\tbar binary\n+\tEOF\n+\tget_patch_id main &&\n+\tget_patch_id same &&\n+\tgit log -p -1 --binary main >top-diff.output &&\n+\tcalc_patch_id top-diff.output &&\n+\tcalc_patch_id .gitattributes <<-\\EOF &&\n+\tfoo binary\n+\tbar binary\n+\tEOF\n+\tget_patch_id main &&\n+\tget_patch_id notsame &&\n+\t! test_cmp patch-id_main patch-id_notsame &&\n+\ttest_when_finished \"rm .gitattributes\"\n+'\n \n test_expect_success 'patch-id supports git-format-patch output' '\n \tget_patch_id main &&\n-- \ngitgitgadget\n\n"},{"id":"465376","messageId":"bb0b4add03c158a0b32306cbe075960fff53d78a.1666307815.git.gitgitgadget@gmail.com","threadId":"58470","inReplyTo":"pull.1359.v4.git.1666307815.gitgitgadget@gmail.com","subject":"[PATCH v4 4/6] patch-id: fix patch-id for mode changes","fromName":"Jerry Zhang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-20T23:16:53Z","receivedAt":"2022-10-20T23:17:17Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"From: Jerry Zhang \n\nCurrently patch-id as used in rebase and cherry-pick does not account\nfor file modes if the file is modified. One consequence of this is\nthat if you have a local patch that changes modes, but upstream\nhas applied an outdated version of the patch that doesn't include\nthat mode change, \"git rebase\" will drop your local version of the\npatch along with your mode changes. It also means that internal\npatch-id doesn't produce the same output as the builtin, which does\naccount for mode changes due to them being part of diff output.\n\nFix by adding mode to the patch-id if it has changed, in the same\nformat that would be produced by diff, so that it is compatible\nwith builtin patch-id.\n\nSigned-off-by: Jerry Zhang \n---\n diff.c | 5 +++++\n t/t3419-rebase-patch-id.sh | 31 ++++++++++++++++++++++++++++++-\n 2 files changed, 35 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 199b63dbcc3..0e336c48560 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6256,6 +6256,11 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid\n \t\t} else if (p->two->mode == 0) {\n \t\t\tpatch_id_add_string(&ctx, \"deletedfilemode\");\n \t\t\tpatch_id_add_mode(&ctx, p->one->mode);\n+\t\t} else if (p->one->mode != p->two->mode) {\n+\t\t\tpatch_id_add_string(&ctx, \"oldmode\");\n+\t\t\tpatch_id_add_mode(&ctx, p->one->mode);\n+\t\t\tpatch_id_add_string(&ctx, \"newmode\");\n+\t\t\tpatch_id_add_mode(&ctx, p->two->mode);\n \t\t}\n \n \t\tif (diff_header_only) {\ndiff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh\nindex d24e55aac8d..7181f176b81 100755\n--- a/t/t3419-rebase-patch-id.sh\n+++ b/t/t3419-rebase-patch-id.sh\n@@ -48,7 +48,17 @@ test_expect_success 'setup: 500 lines' '\n \tgit branch -f squashed main &&\n \tgit checkout -q -f squashed &&\n \tgit reset -q --soft HEAD~2 &&\n-\tgit commit -q -m squashed\n+\tgit commit -q -m squashed &&\n+\n+\tgit branch -f mode main &&\n+\tgit checkout -q -f mode &&\n+\ttest_chmod +x file &&\n+\tgit commit -q -a --amend &&\n+\n+\tgit branch -f modeother other &&\n+\tgit checkout -q -f modeother &&\n+\ttest_chmod +x file &&\n+\tgit commit -q -a --amend\n '\n \n test_expect_success 'detect upstream patch' '\n@@ -71,6 +81,13 @@ test_expect_success 'detect upstream patch binary' '\n \ttest_when_finished \"rm .gitattributes\"\n '\n \n+test_expect_success 'detect upstream patch modechange' '\n+\tgit checkout -q modeother^{} &&\n+\tgit rebase mode &&\n+\tgit rev-list mode...HEAD~ >revs &&\n+\ttest_must_be_empty revs\n+'\n+\n test_expect_success 'do not drop patch' '\n \tgit checkout -q other^{} &&\n \ttest_must_fail git rebase squashed &&\n@@ -85,4 +102,16 @@ test_expect_success 'do not drop patch binary' '\n \ttest_when_finished \"rm .gitattributes\"\n '\n \n+test_expect_success 'do not drop patch modechange' '\n+\tgit checkout -q modeother^{} &&\n+\tgit rebase other &&\n+\tcat >expected <<-\\EOF &&\n+\tdiff --git a/file b/file\n+\told mode 100644\n+\tnew mode 100755\n+\tEOF\n+\tgit diff HEAD~ >modediff &&\n+\ttest_cmp expected modediff\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"465377","messageId":"b160f2ae49f8906249e7690d089a1921c43b3bda.1666307815.git.gitgitgadget@gmail.com","threadId":"58470","inReplyTo":"pull.1359.v4.git.1666307815.gitgitgadget@gmail.com","subject":"[PATCH v4 5/6] builtin: patch-id: add --verbatim as a command mode","fromName":"Jerry Zhang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-20T23:16:54Z","receivedAt":"2022-10-20T23:17:20Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"From: Jerry Zhang \n\nThere are situations where the user might not want the default\nsetting where patch-id strips all whitespace. They might be working\nin a language where white space is syntactically important, or they\nmight have CI testing that enforces strict whitespace linting. In\nthese cases, a whitespace change would result in the patch\nfundamentally changing, and thus deserving of a different id.\n\nAdd a new mode that is exclusive of --stable and --unstable called\n--verbatim. It also corresponds to the config\npatchid.verbatim = true. In this mode, the stable algorithm is\nused and whitespace is not stripped from the patch text.\n\nUsers of --unstable mainly care about compatibility with old git\nversions, which unstripping the whitespace would break. Thus there\nisn't a usecase for the combination of --verbatim and --unstable,\nand we don't expose this so as to not add maintainence burden.\n\nSigned-off-by: Jerry Zhang \nfixes https://github.com/Skydio/revup/issues/2\n---\n Documentation/git-patch-id.txt | 24 +++++++----\n builtin/patch-id.c | 73 ++++++++++++++++++++++------------\n t/t4204-patch-id.sh | 66 +++++++++++++++++++++++++++---\n 3 files changed, 124 insertions(+), 39 deletions(-)\n\ndiff --git a/Documentation/git-patch-id.txt b/Documentation/git-patch-id.txt\nindex 442caff8a9c..1d15fa45d51 100644\n--- a/Documentation/git-patch-id.txt\n+++ b/Documentation/git-patch-id.txt\n@@ -8,18 +8,18 @@ git-patch-id - Compute unique ID for a patch\n SYNOPSIS\n --------\n [verse]\n-'git patch-id' [--stable | --unstable]\n+'git patch-id' [--stable | --unstable | --verbatim]\n \n DESCRIPTION\n -----------\n Read a patch from the standard input and compute the patch ID for it.\n \n A \"patch ID\" is nothing but a sum of SHA-1 of the file diffs 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+patch, with 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\n+\"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+The main usecase for this command is to look for likely duplicate commits.\n \n When dealing with 'git diff-tree' output, it takes advantage of\n the fact that the patch is prefixed with the object name of the\n@@ -30,6 +30,12 @@ This can be used to make a mapping from patch ID to commit ID.\n OPTIONS\n -------\n \n+--verbatim::\n+\tCalculate the patch-id of the input as it is given, do not strip\n+\tany whitespace.\n+\n+\tThis is the default if patchid.verbatim is true.\n+\n --stable::\n \tUse a \"stable\" sum of hashes as the patch ID. With this option:\n \t - Reordering file diffs that make up a patch does not affect the ID.\n@@ -45,14 +51,16 @@ OPTIONS\n \t of \"-O\", thereby making existing databases storing such\n \t \"unstable\" or historical patch-ids unusable.\n \n+\t - All whitespace within the patch is ignored and does not affect the id.\n+\n \tThis is the default if patchid.stable is set to true.\n \n --unstable::\n \tUse an \"unstable\" hash as the patch ID. With this option,\n \tthe result produced is compatible with the patch-id value produced\n-\tby git 1.9 and older. Users with pre-existing databases storing\n-\tpatch-ids produced by git 1.9 and older (who do not deal with reordered\n-\tpatches) may want to use this option.\n+\tby git 1.9 and older and whitespace is ignored. Users with pre-existing\n+\tdatabases storing patch-ids produced by git 1.9 and older (who do not deal\n+\twith reordered patches) may want to use this option.\n \n \tThis is the default.\n \ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex e7a31123142..afdd472369f 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -2,6 +2,7 @@\n #include \"builtin.h\"\n #include \"config.h\"\n #include \"diff.h\"\n+#include \"parse-options.h\"\n \n static void flush_current_id(int patchlen, struct object_id *id, struct object_id *result)\n {\n@@ -57,7 +58,7 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)\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+\t\t\t struct strbuf *line_buf, int stable, int verbatim)\n {\n \tint patchlen = 0, found_next = 0;\n \tint before = -1, after = -1;\n@@ -76,8 +77,11 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \t\tif (!skip_prefix(line, \"diff-tree \", &p) &&\n \t\t !skip_prefix(line, \"commit \", &p) &&\n \t\t !skip_prefix(line, \"From \", &p) &&\n-\t\t starts_with(line, \"\\\\ \") && 12 < strlen(line))\n+\t\t starts_with(line, \"\\\\ \") && 12 < strlen(line)) {\n+\t\t\tif (verbatim)\n+\t\t\t\tthe_hash_algo->update_fn(&ctx, line, strlen(line));\n \t\t\tcontinue;\n+\t\t}\n \n \t\tif (!get_oid_hex(p, next_oid)) {\n \t\t\tfound_next = 1;\n@@ -152,8 +156,8 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \t\tif (line[0] == '+' || line[0] == ' ')\n \t\t\tafter--;\n \n-\t\t/* Compute the sha without whitespace */\n-\t\tlen = remove_space(line);\n+\t\t/* Add line to hash algo (possibly removing whitespace) */\n+\t\tlen = verbatim ? strlen(line) : remove_space(line);\n \t\tpatchlen += len;\n \t\tthe_hash_algo->update_fn(&ctx, line, len);\n \t}\n@@ -166,7 +170,7 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \treturn patchlen;\n }\n \n-static void generate_id_list(int stable)\n+static void generate_id_list(int stable, int verbatim)\n {\n \tstruct object_id oid, n, result;\n \tint patchlen;\n@@ -174,21 +178,32 @@ static void generate_id_list(int stable)\n \n \toidclr(&oid);\n \twhile (!feof(stdin)) {\n-\t\tpatchlen = get_one_patchid(&n, &result, &line_buf, stable);\n+\t\tpatchlen = get_one_patchid(&n, &result, &line_buf, stable, verbatim);\n \t\tflush_current_id(patchlen, &oid, &result);\n \t\toidcpy(&oid, &n);\n \t}\n \tstrbuf_release(&line_buf);\n }\n \n-static const char patch_id_usage[] = \"git patch-id [--stable | --unstable]\";\n+static const char *const patch_id_usage[] = {\n+\tN_(\"git patch-id [--stable | --unstable | --verbatim]\"), NULL\n+};\n+\n+struct patch_id_opts {\n+\tint stable;\n+\tint verbatim;\n+};\n \n static int git_patch_id_config(const char *var, const char *value, void *cb)\n {\n-\tint *stable = cb;\n+\tstruct patch_id_opts *opts = cb;\n \n \tif (!strcmp(var, \"patchid.stable\")) {\n-\t\t*stable = git_config_bool(var, value);\n+\t\topts->stable = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\tif (!strcmp(var, \"patchid.verbatim\")) {\n+\t\topts->verbatim = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n \n@@ -197,21 +212,29 @@ static int git_patch_id_config(const char *var, const char *value, void *cb)\n \n int cmd_patch_id(int argc, const char **argv, const char *prefix)\n {\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(stable);\n+\t/* if nothing is set, default to unstable */\n+\tstruct patch_id_opts config = {0, 0};\n+\tint opts = 0;\n+\tstruct option builtin_patch_id_options[] = {\n+\t\tOPT_CMDMODE(0, \"unstable\", &opts,\n+\t\t N_(\"use the unstable patch-id algorithm\"), 1),\n+\t\tOPT_CMDMODE(0, \"stable\", &opts,\n+\t\t N_(\"use the stable patch-id algorithm\"), 2),\n+\t\tOPT_CMDMODE(0, \"verbatim\", &opts,\n+\t\t\tN_(\"don't strip whitespace from the patch\"), 3),\n+\t\tOPT_END()\n+\t};\n+\n+\tgit_config(git_patch_id_config, &config);\n+\n+\t/* verbatim implies stable */\n+\tif (config.verbatim)\n+\t\tconfig.stable = 1;\n+\n+\targc = parse_options(argc, argv, prefix, builtin_patch_id_options,\n+\t\t\t patch_id_usage, 0);\n+\n+\tgenerate_id_list(opts ? opts > 1 : config.stable,\n+\t\t\t opts ? opts == 3 : config.verbatim);\n \treturn 0;\n }\ndiff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\nindex cdc5191aa8d..a7fa94ce0a2 100755\n--- a/t/t4204-patch-id.sh\n+++ b/t/t4204-patch-id.sh\n@@ -8,13 +8,13 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\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+\tstr=\"ab cd ef gh ij kl mn op\" &&\n+\ttest_write_lines $str >foo &&\n+\ttest_write_lines $str >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+\ttest_write_lines $str b >foo &&\n+\ttest_write_lines $str b >bar &&\n \tgit commit -a -m first &&\n \tgit checkout -b same main &&\n \tgit commit --amend -m same-msg &&\n@@ -22,8 +22,23 @@ test_expect_success 'setup' '\n \techo c >foo &&\n \techo c >bar &&\n \tgit commit --amend -a -m notsame-msg &&\n+\tgit checkout -b with_space main~ &&\n+\tcat >foo <<-\\EOF &&\n+\ta b\n+\tc d\n+\te f\n+\t g h\n+\t i j\n+\tk l\n+\tm n\n+\top\n+\tEOF\n+\tcp foo bar &&\n+\tgit add foo bar &&\n+\tgit commit --amend -m \"with spaces\" &&\n \ttest_write_lines bar foo >bar-then-foo &&\n \ttest_write_lines foo bar >foo-then-bar\n+\n '\n \n test_expect_success 'patch-id output is well-formed' '\n@@ -128,9 +143,21 @@ test_patch_id_file_order () {\n \tgit format-patch -1 --stdout -O foo-then-bar >format-patch.output &&\n \tcalc_patch_id top-diff.output &&\n+\tcalc_patch_id top-diff.output &&\n+\tcalc_patch_id \n\nFrom a \"git grep\" of the repo, no command, including diff-tree itself,\nproduces diff output with \"diff-tree \" prefixed in the header.\n\nThus remove its handling in \"patch-id\".\n\nSigned-off-by: Jerry Zhang \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 afdd472369f..f840fbf1c7e 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -74,8 +74,8 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \t\tconst char *p = line;\n \t\tint len;\n \n-\t\tif (!skip_prefix(line, \"diff-tree \", &p) &&\n-\t\t !skip_prefix(line, \"commit \", &p) &&\n+\t\t/* Possibly skip over the prefix added by \"log\" or \"format-patch\" */\n+\t\tif (!skip_prefix(line, \"commit \", &p) &&\n \t\t !skip_prefix(line, \"From \", &p) &&\n \t\t starts_with(line, \"\\\\ \") && 12 < strlen(line)) {\n \t\t\tif (verbatim)\n-- \ngitgitgadget\n"},{"id":"465442","messageId":"xmqqh6zxo71f.fsf@gitster.g","threadId":"58470","inReplyTo":"dcdfac7a1539103926dd46e8c3d5c10fe640c0f3.1666307815.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 6/6] builtin: patch-id: remove unused diff-tree prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-21T09:33:16Z","receivedAt":"2022-10-21T09:33:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Jerry Zhang via GitGitGadget\" writes:\n\n> From: Jerry Zhang \n>\n> From a \"git grep\" of the repo, no command, including diff-tree itself,\n> produces diff output with \"diff-tree \" prefixed in the header.\n>\n> Thus remove its handling in \"patch-id\".\n\nThere is a bit of leap in the logic flow here, in that the current\nstate alone does not justify such a removal of the code that is not\nhurting anybody. I thought I did the necessary homework the last\ntime to help you update the proposed log message for this step with\nnecessary due diligence, like when we stopped producing it\nourselves. The lack of third-party tools still relying on the code\nwe are removing here is not something we can prove easily, so\ndocumenting that we go by faith there would not hurt, either.\n"},{"id":"465648","messageId":"bb0b4add03c158a0b32306cbe075960fff53d78a.1666642065.git.gitgitgadget@gmail.com","threadId":"58470","inReplyTo":"pull.1359.v5.git.1666642064.gitgitgadget@gmail.com","subject":"[PATCH v5 4/6] patch-id: fix patch-id for mode changes","fromName":"Jerry Zhang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-24T20:07:42Z","receivedAt":"2022-10-24T21:55:33Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"From: Jerry Zhang \n\nCurrently patch-id as used in rebase and cherry-pick does not account\nfor file modes if the file is modified. One consequence of this is\nthat if you have a local patch that changes modes, but upstream\nhas applied an outdated version of the patch that doesn't include\nthat mode change, \"git rebase\" will drop your local version of the\npatch along with your mode changes. It also means that internal\npatch-id doesn't produce the same output as the builtin, which does\naccount for mode changes due to them being part of diff output.\n\nFix by adding mode to the patch-id if it has changed, in the same\nformat that would be produced by diff, so that it is compatible\nwith builtin patch-id.\n\nSigned-off-by: Jerry Zhang \n---\n diff.c | 5 +++++\n t/t3419-rebase-patch-id.sh | 31 ++++++++++++++++++++++++++++++-\n 2 files changed, 35 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 199b63dbcc3..0e336c48560 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6256,6 +6256,11 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid\n \t\t} else if (p->two->mode == 0) {\n \t\t\tpatch_id_add_string(&ctx, \"deletedfilemode\");\n \t\t\tpatch_id_add_mode(&ctx, p->one->mode);\n+\t\t} else if (p->one->mode != p->two->mode) {\n+\t\t\tpatch_id_add_string(&ctx, \"oldmode\");\n+\t\t\tpatch_id_add_mode(&ctx, p->one->mode);\n+\t\t\tpatch_id_add_string(&ctx, \"newmode\");\n+\t\t\tpatch_id_add_mode(&ctx, p->two->mode);\n \t\t}\n \n \t\tif (diff_header_only) {\ndiff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh\nindex d24e55aac8d..7181f176b81 100755\n--- a/t/t3419-rebase-patch-id.sh\n+++ b/t/t3419-rebase-patch-id.sh\n@@ -48,7 +48,17 @@ test_expect_success 'setup: 500 lines' '\n \tgit branch -f squashed main &&\n \tgit checkout -q -f squashed &&\n \tgit reset -q --soft HEAD~2 &&\n-\tgit commit -q -m squashed\n+\tgit commit -q -m squashed &&\n+\n+\tgit branch -f mode main &&\n+\tgit checkout -q -f mode &&\n+\ttest_chmod +x file &&\n+\tgit commit -q -a --amend &&\n+\n+\tgit branch -f modeother other &&\n+\tgit checkout -q -f modeother &&\n+\ttest_chmod +x file &&\n+\tgit commit -q -a --amend\n '\n \n test_expect_success 'detect upstream patch' '\n@@ -71,6 +81,13 @@ test_expect_success 'detect upstream patch binary' '\n \ttest_when_finished \"rm .gitattributes\"\n '\n \n+test_expect_success 'detect upstream patch modechange' '\n+\tgit checkout -q modeother^{} &&\n+\tgit rebase mode &&\n+\tgit rev-list mode...HEAD~ >revs &&\n+\ttest_must_be_empty revs\n+'\n+\n test_expect_success 'do not drop patch' '\n \tgit checkout -q other^{} &&\n \ttest_must_fail git rebase squashed &&\n@@ -85,4 +102,16 @@ test_expect_success 'do not drop patch binary' '\n \ttest_when_finished \"rm .gitattributes\"\n '\n \n+test_expect_success 'do not drop patch modechange' '\n+\tgit checkout -q modeother^{} &&\n+\tgit rebase other &&\n+\tcat >expected <<-\\EOF &&\n+\tdiff --git a/file b/file\n+\told mode 100644\n+\tnew mode 100755\n+\tEOF\n+\tgit diff HEAD~ >modediff &&\n+\ttest_cmp expected modediff\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"465649","messageId":"b160f2ae49f8906249e7690d089a1921c43b3bda.1666642065.git.gitgitgadget@gmail.com","threadId":"58470","inReplyTo":"pull.1359.v5.git.1666642064.gitgitgadget@gmail.com","subject":"[PATCH v5 5/6] builtin: patch-id: add --verbatim as a command mode","fromName":"Jerry Zhang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-24T20:07:43Z","receivedAt":"2022-10-24T21:55:35Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"From: Jerry Zhang \n\nThere are situations where the user might not want the default\nsetting where patch-id strips all whitespace. They might be working\nin a language where white space is syntactically important, or they\nmight have CI testing that enforces strict whitespace linting. In\nthese cases, a whitespace change would result in the patch\nfundamentally changing, and thus deserving of a different id.\n\nAdd a new mode that is exclusive of --stable and --unstable called\n--verbatim. It also corresponds to the config\npatchid.verbatim = true. In this mode, the stable algorithm is\nused and whitespace is not stripped from the patch text.\n\nUsers of --unstable mainly care about compatibility with old git\nversions, which unstripping the whitespace would break. Thus there\nisn't a usecase for the combination of --verbatim and --unstable,\nand we don't expose this so as to not add maintainence burden.\n\nSigned-off-by: Jerry Zhang \nfixes https://github.com/Skydio/revup/issues/2\n---\n Documentation/git-patch-id.txt | 24 +++++++----\n builtin/patch-id.c | 73 ++++++++++++++++++++++------------\n t/t4204-patch-id.sh | 66 +++++++++++++++++++++++++++---\n 3 files changed, 124 insertions(+), 39 deletions(-)\n\ndiff --git a/Documentation/git-patch-id.txt b/Documentation/git-patch-id.txt\nindex 442caff8a9c..1d15fa45d51 100644\n--- a/Documentation/git-patch-id.txt\n+++ b/Documentation/git-patch-id.txt\n@@ -8,18 +8,18 @@ git-patch-id - Compute unique ID for a patch\n SYNOPSIS\n --------\n [verse]\n-'git patch-id' [--stable | --unstable]\n+'git patch-id' [--stable | --unstable | --verbatim]\n \n DESCRIPTION\n -----------\n Read a patch from the standard input and compute the patch ID for it.\n \n A \"patch ID\" is nothing but a sum of SHA-1 of the file diffs 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+patch, with 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\n+\"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+The main usecase for this command is to look for likely duplicate commits.\n \n When dealing with 'git diff-tree' output, it takes advantage of\n the fact that the patch is prefixed with the object name of the\n@@ -30,6 +30,12 @@ This can be used to make a mapping from patch ID to commit ID.\n OPTIONS\n -------\n \n+--verbatim::\n+\tCalculate the patch-id of the input as it is given, do not strip\n+\tany whitespace.\n+\n+\tThis is the default if patchid.verbatim is true.\n+\n --stable::\n \tUse a \"stable\" sum of hashes as the patch ID. With this option:\n \t - Reordering file diffs that make up a patch does not affect the ID.\n@@ -45,14 +51,16 @@ OPTIONS\n \t of \"-O\", thereby making existing databases storing such\n \t \"unstable\" or historical patch-ids unusable.\n \n+\t - All whitespace within the patch is ignored and does not affect the id.\n+\n \tThis is the default if patchid.stable is set to true.\n \n --unstable::\n \tUse an \"unstable\" hash as the patch ID. With this option,\n \tthe result produced is compatible with the patch-id value produced\n-\tby git 1.9 and older. Users with pre-existing databases storing\n-\tpatch-ids produced by git 1.9 and older (who do not deal with reordered\n-\tpatches) may want to use this option.\n+\tby git 1.9 and older and whitespace is ignored. Users with pre-existing\n+\tdatabases storing patch-ids produced by git 1.9 and older (who do not deal\n+\twith reordered patches) may want to use this option.\n \n \tThis is the default.\n \ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex e7a31123142..afdd472369f 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -2,6 +2,7 @@\n #include \"builtin.h\"\n #include \"config.h\"\n #include \"diff.h\"\n+#include \"parse-options.h\"\n \n static void flush_current_id(int patchlen, struct object_id *id, struct object_id *result)\n {\n@@ -57,7 +58,7 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)\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+\t\t\t struct strbuf *line_buf, int stable, int verbatim)\n {\n \tint patchlen = 0, found_next = 0;\n \tint before = -1, after = -1;\n@@ -76,8 +77,11 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \t\tif (!skip_prefix(line, \"diff-tree \", &p) &&\n \t\t !skip_prefix(line, \"commit \", &p) &&\n \t\t !skip_prefix(line, \"From \", &p) &&\n-\t\t starts_with(line, \"\\\\ \") && 12 < strlen(line))\n+\t\t starts_with(line, \"\\\\ \") && 12 < strlen(line)) {\n+\t\t\tif (verbatim)\n+\t\t\t\tthe_hash_algo->update_fn(&ctx, line, strlen(line));\n \t\t\tcontinue;\n+\t\t}\n \n \t\tif (!get_oid_hex(p, next_oid)) {\n \t\t\tfound_next = 1;\n@@ -152,8 +156,8 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \t\tif (line[0] == '+' || line[0] == ' ')\n \t\t\tafter--;\n \n-\t\t/* Compute the sha without whitespace */\n-\t\tlen = remove_space(line);\n+\t\t/* Add line to hash algo (possibly removing whitespace) */\n+\t\tlen = verbatim ? strlen(line) : remove_space(line);\n \t\tpatchlen += len;\n \t\tthe_hash_algo->update_fn(&ctx, line, len);\n \t}\n@@ -166,7 +170,7 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \treturn patchlen;\n }\n \n-static void generate_id_list(int stable)\n+static void generate_id_list(int stable, int verbatim)\n {\n \tstruct object_id oid, n, result;\n \tint patchlen;\n@@ -174,21 +178,32 @@ static void generate_id_list(int stable)\n \n \toidclr(&oid);\n \twhile (!feof(stdin)) {\n-\t\tpatchlen = get_one_patchid(&n, &result, &line_buf, stable);\n+\t\tpatchlen = get_one_patchid(&n, &result, &line_buf, stable, verbatim);\n \t\tflush_current_id(patchlen, &oid, &result);\n \t\toidcpy(&oid, &n);\n \t}\n \tstrbuf_release(&line_buf);\n }\n \n-static const char patch_id_usage[] = \"git patch-id [--stable | --unstable]\";\n+static const char *const patch_id_usage[] = {\n+\tN_(\"git patch-id [--stable | --unstable | --verbatim]\"), NULL\n+};\n+\n+struct patch_id_opts {\n+\tint stable;\n+\tint verbatim;\n+};\n \n static int git_patch_id_config(const char *var, const char *value, void *cb)\n {\n-\tint *stable = cb;\n+\tstruct patch_id_opts *opts = cb;\n \n \tif (!strcmp(var, \"patchid.stable\")) {\n-\t\t*stable = git_config_bool(var, value);\n+\t\topts->stable = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\tif (!strcmp(var, \"patchid.verbatim\")) {\n+\t\topts->verbatim = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n \n@@ -197,21 +212,29 @@ static int git_patch_id_config(const char *var, const char *value, void *cb)\n \n int cmd_patch_id(int argc, const char **argv, const char *prefix)\n {\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(stable);\n+\t/* if nothing is set, default to unstable */\n+\tstruct patch_id_opts config = {0, 0};\n+\tint opts = 0;\n+\tstruct option builtin_patch_id_options[] = {\n+\t\tOPT_CMDMODE(0, \"unstable\", &opts,\n+\t\t N_(\"use the unstable patch-id algorithm\"), 1),\n+\t\tOPT_CMDMODE(0, \"stable\", &opts,\n+\t\t N_(\"use the stable patch-id algorithm\"), 2),\n+\t\tOPT_CMDMODE(0, \"verbatim\", &opts,\n+\t\t\tN_(\"don't strip whitespace from the patch\"), 3),\n+\t\tOPT_END()\n+\t};\n+\n+\tgit_config(git_patch_id_config, &config);\n+\n+\t/* verbatim implies stable */\n+\tif (config.verbatim)\n+\t\tconfig.stable = 1;\n+\n+\targc = parse_options(argc, argv, prefix, builtin_patch_id_options,\n+\t\t\t patch_id_usage, 0);\n+\n+\tgenerate_id_list(opts ? opts > 1 : config.stable,\n+\t\t\t opts ? opts == 3 : config.verbatim);\n \treturn 0;\n }\ndiff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\nindex cdc5191aa8d..a7fa94ce0a2 100755\n--- a/t/t4204-patch-id.sh\n+++ b/t/t4204-patch-id.sh\n@@ -8,13 +8,13 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\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+\tstr=\"ab cd ef gh ij kl mn op\" &&\n+\ttest_write_lines $str >foo &&\n+\ttest_write_lines $str >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+\ttest_write_lines $str b >foo &&\n+\ttest_write_lines $str b >bar &&\n \tgit commit -a -m first &&\n \tgit checkout -b same main &&\n \tgit commit --amend -m same-msg &&\n@@ -22,8 +22,23 @@ test_expect_success 'setup' '\n \techo c >foo &&\n \techo c >bar &&\n \tgit commit --amend -a -m notsame-msg &&\n+\tgit checkout -b with_space main~ &&\n+\tcat >foo <<-\\EOF &&\n+\ta b\n+\tc d\n+\te f\n+\t g h\n+\t i j\n+\tk l\n+\tm n\n+\top\n+\tEOF\n+\tcp foo bar &&\n+\tgit add foo bar &&\n+\tgit commit --amend -m \"with spaces\" &&\n \ttest_write_lines bar foo >bar-then-foo &&\n \ttest_write_lines foo bar >foo-then-bar\n+\n '\n \n test_expect_success 'patch-id output is well-formed' '\n@@ -128,9 +143,21 @@ test_patch_id_file_order () {\n \tgit format-patch -1 --stdout -O foo-then-bar >format-patch.output &&\n \tcalc_patch_id top-diff.output &&\n+\tcalc_patch_id top-diff.output &&\n+\tcalc_patch_id \n\nPatch-ids for binary patches are found by hashing the object\nids of the before and after objects in succession. However in\nthe --stable case, there is a bug where hunks are not flushed\nfor binary and header-only patch ids, which would always result\nin a patch-id of 0000. The --unstable case is currently correct.\n\nReorder the logic to branch into 3 cases for populating the\npatch body: header-only which populates nothing, binary which\npopulates the object ids, and normal which populates the text\ndiff. All branches will end up flushing the hunk.\n\nDon't populate the ---a/ and +++b/ lines for binary diffs, to correspond\nto those lines not being present in the \"git diff\" text output.\nThis is necessary because we advertise that the patch-id calculated\ninternally and used in format-patch is the same that what the\nbuiltin \"git patch-id\" would produce when piped from a diff.\n\nUpdate the test to run on both binary and normal files.\n\nSigned-off-by: Jerry Zhang \n---\n diff.c | 58 +++++++++++++++++++-------------------\n t/t3419-rebase-patch-id.sh | 34 +++++++++++++++-------\n 2 files changed, 53 insertions(+), 39 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 648f6717a55..c15169e4b06 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6253,46 +6253,46 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid\n \t\tif (p->one->mode == 0) {\n \t\t\tpatch_id_add_string(&ctx, \"newfilemode\");\n \t\t\tpatch_id_add_mode(&ctx, p->two->mode);\n-\t\t\tpatch_id_add_string(&ctx, \"---/dev/null\");\n-\t\t\tpatch_id_add_string(&ctx, \"+++b/\");\n-\t\t\tthe_hash_algo->update_fn(&ctx, p->two->path, len2);\n \t\t} else if (p->two->mode == 0) {\n \t\t\tpatch_id_add_string(&ctx, \"deletedfilemode\");\n \t\t\tpatch_id_add_mode(&ctx, p->one->mode);\n-\t\t\tpatch_id_add_string(&ctx, \"---a/\");\n-\t\t\tthe_hash_algo->update_fn(&ctx, p->one->path, len1);\n-\t\t\tpatch_id_add_string(&ctx, \"+++/dev/null\");\n-\t\t} else {\n-\t\t\tpatch_id_add_string(&ctx, \"---a/\");\n-\t\t\tthe_hash_algo->update_fn(&ctx, p->one->path, len1);\n-\t\t\tpatch_id_add_string(&ctx, \"+++b/\");\n-\t\t\tthe_hash_algo->update_fn(&ctx, p->two->path, len2);\n \t\t}\n \n-\t\tif (diff_header_only)\n-\t\t\tcontinue;\n-\n-\t\tif (fill_mmfile(options->repo, &mf1, p->one) < 0 ||\n-\t\t fill_mmfile(options->repo, &mf2, p->two) < 0)\n-\t\t\treturn error(\"unable to read files to diff\");\n-\n-\t\tif (diff_filespec_is_binary(options->repo, p->one) ||\n+\t\tif (diff_header_only) {\n+\t\t\t/* don't do anything since we're only populating header info */\n+\t\t} else if (diff_filespec_is_binary(options->repo, p->one) ||\n \t\t diff_filespec_is_binary(options->repo, p->two)) {\n \t\t\tthe_hash_algo->update_fn(&ctx, oid_to_hex(&p->one->oid),\n \t\t\t\t\tthe_hash_algo->hexsz);\n \t\t\tthe_hash_algo->update_fn(&ctx, oid_to_hex(&p->two->oid),\n \t\t\t\t\tthe_hash_algo->hexsz);\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\txpp.flags = 0;\n-\t\txecfg.ctxlen = 3;\n-\t\txecfg.flags = XDL_EMIT_NO_HUNK_HDR;\n-\t\tif (xdi_diff_outf(&mf1, &mf2, NULL,\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+\t\t} else {\n+\t\t\tif (p->one->mode == 0) {\n+\t\t\t\tpatch_id_add_string(&ctx, \"---/dev/null\");\n+\t\t\t\tpatch_id_add_string(&ctx, \"+++b/\");\n+\t\t\t\tthe_hash_algo->update_fn(&ctx, p->two->path, len2);\n+\t\t\t} else if (p->two->mode == 0) {\n+\t\t\t\tpatch_id_add_string(&ctx, \"---a/\");\n+\t\t\t\tthe_hash_algo->update_fn(&ctx, p->one->path, len1);\n+\t\t\t\tpatch_id_add_string(&ctx, \"+++/dev/null\");\n+\t\t\t} else {\n+\t\t\t\tpatch_id_add_string(&ctx, \"---a/\");\n+\t\t\t\tthe_hash_algo->update_fn(&ctx, p->one->path, len1);\n+\t\t\t\tpatch_id_add_string(&ctx, \"+++b/\");\n+\t\t\t\tthe_hash_algo->update_fn(&ctx, p->two->path, len2);\n+\t\t\t}\n \n+\t\t\tif (fill_mmfile(options->repo, &mf1, p->one) < 0 ||\n+\t\t\t fill_mmfile(options->repo, &mf2, p->two) < 0)\n+\t\t\t\treturn error(\"unable to read files to diff\");\n+\t\t\txpp.flags = 0;\n+\t\t\txecfg.ctxlen = 3;\n+\t\t\txecfg.flags = XDL_EMIT_NO_HUNK_HDR;\n+\t\t\tif (xdi_diff_outf(&mf1, &mf2, NULL,\n+\t\t\t\t\t patch_id_consume, &data, &xpp, &xecfg))\n+\t\t\t\treturn error(\"unable to generate patch-id diff for %s\",\n+\t\t\t\t\t p->one->path);\n+\t\t}\n \t\tif (stable)\n \t\t\tflush_one_hunk(oid, &ctx);\n \t}\ndiff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh\nindex 295040f2fe3..d24e55aac8d 100755\n--- a/t/t3419-rebase-patch-id.sh\n+++ b/t/t3419-rebase-patch-id.sh\n@@ -43,15 +43,16 @@ test_expect_success 'setup: 500 lines' '\n \tgit add newfile &&\n \tgit commit -q -m \"add small file\" &&\n \n-\tgit cherry-pick main >/dev/null 2>&1\n-'\n+\tgit cherry-pick main >/dev/null 2>&1 &&\n \n-test_expect_success 'setup attributes' '\n-\techo \"file binary\" >.gitattributes\n+\tgit branch -f squashed main &&\n+\tgit checkout -q -f squashed &&\n+\tgit reset -q --soft HEAD~2 &&\n+\tgit commit -q -m squashed\n '\n \n test_expect_success 'detect upstream patch' '\n-\tgit checkout -q main &&\n+\tgit checkout -q main^{} &&\n \tscramble file &&\n \tgit add file &&\n \tgit commit -q -m \"change big file again\" &&\n@@ -61,14 +62,27 @@ test_expect_success 'detect upstream patch' '\n \ttest_must_be_empty revs\n '\n \n+test_expect_success 'detect upstream patch binary' '\n+\techo \"file binary\" >.gitattributes &&\n+\tgit checkout -q other^{} &&\n+\tgit rebase main &&\n+\tgit rev-list main...HEAD~ >revs &&\n+\ttest_must_be_empty revs &&\n+\ttest_when_finished \"rm .gitattributes\"\n+'\n+\n test_expect_success 'do not drop patch' '\n-\tgit branch -f squashed main &&\n-\tgit checkout -q -f squashed &&\n-\tgit reset -q --soft HEAD~2 &&\n-\tgit commit -q -m squashed &&\n \tgit checkout -q other^{} &&\n \ttest_must_fail git rebase squashed &&\n-\tgit rebase --quit\n+\ttest_when_finished \"git rebase --abort\"\n+'\n+\n+test_expect_success 'do not drop patch binary' '\n+\techo \"file binary\" >.gitattributes &&\n+\tgit checkout -q other^{} &&\n+\ttest_must_fail git rebase squashed &&\n+\ttest_when_finished \"git rebase --abort\" &&\n+\ttest_when_finished \"rm .gitattributes\"\n '\n \n test_done\n-- \ngitgitgadget\n\n"},{"id":"465651","messageId":"ec4a2422d5b65efcbe8722f7f25f4b6ef6911302.1666642065.git.gitgitgadget@gmail.com","threadId":"58470","inReplyTo":"pull.1359.v5.git.1666642064.gitgitgadget@gmail.com","subject":"[PATCH v5 2/6] patch-id: use stable patch-id for rebases","fromName":"Jerry Zhang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-24T20:07:40Z","receivedAt":"2022-10-24T21:55:50Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"From: Jerry Zhang \n\nGit doesn't persist patch-ids during the rebase process, so there is\nno need to specifically invoke the unstable variant. Use the stable\nlogic for all internal patch-id calculations to minimize the number of\ncode paths and improve test coverage.\n\nSigned-off-by: Jerry Zhang \n---\n builtin/log.c | 2 +-\n diff.c | 12 ++++--------\n diff.h | 2 +-\n patch-ids.c | 10 +++++-----\n patch-ids.h | 2 +-\n 5 files changed, 12 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex ee19dc5d450..e72869afb36 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1763,7 +1763,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, 1))\n+\t\tif (commit_patch_id(commit, &diffopt, &oid, 0))\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/diff.c b/diff.c\nindex c15169e4b06..199b63dbcc3 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6206,7 +6206,7 @@ static void patch_id_add_mode(git_hash_ctx *ctx, unsigned mode)\n }\n \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+static int diff_get_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only)\n {\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n \tint i;\n@@ -6293,21 +6293,17 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid\n \t\t\t\treturn error(\"unable to generate patch-id diff for %s\",\n \t\t\t\t\t p->one->path);\n \t\t}\n-\t\tif (stable)\n-\t\t\tflush_one_hunk(oid, &ctx);\n+\t\tflush_one_hunk(oid, &ctx);\n \t}\n \n-\tif (!stable)\n-\t\tthe_hash_algo->final_oid_fn(oid, &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, int stable)\n+int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int diff_header_only)\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, stable);\n+\tint result = diff_get_patch_id(options, oid, diff_header_only);\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 8ae18e5ab1e..fd33caeb25d 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -634,7 +634,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option);\n int run_diff_index(struct rev_info *revs, unsigned int option);\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, int);\n+int diff_flush_patch_id(struct diff_options *, struct object_id *, int);\n void flush_one_hunk(struct object_id *result, git_hash_ctx *ctx);\n \n int diff_result_code(struct diff_options *, int);\ndiff --git a/patch-ids.c b/patch-ids.c\nindex 46c6a8f3eab..31534466266 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, int stable)\n+\t\t struct object_id *oid, int diff_header_only)\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, stable);\n+\treturn diff_flush_patch_id(options, oid, diff_header_only);\n }\n \n /*\n@@ -48,11 +48,11 @@ static int patch_id_neq(const void *cmpfn_data,\n \tb = container_of(entry_or_key, struct patch_id, ent);\n \n \tif (is_null_oid(&a->patch_id) &&\n-\t commit_patch_id(a->commit, opt, &a->patch_id, 0, 0))\n+\t commit_patch_id(a->commit, opt, &a->patch_id, 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, 0))\n+\t commit_patch_id(b->commit, opt, &b->patch_id, 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@@ -82,7 +82,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, 0))\n+\tif (commit_patch_id(commit, &ids->diffopts, &header_only_patch_id, 1))\n \t\treturn -1;\n \n \thashmap_entry_init(&patch->ent, oidhash(&header_only_patch_id));\ndiff --git a/patch-ids.h b/patch-ids.h\nindex ab6c6a68047..490d7393716 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, int);\n+\t\t struct object_id *oid, int);\n int init_patch_ids(struct repository *, struct patch_ids *);\n int free_patch_ids(struct patch_ids *);\n \n-- \ngitgitgadget\n\n"},{"id":"465652","messageId":"815013553133cddae5baf9d3dca00f8318e250f7.1666642065.git.gitgitgadget@gmail.com","threadId":"58470","inReplyTo":"pull.1359.v5.git.1666642064.gitgitgadget@gmail.com","subject":"[PATCH v5 3/6] builtin: patch-id: fix patch-id with binary diffs","fromName":"Jerry Zhang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-24T20:07:41Z","receivedAt":"2022-10-24T21:55:54Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"From: Jerry Zhang \n\n\"git patch-id\" currently doesn't produce correct output if the\nincoming diff has any binary files. Add logic to get_one_patchid\nto handle the different possible styles of binary diff. This\nattempts to keep resulting patch-ids identical to what would be\nproduced by the counterpart logic in diff.c, that is it produces\nthe id by hashing the a and b oids in succession.\n\nIn general we handle binary diffs by first caching the object ids from\nthe \"index\" line and using those if we then find an indication\nthat the diff is binary.\n\nThe input could contain patches generated with \"git diff --binary\". This\ncurrently breaks the parse logic and results in multiple patch-ids\noutput for a single commit. Here we have to skip the contents of the\npatch itself since those do not go into the patch id. --binary\nimplies --full-index so the object ids are always available.\n\nWhen the diff is generated with --full-index there is no patch content\nto skip over.\n\nWhen a diff is generated without --full-index or --binary, it will\ncontain abbreviated object ids. This will still result in a sufficiently\nunique patch-id when hashed, but does not match internal patch id\noutput. We'll call this ok for now as we already need specialized\narguments to diff in order to match internal patch id (namely -U3).\n\nSigned-off-by: Jerry Zhang \n---\n builtin/patch-id.c | 36 ++++++++++++++++++++++++++++++++++--\n t/t4204-patch-id.sh | 29 ++++++++++++++++++++++++++++-\n 2 files changed, 62 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex 881fcf32732..e7a31123142 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -61,6 +61,8 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n {\n \tint patchlen = 0, found_next = 0;\n \tint before = -1, after = -1;\n+\tint diff_is_binary = 0;\n+\tchar pre_oid_str[GIT_MAX_HEXSZ + 1], post_oid_str[GIT_MAX_HEXSZ + 1];\n \tgit_hash_ctx ctx;\n \n \tthe_hash_algo->init_fn(&ctx);\n@@ -88,14 +90,44 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \n \t\t/* Parsing diff header? */\n \t\tif (before == -1) {\n-\t\t\tif (starts_with(line, \"index \"))\n+\t\t\tif (starts_with(line, \"GIT binary patch\") ||\n+\t\t\t starts_with(line, \"Binary files\")) {\n+\t\t\t\tdiff_is_binary = 1;\n+\t\t\t\tbefore = 0;\n+\t\t\t\tthe_hash_algo->update_fn(&ctx, pre_oid_str,\n+\t\t\t\t\t\t\t strlen(pre_oid_str));\n+\t\t\t\tthe_hash_algo->update_fn(&ctx, post_oid_str,\n+\t\t\t\t\t\t\t strlen(post_oid_str));\n+\t\t\t\tif (stable)\n+\t\t\t\t\tflush_one_hunk(result, &ctx);\n \t\t\t\tcontinue;\n-\t\t\telse if (starts_with(line, \"--- \"))\n+\t\t\t} else if (skip_prefix(line, \"index \", &p)) {\n+\t\t\t\tchar *oid1_end = strstr(line, \"..\");\n+\t\t\t\tchar *oid2_end = NULL;\n+\t\t\t\tif (oid1_end)\n+\t\t\t\t\toid2_end = strstr(oid1_end, \" \");\n+\t\t\t\tif (!oid2_end)\n+\t\t\t\t\toid2_end = line + strlen(line) - 1;\n+\t\t\t\tif (oid1_end != NULL && oid2_end != NULL) {\n+\t\t\t\t\t*oid1_end = *oid2_end = '\\0';\n+\t\t\t\t\tstrlcpy(pre_oid_str, p, GIT_MAX_HEXSZ + 1);\n+\t\t\t\t\tstrlcpy(post_oid_str, oid1_end + 2, GIT_MAX_HEXSZ + 1);\n+\t\t\t\t}\n+\t\t\t\tcontinue;\n+\t\t\t} else if (starts_with(line, \"--- \"))\n \t\t\t\tbefore = after = 1;\n \t\t\telse if (!isalpha(line[0]))\n \t\t\t\tbreak;\n \t\t}\n \n+\t\tif (diff_is_binary) {\n+\t\t\tif (starts_with(line, \"diff \")) {\n+\t\t\t\tdiff_is_binary = 0;\n+\t\t\t\tbefore = -1;\n+\t\t\t}\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\t/* Looking for a valid hunk header? */\n \t\tif (before == 0 && after == 0) {\n \t\t\tif (starts_with(line, \"@@ -\")) {\ndiff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh\nindex a730c0db985..cdc5191aa8d 100755\n--- a/t/t4204-patch-id.sh\n+++ b/t/t4204-patch-id.sh\n@@ -42,7 +42,7 @@ calc_patch_id () {\n }\n \n get_top_diff () {\n-\tgit log -p -1 \"$@\" -O bar-then-foo --\n+\tgit log -p -1 \"$@\" -O bar-then-foo --full-index --\n }\n \n get_patch_id () {\n@@ -61,6 +61,33 @@ test_expect_success 'patch-id detects inequality' '\n \tget_patch_id notsame &&\n \t! test_cmp patch-id_main patch-id_notsame\n '\n+test_expect_success 'patch-id detects equality binary' '\n+\tcat >.gitattributes <<-\\EOF &&\n+\tfoo binary\n+\tbar binary\n+\tEOF\n+\tget_patch_id main &&\n+\tget_patch_id same &&\n+\tgit log -p -1 --binary main >top-diff.output &&\n+\tcalc_patch_id top-diff.output &&\n+\tcalc_patch_id .gitattributes <<-\\EOF &&\n+\tfoo binary\n+\tbar binary\n+\tEOF\n+\tget_patch_id main &&\n+\tget_patch_id notsame &&\n+\t! test_cmp patch-id_main patch-id_notsame &&\n+\ttest_when_finished \"rm .gitattributes\"\n+'\n \n test_expect_success 'patch-id supports git-format-patch output' '\n \tget_patch_id main &&\n-- \ngitgitgadget\n\n"},{"id":"465653","messageId":"pull.1359.v5.git.1666642064.gitgitgadget@gmail.com","threadId":"58470","inReplyTo":"pull.1359.v4.git.1666307815.gitgitgadget@gmail.com","subject":"[PATCH v5 0/6] patch-id fixes and improvements","fromName":"Jerry Zhang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-24T20:07:38Z","receivedAt":"2022-10-24T22:00:55Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"These patches add fixes and features to the \"git patch-id\" command, mostly\ndiscovered through our usage of patch-id in the revup project\n(https://github.com/Skydio/revup). On top of that I've tried to make general\ncleanup changes where I can.\n\nSummary:\n\n1: Fixed a bug in the combination of --stable with binary files and\nheader-only, and expanded the test to cover both binary and non-binary\nfiles.\n\n2: Switch internal usage of patch-id in rebase / cherry-pick to use the\nstable variant to reduce the number of code paths and improve testing for\nbugs like above.\n\n3: Fixed bugs with patch-id and binary diffs. Previously patch-id did not\nbehave correctly for binary diffs regardless of whether \"--binary\" was given\nto \"diff\".\n\n4: Fixed bugs with patch-id and mode changes. Previously mode changes were\nincorrectly excluded from the patch-id.\n\n5: Add a new \"--include-whitespace\" mode to patch-id that prevents\nwhitespace from being stripped during id calculation. Also add a config\noption for the same behavior.\n\n6: Remove unused prefix from patch-id logic.\n\nV1->V2: Fixed comment style V2->V3: The ---/+++ lines no longer get added to\nthe patch-id of binary diffs. Also added patches 3-7 in the series. V3->V4:\nDropped patch7. Updated flag name to --verbatim. Updated commit message\ndescriptions. V4->V5: Updated commit message for patch 6.\n\nSigned-off-by: Jerry Zhang jerry@skydio.com\n\nJerry Zhang (6):\n patch-id: fix stable patch id for binary / header-only\n patch-id: use stable patch-id for rebases\n builtin: patch-id: fix patch-id with binary diffs\n patch-id: fix patch-id for mode changes\n builtin: patch-id: add --verbatim as a command mode\n builtin: patch-id: remove unused diff-tree prefix\n\n Documentation/git-patch-id.txt | 24 ++++---\n builtin/log.c | 2 +-\n builtin/patch-id.c | 113 ++++++++++++++++++++++++---------\n diff.c | 75 +++++++++++-----------\n diff.h | 2 +-\n patch-ids.c | 10 +--\n patch-ids.h | 2 +-\n t/t3419-rebase-patch-id.sh | 63 +++++++++++++++---\n t/t4204-patch-id.sh | 95 +++++++++++++++++++++++++--\n 9 files changed, 287 insertions(+), 99 deletions(-)\n\n\nbase-commit: 45c9f05c44b1cb6bd2d6cb95a22cf5e3d21d5b63\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1359%2Fjerry-skydio%2Fjerry%2Frevup%2Fmaster%2Fpatch_ids-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1359/jerry-skydio/jerry/revup/master/patch_ids-v5\nPull-Request: https://github.com/gitgitgadget/git/pull/1359\n\nRange-diff vs v4:\n\n 1: 321757ef919 = 1: 321757ef919 patch-id: fix stable patch id for binary / header-only\n 2: ec4a2422d5b = 2: ec4a2422d5b patch-id: use stable patch-id for rebases\n 3: 81501355313 = 3: 81501355313 builtin: patch-id: fix patch-id with binary diffs\n 4: bb0b4add03c = 4: bb0b4add03c patch-id: fix patch-id for mode changes\n 5: b160f2ae49f = 5: b160f2ae49f builtin: patch-id: add --verbatim as a command mode\n 6: dcdfac7a153 ! 6: eef2a32f008 builtin: patch-id: remove unused diff-tree prefix\n @@ Metadata\n ## Commit message ##\n builtin: patch-id: remove unused diff-tree prefix\n \n - From a \"git grep\" of the repo, no command, including diff-tree itself,\n - produces diff output with \"diff-tree \" prefixed in the header.\n + The last git version that had \"diff-tree\" in the header text\n + of \"git diff-tree\" output was v1.3.0 from 2006. The header text\n + was changed from \"diff-tree\" to \"commit\" in 91539833\n + (\"Log message printout cleanups\").\n \n - Thus remove its handling in \"patch-id\".\n + Given how long ago this change was made, it is highly unlikely that\n + anyone is still feeding in outputs from that git version.\n +\n + Remove the handling of the \"diff-tree\" prefix and document the\n + source of the other prefixes so that the overall functionality\n + is more clear.\n \n Signed-off-by: Jerry Zhang \n \n\n-- \ngitgitgadget\n"},{"id":"465656","messageId":"eef2a32f008899cec6d66891f830907c26573a55.1666642065.git.gitgitgadget@gmail.com","threadId":"58470","inReplyTo":"pull.1359.v5.git.1666642064.gitgitgadget@gmail.com","subject":"[PATCH v5 6/6] builtin: patch-id: remove unused diff-tree prefix","fromName":"Jerry Zhang via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-24T20:07:44Z","receivedAt":"2022-10-24T22:08:40Z","isPatch":true,"sender":{"key":"jerry@skydio.com","avatar":"https://avatars.githubusercontent.com/u/81337184?v=4"},"body":"From: Jerry Zhang \n\nThe last git version that had \"diff-tree\" in the header text\nof \"git diff-tree\" output was v1.3.0 from 2006. The header text\nwas changed from \"diff-tree\" to \"commit\" in 91539833\n(\"Log message printout cleanups\").\n\nGiven how long ago this change was made, it is highly unlikely that\nanyone is still feeding in outputs from that git version.\n\nRemove the handling of the \"diff-tree\" prefix and document the\nsource of the other prefixes so that the overall functionality\nis more clear.\n\nSigned-off-by: Jerry Zhang \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 afdd472369f..f840fbf1c7e 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -74,8 +74,8 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \t\tconst char *p = line;\n \t\tint len;\n \n-\t\tif (!skip_prefix(line, \"diff-tree \", &p) &&\n-\t\t !skip_prefix(line, \"commit \", &p) &&\n+\t\t/* Possibly skip over the prefix added by \"log\" or \"format-patch\" */\n+\t\tif (!skip_prefix(line, \"commit \", &p) &&\n \t\t !skip_prefix(line, \"From \", &p) &&\n \t\t starts_with(line, \"\\\\ \") && 12 < strlen(line)) {\n \t\t\tif (verbatim)\n-- \ngitgitgadget\n"},{"id":"465665","messageId":"xmqqilk8rfvk.fsf@gitster.g","threadId":"58470","inReplyTo":"pull.1359.v5.git.1666642064.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 0/6] patch-id fixes and improvements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-24T22:55:27Z","receivedAt":"2022-10-25T00:32:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Jerry Zhang via GitGitGadget\" writes:\n\n> These patches add fixes and features to the \"git patch-id\" command, mostly\n> discovered through our usage of patch-id in the revup project\n> (https://github.com/Skydio/revup). On top of that I've tried to make general\n> cleanup changes where I can.\n\nThanks, let's move it forward.\n"}]}