[BUG] cmd given to "rebase -x cmd" cannot see the original HEAD?

[BUG] cmd given to "rebase -x cmd" cannot see the original HEAD?

[Junio C Hamano]

I was trying to figure out why I am losing the amlog note that
maps each commit on topics to the message ID after amending them
with extra trailers (e.g. Reviewed-by).  I use a script that runs
"git commit --amend" to do this and give it to "rebase -x".

It seems that when the command given to "rebase -x" is run, HEAD is
already updated to a commit with tree and log message identical to
the original, with updated committer information.  However, the
note attached to the original commit is *not* copied to this
intermediate HEAD that is shown to the external command.

Hence, even if I try to read the existing note from HEAD that is
given to the script spawned via "rebase -x cmd" mechanism, the
script cannot copy the note to the resulting commit after it adds
extra trailers.  Also, there is no mechansim for the external
command to learn where the original HEAD was.

I see a few possible avenues to fix this.

1. Before "rebase -x" mechanism spawns an external command, make
   sure that the "copy notes upon rewriting" logic has triggered, so
   that the HEAD observable by the command has the notes from the
   original.  It seems that the "copy notes" logic is only used at
   the end, but it does not seem to take the possibility into
   account that the external command might move HEAD to a different
   commit (i.e. it starts from HEAD0, creates HEAD1 that is
   different from HEAD0 only for committer info, hands HEAD1 to
   external command that may move HEAD to HEAD2, but then copies the
   note from HEAD0 to HEAD1 because it is unaware of HEAD2), so it
   only annotates HEAD1 that is immediately lost (nothing other than
   HEAD's reflog would be pointing at it). [*]

2. Make sure the "copy notes" logic copies the notes taken from the
   original HEAD to the HEAD _after_ "rebase -x" external command
   returns.

The first approach would still make the external command responsible
for copying notes from HEAD it sees (i.e. HEAD1) to the new commit
it creates and points HEAD at (i.e. HEAD2).  It _could_ be used as a
mechanism to allow the external command to modify/remove notes.

The second approach would forbid the external command from mucking
with notes at all.  "rebase" would first remember the original HEAD
(i.e. HEAD0), does its thing (i.e. creates HEAD1 and points HEAD at
it, and then lets the external command create HEAD2 and point HEAD
at it), and then figure out the latest HEAD and copy notes from the
original HEAD.  It is simpler, works with scripts that uses things
like "commit --amend" that do not copy notes, but less flexible.

Implementation-wise, it seems that the latter would require more
significant surgery to do_pick_commit() codepath, but somebody more
familiar with the sequencer codepath should research to find the
best approach.

I suspect that "post-commit" hook that is run from try_to_commit()
may have the same issue, here in sequencer.c

1539:	run_commit_hook(0, r->index_file, "post-commit", NULL);
1540:	if (flags & AMEND_MSG)
1541:		commit_post_rewrite(r, current_head, oid);

where oid is not adjusted by re-reading what the post-commit hook
did to our HEAD.

But the codepath to reach do_exec() (which is what spawns the
external command for "rebase -x cmd") is quite away from any
call to commit_post_rewrite(), 



[footnote]

I have a 6-commit series with notes/amlog record on each commit, and
running:

$ git rebase -x '
	git rev-parse HEAD
	git notes --ref=notes/amlog show HEAD || :
' master $(git rev-parse ab/read-tree)

yields the following:

Executing: git rev-parse HEAD; git notes --ref=notes/amlog show HEAD || :
669950d7def2e849cb1ee6e7b3a1beac5c45ce1c
error: no note found for object 669950d7def2e849cb1ee6e7b3a1beac5c45ce1c.
Executing: git rev-parse HEAD; git notes --ref=notes/amlog show HEAD || :
0545e0a567f96eff14dfb93b44aecf9683b44803
error: no note found for object 0545e0a567f96eff14dfb93b44aecf9683b44803.
Executing: git rev-parse HEAD; git notes --ref=notes/amlog show HEAD || :
d9b4e77efe5e65952e05533ebf500b559408d436
error: no note found for object d9b4e77efe5e65952e05533ebf500b559408d436.
Executing: git rev-parse HEAD; git notes --ref=notes/amlog show HEAD || :
6d8735372c0c9b9d3793959157456cd445d025fa
error: no note found for object 6d8735372c0c9b9d3793959157456cd445d025fa.
Executing: git rev-parse HEAD; git notes --ref=notes/amlog show HEAD || :
b86c1305258a11f42927f924498a94b3ffb19472
error: no note found for object b86c1305258a11f42927f924498a94b3ffb19472.
Executing: git rev-parse HEAD; git notes --ref=notes/amlog show HEAD || :
7da85bae9f11ab14d616bcade1fb2a7b434aa716
error: no note found for object 7da85bae9f11ab14d616bcade1fb2a7b434aa716.
Successfully rebased and updated detached HEAD.

We can see each commit on ab/read-tree has notes/amlog entry that
records where it came from:

$ git log --notes=notes/amlog --oneline --reverse master..ab/read-tree
2e3e38a4ad ls-files tests: add meaningful --with-tree tests
Notes (amlog):
    Message-Id: <20210308022138.28166-2-avarab@gmail.com>

2371fda438 tree.c API: move read_tree() into builtin/ls-files.c
Notes (amlog):
    Message-Id: <20210308022138.28166-3-avarab@gmail.com>

32c718db53 ls-files: don't needlessly pass around stage variable
Notes (amlog):
    Message-Id: <20210308022138.28166-4-avarab@gmail.com>

2ebabd63f7 ls-files: refactor away read_tree()
Notes (amlog):
    Message-Id: <20210308022138.28166-5-avarab@gmail.com>

e98fb6ddc7 tree.h API: remove support for starting at prefix != ""
Notes (amlog):
    Message-Id: <20210308022138.28166-6-avarab@gmail.com>

8e88702431 tree.h API: remove "stage" parameter from read_tree_recursive()
Notes (amlog):
    Message-Id: <20210308022138.28166-7-avarab@gmail.com>


In the above transcript, 669950d7def2e8 (the first commit that the
"rebase -x" command saw) corresponds to 2e3e38a4ad (the first commit
in the series), and we can see the only difference between them is
the committer info.  As we already saw in the above, the original
has a note that is not copied to the new one when "rebase -x"
command runs.

$ git show --pretty=fuller 2e3e38a4ad >old
$ git show --pretty=fuller 669950d7def2e8 >new
$ diff -u old new
--- old	2021-03-11 12:06:33.259434213 -0800
+++ new	2021-03-11 12:06:33.259434213 -0800
@@ -1,8 +1,8 @@
-commit 2e3e38a4ad6d9d78381e90348289277acb4c6f8b
+commit 669950d7def2e849cb1ee6e7b3a1beac5c45ce1c
 Author:     Ævar Arnfjörð Bjarmason <avarab@gmail.com>
 AuthorDate: Mon Mar 8 03:21:33 2021 +0100
 Commit:     Junio C Hamano <gitster@pobox.com>
-CommitDate: Mon Mar 8 10:06:35 2021 -0800
+CommitDate: Thu Mar 11 12:01:11 2021 -0800
 
     ls-files tests: add meaningful --with-tree tests
     

HOWEVER, after that failed "rebase -x" session, the note is copied
to these "intermediate" commits.  Using 7da85bae9f1 which is at the
tip of the history fed to "rebase -x" external command, we can see:

$ git log --notes=notes/amlog --oneline --reverse master..7da85bae9f1

669950d7de ls-files tests: add meaningful --with-tree tests
Notes (amlog):
    Message-Id: <20210308022138.28166-2-avarab@gmail.com>

0545e0a567 tree.c API: move read_tree() into builtin/ls-files.c
Notes (amlog):
    Message-Id: <20210308022138.28166-3-avarab@gmail.com>

d9b4e77efe ls-files: don't needlessly pass around stage variable
Notes (amlog):
    Message-Id: <20210308022138.28166-4-avarab@gmail.com>

6d8735372c ls-files: refactor away read_tree()
Notes (amlog):
    Message-Id: <20210308022138.28166-5-avarab@gmail.com>

b86c130525 tree.h API: remove support for starting at prefix != ""
Notes (amlog):
    Message-Id: <20210308022138.28166-6-avarab@gmail.com>

7da85bae9f tree.h API: remove "stage" parameter from read_tree_recursive()
Notes (amlog):
    Message-Id: <20210308022138.28166-7-avarab@gmail.com>


It is only that they seem to be added _after_ the external command
finishes, which is not a very useful behaviour.

[PATCH 0/2] "rebase -x cmd" loses notes

[Junio C Hamano]

So here is a semi-failed attempt to fix the issue I encountered
earlier, cf. https://lore.kernel.org/git/xmqq8s6tcuxc.fsf@gitster.g/
where "git rebase" carries forward notes attached to the commits in
the original history to their equivalents in the rewritten history,
but "git rebase -x cmd" does not when cmd modifies the given HEAD
commit.


Junio C Hamano (2):
  sequencer.c: make commit_post_rewrite() take two object names
  [WIP] sequencer.c: carry forward notes on HEAD across "rebase -x"

 builtin/commit.c              |  6 +++---
 sequencer.c                   | 19 +++++++++++++------
 sequencer.h                   |  2 +-
 t/t3404-rebase-interactive.sh | 18 ++++++++++++++++++
 4 files changed, 35 insertions(+), 10 deletions(-)

-- 
2.31.0-rc2-175-g3820f1c72e

[PATCH 1/2] sequencer.c: make commit_post_rewrite() take two object names

[Junio C Hamano]

The helper function is to copy notes between two commits by taking
the original one and a new one that is supposed to be a rewrite of
the original one.  It somehow takes "struct commit *" for the former
but "struct object_id *" for the latter, but what it does does not
need access to the in-core commit object.

Change the function to take two "struct object_id *" instead.

As we require "struct object_id *" for the original commit now,
the comment on the old "struct commit *" that must be non-NULL
which made taking the address of its object.oid member safe no
longer is relevant to this function (it is the caller's concern
now), so remove it.  Two callers we have are of course safe, and
there is no reason to call this "as we rewrote A to B, please copy
notes on A to B, too" helper if the caller knows A is NULL, so
the comment would have very little value even if it were kept.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/commit.c | 6 +++---
 sequencer.c      | 9 ++++-----
 sequencer.h      | 2 +-
 3 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 739110c5a7..f2fbef053e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1702,9 +1702,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	repo_rerere(the_repository, 0);
 	run_auto_maintenance(quiet);
 	run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);
