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

[Phillip Wood <phillip.wood123@gmail.com>]

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.
>