Hello Junio,

On Wed, Jun 17, 2026 at 06:24:03AM -0700, Junio C Hamano wrote:

Uwe Kleine-König [off-list ref] writes:

quoted

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.

quoted

 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?!

quoted

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`?

quoted

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

quoted

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

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

quoted

+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