-	if (amend && !no_post_rewrite) {
-		commit_post_rewrite(the_repository, current_head, &oid);
-	}
+	if (amend && !no_post_rewrite)
+		commit_post_rewrite(the_repository, &current_head->object.oid, &oid);
+
 	if (!quiet) {
 		unsigned int flags = 0;
 
diff --git a/sequencer.c b/sequencer.c
index d2332d3e17..92a4871997 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1175,18 +1175,17 @@ static int run_rewrite_hook(const struct object_id *oldoid,
 }
 
 void commit_post_rewrite(struct repository *r,
-			 const struct commit *old_head,
+			 const struct object_id *old_head,
 			 const struct object_id *new_head)
 {
 	struct notes_rewrite_cfg *cfg;
 
 	cfg = init_copy_notes_for_rewrite("amend");
 	if (cfg) {
-		/* we are amending, so old_head is not NULL */
-		copy_note_for_rewrite(cfg, &old_head->object.oid, new_head);
+		copy_note_for_rewrite(cfg, old_head, new_head);
 		finish_copy_notes_for_rewrite(r, cfg, "Notes added by 'git commit --amend'");
 	}
-	run_rewrite_hook(&old_head->object.oid, new_head);
+	run_rewrite_hook(old_head, new_head);
 }
 
 static int run_prepare_commit_msg_hook(struct repository *r,
@@ -1538,7 +1537,7 @@ static int try_to_commit(struct repository *r,
 
 	run_commit_hook(0, r->index_file, "post-commit", NULL);
 	if (flags & AMEND_MSG)
-		commit_post_rewrite(r, current_head, oid);
+		commit_post_rewrite(r, &current_head->object.oid, oid);
 
 out:
 	free_commit_extra_headers(extra);
diff --git a/sequencer.h b/sequencer.h
index f8b2e4ab85..79eed55c4b 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -192,7 +192,7 @@ int update_head_with_reflog(const struct commit *old_head,
 			    const char *action, const struct strbuf *msg,
 			    struct strbuf *err);
 void commit_post_rewrite(struct repository *r,
-			 const struct commit *current_head,
+			 const struct object_id *old_head,
 			 const struct object_id *new_head);
 
 void create_autostash(struct repository *r, const char *path,
-- 
2.31.0-rc2-175-g3820f1c72e

[PATCH 2/2] [WIP] sequencer.c: carry forward notes on HEAD across "rebase -x"

[Junio C Hamano]

When the external command invoked by "rebase -x" replaces the HEAD,
we seem to lose the notes attached to the original HEAD, which is
why we have been losing the amlog mappings in the git core project.

Here is a half-successful attempt to fix it.  For whatever reason,
when the external command is "git commit --amend --no-edit", the
updated code carries notes forward correctly, but when the command
is changed to "git commit --amend -m tweak", it fails to do so, and
I do not have more time to work on this, so I'd stop here with an
expected failure in the test.

Help is appreciated.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sequencer.c                   | 10 +++++++++-
 t/t3404-rebase-interactive.sh | 18 ++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 92a4871997..e0bdc39e4d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3276,7 +3276,10 @@ static int do_exec(struct repository *r, const char *command_line)
 {
 	struct strvec child_env = STRVEC_INIT;
 	const char *child_argv[] = { NULL, NULL };
-	int dirty, status;
+	int dirty, status, bad_head;
+	struct object_id old_head_oid, new_head_oid;
+
+	bad_head = get_oid("HEAD", &old_head_oid);
 
 	fprintf(stderr, _("Executing: %s\n"), command_line);
 	child_argv[0] = command_line;
@@ -3286,6 +3289,11 @@ static int do_exec(struct repository *r, const char *command_line)
 	status = run_command_v_opt_cd_env(child_argv, RUN_USING_SHELL, NULL,
 					  child_env.v);
 
+	bad_head |= get_oid("HEAD", &new_head_oid);
+
+	if (!bad_head && !oideq(&old_head_oid, &new_head_oid))
+		commit_post_rewrite(r, &old_head_oid, &new_head_oid);
+
 	/* force re-reading of the cache */
 	if (discard_index(r->index) < 0 || repo_read_index(r) < 0)
 		return error(_("could not read index"));
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 66bcbbf952..3222c594ab 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -155,6 +155,8 @@ test_expect_success 'rebase -i with the exec command checks tree cleanness' '
 	git rebase --continue
 '
 
+# NEEDSWORK: Fix c762aada1ab3a2c428c with s/@/HEAD/;
+
 test_expect_success 'rebase -x with empty command fails' '
 	test_when_finished "git rebase --abort ||:" &&
 	test_must_fail env git rebase -x "" @ 2>actual &&
@@ -867,6 +869,22 @@ test_expect_success 'rebase -i can copy notes over a fixup' '
 	test_cmp expect output
 '
 
+test_expect_success 'notes are copied even rebase -x changes HEAD' '
+	git reset --hard n3 &&
+	git rebase -x "git commit --amend --no-edit" n1^1 &&
+	git log --format="%s <%N>" n1^1..n3 >expect &&
+	git log --format="%s <%N>" n1^1..HEAD >actual &&
+	test_cmp expect actual
+'
+
+test_expect_failure 'notes are copied even rebase -x changes HEAD' '
+	git reset --hard n3 &&
+	git rebase -x "git commit --amend -m tweak" n1^1 &&
+	git log --format="tweak <%N>" n1^1..n3 >expect &&
+	git log --format="%s <%N>" n1^1..HEAD >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'rebase while detaching HEAD' '
 	git symbolic-ref HEAD &&
 	grandparent=$(git rev-parse HEAD~2) &&
-- 
2.31.0-rc2-175-g3820f1c72e

Re: [PATCH 2/2] [WIP] sequencer.c: carry forward notes on HEAD across "rebase -x"

[Phillip Wood]

On 12/03/2021 03:01, Junio C Hamano wrote:
> When the external command invoked by "rebase -x" replaces the HEAD,
> we seem to lose the notes attached to the original HEAD, which is
> why we have been losing the amlog mappings in the git core project.
> 
> Here is a half-successful attempt to fix it.  For whatever reason,
> when the external command is "git commit --amend --no-edit", the
> updated code carries notes forward correctly, but when the command
> is changed to "git commit --amend -m tweak", it fails to do so, and
> I do not have more time to work on this, so I'd stop here with an
> expected failure in the test.
> 
> Help is appreciated.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>   sequencer.c                   | 10 +++++++++-
>   t/t3404-rebase-interactive.sh | 18 ++++++++++++++++++
>   2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 92a4871997..e0bdc39e4d 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3276,7 +3276,10 @@ static int do_exec(struct repository *r, const char *command_line)
>   {
>   	struct strvec child_env = STRVEC_INIT;
>   	const char *child_argv[] = { NULL, NULL };
> -	int dirty, status;
> +	int dirty, status, bad_head;
> +	struct object_id old_head_oid, new_head_oid;
> +
> +	bad_head = get_oid("HEAD", &old_head_oid);
>   
>   	fprintf(stderr, _("Executing: %s\n"), command_line);
>   	child_argv[0] = command_line;
> @@ -3286,6 +3289,11 @@ static int do_exec(struct repository *r, const char *command_line)
>   	status = run_command_v_opt_cd_env(child_argv, RUN_USING_SHELL, NULL,
>   					  child_env.v);
>   
> +	bad_head |= get_oid("HEAD", &new_head_oid);
> +
> +	if (!bad_head && !oideq(&old_head_oid, &new_head_oid))
> +		commit_post_rewrite(r, &old_head_oid, &new_head_oid);

If HEAD has changed then we copy the notes - but we never copied the 
notes from the original commit to the rewritten commit before running 
the exec command so this does not do what we want. I think this is why 
the second test below fails. I think the first test passes because the 
author and committer identities and dates of the amended commit are 
unchanged from the rebased commit so we end up copying the notes at the 
end of the rebase because the oid is unchanged.

Blindly copying the notes here would mean that the notes would be copied 
if an exec command created a new commit on top of HEAD rather than 
amending HEAD which is not what we want.

Best Wishes

Phillip

>   	/* force re-reading of the cache */
>   	if (discard_index(r->index) < 0 || repo_read_index(r) < 0)
>   		return error(_("could not read index"));
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 66bcbbf952..3222c594ab 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -155,6 +155,8 @@ test_expect_success 'rebase -i with the exec command checks tree cleanness' '
>   	git rebase --continue
>   '
>   
> +# NEEDSWORK: Fix c762aada1ab3a2c428c with s/@/HEAD/;
> +
>   test_expect_success 'rebase -x with empty command fails' '
>   	test_when_finished "git rebase --abort ||:" &&
>   	test_must_fail env git rebase -x "" @ 2>actual &&
> @@ -867,6 +869,22 @@ test_expect_success 'rebase -i can copy notes over a fixup' '
>   	test_cmp expect output
>   '
>   
> +test_expect_success 'notes are copied even rebase -x changes HEAD' '
> +	git reset --hard n3 &&
> +	git rebase -x "git commit --amend --no-edit" n1^1 &&
> +	git log --format="%s <%N>" n1^1..n3 >expect &&
> +	git log --format="%s <%N>" n1^1..HEAD >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_failure 'notes are copied even rebase -x changes HEAD' '
> +	git reset --hard n3 &&
> +	git rebase -x "git commit --amend -m tweak" n1^1 &&
> +	git log --format="tweak <%N>" n1^1..n3 >expect &&
> +	git log --format="%s <%N>" n1^1..HEAD >actual &&
> +	test_cmp expect actual
> +'
> +
>   test_expect_success 'rebase while detaching HEAD' '
>   	git symbolic-ref HEAD &&
>   	grandparent=$(git rev-parse HEAD~2) &&
> 

Re: [PATCH 2/2] [WIP] sequencer.c: carry forward notes on HEAD across "rebase -x"

[Ævar Arnfjörð Bjarmason]

On Fri, Mar 12 2021, Junio C Hamano wrote:

> +# NEEDSWORK: Fix c762aada1ab3a2c428c with s/@/HEAD/;
> +
>  test_expect_success 'rebase -x with empty command fails' '
>  	test_when_finished "git rebase --abort ||:" &&
>  	test_must_fail env git rebase -x "" @ 2>actual &&
> @@ -867,6 +869,22 @@ test_expect_success 'rebase -i can copy notes over a fixup' '
>  	test_cmp expect output
>  '

I eyeballed c762aada1ab (rebase -x: sanity check command, 2019-01-29)
for a bit and still don't quite know what this HEAD v.s. @ is about in
that context, seems this is a stray FIXME comment for an unrelated test.

Maybe it would be better to have test_expect_failure etc. here as
appropriate?

> +test_expect_success 'notes are copied even rebase -x changes HEAD' '
> +	git reset --hard n3 &&
> +	git rebase -x "git commit --amend --no-edit" n1^1 &&
> +	git log --format="%s <%N>" n1^1..n3 >expect &&
> +	git log --format="%s <%N>" n1^1..HEAD >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_failure 'notes are copied even rebase -x changes HEAD' '
> +	git reset --hard n3 &&
> +	git rebase -x "git commit --amend -m tweak" n1^1 &&
> +	git log --format="tweak <%N>" n1^1..n3 >expect &&
> +	git log --format="%s <%N>" n1^1..HEAD >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'rebase while detaching HEAD' '
>  	git symbolic-ref HEAD &&
>  	grandparent=$(git rev-parse HEAD~2) &&

Re: [BUG] cmd given to "rebase -x cmd" cannot see the original HEAD?

[Phillip Wood]

Hi Junio

On 11/03/2021 20:11, Junio C Hamano wrote:
> I was trying to figure out why I am losing the amlog note that
> maps each commit on topics to the message ID after amending them
> with extra trailers (e.g. Reviewed-by).  I use a script that runs
> "git commit --amend" to do this and give it to "rebase -x".
> 
> It seems that when the command given to "rebase -x" is run, HEAD is
> already updated to a commit with tree and log message identical to
> the original, with updated committer information.  However, the
> note attached to the original commit is *not* copied to this
> intermediate HEAD that is shown to the external command.

Yes this is annoying I've sometimes wanted to edit the notes when the 
rebase is stopped for for 'edit' or 'break' commands

> Hence, even if I try to read the existing note from HEAD that is
> given to the script spawned via "rebase -x cmd" mechanism, the
> script cannot copy the note to the resulting commit after it adds
> extra trailers.  Also, there is no mechansim for the external
> command to learn where the original HEAD was.
> 
> I see a few possible avenues to fix this.
> 
> 1. Before "rebase -x" mechanism spawns an external command, make
>     sure that the "copy notes upon rewriting" logic has triggered, so
>     that the HEAD observable by the command has the notes from the
>     original.

That would be my preferred way to fix this, but we should also copy the 
notes before stopping for an 'edit' or 'break' and maybe 'reword' so 
that the notes are always available when a rebase stops for some reason 
other than conflict resolution.

>  It seems that the "copy notes" logic is only used at
>     the end, but it does not seem to take the possibility into
>     account that the external command might move HEAD to a different
>     commit (i.e. it starts from HEAD0, creates HEAD1 that is
>     different from HEAD0 only for committer info, hands HEAD1 to
>     external command that may move HEAD to HEAD2, but then copies the
>     note from HEAD0 to HEAD1 because it is unaware of HEAD2), so it
>     only annotates HEAD1 that is immediately lost (nothing other than
>     HEAD's reflog would be pointing at it). [*]

I think the root cause is that 'git commit --amend' does not update the 
list of rewritten commits when run during a rebase, which messes up the 
post-rewrite hooks (as you suspected) and note copying which is 
essentially a built-in post-rewrite hook. I've got a half finished 
solution but there are some corner cases that I've never got round to 
addressing. It would fix your problem but not the problem of making 
notes available to the user during the rebase.


Best Wishes

Phillip

> 2. Make sure the "copy notes" logic copies the notes taken from the
>     original HEAD to the HEAD _after_ "rebase -x" external command
>     returns.
> 
> The first approach would still make the external command responsible
> for copying notes from HEAD it sees (i.e. HEAD1) to the new commit
> it creates and points HEAD at (i.e. HEAD2).  It _could_ be used as a
> mechanism to allow the external command to modify/remove notes.
> 
> The second approach would forbid the external command from mucking
> with notes at all.  "rebase" would first remember the original HEAD
> (i.e. HEAD0), does its thing (i.e. creates HEAD1 and points HEAD at
> it, and then lets the external command create HEAD2 and point HEAD
> at it), and then figure out the latest HEAD and copy notes from the
> original HEAD.  It is simpler, works with scripts that uses things
> like "commit --amend" that do not copy notes, but less flexible.
> 
> Implementation-wise, it seems that the latter would require more
> significant surgery to do_pick_commit() codepath, but somebody more
> familiar with the sequencer codepath should research to find the
> best approach.
> 
> I suspect that "post-commit" hook that is run from try_to_commit()
> may have the same issue, here in sequencer.c
> 
> 1539:	run_commit_hook(0, r->index_file, "post-commit", NULL);
> 1540:	if (flags & AMEND_MSG)
> 1541:		commit_post_rewrite(r, current_head, oid);
> 
> where oid is not adjusted by re-reading what the post-commit hook
> did to our HEAD.
> 
> But the codepath to reach do_exec() (which is what spawns the
> external command for "rebase -x cmd") is quite away from any
> call to commit_post_rewrite(),
> 
> 
> 
> [footnote]
> 
> I have a 6-commit series with notes/amlog record on each commit, and
> running:
> 
> $ git rebase -x '
> 	git rev-parse HEAD
> 	git notes --ref=notes/amlog show HEAD || :
> ' master $(git rev-parse ab/read-tree)
> 
> yields the following:
> 
> Executing: git rev-parse HEAD; git notes --ref=notes/amlog show HEAD || :
> 669950d7def2e849cb1ee6e7b3a1beac5c45ce1c
> error: no note found for object 669950d7def2e849cb1ee6e7b3a1beac5c45ce1c.
> Executing: git rev-parse HEAD; git notes --ref=notes/amlog show HEAD || :
> 0545e0a567f96eff14dfb93b44aecf9683b44803
> error: no note found for object 0545e0a567f96eff14dfb93b44aecf9683b44803.
> Executing: git rev-parse HEAD; git notes --ref=notes/amlog show HEAD || :
> d9b4e77efe5e65952e05533ebf500b559408d436
> error: no note found for object d9b4e77efe5e65952e05533ebf500b559408d436.
> Executing: git rev-parse HEAD; git notes --ref=notes/amlog show HEAD || :
> 6d8735372c0c9b9d3793959157456cd445d025fa
> error: no note found for object 6d8735372c0c9b9d3793959157456cd445d025fa.
> Executing: git rev-parse HEAD; git notes --ref=notes/amlog show HEAD || :
> b86c1305258a11f42927f924498a94b3ffb19472
> error: no note found for object b86c1305258a11f42927f924498a94b3ffb19472.
> Executing: git rev-parse HEAD; git notes --ref=notes/amlog show HEAD || :
> 7da85bae9f11ab14d616bcade1fb2a7b434aa716
> error: no note found for object 7da85bae9f11ab14d616bcade1fb2a7b434aa716.
> Successfully rebased and updated detached HEAD.
> 
> We can see each commit on ab/read-tree has notes/amlog entry that
> records where it came from:
> 
> $ git log --notes=notes/amlog --oneline --reverse master..ab/read-tree
> 2e3e38a4ad ls-files tests: add meaningful --with-tree tests
> Notes (amlog):
>      Message-Id: <20210308022138.28166-2-avarab@gmail.com>
> 
> 2371fda438 tree.c API: move read_tree() into builtin/ls-files.c
> Notes (amlog):
>      Message-Id: <20210308022138.28166-3-avarab@gmail.com>
> 
> 32c718db53 ls-files: don't needlessly pass around stage variable
> Notes (amlog):
>      Message-Id: <20210308022138.28166-4-avarab@gmail.com>
> 
> 2ebabd63f7 ls-files: refactor away read_tree()
> Notes (amlog):
>      Message-Id: <20210308022138.28166-5-avarab@gmail.com>
> 
> e98fb6ddc7 tree.h API: remove support for starting at prefix != ""
> Notes (amlog):
>      Message-Id: <20210308022138.28166-6-avarab@gmail.com>
> 
> 8e88702431 tree.h API: remove "stage" parameter from read_tree_recursive()
> Notes (amlog):
>      Message-Id: <20210308022138.28166-7-avarab@gmail.com>
> 
> 
> In the above transcript, 669950d7def2e8 (the first commit that the
> "rebase -x" command saw) corresponds to 2e3e38a4ad (the first commit
> in the series), and we can see the only difference between them is
> the committer info.  As we already saw in the above, the original
> has a note that is not copied to the new one when "rebase -x"
> command runs.
> 
> $ git show --pretty=fuller 2e3e38a4ad >old
> $ git show --pretty=fuller 669950d7def2e8 >new
> $ diff -u old new
> --- old	2021-03-11 12:06:33.259434213 -0800
> +++ new	2021-03-11 12:06:33.259434213 -0800
> @@ -1,8 +1,8 @@
> -commit 2e3e38a4ad6d9d78381e90348289277acb4c6f8b
> +commit 669950d7def2e849cb1ee6e7b3a1beac5c45ce1c
>   Author:     Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>   AuthorDate: Mon Mar 8 03:21:33 2021 +0100
>   Commit:     Junio C Hamano <gitster@pobox.com>
> -CommitDate: Mon Mar 8 10:06:35 2021 -0800
> +CommitDate: Thu Mar 11 12:01:11 2021 -0800
>   
>       ls-files tests: add meaningful --with-tree tests
>       
> 
> HOWEVER, after that failed "rebase -x" session, the note is copied
> to these "intermediate" commits.  Using 7da85bae9f1 which is at the
> tip of the history fed to "rebase -x" external command, we can see:
> 
> $ git log --notes=notes/amlog --oneline --reverse master..7da85bae9f1
> 
> 669950d7de ls-files tests: add meaningful --with-tree tests
> Notes (amlog):
>      Message-Id: <20210308022138.28166-2-avarab@gmail.com>
> 
> 0545e0a567 tree.c API: move read_tree() into builtin/ls-files.c
> Notes (amlog):
>      Message-Id: <20210308022138.28166-3-avarab@gmail.com>
> 
> d9b4e77efe ls-files: don't needlessly pass around stage variable
> Notes (amlog):
>      Message-Id: <20210308022138.28166-4-avarab@gmail.com>
> 
> 6d8735372c ls-files: refactor away read_tree()
> Notes (amlog):
>      Message-Id: <20210308022138.28166-5-avarab@gmail.com>
> 
> b86c130525 tree.h API: remove support for starting at prefix != ""
> Notes (amlog):
>      Message-Id: <20210308022138.28166-6-avarab@gmail.com>
> 
> 7da85bae9f tree.h API: remove "stage" parameter from read_tree_recursive()
> Notes (amlog):
>      Message-Id: <20210308022138.28166-7-avarab@gmail.com>
> 
> 
> It is only that they seem to be added _after_ the external command
> finishes, which is not a very useful behaviour.
> 

[WIP][PATCH] rebase: update the list of rewritten commits when amending pick

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If HEAD is amended during an exec command then the amended commit is missing
from the list of rewritten commits. The commonest way for this to happen is if a
commit is amended to fix a test failure when running `git rebase --exec "make
test"` but it can also happen if the exec command calls `git commit --amend`
directly. Amending commits with exec commands was discussed on the mailing list
recently where someone wanted to reset the author before submitting patches
upstream[1].

[1] https://public-inbox.org/git/CABPp-BEYRmhrb4Tx3bGzkx8y53T_0BYhLE5J0cEmxj18WtZs9A@mail.gmail.com/#t

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---

This is what I've been using for a couple of years to update the list
of rewritten commits when running "git commit --amend" during a
rebase. I think it changes which commit gets used as the rewritten one
if you split a commit with 'edit', the patch is so old I cannot
remember if there were any other corner cases.


 builtin/commit.c             |   2 +-
 sequencer.c                  | 119 +++++++++++++++++++++++++++++------
 sequencer.h                  |   6 +-
 t/lib-rebase.sh              |   2 +-
 t/t5407-post-rewrite-hook.sh |  70 ++++++++++++++++++++-
 5 files changed, 176 insertions(+), 23 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index ae7aaf6dc6..9b6f3d8b6b 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1697,7 +1697,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
 	run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);
 	if (amend && !no_post_rewrite) {
-		commit_post_rewrite(the_repository, current_head, &oid);
+		commit_post_rewrite(the_repository, current_head, &oid, NULL);
 	}
 	if (!quiet) {
 		unsigned int flags = 0;
diff --git a/sequencer.c b/sequencer.c
index b395dd6e11..5d68e7341d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -129,6 +129,7 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha, "rebase-merge/stopped-sha")
 static GIT_PATH_FUNC(rebase_path_rewritten_list, "rebase-merge/rewritten-list")
 static GIT_PATH_FUNC(rebase_path_rewritten_pending,
 	"rebase-merge/rewritten-pending")
+static GIT_PATH_FUNC(rebase_path_rewritten_head, "rebase-merge/rewritten-head")
 
 /*
  * The path of the file containig the OID of the "squash onto" commit, i.e.
@@ -159,6 +160,9 @@ static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
 static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
 static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, "rebase-merge/reschedule-failed-exec")
 
+static void write_rewritten_head(struct object_id *rewritten_head);
+static void read_rewritten_head(struct object_id *rewritten_head);
+
 static int git_sequencer_config(const char *k, const char *v, void *cb)
 {
 	struct replay_opts *opts = cb;
@@ -953,12 +957,12 @@ static int run_git_commit(struct repository *r,
 			  unsigned int flags)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
+	int res = 0;
 
 	if ((flags & CREATE_ROOT_COMMIT) && !(flags & AMEND_MSG)) {
 		struct strbuf msg = STRBUF_INIT, script = STRBUF_INIT;
 		const char *author = NULL;
 		struct object_id root_commit, *cache_tree_oid;
-		int res = 0;
 
 		if (is_rebase_i(opts)) {
 			author = read_author_ident(&script);
@@ -1004,6 +1008,9 @@ static int run_git_commit(struct repository *r,
 			     gpg_opt, gpg_opt);
 	}
 
+	if (is_rebase_i(opts) && (flags & AMEND_MSG))
+		write_rewritten_head(&opts->rewritten_head);
+
 	argv_array_push(&cmd.args, "commit");
 
 	if (!(flags & VERIFY_MSG))
@@ -1032,9 +1039,14 @@ static int run_git_commit(struct repository *r,
 		argv_array_push(&cmd.args, "--allow-empty-message");
 
 	if (is_rebase_i(opts) && !(flags & EDIT_MSG))
-		return run_command_silent_on_success(&cmd);
+		res = run_command_silent_on_success(&cmd);
 	else
-		return run_command(&cmd);
+		res = run_command(&cmd);
+
+	if (is_rebase_i(opts) && !res && (flags & AMEND_MSG))
+		read_rewritten_head(&opts->rewritten_head);
+
+	return res;
 }
 
 static int rest_is_empty(const struct strbuf *sb, int start)
@@ -1177,12 +1189,42 @@ static int run_rewrite_hook(const struct object_id *oldoid,
 	return finish_command(&proc);
 }
 
+static void update_rewritten(const struct repository *r,
+			     const struct object_id *old_head,
+			     const struct object_id *new_head,
+			     struct object_id *rewritten_head)
+{
+	struct object_id oid;
+
+	if (!rewritten_head) {
+		read_rewritten_head(&oid);
+		rewritten_head = &oid;
+	}
+	if (oideq(old_head, rewritten_head)) {
+		FILE *fp;
+		fp = fopen_or_warn(rebase_path_rewritten_list(), "a");
+		if (fp) {
+			fprintf(fp, "%s %s\n",
+			    oid_to_hex(old_head), oid_to_hex(new_head));
+			fclose(fp);
+		}
+		oidcpy(rewritten_head, new_head);
+	}
+	if (rewritten_head == &oid)
+		write_rewritten_head(rewritten_head);
+
+	return;
+}
+
 void commit_post_rewrite(struct repository *r,
 			 const struct commit *old_head,
-			 const struct object_id *new_head)
+			 const struct object_id *new_head,
+			 struct object_id *rewritten_head)
 {
 	struct notes_rewrite_cfg *cfg;
 
+	update_rewritten(r, &old_head->object.oid, new_head, rewritten_head);
+
 	cfg = init_copy_notes_for_rewrite("amend");
 	if (cfg) {
 		/* we are amending, so old_head is not NULL */
@@ -1473,7 +1515,8 @@ static int try_to_commit(struct repository *r,
 	}
 
 	if (flags & AMEND_MSG)
-		commit_post_rewrite(r, current_head, oid);
+		commit_post_rewrite(r, current_head, oid,
+				    &opts->rewritten_head);
 
 out:
 	free_commit_extra_headers(extra);
@@ -1731,7 +1774,7 @@ static int update_squash_messages(struct repository *r,
 	return res;
 }
 
-static void flush_rewritten_pending(void)
+static void flush_rewritten_pending(struct object_id *rewritten_head)
 {
 	struct strbuf buf = STRBUF_INIT;
 	struct object_id newoid;
@@ -1752,12 +1795,14 @@ static void flush_rewritten_pending(void)
 		}
 		fclose(out);
 		unlink(rebase_path_rewritten_pending());
+		oidcpy(rewritten_head, &newoid);
 	}
 	strbuf_release(&buf);
 }
 
 static void record_in_rewritten(struct object_id *oid,
-		enum todo_command next_command)
+				enum todo_command next_command,
+				struct object_id *rewritten_head)
 {
 	FILE *out = fopen_or_warn(rebase_path_rewritten_pending(), "a");
 
@@ -1768,7 +1813,7 @@ static void record_in_rewritten(struct object_id *oid,
 	fclose(out);
 
 	if (!is_fixup(next_command))
-		flush_rewritten_pending();
+		flush_rewritten_pending(rewritten_head);
 }
 
 static int do_pick_commit(struct repository *r,
@@ -2510,6 +2555,11 @@ static int read_populate_opts(struct replay_opts *opts)
 	if (is_rebase_i(opts)) {
 		struct strbuf buf = STRBUF_INIT;
 
+		if (file_exists(rebase_path_rewritten_head()))
+			read_rewritten_head(&opts->rewritten_head);
+		else
+			opts->rewritten_head = null_oid;
+
 		if (read_oneliner(&buf, rebase_path_gpg_sign_opt(), 1)) {
 			if (!starts_with(buf.buf, "-S"))
 				strbuf_reset(&buf);
@@ -3065,6 +3115,7 @@ static int error_with_patch(struct repository *r,
 			    struct replay_opts *opts,
 			    int exit_code, int to_amend)
 {
+	write_rewritten_head(&opts->rewritten_head);
 	if (commit) {
 		if (make_patch(r, commit, opts))
 			return -1;
@@ -3119,12 +3170,14 @@ static int error_failed_squash(struct repository *r,
 	return error_with_patch(r, commit, subject, subject_len, opts, 1, 0);
 }
 
-static int do_exec(struct repository *r, const char *command_line)
+static int do_exec(struct repository *r, const char *command_line,
+		   struct object_id *rewritten_head)
 {
 	struct argv_array child_env = ARGV_ARRAY_INIT;
 	const char *child_argv[] = { NULL, NULL };
 	int dirty, status;
 
+	write_rewritten_head(rewritten_head);
 	fprintf(stderr, "Executing: %s\n", command_line);
 	child_argv[0] = command_line;
 	argv_array_pushf(&child_env, "GIT_DIR=%s", absolute_path(get_git_dir()));
@@ -3133,6 +3186,7 @@ static int do_exec(struct repository *r, const char *command_line)
 	status = run_command_v_opt_cd_env(child_argv, RUN_USING_SHELL, NULL,
 					  child_env.argv);
 
+	read_rewritten_head(rewritten_head);
 	/* force re-reading of the cache */
 	if (discard_index(r->index) < 0 || repo_read_index(r) < 0)
 		return error(_("could not read index"));
@@ -3331,10 +3385,12 @@ static int do_reset(struct repository *r,
 		ret = error(_("could not write index"));
 	free((void *)desc.buffer);
 
-	if (!ret)
+	if (!ret) {
 		ret = update_ref(reflog_message(opts, "reset", "'%.*s'",
 						len, name), "HEAD", &oid,
 				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
+		oidcpy(&opts->rewritten_head, &oid);
+	}
 
 	strbuf_release(&ref_name);
 	return ret;
@@ -3862,6 +3918,7 @@ static int pick_commits(struct repository *r,
 			delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF);
 
 			if (item->command == TODO_BREAK) {
+				write_rewritten_head(&opts->rewritten_head);
 				if (!opts->verbose)
 					term_clear_line();
 				return stopped_at_head(r);
@@ -3900,7 +3957,8 @@ static int pick_commits(struct repository *r,
 			}
 			if (is_rebase_i(opts) && !res)
 				record_in_rewritten(&item->commit->object.oid,
-					peek_command(todo_list, 1));
+						    peek_command(todo_list, 1),
+						    &opts->rewritten_head);
 			if (res && is_fixup(item->command)) {
 				if (res == 1)
 					intend_to_amend();
@@ -3935,7 +3993,7 @@ static int pick_commits(struct repository *r,
 			if (!opts->verbose)
 				term_clear_line();
 			*end_of_arg = '\0';
-			res = do_exec(r, arg);
+			res = do_exec(r, arg, &opts->rewritten_head);
 			*end_of_arg = saved;
 
 			if (res) {
@@ -3965,7 +4023,8 @@ static int pick_commits(struct repository *r,
 				reschedule = 1;
 			else if (item->commit)
 				record_in_rewritten(&item->commit->object.oid,
-						    peek_command(todo_list, 1));
+						    peek_command(todo_list, 1),
+						    &opts->rewritten_head);
 			if (res > 0)
 				/* failed with merge conflicts */
 				return error_with_patch(r, item->commit,
@@ -4062,7 +4121,7 @@ static int pick_commits(struct repository *r,
 				log_tree_diff_flush(&log_tree_opt);
 			}
 		}
-		flush_rewritten_pending();
+		flush_rewritten_pending(&opts->rewritten_head);
 		if (!stat(rebase_path_rewritten_list(), &st) &&
 				st.st_size > 0) {
 			struct child_process child = CHILD_PROCESS_INIT;
@@ -4299,7 +4358,8 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
 
 		if (read_oneliner(&buf, rebase_path_stopped_sha(), 1) &&
 		    !get_oid_committish(buf.buf, &oid))
-			record_in_rewritten(&oid, peek_command(&todo_list, 0));
+			record_in_rewritten(&oid, peek_command(&todo_list, 0),
+					    &opts->rewritten_head);
 		strbuf_release(&buf);
 	}
 
@@ -5024,7 +5084,8 @@ int check_todo_list_from_file(struct repository *r)
 /* skip picking commits whose parents are unchanged */
 static int skip_unnecessary_picks(struct repository *r,
 				  struct todo_list *todo_list,
-				  struct object_id *base_oid)
+				  struct object_id *base_oid,
+				  struct object_id *rewritten_head)
 {
 	struct object_id *parent_oid;
 	int i;
@@ -5062,8 +5123,15 @@ static int skip_unnecessary_picks(struct repository *r,
 		todo_list->current = 0;
 		todo_list->done_nr += i;
 
-		if (is_fixup(peek_command(todo_list, 0)))
-			record_in_rewritten(base_oid, peek_command(todo_list, 0));
+		if (is_fixup(peek_command(todo_list, 0))) {
+			record_in_rewritten(base_oid, peek_command(todo_list, 0),
+					    rewritten_head);
+			oidcpy(rewritten_head, &null_oid);
+		} else {
+			oidcpy(rewritten_head, base_oid);
+		}
+	} else {
+		oidcpy(rewritten_head, base_oid);
 	}
 
 	return 0;
@@ -5129,7 +5197,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 		return -1;
 	}
 
-	if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid)) {
+	if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid,
+						     &opts->rewritten_head)) {
 		todo_list_release(&new_todo);
 		return error(_("could not skip unnecessary pick commands"));
 	}
@@ -5315,3 +5384,15 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
 
 	return 0;
 }
+static void write_rewritten_head(struct object_id *rewritten_head)
+{
+	const char *hex = oid_to_hex(rewritten_head);
+	write_message(hex, strlen(hex), rebase_path_rewritten_head(), 1);
+}
+
+static void read_rewritten_head(struct object_id *rewritten_head)
+{
+	struct strbuf buf = STRBUF_INIT;
+	read_oneliner(&buf, rebase_path_rewritten_head(), 0);
+	get_oid(buf.buf, rewritten_head);
+}
diff --git a/sequencer.h b/sequencer.h
index 574260f621..91834cfe1c 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -63,6 +63,9 @@ struct replay_opts {
 	struct object_id squash_onto;
 	int have_squash_onto;
 
+	/* Used to update rewritten-list */
+	struct object_id rewritten_head;
+
 	/* Only used by REPLAY_NONE */
 	struct rev_info *revs;
 };
@@ -188,7 +191,8 @@ int update_head_with_reflog(const struct commit *old_head,
 			    struct strbuf *err);
 void commit_post_rewrite(struct repository *r,
 			 const struct commit *current_head,
-			 const struct object_id *new_head);
+			 const struct object_id *new_head,
+			 struct object_id *rewritten_head);
 
 int prepare_branch_to_be_rebased(struct repository *r, struct replay_opts *opts,
 				 const char *commit);
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 6d87961e41..9c0016848d 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -49,7 +49,7 @@ set_fake_editor () {
 		case $line in
 		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|r|merge|m)
 			action="$line";;
-		exec_*|x_*|break|b)
+		exec_*|x_*|break|b|reset_*|t_*)
 			echo "$line" | sed 's/_/ /g' >> "$1";;
 		"#")
 			echo '# comment' >> "$1";;
diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
index 7344253bfb..18773709a3 100755
--- a/t/t5407-post-rewrite-hook.sh
+++ b/t/t5407-post-rewrite-hook.sh
@@ -14,7 +14,11 @@ test_expect_success 'setup' '
 	git checkout A^0 &&
 	test_commit E bar E &&
 	test_commit F foo F &&
-	git checkout master
+	git checkout master &&
+
+	write_script amend-head <<-\EOS
+	git commit --amend --only --allow-empty -m "$1"
+	EOS
 '
 
 mkdir .git/hooks
@@ -263,4 +267,68 @@ test_expect_success 'git rebase -i (exec)' '
 	verify_hook_input
 '
 
+test_expect_success 'git rebase -i (exec amends commit)' '
+	git reset --hard D &&
+	clear_hook_input &&
+	test_must_fail env FAKE_LINES="1 \
+		exec_./amend-head_edited-1a \
+		exec_./amend-head_edited-1b \
+		2 \
+		exec_false \
+		3 \
+		break" git rebase -i A &&
+	./amend-head edited-2 &&
+	git rebase --continue &&
+	./amend-head edited-3 &&
+	git rebase --continue &&
+	echo rebase >expected.args &&
+	printf "%s %s\n%s %s\n%s %s\n%s %s\n%s %s\n%s %s\n" >expected.data \
+		$(git rev-parse B        HEAD@{6} \
+				HEAD@{6} HEAD^^   \
+				C        HEAD@{4} \
+				HEAD@{4} HEAD^    \
+				D        HEAD@{2} \
+				HEAD@{2} HEAD) &&
+
+	verify_hook_input
+'
+
+test_expect_success 'git rebase -i (exec amends onto)' '
+	git reset --hard D &&
+	clear_hook_input &&
+	FAKE_LINES="exec_./amend-head_edited 1 \
+		exec_git_commit_--allow-empty_-m_empty \
+		exec_./amend-head_edited-empty" git rebase -i B &&
+	echo rebase >expected.args &&
+	printf "%s %s\n%s %s\n" >expected.data \
+		$(git rev-parse B HEAD^^ \
+				C HEAD^) &&
+	verify_hook_input
+'
+
+test_expect_success 'git rebase -i (fixup after exec)' '
+	git reset --hard D &&
+	clear_hook_input &&
+	FAKE_LINES="1 exec_true fixup 2 squash 3" git rebase -i A &&
+	echo rebase >expected.args &&
+	printf "%s %s\n%s %s\n%s %s\n%s %s\n" >expected.data \
+		$(git rev-parse B        HEAD@{2} \
+				HEAD@{2} HEAD     \
+				C        HEAD     \
+				D        HEAD) &&
+	verify_hook_input
+'
+
+test_expect_success 'git rebase -i (exec after reset)' '
+	git reset --hard D &&
+	clear_hook_input &&
+	FAKE_LINES="reset_C \
+		exec_./amend-head_edited 3" git rebase -i A &&
+	echo rebase >expected.args &&
+	printf "%s %s\n%s %s\n" >expected.data \
+		$(git rev-parse C HEAD^ \
+				D HEAD) &&
+	verify_hook_input
+'
+
 test_done
-- 
2.30.1