Hi Uwe and Junio

On 17/06/2026 14:58, Uwe Kleine-König wrote:

quoted

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.

quoted

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

I tend to agree with this - if we're throwing away the commit message 
without asking the user I think it makes sense to do the same for the 
notes. We have "--empty=ask" if the user does not want commits that 
become empty to be automatically discarded.

quoted

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?

That's a good point - we should call flush_rewritten_pending() in that 
case. Looking at the code there are some other bugs related to dropping 
commits either because they become empty or the user runs "git rebase 
--skip"

  - If we drop the final fixup we don't cleanup the commit message

  - If we drop an "edit" command then "git rebase --continue" records it
    as being rewritten HEAD so we'll copy the notes to the wrong commit

  - Running "git rebase --skip" causes the commit that had conflicts
    to also be recorded as as being rewritten to HEAD leading to the
    same issue.

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

I'm happy to take this forward and try and fix at least some of the 
other bugs I've listed above. Uwe - if I don't cc you on some patches 
within the next couple of weeks please feel free to send a reminder.

Thanks

Phillip

quoted

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.
  

quoted

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