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

["Uwe Kleine-König" <u.kleine-koenig@baylibre.com>]

[-- 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 --]