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

[Junio C Hamano <gitster@pobox.com>]

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.