{"thread":{"id":"60411","subject":"[RFC PATCH 0/2] pretty: add %I formatting for patch-id","startedAt":"2023-10-22T02:28:14Z","lastAt":"2023-10-25T08:44:35Z","messageCount":4,"participants":["Michael McClimon","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"483624","messageId":"20231022022800.69219-1-michael@mcclimon.org","threadId":"60411","inReplyTo":null,"subject":"[RFC PATCH 0/2] pretty: add %I formatting for patch-id","fromName":"Michael McClimon","fromEmail":"michael@mcclimon.org","sentAt":"2023-10-22T02:27:58Z","receivedAt":"2023-10-22T02:28:14Z","isPatch":true,"sender":{"key":"michael@mcclimon.org","avatar":"https://avatars.githubusercontent.com/u/1698723?v=4"},"body":"I'll say up front that this patch doesn't actually work, but I am stuck\non it and interested in continuing, and am hopeful someone can point me\nin something closer to the right direction. (Also, the proposed log\nmessages are not good, and I would not actually submit the patch for\nmerge consideration with these messages.)\n\nI would like to have a single-command way to get the patch id for a\ncommit: the thing you'd see in a pipeline like\n    git diff-tree --patch-with-raw HEAD | git patch-id\n\nMy initial thought was to add a --patch-id flag to git diff-tree, but\nthen I thought that maybe better would be to add a pretty specifier to\ndo so, so that (for instance) you could generate patch-ids for\neverything in a branch by saying something like\n    git log --pretty='%I %H' start..\n\nI have taken a pass at doing so here, but it doesn't _work_, and I'm not\nsure why. It seemed like the thing to do was to use commit_patch_id from\npatch-ids.c, but I think that either I'm not holding it correctly, or\nthat it's not fit for purpose. It was added (or rather, made public) in\nded2c097 (patch-ids: make commit_patch_id() a public helper function,\n2016-04-26), which was in service of fa2ab86d (format-patch: add\n'--base' option to record base tree info, 2016-04-26). It seems like at\nthe very least, it probably won't work as expected on merge commits.\n\nThe thing that is perplexing to me is that it _does_ appear to work on\nsome commits, for example (where 8b3aa36f is a recent-ish commit from\nmaster, chosen at random):\n\n$ ./git --no-pager show -s --format='%I %H' 8b3aa36f5a7a0c923bc4a28ff19caae78644ae08\nec66952bdef82a1fc6d31c1057195af31c86da48 8b3aa36f5a7a0c923bc4a28ff19caae78644ae08\n\n$ git diff-tree --patch-with-raw 8b3aa36f5a7a0c923bc4a28ff19caae78644ae08 | git patch-id\nec66952bdef82a1fc6d31c1057195af31c86da48 8b3aa36f5a7a0c923bc4a28ff19caae78644ae08\n\nBut for other commits, like the one in the test here, it does not. I\nhave done a bit of investigation, but I would not really call myself a C\nprogrammer and I'm not super familiar with the codebase, so I'm a bit\nstuck. I thought maybe at first I wasn't initializing the diff_options\ncorrectly, but I suspect the problem is actually more fundamental than\nthat.\n\nAnyway: I would be happy to hear any response at all, whether that's \na) a pointer to some other way of implementing a %I pretty format,\nb) an easier-to-implement possible solution (maybe diff-tree --patch-id\nwould actually be better), or c) saying the thing I want isn't feasible\nand I should stick with the pipeline. Thanks!\n\n\nMichael McClimon (2):\n  patch-ids: add const modifier to commit\n  pretty: add 'I' placeholder for patch-id\n\n patch-ids.c                   |  4 ++--\n patch-ids.h                   |  2 +-\n pretty.c                      | 11 +++++++++++\n t/t4205-log-pretty-formats.sh |  7 +++++++\n 4 files changed, 21 insertions(+), 3 deletions(-)\n\n-- \n2.42.0.424.gceadf0f3\n\n"},{"id":"483625","messageId":"20231022022800.69219-2-michael@mcclimon.org","threadId":"60411","inReplyTo":"20231022022800.69219-1-michael@mcclimon.org","subject":"[RFC PATCH 1/2] patch-ids: add const modifier to commit","fromName":"Michael McClimon","fromEmail":"michael@mcclimon.org","sentAt":"2023-10-22T02:27:59Z","receivedAt":"2023-10-22T02:28:15Z","isPatch":true,"sender":{"key":"michael@mcclimon.org","avatar":"https://avatars.githubusercontent.com/u/1698723?v=4"},"body":"These aren't modified by these functions, and I want to use them\nsomewhere where the commit is in fact a const.\n\nSigned-off-by: Michael McClimon <michael@mcclimon.org>\n---\n patch-ids.c | 4 ++--\n patch-ids.h | 2 +-\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/patch-ids.c b/patch-ids.c\nindex c3e1a0dd..ecfd7ba0 100644\n--- a/patch-ids.c\n+++ b/patch-ids.c\n@@ -6,13 +6,13 @@\n #include \"hex.h\"\n #include \"patch-ids.h\"\n \n-static int patch_id_defined(struct commit *commit)\n+static int patch_id_defined(const struct commit *commit)\n {\n \t/* must be 0 or 1 parents */\n \treturn !commit->parents || !commit->parents->next;\n }\n \n-int commit_patch_id(struct commit *commit, struct diff_options *options,\n+int commit_patch_id(const struct commit *commit, struct diff_options *options,\n \t\t    struct object_id *oid, int diff_header_only)\n {\n \tif (!patch_id_defined(commit))\ndiff --git a/patch-ids.h b/patch-ids.h\nindex 490d7393..3f61d88a 100644\n--- a/patch-ids.h\n+++ b/patch-ids.h\n@@ -19,7 +19,7 @@ struct patch_ids {\n \tstruct diff_options diffopts;\n };\n \n-int commit_patch_id(struct commit *commit, struct diff_options *options,\n+int commit_patch_id(const struct commit *commit, struct diff_options *options,\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-- \n2.42.0.424.gceadf0f3\n\n"},{"id":"483626","messageId":"20231022022800.69219-3-michael@mcclimon.org","threadId":"60411","inReplyTo":"20231022022800.69219-1-michael@mcclimon.org","subject":"[RFC PATCH 2/2] pretty: add 'I' placeholder for patch-id","fromName":"Michael McClimon","fromEmail":"michael@mcclimon.org","sentAt":"2023-10-22T02:28:00Z","receivedAt":"2023-10-22T02:28:17Z","isPatch":true,"sender":{"key":"michael@mcclimon.org","avatar":"https://avatars.githubusercontent.com/u/1698723?v=4"},"body":"This doesn't actually work yet, and the test is probably in the wrong\nplace, but I think it's sort of close enough to send an RFC patch to ask\nsome questions.\n\nSigned-off-by: Michael McClimon <michael@mcclimon.org>\n---\n pretty.c                      | 11 +++++++++++\n t/t4205-log-pretty-formats.sh |  7 +++++++\n 2 files changed, 18 insertions(+)\n\ndiff --git a/pretty.c b/pretty.c\nindex cf964b06..47e2e6e9 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -19,6 +19,7 @@\n #include \"trailer.h\"\n #include \"run-command.h\"\n #include \"object-name.h\"\n+#include \"patch-ids.h\"\n \n /*\n  * The limit for formatting directives, which enable the caller to append\n@@ -1571,6 +1572,16 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\t\t\t\t c->pretty_ctx->abbrev);\n \t\t}\n \t\treturn 1;\n+\tcase 'I':\n+\t\t{\n+\t\t\tstruct diff_options diffopt;\n+\t\t\tstruct object_id patch_id;\n+\t\t\trepo_diff_setup(the_repository, &diffopt);\n+\t\t\tif (commit_patch_id(commit, &diffopt, &patch_id, 0))\n+\t\t\t\tdie(_(\"cannot get patch id\"));\n+\t\t\tstrbuf_addstr(sb, oid_to_hex(&patch_id));\n+\t\t\treturn 1;\n+\t\t}\n \tcase 'm':\t\t/* left/right/bottom */\n \t\tstrbuf_addstr(sb, get_revision_mark(NULL, commit));\n \t\treturn 1;\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex e3d655e6..1e9fdcfe 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -38,6 +38,13 @@ test_expect_success 'set up basic repos' '\n \tgit config --unset i18n.commitEncoding\n '\n \n+# %I placeholder\n+test_expect_success '%I placeholder is a patch-id' '\n+\tgit diff-tree --patch-with-raw HEAD | git patch-id >expected &&\n+\tgit show -s --pretty=\"%I %H\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'alias builtin format' '\n \tgit log --pretty=oneline >expected &&\n \tgit config pretty.test-alias oneline &&\n-- \n2.42.0.424.gceadf0f3\n\n"},{"id":"483845","messageId":"20231025084432.GC2145145@coredump.intra.peff.net","threadId":"60411","inReplyTo":"20231022022800.69219-1-michael@mcclimon.org","subject":"Re: [RFC PATCH 0/2] pretty: add %I formatting for patch-id","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-25T08:44:32Z","receivedAt":"2023-10-25T08:44:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 21, 2023 at 10:27:58PM -0400, Michael McClimon wrote:\n\n> I would like to have a single-command way to get the patch id for a\n> commit: the thing you'd see in a pipeline like\n>     git diff-tree --patch-with-raw HEAD | git patch-id\n\nSo I actually don't think this is the worst thing in the world. You are\nusing two plumbing commands as intended. And if you want to get N\npatch-ids, you can do it with a constant number of processes like:\n\n  git rev-list HEAD | git diff-tree --stdin -p | git patch-id\n\nAlternatively, I wonder if patch-id could generate the ids internally\n(which we certainly have code for; that's how git-cherry, etc, work).\nWould it make sense to teach patch-id:\n\n  1. To accept just commit id's on stdin to produce the patches for\n     those commit id's against their parents, without receiving the\n     patch text itself on stdin. I guess --commits-only or something.\n\n  2. To accept commit-ishes on the command line, overriding the need to\n     read from stdin.\n\nWhich would let your single-patch case become just:\n\n  git patch-id --commits-only HEAD\n\nBut let's read on...\n\n> My initial thought was to add a --patch-id flag to git diff-tree, but\n> then I thought that maybe better would be to add a pretty specifier to\n> do so, so that (for instance) you could generate patch-ids for\n> everything in a branch by saying something like\n>     git log --pretty='%I %H' start..\n\nI can see how \"%I\" would be convenient, especially from a scripting\nstandpoint, since you can control how it appears in the output. But it\ndoes feel a little funny to me as a commit pretty-printing item.\nComputing it requires calculating the diff, which we'll of course want\nto do separately for the diff portion of many invocations. E.g., I\ndidn't check, but I'd guess that:\n\n  git log --pretty=\"%I %H\" -p\n\nwould compute each diff twice. Would it make more sense as a\ndiff-format alongside --raw, --name-status, and so on?\n\nI have a more subtle reason to think this might be a good idea, which\nI'll discuss below.\n\n> The thing that is perplexing to me is that it _does_ appear to work on\n> some commits, for example (where 8b3aa36f is a recent-ish commit from\n> master, chosen at random):\n> [...]\n> But for other commits, like the one in the test here, it does not.\n\nHmm. When I run the failing test in a debugger, I see that going into\ncommit_patch_id(), there is already an entry in diff_queued_diff (which\nis a global that stores the result of the last diff). So I think there\nis already a diff there from the call to diff_tree_oid() in\nlog_tree_diff(). And then you diff _again_ and end up with duplicates\nwhen computing the patch-id.\n\n(This causes another bug, too; at the end of diff_flush_patch_id we\nclear the queue, so the actual diff is not showed by \"git show\" anymore;\nthe use of \"-s\" in the test obscures that).\n\nThe thing that most surprises me is that it ever works at all. ;) But I\nthink what happens is this:\n\n  - in the code in your patch, you fail to set up the diff_options\n    correctly. In particular, you do not set the \"recursive\" flag.\n\n  - so in the case of 8b3aa36f, which touches only a file in\n    Documentation/, the call from log_tree_diff() queues the correct\n    file modification entry. The second call from commit_patch_id()\n    mistakenly queues only the parent tree \"Documentation\"\n\n  - a patch-id is defined on the textual patch changes, so it ignores\n    non-file changes that are queued (and in Git you wouldn't normally\n    see these anyway unless you use a non-recursive diff or a special\n    option like \"-t\").\n\nAnd hence any patch which consists of changes to files only in subtrees\nwill \"work\", and other ones fail. To \"fix\" your patch, you could set\ndiffopt.flags.recursive, as we do in format-patch's prepare_bases()\ncall. But of course because of the duplicate diff, that just means we'll\nconsistently get the wrong answer now. ;)\n\nSo I'd expect something like this to work:\n\ndiff --git a/pretty.c b/pretty.c\nindex 47e2e6e99a..5a473c7941 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -20,6 +20,7 @@\n #include \"run-command.h\"\n #include \"object-name.h\"\n #include \"patch-ids.h\"\n+#include \"diffcore.h\"\n \n /*\n  * The limit for formatting directives, which enable the caller to append\n@@ -1576,10 +1577,28 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t{\n \t\t\tstruct diff_options diffopt;\n \t\t\tstruct object_id patch_id;\n+\n+\t\t\t/*\n+\t\t\t * save the existing diff queue, as it may be\n+\t\t\t * populated, e.g. by log_tree_diff(). Of course\n+\t\t\t * it would be nice to use its results, but:\n+\t\t\t *\n+\t\t\t *   1. We can't count on it (we might be formatting a\n+\t\t\t *      via some other code path).\n+\t\t\t *   2. We have no clue which diff options were used.\n+\t\t\t */\n+\t\t\tstruct diff_queue_struct old_queue = diff_queued_diff;\n+\t\t\tDIFF_QUEUE_CLEAR(&diff_queued_diff);\n+\n \t\t\trepo_diff_setup(the_repository, &diffopt);\n+\t\t\tdiffopt.flags.recursive = 1;\n+\t\t\tdiff_setup_done(&diffopt);\n+\n \t\t\tif (commit_patch_id(commit, &diffopt, &patch_id, 0))\n \t\t\t\tdie(_(\"cannot get patch id\"));\n \t\t\tstrbuf_addstr(sb, oid_to_hex(&patch_id));\n+\n+\t\t\tdiff_queued_diff = old_queue;\n \t\t\treturn 1;\n \t\t}\n \tcase 'm':\t\t/* left/right/bottom */\n\nbut it doesn't quite fix your test. There I think the issue is that\nthe patch-id code is a bit wonky for the special case of the creation of\nan empty file (even before your patch). The internal patch-id generator\nadds fake:\n\n  --- /dev/null\n  +++ b/whatever\n\nlines that would normally appear in the patch for a newly created file.\nBut for an empty file, the \"--patch\" format does not emit those lines at\nall! So we need this hackery on top:\n\ndiff --git a/diff.c b/diff.c\nindex 2c602df10a..9370e585f1 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6431,15 +6431,15 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid\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} else {\n-\t\t\tif (p->one->mode == 0) {\n+\t\t\tif (p->one->mode == 0 && p->two->size) {\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} else if (p->two->mode == 0 && p->one->size) {\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} else if (p->one->size || p->two->size) {\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\nAnd with that, your new test succeeds.\n\n> have done a bit of investigation, but I would not really call myself a C\n> programmer and I'm not super familiar with the codebase, so I'm a bit\n> stuck. I thought maybe at first I wasn't initializing the diff_options\n> correctly, but I suspect the problem is actually more fundamental than\n> that.\n\nSo as you can see, the diff options do matter. And that is a more\nfundamental issue.  In many cases patch-id will be pretty stable and\nindependent of options, but I suspect it would get confused by things\nlike rename detection.\n\nIf we run \"git log --format=%I\", which diff options should be used for\nthe patch-id? A stable set (like we do for \"format-patch --base\")? Or\nones that match the diff we are otherwise generating? I could see\narguments both ways.\n\nOne nice thing about \"diff-tree | patch-id\" is that you are in control\nof the options that diff-tree uses. But I'd think in most cases you just\nwant a stable patch-id, so the \"git patch-id HEAD\" I suggested earlier\nwould presumably use some very basic set of diff options.\n\nI dunno. I do not use patch-id much (aside from internal use via things\nlike \"git cherry\", etc). So there may be use cases or corner cases I'm\nnot thinking about. But I do think there's a bit of complexity to think\nabout here.\n\n-Peff\n"}]}