{"thread":{"id":"55295","subject":"[BUG] cmd given to \"rebase -x cmd\" cannot see the original HEAD?","startedAt":"2021-03-11T20:12:33Z","lastAt":"2021-03-12T12:27:44Z","messageCount":8,"participants":["Junio C Hamano","Phillip Wood","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"418902","messageId":"xmqq8s6tcuxc.fsf@gitster.g","threadId":"55295","inReplyTo":null,"subject":"[BUG] cmd given to \"rebase -x cmd\" cannot see the original HEAD?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-11T20:11:27Z","receivedAt":"2021-03-11T20:12:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I was trying to figure out why I am losing the amlog note that\nmaps each commit on topics to the message ID after amending them\nwith extra trailers (e.g. Reviewed-by).  I use a script that runs\n\"git commit --amend\" to do this and give it to \"rebase -x\".\n\nIt seems that when the command given to \"rebase -x\" is run, HEAD is\nalready updated to a commit with tree and log message identical to\nthe original, with updated committer information.  However, the\nnote attached to the original commit is *not* copied to this\nintermediate HEAD that is shown to the external command.\n\nHence, even if I try to read the existing note from HEAD that is\ngiven to the script spawned via \"rebase -x cmd\" mechanism, the\nscript cannot copy the note to the resulting commit after it adds\nextra trailers.  Also, there is no mechansim for the external\ncommand to learn where the original HEAD was.\n\nI see a few possible avenues to fix this.\n\n1. Before \"rebase -x\" mechanism spawns an external command, make\n   sure that the \"copy notes upon rewriting\" logic has triggered, so\n   that the HEAD observable by the command has the notes from the\n   original.  It seems that the \"copy notes\" logic is only used at\n   the end, but it does not seem to take the possibility into\n   account that the external command might move HEAD to a different\n   commit (i.e. it starts from HEAD0, creates HEAD1 that is\n   different from HEAD0 only for committer info, hands HEAD1 to\n   external command that may move HEAD to HEAD2, but then copies the\n   note from HEAD0 to HEAD1 because it is unaware of HEAD2), so it\n   only annotates HEAD1 that is immediately lost (nothing other than\n   HEAD's reflog would be pointing at it). [*]\n\n2. Make sure the \"copy notes\" logic copies the notes taken from the\n   original HEAD to the HEAD _after_ \"rebase -x\" external command\n   returns.\n\nThe first approach would still make the external command responsible\nfor copying notes from HEAD it sees (i.e. HEAD1) to the new commit\nit creates and points HEAD at (i.e. HEAD2).  It _could_ be used as a\nmechanism to allow the external command to modify/remove notes.\n\nThe second approach would forbid the external command from mucking\nwith notes at all.  \"rebase\" would first remember the original HEAD\n(i.e. HEAD0), does its thing (i.e. creates HEAD1 and points HEAD at\nit, and then lets the external command create HEAD2 and point HEAD\nat it), and then figure out the latest HEAD and copy notes from the\noriginal HEAD.  It is simpler, works with scripts that uses things\nlike \"commit --amend\" that do not copy notes, but less flexible.\n\nImplementation-wise, it seems that the latter would require more\nsignificant surgery to do_pick_commit() codepath, but somebody more\nfamiliar with the sequencer codepath should research to find the\nbest approach.\n\nI suspect that \"post-commit\" hook that is run from try_to_commit()\nmay have the same issue, here in sequencer.c\n\n1539:\trun_commit_hook(0, r->index_file, \"post-commit\", NULL);\n1540:\tif (flags & AMEND_MSG)\n1541:\t\tcommit_post_rewrite(r, current_head, oid);\n\nwhere oid is not adjusted by re-reading what the post-commit hook\ndid to our HEAD.\n\nBut the codepath to reach do_exec() (which is what spawns the\nexternal command for \"rebase -x cmd\") is quite away from any\ncall to commit_post_rewrite(), \n\n\n\n[footnote]\n\nI have a 6-commit series with notes/amlog record on each commit, and\nrunning:\n\n$ git rebase -x '\n\tgit rev-parse HEAD\n\tgit notes --ref=notes/amlog show HEAD || :\n' master $(git rev-parse ab/read-tree)\n\nyields the following:\n\nExecuting: git rev-parse HEAD; git notes --ref=notes/amlog show HEAD || :\n669950d7def2e849cb1ee6e7b3a1beac5c45ce1c\nerror: no note found for object 669950d7def2e849cb1ee6e7b3a1beac5c45ce1c.\nExecuting: git rev-parse HEAD; git notes --ref=notes/amlog show HEAD || :\n0545e0a567f96eff14dfb93b44aecf9683b44803\nerror: no note found for object 0545e0a567f96eff14dfb93b44aecf9683b44803.\nExecuting: git rev-parse HEAD; git notes --ref=notes/amlog show HEAD || :\nd9b4e77efe5e65952e05533ebf500b559408d436\nerror: no note found for object d9b4e77efe5e65952e05533ebf500b559408d436.\nExecuting: git rev-parse HEAD; git notes --ref=notes/amlog show HEAD || :\n6d8735372c0c9b9d3793959157456cd445d025fa\nerror: no note found for object 6d8735372c0c9b9d3793959157456cd445d025fa.\nExecuting: git rev-parse HEAD; git notes --ref=notes/amlog show HEAD || :\nb86c1305258a11f42927f924498a94b3ffb19472\nerror: no note found for object b86c1305258a11f42927f924498a94b3ffb19472.\nExecuting: git rev-parse HEAD; git notes --ref=notes/amlog show HEAD || :\n7da85bae9f11ab14d616bcade1fb2a7b434aa716\nerror: no note found for object 7da85bae9f11ab14d616bcade1fb2a7b434aa716.\nSuccessfully rebased and updated detached HEAD.\n\nWe can see each commit on ab/read-tree has notes/amlog entry that\nrecords where it came from:\n\n$ git log --notes=notes/amlog --oneline --reverse master..ab/read-tree\n2e3e38a4ad ls-files tests: add meaningful --with-tree tests\nNotes (amlog):\n    Message-Id: <20210308022138.28166-2-avarab@gmail.com>\n\n2371fda438 tree.c API: move read_tree() into builtin/ls-files.c\nNotes (amlog):\n    Message-Id: <20210308022138.28166-3-avarab@gmail.com>\n\n32c718db53 ls-files: don't needlessly pass around stage variable\nNotes (amlog):\n    Message-Id: <20210308022138.28166-4-avarab@gmail.com>\n\n2ebabd63f7 ls-files: refactor away read_tree()\nNotes (amlog):\n    Message-Id: <20210308022138.28166-5-avarab@gmail.com>\n\ne98fb6ddc7 tree.h API: remove support for starting at prefix != \"\"\nNotes (amlog):\n    Message-Id: <20210308022138.28166-6-avarab@gmail.com>\n\n8e88702431 tree.h API: remove \"stage\" parameter from read_tree_recursive()\nNotes (amlog):\n    Message-Id: <20210308022138.28166-7-avarab@gmail.com>\n\n\nIn the above transcript, 669950d7def2e8 (the first commit that the\n\"rebase -x\" command saw) corresponds to 2e3e38a4ad (the first commit\nin the series), and we can see the only difference between them is\nthe committer info.  As we already saw in the above, the original\nhas a note that is not copied to the new one when \"rebase -x\"\ncommand runs.\n\n$ git show --pretty=fuller 2e3e38a4ad >old\n$ git show --pretty=fuller 669950d7def2e8 >new\n$ diff -u old new\n--- old\t2021-03-11 12:06:33.259434213 -0800\n+++ new\t2021-03-11 12:06:33.259434213 -0800\n@@ -1,8 +1,8 @@\n-commit 2e3e38a4ad6d9d78381e90348289277acb4c6f8b\n+commit 669950d7def2e849cb1ee6e7b3a1beac5c45ce1c\n Author:     Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n AuthorDate: Mon Mar 8 03:21:33 2021 +0100\n Commit:     Junio C Hamano <gitster@pobox.com>\n-CommitDate: Mon Mar 8 10:06:35 2021 -0800\n+CommitDate: Thu Mar 11 12:01:11 2021 -0800\n \n     ls-files tests: add meaningful --with-tree tests\n     \n\nHOWEVER, after that failed \"rebase -x\" session, the note is copied\nto these \"intermediate\" commits.  Using 7da85bae9f1 which is at the\ntip of the history fed to \"rebase -x\" external command, we can see:\n\n$ git log --notes=notes/amlog --oneline --reverse master..7da85bae9f1\n\n669950d7de ls-files tests: add meaningful --with-tree tests\nNotes (amlog):\n    Message-Id: <20210308022138.28166-2-avarab@gmail.com>\n\n0545e0a567 tree.c API: move read_tree() into builtin/ls-files.c\nNotes (amlog):\n    Message-Id: <20210308022138.28166-3-avarab@gmail.com>\n\nd9b4e77efe ls-files: don't needlessly pass around stage variable\nNotes (amlog):\n    Message-Id: <20210308022138.28166-4-avarab@gmail.com>\n\n6d8735372c ls-files: refactor away read_tree()\nNotes (amlog):\n    Message-Id: <20210308022138.28166-5-avarab@gmail.com>\n\nb86c130525 tree.h API: remove support for starting at prefix != \"\"\nNotes (amlog):\n    Message-Id: <20210308022138.28166-6-avarab@gmail.com>\n\n7da85bae9f tree.h API: remove \"stage\" parameter from read_tree_recursive()\nNotes (amlog):\n    Message-Id: <20210308022138.28166-7-avarab@gmail.com>\n\n\nIt is only that they seem to be added _after_ the external command\nfinishes, which is not a very useful behaviour.\n"},{"id":"418926","messageId":"20210312030107.1849942-1-gitster@pobox.com","threadId":"55295","inReplyTo":"xmqq8s6tcuxc.fsf@gitster.g","subject":"[PATCH 0/2] \"rebase -x cmd\" loses notes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-12T03:01:05Z","receivedAt":"2021-03-12T03:01:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"So here is a semi-failed attempt to fix the issue I encountered\nearlier, cf. https://lore.kernel.org/git/xmqq8s6tcuxc.fsf@gitster.g/\nwhere \"git rebase\" carries forward notes attached to the commits in\nthe original history to their equivalents in the rewritten history,\nbut \"git rebase -x cmd\" does not when cmd modifies the given HEAD\ncommit.\n\n\nJunio C Hamano (2):\n  sequencer.c: make commit_post_rewrite() take two object names\n  [WIP] sequencer.c: carry forward notes on HEAD across \"rebase -x\"\n\n builtin/commit.c              |  6 +++---\n sequencer.c                   | 19 +++++++++++++------\n sequencer.h                   |  2 +-\n t/t3404-rebase-interactive.sh | 18 ++++++++++++++++++\n 4 files changed, 35 insertions(+), 10 deletions(-)\n\n-- \n2.31.0-rc2-175-g3820f1c72e\n\n"},{"id":"418927","messageId":"20210312030107.1849942-2-gitster@pobox.com","threadId":"55295","inReplyTo":"20210312030107.1849942-1-gitster@pobox.com","subject":"[PATCH 1/2] sequencer.c: make commit_post_rewrite() take two object names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-12T03:01:06Z","receivedAt":"2021-03-12T03:02:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The helper function is to copy notes between two commits by taking\nthe original one and a new one that is supposed to be a rewrite of\nthe original one.  It somehow takes \"struct commit *\" for the former\nbut \"struct object_id *\" for the latter, but what it does does not\nneed access to the in-core commit object.\n\nChange the function to take two \"struct object_id *\" instead.\n\nAs we require \"struct object_id *\" for the original commit now,\nthe comment on the old \"struct commit *\" that must be non-NULL\nwhich made taking the address of its object.oid member safe no\nlonger is relevant to this function (it is the caller's concern\nnow), so remove it.  Two callers we have are of course safe, and\nthere is no reason to call this \"as we rewrote A to B, please copy\nnotes on A to B, too\" helper if the caller knows A is NULL, so\nthe comment would have very little value even if it were kept.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/commit.c | 6 +++---\n sequencer.c      | 9 ++++-----\n sequencer.h      | 2 +-\n 3 files changed, 8 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 739110c5a7..f2fbef053e 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1702,9 +1702,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \trepo_rerere(the_repository, 0);\n \trun_auto_maintenance(quiet);\n \trun_commit_hook(use_editor, get_index_file(), \"post-commit\", NULL);\n-\tif (amend && !no_post_rewrite) {\n-\t\tcommit_post_rewrite(the_repository, current_head, &oid);\n-\t}\n+\tif (amend && !no_post_rewrite)\n+\t\tcommit_post_rewrite(the_repository, &current_head->object.oid, &oid);\n+\n \tif (!quiet) {\n \t\tunsigned int flags = 0;\n \ndiff --git a/sequencer.c b/sequencer.c\nindex d2332d3e17..92a4871997 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1175,18 +1175,17 @@ static int run_rewrite_hook(const struct object_id *oldoid,\n }\n \n void commit_post_rewrite(struct repository *r,\n-\t\t\t const struct commit *old_head,\n+\t\t\t const struct object_id *old_head,\n \t\t\t const struct object_id *new_head)\n {\n \tstruct notes_rewrite_cfg *cfg;\n \n \tcfg = init_copy_notes_for_rewrite(\"amend\");\n \tif (cfg) {\n-\t\t/* we are amending, so old_head is not NULL */\n-\t\tcopy_note_for_rewrite(cfg, &old_head->object.oid, new_head);\n+\t\tcopy_note_for_rewrite(cfg, old_head, new_head);\n \t\tfinish_copy_notes_for_rewrite(r, cfg, \"Notes added by 'git commit --amend'\");\n \t}\n-\trun_rewrite_hook(&old_head->object.oid, new_head);\n+\trun_rewrite_hook(old_head, new_head);\n }\n \n static int run_prepare_commit_msg_hook(struct repository *r,\n@@ -1538,7 +1537,7 @@ static int try_to_commit(struct repository *r,\n \n \trun_commit_hook(0, r->index_file, \"post-commit\", NULL);\n \tif (flags & AMEND_MSG)\n-\t\tcommit_post_rewrite(r, current_head, oid);\n+\t\tcommit_post_rewrite(r, &current_head->object.oid, oid);\n \n out:\n \tfree_commit_extra_headers(extra);\ndiff --git a/sequencer.h b/sequencer.h\nindex f8b2e4ab85..79eed55c4b 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -192,7 +192,7 @@ int update_head_with_reflog(const struct commit *old_head,\n \t\t\t    const char *action, const struct strbuf *msg,\n \t\t\t    struct strbuf *err);\n void commit_post_rewrite(struct repository *r,\n-\t\t\t const struct commit *current_head,\n+\t\t\t const struct object_id *old_head,\n \t\t\t const struct object_id *new_head);\n \n void create_autostash(struct repository *r, const char *path,\n-- \n2.31.0-rc2-175-g3820f1c72e\n\n"},{"id":"418928","messageId":"20210312030107.1849942-3-gitster@pobox.com","threadId":"55295","inReplyTo":"20210312030107.1849942-1-gitster@pobox.com","subject":"[PATCH 2/2] [WIP] sequencer.c: carry forward notes on HEAD across \"rebase -x\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-12T03:01:07Z","receivedAt":"2021-03-12T03:02:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When the external command invoked by \"rebase -x\" replaces the HEAD,\nwe seem to lose the notes attached to the original HEAD, which is\nwhy we have been losing the amlog mappings in the git core project.\n\nHere is a half-successful attempt to fix it.  For whatever reason,\nwhen the external command is \"git commit --amend --no-edit\", the\nupdated code carries notes forward correctly, but when the command\nis changed to \"git commit --amend -m tweak\", it fails to do so, and\nI do not have more time to work on this, so I'd stop here with an\nexpected failure in the test.\n\nHelp is appreciated.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n sequencer.c                   | 10 +++++++++-\n t/t3404-rebase-interactive.sh | 18 ++++++++++++++++++\n 2 files changed, 27 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 92a4871997..e0bdc39e4d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3276,7 +3276,10 @@ static int do_exec(struct repository *r, const char *command_line)\n {\n \tstruct strvec child_env = STRVEC_INIT;\n \tconst char *child_argv[] = { NULL, NULL };\n-\tint dirty, status;\n+\tint dirty, status, bad_head;\n+\tstruct object_id old_head_oid, new_head_oid;\n+\n+\tbad_head = get_oid(\"HEAD\", &old_head_oid);\n \n \tfprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n \tchild_argv[0] = command_line;\n@@ -3286,6 +3289,11 @@ static int do_exec(struct repository *r, const char *command_line)\n \tstatus = run_command_v_opt_cd_env(child_argv, RUN_USING_SHELL, NULL,\n \t\t\t\t\t  child_env.v);\n \n+\tbad_head |= get_oid(\"HEAD\", &new_head_oid);\n+\n+\tif (!bad_head && !oideq(&old_head_oid, &new_head_oid))\n+\t\tcommit_post_rewrite(r, &old_head_oid, &new_head_oid);\n+\n \t/* force re-reading of the cache */\n \tif (discard_index(r->index) < 0 || repo_read_index(r) < 0)\n \t\treturn error(_(\"could not read index\"));\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 66bcbbf952..3222c594ab 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -155,6 +155,8 @@ test_expect_success 'rebase -i with the exec command checks tree cleanness' '\n \tgit rebase --continue\n '\n \n+# NEEDSWORK: Fix c762aada1ab3a2c428c with s/@/HEAD/;\n+\n test_expect_success 'rebase -x with empty command fails' '\n \ttest_when_finished \"git rebase --abort ||:\" &&\n \ttest_must_fail env git rebase -x \"\" @ 2>actual &&\n@@ -867,6 +869,22 @@ test_expect_success 'rebase -i can copy notes over a fixup' '\n \ttest_cmp expect output\n '\n \n+test_expect_success 'notes are copied even rebase -x changes HEAD' '\n+\tgit reset --hard n3 &&\n+\tgit rebase -x \"git commit --amend --no-edit\" n1^1 &&\n+\tgit log --format=\"%s <%N>\" n1^1..n3 >expect &&\n+\tgit log --format=\"%s <%N>\" n1^1..HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'notes are copied even rebase -x changes HEAD' '\n+\tgit reset --hard n3 &&\n+\tgit rebase -x \"git commit --amend -m tweak\" n1^1 &&\n+\tgit log --format=\"tweak <%N>\" n1^1..n3 >expect &&\n+\tgit log --format=\"%s <%N>\" n1^1..HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'rebase while detaching HEAD' '\n \tgit symbolic-ref HEAD &&\n \tgrandparent=$(git rev-parse HEAD~2) &&\n-- \n2.31.0-rc2-175-g3820f1c72e\n\n"},{"id":"418960","messageId":"f6cdbe1b-eb5a-5822-0ca9-a78c38a2fe7a@gmail.com","threadId":"55295","inReplyTo":"xmqq8s6tcuxc.fsf@gitster.g","subject":"Re: [BUG] cmd given to \"rebase -x cmd\" cannot see the original HEAD?","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-03-12T11:09:17Z","receivedAt":"2021-03-12T11:10:22Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 11/03/2021 20:11, Junio C Hamano wrote:\n> I was trying to figure out why I am losing the amlog note that\n> maps each commit on topics to the message ID after amending them\n> with extra trailers (e.g. Reviewed-by).  I use a script that runs\n> \"git commit --amend\" to do this and give it to \"rebase -x\".\n> \n> It seems that when the command given to \"rebase -x\" is run, HEAD is\n> already updated to a commit with tree and log message identical to\n> the original, with updated committer information.  However, the\n> note attached to the original commit is *not* copied to this\n> intermediate HEAD that is shown to the external command.\n\nYes this is annoying I've sometimes wanted to edit the notes when the \nrebase is stopped for for 'edit' or 'break' commands\n\n> Hence, even if I try to read the existing note from HEAD that is\n> given to the script spawned via \"rebase -x cmd\" mechanism, the\n> script cannot copy the note to the resulting commit after it adds\n> extra trailers.  Also, there is no mechansim for the external\n> command to learn where the original HEAD was.\n> \n> I see a few possible avenues to fix this.\n> \n> 1. Before \"rebase -x\" mechanism spawns an external command, make\n>     sure that the \"copy notes upon rewriting\" logic has triggered, so\n>     that the HEAD observable by the command has the notes from the\n>     original.\n\nThat would be my preferred way to fix this, but we should also copy the \nnotes before stopping for an 'edit' or 'break' and maybe 'reword' so \nthat the notes are always available when a rebase stops for some reason \nother than conflict resolution.\n\n>  It seems that the \"copy notes\" logic is only used at\n>     the end, but it does not seem to take the possibility into\n>     account that the external command might move HEAD to a different\n>     commit (i.e. it starts from HEAD0, creates HEAD1 that is\n>     different from HEAD0 only for committer info, hands HEAD1 to\n>     external command that may move HEAD to HEAD2, but then copies the\n>     note from HEAD0 to HEAD1 because it is unaware of HEAD2), so it\n>     only annotates HEAD1 that is immediately lost (nothing other than\n>     HEAD's reflog would be pointing at it). [*]\n\nI think the root cause is that 'git commit --amend' does not update the \nlist of rewritten commits when run during a rebase, which messes up the \npost-rewrite hooks (as you suspected) and note copying which is \nessentially a built-in post-rewrite hook. I've got a half finished \nsolution but there are some corner cases that I've never got round to \naddressing. It would fix your problem but not the problem of making \nnotes available to the user during the rebase.\n\n\nBest Wishes\n\nPhillip\n\n> 2. Make sure the \"copy notes\" logic copies the notes taken from the\n>     original HEAD to the HEAD _after_ \"rebase -x\" external command\n>     returns.\n> \n> The first approach would still make the external command responsible\n> for copying notes from HEAD it sees (i.e. HEAD1) to the new commit\n> it creates and points HEAD at (i.e. HEAD2).  It _could_ be used as a\n> mechanism to allow the external command to modify/remove notes.\n> \n> The second approach would forbid the external command from mucking\n> with notes at all.  \"rebase\" would first remember the original HEAD\n> (i.e. HEAD0), does its thing (i.e. creates HEAD1 and points HEAD at\n> it, and then lets the external command create HEAD2 and point HEAD\n> at it), and then figure out the latest HEAD and copy notes from the\n> original HEAD.  It is simpler, works with scripts that uses things\n> like \"commit --amend\" that do not copy notes, but less flexible.\n> \n> Implementation-wise, it seems that the latter would require more\n> significant surgery to do_pick_commit() codepath, but somebody more\n> familiar with the sequencer codepath should research to find the\n> best approach.\n> \n> I suspect that \"post-commit\" hook that is run from try_to_commit()\n> may have the same issue, here in sequencer.c\n> \n> 1539:\trun_commit_hook(0, r->index_file, \"post-commit\", NULL);\n> 1540:\tif (flags & AMEND_MSG)\n> 1541:\t\tcommit_post_rewrite(r, current_head, oid);\n> \n> where oid is not adjusted by re-reading what the post-commit hook\n> did to our HEAD.\n> \n> But the codepath to reach do_exec() (which is what spawns the\n> external command for \"rebase -x cmd\") is quite away from any\n> call to commit_post_rewrite(),\n> \n> \n> \n> [footnote]\n> \n> I have a 6-commit series with notes/amlog record on each commit, and\n> running:\n> \n> $ git rebase -x '\n> \tgit rev-parse HEAD\n> \tgit notes --ref=notes/amlog show HEAD || :\n> ' master $(git rev-parse ab/read-tree)\n> \n> yields the following:\n> \n> Executing: git rev-parse HEAD; git notes --ref=notes/amlog show HEAD || :\n> 669950d7def2e849cb1ee6e7b3a1beac5c45ce1c\n> error: no note found for object 669950d7def2e849cb1ee6e7b3a1beac5c45ce1c.\n> Executing: git rev-parse HEAD; git notes --ref=notes/amlog show HEAD || :\n> 0545e0a567f96eff14dfb93b44aecf9683b44803\n> error: no note found for object 0545e0a567f96eff14dfb93b44aecf9683b44803.\n> Executing: git rev-parse HEAD; git notes --ref=notes/amlog show HEAD || :\n> d9b4e77efe5e65952e05533ebf500b559408d436\n> error: no note found for object d9b4e77efe5e65952e05533ebf500b559408d436.\n> Executing: git rev-parse HEAD; git notes --ref=notes/amlog show HEAD || :\n> 6d8735372c0c9b9d3793959157456cd445d025fa\n> error: no note found for object 6d8735372c0c9b9d3793959157456cd445d025fa.\n> Executing: git rev-parse HEAD; git notes --ref=notes/amlog show HEAD || :\n> b86c1305258a11f42927f924498a94b3ffb19472\n> error: no note found for object b86c1305258a11f42927f924498a94b3ffb19472.\n> Executing: git rev-parse HEAD; git notes --ref=notes/amlog show HEAD || :\n> 7da85bae9f11ab14d616bcade1fb2a7b434aa716\n> error: no note found for object 7da85bae9f11ab14d616bcade1fb2a7b434aa716.\n> Successfully rebased and updated detached HEAD.\n> \n> We can see each commit on ab/read-tree has notes/amlog entry that\n> records where it came from:\n> \n> $ git log --notes=notes/amlog --oneline --reverse master..ab/read-tree\n> 2e3e38a4ad ls-files tests: add meaningful --with-tree tests\n> Notes (amlog):\n>      Message-Id: <20210308022138.28166-2-avarab@gmail.com>\n> \n> 2371fda438 tree.c API: move read_tree() into builtin/ls-files.c\n> Notes (amlog):\n>      Message-Id: <20210308022138.28166-3-avarab@gmail.com>\n> \n> 32c718db53 ls-files: don't needlessly pass around stage variable\n> Notes (amlog):\n>      Message-Id: <20210308022138.28166-4-avarab@gmail.com>\n> \n> 2ebabd63f7 ls-files: refactor away read_tree()\n> Notes (amlog):\n>      Message-Id: <20210308022138.28166-5-avarab@gmail.com>\n> \n> e98fb6ddc7 tree.h API: remove support for starting at prefix != \"\"\n> Notes (amlog):\n>      Message-Id: <20210308022138.28166-6-avarab@gmail.com>\n> \n> 8e88702431 tree.h API: remove \"stage\" parameter from read_tree_recursive()\n> Notes (amlog):\n>      Message-Id: <20210308022138.28166-7-avarab@gmail.com>\n> \n> \n> In the above transcript, 669950d7def2e8 (the first commit that the\n> \"rebase -x\" command saw) corresponds to 2e3e38a4ad (the first commit\n> in the series), and we can see the only difference between them is\n> the committer info.  As we already saw in the above, the original\n> has a note that is not copied to the new one when \"rebase -x\"\n> command runs.\n> \n> $ git show --pretty=fuller 2e3e38a4ad >old\n> $ git show --pretty=fuller 669950d7def2e8 >new\n> $ diff -u old new\n> --- old\t2021-03-11 12:06:33.259434213 -0800\n> +++ new\t2021-03-11 12:06:33.259434213 -0800\n> @@ -1,8 +1,8 @@\n> -commit 2e3e38a4ad6d9d78381e90348289277acb4c6f8b\n> +commit 669950d7def2e849cb1ee6e7b3a1beac5c45ce1c\n>   Author:     Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>   AuthorDate: Mon Mar 8 03:21:33 2021 +0100\n>   Commit:     Junio C Hamano <gitster@pobox.com>\n> -CommitDate: Mon Mar 8 10:06:35 2021 -0800\n> +CommitDate: Thu Mar 11 12:01:11 2021 -0800\n>   \n>       ls-files tests: add meaningful --with-tree tests\n>       \n> \n> HOWEVER, after that failed \"rebase -x\" session, the note is copied\n> to these \"intermediate\" commits.  Using 7da85bae9f1 which is at the\n> tip of the history fed to \"rebase -x\" external command, we can see:\n> \n> $ git log --notes=notes/amlog --oneline --reverse master..7da85bae9f1\n> \n> 669950d7de ls-files tests: add meaningful --with-tree tests\n> Notes (amlog):\n>      Message-Id: <20210308022138.28166-2-avarab@gmail.com>\n> \n> 0545e0a567 tree.c API: move read_tree() into builtin/ls-files.c\n> Notes (amlog):\n>      Message-Id: <20210308022138.28166-3-avarab@gmail.com>\n> \n> d9b4e77efe ls-files: don't needlessly pass around stage variable\n> Notes (amlog):\n>      Message-Id: <20210308022138.28166-4-avarab@gmail.com>\n> \n> 6d8735372c ls-files: refactor away read_tree()\n> Notes (amlog):\n>      Message-Id: <20210308022138.28166-5-avarab@gmail.com>\n> \n> b86c130525 tree.h API: remove support for starting at prefix != \"\"\n> Notes (amlog):\n>      Message-Id: <20210308022138.28166-6-avarab@gmail.com>\n> \n> 7da85bae9f tree.h API: remove \"stage\" parameter from read_tree_recursive()\n> Notes (amlog):\n>      Message-Id: <20210308022138.28166-7-avarab@gmail.com>\n> \n> \n> It is only that they seem to be added _after_ the external command\n> finishes, which is not a very useful behaviour.\n> \n\n"},{"id":"418961","messageId":"8f5010fa-cafe-779e-b555-f018bf15c7ee@gmail.com","threadId":"55295","inReplyTo":"20210312030107.1849942-3-gitster@pobox.com","subject":"Re: [PATCH 2/2] [WIP] sequencer.c: carry forward notes on HEAD across \"rebase -x\"","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-03-12T11:12:02Z","receivedAt":"2021-03-12T11:13:04Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 12/03/2021 03:01, Junio C Hamano wrote:\n> When the external command invoked by \"rebase -x\" replaces the HEAD,\n> we seem to lose the notes attached to the original HEAD, which is\n> why we have been losing the amlog mappings in the git core project.\n> \n> Here is a half-successful attempt to fix it.  For whatever reason,\n> when the external command is \"git commit --amend --no-edit\", the\n> updated code carries notes forward correctly, but when the command\n> is changed to \"git commit --amend -m tweak\", it fails to do so, and\n> I do not have more time to work on this, so I'd stop here with an\n> expected failure in the test.\n> \n> Help is appreciated.\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>   sequencer.c                   | 10 +++++++++-\n>   t/t3404-rebase-interactive.sh | 18 ++++++++++++++++++\n>   2 files changed, 27 insertions(+), 1 deletion(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 92a4871997..e0bdc39e4d 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3276,7 +3276,10 @@ static int do_exec(struct repository *r, const char *command_line)\n>   {\n>   \tstruct strvec child_env = STRVEC_INIT;\n>   \tconst char *child_argv[] = { NULL, NULL };\n> -\tint dirty, status;\n> +\tint dirty, status, bad_head;\n> +\tstruct object_id old_head_oid, new_head_oid;\n> +\n> +\tbad_head = get_oid(\"HEAD\", &old_head_oid);\n>   \n>   \tfprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n>   \tchild_argv[0] = command_line;\n> @@ -3286,6 +3289,11 @@ static int do_exec(struct repository *r, const char *command_line)\n>   \tstatus = run_command_v_opt_cd_env(child_argv, RUN_USING_SHELL, NULL,\n>   \t\t\t\t\t  child_env.v);\n>   \n> +\tbad_head |= get_oid(\"HEAD\", &new_head_oid);\n> +\n> +\tif (!bad_head && !oideq(&old_head_oid, &new_head_oid))\n> +\t\tcommit_post_rewrite(r, &old_head_oid, &new_head_oid);\n\nIf HEAD has changed then we copy the notes - but we never copied the \nnotes from the original commit to the rewritten commit before running \nthe exec command so this does not do what we want. I think this is why \nthe second test below fails. I think the first test passes because the \nauthor and committer identities and dates of the amended commit are \nunchanged from the rebased commit so we end up copying the notes at the \nend of the rebase because the oid is unchanged.\n\nBlindly copying the notes here would mean that the notes would be copied \nif an exec command created a new commit on top of HEAD rather than \namending HEAD which is not what we want.\n\nBest Wishes\n\nPhillip\n\n>   \t/* force re-reading of the cache */\n>   \tif (discard_index(r->index) < 0 || repo_read_index(r) < 0)\n>   \t\treturn error(_(\"could not read index\"));\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 66bcbbf952..3222c594ab 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -155,6 +155,8 @@ test_expect_success 'rebase -i with the exec command checks tree cleanness' '\n>   \tgit rebase --continue\n>   '\n>   \n> +# NEEDSWORK: Fix c762aada1ab3a2c428c with s/@/HEAD/;\n> +\n>   test_expect_success 'rebase -x with empty command fails' '\n>   \ttest_when_finished \"git rebase --abort ||:\" &&\n>   \ttest_must_fail env git rebase -x \"\" @ 2>actual &&\n> @@ -867,6 +869,22 @@ test_expect_success 'rebase -i can copy notes over a fixup' '\n>   \ttest_cmp expect output\n>   '\n>   \n> +test_expect_success 'notes are copied even rebase -x changes HEAD' '\n> +\tgit reset --hard n3 &&\n> +\tgit rebase -x \"git commit --amend --no-edit\" n1^1 &&\n> +\tgit log --format=\"%s <%N>\" n1^1..n3 >expect &&\n> +\tgit log --format=\"%s <%N>\" n1^1..HEAD >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_failure 'notes are copied even rebase -x changes HEAD' '\n> +\tgit reset --hard n3 &&\n> +\tgit rebase -x \"git commit --amend -m tweak\" n1^1 &&\n> +\tgit log --format=\"tweak <%N>\" n1^1..n3 >expect &&\n> +\tgit log --format=\"%s <%N>\" n1^1..HEAD >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>   test_expect_success 'rebase while detaching HEAD' '\n>   \tgit symbolic-ref HEAD &&\n>   \tgrandparent=$(git rev-parse HEAD~2) &&\n> \n\n"},{"id":"418963","messageId":"20210312112636.22711-1-phillip.wood123@gmail.com","threadId":"55295","inReplyTo":"xmqq8s6tcuxc.fsf@gitster.g","subject":"[WIP][PATCH] rebase: update the list of rewritten commits when amending pick","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-03-12T11:26:36Z","receivedAt":"2021-03-12T11:27:38Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nIf HEAD is amended during an exec command then the amended commit is missing\nfrom the list of rewritten commits. The commonest way for this to happen is if a\ncommit is amended to fix a test failure when running `git rebase --exec \"make\ntest\"` but it can also happen if the exec command calls `git commit --amend`\ndirectly. Amending commits with exec commands was discussed on the mailing list\nrecently where someone wanted to reset the author before submitting patches\nupstream[1].\n\n[1] https://public-inbox.org/git/CABPp-BEYRmhrb4Tx3bGzkx8y53T_0BYhLE5J0cEmxj18WtZs9A@mail.gmail.com/#t\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n\nThis is what I've been using for a couple of years to update the list\nof rewritten commits when running \"git commit --amend\" during a\nrebase. I think it changes which commit gets used as the rewritten one\nif you split a commit with 'edit', the patch is so old I cannot\nremember if there were any other corner cases.\n\n\n builtin/commit.c             |   2 +-\n sequencer.c                  | 119 +++++++++++++++++++++++++++++------\n sequencer.h                  |   6 +-\n t/lib-rebase.sh              |   2 +-\n t/t5407-post-rewrite-hook.sh |  70 ++++++++++++++++++++-\n 5 files changed, 176 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex ae7aaf6dc6..9b6f3d8b6b 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1697,7 +1697,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \trun_command_v_opt(argv_gc_auto, RUN_GIT_CMD);\n \trun_commit_hook(use_editor, get_index_file(), \"post-commit\", NULL);\n \tif (amend && !no_post_rewrite) {\n-\t\tcommit_post_rewrite(the_repository, current_head, &oid);\n+\t\tcommit_post_rewrite(the_repository, current_head, &oid, NULL);\n \t}\n \tif (!quiet) {\n \t\tunsigned int flags = 0;\ndiff --git a/sequencer.c b/sequencer.c\nindex b395dd6e11..5d68e7341d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -129,6 +129,7 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha, \"rebase-merge/stopped-sha\")\n static GIT_PATH_FUNC(rebase_path_rewritten_list, \"rebase-merge/rewritten-list\")\n static GIT_PATH_FUNC(rebase_path_rewritten_pending,\n \t\"rebase-merge/rewritten-pending\")\n+static GIT_PATH_FUNC(rebase_path_rewritten_head, \"rebase-merge/rewritten-head\")\n \n /*\n  * The path of the file containig the OID of the \"squash onto\" commit, i.e.\n@@ -159,6 +160,9 @@ static GIT_PATH_FUNC(rebase_path_strategy_opts, \"rebase-merge/strategy_opts\")\n static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, \"rebase-merge/allow_rerere_autoupdate\")\n static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, \"rebase-merge/reschedule-failed-exec\")\n \n+static void write_rewritten_head(struct object_id *rewritten_head);\n+static void read_rewritten_head(struct object_id *rewritten_head);\n+\n static int git_sequencer_config(const char *k, const char *v, void *cb)\n {\n \tstruct replay_opts *opts = cb;\n@@ -953,12 +957,12 @@ static int run_git_commit(struct repository *r,\n \t\t\t  unsigned int flags)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tint res = 0;\n \n \tif ((flags & CREATE_ROOT_COMMIT) && !(flags & AMEND_MSG)) {\n \t\tstruct strbuf msg = STRBUF_INIT, script = STRBUF_INIT;\n \t\tconst char *author = NULL;\n \t\tstruct object_id root_commit, *cache_tree_oid;\n-\t\tint res = 0;\n \n \t\tif (is_rebase_i(opts)) {\n \t\t\tauthor = read_author_ident(&script);\n@@ -1004,6 +1008,9 @@ static int run_git_commit(struct repository *r,\n \t\t\t     gpg_opt, gpg_opt);\n \t}\n \n+\tif (is_rebase_i(opts) && (flags & AMEND_MSG))\n+\t\twrite_rewritten_head(&opts->rewritten_head);\n+\n \targv_array_push(&cmd.args, \"commit\");\n \n \tif (!(flags & VERIFY_MSG))\n@@ -1032,9 +1039,14 @@ static int run_git_commit(struct repository *r,\n \t\targv_array_push(&cmd.args, \"--allow-empty-message\");\n \n \tif (is_rebase_i(opts) && !(flags & EDIT_MSG))\n-\t\treturn run_command_silent_on_success(&cmd);\n+\t\tres = run_command_silent_on_success(&cmd);\n \telse\n-\t\treturn run_command(&cmd);\n+\t\tres = run_command(&cmd);\n+\n+\tif (is_rebase_i(opts) && !res && (flags & AMEND_MSG))\n+\t\tread_rewritten_head(&opts->rewritten_head);\n+\n+\treturn res;\n }\n \n static int rest_is_empty(const struct strbuf *sb, int start)\n@@ -1177,12 +1189,42 @@ static int run_rewrite_hook(const struct object_id *oldoid,\n \treturn finish_command(&proc);\n }\n \n+static void update_rewritten(const struct repository *r,\n+\t\t\t     const struct object_id *old_head,\n+\t\t\t     const struct object_id *new_head,\n+\t\t\t     struct object_id *rewritten_head)\n+{\n+\tstruct object_id oid;\n+\n+\tif (!rewritten_head) {\n+\t\tread_rewritten_head(&oid);\n+\t\trewritten_head = &oid;\n+\t}\n+\tif (oideq(old_head, rewritten_head)) {\n+\t\tFILE *fp;\n+\t\tfp = fopen_or_warn(rebase_path_rewritten_list(), \"a\");\n+\t\tif (fp) {\n+\t\t\tfprintf(fp, \"%s %s\\n\",\n+\t\t\t    oid_to_hex(old_head), oid_to_hex(new_head));\n+\t\t\tfclose(fp);\n+\t\t}\n+\t\toidcpy(rewritten_head, new_head);\n+\t}\n+\tif (rewritten_head == &oid)\n+\t\twrite_rewritten_head(rewritten_head);\n+\n+\treturn;\n+}\n+\n void commit_post_rewrite(struct repository *r,\n \t\t\t const struct commit *old_head,\n-\t\t\t const struct object_id *new_head)\n+\t\t\t const struct object_id *new_head,\n+\t\t\t struct object_id *rewritten_head)\n {\n \tstruct notes_rewrite_cfg *cfg;\n \n+\tupdate_rewritten(r, &old_head->object.oid, new_head, rewritten_head);\n+\n \tcfg = init_copy_notes_for_rewrite(\"amend\");\n \tif (cfg) {\n \t\t/* we are amending, so old_head is not NULL */\n@@ -1473,7 +1515,8 @@ static int try_to_commit(struct repository *r,\n \t}\n \n \tif (flags & AMEND_MSG)\n-\t\tcommit_post_rewrite(r, current_head, oid);\n+\t\tcommit_post_rewrite(r, current_head, oid,\n+\t\t\t\t    &opts->rewritten_head);\n \n out:\n \tfree_commit_extra_headers(extra);\n@@ -1731,7 +1774,7 @@ static int update_squash_messages(struct repository *r,\n \treturn res;\n }\n \n-static void flush_rewritten_pending(void)\n+static void flush_rewritten_pending(struct object_id *rewritten_head)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct object_id newoid;\n@@ -1752,12 +1795,14 @@ static void flush_rewritten_pending(void)\n \t\t}\n \t\tfclose(out);\n \t\tunlink(rebase_path_rewritten_pending());\n+\t\toidcpy(rewritten_head, &newoid);\n \t}\n \tstrbuf_release(&buf);\n }\n \n static void record_in_rewritten(struct object_id *oid,\n-\t\tenum todo_command next_command)\n+\t\t\t\tenum todo_command next_command,\n+\t\t\t\tstruct object_id *rewritten_head)\n {\n \tFILE *out = fopen_or_warn(rebase_path_rewritten_pending(), \"a\");\n \n@@ -1768,7 +1813,7 @@ static void record_in_rewritten(struct object_id *oid,\n \tfclose(out);\n \n \tif (!is_fixup(next_command))\n-\t\tflush_rewritten_pending();\n+\t\tflush_rewritten_pending(rewritten_head);\n }\n \n static int do_pick_commit(struct repository *r,\n@@ -2510,6 +2555,11 @@ static int read_populate_opts(struct replay_opts *opts)\n \tif (is_rebase_i(opts)) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n+\t\tif (file_exists(rebase_path_rewritten_head()))\n+\t\t\tread_rewritten_head(&opts->rewritten_head);\n+\t\telse\n+\t\t\topts->rewritten_head = null_oid;\n+\n \t\tif (read_oneliner(&buf, rebase_path_gpg_sign_opt(), 1)) {\n \t\t\tif (!starts_with(buf.buf, \"-S\"))\n \t\t\t\tstrbuf_reset(&buf);\n@@ -3065,6 +3115,7 @@ static int error_with_patch(struct repository *r,\n \t\t\t    struct replay_opts *opts,\n \t\t\t    int exit_code, int to_amend)\n {\n+\twrite_rewritten_head(&opts->rewritten_head);\n \tif (commit) {\n \t\tif (make_patch(r, commit, opts))\n \t\t\treturn -1;\n@@ -3119,12 +3170,14 @@ static int error_failed_squash(struct repository *r,\n \treturn error_with_patch(r, commit, subject, subject_len, opts, 1, 0);\n }\n \n-static int do_exec(struct repository *r, const char *command_line)\n+static int do_exec(struct repository *r, const char *command_line,\n+\t\t   struct object_id *rewritten_head)\n {\n \tstruct argv_array child_env = ARGV_ARRAY_INIT;\n \tconst char *child_argv[] = { NULL, NULL };\n \tint dirty, status;\n \n+\twrite_rewritten_head(rewritten_head);\n \tfprintf(stderr, \"Executing: %s\\n\", command_line);\n \tchild_argv[0] = command_line;\n \targv_array_pushf(&child_env, \"GIT_DIR=%s\", absolute_path(get_git_dir()));\n@@ -3133,6 +3186,7 @@ static int do_exec(struct repository *r, const char *command_line)\n \tstatus = run_command_v_opt_cd_env(child_argv, RUN_USING_SHELL, NULL,\n \t\t\t\t\t  child_env.argv);\n \n+\tread_rewritten_head(rewritten_head);\n \t/* force re-reading of the cache */\n \tif (discard_index(r->index) < 0 || repo_read_index(r) < 0)\n \t\treturn error(_(\"could not read index\"));\n@@ -3331,10 +3385,12 @@ static int do_reset(struct repository *r,\n \t\tret = error(_(\"could not write index\"));\n \tfree((void *)desc.buffer);\n \n-\tif (!ret)\n+\tif (!ret) {\n \t\tret = update_ref(reflog_message(opts, \"reset\", \"'%.*s'\",\n \t\t\t\t\t\tlen, name), \"HEAD\", &oid,\n \t\t\t\t NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n+\t\toidcpy(&opts->rewritten_head, &oid);\n+\t}\n \n \tstrbuf_release(&ref_name);\n \treturn ret;\n@@ -3862,6 +3918,7 @@ static int pick_commits(struct repository *r,\n \t\t\tdelete_ref(NULL, \"REBASE_HEAD\", NULL, REF_NO_DEREF);\n \n \t\t\tif (item->command == TODO_BREAK) {\n+\t\t\t\twrite_rewritten_head(&opts->rewritten_head);\n \t\t\t\tif (!opts->verbose)\n \t\t\t\t\tterm_clear_line();\n \t\t\t\treturn stopped_at_head(r);\n@@ -3900,7 +3957,8 @@ static int pick_commits(struct repository *r,\n \t\t\t}\n \t\t\tif (is_rebase_i(opts) && !res)\n \t\t\t\trecord_in_rewritten(&item->commit->object.oid,\n-\t\t\t\t\tpeek_command(todo_list, 1));\n+\t\t\t\t\t\t    peek_command(todo_list, 1),\n+\t\t\t\t\t\t    &opts->rewritten_head);\n \t\t\tif (res && is_fixup(item->command)) {\n \t\t\t\tif (res == 1)\n \t\t\t\t\tintend_to_amend();\n@@ -3935,7 +3993,7 @@ static int pick_commits(struct repository *r,\n \t\t\tif (!opts->verbose)\n \t\t\t\tterm_clear_line();\n \t\t\t*end_of_arg = '\\0';\n-\t\t\tres = do_exec(r, arg);\n+\t\t\tres = do_exec(r, arg, &opts->rewritten_head);\n \t\t\t*end_of_arg = saved;\n \n \t\t\tif (res) {\n@@ -3965,7 +4023,8 @@ static int pick_commits(struct repository *r,\n \t\t\t\treschedule = 1;\n \t\t\telse if (item->commit)\n \t\t\t\trecord_in_rewritten(&item->commit->object.oid,\n-\t\t\t\t\t\t    peek_command(todo_list, 1));\n+\t\t\t\t\t\t    peek_command(todo_list, 1),\n+\t\t\t\t\t\t    &opts->rewritten_head);\n \t\t\tif (res > 0)\n \t\t\t\t/* failed with merge conflicts */\n \t\t\t\treturn error_with_patch(r, item->commit,\n@@ -4062,7 +4121,7 @@ static int pick_commits(struct repository *r,\n \t\t\t\tlog_tree_diff_flush(&log_tree_opt);\n \t\t\t}\n \t\t}\n-\t\tflush_rewritten_pending();\n+\t\tflush_rewritten_pending(&opts->rewritten_head);\n \t\tif (!stat(rebase_path_rewritten_list(), &st) &&\n \t\t\t\tst.st_size > 0) {\n \t\t\tstruct child_process child = CHILD_PROCESS_INIT;\n@@ -4299,7 +4358,8 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)\n \n \t\tif (read_oneliner(&buf, rebase_path_stopped_sha(), 1) &&\n \t\t    !get_oid_committish(buf.buf, &oid))\n-\t\t\trecord_in_rewritten(&oid, peek_command(&todo_list, 0));\n+\t\t\trecord_in_rewritten(&oid, peek_command(&todo_list, 0),\n+\t\t\t\t\t    &opts->rewritten_head);\n \t\tstrbuf_release(&buf);\n \t}\n \n@@ -5024,7 +5084,8 @@ int check_todo_list_from_file(struct repository *r)\n /* skip picking commits whose parents are unchanged */\n static int skip_unnecessary_picks(struct repository *r,\n \t\t\t\t  struct todo_list *todo_list,\n-\t\t\t\t  struct object_id *base_oid)\n+\t\t\t\t  struct object_id *base_oid,\n+\t\t\t\t  struct object_id *rewritten_head)\n {\n \tstruct object_id *parent_oid;\n \tint i;\n@@ -5062,8 +5123,15 @@ static int skip_unnecessary_picks(struct repository *r,\n \t\ttodo_list->current = 0;\n \t\ttodo_list->done_nr += i;\n \n-\t\tif (is_fixup(peek_command(todo_list, 0)))\n-\t\t\trecord_in_rewritten(base_oid, peek_command(todo_list, 0));\n+\t\tif (is_fixup(peek_command(todo_list, 0))) {\n+\t\t\trecord_in_rewritten(base_oid, peek_command(todo_list, 0),\n+\t\t\t\t\t    rewritten_head);\n+\t\t\toidcpy(rewritten_head, &null_oid);\n+\t\t} else {\n+\t\t\toidcpy(rewritten_head, base_oid);\n+\t\t}\n+\t} else {\n+\t\toidcpy(rewritten_head, base_oid);\n \t}\n \n \treturn 0;\n@@ -5129,7 +5197,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla\n \t\treturn -1;\n \t}\n \n-\tif (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid)) {\n+\tif (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid,\n+\t\t\t\t\t\t     &opts->rewritten_head)) {\n \t\ttodo_list_release(&new_todo);\n \t\treturn error(_(\"could not skip unnecessary pick commands\"));\n \t}\n@@ -5315,3 +5384,15 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)\n \n \treturn 0;\n }\n+static void write_rewritten_head(struct object_id *rewritten_head)\n+{\n+\tconst char *hex = oid_to_hex(rewritten_head);\n+\twrite_message(hex, strlen(hex), rebase_path_rewritten_head(), 1);\n+}\n+\n+static void read_rewritten_head(struct object_id *rewritten_head)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tread_oneliner(&buf, rebase_path_rewritten_head(), 0);\n+\tget_oid(buf.buf, rewritten_head);\n+}\ndiff --git a/sequencer.h b/sequencer.h\nindex 574260f621..91834cfe1c 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -63,6 +63,9 @@ struct replay_opts {\n \tstruct object_id squash_onto;\n \tint have_squash_onto;\n \n+\t/* Used to update rewritten-list */\n+\tstruct object_id rewritten_head;\n+\n \t/* Only used by REPLAY_NONE */\n \tstruct rev_info *revs;\n };\n@@ -188,7 +191,8 @@ int update_head_with_reflog(const struct commit *old_head,\n \t\t\t    struct strbuf *err);\n void commit_post_rewrite(struct repository *r,\n \t\t\t const struct commit *current_head,\n-\t\t\t const struct object_id *new_head);\n+\t\t\t const struct object_id *new_head,\n+\t\t\t struct object_id *rewritten_head);\n \n int prepare_branch_to_be_rebased(struct repository *r, struct replay_opts *opts,\n \t\t\t\t const char *commit);\ndiff --git a/t/lib-rebase.sh b/t/lib-rebase.sh\nindex 6d87961e41..9c0016848d 100644\n--- a/t/lib-rebase.sh\n+++ b/t/lib-rebase.sh\n@@ -49,7 +49,7 @@ set_fake_editor () {\n \t\tcase $line in\n \t\tpick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|r|merge|m)\n \t\t\taction=\"$line\";;\n-\t\texec_*|x_*|break|b)\n+\t\texec_*|x_*|break|b|reset_*|t_*)\n \t\t\techo \"$line\" | sed 's/_/ /g' >> \"$1\";;\n \t\t\"#\")\n \t\t\techo '# comment' >> \"$1\";;\ndiff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh\nindex 7344253bfb..18773709a3 100755\n--- a/t/t5407-post-rewrite-hook.sh\n+++ b/t/t5407-post-rewrite-hook.sh\n@@ -14,7 +14,11 @@ test_expect_success 'setup' '\n \tgit checkout A^0 &&\n \ttest_commit E bar E &&\n \ttest_commit F foo F &&\n-\tgit checkout master\n+\tgit checkout master &&\n+\n+\twrite_script amend-head <<-\\EOS\n+\tgit commit --amend --only --allow-empty -m \"$1\"\n+\tEOS\n '\n \n mkdir .git/hooks\n@@ -263,4 +267,68 @@ test_expect_success 'git rebase -i (exec)' '\n \tverify_hook_input\n '\n \n+test_expect_success 'git rebase -i (exec amends commit)' '\n+\tgit reset --hard D &&\n+\tclear_hook_input &&\n+\ttest_must_fail env FAKE_LINES=\"1 \\\n+\t\texec_./amend-head_edited-1a \\\n+\t\texec_./amend-head_edited-1b \\\n+\t\t2 \\\n+\t\texec_false \\\n+\t\t3 \\\n+\t\tbreak\" git rebase -i A &&\n+\t./amend-head edited-2 &&\n+\tgit rebase --continue &&\n+\t./amend-head edited-3 &&\n+\tgit rebase --continue &&\n+\techo rebase >expected.args &&\n+\tprintf \"%s %s\\n%s %s\\n%s %s\\n%s %s\\n%s %s\\n%s %s\\n\" >expected.data \\\n+\t\t$(git rev-parse B        HEAD@{6} \\\n+\t\t\t\tHEAD@{6} HEAD^^   \\\n+\t\t\t\tC        HEAD@{4} \\\n+\t\t\t\tHEAD@{4} HEAD^    \\\n+\t\t\t\tD        HEAD@{2} \\\n+\t\t\t\tHEAD@{2} HEAD) &&\n+\n+\tverify_hook_input\n+'\n+\n+test_expect_success 'git rebase -i (exec amends onto)' '\n+\tgit reset --hard D &&\n+\tclear_hook_input &&\n+\tFAKE_LINES=\"exec_./amend-head_edited 1 \\\n+\t\texec_git_commit_--allow-empty_-m_empty \\\n+\t\texec_./amend-head_edited-empty\" git rebase -i B &&\n+\techo rebase >expected.args &&\n+\tprintf \"%s %s\\n%s %s\\n\" >expected.data \\\n+\t\t$(git rev-parse B HEAD^^ \\\n+\t\t\t\tC HEAD^) &&\n+\tverify_hook_input\n+'\n+\n+test_expect_success 'git rebase -i (fixup after exec)' '\n+\tgit reset --hard D &&\n+\tclear_hook_input &&\n+\tFAKE_LINES=\"1 exec_true fixup 2 squash 3\" git rebase -i A &&\n+\techo rebase >expected.args &&\n+\tprintf \"%s %s\\n%s %s\\n%s %s\\n%s %s\\n\" >expected.data \\\n+\t\t$(git rev-parse B        HEAD@{2} \\\n+\t\t\t\tHEAD@{2} HEAD     \\\n+\t\t\t\tC        HEAD     \\\n+\t\t\t\tD        HEAD) &&\n+\tverify_hook_input\n+'\n+\n+test_expect_success 'git rebase -i (exec after reset)' '\n+\tgit reset --hard D &&\n+\tclear_hook_input &&\n+\tFAKE_LINES=\"reset_C \\\n+\t\texec_./amend-head_edited 3\" git rebase -i A &&\n+\techo rebase >expected.args &&\n+\tprintf \"%s %s\\n%s %s\\n\" >expected.data \\\n+\t\t$(git rev-parse C HEAD^ \\\n+\t\t\t\tD HEAD) &&\n+\tverify_hook_input\n+'\n+\n test_done\n-- \n2.30.1\n\n"},{"id":"418968","messageId":"87k0qcfthd.fsf@evledraar.gmail.com","threadId":"55295","inReplyTo":"20210312030107.1849942-3-gitster@pobox.com","subject":"Re: [PATCH 2/2] [WIP] sequencer.c: carry forward notes on HEAD across \"rebase -x\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-03-12T12:26:38Z","receivedAt":"2021-03-12T12:27:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 12 2021, Junio C Hamano wrote:\n\n> +# NEEDSWORK: Fix c762aada1ab3a2c428c with s/@/HEAD/;\n> +\n>  test_expect_success 'rebase -x with empty command fails' '\n>  \ttest_when_finished \"git rebase --abort ||:\" &&\n>  \ttest_must_fail env git rebase -x \"\" @ 2>actual &&\n> @@ -867,6 +869,22 @@ test_expect_success 'rebase -i can copy notes over a fixup' '\n>  \ttest_cmp expect output\n>  '\n\nI eyeballed c762aada1ab (rebase -x: sanity check command, 2019-01-29)\nfor a bit and still don't quite know what this HEAD v.s. @ is about in\nthat context, seems this is a stray FIXME comment for an unrelated test.\n\nMaybe it would be better to have test_expect_failure etc. here as\nappropriate?\n\n> +test_expect_success 'notes are copied even rebase -x changes HEAD' '\n> +\tgit reset --hard n3 &&\n> +\tgit rebase -x \"git commit --amend --no-edit\" n1^1 &&\n> +\tgit log --format=\"%s <%N>\" n1^1..n3 >expect &&\n> +\tgit log --format=\"%s <%N>\" n1^1..HEAD >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_failure 'notes are copied even rebase -x changes HEAD' '\n> +\tgit reset --hard n3 &&\n> +\tgit rebase -x \"git commit --amend -m tweak\" n1^1 &&\n> +\tgit log --format=\"tweak <%N>\" n1^1..n3 >expect &&\n> +\tgit log --format=\"%s <%N>\" n1^1..HEAD >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_expect_success 'rebase while detaching HEAD' '\n>  \tgit symbolic-ref HEAD &&\n>  \tgrandparent=$(git rev-parse HEAD~2) &&\n\n"}]}