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

> 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