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

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

[Uwe Kleine-König]

When a commit disappears during rebase because the patch content is
already there (but not by the same patch in which case the commit would
be skipped) the notes of that disappearing commit should not be copied
to the unrelated commit that happens to be HEAD.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
Hello,

after also my 2nd bug report[1] didn't motivate anyone to come up with a
fix, I invested the time to work out one according to Phillip Wood's
suggestion.

IMHO it's not pretty, but it works for me.

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.

Best regards
Uwe

[1] https://lore.kernel.org/git/20260612143952.3281115-2-u.kleine-koenig@baylibre.com


 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

diff --git a/sequencer.c b/sequencer.c
index 57855b0066ac..da2185a37c5d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2263,7 +2263,7 @@ static const char *reflog_message(struct replay_opts *opts,
 static int do_pick_commit(struct repository *r,
 			  struct todo_item *item,
 			  struct replay_opts *opts,
-			  int final_fixup, int *check_todo)
+			  int final_fixup, int *check_todo, int *dropped_commit)
 {
 	struct replay_ctx *ctx = opts->ctx;
 	unsigned int flags = should_edit(opts) ? EDIT_MSG : 0;
@@ -2273,7 +2273,7 @@ static int do_pick_commit(struct repository *r,
 	const char *base_label, *next_label, *reflog_action;
 	char *author = NULL;
 	struct commit_message msg = { NULL, NULL, NULL, NULL };
-	int res, unborn = 0, reword = 0, allow, drop_commit;
+	int res, unborn = 0, reword = 0, allow;
 	enum todo_command command = item->command;
 	struct commit *commit = item->commit;
 
@@ -2492,7 +2492,7 @@ static int do_pick_commit(struct repository *r,
 		goto leave;
 	}
 
-	drop_commit = 0;
+	*dropped_commit = 0;
 	allow = allow_empty(r, opts, commit);
 	if (allow < 0) {
 		res = allow;
@@ -2500,7 +2500,7 @@ static int do_pick_commit(struct repository *r,
 	} else if (allow == 1) {
 		flags |= ALLOW_EMPTY;
 	} else if (allow == 2) {
-		drop_commit = 1;
+		*dropped_commit = 1;
 		refs_delete_ref(get_main_ref_store(r), "", "CHERRY_PICK_HEAD",
 				NULL, REF_NO_DEREF);
 		unlink(git_path_merge_msg(r));
@@ -2510,7 +2510,7 @@ static int do_pick_commit(struct repository *r,
 			_("dropping %s %s -- patch contents already upstream\n"),
 			oid_to_hex(&commit->object.oid), msg.subject);
 	} /* else allow == 0 and there's nothing special to do */
-	if (!opts->no_commit && !drop_commit) {
+	if (!opts->no_commit && !*dropped_commit) {
 		if (author || command == TODO_REVERT || (flags & AMEND_MSG))
 			res = do_commit(r, msg_file, author, reflog_action,
 					opts, flags,
@@ -4943,12 +4943,12 @@ static int pick_one_commit(struct repository *r,
 			   struct replay_opts *opts,
 			   int *check_todo, int* reschedule)
 {
-	int res;
+	int res, dropped_commit;
 	struct todo_item *item = todo_list->items + todo_list->current;
 	const char *arg = todo_item_get_arg(todo_list, item);
 
 	res = do_pick_commit(r, item, opts, is_final_fixup(todo_list),
-			     check_todo);
+			     check_todo, &dropped_commit);
 	if (is_rebase_i(opts) && res < 0) {
 		/* Reschedule */
 		*reschedule = 1;
@@ -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 (res && is_fixup(item->command)) {
@@ -5523,14 +5523,14 @@ static int single_pick(struct repository *r,
 		       struct commit *cmit,
 		       struct replay_opts *opts)
 {
-	int check_todo;
+	int check_todo, dummy;
 	struct todo_item item;
 
 	item.command = opts->action == REPLAY_PICK ?
 			TODO_PICK : TODO_REVERT;
 	item.commit = cmit;
 
-	return do_pick_commit(r, &item, opts, 0, &check_todo);
+	return do_pick_commit(r, &item, opts, 0, &check_todo, &dummy);
 }
 
 int sequencer_pick_revisions(struct repository *r,
diff --git a/t/meson.build b/t/meson.build
index c5832fee0535..6927bd9c794f 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -358,6 +358,7 @@ integration_tests = [
   't3311-notes-merge-fanout.sh',
   't3320-notes-merge-worktrees.sh',
   't3321-notes-stripspace.sh',
+  't3322-notes-rebase.sh',
   't3400-rebase.sh',
   't3401-rebase-and-am-rename.sh',
   't3402-rebase-merge.sh',
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 &&
+	git add A &&
+	git commit -m A &&
+	git branch branch &&
+	echo B > B &&
+	git add B &&
+	git commit -m B &&
+	git notes add -m "This is B" @ &&
+	echo C > C &&
+	git add C &&
+	git commit -m C &&
+	git checkout branch &&
+	echo B > B &&
+	echo D > D &&
+	git add B D &&
+	git commit -m BD
+'
+
+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
+'
+
+test_done

base-commit: 3e65291872de10c3f0bf05ea8c24187e7a71ebf0
-- 
2.47.3

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

[Junio C Hamano]

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.

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

[Uwe Kleine-König]

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

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

[Phillip Wood]

Hi Uwe and Junio

On 17/06/2026 14:58, Uwe Kleine-König wrote:
> 
>> 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?!

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.

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


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

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 366 bytes --]

Hello Phillip,

On Fri, Jun 19, 2026 at 11:13:32AM +0100, Phillip Wood wrote:
> 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.

Very appreciated! Looking forward to test your patches.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 00/11] sequencer: do not record dropped commits as rewritten

[Phillip Wood]

On 19/06/2026 11:13, Phillip Wood wrote:
> 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.

Here is the first batch that fixes the same problem as Uwe's patch. I've
taken a slightly different approach that uses the return value from
do_pick_commit() to signal that a commit was dropped rather than
adding another function argument. That involves a number of preparatory
patches, but they are hopefully reasonably small and easy to follow.

If a commit gets dropped because its changes are already upstream
then we should not record it as rewritten. As well as confusing any
post-rewrite hooks this means we end up copying the notes from the
dropped commit to the commit that was picked immediately before the
one that was dropped.

This series is structured as follows:

Patch 1 restores some test coverage that was lost when the default
rebase backend was changed.

Patch 2 moves a function so it can be called without a forward
declaration in Patch 11.

Patches 3 & 4 fix the return value of do_pick_commit() when an external
command fails (this is in preparation for patch 10).

Patches 5-9 try and simplify the control flow in pick_one_commit()
in preparation for patch 10.

Patch 10 changes the return type of do_pick_commit() to an enum.

Patch 11 adds a new member to the enum from patch 10 for commits that
are dropped when they become empty and uses that to stop them from
being recorded as rewritten.

Base-Commit: 6c3d7b73556db708feb3b16232fab1efc4353428
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Frebase-drop-notes-with-commit%2Fv1
View-Changes-At: https://github.com/phillipwood/git/compare/6c3d7b735...26551f268
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/rebase-drop-notes-with-commit/v1


Phillip Wood (11):
  t3400: restore coverage for note copying with apply backend
  sequencer: move definition of is_final_fixup()
  sequencer: be more careful with external merge
  sequencer: never reschedule on failed commit
  sequencer: remove unnecessary "or" in pick_one_commit()
  sequencer: simplify handing of fixup with conflicts
  sequencer: remove unnecessary condition in pick_one_commit()
  sequencer: simplify pick_one_commit()
  sequencer: return early from pick_one_commit() on success
  sequencer: use an enum to represent result of picking a commit
  sequencer: do not record dropped commits as rewritten

 sequencer.c                   | 154 +++++++++++++++++++++++-----------
 t/t3400-rebase.sh             |  16 +++-
 t/t3404-rebase-interactive.sh |  11 +++
 t/t5407-post-rewrite-hook.sh  |  23 +++++
 4 files changed, 155 insertions(+), 49 deletions(-)

-- 
2.54.0.200.gfd8d68259e3

[PATCH 01/11] t3400: restore coverage for note copying with apply backend

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Now that the merge backend is the default we have lost coverage for
"git rebase --apply" copying notes. Fix this by replacing "-m" with
"--apply" as the previous test which uses the default backend now
checks the merge backend.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3400-rebase.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index c0c00fbb7b1..f0e7fcf649a 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -270,9 +270,9 @@ test_expect_success 'rebase can copy notes' '
 	test "a note" = "$(git notes show HEAD)"
 '
 
-test_expect_success 'rebase -m can copy notes' '
+test_expect_success 'rebase --apply can copy notes' '
 	git reset --hard n3 &&
-	git rebase -m --onto n1 n2 &&
+	git rebase --apply --onto n1 n2 &&
 	test "a note" = "$(git notes show HEAD)"
 '
 
-- 
2.54.0.200.gfd8d68259e3

[PATCH 02/11] sequencer: move definition of is_final_fixup()

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Move this function earlier in the file in preparation for adding a
new caller in a later commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 57855b0066a..32a09b6e87d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4627,21 +4627,6 @@ static int do_update_refs(struct repository *r, int quiet)
 	strbuf_release(&update_msg);
 	strbuf_release(&error_msg);
 	return res;
-}
-
-static int is_final_fixup(struct todo_list *todo_list)
-{
-	int i = todo_list->current;
-
-	if (!is_fixup(todo_list->items[i].command))
-		return 0;
-
-	while (++i < todo_list->nr)
-		if (is_fixup(todo_list->items[i].command))
-			return 0;
-		else if (!is_noop(todo_list->items[i].command))
-			break;
-	return 1;
 }
 
 static enum todo_command peek_command(struct todo_list *todo_list, int offset)
@@ -4925,6 +4910,21 @@ static int reread_todo_if_changed(struct repository *r,
 	strbuf_release(&buf);
 
 	return 0;
+}
+
+static int is_final_fixup(struct todo_list *todo_list)
+{
+	int i = todo_list->current;
+
+	if (!is_fixup(todo_list->items[i].command))
+		return 0;
+
+	while (++i < todo_list->nr)
+		if (is_fixup(todo_list->items[i].command))
+			return 0;
+		else if (!is_noop(todo_list->items[i].command))
+			break;
+	return 1;
 }
 
 static const char rescheduled_advice[] =
-- 
2.54.0.200.gfd8d68259e3

[PATCH 03/11] sequencer: be more careful with external merge

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If an external merge strategy cannot merge (for example because it
would overwrite an untracked file) it exits with a non-zero exit
code other than 1. This should be treated differently to a merge
with conflicts which is signalled by an exit code of 1 because as
the merge failed we need to reschedule the last pick. The caller
expects us to return -1 in this case. Also reschedule without trying
to merge if the commit message cannot be written as that prevents us
from successfully picking the commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c                   | 19 +++++++++++++++----
 t/t3404-rebase-interactive.sh | 11 +++++++++++
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 32a09b6e87d..e6626c4db4e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2453,14 +2453,25 @@ static int do_pick_commit(struct repository *r,
 		struct commit_list *common = NULL;
 		struct commit_list *remotes = NULL;
 
-		res = write_message(ctx->message.buf, ctx->message.len,
-				    git_path_merge_msg(r), 0);
+		if (write_message(ctx->message.buf, ctx->message.len,
+				  git_path_merge_msg(r), 0)) {
+			res = -1;
+			goto leave;
+		}
 
 		commit_list_insert(base, &common);
 		commit_list_insert(next, &remotes);
-		res |= try_merge_command(r, opts->strategy,
-					 opts->xopts.nr, opts->xopts.v,
+		res = try_merge_command(r, opts->strategy,
+					opts->xopts.nr, opts->xopts.v,
 					common, oid_to_hex(&head), remotes);
+		/*
+		 * If the there were conflicts, try_merge_command() returns 1,
+		 * any other no-zero return code means that either the merge
+		 * command could not be run, or it failed to merge.
+		 */
+		if (res && res != 1)
+			res = -1;
+
 		commit_list_free(common);
 		commit_list_free(remotes);
 	}
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 58b3bb0c271..297b84e60d5 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1249,6 +1249,17 @@ test_expect_success 'interrupted rebase -i with --strategy and -X' '
 	git rebase --continue &&
 	test $(git show conflict-branch:conflict) = $(cat conflict) &&
 	test $(cat file1) = Z
+'
+
+test_expect_success 'failing pick with --strategy is rescheduled' '
+	test_when_finished "rm -rf bin; test_might_fail git rebase --abort" &&
+	mkdir bin &&
+	echo exit 2 | write_script bin/git-merge-fail &&
+	git log -1 --format="pick %H # %s" HEAD >expect &&
+	test_must_fail env PATH="$PWD/bin:$PATH" \
+		git rebase --no-ff --strategy fail HEAD^ &&
+	test_cmp expect .git/rebase-merge/git-rebase-todo &&
+	test_cmp expect .git/rebase-merge/done
 '
 
 test_expect_success 'rebase -i error on commits with \ in message' '
-- 
2.54.0.200.gfd8d68259e3

[PATCH 04/11] sequencer: never reschedule on failed commit

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If "git commit" fails to run then run_git_commit() returns -1 which
causes the current command to be rescheduled. This is incorrect as
we have successfully picked the commit and have written all the state
files we need to successfully commit when the user continues. Fix this
by converting -1 to 1 which matches what do_merge() does.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index e6626c4db4e..d7e439b1feb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2542,6 +2542,12 @@ static int do_pick_commit(struct repository *r,
 			res = run_git_commit(NULL, reflog_action, opts, flags);
 			*check_todo = 1;
 		}
+		/*
+		 * If "git commit" failed to run than res == -1 but we dont
+		 * want reschedule the last command because the picking the
+		 * commit was successful.
+		 */
+		res = !!res;
 	}
 
 
-- 
2.54.0.200.gfd8d68259e3

[PATCH 05/11] sequencer: remove unnecessary "or" in pick_one_commit()

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If error_with_patch(..., res, ...) succeeds then it returns "res", if
it fails then it returns -1. This means that or-ing the return value
with "res" is pointless the result is the same as the return value.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index d7e439b1feb..39cbb7b6e3e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5007,9 +5007,8 @@ static int pick_one_commit(struct repository *r,
 		      oideq(&opts->squash_onto, &oid))))
 			to_amend = 1;
 
-		return res | error_with_patch(r, item->commit,
-					      arg, item->arg_len, opts,
-					      res, to_amend);
+		return error_with_patch(r, item->commit, arg, item->arg_len,
+					opts, res, to_amend);
 	}
 	return res;
 }
-- 
2.54.0.200.gfd8d68259e3

[PATCH 06/11] sequencer: simplify handing of fixup with conflicts

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Commit e032abd5a0 (rebase: fix rewritten list for failed pick,
2023-09-06) introduced an early return when res == -1, so if we enter
this conditional block then res is positive. After the last couple
of commits the only possible positive value is 1 so we can simplify
the code by removing the conditional call to intend_to_amend() and
call it error_with_patch() instead.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 39cbb7b6e3e..bcfbda018a7 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3874,7 +3874,7 @@ static int error_failed_squash(struct repository *r,
 		return error(_("could not copy '%s' to '%s'"),
 			     rebase_path_message(),
 			     git_path_merge_msg(r));
-	return error_with_patch(r, commit, subject, subject_len, opts, 1, 0);
+	return error_with_patch(r, commit, subject, subject_len, opts, 1, 1);
 }
 
 static int do_exec(struct repository *r, const char *command_line, int quiet)
@@ -4986,8 +4986,6 @@ static int pick_one_commit(struct repository *r,
 		record_in_rewritten(&item->commit->object.oid,
 				    peek_command(todo_list, 1));
 	if (res && is_fixup(item->command)) {
-		if (res == 1)
-			intend_to_amend();
 		return error_failed_squash(r, item->commit, opts,
 					   item->arg_len, arg);
 	} else if (res && is_rebase_i(opts) && item->commit) {
-- 
2.54.0.200.gfd8d68259e3

[PATCH 07/11] sequencer: remove unnecessary condition in pick_one_commit()

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

item->commit holds the commit to be picked and so it must be non-NULL
otherwise pick_one_commit() would not know which commit to pick.
It is also unconditionally dereferenced in do_pick_commit() which is
called at the top of this function. Therefore the check to see if it
is non-NULL is superfluous.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index bcfbda018a7..ff28873d21c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4988,7 +4988,7 @@ static int pick_one_commit(struct repository *r,
 	if (res && is_fixup(item->command)) {
 		return error_failed_squash(r, item->commit, opts,
 					   item->arg_len, arg);
-	} else if (res && is_rebase_i(opts) && item->commit) {
+	} else if (res && is_rebase_i(opts)) {
 		int to_amend = 0;
 		struct object_id oid;
 
-- 
2.54.0.200.gfd8d68259e3

[PATCH 08/11] sequencer: simplify pick_one_commit()

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Unless we're rebasing all we do in pick_one_commit() is call
do_pick_commit() and return its result. Simplify the code by returing
early if we're not rebasing so that we don't have to continually call
is_rebase_i() in the rest of the function. Note that there are a couple
of conditions that do not call is_rebase_i() but they check for either
an "edit" or a "fixup" command, both of which imply we're rebasing.

As the conditional blocks are all mutually exclusive (either the
conditions are mutually exclusive, or an earlier conditional block
that would match a later one contains a "return" statement) chain
them together with "else if" to make that clear.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index ff28873d21c..416729f30a7 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4966,12 +4966,14 @@ static int pick_one_commit(struct repository *r,
 
 	res = do_pick_commit(r, item, opts, is_final_fixup(todo_list),
 			     check_todo);
-	if (is_rebase_i(opts) && res < 0) {
+	if (!is_rebase_i(opts))
+		return res;
+
+	if (res < 0) {
 		/* Reschedule */
 		*reschedule = 1;
 		return -1;
-	}
-	if (item->command == TODO_EDIT) {
+	} else if (item->command == TODO_EDIT) {
 		struct commit *commit = item->commit;
 		if (!res) {
 			if (!opts->verbose)
@@ -4981,14 +4983,13 @@ 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)
+	} else if (!res) {
 		record_in_rewritten(&item->commit->object.oid,
 				    peek_command(todo_list, 1));
-	if (res && is_fixup(item->command)) {
+	} else if (res && is_fixup(item->command)) {
 		return error_failed_squash(r, item->commit, opts,
 					   item->arg_len, arg);
-	} else if (res && is_rebase_i(opts)) {
+	} else if (res) {
 		int to_amend = 0;
 		struct object_id oid;
 
-- 
2.54.0.200.gfd8d68259e3

[PATCH 09/11] sequencer: return early from pick_one_commit() on success

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The only block that does not return early is the one guarded by
"!res". Move the return into that block to make it clear that after
recording the commit as rewritten all we do is return from the function.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 416729f30a7..655a2e84bef 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4986,6 +4986,7 @@ static int pick_one_commit(struct repository *r,
 	} else if (!res) {
 		record_in_rewritten(&item->commit->object.oid,
 				    peek_command(todo_list, 1));
+		return 0;
 	} else if (res && is_fixup(item->command)) {
 		return error_failed_squash(r, item->commit, opts,
 					   item->arg_len, arg);
@@ -5009,7 +5010,8 @@ static int pick_one_commit(struct repository *r,
 		return error_with_patch(r, item->commit, arg, item->arg_len,
 					opts, res, to_amend);
 	}
-	return res;
+
+	BUG("Unhandled return value from do_pick_commit()");
 }
 
 static int pick_commits(struct repository *r,
-- 
2.54.0.200.gfd8d68259e3

[PATCH 10/11] sequencer: use an enum to represent result of picking a commit

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Rather than using an integer where -1 is an error, 0 is success and
1 means there were conflicts use an enum. This is clearer and lets
us add a separate return value for commits that are dropped because
they become empty in the next commit.

Note we continue to use "return error(...)" to return errors and
take advantage of C's lax typing of enums

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 61 +++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 45 insertions(+), 16 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 655a2e84bef..ca005b969c4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2260,10 +2260,16 @@ static const char *reflog_message(struct replay_opts *opts,
 	return buf.buf;
 }
 
-static int do_pick_commit(struct repository *r,
-			  struct todo_item *item,
-			  struct replay_opts *opts,
-			  int final_fixup, int *check_todo)
+enum pick_result {
+	PICK_RESULT_ERROR = -1,
+	PICK_RESULT_OK,
+	PICK_RESULT_CONFLICTS,
+};
+
+static enum pick_result do_pick_commit(struct repository *r,
+				       struct todo_item *item,
+				       struct replay_opts *opts,
+				       int final_fixup, int *check_todo)
 {
 	struct replay_ctx *ctx = opts->ctx;
 	unsigned int flags = should_edit(opts) ? EDIT_MSG : 0;
@@ -2564,7 +2570,12 @@ static int do_pick_commit(struct repository *r,
 	free(author);
 	update_abort_safety_file();
 
-	return res;
+	if (res < 0)
+		return PICK_RESULT_ERROR;
+	else if (res > 0)
+		return PICK_RESULT_CONFLICTS;
+	else
+		return PICK_RESULT_OK;
 }
 
 static int prepare_revs(struct replay_opts *opts)
@@ -4960,37 +4971,47 @@ static int pick_one_commit(struct repository *r,
 			   struct replay_opts *opts,
 			   int *check_todo, int* reschedule)
 {
-	int res;
+	enum pick_result pick_res;
 	struct todo_item *item = todo_list->items + todo_list->current;
 	const char *arg = todo_item_get_arg(todo_list, item);
 
-	res = do_pick_commit(r, item, opts, is_final_fixup(todo_list),
-			     check_todo);
+	pick_res = do_pick_commit(r, item, opts, is_final_fixup(todo_list),
+				  check_todo);
 	if (!is_rebase_i(opts))
-		return res;
+		switch (pick_res) {
+		case PICK_RESULT_ERROR:
+			return -1;
+		case PICK_RESULT_CONFLICTS:
+			return 1;
+		default:
+			return 0;
+		}
 
-	if (res < 0) {
+	if (pick_res == PICK_RESULT_ERROR) {
 		/* Reschedule */
 		*reschedule = 1;
 		return -1;
 	} else if (item->command == TODO_EDIT) {
 		struct commit *commit = item->commit;
-		if (!res) {
+		int res = pick_res == PICK_RESULT_CONFLICTS;
+
+		if (pick_res == PICK_RESULT_OK) {
 			if (!opts->verbose)
 				term_clear_line();
 			fprintf(stderr, _("Stopped at %s...  %.*s\n"),
 				short_commit_name(r, commit), item->arg_len, arg);
 		}
 		return error_with_patch(r, commit,
 					arg, item->arg_len, opts, res, !res);
-	} else if (!res) {
+	} else if (pick_res == PICK_RESULT_OK) {
 		record_in_rewritten(&item->commit->object.oid,
 				    peek_command(todo_list, 1));
 		return 0;
-	} else if (res && is_fixup(item->command)) {
+	} else if (pick_res == PICK_RESULT_CONFLICTS &&
+		   is_fixup(item->command)) {
 		return error_failed_squash(r, item->commit, opts,
 					   item->arg_len, arg);
-	} else if (res) {
+	} else if (pick_res == PICK_RESULT_CONFLICTS) {
 		int to_amend = 0;
 		struct object_id oid;
 
@@ -5008,7 +5029,7 @@ static int pick_one_commit(struct repository *r,
 			to_amend = 1;
 
 		return error_with_patch(r, item->commit, arg, item->arg_len,
-					opts, res, to_amend);
+					opts, 1, to_amend);
 	}
 
 	BUG("Unhandled return value from do_pick_commit()");
@@ -5547,7 +5568,15 @@ static int single_pick(struct repository *r,
 			TODO_PICK : TODO_REVERT;
 	item.commit = cmit;
 
-	return do_pick_commit(r, &item, opts, 0, &check_todo);
+	switch (do_pick_commit(r, &item, opts, 0, &check_todo)) {
+	case PICK_RESULT_ERROR:
+		return -1;
+	case PICK_RESULT_CONFLICTS:
+		return 1;
+	default:
+		return 0;
+	}
+
 }
 
 int sequencer_pick_revisions(struct repository *r,
-- 
2.54.0.200.gfd8d68259e3

[PATCH 11/11] sequencer: do not record dropped commits as rewritten

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If a commit gets dropped because its changes are already upstream
then we should not record it as rewritten. As well as confusing any
post-rewrite hooks this means we end up copying the notes from the
dropped commit to the commit that was picked immediately before the
one that was dropped.

While we do not want to record the dropped commit is rewritten, if
it is the final commit in a chain of fixups then we need to flush
the list of rewritten commits. The behavior of an "edit" command
where the commit is dropped is changed so that "rebase --continue"
will not amend the previous pick. However, as the code comment notes
it will still be erroneously recorded as rewritten when the rebase
continues. That will need to be addressed separately along with not
recording skipped commits as rewritten.

The initialization of "drop_commit" is moved to ensure it is initialized
when rewording a fast-forwarded commit.

Reported-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c                  | 24 +++++++++++++++++++-----
 t/t3400-rebase.sh            | 12 ++++++++++++
 t/t5407-post-rewrite-hook.sh | 23 +++++++++++++++++++++++
 3 files changed, 54 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index ca005b969c4..a85f9e8b77d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2264,6 +2264,7 @@ enum pick_result {
 	PICK_RESULT_ERROR = -1,
 	PICK_RESULT_OK,
 	PICK_RESULT_CONFLICTS,
+	PICK_RESULT_DROPPED,
 };
 
 static enum pick_result do_pick_commit(struct repository *r,
@@ -2279,7 +2280,7 @@ static enum pick_result do_pick_commit(struct repository *r,
 	const char *base_label, *next_label, *reflog_action;
 	char *author = NULL;
 	struct commit_message msg = { NULL, NULL, NULL, NULL };
-	int res, unborn = 0, reword = 0, allow, drop_commit;
+	int res, unborn = 0, reword = 0, allow, drop_commit = 0;
 	enum todo_command command = item->command;
 	struct commit *commit = item->commit;
 
@@ -2509,7 +2510,6 @@ static enum pick_result do_pick_commit(struct repository *r,
 		goto leave;
 	}
 
-	drop_commit = 0;
 	allow = allow_empty(r, opts, commit);
 	if (allow < 0) {
 		res = allow;
@@ -2574,6 +2574,8 @@ static enum pick_result do_pick_commit(struct repository *r,
 		return PICK_RESULT_ERROR;
 	else if (res > 0)
 		return PICK_RESULT_CONFLICTS;
+	else if (drop_commit)
+		return PICK_RESULT_DROPPED;
 	else
 		return PICK_RESULT_OK;
 }
@@ -4994,18 +4996,30 @@ static int pick_one_commit(struct repository *r,
 	} else if (item->command == TODO_EDIT) {
 		struct commit *commit = item->commit;
 		int res = pick_res == PICK_RESULT_CONFLICTS;
+		int to_amend = pick_res != PICK_RESULT_CONFLICTS &&
+				pick_res != PICK_RESULT_DROPPED;
 
-		if (pick_res == PICK_RESULT_OK) {
+		/*
+		 * NEEDSWORK: Do not record the commit as rewritten when
+		 * continuing if it was dropped. Does it even make sense
+		 * to stop if the commit was dropped?
+		 */
+		if (pick_res == PICK_RESULT_OK ||
+		    pick_res == PICK_RESULT_DROPPED) {
 			if (!opts->verbose)
 				term_clear_line();
 			fprintf(stderr, _("Stopped at %s...  %.*s\n"),
 				short_commit_name(r, commit), item->arg_len, arg);
 		}
-		return error_with_patch(r, commit,
-					arg, item->arg_len, opts, res, !res);
+		return error_with_patch(r, commit, arg, item->arg_len, opts,
+					res, to_amend);
 	} else if (pick_res == PICK_RESULT_OK) {
 		record_in_rewritten(&item->commit->object.oid,
 				    peek_command(todo_list, 1));
+		return 0;
+	} else if (pick_res == PICK_RESULT_DROPPED) {
+		if (is_final_fixup(todo_list))
+			flush_rewritten_pending();
 		return 0;
 	} else if (pick_res == PICK_RESULT_CONFLICTS &&
 		   is_fixup(item->command)) {
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index f0e7fcf649a..1d09886ea35 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -274,6 +274,18 @@ test_expect_success 'rebase --apply can copy notes' '
 	git reset --hard n3 &&
 	git rebase --apply --onto n1 n2 &&
 	test "a note" = "$(git notes show HEAD)"
+'
+
+test_expect_success 'rebase drops notes of dropped commits' '
+	git checkout n1 &&
+	echo n3 >n3.t &&
+	echo n4 >n4.t &&
+	git add n3.t n4.t &&
+	git commit -m n34 &&
+	git rebase HEAD n3 &&
+	test_commit_message HEAD -m n2 &&
+	test_must_fail git notes list HEAD >actual &&
+	test_must_be_empty actual
 '
 
 test_expect_success 'rebase commit with an ancient timestamp' '
diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
index ad7f8c6f002..51991956d1d 100755
--- a/t/t5407-post-rewrite-hook.sh
+++ b/t/t5407-post-rewrite-hook.sh
@@ -306,6 +306,29 @@ test_expect_success 'git rebase -i (exec)' '
 	cat >expected.data <<-EOF &&
 	$(git rev-parse C) $(git rev-parse HEAD^)
 	$(git rev-parse D) $(git rev-parse HEAD)
+	EOF
+	verify_hook_input
+'
+
+test_expect_success 'rebase with commits that become empty' '
+	cat >todo <<-\EOF &&
+	pick H
+	pick E
+	fixup I
+	fixup H
+	pick G
+	pick I
+	EOF
+	(
+		set_replace_editor todo &&
+		git rebase -i --empty=drop A A
+	) &&
+	echo rebase >expected.args &&
+	cat >expected.data <<-EOF &&
+	$(git rev-parse H) $(git rev-parse HEAD~2)
+	$(git rev-parse E) $(git rev-parse HEAD~1)
+	$(git rev-parse I) $(git rev-parse HEAD~1)
+	$(git rev-parse G) $(git rev-parse HEAD)
 	EOF
 	verify_hook_input
 '
-- 
2.54.0.200.gfd8d68259e3