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

Uwe Kleine-König <u.kleine-koenig@baylibre.com> writes:

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.

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.

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

Style. In our codebase, redirection operator sticks to the redirection target without SP in between, i.e.

	git version >version &&
	echo A >A &&

'@' is hard to read; when you refer to HEAD, please write HEAD.

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.

Hello Junio,

On Wed, Jun 17, 2026 at 06:24:03AM -0700, Junio C Hamano wrote:

OK, sounds fair.

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.

Yeah, or in a patch that was picked before.

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

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

Oh, that is a debug thing that shouldn't have made it into the patch.
 

Ah, I really wondered if I'm missing something because it should be easier to say "this command should fail".

Best regards Uwe

Hi Uwe and Junio

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

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.

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.

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

Hello Phillip,

On Fri, Jun 19, 2026 at 11:13:32AM +0100, Phillip Wood wrote:

Very appreciated! Looking forward to test your patches.

Best regards Uwe

On 19/06/2026 11:13, Phillip Wood wrote:

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

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

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

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

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(+)

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

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

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

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

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

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

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

Phillip Wood <phillip.wood123@gmail.com> writes:

Thanks.

A tangent (I Cc'ed Konstantin for this), but

    $ b4 am -o- '<cover.1782833268.git.phillip.wood@dunelm.org.uk>' >b4am.mbx

failed to produce a usable mailbox. It somehow did not think [2/11] existed. I manually examined the References and In-Reply-To headers of that particular message and compared them with those from other messages but did not find anything suspicious X-<.

I have a bunch of typofixes queued on top of these 11 patches (made with "git commit --fixup reword:<sha1>"); please double check when you reroll after seeing more substantial reviews than mere typofixes, possibly from others.

Thanks.

Here is the transcript of failed b4 am invocation.
---- >8 ----
Looking up https://lore.kernel.org/all/cover.1782833268.git.phillip.wood@dunelm.org.uk/
Grabbing thread from lore.kernel.org/all/cover.1782833268.git.phillip.wood@dunelm.org.uk/t.mbox.gz
Analyzing 17 messages in the thread
WARNING: duplicate messages found at index 1
   Subject 1: sequencer: Skip copying notes for commits that disappear during rebase
   Subject 2: t3400: restore coverage for note copying with apply backend
  2 is not a reply... assume additional patch
Looking for additional code-review trailers on lore.kernel.org
Analyzing 0 code-review messages
Checking attestation on all messages, may take a moment...
---
  ✗ [PATCH] sequencer: Skip copying notes for commits that disappear during rebase
    ✗ No key: openpgp/u.kleine-koenig@baylibre.com
    ✗ BADSIG: DKIM/baylibre.com
  ✓ [PATCH 1/11] t3400: restore coverage for note copying with apply backend
    ✓ Signed: DKIM/gmail.com
  ✓ [PATCH 3/11] sequencer: be more careful with external merge
    ✓ Signed: DKIM/gmail.com
  ✓ [PATCH 4/11] sequencer: never reschedule on failed commit
    ✓ Signed: DKIM/gmail.com
  ✓ [PATCH 5/11] sequencer: remove unnecessary "or" in pick_one_commit()
    ✓ Signed: DKIM/gmail.com
  ✓ [PATCH 6/11] sequencer: simplify handing of fixup with conflicts
    ✓ Signed: DKIM/gmail.com
  ✓ [PATCH 7/11] sequencer: remove unnecessary condition in pick_one_commit()
    ✓ Signed: DKIM/gmail.com
  ✓ [PATCH 8/11] sequencer: simplify pick_one_commit()
    ✓ Signed: DKIM/gmail.com
  ✓ [PATCH 9/11] sequencer: return early from pick_one_commit() on success
    ✓ Signed: DKIM/gmail.com
  ✓ [PATCH 10/11] sequencer: use an enum to represent result of picking a commit
    ✓ Signed: DKIM/gmail.com
  ✓ [PATCH 11/11] sequencer: do not record dropped commits as rewritten
    ✓ Signed: DKIM/gmail.com
  ERROR: missing [12/2]!
---
Total patches: 11
---
WARNING: Thread incomplete!
 Link: https://patch.msgid.link/cover.1782833268.git.phillip.wood@dunelm.org.uk
:

Hello,

On Tue, Jun 30, 2026 at 12:57:32PM -0700, Junio C Hamano wrote:

FTR: The mail is on lore.kernel.org.

Also to yield a usable mailbox my patch shouldn't be included.

I think here is the origin of the problem. It guesses that the t3400 should be added, and it takes the place of Phillip's second patch.

This is irritating, I would have expected "[2/12]" here?

	b4 am --no-parent cover.1782833268.git.phillip.wood@dunelm.org.uk

works fine for me.

Best regards Uwe

Hello Phillip,

thanks a lot for addressing this, very appreciated!

On Tue, Jun 30, 2026 at 04:28:50PM +0100, Phillip Wood wrote:

With my very little knowledge about git internals, this looks reasonable, and it behaves as I expect in my test case. I installed a local

Tested-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

BTW, b4 didn't pick this up, for me it says:

	Base: not specified

(and I applied it on top of 2.55.0).

Best regards Uwe

On 01/07/2026 07:00, Uwe Kleine-König wrote:

Sorry I had intended to send these as v2 to avoid any confusion, but I forgot about that when I actually came to send them.

Thanks

Phillip

Hi Junio

On 30/06/2026 20:57, Junio C Hamano wrote:

Thanks, I'll squash those locally and wait before resending

Phillip

Hi Uwe

On 01/07/2026 10:38, Uwe Kleine-König wrote:

Thanks for testing these patches

Oh, my script generates the same trailers as GitGitGadget but I see "git format-patch" uses "base-commit:" I wonder if b4 expects it to be all lower case.

Thanks

Phillip

On Tue, Jun 30, 2026 at 04:28:58PM +0100, Phillip Wood wrote:

because of this ...

.. the res conditional is pointless here.

and here as well.

On Tue, Jun 30, 2026 at 04:28:59PM +0100, Phillip Wood wrote:

i think it would be much more logical to just squash that into the parent commit.

On Tue, Jun 30, 2026 at 04:29:00PM +0100, Phillip Wood wrote:

have you attempted widening the scope of the enum? the three conversions between the new enum and existing int return values irk me.

On 06/07/2026 12:12, Oswald Buddenhagen wrote:

I know what you mean, but how wide should be go? Using the enum just one level up the call chain means converting a whole load of functions which creates a lot of churn that someone needs to review. I decided to keep the enum limited to this scope for now to avoid that.

Thanks

Phillip

On 06/07/2026 12:06, Oswald Buddenhagen wrote:

I meant to add a comment about that to the commit message. I deliberately left them alone so that when we convert them to use the enum it is clear that these arms are handling cases with conflicts.

Thanks

Phillip

Phillip Wood <phillip.wood123@gmail.com> writes:

Thanks.

Just responding belatedly as I was scanning topics that are marked as "Expecting a reroll" in my draft copy of the "What's cooking" report that I work from.

Thanks to everyone who commented on v1. I've squashed the fixups that Junio had in "seen", squashed patches 8 & 9 together as suggested by Oswald and expanded the commit message, and added Uwe's Tested-by: trailer to the final patch. Oswald suggested extended the use of the enum which I think is a good idea in the long-term but I punted on that for now because I think it would be fairly invasive and this series has enough refactoring in it already.

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 9).

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

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

