[PATCH] sequencer: Skip copying notes for commits that disappear during rebase

[PATCH] sequencer: Skip copying notes for commits that disappear during rebase

[Uwe Kleine-König]

When a commit disappears during rebase because the patch content is
already there (but not by the same patch in which case the commit would
be skipped) the notes of that disappearing commit should not be copied
to the unrelated commit that happens to be HEAD.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
Hello,

after also my 2nd bug report[1] didn't motivate anyone to come up with a
fix, I invested the time to work out one according to Phillip Wood's
suggestion.

IMHO it's not pretty, but it works for me.

Note that Phillip also suggested to integrete the test into
t3400-rebase.sh . IMHO it doesn't matter much if this is considered a
rebase test or a notes test. I kept it where I have it because I'm lazy
and failed to understand the git history created in that test.

Best regards
Uwe

[1] https://lore.kernel.org/git/20260612143952.3281115-2-u.kleine-koenig@baylibre.com


 sequencer.c             | 20 ++++++++++----------
 t/meson.build           |  1 +
 t/t3322-notes-rebase.sh | 37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 48 insertions(+), 10 deletions(-)
 create mode 100755 t/t3322-notes-rebase.sh

diff --git a/sequencer.c b/sequencer.c
index 57855b0066ac..da2185a37c5d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2263,7 +2263,7 @@ static const char *reflog_message(struct replay_opts *opts,
 static int do_pick_commit(struct repository *r,
 			  struct todo_item *item,
 			  struct replay_opts *opts,
-			  int final_fixup, int *check_todo)
+			  int final_fixup, int *check_todo, int *dropped_commit)
 {
 	struct replay_ctx *ctx = opts->ctx;
 	unsigned int flags = should_edit(opts) ? EDIT_MSG : 0;
@@ -2273,7 +2273,7 @@ static int do_pick_commit(struct repository *r,
 	const char *base_label, *next_label, *reflog_action;
 	char *author = NULL;
 	struct commit_message msg = { NULL, NULL, NULL, NULL };
-	int res, unborn = 0, reword = 0, allow, drop_commit;
+	int res, unborn = 0, reword = 0, allow;
 	enum todo_command command = item->command;
 	struct commit *commit = item->commit;
 
@@ -2492,7 +2492,7 @@ static int do_pick_commit(struct repository *r,
 		goto leave;
 	}
 
-	drop_commit = 0;
+	*dropped_commit = 0;
 	allow = allow_empty(r, opts, commit);
 	if (allow < 0) {
 		res = allow;
@@ -2500,7 +2500,7 @@ static int do_pick_commit(struct repository *r,
 	} else if (allow == 1) {
 		flags |= ALLOW_EMPTY;
 	} else if (allow == 2) {
-		drop_commit = 1;
+		*dropped_commit = 1;
 		refs_delete_ref(get_main_ref_store(r), "", "CHERRY_PICK_HEAD",
 				NULL, REF_NO_DEREF);
 		unlink(git_path_merge_msg(r));
@@ -2510,7 +2510,7 @@ static int do_pick_commit(struct repository *r,
 			_("dropping %s %s -- patch contents already upstream\n"),
 			oid_to_hex(&commit->object.oid), msg.subject);
 	} /* else allow == 0 and there's nothing special to do */
-	if (!opts->no_commit && !drop_commit) {
+	if (!opts->no_commit && !*dropped_commit) {
 		if (author || command == TODO_REVERT || (flags & AMEND_MSG))
 			res = do_commit(r, msg_file, author, reflog_action,
 					opts, flags,
@@ -4943,12 +4943,12 @@ static int pick_one_commit(struct repository *r,
 			   struct replay_opts *opts,
 			   int *check_todo, int* reschedule)
 {
-	int res;
+	int res, dropped_commit;
 	struct todo_item *item = todo_list->items + todo_list->current;
 	const char *arg = todo_item_get_arg(todo_list, item);
 
 	res = do_pick_commit(r, item, opts, is_final_fixup(todo_list),
-			     check_todo);
+			     check_todo, &dropped_commit);
 	if (is_rebase_i(opts) && res < 0) {
 		/* Reschedule */
 		*reschedule = 1;
@@ -4965,7 +4965,7 @@ static int pick_one_commit(struct repository *r,
 		return error_with_patch(r, commit,
 					arg, item->arg_len, opts, res, !res);
 	}
-	if (is_rebase_i(opts) && !res)
+	if (is_rebase_i(opts) && !res && !dropped_commit)
 		record_in_rewritten(&item->commit->object.oid,
 				    peek_command(todo_list, 1));
 	if (res && is_fixup(item->command)) {
@@ -5523,14 +5523,14 @@ static int single_pick(struct repository *r,
 		       struct commit *cmit,
 		       struct replay_opts *opts)
 {
-	int check_todo;
+	int check_todo, dummy;
 	struct todo_item item;
 
 	item.command = opts->action == REPLAY_PICK ?
 			TODO_PICK : TODO_REVERT;
 	item.commit = cmit;
 
-	return do_pick_commit(r, &item, opts, 0, &check_todo);
+	return do_pick_commit(r, &item, opts, 0, &check_todo, &dummy);
 }
 
 int sequencer_pick_revisions(struct repository *r,
diff --git a/t/meson.build b/t/meson.build
index c5832fee0535..6927bd9c794f 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -358,6 +358,7 @@ integration_tests = [
   't3311-notes-merge-fanout.sh',
   't3320-notes-merge-worktrees.sh',
   't3321-notes-stripspace.sh',
+  't3322-notes-rebase.sh',
   't3400-rebase.sh',
   't3401-rebase-and-am-rename.sh',
   't3402-rebase-merge.sh',
diff --git a/t/t3322-notes-rebase.sh b/t/t3322-notes-rebase.sh
new file mode 100755
index 000000000000..0eddde7f9961
--- /dev/null
+++ b/t/t3322-notes-rebase.sh
@@ -0,0 +1,37 @@
+#!/bin/sh
+
+test_description='Test notes on rebase'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	git init &&
+	git config notes.rewriteRef refs/notes/commits &&
+	git version > version &&
+	echo A > A &&
+	git add A &&
+	git commit -m A &&
+	git branch branch &&
+	echo B > B &&
+	git add B &&
+	git commit -m B &&
+	git notes add -m "This is B" @ &&
+	echo C > C &&
+	git add C &&
+	git commit -m C &&
+	git checkout branch &&
+	echo B > B &&
+	echo D > D &&
+	git add B D &&
+	git commit -m BD
+'
+
+test_expect_success 'rebase B + C on top of BD' '
+	git rebase @ master
+'
+
+test_expect_success 'assert there is no note on BD' '
+	if git notes list branch >/tmp/lalaa; then return 1; fi
+'
+
+test_done

base-commit: 3e65291872de10c3f0bf05ea8c24187e7a71ebf0
-- 
2.47.3

Re: [PATCH] sequencer: Skip copying notes for commits that disappear during rebase

[Junio C Hamano]

Uwe Kleine-König <u.kleine-koenig@baylibre.com> writes:

> Note that Phillip also suggested to integrete the test into
> t3400-rebase.sh . IMHO it doesn't matter much if this is considered a
> rebase test or a notes test. I kept it where I have it because I'm lazy
> and failed to understand the git history created in that test.

I do not think his suggestion was about "is this rebase or notes?"
at all.  It was a lot more about "let's not add a new test script
that does only one thing, when there is already a script that covers
the same command and the same option for the command".  In fact,
around 3400.28 there are test pieces that rebases commits that have
notes.

>  sequencer.c             | 20 ++++++++++----------
>  t/meson.build           |  1 +
>  t/t3322-notes-rebase.sh | 37 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 48 insertions(+), 10 deletions(-)
>  create mode 100755 t/t3322-notes-rebase.sh

We need some documentation updates to describe that the users can
lose notes by doing a rebase and under what condition, no?

It is not yet clear to me if we want to _always_ discard a note from
a commit that would become "empty" during a rebase session (in other
words, a commit that becomes empty during a rebase is _always_ a
sign that the change it brings in is _already_ in the new base of
the rebase and the necessary information the note wanted to carry to
the target branch is there without need to _duplicate_ it by copying
the note).  But assuming that we want the behaviour, the code change
to sequencer.c looks very reasonable to me, except for one thing that
I am not clear about.

> diff --git a/sequencer.c b/sequencer.c
> index 57855b0066ac..da2185a37c5d 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> ...
> @@ -4965,7 +4965,7 @@ static int pick_one_commit(struct repository *r,
>  		return error_with_patch(r, commit,
>  					arg, item->arg_len, opts, res, !res);
>  	}
> -	if (is_rebase_i(opts) && !res)
> +	if (is_rebase_i(opts) && !res && !dropped_commit)
>  		record_in_rewritten(&item->commit->object.oid,
>  				    peek_command(todo_list, 1));

If we have a sequence of commits where a commit that was *not*
dropped is followed by a fixup commit that *is* dropped (e.g.,
because it became empty/redundant), wouldn't it prevent the
previously pending commit from being flushed to skip
`record_in_rewritten` entirely for the dropped fixup commit?

For example, if we have

    pick X (with note)
    fixup B (dropped because it is redundant)
    pick C

1. `pick X`: calls `record_in_rewritten(X, TODO_FIXUP)`. `X` is
   written to `pending`, but not flushed because the next insn is
   `TODO_FIXUP` (B).

2. `fixup B`: gets dropped. `dropped_commit` is 1 in the code above,
   so `record_in_rewritten` is skipped.

3. `pick C`: calls `record_in_rewritten(C, -1)`. `C` is written to
   `pending`. Since next insn is not a fixup, it flushes `pending`
   (which contains both `X` and `C`) to the commit created for `C`.

Wouldn't it map the note for `X` to rewritten `C`?

> diff --git a/t/t3322-notes-rebase.sh b/t/t3322-notes-rebase.sh
> new file mode 100755
> index 000000000000..0eddde7f9961
> --- /dev/null
> +++ b/t/t3322-notes-rebase.sh
> @@ -0,0 +1,37 @@
> +#!/bin/sh
> +
> +test_description='Test notes on rebase'
> +
> +. ./test-lib.sh
> +
> +test_expect_success setup '
> +	git init &&
> +	git config notes.rewriteRef refs/notes/commits &&
> +	git version > version &&
> +	echo A > A &&

Style.  In our codebase, redirection operator sticks to the
redirection target without SP in between, i.e.

	git version >version &&
	echo A >A &&

> +	git notes add -m "This is B" @ &&

'@' is hard to read; when you refer to HEAD, please write HEAD.


> +test_expect_success 'rebase B + C on top of BD' '
> +	git rebase @ master
> +'
> +
> +test_expect_success 'assert there is no note on BD' '
> +	if git notes list branch >/tmp/lalaa; then return 1; fi
> +'

Do not step outside of $TRASH_DIRECTORY without a good reason.

Style.  In our codebase, shell scripts do not use ';' and written
more like

	if git notes list branch >notes-list
	then
		return 1
	fi

But more importantly, if you want to make sure the command makes a
controlled exit (not crash), use

	test_must_fail git notes list branch

That will pass the test happily if "git notes list branch" makes a
controlled die() call (e.g., when there is no notes attached to that
commit, the command exits with 1), but still makes the test fail if
"git notes list branch" segfaults.

Again, we do not want to add a new test script that does only one
thing, when there is already a script that covers the same command
and the same option for the command.

Thanks.

Re: [PATCH] sequencer: Skip copying notes for commits that disappear during rebase

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 5911 bytes --]

Hello Junio,

On Wed, Jun 17, 2026 at 06:24:03AM -0700, Junio C Hamano wrote:
> Uwe Kleine-König <u.kleine-koenig@baylibre.com> writes:
> 
> > Note that Phillip also suggested to integrete the test into
> > t3400-rebase.sh . IMHO it doesn't matter much if this is considered a
> > rebase test or a notes test. I kept it where I have it because I'm lazy
> > and failed to understand the git history created in that test.
> 
> I do not think his suggestion was about "is this rebase or notes?"
> at all.  It was a lot more about "let's not add a new test script
> that does only one thing, when there is already a script that covers
> the same command and the same option for the command".  In fact,
> around 3400.28 there are test pieces that rebases commits that have
> notes.

OK, sounds fair.

> >  sequencer.c             | 20 ++++++++++----------
> >  t/meson.build           |  1 +
> >  t/t3322-notes-rebase.sh | 37 +++++++++++++++++++++++++++++++++++++
> >  3 files changed, 48 insertions(+), 10 deletions(-)
> >  create mode 100755 t/t3322-notes-rebase.sh
> 
> We need some documentation updates to describe that the users can
> lose notes by doing a rebase and under what condition, no?

Well, the current state is that we're not losing notes, but that we
attach it to commits that most of the time are completely unrelated to
the commit the note was initially attached to. (i.e. in general it's not
attached to the commit that made the currently picked commit empty.) So
essentially the notes are lost, too, but also add confusion to where
they happen to get attached to.

> It is not yet clear to me if we want to _always_ discard a note from
> a commit that would become "empty" during a rebase session (in other
> words, a commit that becomes empty during a rebase is _always_ a
> sign that the change it brings in is _already_ in the new base of
> the rebase

Yeah, or in a patch that was picked before.

> and the necessary information the note wanted to carry to
> the target branch is there without need to _duplicate_ it by copying
> the note).  But assuming that we want the behaviour, the code change
> to sequencer.c looks very reasonable to me, except for one thing that
> I am not clear about.

I think given the commit goes away, it's natural that the note goes
away, too. And to come back to your question above: I think it doesn't
need documentation, that if a commit disappears its notes go away, too.
But that might be subjective?!

> > diff --git a/sequencer.c b/sequencer.c
> > index 57855b0066ac..da2185a37c5d 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > ...
> > @@ -4965,7 +4965,7 @@ static int pick_one_commit(struct repository *r,
> >  		return error_with_patch(r, commit,
> >  					arg, item->arg_len, opts, res, !res);
> >  	}
> > -	if (is_rebase_i(opts) && !res)
> > +	if (is_rebase_i(opts) && !res && !dropped_commit)
> >  		record_in_rewritten(&item->commit->object.oid,
> >  				    peek_command(todo_list, 1));
> 
> If we have a sequence of commits where a commit that was *not*
> dropped is followed by a fixup commit that *is* dropped (e.g.,
> because it became empty/redundant), wouldn't it prevent the
> previously pending commit from being flushed to skip
> `record_in_rewritten` entirely for the dropped fixup commit?
> 
> For example, if we have
> 
>     pick X (with note)
>     fixup B (dropped because it is redundant)
>     pick C
> 
> 1. `pick X`: calls `record_in_rewritten(X, TODO_FIXUP)`. `X` is
>    written to `pending`, but not flushed because the next insn is
>    `TODO_FIXUP` (B).
> 
> 2. `fixup B`: gets dropped. `dropped_commit` is 1 in the code above,
>    so `record_in_rewritten` is skipped.
> 
> 3. `pick C`: calls `record_in_rewritten(C, -1)`. `C` is written to
>    `pending`. Since next insn is not a fixup, it flushes `pending`
>    (which contains both `X` and `C`) to the commit created for `C`.

Huh, sounds possible. I wonder if that makes the change so complicated
that my time isn't well spend working on that given that I'm not used to
git's source code and it's better addressed by someone with deeper
knowledge. Sounds as if we need a state signaling "Current commit is
done".

> Wouldn't it map the note for `X` to rewritten `C`?
> 
> > diff --git a/t/t3322-notes-rebase.sh b/t/t3322-notes-rebase.sh
> > new file mode 100755
> > index 000000000000..0eddde7f9961
> > --- /dev/null
> > +++ b/t/t3322-notes-rebase.sh
> > @@ -0,0 +1,37 @@
> > +#!/bin/sh
> > +
> > +test_description='Test notes on rebase'
> > +
> > +. ./test-lib.sh
> > +
> > +test_expect_success setup '
> > +	git init &&
> > +	git config notes.rewriteRef refs/notes/commits &&
> > +	git version > version &&
> > +	echo A > A &&
> 
> Style.  In our codebase, redirection operator sticks to the
> redirection target without SP in between, i.e.
> 
> 	git version >version &&
> 	echo A >A &&
> 
> > +	git notes add -m "This is B" @ &&
> 
> '@' is hard to read; when you refer to HEAD, please write HEAD.
> 
> 
> > +test_expect_success 'rebase B + C on top of BD' '
> > +	git rebase @ master
> > +'
> > +
> > +test_expect_success 'assert there is no note on BD' '
> > +	if git notes list branch >/tmp/lalaa; then return 1; fi
> > +'
> 
> Do not step outside of $TRASH_DIRECTORY without a good reason.

Oh, that is a debug thing that shouldn't have made it into the patch.
 
> Style.  In our codebase, shell scripts do not use ';' and written
> more like
> 
> 	if git notes list branch >notes-list
> 	then
> 		return 1
> 	fi
> 
> But more importantly, if you want to make sure the command makes a
> controlled exit (not crash), use
> 
> 	test_must_fail git notes list branch

Ah, I really wondered if I'm missing something because it should be
easier to say "this command should fail".

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]