Patch 10 adds a new member to the enum from patch 9 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%2Fv2
View-Changes-At: https://github.com/phillipwood/git/compare/6c3d7b735...c89234dd9
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/rebase-drop-notes-with-commit/v2

Phillip Wood (10):
  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: 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(-)

Range-diff against v1:
 1:  65af2ac07a2 =  1:  65af2ac07a2 t3400: restore coverage for note copying with apply backend
 2:  02670f57e7d =  2:  02670f57e7d sequencer: move definition of is_final_fixup()
 3:  16fba1e823b !  3:  3d79362332c sequencer: be more careful with external merge
    @@ sequencer.c: static int do_pick_commit(struct repository *r,
     +					opts->xopts.nr, opts->xopts.v,
      					common, oid_to_hex(&head), remotes);
     +		/*
    -+		 * If the there were conflicts, try_merge_command() returns 1,
    ++		 * If 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.
     +		 */
 4:  3ffd06d6509 !  4:  fc89e77c6e8 sequencer: never reschedule on failed commit
    @@ sequencer.c: static int do_pick_commit(struct repository *r,
      			*check_todo = 1;
      		}
     +		/*
    -+		 * If "git commit" failed to run than res == -1 but we dont
    ++		 * If "git commit" failed to run then res == -1, but we don't
     +		 * want reschedule the last command because the picking the
     +		 * commit was successful.
     +		 */
 5:  cb286ac70d7 !  5:  26eef6c0958 sequencer: remove unnecessary "or" in pick_one_commit()
    @@ Commit message
     
         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.
    +    with "res" is pointless as the result is the same as the return value.
     
         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
 6:  1585d47e2ea =  6:  26dc48951ce sequencer: simplify handing of fixup with conflicts
 7:  4386ca67d10 =  7:  71ed717d322 sequencer: remove unnecessary condition in pick_one_commit()
 8:  f51751fa3ec !  8:  e8b7fa4c59e sequencer: simplify pick_one_commit()
    @@ Commit message
         sequencer: simplify pick_one_commit()
     
         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
    +    do_pick_commit() and return its result. Simplify the code by returning
         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.
    +
    +    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.
     
         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.
    +
    +    While we could remove "res" from the conditions below "if (!res)"
    +    they are left alone because, when we start using an enum in the next
    +    commit, it makes it clear that these clauses are handling cases where
    +    there are conflicts.
     
         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
    @@ sequencer.c: 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)) {
    ++		return 0;
     +	} else if (res && is_fixup(item->command)) {
      		return error_failed_squash(r, item->commit, opts,
      					   item->arg_len, arg);
    @@ sequencer.c: static int pick_one_commit(struct repository *r,
      		int to_amend = 0;
      		struct object_id oid;
      
    +@@ sequencer.c: 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,
 9:  2541a4d6e3d <  -:  ----------- sequencer: return early from pick_one_commit() on success
10:  e4050ead27f =  9:  4fb641afb3c sequencer: use an enum to represent result of picking a commit
11:  26551f2687b ! 10:  c89234dd949 sequencer: do not record dropped commits as rewritten
    @@ Commit message
         when rewording a fast-forwarded commit.
     
         Reported-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
    +    Tested-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
      ## sequencer.c ##

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

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

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

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(+)

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 as 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(-)

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

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

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

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.

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.

While we could remove "res" from the conditions below "if (!res)" they are left alone because, when we start using an enum in the next commit, it makes it clear that these clauses are handling cases where there are conflicts.

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

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

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>
Tested-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(-)

On Mon, Jul 13, 2026 at 02:17:18PM +0100, Phillip Wood wrote:

add comma here for ease of parsing?

and here?

On Mon, Jul 13, 2026 at 02:17:20PM +0100, Phillip Wood wrote:

s/to/from/, i think?

parenthesize, and add comma?

(maybe add comma? here it becomes muddy ...)

add comma?

i know that most commas (and parens (or em-dashes)) are optional in english, but they _really_ help parsing complex sentences, because they reduce the amount of "read-ahead" required. i'm stopping at this commit, but subsequent ones could also use the treatment. i trust that you don't actually need detailed suggestions.

On Mon, Jul 13, 2026 at 02:17:23PM +0100, Phillip Wood wrote:

that part makes no sense, subverting the argumentation. (as-is, i actually can't follow the logic, but i suppose it would be clear with (much) more diff context. i'm not sure whether the commit message is supposed to substitute for that, or the reviewer is supposed to deal with that on their end.)

Phillip Wood <phillip.wood123@gmail.com> writes:

Thanks for a concise yet very informative summary of the changes upfront. This may be a format we want to encourage to contributors.

Very well. I did not see anything questionable in this edition. The contents of the tree at the end of the series is unchanged since the previous iteration.

Shall we mark the topic ready for 'next' now?

Thanks.

4910 is greater than 4627, the function is_final_fixup() seems to have been moved _later_ in the file. But the commit message says "Move this function earlier in the file". Am I missing something?

Hi Andrei

On 14/07/2026 23:50, Andrei Rybak wrote:

Oh, thanks for the sanity check. I could have sworn I had to move this function to get a later commit to compile at one point, but it clearly doesn't need to move now. I'll drop this patch.

Thanks

Phillip

Hi Oswald

On 13/07/2026 15:09, Oswald Buddenhagen wrote:

It should say "call it in error_with_patch() instead"

Its tricky because error_with_patch() isn't changed at all, we change error_failed_squash() to tell error_with_patch() to call intend_to_amend(). I've expanded the commit message to explain that better.

Thanks

Phillip

Hi Oswald

On 13/07/2026 15:01, Oswald Buddenhagen wrote:

Both are valid - the internet tells be "different to" is more common it British English, whereas "different from" is more common in American English. I guess for an international audience "from" would be the better choice.

I've added a few more commas to later commits, but concrete suggestions are always welcome.

Thanks

Phillip

On 15/07/2026 10:35, Phillip Wood wrote:

sigh s/be/me/

Phillip

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

Thanks to everyone who commented on v2. I've dropped patch 2 which Andrei pointed out was pointless and tried to make the remaining commit messages clearer as requested by Oswald.

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.

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

Patches 4-7 try and simplify the control flow in pick_one_commit() in preparation for patch 8.

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

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

Cover letter for v2:

Thanks to everyone who commented on v1. I've squashed the fixups that Junio had in "seen", squashed patches 8 & 9 together as suggested by Oswald and expanded the commit message, and added Uwe's Tested-by: trailer to the final patch. Oswald suggested extended the use of the enum which I think is a good idea in the long-term but I punted on that for now because I think it would be fairly invasive and this series has enough refactoring in it already.

base-commit: 6c3d7b73556db708feb3b16232fab1efc4353428
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Frebase-drop-notes-with-commit%2Fv3
View-Changes-At: https://github.com/phillipwood/git/compare/6c3d7b735...2ef36b9ee
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/rebase-drop-notes-with-commit/v3

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

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

Range-diff against v2:
 1:  65af2ac07a2 !  1:  c4705066ee0 t3400: restore coverage for note copying with apply backend
    @@ Metadata
      ## Commit message ##
         t3400: restore coverage for note copying with apply backend
     
    -    Now that the merge backend is the default we have lost coverage for
    +    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.
 2:  02670f57e7d <  -:  ----------- sequencer: move definition of is_final_fixup()
 3:  3d79362332c !  2:  947bb77e44f sequencer: be more careful with external merge
    @@ Commit message
     
         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
    +    code other than 1. This should be treated differently from a merge
    +    with conflicts, which is signaled 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.
 4:  fc89e77c6e8 =  3:  bff5f319e91 sequencer: never reschedule on failed commit
 5:  26eef6c0958 =  4:  e785433ad3d sequencer: remove unnecessary "or" in pick_one_commit()
 6:  26dc48951ce !  5:  134d8f7e935 sequencer: simplify handing of fixup with conflicts
    @@ Metadata
     Author: Phillip Wood <phillip.wood@dunelm.org.uk>
     
      ## Commit message ##
    -    sequencer: simplify handing of fixup with conflicts
    +    sequencer: simplify handling of fixup with conflicts
     
         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.
    +    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. That
    +    means we can simplify the code by removing the conditional call to
    +    intend_to_amend() and have error_failed_squash() request that it is
    +    called in error_with_patch() instead.
     
         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
 7:  71ed717d322 =  6:  e3091dee633 sequencer: remove unnecessary condition in pick_one_commit()
 8:  e8b7fa4c59e !  7:  7c1642b0a49 sequencer: simplify pick_one_commit()
    @@ Metadata
      ## Commit message ##
         sequencer: simplify pick_one_commit()
     
    -    Unless we're rebasing all we do in pick_one_commit() is call
    +    Unless we're rebasing, all we do in pick_one_commit() is call
         do_pick_commit() and return its result. Simplify the code by returning
    -    early if we're not rebasing so that we don't have to continually call
    +    early if we're not rebasing so that we don't have to repeatedly 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.
     
         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.
    +    recording the commit as rewritten, all we do is return from the
    +    function.
     
         As the conditional blocks are all mutually exclusive (either the
         conditions are mutually exclusive, or an earlier conditional block
 9:  4fb641afb3c !  8:  0a146d57266 sequencer: use an enum to represent result of picking a commit
    @@ Metadata
      ## Commit message ##
         sequencer: use an enum to represent result of picking a commit
     
    -    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
    +    Rather than using an integer where -1 is an error, 0 is success and 1
    +    indicates 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.
     
10:  c89234dd949 !  9:  2ef36b9ee5a sequencer: do not record dropped commits as rewritten
    @@ Commit message
     
         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
    +    post-rewrite hooks, it 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
    +    While we do not want to record the dropped commit as 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"

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(+)

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 from a merge with conflicts, which is signaled 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(-)

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 as 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(-)

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. That means we can simplify the code by removing the conditional call to intend_to_amend() and have error_failed_squash() request that it is called in error_with_patch() instead.

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

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

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 returning early if we're not rebasing so that we don't have to repeatedly 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.

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.

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.

While we could remove "res" from the conditions below "if (!res)" they are left alone because, when we start using an enum in the next commit, it makes it clear that these clauses are handling cases where there are conflicts.

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

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

Rather than using an integer where -1 is an error, 0 is success and 1 indicates 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(-)

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, it 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 as 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>
Tested-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(-)

Phillip Wood <phillip.wood123@gmail.com> writes:

sigh s/it/in/ ;-)

Hello,

On Wed, Jul 01, 2026 at 11:38:27AM +0200, Uwe Kleine-König wrote:

While it works fine in my test case, it doesn't in my real-life workflow.

I have a big branch of changes that I maintain on top of next/master, on todays rebase I experience:

	uwe@monoceros:~/gsrc/linux-2nd$ git rebase --onto=next-20260717 next-20260716 -r -i device_id^{}
	... handling commits that get empty using `git rebase --skip` ...

	uwe@monoceros:~/gsrc/linux-2nd$ git range-diff next-20260716..device_id next-20260717..
	...
	 24:  901ca5f67bc5 !  24:  9f3e8813f6b4 mtd: nand-omap2: Move omap_nand_ids[] to raw nand driver
	    @@ Commit message
	      ## Notes ##
		 Forwarded: id:901ca5f67bc57219a9222115fabe1a1729b87e25.1784229863.git.ukleinek@kernel.org

	    +    Forwarded: id:20260716123646.1933293-2-u.kleine-koenig@baylibre.com
	    +
	      ## drivers/memory/omap-gpmc.c ##
	     @@ drivers/memory/omap-gpmc.c: static void __maybe_unused gpmc_read_timings_dt(struct device_node *np,
			of_property_read_bool(np, "gpmc,time-para-granularity");
	 25:  69be5d4f9f13 <   -:  ------------ drm/radeon: Only define radeon_acpi_vfct_match when actually used
	...

with:

	uwe@monoceros:~/gsrc/linux-2nd$ git notes show 69be5d4f9f13
	Forwarded: id:20260716123646.1933293-2-u.kleine-koenig@baylibre.com

When I rebase without -i, the rebase happens without hitting empty commits that I have to manually skip and then the notes for 69be5d4f9f13 doesn't make it into the neighbour commit after rebase.

So it seems there is still something fishy with interactive rebase.

Best regards Uwe

Hi Uwe

On 18/07/2026 09:37, Uwe Kleine-König wrote:

For historic reasons "-i" implies "--empty=ask", without "-i" the default "--empty=drop" (the UI is a mess). This patch series only stops commits that are dropped by "--empty=drop" from being recorded as rewritten, so it will only have an effect with "-i" if you add "--empty=drop". I'm still thinking about how to handle commits that are dropped by the user, for example when when they run "git rebase --skip" after a conflict, or they run "git rebase --continue" without committing after a commit that becomes empty with "--empty=ask". As an aside I really wish "--empty=ask" kept the empty commit on "git rebase --continue" and dropped it on "git rebase --skip" but the current behavior dates from the early days of git.

Thanks

Phillip

Phillip Wood <phillip.wood123@gmail.com> writes:

I see Phillip Cc'ed everybody who participated in the review for the previous iterations, which is very much appreciated.

It looks like this is now ready to go?  Any further comments?

Thanks.

On Sun, Jul 19, 2026 at 12:29:31PM -0700, Junio C Hamano wrote:

you can add whatever footer is appropriate for "i read it, it seems to make sense, but i didn't double-check" for me.

(same for phillip's new 2-patch series.)

(it feels silly to "spam" the list with such low-value verdicts. i really miss gerrit code review here, where i'd leave a +1 in passing.)

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

Actually, reducing the signal to a single bit, 'did I or did I not see a +1 from them?', means Gerrit users see less 'spam' but must make decisions based on too little signal. I do not know whether that is an advantage.

With your email, we can at least discern that your comment is much closer to an 'Acked-by' than a 'Reviewed-by', and we can respect that distinction when judging whether there is sufficient consensus on the list to move the topic forward.

In any case, thank you for reading it over and letting us know that you found nothing glaringly wrong. That is indeed valuable information.

Thanks.

On Mon, Jul 20, 2026 at 10:03:23AM -0700, Junio C Hamano wrote:

gerrit discerns from -2 to +2 (*), so that angle is covered (**). https://gerrit-review.googlesource.com/Documentation/config-labels.html#label_Code-Review

(*) actually however many levels the project chooses to configure, though things aren't as smooth when deviating from the defaults

(**) mostly - https://issues.gerritcodereview.com/issues/40000793

Hi Oswald

On 20/07/2026 13:15, Oswald Buddenhagen wrote:

Thanks for reading them through - I'm glad to hear the commit messages make sense now.

Phillip