{"thread":{"id":"65820","subject":"[PATCH] sequencer: Skip copying notes for commits that disappear during rebase","startedAt":"2026-06-16T17:40:24Z","lastAt":"2026-07-22T15:15:47Z","messageCount":67,"participants":["Uwe Kleine-König","Junio C Hamano","Phillip Wood","Oswald Buddenhagen","Andrei Rybak"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"545675","messageId":"20260616174012.601651-2-u.kleine-koenig@baylibre.com","threadId":"65820","inReplyTo":null,"subject":"[PATCH] sequencer: Skip copying notes for commits that disappear during rebase","fromName":"Uwe Kleine-König","fromEmail":"u.kleine-koenig@baylibre.com","sentAt":"2026-06-16T17:40:12Z","receivedAt":"2026-06-16T17:40:24Z","isPatch":true,"body":"When a commit disappears during rebase because the patch content is\nalready there (but not by the same patch in which case the commit would\nbe skipped) the notes of that disappearing commit should not be copied\nto the unrelated commit that happens to be HEAD.\n\nSigned-off-by: Uwe Kleine-König \n---\nHello,\n\nafter also my 2nd bug report[1] didn't motivate anyone to come up with a\nfix, I invested the time to work out one according to Phillip Wood's\nsuggestion.\n\nIMHO it's not pretty, but it works for me.\n\nNote that Phillip also suggested to integrete the test into\nt3400-rebase.sh . IMHO it doesn't matter much if this is considered a\nrebase test or a notes test. I kept it where I have it because I'm lazy\nand failed to understand the git history created in that test.\n\nBest regards\nUwe\n\n[1] https://lore.kernel.org/git/20260612143952.3281115-2-u.kleine-koenig@baylibre.com\n\n\n sequencer.c | 20 ++++++++++----------\n t/meson.build | 1 +\n t/t3322-notes-rebase.sh | 37 +++++++++++++++++++++++++++++++++++++\n 3 files changed, 48 insertions(+), 10 deletions(-)\n create mode 100755 t/t3322-notes-rebase.sh\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 57855b0066ac..da2185a37c5d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2263,7 +2263,7 @@ static const char *reflog_message(struct replay_opts *opts,\n static int do_pick_commit(struct repository *r,\n \t\t\t struct todo_item *item,\n \t\t\t struct replay_opts *opts,\n-\t\t\t int final_fixup, int *check_todo)\n+\t\t\t int final_fixup, int *check_todo, int *dropped_commit)\n {\n \tstruct replay_ctx *ctx = opts->ctx;\n \tunsigned int flags = should_edit(opts) ? EDIT_MSG : 0;\n@@ -2273,7 +2273,7 @@ static int do_pick_commit(struct repository *r,\n \tconst char *base_label, *next_label, *reflog_action;\n \tchar *author = NULL;\n \tstruct commit_message msg = { NULL, NULL, NULL, NULL };\n-\tint res, unborn = 0, reword = 0, allow, drop_commit;\n+\tint res, unborn = 0, reword = 0, allow;\n \tenum todo_command command = item->command;\n \tstruct commit *commit = item->commit;\n \n@@ -2492,7 +2492,7 @@ static int do_pick_commit(struct repository *r,\n \t\tgoto leave;\n \t}\n \n-\tdrop_commit = 0;\n+\t*dropped_commit = 0;\n \tallow = allow_empty(r, opts, commit);\n \tif (allow < 0) {\n \t\tres = allow;\n@@ -2500,7 +2500,7 @@ static int do_pick_commit(struct repository *r,\n \t} else if (allow == 1) {\n \t\tflags |= ALLOW_EMPTY;\n \t} else if (allow == 2) {\n-\t\tdrop_commit = 1;\n+\t\t*dropped_commit = 1;\n \t\trefs_delete_ref(get_main_ref_store(r), \"\", \"CHERRY_PICK_HEAD\",\n \t\t\t\tNULL, REF_NO_DEREF);\n \t\tunlink(git_path_merge_msg(r));\n@@ -2510,7 +2510,7 @@ static int do_pick_commit(struct repository *r,\n \t\t\t_(\"dropping %s %s -- patch contents already upstream\\n\"),\n \t\t\toid_to_hex(&commit->object.oid), msg.subject);\n \t} /* else allow == 0 and there's nothing special to do */\n-\tif (!opts->no_commit && !drop_commit) {\n+\tif (!opts->no_commit && !*dropped_commit) {\n \t\tif (author || command == TODO_REVERT || (flags & AMEND_MSG))\n \t\t\tres = do_commit(r, msg_file, author, reflog_action,\n \t\t\t\t\topts, flags,\n@@ -4943,12 +4943,12 @@ static int pick_one_commit(struct repository *r,\n \t\t\t struct replay_opts *opts,\n \t\t\t int *check_todo, int* reschedule)\n {\n-\tint res;\n+\tint res, dropped_commit;\n \tstruct todo_item *item = todo_list->items + todo_list->current;\n \tconst char *arg = todo_item_get_arg(todo_list, item);\n \n \tres = do_pick_commit(r, item, opts, is_final_fixup(todo_list),\n-\t\t\t check_todo);\n+\t\t\t check_todo, &dropped_commit);\n \tif (is_rebase_i(opts) && res < 0) {\n \t\t/* Reschedule */\n \t\t*reschedule = 1;\n@@ -4965,7 +4965,7 @@ static int pick_one_commit(struct repository *r,\n \t\treturn error_with_patch(r, commit,\n \t\t\t\t\targ, item->arg_len, opts, res, !res);\n \t}\n-\tif (is_rebase_i(opts) && !res)\n+\tif (is_rebase_i(opts) && !res && !dropped_commit)\n \t\trecord_in_rewritten(&item->commit->object.oid,\n \t\t\t\t peek_command(todo_list, 1));\n \tif (res && is_fixup(item->command)) {\n@@ -5523,14 +5523,14 @@ static int single_pick(struct repository *r,\n \t\t struct commit *cmit,\n \t\t struct replay_opts *opts)\n {\n-\tint check_todo;\n+\tint check_todo, dummy;\n \tstruct todo_item item;\n \n \titem.command = opts->action == REPLAY_PICK ?\n \t\t\tTODO_PICK : TODO_REVERT;\n \titem.commit = cmit;\n \n-\treturn do_pick_commit(r, &item, opts, 0, &check_todo);\n+\treturn do_pick_commit(r, &item, opts, 0, &check_todo, &dummy);\n }\n \n int sequencer_pick_revisions(struct repository *r,\ndiff --git a/t/meson.build b/t/meson.build\nindex c5832fee0535..6927bd9c794f 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -358,6 +358,7 @@ integration_tests = [\n 't3311-notes-merge-fanout.sh',\n 't3320-notes-merge-worktrees.sh',\n 't3321-notes-stripspace.sh',\n+ 't3322-notes-rebase.sh',\n 't3400-rebase.sh',\n 't3401-rebase-and-am-rename.sh',\n 't3402-rebase-merge.sh',\ndiff --git a/t/t3322-notes-rebase.sh b/t/t3322-notes-rebase.sh\nnew file mode 100755\nindex 000000000000..0eddde7f9961\n--- /dev/null\n+++ b/t/t3322-notes-rebase.sh\n@@ -0,0 +1,37 @@\n+#!/bin/sh\n+\n+test_description='Test notes on rebase'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\tgit init &&\n+\tgit config notes.rewriteRef refs/notes/commits &&\n+\tgit version > version &&\n+\techo A > A &&\n+\tgit add A &&\n+\tgit commit -m A &&\n+\tgit branch branch &&\n+\techo B > B &&\n+\tgit add B &&\n+\tgit commit -m B &&\n+\tgit notes add -m \"This is B\" @ &&\n+\techo C > C &&\n+\tgit add C &&\n+\tgit commit -m C &&\n+\tgit checkout branch &&\n+\techo B > B &&\n+\techo D > D &&\n+\tgit add B D &&\n+\tgit commit -m BD\n+'\n+\n+test_expect_success 'rebase B + C on top of BD' '\n+\tgit rebase @ master\n+'\n+\n+test_expect_success 'assert there is no note on BD' '\n+\tif git notes list branch >/tmp/lalaa; then return 1; fi\n+'\n+\n+test_done\n\nbase-commit: 3e65291872de10c3f0bf05ea8c24187e7a71ebf0\n-- \n2.47.3\n\n"},{"id":"545764","messageId":"xmqqzf0txpu4.fsf@gitster.g","threadId":"65820","inReplyTo":"20260616174012.601651-2-u.kleine-koenig@baylibre.com","subject":"Re: [PATCH] sequencer: Skip copying notes for commits that disappear during rebase","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-17T13:24:03Z","receivedAt":"2026-06-17T13:24:06Z","isPatch":true,"body":"Uwe Kleine-König writes:\n\n> Note that Phillip also suggested to integrete the test into\n> t3400-rebase.sh . IMHO it doesn't matter much if this is considered a\n> rebase test or a notes test. I kept it where I have it because I'm lazy\n> and failed to understand the git history created in that test.\n\nI do not think his suggestion was about \"is this rebase or notes?\"\nat all. It was a lot more about \"let's not add a new test script\nthat does only one thing, when there is already a script that covers\nthe same command and the same option for the command\". In fact,\naround 3400.28 there are test pieces that rebases commits that have\nnotes.\n\n> sequencer.c | 20 ++++++++++----------\n> t/meson.build | 1 +\n> t/t3322-notes-rebase.sh | 37 +++++++++++++++++++++++++++++++++++++\n> 3 files changed, 48 insertions(+), 10 deletions(-)\n> create mode 100755 t/t3322-notes-rebase.sh\n\nWe need some documentation updates to describe that the users can\nlose notes by doing a rebase and under what condition, no?\n\nIt is not yet clear to me if we want to _always_ discard a note from\na commit that would become \"empty\" during a rebase session (in other\nwords, a commit that becomes empty during a rebase is _always_ a\nsign that the change it brings in is _already_ in the new base of\nthe rebase and the necessary information the note wanted to carry to\nthe target branch is there without need to _duplicate_ it by copying\nthe note). But assuming that we want the behaviour, the code change\nto sequencer.c looks very reasonable to me, except for one thing that\nI am not clear about.\n\n> diff --git a/sequencer.c b/sequencer.c\n> index 57855b0066ac..da2185a37c5d 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> ...\n> @@ -4965,7 +4965,7 @@ static int pick_one_commit(struct repository *r,\n> \t\treturn error_with_patch(r, commit,\n> \t\t\t\t\targ, item->arg_len, opts, res, !res);\n> \t}\n> -\tif (is_rebase_i(opts) && !res)\n> +\tif (is_rebase_i(opts) && !res && !dropped_commit)\n> \t\trecord_in_rewritten(&item->commit->object.oid,\n> \t\t\t\t peek_command(todo_list, 1));\n\nIf we have a sequence of commits where a commit that was *not*\ndropped is followed by a fixup commit that *is* dropped (e.g.,\nbecause it became empty/redundant), wouldn't it prevent the\npreviously pending commit from being flushed to skip\n`record_in_rewritten` entirely for the dropped fixup commit?\n\nFor example, if we have\n\n pick X (with note)\n fixup B (dropped because it is redundant)\n pick C\n\n1. `pick X`: calls `record_in_rewritten(X, TODO_FIXUP)`. `X` is\n written to `pending`, but not flushed because the next insn is\n `TODO_FIXUP` (B).\n\n2. `fixup B`: gets dropped. `dropped_commit` is 1 in the code above,\n so `record_in_rewritten` is skipped.\n\n3. `pick C`: calls `record_in_rewritten(C, -1)`. `C` is written to\n `pending`. Since next insn is not a fixup, it flushes `pending`\n (which contains both `X` and `C`) to the commit created for `C`.\n\nWouldn't it map the note for `X` to rewritten `C`?\n\n> diff --git a/t/t3322-notes-rebase.sh b/t/t3322-notes-rebase.sh\n> new file mode 100755\n> index 000000000000..0eddde7f9961\n> --- /dev/null\n> +++ b/t/t3322-notes-rebase.sh\n> @@ -0,0 +1,37 @@\n> +#!/bin/sh\n> +\n> +test_description='Test notes on rebase'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success setup '\n> +\tgit init &&\n> +\tgit config notes.rewriteRef refs/notes/commits &&\n> +\tgit version > version &&\n> +\techo A > A &&\n\nStyle. In our codebase, redirection operator sticks to the\nredirection target without SP in between, i.e.\n\n\tgit version >version &&\n\techo A >A &&\n\n> +\tgit notes add -m \"This is B\" @ &&\n\n'@' is hard to read; when you refer to HEAD, please write HEAD.\n\n\n> +test_expect_success 'rebase B + C on top of BD' '\n> +\tgit rebase @ master\n> +'\n> +\n> +test_expect_success 'assert there is no note on BD' '\n> +\tif git notes list branch >/tmp/lalaa; then return 1; fi\n> +'\n\nDo not step outside of $TRASH_DIRECTORY without a good reason.\n\nStyle. In our codebase, shell scripts do not use ';' and written\nmore like\n\n\tif git notes list branch >notes-list\n\tthen\n\t\treturn 1\n\tfi\n\nBut more importantly, if you want to make sure the command makes a\ncontrolled exit (not crash), use\n\n\ttest_must_fail git notes list branch\n\nThat will pass the test happily if \"git notes list branch\" makes a\ncontrolled die() call (e.g., when there is no notes attached to that\ncommit, the command exits with 1), but still makes the test fail if\n\"git notes list branch\" segfaults.\n\nAgain, we do not want to add a new test script that does only one\nthing, when there is already a script that covers the same command\nand the same option for the command.\n\nThanks.\n"},{"id":"545765","messageId":"ajKimV1TDCgE-GzK@monoceros","threadId":"65820","inReplyTo":"xmqqzf0txpu4.fsf@gitster.g","subject":"Re: [PATCH] sequencer: Skip copying notes for commits that disappear during rebase","fromName":"Uwe Kleine-König","fromEmail":"u.kleine-koenig@baylibre.com","sentAt":"2026-06-17T13:58:19Z","receivedAt":"2026-06-17T13:58:23Z","isPatch":true,"body":"Hello Junio,\n\nOn Wed, Jun 17, 2026 at 06:24:03AM -0700, Junio C Hamano wrote:\n> Uwe Kleine-König writes:\n> \n> > Note that Phillip also suggested to integrete the test into\n> > t3400-rebase.sh . IMHO it doesn't matter much if this is considered a\n> > rebase test or a notes test. I kept it where I have it because I'm lazy\n> > and failed to understand the git history created in that test.\n> \n> I do not think his suggestion was about \"is this rebase or notes?\"\n> at all. It was a lot more about \"let's not add a new test script\n> that does only one thing, when there is already a script that covers\n> the same command and the same option for the command\". In fact,\n> around 3400.28 there are test pieces that rebases commits that have\n> notes.\n\nOK, sounds fair.\n\n> > sequencer.c | 20 ++++++++++----------\n> > t/meson.build | 1 +\n> > t/t3322-notes-rebase.sh | 37 +++++++++++++++++++++++++++++++++++++\n> > 3 files changed, 48 insertions(+), 10 deletions(-)\n> > create mode 100755 t/t3322-notes-rebase.sh\n> \n> We need some documentation updates to describe that the users can\n> lose notes by doing a rebase and under what condition, no?\n\nWell, the current state is that we're not losing notes, but that we\nattach it to commits that most of the time are completely unrelated to\nthe commit the note was initially attached to. (i.e. in general it's not\nattached to the commit that made the currently picked commit empty.) So\nessentially the notes are lost, too, but also add confusion to where\nthey happen to get attached to.\n\n> It is not yet clear to me if we want to _always_ discard a note from\n> a commit that would become \"empty\" during a rebase session (in other\n> words, a commit that becomes empty during a rebase is _always_ a\n> sign that the change it brings in is _already_ in the new base of\n> the rebase\n\nYeah, or in a patch that was picked before.\n\n> and the necessary information the note wanted to carry to\n> the target branch is there without need to _duplicate_ it by copying\n> the note). But assuming that we want the behaviour, the code change\n> to sequencer.c looks very reasonable to me, except for one thing that\n> I am not clear about.\n\nI think given the commit goes away, it's natural that the note goes\naway, too. And to come back to your question above: I think it doesn't\nneed documentation, that if a commit disappears its notes go away, too.\nBut that might be subjective?!\n\n> > diff --git a/sequencer.c b/sequencer.c\n> > index 57855b0066ac..da2185a37c5d 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > ...\n> > @@ -4965,7 +4965,7 @@ static int pick_one_commit(struct repository *r,\n> > \t\treturn error_with_patch(r, commit,\n> > \t\t\t\t\targ, item->arg_len, opts, res, !res);\n> > \t}\n> > -\tif (is_rebase_i(opts) && !res)\n> > +\tif (is_rebase_i(opts) && !res && !dropped_commit)\n> > \t\trecord_in_rewritten(&item->commit->object.oid,\n> > \t\t\t\t peek_command(todo_list, 1));\n> \n> If we have a sequence of commits where a commit that was *not*\n> dropped is followed by a fixup commit that *is* dropped (e.g.,\n> because it became empty/redundant), wouldn't it prevent the\n> previously pending commit from being flushed to skip\n> `record_in_rewritten` entirely for the dropped fixup commit?\n> \n> For example, if we have\n> \n> pick X (with note)\n> fixup B (dropped because it is redundant)\n> pick C\n> \n> 1. `pick X`: calls `record_in_rewritten(X, TODO_FIXUP)`. `X` is\n> written to `pending`, but not flushed because the next insn is\n> `TODO_FIXUP` (B).\n> \n> 2. `fixup B`: gets dropped. `dropped_commit` is 1 in the code above,\n> so `record_in_rewritten` is skipped.\n> \n> 3. `pick C`: calls `record_in_rewritten(C, -1)`. `C` is written to\n> `pending`. Since next insn is not a fixup, it flushes `pending`\n> (which contains both `X` and `C`) to the commit created for `C`.\n\nHuh, sounds possible. I wonder if that makes the change so complicated\nthat my time isn't well spend working on that given that I'm not used to\ngit's source code and it's better addressed by someone with deeper\nknowledge. Sounds as if we need a state signaling \"Current commit is\ndone\".\n\n> Wouldn't it map the note for `X` to rewritten `C`?\n> \n> > diff --git a/t/t3322-notes-rebase.sh b/t/t3322-notes-rebase.sh\n> > new file mode 100755\n> > index 000000000000..0eddde7f9961\n> > --- /dev/null\n> > +++ b/t/t3322-notes-rebase.sh\n> > @@ -0,0 +1,37 @@\n> > +#!/bin/sh\n> > +\n> > +test_description='Test notes on rebase'\n> > +\n> > +. ./test-lib.sh\n> > +\n> > +test_expect_success setup '\n> > +\tgit init &&\n> > +\tgit config notes.rewriteRef refs/notes/commits &&\n> > +\tgit version > version &&\n> > +\techo A > A &&\n> \n> Style. In our codebase, redirection operator sticks to the\n> redirection target without SP in between, i.e.\n> \n> \tgit version >version &&\n> \techo A >A &&\n> \n> > +\tgit notes add -m \"This is B\" @ &&\n> \n> '@' is hard to read; when you refer to HEAD, please write HEAD.\n> \n> \n> > +test_expect_success 'rebase B + C on top of BD' '\n> > +\tgit rebase @ master\n> > +'\n> > +\n> > +test_expect_success 'assert there is no note on BD' '\n> > +\tif git notes list branch >/tmp/lalaa; then return 1; fi\n> > +'\n> \n> Do not step outside of $TRASH_DIRECTORY without a good reason.\n\nOh, that is a debug thing that shouldn't have made it into the patch.\n \n> Style. In our codebase, shell scripts do not use ';' and written\n> more like\n> \n> \tif git notes list branch >notes-list\n> \tthen\n> \t\treturn 1\n> \tfi\n> \n> But more importantly, if you want to make sure the command makes a\n> controlled exit (not crash), use\n> \n> \ttest_must_fail git notes list branch\n\nAh, I really wondered if I'm missing something because it should be\neasier to say \"this command should fail\".\n\nBest regards\nUwe\n"},{"id":"545935","messageId":"67dbfb5c-5f07-49b8-aa32-a4635c585028@gmail.com","threadId":"65820","inReplyTo":"ajKimV1TDCgE-GzK@monoceros","subject":"Re: [PATCH] sequencer: Skip copying notes for commits that disappear during rebase","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-06-19T10:13:32Z","receivedAt":"2026-06-19T10:13:38Z","isPatch":true,"body":"Hi Uwe and Junio\n\nOn 17/06/2026 14:58, Uwe Kleine-König wrote:\n> \n>> It is not yet clear to me if we want to _always_ discard a note from\n>> a commit that would become \"empty\" during a rebase session (in other\n>> words, a commit that becomes empty during a rebase is _always_ a\n>> sign that the change it brings in is _already_ in the new base of\n>> the rebase\n> \n> Yeah, or in a patch that was picked before.\n> \n>> and the necessary information the note wanted to carry to\n>> the target branch is there without need to _duplicate_ it by copying\n>> the note). But assuming that we want the behaviour, the code change\n>> to sequencer.c looks very reasonable to me, except for one thing that\n>> I am not clear about.\n> \n> I think given the commit goes away, it's natural that the note goes\n> away, too. And to come back to your question above: I think it doesn't\n> need documentation, that if a commit disappears its notes go away, too.\n> But that might be subjective?!\n\nI tend to agree with this - if we're throwing away the commit message \nwithout asking the user I think it makes sense to do the same for the \nnotes. We have \"--empty=ask\" if the user does not want commits that \nbecome empty to be automatically discarded.\n\n>>> diff --git a/sequencer.c b/sequencer.c\n>>> index 57855b0066ac..da2185a37c5d 100644\n>>> --- a/sequencer.c\n>>> +++ b/sequencer.c\n>>> ...\n>>> @@ -4965,7 +4965,7 @@ static int pick_one_commit(struct repository *r,\n>>> \t\treturn error_with_patch(r, commit,\n>>> \t\t\t\t\targ, item->arg_len, opts, res, !res);\n>>> \t}\n>>> -\tif (is_rebase_i(opts) && !res)\n>>> +\tif (is_rebase_i(opts) && !res && !dropped_commit)\n>>> \t\trecord_in_rewritten(&item->commit->object.oid,\n>>> \t\t\t\t peek_command(todo_list, 1));\n>>\n>> If we have a sequence of commits where a commit that was *not*\n>> dropped is followed by a fixup commit that *is* dropped (e.g.,\n>> because it became empty/redundant), wouldn't it prevent the\n>> previously pending commit from being flushed to skip\n>> `record_in_rewritten` entirely for the dropped fixup commit?\n\nThat's a good point - we should call flush_rewritten_pending() in that \ncase. Looking at the code there are some other bugs related to dropping \ncommits either because they become empty or the user runs \"git rebase \n--skip\"\n\n - If we drop the final fixup we don't cleanup the commit message\n\n - If we drop an \"edit\" command then \"git rebase --continue\" records it\n as being rewritten HEAD so we'll copy the notes to the wrong commit\n\n - Running \"git rebase --skip\" causes the commit that had conflicts\n to also be recorded as as being rewritten to HEAD leading to the\n same issue.\n\n> Huh, sounds possible. I wonder if that makes the change so complicated\n> that my time isn't well spend working on that given that I'm not used to\n> git's source code and it's better addressed by someone with deeper\n> knowledge. Sounds as if we need a state signaling \"Current commit is\n> done\".\n\nI'm happy to take this forward and try and fix at least some of the \nother bugs I've listed above. Uwe - if I don't cc you on some patches \nwithin the next couple of weeks please feel free to send a reminder.\n\nThanks\n\nPhillip\n\n\n>> Wouldn't it map the note for `X` to rewritten `C`?\n>>\n>>> diff --git a/t/t3322-notes-rebase.sh b/t/t3322-notes-rebase.sh\n>>> new file mode 100755\n>>> index 000000000000..0eddde7f9961\n>>> --- /dev/null\n>>> +++ b/t/t3322-notes-rebase.sh\n>>> @@ -0,0 +1,37 @@\n>>> +#!/bin/sh\n>>> +\n>>> +test_description='Test notes on rebase'\n>>> +\n>>> +. ./test-lib.sh\n>>> +\n>>> +test_expect_success setup '\n>>> +\tgit init &&\n>>> +\tgit config notes.rewriteRef refs/notes/commits &&\n>>> +\tgit version > version &&\n>>> +\techo A > A &&\n>>\n>> Style. In our codebase, redirection operator sticks to the\n>> redirection target without SP in between, i.e.\n>>\n>> \tgit version >version &&\n>> \techo A >A &&\n>>\n>>> +\tgit notes add -m \"This is B\" @ &&\n>>\n>> '@' is hard to read; when you refer to HEAD, please write HEAD.\n>>\n>>\n>>> +test_expect_success 'rebase B + C on top of BD' '\n>>> +\tgit rebase @ master\n>>> +'\n>>> +\n>>> +test_expect_success 'assert there is no note on BD' '\n>>> +\tif git notes list branch >/tmp/lalaa; then return 1; fi\n>>> +'\n>>\n>> Do not step outside of $TRASH_DIRECTORY without a good reason.\n> \n> Oh, that is a debug thing that shouldn't have made it into the patch.\n> \n>> Style. In our codebase, shell scripts do not use ';' and written\n>> more like\n>>\n>> \tif git notes list branch >notes-list\n>> \tthen\n>> \t\treturn 1\n>> \tfi\n>>\n>> But more importantly, if you want to make sure the command makes a\n>> controlled exit (not crash), use\n>>\n>> \ttest_must_fail git notes list branch\n> \n> Ah, I really wondered if I'm missing something because it should be\n> easier to say \"this command should fail\".\n> \n> Best regards\n> Uwe\n\n"},{"id":"545949","messageId":"ajU9ju4jUpMCfksJ@monoceros","threadId":"65820","inReplyTo":"67dbfb5c-5f07-49b8-aa32-a4635c585028@gmail.com","subject":"Re: [PATCH] sequencer: Skip copying notes for commits that disappear during rebase","fromName":"Uwe Kleine-König","fromEmail":"u.kleine-koenig@baylibre.com","sentAt":"2026-06-19T13:01:52Z","receivedAt":"2026-06-19T13:01:58Z","isPatch":true,"body":"Hello Phillip,\n\nOn Fri, Jun 19, 2026 at 11:13:32AM +0100, Phillip Wood wrote:\n> I'm happy to take this forward and try and fix at least some of the other\n> bugs I've listed above. Uwe - if I don't cc you on some patches within the\n> next couple of weeks please feel free to send a reminder.\n\nVery appreciated! Looking forward to test your patches.\n\nBest regards\nUwe\n"},{"id":"546784","messageId":"cover.1782833268.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"67dbfb5c-5f07-49b8-aa32-a4635c585028@gmail.com","subject":"[PATCH 00/11] sequencer: do not record dropped commits as rewritten","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-06-30T15:28:50Z","receivedAt":"2026-06-30T15:29:17Z","isPatch":true,"body":"On 19/06/2026 11:13, Phillip Wood wrote:\n> I'm happy to take this forward and try and fix at least some of the\n> other bugs I've listed above. Uwe - if I don't cc you on some patches\n> within the next couple of weeks please feel free to send a reminder.\n\nHere is the first batch that fixes the same problem as Uwe's patch. I've\ntaken a slightly different approach that uses the return value from\ndo_pick_commit() to signal that a commit was dropped rather than\nadding another function argument. That involves a number of preparatory\npatches, but they are hopefully reasonably small and easy to follow.\n\nIf a commit gets dropped because its changes are already upstream\nthen we should not record it as rewritten. As well as confusing any\npost-rewrite hooks this means we end up copying the notes from the\ndropped commit to the commit that was picked immediately before the\none that was dropped.\n\nThis series is structured as follows:\n\nPatch 1 restores some test coverage that was lost when the default\nrebase backend was changed.\n\nPatch 2 moves a function so it can be called without a forward\ndeclaration in Patch 11.\n\nPatches 3 & 4 fix the return value of do_pick_commit() when an external\ncommand fails (this is in preparation for patch 10).\n\nPatches 5-9 try and simplify the control flow in pick_one_commit()\nin preparation for patch 10.\n\nPatch 10 changes the return type of do_pick_commit() to an enum.\n\nPatch 11 adds a new member to the enum from patch 10 for commits that\nare dropped when they become empty and uses that to stop them from\nbeing recorded as rewritten.\n\nBase-Commit: 6c3d7b73556db708feb3b16232fab1efc4353428\nPublished-As: https://github.com/phillipwood/git/releases/tag/pw%2Frebase-drop-notes-with-commit%2Fv1\nView-Changes-At: https://github.com/phillipwood/git/compare/6c3d7b735...26551f268\nFetch-It-Via: git fetch https://github.com/phillipwood/git pw/rebase-drop-notes-with-commit/v1\n\n\nPhillip Wood (11):\n t3400: restore coverage for note copying with apply backend\n sequencer: move definition of is_final_fixup()\n sequencer: be more careful with external merge\n sequencer: never reschedule on failed commit\n sequencer: remove unnecessary \"or\" in pick_one_commit()\n sequencer: simplify handing of fixup with conflicts\n sequencer: remove unnecessary condition in pick_one_commit()\n sequencer: simplify pick_one_commit()\n sequencer: return early from pick_one_commit() on success\n sequencer: use an enum to represent result of picking a commit\n sequencer: do not record dropped commits as rewritten\n\n sequencer.c | 154 +++++++++++++++++++++++-----------\n t/t3400-rebase.sh | 16 +++-\n t/t3404-rebase-interactive.sh | 11 +++\n t/t5407-post-rewrite-hook.sh | 23 +++++\n 4 files changed, 155 insertions(+), 49 deletions(-)\n\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"546786","messageId":"65af2ac07a2bf85336245a7d9b9f0a8a0e8affdb.1782833268.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1782833268.git.phillip.wood@dunelm.org.uk","subject":"[PATCH 01/11] t3400: restore coverage for note copying with apply backend","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-06-30T15:28:51Z","receivedAt":"2026-06-30T15:29:17Z","isPatch":true,"body":"From: Phillip Wood \n\nNow that the merge backend is the default we have lost coverage for\n\"git rebase --apply\" copying notes. Fix this by replacing \"-m\" with\n\"--apply\" as the previous test which uses the default backend now\nchecks the merge backend.\n\nSigned-off-by: Phillip Wood \n---\n t/t3400-rebase.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex c0c00fbb7b1..f0e7fcf649a 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -270,9 +270,9 @@ test_expect_success 'rebase can copy notes' '\n \ttest \"a note\" = \"$(git notes show HEAD)\"\n '\n \n-test_expect_success 'rebase -m can copy notes' '\n+test_expect_success 'rebase --apply can copy notes' '\n \tgit reset --hard n3 &&\n-\tgit rebase -m --onto n1 n2 &&\n+\tgit rebase --apply --onto n1 n2 &&\n \ttest \"a note\" = \"$(git notes show HEAD)\"\n '\n \n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"546785","messageId":"02670f57e7d81d4ff7341fecff3ef04b9fdc0102.1782833268.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1782833268.git.phillip.wood@dunelm.org.uk","subject":"[PATCH 02/11] sequencer: move definition of is_final_fixup()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-06-30T15:28:52Z","receivedAt":"2026-06-30T15:29:18Z","isPatch":true,"body":"From: Phillip Wood \n\nMove this function earlier in the file in preparation for adding a\nnew caller in a later commit.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 30 +++++++++++++++---------------\n 1 file changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 57855b0066a..32a09b6e87d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4627,21 +4627,6 @@ static int do_update_refs(struct repository *r, int quiet)\n \tstrbuf_release(&update_msg);\n \tstrbuf_release(&error_msg);\n \treturn res;\n-}\n-\n-static int is_final_fixup(struct todo_list *todo_list)\n-{\n-\tint i = todo_list->current;\n-\n-\tif (!is_fixup(todo_list->items[i].command))\n-\t\treturn 0;\n-\n-\twhile (++i < todo_list->nr)\n-\t\tif (is_fixup(todo_list->items[i].command))\n-\t\t\treturn 0;\n-\t\telse if (!is_noop(todo_list->items[i].command))\n-\t\t\tbreak;\n-\treturn 1;\n }\n \n static enum todo_command peek_command(struct todo_list *todo_list, int offset)\n@@ -4925,6 +4910,21 @@ static int reread_todo_if_changed(struct repository *r,\n \tstrbuf_release(&buf);\n \n \treturn 0;\n+}\n+\n+static int is_final_fixup(struct todo_list *todo_list)\n+{\n+\tint i = todo_list->current;\n+\n+\tif (!is_fixup(todo_list->items[i].command))\n+\t\treturn 0;\n+\n+\twhile (++i < todo_list->nr)\n+\t\tif (is_fixup(todo_list->items[i].command))\n+\t\t\treturn 0;\n+\t\telse if (!is_noop(todo_list->items[i].command))\n+\t\t\tbreak;\n+\treturn 1;\n }\n \n static const char rescheduled_advice[] =\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"546787","messageId":"16fba1e823bae633da6c4b76e239aa013fe2c6c9.1782833268.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1782833268.git.phillip.wood@dunelm.org.uk","subject":"[PATCH 03/11] sequencer: be more careful with external merge","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-06-30T15:28:53Z","receivedAt":"2026-06-30T15:29:19Z","isPatch":true,"body":"From: Phillip Wood \n\nIf an external merge strategy cannot merge (for example because it\nwould overwrite an untracked file) it exits with a non-zero exit\ncode other than 1. This should be treated differently to a merge\nwith conflicts which is signalled by an exit code of 1 because as\nthe merge failed we need to reschedule the last pick. The caller\nexpects us to return -1 in this case. Also reschedule without trying\nto merge if the commit message cannot be written as that prevents us\nfrom successfully picking the commit.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 19 +++++++++++++++----\n t/t3404-rebase-interactive.sh | 11 +++++++++++\n 2 files changed, 26 insertions(+), 4 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 32a09b6e87d..e6626c4db4e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2453,14 +2453,25 @@ static int do_pick_commit(struct repository *r,\n \t\tstruct commit_list *common = NULL;\n \t\tstruct commit_list *remotes = NULL;\n \n-\t\tres = write_message(ctx->message.buf, ctx->message.len,\n-\t\t\t\t git_path_merge_msg(r), 0);\n+\t\tif (write_message(ctx->message.buf, ctx->message.len,\n+\t\t\t\t git_path_merge_msg(r), 0)) {\n+\t\t\tres = -1;\n+\t\t\tgoto leave;\n+\t\t}\n \n \t\tcommit_list_insert(base, &common);\n \t\tcommit_list_insert(next, &remotes);\n-\t\tres |= try_merge_command(r, opts->strategy,\n-\t\t\t\t\t opts->xopts.nr, opts->xopts.v,\n+\t\tres = try_merge_command(r, opts->strategy,\n+\t\t\t\t\topts->xopts.nr, opts->xopts.v,\n \t\t\t\t\tcommon, oid_to_hex(&head), remotes);\n+\t\t/*\n+\t\t * If the there were conflicts, try_merge_command() returns 1,\n+\t\t * any other no-zero return code means that either the merge\n+\t\t * command could not be run, or it failed to merge.\n+\t\t */\n+\t\tif (res && res != 1)\n+\t\t\tres = -1;\n+\n \t\tcommit_list_free(common);\n \t\tcommit_list_free(remotes);\n \t}\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 58b3bb0c271..297b84e60d5 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1249,6 +1249,17 @@ test_expect_success 'interrupted rebase -i with --strategy and -X' '\n \tgit rebase --continue &&\n \ttest $(git show conflict-branch:conflict) = $(cat conflict) &&\n \ttest $(cat file1) = Z\n+'\n+\n+test_expect_success 'failing pick with --strategy is rescheduled' '\n+\ttest_when_finished \"rm -rf bin; test_might_fail git rebase --abort\" &&\n+\tmkdir bin &&\n+\techo exit 2 | write_script bin/git-merge-fail &&\n+\tgit log -1 --format=\"pick %H # %s\" HEAD >expect &&\n+\ttest_must_fail env PATH=\"$PWD/bin:$PATH\" \\\n+\t\tgit rebase --no-ff --strategy fail HEAD^ &&\n+\ttest_cmp expect .git/rebase-merge/git-rebase-todo &&\n+\ttest_cmp expect .git/rebase-merge/done\n '\n \n test_expect_success 'rebase -i error on commits with \\ in message' '\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"546788","messageId":"3ffd06d65096d77bdc372a13bd883a00c8cf4c82.1782833268.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1782833268.git.phillip.wood@dunelm.org.uk","subject":"[PATCH 04/11] sequencer: never reschedule on failed commit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-06-30T15:28:54Z","receivedAt":"2026-06-30T15:29:20Z","isPatch":true,"body":"From: Phillip Wood \n\nIf \"git commit\" fails to run then run_git_commit() returns -1 which\ncauses the current command to be rescheduled. This is incorrect as\nwe have successfully picked the commit and have written all the state\nfiles we need to successfully commit when the user continues. Fix this\nby converting -1 to 1 which matches what do_merge() does.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex e6626c4db4e..d7e439b1feb 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2542,6 +2542,12 @@ static int do_pick_commit(struct repository *r,\n \t\t\tres = run_git_commit(NULL, reflog_action, opts, flags);\n \t\t\t*check_todo = 1;\n \t\t}\n+\t\t/*\n+\t\t * If \"git commit\" failed to run than res == -1 but we dont\n+\t\t * want reschedule the last command because the picking the\n+\t\t * commit was successful.\n+\t\t */\n+\t\tres = !!res;\n \t}\n \n \n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"546790","messageId":"cb286ac70d77cdcbb644a34b2fa89663b3c443b9.1782833268.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1782833268.git.phillip.wood@dunelm.org.uk","subject":"[PATCH 05/11] sequencer: remove unnecessary \"or\" in pick_one_commit()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-06-30T15:28:55Z","receivedAt":"2026-06-30T15:29:21Z","isPatch":true,"body":"From: Phillip Wood \n\nIf error_with_patch(..., res, ...) succeeds then it returns \"res\", if\nit fails then it returns -1. This means that or-ing the return value\nwith \"res\" is pointless the result is the same as the return value.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex d7e439b1feb..39cbb7b6e3e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5007,9 +5007,8 @@ static int pick_one_commit(struct repository *r,\n \t\t oideq(&opts->squash_onto, &oid))))\n \t\t\tto_amend = 1;\n \n-\t\treturn res | error_with_patch(r, item->commit,\n-\t\t\t\t\t arg, item->arg_len, opts,\n-\t\t\t\t\t res, to_amend);\n+\t\treturn error_with_patch(r, item->commit, arg, item->arg_len,\n+\t\t\t\t\topts, res, to_amend);\n \t}\n \treturn res;\n }\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"546789","messageId":"1585d47e2ea72c9d70160071109ce1ed23021495.1782833268.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1782833268.git.phillip.wood@dunelm.org.uk","subject":"[PATCH 06/11] sequencer: simplify handing of fixup with conflicts","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-06-30T15:28:56Z","receivedAt":"2026-06-30T15:29:22Z","isPatch":true,"body":"From: Phillip Wood \n\nCommit e032abd5a0 (rebase: fix rewritten list for failed pick,\n2023-09-06) introduced an early return when res == -1, so if we enter\nthis conditional block then res is positive. After the last couple\nof commits the only possible positive value is 1 so we can simplify\nthe code by removing the conditional call to intend_to_amend() and\ncall it error_with_patch() instead.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 39cbb7b6e3e..bcfbda018a7 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3874,7 +3874,7 @@ static int error_failed_squash(struct repository *r,\n \t\treturn error(_(\"could not copy '%s' to '%s'\"),\n \t\t\t rebase_path_message(),\n \t\t\t git_path_merge_msg(r));\n-\treturn error_with_patch(r, commit, subject, subject_len, opts, 1, 0);\n+\treturn error_with_patch(r, commit, subject, subject_len, opts, 1, 1);\n }\n \n static int do_exec(struct repository *r, const char *command_line, int quiet)\n@@ -4986,8 +4986,6 @@ static int pick_one_commit(struct repository *r,\n \t\trecord_in_rewritten(&item->commit->object.oid,\n \t\t\t\t peek_command(todo_list, 1));\n \tif (res && is_fixup(item->command)) {\n-\t\tif (res == 1)\n-\t\t\tintend_to_amend();\n \t\treturn error_failed_squash(r, item->commit, opts,\n \t\t\t\t\t item->arg_len, arg);\n \t} else if (res && is_rebase_i(opts) && item->commit) {\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"546791","messageId":"4386ca67d1052b15d15f62b9761f716508ce276d.1782833268.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1782833268.git.phillip.wood@dunelm.org.uk","subject":"[PATCH 07/11] sequencer: remove unnecessary condition in pick_one_commit()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-06-30T15:28:57Z","receivedAt":"2026-06-30T15:29:23Z","isPatch":true,"body":"From: Phillip Wood \n\nitem->commit holds the commit to be picked and so it must be non-NULL\notherwise pick_one_commit() would not know which commit to pick.\nIt is also unconditionally dereferenced in do_pick_commit() which is\ncalled at the top of this function. Therefore the check to see if it\nis non-NULL is superfluous.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex bcfbda018a7..ff28873d21c 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4988,7 +4988,7 @@ static int pick_one_commit(struct repository *r,\n \tif (res && is_fixup(item->command)) {\n \t\treturn error_failed_squash(r, item->commit, opts,\n \t\t\t\t\t item->arg_len, arg);\n-\t} else if (res && is_rebase_i(opts) && item->commit) {\n+\t} else if (res && is_rebase_i(opts)) {\n \t\tint to_amend = 0;\n \t\tstruct object_id oid;\n \n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"546792","messageId":"f51751fa3ec1545b7304b869d91d21b055218755.1782833268.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1782833268.git.phillip.wood@dunelm.org.uk","subject":"[PATCH 08/11] sequencer: simplify pick_one_commit()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-06-30T15:28:58Z","receivedAt":"2026-06-30T15:29:24Z","isPatch":true,"body":"From: Phillip Wood \n\nUnless we're rebasing all we do in pick_one_commit() is call\ndo_pick_commit() and return its result. Simplify the code by returing\nearly if we're not rebasing so that we don't have to continually call\nis_rebase_i() in the rest of the function. Note that there are a couple\nof conditions that do not call is_rebase_i() but they check for either\nan \"edit\" or a \"fixup\" command, both of which imply we're rebasing.\n\nAs the conditional blocks are all mutually exclusive (either the\nconditions are mutually exclusive, or an earlier conditional block\nthat would match a later one contains a \"return\" statement) chain\nthem together with \"else if\" to make that clear.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 15 ++++++++-------\n 1 file changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex ff28873d21c..416729f30a7 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4966,12 +4966,14 @@ static int pick_one_commit(struct repository *r,\n \n \tres = do_pick_commit(r, item, opts, is_final_fixup(todo_list),\n \t\t\t check_todo);\n-\tif (is_rebase_i(opts) && res < 0) {\n+\tif (!is_rebase_i(opts))\n+\t\treturn res;\n+\n+\tif (res < 0) {\n \t\t/* Reschedule */\n \t\t*reschedule = 1;\n \t\treturn -1;\n-\t}\n-\tif (item->command == TODO_EDIT) {\n+\t} else if (item->command == TODO_EDIT) {\n \t\tstruct commit *commit = item->commit;\n \t\tif (!res) {\n \t\t\tif (!opts->verbose)\n@@ -4981,14 +4983,13 @@ static int pick_one_commit(struct repository *r,\n \t\t}\n \t\treturn error_with_patch(r, commit,\n \t\t\t\t\targ, item->arg_len, opts, res, !res);\n-\t}\n-\tif (is_rebase_i(opts) && !res)\n+\t} else if (!res) {\n \t\trecord_in_rewritten(&item->commit->object.oid,\n \t\t\t\t peek_command(todo_list, 1));\n-\tif (res && is_fixup(item->command)) {\n+\t} else if (res && is_fixup(item->command)) {\n \t\treturn error_failed_squash(r, item->commit, opts,\n \t\t\t\t\t item->arg_len, arg);\n-\t} else if (res && is_rebase_i(opts)) {\n+\t} else if (res) {\n \t\tint to_amend = 0;\n \t\tstruct object_id oid;\n \n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"546793","messageId":"2541a4d6e3d41272c31c8fafdf4eadcbc71b63f3.1782833268.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1782833268.git.phillip.wood@dunelm.org.uk","subject":"[PATCH 09/11] sequencer: return early from pick_one_commit() on success","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-06-30T15:28:59Z","receivedAt":"2026-06-30T15:29:24Z","isPatch":true,"body":"From: Phillip Wood \n\nThe only block that does not return early is the one guarded by\n\"!res\". Move the return into that block to make it clear that after\nrecording the commit as rewritten all we do is return from the function.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 416729f30a7..655a2e84bef 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4986,6 +4986,7 @@ static int pick_one_commit(struct repository *r,\n \t} else if (!res) {\n \t\trecord_in_rewritten(&item->commit->object.oid,\n \t\t\t\t peek_command(todo_list, 1));\n+\t\treturn 0;\n \t} else if (res && is_fixup(item->command)) {\n \t\treturn error_failed_squash(r, item->commit, opts,\n \t\t\t\t\t item->arg_len, arg);\n@@ -5009,7 +5010,8 @@ static int pick_one_commit(struct repository *r,\n \t\treturn error_with_patch(r, item->commit, arg, item->arg_len,\n \t\t\t\t\topts, res, to_amend);\n \t}\n-\treturn res;\n+\n+\tBUG(\"Unhandled return value from do_pick_commit()\");\n }\n \n static int pick_commits(struct repository *r,\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"546794","messageId":"26551f2687be0f5c1d2f503bdd50729a20b0dade.1782833268.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1782833268.git.phillip.wood@dunelm.org.uk","subject":"[PATCH 11/11] sequencer: do not record dropped commits as rewritten","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-06-30T15:29:01Z","receivedAt":"2026-06-30T15:29:26Z","isPatch":true,"body":"From: Phillip Wood \n\nIf a commit gets dropped because its changes are already upstream\nthen we should not record it as rewritten. As well as confusing any\npost-rewrite hooks this means we end up copying the notes from the\ndropped commit to the commit that was picked immediately before the\none that was dropped.\n\nWhile we do not want to record the dropped commit is rewritten, if\nit is the final commit in a chain of fixups then we need to flush\nthe list of rewritten commits. The behavior of an \"edit\" command\nwhere the commit is dropped is changed so that \"rebase --continue\"\nwill not amend the previous pick. However, as the code comment notes\nit will still be erroneously recorded as rewritten when the rebase\ncontinues. That will need to be addressed separately along with not\nrecording skipped commits as rewritten.\n\nThe initialization of \"drop_commit\" is moved to ensure it is initialized\nwhen rewording a fast-forwarded commit.\n\nReported-by: Uwe Kleine-König \nSigned-off-by: Phillip Wood \n---\n sequencer.c | 24 +++++++++++++++++++-----\n t/t3400-rebase.sh | 12 ++++++++++++\n t/t5407-post-rewrite-hook.sh | 23 +++++++++++++++++++++++\n 3 files changed, 54 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex ca005b969c4..a85f9e8b77d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2264,6 +2264,7 @@ enum pick_result {\n \tPICK_RESULT_ERROR = -1,\n \tPICK_RESULT_OK,\n \tPICK_RESULT_CONFLICTS,\n+\tPICK_RESULT_DROPPED,\n };\n \n static enum pick_result do_pick_commit(struct repository *r,\n@@ -2279,7 +2280,7 @@ static enum pick_result do_pick_commit(struct repository *r,\n \tconst char *base_label, *next_label, *reflog_action;\n \tchar *author = NULL;\n \tstruct commit_message msg = { NULL, NULL, NULL, NULL };\n-\tint res, unborn = 0, reword = 0, allow, drop_commit;\n+\tint res, unborn = 0, reword = 0, allow, drop_commit = 0;\n \tenum todo_command command = item->command;\n \tstruct commit *commit = item->commit;\n \n@@ -2509,7 +2510,6 @@ static enum pick_result do_pick_commit(struct repository *r,\n \t\tgoto leave;\n \t}\n \n-\tdrop_commit = 0;\n \tallow = allow_empty(r, opts, commit);\n \tif (allow < 0) {\n \t\tres = allow;\n@@ -2574,6 +2574,8 @@ static enum pick_result do_pick_commit(struct repository *r,\n \t\treturn PICK_RESULT_ERROR;\n \telse if (res > 0)\n \t\treturn PICK_RESULT_CONFLICTS;\n+\telse if (drop_commit)\n+\t\treturn PICK_RESULT_DROPPED;\n \telse\n \t\treturn PICK_RESULT_OK;\n }\n@@ -4994,18 +4996,30 @@ static int pick_one_commit(struct repository *r,\n \t} else if (item->command == TODO_EDIT) {\n \t\tstruct commit *commit = item->commit;\n \t\tint res = pick_res == PICK_RESULT_CONFLICTS;\n+\t\tint to_amend = pick_res != PICK_RESULT_CONFLICTS &&\n+\t\t\t\tpick_res != PICK_RESULT_DROPPED;\n \n-\t\tif (pick_res == PICK_RESULT_OK) {\n+\t\t/*\n+\t\t * NEEDSWORK: Do not record the commit as rewritten when\n+\t\t * continuing if it was dropped. Does it even make sense\n+\t\t * to stop if the commit was dropped?\n+\t\t */\n+\t\tif (pick_res == PICK_RESULT_OK ||\n+\t\t pick_res == PICK_RESULT_DROPPED) {\n \t\t\tif (!opts->verbose)\n \t\t\t\tterm_clear_line();\n \t\t\tfprintf(stderr, _(\"Stopped at %s... %.*s\\n\"),\n \t\t\t\tshort_commit_name(r, commit), item->arg_len, arg);\n \t\t}\n-\t\treturn error_with_patch(r, commit,\n-\t\t\t\t\targ, item->arg_len, opts, res, !res);\n+\t\treturn error_with_patch(r, commit, arg, item->arg_len, opts,\n+\t\t\t\t\tres, to_amend);\n \t} else if (pick_res == PICK_RESULT_OK) {\n \t\trecord_in_rewritten(&item->commit->object.oid,\n \t\t\t\t peek_command(todo_list, 1));\n+\t\treturn 0;\n+\t} else if (pick_res == PICK_RESULT_DROPPED) {\n+\t\tif (is_final_fixup(todo_list))\n+\t\t\tflush_rewritten_pending();\n \t\treturn 0;\n \t} else if (pick_res == PICK_RESULT_CONFLICTS &&\n \t\t is_fixup(item->command)) {\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex f0e7fcf649a..1d09886ea35 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -274,6 +274,18 @@ test_expect_success 'rebase --apply can copy notes' '\n \tgit reset --hard n3 &&\n \tgit rebase --apply --onto n1 n2 &&\n \ttest \"a note\" = \"$(git notes show HEAD)\"\n+'\n+\n+test_expect_success 'rebase drops notes of dropped commits' '\n+\tgit checkout n1 &&\n+\techo n3 >n3.t &&\n+\techo n4 >n4.t &&\n+\tgit add n3.t n4.t &&\n+\tgit commit -m n34 &&\n+\tgit rebase HEAD n3 &&\n+\ttest_commit_message HEAD -m n2 &&\n+\ttest_must_fail git notes list HEAD >actual &&\n+\ttest_must_be_empty actual\n '\n \n test_expect_success 'rebase commit with an ancient timestamp' '\ndiff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh\nindex ad7f8c6f002..51991956d1d 100755\n--- a/t/t5407-post-rewrite-hook.sh\n+++ b/t/t5407-post-rewrite-hook.sh\n@@ -306,6 +306,29 @@ test_expect_success 'git rebase -i (exec)' '\n \tcat >expected.data <<-EOF &&\n \t$(git rev-parse C) $(git rev-parse HEAD^)\n \t$(git rev-parse D) $(git rev-parse HEAD)\n+\tEOF\n+\tverify_hook_input\n+'\n+\n+test_expect_success 'rebase with commits that become empty' '\n+\tcat >todo <<-\\EOF &&\n+\tpick H\n+\tpick E\n+\tfixup I\n+\tfixup H\n+\tpick G\n+\tpick I\n+\tEOF\n+\t(\n+\t\tset_replace_editor todo &&\n+\t\tgit rebase -i --empty=drop A A\n+\t) &&\n+\techo rebase >expected.args &&\n+\tcat >expected.data <<-EOF &&\n+\t$(git rev-parse H) $(git rev-parse HEAD~2)\n+\t$(git rev-parse E) $(git rev-parse HEAD~1)\n+\t$(git rev-parse I) $(git rev-parse HEAD~1)\n+\t$(git rev-parse G) $(git rev-parse HEAD)\n \tEOF\n \tverify_hook_input\n '\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"546795","messageId":"e4050ead27f1e01ca72acc849fa16bd67e0d1c4b.1782833268.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1782833268.git.phillip.wood@dunelm.org.uk","subject":"[PATCH 10/11] sequencer: use an enum to represent result of picking a commit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-06-30T15:29:00Z","receivedAt":"2026-06-30T15:29:26Z","isPatch":true,"body":"From: Phillip Wood \n\nRather than using an integer where -1 is an error, 0 is success and\n1 means there were conflicts use an enum. This is clearer and lets\nus add a separate return value for commits that are dropped because\nthey become empty in the next commit.\n\nNote we continue to use \"return error(...)\" to return errors and\ntake advantage of C's lax typing of enums\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 61 +++++++++++++++++++++++++++++++++++++++--------------\n 1 file changed, 45 insertions(+), 16 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 655a2e84bef..ca005b969c4 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2260,10 +2260,16 @@ static const char *reflog_message(struct replay_opts *opts,\n \treturn buf.buf;\n }\n \n-static int do_pick_commit(struct repository *r,\n-\t\t\t struct todo_item *item,\n-\t\t\t struct replay_opts *opts,\n-\t\t\t int final_fixup, int *check_todo)\n+enum pick_result {\n+\tPICK_RESULT_ERROR = -1,\n+\tPICK_RESULT_OK,\n+\tPICK_RESULT_CONFLICTS,\n+};\n+\n+static enum pick_result do_pick_commit(struct repository *r,\n+\t\t\t\t struct todo_item *item,\n+\t\t\t\t struct replay_opts *opts,\n+\t\t\t\t int final_fixup, int *check_todo)\n {\n \tstruct replay_ctx *ctx = opts->ctx;\n \tunsigned int flags = should_edit(opts) ? EDIT_MSG : 0;\n@@ -2564,7 +2570,12 @@ static int do_pick_commit(struct repository *r,\n \tfree(author);\n \tupdate_abort_safety_file();\n \n-\treturn res;\n+\tif (res < 0)\n+\t\treturn PICK_RESULT_ERROR;\n+\telse if (res > 0)\n+\t\treturn PICK_RESULT_CONFLICTS;\n+\telse\n+\t\treturn PICK_RESULT_OK;\n }\n \n static int prepare_revs(struct replay_opts *opts)\n@@ -4960,37 +4971,47 @@ static int pick_one_commit(struct repository *r,\n \t\t\t struct replay_opts *opts,\n \t\t\t int *check_todo, int* reschedule)\n {\n-\tint res;\n+\tenum pick_result pick_res;\n \tstruct todo_item *item = todo_list->items + todo_list->current;\n \tconst char *arg = todo_item_get_arg(todo_list, item);\n \n-\tres = do_pick_commit(r, item, opts, is_final_fixup(todo_list),\n-\t\t\t check_todo);\n+\tpick_res = do_pick_commit(r, item, opts, is_final_fixup(todo_list),\n+\t\t\t\t check_todo);\n \tif (!is_rebase_i(opts))\n-\t\treturn res;\n+\t\tswitch (pick_res) {\n+\t\tcase PICK_RESULT_ERROR:\n+\t\t\treturn -1;\n+\t\tcase PICK_RESULT_CONFLICTS:\n+\t\t\treturn 1;\n+\t\tdefault:\n+\t\t\treturn 0;\n+\t\t}\n \n-\tif (res < 0) {\n+\tif (pick_res == PICK_RESULT_ERROR) {\n \t\t/* Reschedule */\n \t\t*reschedule = 1;\n \t\treturn -1;\n \t} else if (item->command == TODO_EDIT) {\n \t\tstruct commit *commit = item->commit;\n-\t\tif (!res) {\n+\t\tint res = pick_res == PICK_RESULT_CONFLICTS;\n+\n+\t\tif (pick_res == PICK_RESULT_OK) {\n \t\t\tif (!opts->verbose)\n \t\t\t\tterm_clear_line();\n \t\t\tfprintf(stderr, _(\"Stopped at %s... %.*s\\n\"),\n \t\t\t\tshort_commit_name(r, commit), item->arg_len, arg);\n \t\t}\n \t\treturn error_with_patch(r, commit,\n \t\t\t\t\targ, item->arg_len, opts, res, !res);\n-\t} else if (!res) {\n+\t} else if (pick_res == PICK_RESULT_OK) {\n \t\trecord_in_rewritten(&item->commit->object.oid,\n \t\t\t\t peek_command(todo_list, 1));\n \t\treturn 0;\n-\t} else if (res && is_fixup(item->command)) {\n+\t} else if (pick_res == PICK_RESULT_CONFLICTS &&\n+\t\t is_fixup(item->command)) {\n \t\treturn error_failed_squash(r, item->commit, opts,\n \t\t\t\t\t item->arg_len, arg);\n-\t} else if (res) {\n+\t} else if (pick_res == PICK_RESULT_CONFLICTS) {\n \t\tint to_amend = 0;\n \t\tstruct object_id oid;\n \n@@ -5008,7 +5029,7 @@ static int pick_one_commit(struct repository *r,\n \t\t\tto_amend = 1;\n \n \t\treturn error_with_patch(r, item->commit, arg, item->arg_len,\n-\t\t\t\t\topts, res, to_amend);\n+\t\t\t\t\topts, 1, to_amend);\n \t}\n \n \tBUG(\"Unhandled return value from do_pick_commit()\");\n@@ -5547,7 +5568,15 @@ static int single_pick(struct repository *r,\n \t\t\tTODO_PICK : TODO_REVERT;\n \titem.commit = cmit;\n \n-\treturn do_pick_commit(r, &item, opts, 0, &check_todo);\n+\tswitch (do_pick_commit(r, &item, opts, 0, &check_todo)) {\n+\tcase PICK_RESULT_ERROR:\n+\t\treturn -1;\n+\tcase PICK_RESULT_CONFLICTS:\n+\t\treturn 1;\n+\tdefault:\n+\t\treturn 0;\n+\t}\n+\n }\n \n int sequencer_pick_revisions(struct repository *r,\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"546808","messageId":"xmqqpl17rec3.fsf@gitster.g","threadId":"65820","inReplyTo":"cover.1782833268.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH 00/11] sequencer: do not record dropped commits as rewritten","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-30T19:57:32Z","receivedAt":"2026-06-30T19:57:34Z","isPatch":true,"body":"Phillip Wood writes:\n\n> On 19/06/2026 11:13, Phillip Wood wrote:\n>> I'm happy to take this forward and try and fix at least some of the\n>> other bugs I've listed above. Uwe - if I don't cc you on some patches\n>> within the next couple of weeks please feel free to send a reminder.\n>\n> Here is the first batch that fixes the same problem as Uwe's patch. I've\n> taken a slightly different approach that uses the return value from\n> do_pick_commit() to signal that a commit was dropped rather than\n> adding another function argument. That involves a number of preparatory\n> patches, but they are hopefully reasonably small and easy to follow.\n>\n> If a commit gets dropped because its changes are already upstream\n> then we should not record it as rewritten. As well as confusing any\n> post-rewrite hooks this means we end up copying the notes from the\n> dropped commit to the commit that was picked immediately before the\n> one that was dropped.\n>\n> This series is structured as follows:\n>\n> Patch 1 restores some test coverage that was lost when the default\n> rebase backend was changed.\n>\n> Patch 2 moves a function so it can be called without a forward\n> declaration in Patch 11.\n>\n> Patches 3 & 4 fix the return value of do_pick_commit() when an external\n> command fails (this is in preparation for patch 10).\n>\n> Patches 5-9 try and simplify the control flow in pick_one_commit()\n> in preparation for patch 10.\n>\n> Patch 10 changes the return type of do_pick_commit() to an enum.\n>\n> Patch 11 adds a new member to the enum from patch 10 for commits that\n> are dropped when they become empty and uses that to stop them from\n> being recorded as rewritten.\n>\n> Base-Commit: 6c3d7b73556db708feb3b16232fab1efc4353428\n> Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Frebase-drop-notes-with-commit%2Fv1\n> View-Changes-At: https://github.com/phillipwood/git/compare/6c3d7b735...26551f268\n> Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/rebase-drop-notes-with-commit/v1\n\nThanks.\n\nA tangent (I Cc'ed Konstantin for this), but\n\n $ b4 am -o- '' >b4am.mbx\n\nfailed to produce a usable mailbox. It somehow did not think [2/11]\nexisted. I manually examined the References and In-Reply-To headers\nof that particular message and compared them with those from other\nmessages but did not find anything suspicious X-<.\n\nI have a bunch of typofixes queued on top of these 11 patches (made\nwith \"git commit --fixup reword:\"); please double check when\nyou reroll after seeing more substantial reviews than mere typofixes,\npossibly from others.\n\nThanks.\n\n\nHere is the transcript of failed b4 am invocation.\n---- >8 ----\nLooking up https://lore.kernel.org/all/cover.1782833268.git.phillip.wood@dunelm.org.uk/\nGrabbing thread from lore.kernel.org/all/cover.1782833268.git.phillip.wood@dunelm.org.uk/t.mbox.gz\nAnalyzing 17 messages in the thread\nWARNING: duplicate messages found at index 1\n Subject 1: sequencer: Skip copying notes for commits that disappear during rebase\n Subject 2: t3400: restore coverage for note copying with apply backend\n 2 is not a reply... assume additional patch\nLooking for additional code-review trailers on lore.kernel.org\nAnalyzing 0 code-review messages\nChecking attestation on all messages, may take a moment...\n---\n ✗ [PATCH] sequencer: Skip copying notes for commits that disappear during rebase\n ✗ No key: openpgp/u.kleine-koenig@baylibre.com\n ✗ BADSIG: DKIM/baylibre.com\n ✓ [PATCH 1/11] t3400: restore coverage for note copying with apply backend\n ✓ Signed: DKIM/gmail.com\n ✓ [PATCH 3/11] sequencer: be more careful with external merge\n ✓ Signed: DKIM/gmail.com\n ✓ [PATCH 4/11] sequencer: never reschedule on failed commit\n ✓ Signed: DKIM/gmail.com\n ✓ [PATCH 5/11] sequencer: remove unnecessary \"or\" in pick_one_commit()\n ✓ Signed: DKIM/gmail.com\n ✓ [PATCH 6/11] sequencer: simplify handing of fixup with conflicts\n ✓ Signed: DKIM/gmail.com\n ✓ [PATCH 7/11] sequencer: remove unnecessary condition in pick_one_commit()\n ✓ Signed: DKIM/gmail.com\n ✓ [PATCH 8/11] sequencer: simplify pick_one_commit()\n ✓ Signed: DKIM/gmail.com\n ✓ [PATCH 9/11] sequencer: return early from pick_one_commit() on success\n ✓ Signed: DKIM/gmail.com\n ✓ [PATCH 10/11] sequencer: use an enum to represent result of picking a commit\n ✓ Signed: DKIM/gmail.com\n ✓ [PATCH 11/11] sequencer: do not record dropped commits as rewritten\n ✓ Signed: DKIM/gmail.com\n ERROR: missing [12/2]!\n---\nTotal patches: 11\n---\nWARNING: Thread incomplete!\n Link: https://patch.msgid.link/cover.1782833268.git.phillip.wood@dunelm.org.uk\n:\n"},{"id":"546817","messageId":"akSqjIzdvsjK0yoM@monoceros","threadId":"65820","inReplyTo":"xmqqpl17rec3.fsf@gitster.g","subject":"Re: [PATCH 00/11] sequencer: do not record dropped commits as rewritten","fromName":"Uwe Kleine-König","fromEmail":"u.kleine-koenig@baylibre.com","sentAt":"2026-07-01T06:00:48Z","receivedAt":"2026-07-01T06:00:53Z","isPatch":true,"body":"Hello,\n\nOn Tue, Jun 30, 2026 at 12:57:32PM -0700, Junio C Hamano wrote:\n> A tangent (I Cc'ed Konstantin for this), but\n> \n> $ b4 am -o- '' >b4am.mbx\n> \n> failed to produce a usable mailbox. It somehow did not think [2/11]\n> existed.\n\nFTR: The mail is on lore.kernel.org.\n\nAlso to yield a usable mailbox my patch shouldn't be included.\n\n> I manually examined the References and In-Reply-To headers\n> of that particular message and compared them with those from other\n> messages but did not find anything suspicious X-<.\n\n\n> \n> I have a bunch of typofixes queued on top of these 11 patches (made\n> with \"git commit --fixup reword:\"); please double check when\n> you reroll after seeing more substantial reviews than mere typofixes,\n> possibly from others.\n> \n> Thanks.\n> \n> \n> Here is the transcript of failed b4 am invocation.\n> ---- >8 ----\n> Looking up https://lore.kernel.org/all/cover.1782833268.git.phillip.wood@dunelm.org.uk/\n> Grabbing thread from lore.kernel.org/all/cover.1782833268.git.phillip.wood@dunelm.org.uk/t.mbox.gz\n> Analyzing 17 messages in the thread\n> WARNING: duplicate messages found at index 1\n> Subject 1: sequencer: Skip copying notes for commits that disappear during rebase\n> Subject 2: t3400: restore coverage for note copying with apply backend\n> 2 is not a reply... assume additional patch\n\nI think here is the origin of the problem. It guesses that the t3400\nshould be added, and it takes the place of Phillip's second patch.\n\n> ERROR: missing [12/2]!\n\nThis is irritating, I would have expected \"[2/12]\" here?\n\n\tb4 am --no-parent cover.1782833268.git.phillip.wood@dunelm.org.uk\n\nworks fine for me.\n\nBest regards\nUwe\n"},{"id":"546865","messageId":"akSuP-IWiH2wPd6S@monoceros","threadId":"65820","inReplyTo":"cover.1782833268.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH 00/11] sequencer: do not record dropped commits as rewritten","fromName":"Uwe Kleine-König","fromEmail":"u.kleine-koenig@baylibre.com","sentAt":"2026-07-01T09:38:25Z","receivedAt":"2026-07-01T09:38:28Z","isPatch":true,"body":"Hello Phillip,\n\nthanks a lot for addressing this, very appreciated!\n\nOn Tue, Jun 30, 2026 at 04:28:50PM +0100, Phillip Wood wrote:\n> On 19/06/2026 11:13, Phillip Wood wrote:\n> > I'm happy to take this forward and try and fix at least some of the\n> > other bugs I've listed above. Uwe - if I don't cc you on some patches\n> > within the next couple of weeks please feel free to send a reminder.\n> \n> Here is the first batch that fixes the same problem as Uwe's patch. I've\n> taken a slightly different approach that uses the return value from\n> do_pick_commit() to signal that a commit was dropped rather than\n> adding another function argument. That involves a number of preparatory\n> patches, but they are hopefully reasonably small and easy to follow.\n> \n> If a commit gets dropped because its changes are already upstream\n> then we should not record it as rewritten. As well as confusing any\n> post-rewrite hooks this means we end up copying the notes from the\n> dropped commit to the commit that was picked immediately before the\n> one that was dropped.\n> \n> This series is structured as follows:\n> \n> Patch 1 restores some test coverage that was lost when the default\n> rebase backend was changed.\n> \n> Patch 2 moves a function so it can be called without a forward\n> declaration in Patch 11.\n> \n> Patches 3 & 4 fix the return value of do_pick_commit() when an external\n> command fails (this is in preparation for patch 10).\n> \n> Patches 5-9 try and simplify the control flow in pick_one_commit()\n> in preparation for patch 10.\n> \n> Patch 10 changes the return type of do_pick_commit() to an enum.\n> \n> Patch 11 adds a new member to the enum from patch 10 for commits that\n> are dropped when they become empty and uses that to stop them from\n> being recorded as rewritten.\n\nWith my very little knowledge about git internals, this looks\nreasonable, and it behaves as I expect in my test case. I installed a\nlocal \n\nTested-by: Uwe Kleine-König \n\n> Base-Commit: 6c3d7b73556db708feb3b16232fab1efc4353428\n\nBTW, b4 didn't pick this up, for me it says:\n\n\tBase: not specified\n\n(and I applied it on top of 2.55.0).\n\nBest regards\nUwe\n"},{"id":"546898","messageId":"822d2b16-8275-480b-9fed-9f9c5cbf09dc@gmail.com","threadId":"65820","inReplyTo":"akSqjIzdvsjK0yoM@monoceros","subject":"Re: [PATCH 00/11] sequencer: do not record dropped commits as rewritten","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-01T13:29:35Z","receivedAt":"2026-07-01T13:29:39Z","isPatch":true,"body":"On 01/07/2026 07:00, Uwe Kleine-König wrote:\n> Hello,\n> \n> On Tue, Jun 30, 2026 at 12:57:32PM -0700, Junio C Hamano wrote:\n>> A tangent (I Cc'ed Konstantin for this), but\n>>\n>> $ b4 am -o- '' >b4am.mbx\n>>\n>> failed to produce a usable mailbox. It somehow did not think [2/11]\n>> existed.\n> \n> FTR: The mail is on lore.kernel.org.\n> \n> Also to yield a usable mailbox my patch shouldn't be included.\n\nSorry I had intended to send these as v2 to avoid any confusion, but I \nforgot about that when I actually came to send them.\n\nThanks\n\nPhillip\n\n>> I manually examined the References and In-Reply-To headers\n>> of that particular message and compared them with those from other\n>> messages but did not find anything suspicious X-<.\n> \n> \n>>\n>> I have a bunch of typofixes queued on top of these 11 patches (made\n>> with \"git commit --fixup reword:\"); please double check when\n>> you reroll after seeing more substantial reviews than mere typofixes,\n>> possibly from others.\n>>\n>> Thanks.\n>>\n>>\n>> Here is the transcript of failed b4 am invocation.\n>> ---- >8 ----\n>> Looking up https://lore.kernel.org/all/cover.1782833268.git.phillip.wood@dunelm.org.uk/\n>> Grabbing thread from lore.kernel.org/all/cover.1782833268.git.phillip.wood@dunelm.org.uk/t.mbox.gz\n>> Analyzing 17 messages in the thread\n>> WARNING: duplicate messages found at index 1\n>> Subject 1: sequencer: Skip copying notes for commits that disappear during rebase\n>> Subject 2: t3400: restore coverage for note copying with apply backend\n>> 2 is not a reply... assume additional patch\n> \n> I think here is the origin of the problem. It guesses that the t3400\n> should be added, and it takes the place of Phillip's second patch.\n> \n>> ERROR: missing [12/2]!\n> \n> This is irritating, I would have expected \"[2/12]\" here?\n> \n> \tb4 am --no-parent cover.1782833268.git.phillip.wood@dunelm.org.uk\n> \n> works fine for me.\n> \n> Best regards\n> Uwe\n\n"},{"id":"546899","messageId":"dce74d17-eefd-40bb-82f3-f6b3179cc2b6@gmail.com","threadId":"65820","inReplyTo":"xmqqpl17rec3.fsf@gitster.g","subject":"Re: [PATCH 00/11] sequencer: do not record dropped commits as rewritten","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-01T13:31:04Z","receivedAt":"2026-07-01T13:31:08Z","isPatch":true,"body":"Hi Junio\n\nOn 30/06/2026 20:57, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n> I have a bunch of typofixes queued on top of these 11 patches (made\n> with \"git commit --fixup reword:\"); please double check when\n> you reroll after seeing more substantial reviews than mere typofixes,\n> possibly from others.\n\nThanks, I'll squash those locally and wait before resending\n\nPhillip\n\n> Thanks.\n> \n> \n> Here is the transcript of failed b4 am invocation.\n> ---- >8 ----\n> Looking up https://lore.kernel.org/all/cover.1782833268.git.phillip.wood@dunelm.org.uk/\n> Grabbing thread from lore.kernel.org/all/cover.1782833268.git.phillip.wood@dunelm.org.uk/t.mbox.gz\n> Analyzing 17 messages in the thread\n> WARNING: duplicate messages found at index 1\n> Subject 1: sequencer: Skip copying notes for commits that disappear during rebase\n> Subject 2: t3400: restore coverage for note copying with apply backend\n> 2 is not a reply... assume additional patch\n> Looking for additional code-review trailers on lore.kernel.org\n> Analyzing 0 code-review messages\n> Checking attestation on all messages, may take a moment...\n> ---\n> ✗ [PATCH] sequencer: Skip copying notes for commits that disappear during rebase\n> ✗ No key: openpgp/u.kleine-koenig@baylibre.com\n> ✗ BADSIG: DKIM/baylibre.com\n> ✓ [PATCH 1/11] t3400: restore coverage for note copying with apply backend\n> ✓ Signed: DKIM/gmail.com\n> ✓ [PATCH 3/11] sequencer: be more careful with external merge\n> ✓ Signed: DKIM/gmail.com\n> ✓ [PATCH 4/11] sequencer: never reschedule on failed commit\n> ✓ Signed: DKIM/gmail.com\n> ✓ [PATCH 5/11] sequencer: remove unnecessary \"or\" in pick_one_commit()\n> ✓ Signed: DKIM/gmail.com\n> ✓ [PATCH 6/11] sequencer: simplify handing of fixup with conflicts\n> ✓ Signed: DKIM/gmail.com\n> ✓ [PATCH 7/11] sequencer: remove unnecessary condition in pick_one_commit()\n> ✓ Signed: DKIM/gmail.com\n> ✓ [PATCH 8/11] sequencer: simplify pick_one_commit()\n> ✓ Signed: DKIM/gmail.com\n> ✓ [PATCH 9/11] sequencer: return early from pick_one_commit() on success\n> ✓ Signed: DKIM/gmail.com\n> ✓ [PATCH 10/11] sequencer: use an enum to represent result of picking a commit\n> ✓ Signed: DKIM/gmail.com\n> ✓ [PATCH 11/11] sequencer: do not record dropped commits as rewritten\n> ✓ Signed: DKIM/gmail.com\n> ERROR: missing [12/2]!\n> ---\n> Total patches: 11\n> ---\n> WARNING: Thread incomplete!\n> Link: https://patch.msgid.link/cover.1782833268.git.phillip.wood@dunelm.org.uk\n> :\n> \n\n"},{"id":"546900","messageId":"3c397d49-58cb-4a2e-b187-04840bbe75b9@gmail.com","threadId":"65820","inReplyTo":"akSuP-IWiH2wPd6S@monoceros","subject":"Re: [PATCH 00/11] sequencer: do not record dropped commits as rewritten","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-01T13:37:43Z","receivedAt":"2026-07-01T13:37:47Z","isPatch":true,"body":"Hi Uwe\n\nOn 01/07/2026 10:38, Uwe Kleine-König wrote:\n> \n> With my very little knowledge about git internals, this looks\n> reasonable, and it behaves as I expect in my test case. I installed a\n> local\n> \n> Tested-by: Uwe Kleine-König \n\nThanks for testing these patches\n\n>> Base-Commit: 6c3d7b73556db708feb3b16232fab1efc4353428\n> \n> BTW, b4 didn't pick this up, for me it says:\n> \n> \tBase: not specified\n\nOh, my script generates the same trailers as GitGitGadget but I see \"git \nformat-patch\" uses \"base-commit:\" I wonder if b4 expects it to be all \nlower case.\n\nThanks\n\nPhillip\n> (and I applied it on top of 2.55.0).\n> \n> Best regards\n> Uwe\n\n"},{"id":"547212","messageId":"akuMQ45aQejRcQ_Y@ugly.lan","threadId":"65820","inReplyTo":"f51751fa3ec1545b7304b869d91d21b055218755.1782833268.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH 08/11] sequencer: simplify pick_one_commit()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2026-07-06T11:06:43Z","receivedAt":"2026-07-06T11:06:51Z","isPatch":true,"body":"On Tue, Jun 30, 2026 at 04:28:58PM +0100, Phillip Wood wrote:\n>+++ b/sequencer.c\n>@@ -4981,14 +4983,13 @@ static int pick_one_commit(struct repository *r,\n> \t\t}\n> \t\treturn error_with_patch(r, commit,\n> \t\t\t\t\targ, item->arg_len, opts, res, !res);\n>-\t}\n>-\tif (is_rebase_i(opts) && !res)\n>+\t} else if (!res) {\n>\nbecause of this ...\n\n> \t\trecord_in_rewritten(&item->commit->object.oid,\n> \t\t\t\t peek_command(todo_list, 1));\n>-\tif (res && is_fixup(item->command)) {\n>+\t} else if (res && is_fixup(item->command)) {\n>\n.. the res conditional is pointless here.\n\n> \t\treturn error_failed_squash(r, item->commit, opts,\n> \t\t\t\t\t item->arg_len, arg);\n>-\t} else if (res && is_rebase_i(opts)) {\n>+\t} else if (res) {\n>\nand here as well.\n\n> \t\tint to_amend = 0;\n> \t\tstruct object_id oid;\n> \n"},{"id":"547213","messageId":"akuMrp3W1bG6d43D@ugly.lan","threadId":"65820","inReplyTo":"2541a4d6e3d41272c31c8fafdf4eadcbc71b63f3.1782833268.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH 09/11] sequencer: return early from pick_one_commit() on success","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2026-07-06T11:08:30Z","receivedAt":"2026-07-06T11:08:32Z","isPatch":true,"body":"On Tue, Jun 30, 2026 at 04:28:59PM +0100, Phillip Wood wrote:\n>The only block that does not return early is the one guarded by\n>\"!res\". Move the return into that block to make it clear that after\n>recording the commit as rewritten all we do is return from the function.\n>\ni think it would be much more logical to just squash that into the \nparent commit.\n\n"},{"id":"547214","messageId":"akuNmMFST8W2H2Ru@ugly.lan","threadId":"65820","inReplyTo":"e4050ead27f1e01ca72acc849fa16bd67e0d1c4b.1782833268.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH 10/11] sequencer: use an enum to represent result of picking a commit","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2026-07-06T11:12:24Z","receivedAt":"2026-07-06T11:12:29Z","isPatch":true,"body":"On Tue, Jun 30, 2026 at 04:29:00PM +0100, Phillip Wood wrote:\n>Rather than using an integer where -1 is an error, 0 is success and\n>1 means there were conflicts use an enum. This is clearer and lets\n>us add a separate return value for commits that are dropped because\n>they become empty in the next commit.\n>\nhave you attempted widening the scope of the enum? the three conversions \nbetween the new enum and existing int return values irk me.\n\n"},{"id":"547229","messageId":"9771702d-49c4-496a-a77f-22c244acc443@gmail.com","threadId":"65820","inReplyTo":"akuNmMFST8W2H2Ru@ugly.lan","subject":"Re: [PATCH 10/11] sequencer: use an enum to represent result of picking a commit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-06T13:39:23Z","receivedAt":"2026-07-06T13:39:32Z","isPatch":true,"body":"On 06/07/2026 12:12, Oswald Buddenhagen wrote:\n> On Tue, Jun 30, 2026 at 04:29:00PM +0100, Phillip Wood wrote:\n>> Rather than using an integer where -1 is an error, 0 is success and\n>> 1 means there were conflicts use an enum. This is clearer and lets\n>> us add a separate return value for commits that are dropped because\n>> they become empty in the next commit.\n>>\n> have you attempted widening the scope of the enum? the three conversions \n> between the new enum and existing int return values irk me.\n\nI know what you mean, but how wide should be go? Using the enum just one \nlevel up the call chain means converting a whole load of functions which \ncreates a lot of churn that someone needs to review. I decided to keep \nthe enum limited to this scope for now to avoid that.\n\nThanks\n\nPhillip\n\n"},{"id":"547230","messageId":"5d112698-75be-4b44-a3d9-8b6ecb4924de@gmail.com","threadId":"65820","inReplyTo":"akuMQ45aQejRcQ_Y@ugly.lan","subject":"Re: [PATCH 08/11] sequencer: simplify pick_one_commit()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-06T13:40:07Z","receivedAt":"2026-07-06T13:40:16Z","isPatch":true,"body":"\n\nOn 06/07/2026 12:06, Oswald Buddenhagen wrote:\n> On Tue, Jun 30, 2026 at 04:28:58PM +0100, Phillip Wood wrote:\n>> +++ b/sequencer.c\n>> @@ -4981,14 +4983,13 @@ static int pick_one_commit(struct repository *r,\n>>         }\n>>         return error_with_patch(r, commit,\n>>                     arg, item->arg_len, opts, res, !res);\n>> -    }\n>> -    if (is_rebase_i(opts) && !res)\n>> +    } else if (!res) {\n>>\n> because of this ...\n> \n>>         record_in_rewritten(&item->commit->object.oid,\n>>                     peek_command(todo_list, 1));\n>> -    if (res && is_fixup(item->command)) {\n>> +    } else if (res && is_fixup(item->command)) {\n>>\n> .. the res conditional is pointless here.\n> \n>>         return error_failed_squash(r, item->commit, opts,\n>>                        item->arg_len, arg);\n>> -    } else if (res && is_rebase_i(opts)) {\n>> +    } else if (res) {\n>>\n> and here as well.\n\nI meant to add a comment about that to the commit message. I \ndeliberately left them alone so that when we convert them to use the \nenum it is clear that these arms are handling cases with conflicts.\n\nThanks\n\nPhillip\n\n"},{"id":"547922","messageId":"xmqqech73g90.fsf@gitster.g","threadId":"65820","inReplyTo":"dce74d17-eefd-40bb-82f3-f6b3179cc2b6@gmail.com","subject":"Re: [PATCH 00/11] sequencer: do not record dropped commits as rewritten","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-13T00:06:19Z","receivedAt":"2026-07-13T00:06:22Z","isPatch":true,"body":"Phillip Wood writes:\n\n> Hi Junio\n>\n> On 30/06/2026 20:57, Junio C Hamano wrote:\n>> Phillip Wood writes:\n>> \n>> I have a bunch of typofixes queued on top of these 11 patches (made\n>> with \"git commit --fixup reword:\"); please double check when\n>> you reroll after seeing more substantial reviews than mere typofixes,\n>> possibly from others.\n>\n> Thanks, I'll squash those locally and wait before resending\n>\n> Phillip\n\nThanks.\n\nJust responding belatedly as I was scanning topics that are marked\nas \"Expecting a reroll\" in my draft copy of the \"What's cooking\"\nreport that I work from.\n"},{"id":"547977","messageId":"cover.1783948637.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1782833268.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 00/10] sequencer: do not record dropped commits as rewritten","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-13T13:17:17Z","receivedAt":"2026-07-13T13:17:44Z","isPatch":true,"body":"Thanks to everyone who commented on v1. I've squashed the fixups that\nJunio had in \"seen\", squashed patches 8 & 9 together as suggested by\nOswald and expanded the commit message, and added Uwe's Tested-by:\ntrailer to the final patch. Oswald suggested extended the use of the\nenum which I think is a good idea in the long-term but I punted on\nthat for now because I think it would be fairly invasive and this\nseries has enough refactoring in it already.\n\nIf a commit gets dropped because its changes are already upstream\nthen we should not record it as rewritten. As well as confusing any\npost-rewrite hooks this means we end up copying the notes from the\ndropped commit to the commit that was picked immediately before the\none that was dropped.\n\nThis series is structured as follows:\n\nPatch 1 restores some test coverage that was lost when the default\nrebase backend was changed.\n\nPatch 2 moves a function so it can be called without a forward\ndeclaration in Patch 11.\n\nPatches 3 & 4 fix the return value of do_pick_commit() when an external\ncommand fails (this is in preparation for patch 9).\n\nPatches 5-8 try and simplify the control flow in pick_one_commit()\nin preparation for patch 9.\n\nPatch 9 changes the return type of do_pick_commit() to an enum.\n\nPatch 10 adds a new member to the enum from patch 9 for commits that\nare dropped when they become empty and uses that to stop them from\nbeing recorded as rewritten.\n\nbase-commit: 6c3d7b73556db708feb3b16232fab1efc4353428\nPublished-As: https://github.com/phillipwood/git/releases/tag/pw%2Frebase-drop-notes-with-commit%2Fv2\nView-Changes-At: https://github.com/phillipwood/git/compare/6c3d7b735...c89234dd9\nFetch-It-Via: git fetch https://github.com/phillipwood/git pw/rebase-drop-notes-with-commit/v2\n\n\nPhillip Wood (10):\n t3400: restore coverage for note copying with apply backend\n sequencer: move definition of is_final_fixup()\n sequencer: be more careful with external merge\n sequencer: never reschedule on failed commit\n sequencer: remove unnecessary \"or\" in pick_one_commit()\n sequencer: simplify handing of fixup with conflicts\n sequencer: remove unnecessary condition in pick_one_commit()\n sequencer: simplify pick_one_commit()\n sequencer: use an enum to represent result of picking a commit\n sequencer: do not record dropped commits as rewritten\n\n sequencer.c | 154 +++++++++++++++++++++++-----------\n t/t3400-rebase.sh | 16 +++-\n t/t3404-rebase-interactive.sh | 11 +++\n t/t5407-post-rewrite-hook.sh | 23 +++++\n 4 files changed, 155 insertions(+), 49 deletions(-)\n\nRange-diff against v1:\n 1: 65af2ac07a2 = 1: 65af2ac07a2 t3400: restore coverage for note copying with apply backend\n 2: 02670f57e7d = 2: 02670f57e7d sequencer: move definition of is_final_fixup()\n 3: 16fba1e823b ! 3: 3d79362332c sequencer: be more careful with external merge\n @@ sequencer.c: static int do_pick_commit(struct repository *r,\n +\t\t\t\t\topts->xopts.nr, opts->xopts.v,\n \t\t\t\t\tcommon, oid_to_hex(&head), remotes);\n +\t\t/*\n -+\t\t * If the there were conflicts, try_merge_command() returns 1,\n ++\t\t * If there were conflicts, try_merge_command() returns 1,\n +\t\t * any other no-zero return code means that either the merge\n +\t\t * command could not be run, or it failed to merge.\n +\t\t */\n 4: 3ffd06d6509 ! 4: fc89e77c6e8 sequencer: never reschedule on failed commit\n @@ sequencer.c: static int do_pick_commit(struct repository *r,\n \t\t\t*check_todo = 1;\n \t\t}\n +\t\t/*\n -+\t\t * If \"git commit\" failed to run than res == -1 but we dont\n ++\t\t * If \"git commit\" failed to run then res == -1, but we don't\n +\t\t * want reschedule the last command because the picking the\n +\t\t * commit was successful.\n +\t\t */\n 5: cb286ac70d7 ! 5: 26eef6c0958 sequencer: remove unnecessary \"or\" in pick_one_commit()\n @@ Commit message\n \n If error_with_patch(..., res, ...) succeeds then it returns \"res\", if\n it fails then it returns -1. This means that or-ing the return value\n - with \"res\" is pointless the result is the same as the return value.\n + with \"res\" is pointless as the result is the same as the return value.\n \n Signed-off-by: Phillip Wood \n \n 6: 1585d47e2ea = 6: 26dc48951ce sequencer: simplify handing of fixup with conflicts\n 7: 4386ca67d10 = 7: 71ed717d322 sequencer: remove unnecessary condition in pick_one_commit()\n 8: f51751fa3ec ! 8: e8b7fa4c59e sequencer: simplify pick_one_commit()\n @@ Commit message\n sequencer: simplify pick_one_commit()\n \n Unless we're rebasing all we do in pick_one_commit() is call\n - do_pick_commit() and return its result. Simplify the code by returing\n + do_pick_commit() and return its result. Simplify the code by returning\n early if we're not rebasing so that we don't have to continually call\n is_rebase_i() in the rest of the function. Note that there are a couple\n of conditions that do not call is_rebase_i() but they check for either\n an \"edit\" or a \"fixup\" command, both of which imply we're rebasing.\n +\n + The only block that does not return early is the one guarded by\n + \"!res\". Move the return into that block to make it clear that after\n + recording the commit as rewritten all we do is return from the function.\n \n As the conditional blocks are all mutually exclusive (either the\n conditions are mutually exclusive, or an earlier conditional block\n that would match a later one contains a \"return\" statement) chain\n them together with \"else if\" to make that clear.\n +\n + While we could remove \"res\" from the conditions below \"if (!res)\"\n + they are left alone because, when we start using an enum in the next\n + commit, it makes it clear that these clauses are handling cases where\n + there are conflicts.\n \n Signed-off-by: Phillip Wood \n \n @@ sequencer.c: static int pick_one_commit(struct repository *r,\n \t\trecord_in_rewritten(&item->commit->object.oid,\n \t\t\t\t peek_command(todo_list, 1));\n -\tif (res && is_fixup(item->command)) {\n ++\t\treturn 0;\n +\t} else if (res && is_fixup(item->command)) {\n \t\treturn error_failed_squash(r, item->commit, opts,\n \t\t\t\t\t item->arg_len, arg);\n @@ sequencer.c: static int pick_one_commit(struct repository *r,\n \t\tint to_amend = 0;\n \t\tstruct object_id oid;\n \n +@@ sequencer.c: static int pick_one_commit(struct repository *r,\n + \t\treturn error_with_patch(r, item->commit, arg, item->arg_len,\n + \t\t\t\t\topts, res, to_amend);\n + \t}\n +-\treturn res;\n ++\n ++\tBUG(\"Unhandled return value from do_pick_commit()\");\n + }\n + \n + static int pick_commits(struct repository *r,\n 9: 2541a4d6e3d < -: ----------- sequencer: return early from pick_one_commit() on success\n10: e4050ead27f = 9: 4fb641afb3c sequencer: use an enum to represent result of picking a commit\n11: 26551f2687b ! 10: c89234dd949 sequencer: do not record dropped commits as rewritten\n @@ Commit message\n when rewording a fast-forwarded commit.\n \n Reported-by: Uwe Kleine-König \n + Tested-by: Uwe Kleine-König \n Signed-off-by: Phillip Wood \n \n ## sequencer.c ##\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"547978","messageId":"65af2ac07a2bf85336245a7d9b9f0a8a0e8affdb.1783948637.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1783948637.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 01/10] t3400: restore coverage for note copying with apply backend","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-13T13:17:18Z","receivedAt":"2026-07-13T13:17:45Z","isPatch":true,"body":"From: Phillip Wood \n\nNow that the merge backend is the default we have lost coverage for\n\"git rebase --apply\" copying notes. Fix this by replacing \"-m\" with\n\"--apply\" as the previous test which uses the default backend now\nchecks the merge backend.\n\nSigned-off-by: Phillip Wood \n---\n t/t3400-rebase.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex c0c00fbb7b1..f0e7fcf649a 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -270,9 +270,9 @@ test_expect_success 'rebase can copy notes' '\n \ttest \"a note\" = \"$(git notes show HEAD)\"\n '\n \n-test_expect_success 'rebase -m can copy notes' '\n+test_expect_success 'rebase --apply can copy notes' '\n \tgit reset --hard n3 &&\n-\tgit rebase -m --onto n1 n2 &&\n+\tgit rebase --apply --onto n1 n2 &&\n \ttest \"a note\" = \"$(git notes show HEAD)\"\n '\n \n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"547979","messageId":"02670f57e7d81d4ff7341fecff3ef04b9fdc0102.1783948637.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1783948637.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 02/10] sequencer: move definition of is_final_fixup()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-13T13:17:19Z","receivedAt":"2026-07-13T13:17:46Z","isPatch":true,"body":"From: Phillip Wood \n\nMove this function earlier in the file in preparation for adding a\nnew caller in a later commit.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 30 +++++++++++++++---------------\n 1 file changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 57855b0066a..32a09b6e87d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4627,21 +4627,6 @@ static int do_update_refs(struct repository *r, int quiet)\n \tstrbuf_release(&update_msg);\n \tstrbuf_release(&error_msg);\n \treturn res;\n-}\n-\n-static int is_final_fixup(struct todo_list *todo_list)\n-{\n-\tint i = todo_list->current;\n-\n-\tif (!is_fixup(todo_list->items[i].command))\n-\t\treturn 0;\n-\n-\twhile (++i < todo_list->nr)\n-\t\tif (is_fixup(todo_list->items[i].command))\n-\t\t\treturn 0;\n-\t\telse if (!is_noop(todo_list->items[i].command))\n-\t\t\tbreak;\n-\treturn 1;\n }\n \n static enum todo_command peek_command(struct todo_list *todo_list, int offset)\n@@ -4925,6 +4910,21 @@ static int reread_todo_if_changed(struct repository *r,\n \tstrbuf_release(&buf);\n \n \treturn 0;\n+}\n+\n+static int is_final_fixup(struct todo_list *todo_list)\n+{\n+\tint i = todo_list->current;\n+\n+\tif (!is_fixup(todo_list->items[i].command))\n+\t\treturn 0;\n+\n+\twhile (++i < todo_list->nr)\n+\t\tif (is_fixup(todo_list->items[i].command))\n+\t\t\treturn 0;\n+\t\telse if (!is_noop(todo_list->items[i].command))\n+\t\t\tbreak;\n+\treturn 1;\n }\n \n static const char rescheduled_advice[] =\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"547980","messageId":"3d79362332c1208eed1fb7f8b0d431ee92fe45c5.1783948637.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1783948637.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 03/10] sequencer: be more careful with external merge","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-13T13:17:20Z","receivedAt":"2026-07-13T13:17:46Z","isPatch":true,"body":"From: Phillip Wood \n\nIf an external merge strategy cannot merge (for example because it\nwould overwrite an untracked file) it exits with a non-zero exit\ncode other than 1. This should be treated differently to a merge\nwith conflicts which is signalled by an exit code of 1 because as\nthe merge failed we need to reschedule the last pick. The caller\nexpects us to return -1 in this case. Also reschedule without trying\nto merge if the commit message cannot be written as that prevents us\nfrom successfully picking the commit.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 19 +++++++++++++++----\n t/t3404-rebase-interactive.sh | 11 +++++++++++\n 2 files changed, 26 insertions(+), 4 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 32a09b6e87d..21dd5ec9799 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2453,14 +2453,25 @@ static int do_pick_commit(struct repository *r,\n \t\tstruct commit_list *common = NULL;\n \t\tstruct commit_list *remotes = NULL;\n \n-\t\tres = write_message(ctx->message.buf, ctx->message.len,\n-\t\t\t\t git_path_merge_msg(r), 0);\n+\t\tif (write_message(ctx->message.buf, ctx->message.len,\n+\t\t\t\t git_path_merge_msg(r), 0)) {\n+\t\t\tres = -1;\n+\t\t\tgoto leave;\n+\t\t}\n \n \t\tcommit_list_insert(base, &common);\n \t\tcommit_list_insert(next, &remotes);\n-\t\tres |= try_merge_command(r, opts->strategy,\n-\t\t\t\t\t opts->xopts.nr, opts->xopts.v,\n+\t\tres = try_merge_command(r, opts->strategy,\n+\t\t\t\t\topts->xopts.nr, opts->xopts.v,\n \t\t\t\t\tcommon, oid_to_hex(&head), remotes);\n+\t\t/*\n+\t\t * If there were conflicts, try_merge_command() returns 1,\n+\t\t * any other no-zero return code means that either the merge\n+\t\t * command could not be run, or it failed to merge.\n+\t\t */\n+\t\tif (res && res != 1)\n+\t\t\tres = -1;\n+\n \t\tcommit_list_free(common);\n \t\tcommit_list_free(remotes);\n \t}\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 58b3bb0c271..297b84e60d5 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1249,6 +1249,17 @@ test_expect_success 'interrupted rebase -i with --strategy and -X' '\n \tgit rebase --continue &&\n \ttest $(git show conflict-branch:conflict) = $(cat conflict) &&\n \ttest $(cat file1) = Z\n+'\n+\n+test_expect_success 'failing pick with --strategy is rescheduled' '\n+\ttest_when_finished \"rm -rf bin; test_might_fail git rebase --abort\" &&\n+\tmkdir bin &&\n+\techo exit 2 | write_script bin/git-merge-fail &&\n+\tgit log -1 --format=\"pick %H # %s\" HEAD >expect &&\n+\ttest_must_fail env PATH=\"$PWD/bin:$PATH\" \\\n+\t\tgit rebase --no-ff --strategy fail HEAD^ &&\n+\ttest_cmp expect .git/rebase-merge/git-rebase-todo &&\n+\ttest_cmp expect .git/rebase-merge/done\n '\n \n test_expect_success 'rebase -i error on commits with \\ in message' '\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"547981","messageId":"fc89e77c6e890993d314cfedc53b4e4bb5b1ad5f.1783948637.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1783948637.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 04/10] sequencer: never reschedule on failed commit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-13T13:17:21Z","receivedAt":"2026-07-13T13:17:47Z","isPatch":true,"body":"From: Phillip Wood \n\nIf \"git commit\" fails to run then run_git_commit() returns -1 which\ncauses the current command to be rescheduled. This is incorrect as\nwe have successfully picked the commit and have written all the state\nfiles we need to successfully commit when the user continues. Fix this\nby converting -1 to 1 which matches what do_merge() does.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 21dd5ec9799..c97b996bebc 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2542,6 +2542,12 @@ static int do_pick_commit(struct repository *r,\n \t\t\tres = run_git_commit(NULL, reflog_action, opts, flags);\n \t\t\t*check_todo = 1;\n \t\t}\n+\t\t/*\n+\t\t * If \"git commit\" failed to run then res == -1, but we don't\n+\t\t * want reschedule the last command because the picking the\n+\t\t * commit was successful.\n+\t\t */\n+\t\tres = !!res;\n \t}\n \n \n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"547982","messageId":"26eef6c09586ff2fec42614079189350e137751f.1783948637.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1783948637.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 05/10] sequencer: remove unnecessary \"or\" in pick_one_commit()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-13T13:17:22Z","receivedAt":"2026-07-13T13:17:48Z","isPatch":true,"body":"From: Phillip Wood \n\nIf error_with_patch(..., res, ...) succeeds then it returns \"res\", if\nit fails then it returns -1. This means that or-ing the return value\nwith \"res\" is pointless as the result is the same as the return value.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex c97b996bebc..d0d2cc228c8 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5007,9 +5007,8 @@ static int pick_one_commit(struct repository *r,\n \t\t oideq(&opts->squash_onto, &oid))))\n \t\t\tto_amend = 1;\n \n-\t\treturn res | error_with_patch(r, item->commit,\n-\t\t\t\t\t arg, item->arg_len, opts,\n-\t\t\t\t\t res, to_amend);\n+\t\treturn error_with_patch(r, item->commit, arg, item->arg_len,\n+\t\t\t\t\topts, res, to_amend);\n \t}\n \treturn res;\n }\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"547983","messageId":"26dc48951cea663080bacf7d8d4760528125cbf5.1783948637.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1783948637.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 06/10] sequencer: simplify handing of fixup with conflicts","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-13T13:17:23Z","receivedAt":"2026-07-13T13:17:49Z","isPatch":true,"body":"From: Phillip Wood \n\nCommit e032abd5a0 (rebase: fix rewritten list for failed pick,\n2023-09-06) introduced an early return when res == -1, so if we enter\nthis conditional block then res is positive. After the last couple\nof commits the only possible positive value is 1 so we can simplify\nthe code by removing the conditional call to intend_to_amend() and\ncall it error_with_patch() instead.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex d0d2cc228c8..a70889a107e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3874,7 +3874,7 @@ static int error_failed_squash(struct repository *r,\n \t\treturn error(_(\"could not copy '%s' to '%s'\"),\n \t\t\t rebase_path_message(),\n \t\t\t git_path_merge_msg(r));\n-\treturn error_with_patch(r, commit, subject, subject_len, opts, 1, 0);\n+\treturn error_with_patch(r, commit, subject, subject_len, opts, 1, 1);\n }\n \n static int do_exec(struct repository *r, const char *command_line, int quiet)\n@@ -4986,8 +4986,6 @@ static int pick_one_commit(struct repository *r,\n \t\trecord_in_rewritten(&item->commit->object.oid,\n \t\t\t\t peek_command(todo_list, 1));\n \tif (res && is_fixup(item->command)) {\n-\t\tif (res == 1)\n-\t\t\tintend_to_amend();\n \t\treturn error_failed_squash(r, item->commit, opts,\n \t\t\t\t\t item->arg_len, arg);\n \t} else if (res && is_rebase_i(opts) && item->commit) {\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"547984","messageId":"71ed717d3224dbd143fe0cce8adfac65b7ea4ed6.1783948637.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1783948637.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 07/10] sequencer: remove unnecessary condition in pick_one_commit()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-13T13:17:24Z","receivedAt":"2026-07-13T13:17:50Z","isPatch":true,"body":"From: Phillip Wood \n\nitem->commit holds the commit to be picked and so it must be non-NULL\notherwise pick_one_commit() would not know which commit to pick.\nIt is also unconditionally dereferenced in do_pick_commit() which is\ncalled at the top of this function. Therefore the check to see if it\nis non-NULL is superfluous.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex a70889a107e..5f5ff3783e6 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4988,7 +4988,7 @@ static int pick_one_commit(struct repository *r,\n \tif (res && is_fixup(item->command)) {\n \t\treturn error_failed_squash(r, item->commit, opts,\n \t\t\t\t\t item->arg_len, arg);\n-\t} else if (res && is_rebase_i(opts) && item->commit) {\n+\t} else if (res && is_rebase_i(opts)) {\n \t\tint to_amend = 0;\n \t\tstruct object_id oid;\n \n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"547985","messageId":"e8b7fa4c59e81487c5302423dc894776ecc9416d.1783948637.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1783948637.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 08/10] sequencer: simplify pick_one_commit()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-13T13:17:25Z","receivedAt":"2026-07-13T13:17:51Z","isPatch":true,"body":"From: Phillip Wood \n\nUnless we're rebasing all we do in pick_one_commit() is call\ndo_pick_commit() and return its result. Simplify the code by returning\nearly if we're not rebasing so that we don't have to continually call\nis_rebase_i() in the rest of the function. Note that there are a couple\nof conditions that do not call is_rebase_i() but they check for either\nan \"edit\" or a \"fixup\" command, both of which imply we're rebasing.\n\nThe only block that does not return early is the one guarded by\n\"!res\". Move the return into that block to make it clear that after\nrecording the commit as rewritten all we do is return from the function.\n\nAs the conditional blocks are all mutually exclusive (either the\nconditions are mutually exclusive, or an earlier conditional block\nthat would match a later one contains a \"return\" statement) chain\nthem together with \"else if\" to make that clear.\n\nWhile we could remove \"res\" from the conditions below \"if (!res)\"\nthey are left alone because, when we start using an enum in the next\ncommit, it makes it clear that these clauses are handling cases where\nthere are conflicts.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 19 +++++++++++--------\n 1 file changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 5f5ff3783e6..ff4547d417e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4966,12 +4966,14 @@ static int pick_one_commit(struct repository *r,\n \n \tres = do_pick_commit(r, item, opts, is_final_fixup(todo_list),\n \t\t\t check_todo);\n-\tif (is_rebase_i(opts) && res < 0) {\n+\tif (!is_rebase_i(opts))\n+\t\treturn res;\n+\n+\tif (res < 0) {\n \t\t/* Reschedule */\n \t\t*reschedule = 1;\n \t\treturn -1;\n-\t}\n-\tif (item->command == TODO_EDIT) {\n+\t} else if (item->command == TODO_EDIT) {\n \t\tstruct commit *commit = item->commit;\n \t\tif (!res) {\n \t\t\tif (!opts->verbose)\n@@ -4981,14 +4983,14 @@ static int pick_one_commit(struct repository *r,\n \t\t}\n \t\treturn error_with_patch(r, commit,\n \t\t\t\t\targ, item->arg_len, opts, res, !res);\n-\t}\n-\tif (is_rebase_i(opts) && !res)\n+\t} else if (!res) {\n \t\trecord_in_rewritten(&item->commit->object.oid,\n \t\t\t\t peek_command(todo_list, 1));\n-\tif (res && is_fixup(item->command)) {\n+\t\treturn 0;\n+\t} else if (res && is_fixup(item->command)) {\n \t\treturn error_failed_squash(r, item->commit, opts,\n \t\t\t\t\t item->arg_len, arg);\n-\t} else if (res && is_rebase_i(opts)) {\n+\t} else if (res) {\n \t\tint to_amend = 0;\n \t\tstruct object_id oid;\n \n@@ -5008,7 +5010,8 @@ static int pick_one_commit(struct repository *r,\n \t\treturn error_with_patch(r, item->commit, arg, item->arg_len,\n \t\t\t\t\topts, res, to_amend);\n \t}\n-\treturn res;\n+\n+\tBUG(\"Unhandled return value from do_pick_commit()\");\n }\n \n static int pick_commits(struct repository *r,\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"547986","messageId":"4fb641afb3cb99858ccabd69d4a052a6b19b6148.1783948637.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1783948637.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 09/10] sequencer: use an enum to represent result of picking a commit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-13T13:17:26Z","receivedAt":"2026-07-13T13:17:52Z","isPatch":true,"body":"From: Phillip Wood \n\nRather than using an integer where -1 is an error, 0 is success and\n1 means there were conflicts use an enum. This is clearer and lets\nus add a separate return value for commits that are dropped because\nthey become empty in the next commit.\n\nNote we continue to use \"return error(...)\" to return errors and\ntake advantage of C's lax typing of enums\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 61 +++++++++++++++++++++++++++++++++++++++--------------\n 1 file changed, 45 insertions(+), 16 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex ff4547d417e..4b89349251b 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2260,10 +2260,16 @@ static const char *reflog_message(struct replay_opts *opts,\n \treturn buf.buf;\n }\n \n-static int do_pick_commit(struct repository *r,\n-\t\t\t struct todo_item *item,\n-\t\t\t struct replay_opts *opts,\n-\t\t\t int final_fixup, int *check_todo)\n+enum pick_result {\n+\tPICK_RESULT_ERROR = -1,\n+\tPICK_RESULT_OK,\n+\tPICK_RESULT_CONFLICTS,\n+};\n+\n+static enum pick_result do_pick_commit(struct repository *r,\n+\t\t\t\t struct todo_item *item,\n+\t\t\t\t struct replay_opts *opts,\n+\t\t\t\t int final_fixup, int *check_todo)\n {\n \tstruct replay_ctx *ctx = opts->ctx;\n \tunsigned int flags = should_edit(opts) ? EDIT_MSG : 0;\n@@ -2564,7 +2570,12 @@ static int do_pick_commit(struct repository *r,\n \tfree(author);\n \tupdate_abort_safety_file();\n \n-\treturn res;\n+\tif (res < 0)\n+\t\treturn PICK_RESULT_ERROR;\n+\telse if (res > 0)\n+\t\treturn PICK_RESULT_CONFLICTS;\n+\telse\n+\t\treturn PICK_RESULT_OK;\n }\n \n static int prepare_revs(struct replay_opts *opts)\n@@ -4960,37 +4971,47 @@ static int pick_one_commit(struct repository *r,\n \t\t\t struct replay_opts *opts,\n \t\t\t int *check_todo, int* reschedule)\n {\n-\tint res;\n+\tenum pick_result pick_res;\n \tstruct todo_item *item = todo_list->items + todo_list->current;\n \tconst char *arg = todo_item_get_arg(todo_list, item);\n \n-\tres = do_pick_commit(r, item, opts, is_final_fixup(todo_list),\n-\t\t\t check_todo);\n+\tpick_res = do_pick_commit(r, item, opts, is_final_fixup(todo_list),\n+\t\t\t\t check_todo);\n \tif (!is_rebase_i(opts))\n-\t\treturn res;\n+\t\tswitch (pick_res) {\n+\t\tcase PICK_RESULT_ERROR:\n+\t\t\treturn -1;\n+\t\tcase PICK_RESULT_CONFLICTS:\n+\t\t\treturn 1;\n+\t\tdefault:\n+\t\t\treturn 0;\n+\t\t}\n \n-\tif (res < 0) {\n+\tif (pick_res == PICK_RESULT_ERROR) {\n \t\t/* Reschedule */\n \t\t*reschedule = 1;\n \t\treturn -1;\n \t} else if (item->command == TODO_EDIT) {\n \t\tstruct commit *commit = item->commit;\n-\t\tif (!res) {\n+\t\tint res = pick_res == PICK_RESULT_CONFLICTS;\n+\n+\t\tif (pick_res == PICK_RESULT_OK) {\n \t\t\tif (!opts->verbose)\n \t\t\t\tterm_clear_line();\n \t\t\tfprintf(stderr, _(\"Stopped at %s... %.*s\\n\"),\n \t\t\t\tshort_commit_name(r, commit), item->arg_len, arg);\n \t\t}\n \t\treturn error_with_patch(r, commit,\n \t\t\t\t\targ, item->arg_len, opts, res, !res);\n-\t} else if (!res) {\n+\t} else if (pick_res == PICK_RESULT_OK) {\n \t\trecord_in_rewritten(&item->commit->object.oid,\n \t\t\t\t peek_command(todo_list, 1));\n \t\treturn 0;\n-\t} else if (res && is_fixup(item->command)) {\n+\t} else if (pick_res == PICK_RESULT_CONFLICTS &&\n+\t\t is_fixup(item->command)) {\n \t\treturn error_failed_squash(r, item->commit, opts,\n \t\t\t\t\t item->arg_len, arg);\n-\t} else if (res) {\n+\t} else if (pick_res == PICK_RESULT_CONFLICTS) {\n \t\tint to_amend = 0;\n \t\tstruct object_id oid;\n \n@@ -5008,7 +5029,7 @@ static int pick_one_commit(struct repository *r,\n \t\t\tto_amend = 1;\n \n \t\treturn error_with_patch(r, item->commit, arg, item->arg_len,\n-\t\t\t\t\topts, res, to_amend);\n+\t\t\t\t\topts, 1, to_amend);\n \t}\n \n \tBUG(\"Unhandled return value from do_pick_commit()\");\n@@ -5547,7 +5568,15 @@ static int single_pick(struct repository *r,\n \t\t\tTODO_PICK : TODO_REVERT;\n \titem.commit = cmit;\n \n-\treturn do_pick_commit(r, &item, opts, 0, &check_todo);\n+\tswitch (do_pick_commit(r, &item, opts, 0, &check_todo)) {\n+\tcase PICK_RESULT_ERROR:\n+\t\treturn -1;\n+\tcase PICK_RESULT_CONFLICTS:\n+\t\treturn 1;\n+\tdefault:\n+\t\treturn 0;\n+\t}\n+\n }\n \n int sequencer_pick_revisions(struct repository *r,\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"547987","messageId":"c89234dd949f59ce150f0fb5d7442e1f46e3fef3.1783948637.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1783948637.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 10/10] sequencer: do not record dropped commits as rewritten","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-13T13:17:27Z","receivedAt":"2026-07-13T13:17:53Z","isPatch":true,"body":"From: Phillip Wood \n\nIf a commit gets dropped because its changes are already upstream\nthen we should not record it as rewritten. As well as confusing any\npost-rewrite hooks this means we end up copying the notes from the\ndropped commit to the commit that was picked immediately before the\none that was dropped.\n\nWhile we do not want to record the dropped commit is rewritten, if\nit is the final commit in a chain of fixups then we need to flush\nthe list of rewritten commits. The behavior of an \"edit\" command\nwhere the commit is dropped is changed so that \"rebase --continue\"\nwill not amend the previous pick. However, as the code comment notes\nit will still be erroneously recorded as rewritten when the rebase\ncontinues. That will need to be addressed separately along with not\nrecording skipped commits as rewritten.\n\nThe initialization of \"drop_commit\" is moved to ensure it is initialized\nwhen rewording a fast-forwarded commit.\n\nReported-by: Uwe Kleine-König \nTested-by: Uwe Kleine-König \nSigned-off-by: Phillip Wood \n---\n sequencer.c | 24 +++++++++++++++++++-----\n t/t3400-rebase.sh | 12 ++++++++++++\n t/t5407-post-rewrite-hook.sh | 23 +++++++++++++++++++++++\n 3 files changed, 54 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 4b89349251b..7bc885085f9 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2264,6 +2264,7 @@ enum pick_result {\n \tPICK_RESULT_ERROR = -1,\n \tPICK_RESULT_OK,\n \tPICK_RESULT_CONFLICTS,\n+\tPICK_RESULT_DROPPED,\n };\n \n static enum pick_result do_pick_commit(struct repository *r,\n@@ -2279,7 +2280,7 @@ static enum pick_result do_pick_commit(struct repository *r,\n \tconst char *base_label, *next_label, *reflog_action;\n \tchar *author = NULL;\n \tstruct commit_message msg = { NULL, NULL, NULL, NULL };\n-\tint res, unborn = 0, reword = 0, allow, drop_commit;\n+\tint res, unborn = 0, reword = 0, allow, drop_commit = 0;\n \tenum todo_command command = item->command;\n \tstruct commit *commit = item->commit;\n \n@@ -2509,7 +2510,6 @@ static enum pick_result do_pick_commit(struct repository *r,\n \t\tgoto leave;\n \t}\n \n-\tdrop_commit = 0;\n \tallow = allow_empty(r, opts, commit);\n \tif (allow < 0) {\n \t\tres = allow;\n@@ -2574,6 +2574,8 @@ static enum pick_result do_pick_commit(struct repository *r,\n \t\treturn PICK_RESULT_ERROR;\n \telse if (res > 0)\n \t\treturn PICK_RESULT_CONFLICTS;\n+\telse if (drop_commit)\n+\t\treturn PICK_RESULT_DROPPED;\n \telse\n \t\treturn PICK_RESULT_OK;\n }\n@@ -4994,18 +4996,30 @@ static int pick_one_commit(struct repository *r,\n \t} else if (item->command == TODO_EDIT) {\n \t\tstruct commit *commit = item->commit;\n \t\tint res = pick_res == PICK_RESULT_CONFLICTS;\n+\t\tint to_amend = pick_res != PICK_RESULT_CONFLICTS &&\n+\t\t\t\tpick_res != PICK_RESULT_DROPPED;\n \n-\t\tif (pick_res == PICK_RESULT_OK) {\n+\t\t/*\n+\t\t * NEEDSWORK: Do not record the commit as rewritten when\n+\t\t * continuing if it was dropped. Does it even make sense\n+\t\t * to stop if the commit was dropped?\n+\t\t */\n+\t\tif (pick_res == PICK_RESULT_OK ||\n+\t\t pick_res == PICK_RESULT_DROPPED) {\n \t\t\tif (!opts->verbose)\n \t\t\t\tterm_clear_line();\n \t\t\tfprintf(stderr, _(\"Stopped at %s... %.*s\\n\"),\n \t\t\t\tshort_commit_name(r, commit), item->arg_len, arg);\n \t\t}\n-\t\treturn error_with_patch(r, commit,\n-\t\t\t\t\targ, item->arg_len, opts, res, !res);\n+\t\treturn error_with_patch(r, commit, arg, item->arg_len, opts,\n+\t\t\t\t\tres, to_amend);\n \t} else if (pick_res == PICK_RESULT_OK) {\n \t\trecord_in_rewritten(&item->commit->object.oid,\n \t\t\t\t peek_command(todo_list, 1));\n+\t\treturn 0;\n+\t} else if (pick_res == PICK_RESULT_DROPPED) {\n+\t\tif (is_final_fixup(todo_list))\n+\t\t\tflush_rewritten_pending();\n \t\treturn 0;\n \t} else if (pick_res == PICK_RESULT_CONFLICTS &&\n \t\t is_fixup(item->command)) {\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex f0e7fcf649a..1d09886ea35 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -274,6 +274,18 @@ test_expect_success 'rebase --apply can copy notes' '\n \tgit reset --hard n3 &&\n \tgit rebase --apply --onto n1 n2 &&\n \ttest \"a note\" = \"$(git notes show HEAD)\"\n+'\n+\n+test_expect_success 'rebase drops notes of dropped commits' '\n+\tgit checkout n1 &&\n+\techo n3 >n3.t &&\n+\techo n4 >n4.t &&\n+\tgit add n3.t n4.t &&\n+\tgit commit -m n34 &&\n+\tgit rebase HEAD n3 &&\n+\ttest_commit_message HEAD -m n2 &&\n+\ttest_must_fail git notes list HEAD >actual &&\n+\ttest_must_be_empty actual\n '\n \n test_expect_success 'rebase commit with an ancient timestamp' '\ndiff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh\nindex ad7f8c6f002..51991956d1d 100755\n--- a/t/t5407-post-rewrite-hook.sh\n+++ b/t/t5407-post-rewrite-hook.sh\n@@ -306,6 +306,29 @@ test_expect_success 'git rebase -i (exec)' '\n \tcat >expected.data <<-EOF &&\n \t$(git rev-parse C) $(git rev-parse HEAD^)\n \t$(git rev-parse D) $(git rev-parse HEAD)\n+\tEOF\n+\tverify_hook_input\n+'\n+\n+test_expect_success 'rebase with commits that become empty' '\n+\tcat >todo <<-\\EOF &&\n+\tpick H\n+\tpick E\n+\tfixup I\n+\tfixup H\n+\tpick G\n+\tpick I\n+\tEOF\n+\t(\n+\t\tset_replace_editor todo &&\n+\t\tgit rebase -i --empty=drop A A\n+\t) &&\n+\techo rebase >expected.args &&\n+\tcat >expected.data <<-EOF &&\n+\t$(git rev-parse H) $(git rev-parse HEAD~2)\n+\t$(git rev-parse E) $(git rev-parse HEAD~1)\n+\t$(git rev-parse I) $(git rev-parse HEAD~1)\n+\t$(git rev-parse G) $(git rev-parse HEAD)\n \tEOF\n \tverify_hook_input\n '\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"547991","messageId":"alTrZG34m85spT8Z@ugly.lan","threadId":"65820","inReplyTo":"65af2ac07a2bf85336245a7d9b9f0a8a0e8affdb.1783948637.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH v2 01/10] t3400: restore coverage for note copying with apply backend","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2026-07-13T13:43:00Z","receivedAt":"2026-07-13T13:43:09Z","isPatch":true,"body":"On Mon, Jul 13, 2026 at 02:17:18PM +0100, Phillip Wood wrote:\n>Now that the merge backend is the default\n>\nadd comma here for ease of parsing?\n\n> we have lost coverage for\n>\"git rebase --apply\" copying notes. Fix this by replacing \"-m\" with\n>\"--apply\"\n>\nand here?\n\n>as the previous test which uses the default backend now\n>checks the merge backend.\n>\n"},{"id":"547992","messageId":"alTvtOc39bLR4ocx@ugly.lan","threadId":"65820","inReplyTo":"3d79362332c1208eed1fb7f8b0d431ee92fe45c5.1783948637.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH v2 03/10] sequencer: be more careful with external merge","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2026-07-13T14:01:24Z","receivedAt":"2026-07-13T14:01:26Z","isPatch":true,"body":"On Mon, Jul 13, 2026 at 02:17:20PM +0100, Phillip Wood wrote:\n>If an external merge strategy cannot merge (for example because it\n>would overwrite an untracked file) it exits with a non-zero exit\n>code other than 1. This should be treated differently to a merge\n>\ns/to/from/, i think?\n\n>with conflicts\n\n>which is signalled by an exit code of 1\n>\nparenthesize, and add comma?\n\n>because as\n>the merge failed\n>\n(maybe add comma? here it becomes muddy ...)\n\n>we need to reschedule the last pick. The caller\n>expects us to return -1 in this case. Also reschedule without trying\n>to merge if the commit message cannot be written\n>\nadd comma?\n\n>as that prevents us\n>from successfully picking the commit.\n\ni know that most commas (and parens (or em-dashes)) are optional in \nenglish, but they _really_ help parsing complex sentences, because they \nreduce the amount of \"read-ahead\" required.\ni'm stopping at this commit, but subsequent ones could also use the \ntreatment. i trust that you don't actually need detailed suggestions.\n"},{"id":"547997","messageId":"alTxn7MmX3aH_7gp@ugly.lan","threadId":"65820","inReplyTo":"26dc48951cea663080bacf7d8d4760528125cbf5.1783948637.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH v2 06/10] sequencer: simplify handing of fixup with conflicts","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2026-07-13T14:09:35Z","receivedAt":"2026-07-13T14:09:43Z","isPatch":true,"body":"On Mon, Jul 13, 2026 at 02:17:23PM +0100, Phillip Wood wrote:\n>Commit e032abd5a0 (rebase: fix rewritten list for failed pick,\n>2023-09-06) introduced an early return when res == -1, so if we enter\n>this conditional block then res is positive. After the last couple\n>of commits the only possible positive value is 1 so we can simplify\n>the code by removing the conditional call to intend_to_amend() and\n\n>call it error_with_patch() instead.\n>\nthat part makes no sense, subverting the argumentation.\n(as-is, i actually can't follow the logic, but i suppose it would be \nclear with (much) more diff context. i'm not sure whether the commit \nmessage is supposed to substitute for that, or the reviewer is supposed \nto deal with that on their end.)\n\n"},{"id":"548033","messageId":"xmqq5x2iygd6.fsf@gitster.g","threadId":"65820","inReplyTo":"cover.1783948637.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH v2 00/10] sequencer: do not record dropped commits as rewritten","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-13T17:00:21Z","receivedAt":"2026-07-13T17:00:24Z","isPatch":true,"body":"Phillip Wood writes:\n\n> Thanks to everyone who commented on v1. I've squashed the fixups that\n> Junio had in \"seen\", squashed patches 8 & 9 together as suggested by\n> Oswald and expanded the commit message, and added Uwe's Tested-by:\n> trailer to the final patch. Oswald suggested extended the use of the\n> enum which I think is a good idea in the long-term but I punted on\n> that for now because I think it would be fairly invasive and this\n> series has enough refactoring in it already.\n\nThanks for a concise yet very informative summary of the changes\nupfront. This may be a format we want to encourage to contributors.\n\n> If a commit gets dropped because its changes are already upstream\n> then we should not record it as rewritten. As well as confusing any\n> post-rewrite hooks this means we end up copying the notes from the\n> dropped commit to the commit that was picked immediately before the\n> one that was dropped.\n\nVery well. I did not see anything questionable in this edition.\nThe contents of the tree at the end of the series is unchanged since\nthe previous iteration.\n\nShall we mark the topic ready for 'next' now?\n\nThanks.\n\n> This series is structured as follows:\n>\n> Patch 1 restores some test coverage that was lost when the default\n> rebase backend was changed.\n>\n> Patch 2 moves a function so it can be called without a forward\n> declaration in Patch 11.\n>\n> Patches 3 & 4 fix the return value of do_pick_commit() when an external\n> command fails (this is in preparation for patch 9).\n>\n> Patches 5-8 try and simplify the control flow in pick_one_commit()\n> in preparation for patch 9.\n>\n> Patch 9 changes the return type of do_pick_commit() to an enum.\n>\n> Patch 10 adds a new member to the enum from patch 9 for commits that\n> are dropped when they become empty and uses that to stop them from\n> being recorded as rewritten.\n>\n> base-commit: 6c3d7b73556db708feb3b16232fab1efc4353428\n> Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Frebase-drop-notes-with-commit%2Fv2\n> View-Changes-At: https://github.com/phillipwood/git/compare/6c3d7b735...c89234dd9\n> Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/rebase-drop-notes-with-commit/v2\n>\n>\n> Phillip Wood (10):\n> t3400: restore coverage for note copying with apply backend\n> sequencer: move definition of is_final_fixup()\n> sequencer: be more careful with external merge\n> sequencer: never reschedule on failed commit\n> sequencer: remove unnecessary \"or\" in pick_one_commit()\n> sequencer: simplify handing of fixup with conflicts\n> sequencer: remove unnecessary condition in pick_one_commit()\n> sequencer: simplify pick_one_commit()\n> sequencer: use an enum to represent result of picking a commit\n> sequencer: do not record dropped commits as rewritten\n>\n> sequencer.c | 154 +++++++++++++++++++++++-----------\n> t/t3400-rebase.sh | 16 +++-\n> t/t3404-rebase-interactive.sh | 11 +++\n> t/t5407-post-rewrite-hook.sh | 23 +++++\n> 4 files changed, 155 insertions(+), 49 deletions(-)\n>\n> Range-diff against v1:\n> 1: 65af2ac07a2 = 1: 65af2ac07a2 t3400: restore coverage for note copying with apply backend\n> 2: 02670f57e7d = 2: 02670f57e7d sequencer: move definition of is_final_fixup()\n> 3: 16fba1e823b ! 3: 3d79362332c sequencer: be more careful with external merge\n> @@ sequencer.c: static int do_pick_commit(struct repository *r,\n> +\t\t\t\t\topts->xopts.nr, opts->xopts.v,\n> \t\t\t\t\tcommon, oid_to_hex(&head), remotes);\n> +\t\t/*\n> -+\t\t * If the there were conflicts, try_merge_command() returns 1,\n> ++\t\t * If there were conflicts, try_merge_command() returns 1,\n> +\t\t * any other no-zero return code means that either the merge\n> +\t\t * command could not be run, or it failed to merge.\n> +\t\t */\n> 4: 3ffd06d6509 ! 4: fc89e77c6e8 sequencer: never reschedule on failed commit\n> @@ sequencer.c: static int do_pick_commit(struct repository *r,\n> \t\t\t*check_todo = 1;\n> \t\t}\n> +\t\t/*\n> -+\t\t * If \"git commit\" failed to run than res == -1 but we dont\n> ++\t\t * If \"git commit\" failed to run then res == -1, but we don't\n> +\t\t * want reschedule the last command because the picking the\n> +\t\t * commit was successful.\n> +\t\t */\n> 5: cb286ac70d7 ! 5: 26eef6c0958 sequencer: remove unnecessary \"or\" in pick_one_commit()\n> @@ Commit message\n> \n> If error_with_patch(..., res, ...) succeeds then it returns \"res\", if\n> it fails then it returns -1. This means that or-ing the return value\n> - with \"res\" is pointless the result is the same as the return value.\n> + with \"res\" is pointless as the result is the same as the return value.\n> \n> Signed-off-by: Phillip Wood \n> \n> 6: 1585d47e2ea = 6: 26dc48951ce sequencer: simplify handing of fixup with conflicts\n> 7: 4386ca67d10 = 7: 71ed717d322 sequencer: remove unnecessary condition in pick_one_commit()\n> 8: f51751fa3ec ! 8: e8b7fa4c59e sequencer: simplify pick_one_commit()\n> @@ Commit message\n> sequencer: simplify pick_one_commit()\n> \n> Unless we're rebasing all we do in pick_one_commit() is call\n> - do_pick_commit() and return its result. Simplify the code by returing\n> + do_pick_commit() and return its result. Simplify the code by returning\n> early if we're not rebasing so that we don't have to continually call\n> is_rebase_i() in the rest of the function. Note that there are a couple\n> of conditions that do not call is_rebase_i() but they check for either\n> an \"edit\" or a \"fixup\" command, both of which imply we're rebasing.\n> +\n> + The only block that does not return early is the one guarded by\n> + \"!res\". Move the return into that block to make it clear that after\n> + recording the commit as rewritten all we do is return from the function.\n> \n> As the conditional blocks are all mutually exclusive (either the\n> conditions are mutually exclusive, or an earlier conditional block\n> that would match a later one contains a \"return\" statement) chain\n> them together with \"else if\" to make that clear.\n> +\n> + While we could remove \"res\" from the conditions below \"if (!res)\"\n> + they are left alone because, when we start using an enum in the next\n> + commit, it makes it clear that these clauses are handling cases where\n> + there are conflicts.\n> \n> Signed-off-by: Phillip Wood \n> \n> @@ sequencer.c: static int pick_one_commit(struct repository *r,\n> \t\trecord_in_rewritten(&item->commit->object.oid,\n> \t\t\t\t peek_command(todo_list, 1));\n> -\tif (res && is_fixup(item->command)) {\n> ++\t\treturn 0;\n> +\t} else if (res && is_fixup(item->command)) {\n> \t\treturn error_failed_squash(r, item->commit, opts,\n> \t\t\t\t\t item->arg_len, arg);\n> @@ sequencer.c: static int pick_one_commit(struct repository *r,\n> \t\tint to_amend = 0;\n> \t\tstruct object_id oid;\n> \n> +@@ sequencer.c: static int pick_one_commit(struct repository *r,\n> + \t\treturn error_with_patch(r, item->commit, arg, item->arg_len,\n> + \t\t\t\t\topts, res, to_amend);\n> + \t}\n> +-\treturn res;\n> ++\n> ++\tBUG(\"Unhandled return value from do_pick_commit()\");\n> + }\n> + \n> + static int pick_commits(struct repository *r,\n> 9: 2541a4d6e3d < -: ----------- sequencer: return early from pick_one_commit() on success\n> 10: e4050ead27f = 9: 4fb641afb3c sequencer: use an enum to represent result of picking a commit\n> 11: 26551f2687b ! 10: c89234dd949 sequencer: do not record dropped commits as rewritten\n> @@ Commit message\n> when rewording a fast-forwarded commit.\n> \n> Reported-by: Uwe Kleine-König \n> + Tested-by: Uwe Kleine-König \n> Signed-off-by: Phillip Wood \n> \n> ## sequencer.c ##\n"},{"id":"548186","messageId":"20260714225056.2285055-1-rybak.a.v@gmail.com","threadId":"65820","inReplyTo":"02670f57e7d81d4ff7341fecff3ef04b9fdc0102.1783948637.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH v2 02/10] sequencer: move definition of is_final_fixup()","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2026-07-14T22:50:56Z","receivedAt":"2026-07-14T22:51:02Z","isPatch":true,"body":"> Move this function earlier in the file in preparation for adding a\n> new caller in a later commit.\n> \n> Signed-off-by: Phillip Wood \n> ---\n> sequencer.c | 30 +++++++++++++++---------------\n> 1 file changed, 15 insertions(+), 15 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 57855b0066a..32a09b6e87d 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -4627,21 +4627,6 @@ static int do_update_refs(struct repository *r, int quiet)\n> \tstrbuf_release(&update_msg);\n> \tstrbuf_release(&error_msg);\n> \treturn res;\n> -}\n> -\n> -static int is_final_fixup(struct todo_list *todo_list)\n> -{\n> -\tint i = todo_list->current;\n> -\n> -\tif (!is_fixup(todo_list->items[i].command))\n> -\t\treturn 0;\n> -\n> -\twhile (++i < todo_list->nr)\n> -\t\tif (is_fixup(todo_list->items[i].command))\n> -\t\t\treturn 0;\n> -\t\telse if (!is_noop(todo_list->items[i].command))\n> -\t\t\tbreak;\n> -\treturn 1;\n> }\n> \n> static enum todo_command peek_command(struct todo_list *todo_list, int offset)\n> @@ -4925,6 +4910,21 @@ static int reread_todo_if_changed(struct repository *r,\n\n4910 is greater than 4627, the function is_final_fixup() seems to have been\nmoved _later_ in the file. But the commit message says \"Move this function\nearlier in the file\". Am I missing something?\n\n> \tstrbuf_release(&buf);\n> \n> \treturn 0;\n> +}\n> +\n> +static int is_final_fixup(struct todo_list *todo_list)\n> +{\n> +\tint i = todo_list->current;\n> +\n> +\tif (!is_fixup(todo_list->items[i].command))\n> +\t\treturn 0;\n> +\n> +\twhile (++i < todo_list->nr)\n> +\t\tif (is_fixup(todo_list->items[i].command))\n> +\t\t\treturn 0;\n> +\t\telse if (!is_noop(todo_list->items[i].command))\n> +\t\t\tbreak;\n> +\treturn 1;\n> }\n> \n> static const char rescheduled_advice[] =\n> -- \n> 2.54.0.200.gfd8d68259e3\n"},{"id":"548248","messageId":"3856a84b-4680-41fd-bae6-3fab538dc3d7@gmail.com","threadId":"65820","inReplyTo":"20260714225056.2285055-1-rybak.a.v@gmail.com","subject":"Re: [PATCH v2 02/10] sequencer: move definition of is_final_fixup()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-15T09:12:58Z","receivedAt":"2026-07-15T09:13:04Z","isPatch":true,"body":"Hi Andrei\n\nOn 14/07/2026 23:50, Andrei Rybak wrote:\n>> Move this function earlier in the file in preparation for adding a\n>> new caller in a later commit.\n>>\n>> @@ -4925,6 +4910,21 @@ static int reread_todo_if_changed(struct repository *r,\n> \n> 4910 is greater than 4627, the function is_final_fixup() seems to have been\n> moved _later_ in the file. But the commit message says \"Move this function\n> earlier in the file\". Am I missing something?\n\nOh, thanks for the sanity check. I could have sworn I had to move this \nfunction to get a later commit to compile at one point, but it clearly \ndoesn't need to move now. I'll drop this patch.\n\nThanks\n\nPhillip\n\n> \n>> \tstrbuf_release(&buf);\n>> \n>> \treturn 0;\n>> +}\n>> +\n>> +static int is_final_fixup(struct todo_list *todo_list)\n>> +{\n>> +\tint i = todo_list->current;\n>> +\n>> +\tif (!is_fixup(todo_list->items[i].command))\n>> +\t\treturn 0;\n>> +\n>> +\twhile (++i < todo_list->nr)\n>> +\t\tif (is_fixup(todo_list->items[i].command))\n>> +\t\t\treturn 0;\n>> +\t\telse if (!is_noop(todo_list->items[i].command))\n>> +\t\t\tbreak;\n>> +\treturn 1;\n>> }\n>> \n>> static const char rescheduled_advice[] =\n>> -- \n>> 2.54.0.200.gfd8d68259e3\n\n"},{"id":"548249","messageId":"5c9991e0-81f8-41f8-b78c-b436d4a494b1@gmail.com","threadId":"65820","inReplyTo":"alTxn7MmX3aH_7gp@ugly.lan","subject":"Re: [PATCH v2 06/10] sequencer: simplify handing of fixup with conflicts","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-15T09:20:58Z","receivedAt":"2026-07-15T09:21:01Z","isPatch":true,"body":"Hi Oswald\n\nOn 13/07/2026 15:09, Oswald Buddenhagen wrote:\n> On Mon, Jul 13, 2026 at 02:17:23PM +0100, Phillip Wood wrote:\n>> Commit e032abd5a0 (rebase: fix rewritten list for failed pick,\n>> 2023-09-06) introduced an early return when res == -1, so if we enter\n>> this conditional block then res is positive. After the last couple\n>> of commits the only possible positive value is 1 so we can simplify\n>> the code by removing the conditional call to intend_to_amend() and\n> \n>> call it error_with_patch() instead.\n>>\n> that part makes no sense, \n\nIt should say \"call it in error_with_patch() instead\"\n\n> subverting the argumentation.\n> (as-is, i actually can't follow the logic, but i suppose it would be \n> clear with (much) more diff context. i'm not sure whether the commit \n> message is supposed to substitute for that, or the reviewer is supposed \n> to deal with that on their end.)\n\nIts tricky because error_with_patch() isn't changed at all, we change \nerror_failed_squash() to tell error_with_patch() to call \nintend_to_amend(). I've expanded the commit message to explain that better.\n\nThanks\n\nPhillip\n\n"},{"id":"548250","messageId":"58c488c1-139a-4b56-9f80-2492b081f659@gmail.com","threadId":"65820","inReplyTo":"alTvtOc39bLR4ocx@ugly.lan","subject":"Re: [PATCH v2 03/10] sequencer: be more careful with external merge","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-15T09:35:18Z","receivedAt":"2026-07-15T09:35:24Z","isPatch":true,"body":"Hi Oswald\n\nOn 13/07/2026 15:01, Oswald Buddenhagen wrote:\n> On Mon, Jul 13, 2026 at 02:17:20PM +0100, Phillip Wood wrote:\n>> If an external merge strategy cannot merge (for example because it\n>> would overwrite an untracked file) it exits with a non-zero exit\n>> code other than 1. This should be treated differently to a merge\n>>\n> s/to/from/, i think?\n\nBoth are valid - the internet tells be \"different to\" is more common it \nBritish English, whereas \"different from\" is more common in American \nEnglish. I guess for an international audience \"from\" would be the \nbetter choice.\n\n>> with conflicts\n> \n>> which is signalled by an exit code of 1\n>>\n> parenthesize, and add comma?\n> \n>> because as\n>> the merge failed\n>>\n> (maybe add comma? here it becomes muddy ...)\n> \n>> we need to reschedule the last pick. The caller\n>> expects us to return -1 in this case. Also reschedule without trying\n>> to merge if the commit message cannot be written\n>>\n> add comma?\n> \n>> as that prevents us\n>> from successfully picking the commit.\n> \n> i know that most commas (and parens (or em-dashes)) are optional in \n> english, but they _really_ help parsing complex sentences, because they \n> reduce the amount of \"read-ahead\" required.\n> i'm stopping at this commit, but subsequent ones could also use the \n> treatment. i trust that you don't actually need detailed suggestions.\n\nI've added a few more commas to later commits, but concrete suggestions \nare always welcome.\n\nThanks\n\nPhillip\n\n"},{"id":"548251","messageId":"6cdccc2b-c0b4-497f-8408-a18bd0981505@gmail.com","threadId":"65820","inReplyTo":"58c488c1-139a-4b56-9f80-2492b081f659@gmail.com","subject":"Re: [PATCH v2 03/10] sequencer: be more careful with external merge","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-15T09:42:55Z","receivedAt":"2026-07-15T09:42:59Z","isPatch":true,"body":"On 15/07/2026 10:35, Phillip Wood wrote:\n> Hi Oswald\n> \n> On 13/07/2026 15:01, Oswald Buddenhagen wrote:\n>> On Mon, Jul 13, 2026 at 02:17:20PM +0100, Phillip Wood wrote:\n>>> If an external merge strategy cannot merge (for example because it\n>>> would overwrite an untracked file) it exits with a non-zero exit\n>>> code other than 1. This should be treated differently to a merge\n>>>\n>> s/to/from/, i think?\n> \n> Both are valid - the internet tells be \"different to\" is more common it \n\nsigh s/be/me/\n\nPhillip\n\n> British English, whereas \"different from\" is more common in American \n> English. I guess for an international audience \"from\" would be the \n> better choice.\n> \n>>> with conflicts\n>>\n>>> which is signalled by an exit code of 1\n>>>\n>> parenthesize, and add comma?\n>>\n>>> because as\n>>> the merge failed\n>>>\n>> (maybe add comma? here it becomes muddy ...)\n>>\n>>> we need to reschedule the last pick. The caller\n>>> expects us to return -1 in this case. Also reschedule without trying\n>>> to merge if the commit message cannot be written\n>>>\n>> add comma?\n>>\n>>> as that prevents us\n>>> from successfully picking the commit.\n>>\n>> i know that most commas (and parens (or em-dashes)) are optional in \n>> english, but they _really_ help parsing complex sentences, because \n>> they reduce the amount of \"read-ahead\" required.\n>> i'm stopping at this commit, but subsequent ones could also use the \n>> treatment. i trust that you don't actually need detailed suggestions.\n> \n> I've added a few more commas to later commits, but concrete suggestions \n> are always welcome.\n> \n> Thanks\n> \n> Phillip\n> \n\n"},{"id":"548289","messageId":"c4705066ee0e3da2110fe155161ab669aff87425.1784128921.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1784128921.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v3 1/9] t3400: restore coverage for note copying with apply backend","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-15T15:21:55Z","receivedAt":"2026-07-15T15:22:20Z","isPatch":true,"body":"From: Phillip Wood \n\nNow that the merge backend is the default, we have lost coverage for\n\"git rebase --apply\" copying notes. Fix this by replacing \"-m\" with\n\"--apply\" as the previous test which uses the default backend now\nchecks the merge backend.\n\nSigned-off-by: Phillip Wood \n---\n t/t3400-rebase.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex c0c00fbb7b1..f0e7fcf649a 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -270,9 +270,9 @@ test_expect_success 'rebase can copy notes' '\n \ttest \"a note\" = \"$(git notes show HEAD)\"\n '\n \n-test_expect_success 'rebase -m can copy notes' '\n+test_expect_success 'rebase --apply can copy notes' '\n \tgit reset --hard n3 &&\n-\tgit rebase -m --onto n1 n2 &&\n+\tgit rebase --apply --onto n1 n2 &&\n \ttest \"a note\" = \"$(git notes show HEAD)\"\n '\n \n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"548290","messageId":"cover.1784128921.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1782833268.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v3 0/9] sequencer: do not record dropped commits as rewritten","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-15T15:21:54Z","receivedAt":"2026-07-15T15:22:20Z","isPatch":true,"body":"Thanks to everyone who commented on v2. I've dropped patch 2 which\nAndrei pointed out was pointless and tried to make the remaining\ncommit messages clearer as requested by Oswald.\n\nIf a commit gets dropped because its changes are already upstream\nthen we should not record it as rewritten. As well as confusing any\npost-rewrite hooks this means we end up copying the notes from the\ndropped commit to the commit that was picked immediately before the\none that was dropped.\n\nThis series is structured as follows:\n\nPatch 1 restores some test coverage that was lost when the default\nrebase backend was changed.\n\nPatches 2 & 3 fix the return value of do_pick_commit() when an external\ncommand fails (this is in preparation for patch 8).\n\nPatches 4-7 try and simplify the control flow in pick_one_commit()\nin preparation for patch 8.\n\nPatch 8 changes the return type of do_pick_commit() to an enum.\n\nPatch 9 adds a new member to the enum from patch 8 for commits that\nare dropped when they become empty and uses that to stop them from\nbeing recorded as rewritten.\n\nCover letter for v2:\n\nThanks to everyone who commented on v1. I've squashed the fixups that\nJunio had in \"seen\", squashed patches 8 & 9 together as suggested by\nOswald and expanded the commit message, and added Uwe's Tested-by:\ntrailer to the final patch. Oswald suggested extended the use of the\nenum which I think is a good idea in the long-term but I punted on\nthat for now because I think it would be fairly invasive and this\nseries has enough refactoring in it already.\n\nbase-commit: 6c3d7b73556db708feb3b16232fab1efc4353428\nPublished-As: https://github.com/phillipwood/git/releases/tag/pw%2Frebase-drop-notes-with-commit%2Fv3\nView-Changes-At: https://github.com/phillipwood/git/compare/6c3d7b735...2ef36b9ee\nFetch-It-Via: git fetch https://github.com/phillipwood/git pw/rebase-drop-notes-with-commit/v3\n\n\nPhillip Wood (9):\n t3400: restore coverage for note copying with apply backend\n sequencer: be more careful with external merge\n sequencer: never reschedule on failed commit\n sequencer: remove unnecessary \"or\" in pick_one_commit()\n sequencer: simplify handling of fixup with conflicts\n sequencer: remove unnecessary condition in pick_one_commit()\n sequencer: simplify pick_one_commit()\n sequencer: use an enum to represent result of picking a commit\n sequencer: do not record dropped commits as rewritten\n\n sequencer.c | 124 +++++++++++++++++++++++++---------\n t/t3400-rebase.sh | 16 ++++-\n t/t3404-rebase-interactive.sh | 11 +++\n t/t5407-post-rewrite-hook.sh | 23 +++++++\n 4 files changed, 140 insertions(+), 34 deletions(-)\n\nRange-diff against v2:\n 1: 65af2ac07a2 ! 1: c4705066ee0 t3400: restore coverage for note copying with apply backend\n @@ Metadata\n ## Commit message ##\n t3400: restore coverage for note copying with apply backend\n \n - Now that the merge backend is the default we have lost coverage for\n + Now that the merge backend is the default, we have lost coverage for\n \"git rebase --apply\" copying notes. Fix this by replacing \"-m\" with\n \"--apply\" as the previous test which uses the default backend now\n checks the merge backend.\n 2: 02670f57e7d < -: ----------- sequencer: move definition of is_final_fixup()\n 3: 3d79362332c ! 2: 947bb77e44f sequencer: be more careful with external merge\n @@ Commit message\n \n If an external merge strategy cannot merge (for example because it\n would overwrite an untracked file) it exits with a non-zero exit\n - code other than 1. This should be treated differently to a merge\n - with conflicts which is signalled by an exit code of 1 because as\n - the merge failed we need to reschedule the last pick. The caller\n + code other than 1. This should be treated differently from a merge\n + with conflicts, which is signaled by an exit code of 1, because, as\n + the merge failed, we need to reschedule the last pick. The caller\n expects us to return -1 in this case. Also reschedule without trying\n to merge if the commit message cannot be written as that prevents us\n from successfully picking the commit.\n 4: fc89e77c6e8 = 3: bff5f319e91 sequencer: never reschedule on failed commit\n 5: 26eef6c0958 = 4: e785433ad3d sequencer: remove unnecessary \"or\" in pick_one_commit()\n 6: 26dc48951ce ! 5: 134d8f7e935 sequencer: simplify handing of fixup with conflicts\n @@ Metadata\n Author: Phillip Wood \n \n ## Commit message ##\n - sequencer: simplify handing of fixup with conflicts\n + sequencer: simplify handling of fixup with conflicts\n \n Commit e032abd5a0 (rebase: fix rewritten list for failed pick,\n - 2023-09-06) introduced an early return when res == -1, so if we enter\n - this conditional block then res is positive. After the last couple\n - of commits the only possible positive value is 1 so we can simplify\n - the code by removing the conditional call to intend_to_amend() and\n - call it error_with_patch() instead.\n + 2023-09-06) introduced an early return when res == -1, so if\n + we enter this conditional block then res is positive. After the\n + last couple of commits the only possible positive value is 1. That\n + means we can simplify the code by removing the conditional call to\n + intend_to_amend() and have error_failed_squash() request that it is\n + called in error_with_patch() instead.\n \n Signed-off-by: Phillip Wood \n \n 7: 71ed717d322 = 6: e3091dee633 sequencer: remove unnecessary condition in pick_one_commit()\n 8: e8b7fa4c59e ! 7: 7c1642b0a49 sequencer: simplify pick_one_commit()\n @@ Metadata\n ## Commit message ##\n sequencer: simplify pick_one_commit()\n \n - Unless we're rebasing all we do in pick_one_commit() is call\n + Unless we're rebasing, all we do in pick_one_commit() is call\n do_pick_commit() and return its result. Simplify the code by returning\n - early if we're not rebasing so that we don't have to continually call\n + early if we're not rebasing so that we don't have to repeatedly call\n is_rebase_i() in the rest of the function. Note that there are a couple\n of conditions that do not call is_rebase_i() but they check for either\n an \"edit\" or a \"fixup\" command, both of which imply we're rebasing.\n \n The only block that does not return early is the one guarded by\n \"!res\". Move the return into that block to make it clear that after\n - recording the commit as rewritten all we do is return from the function.\n + recording the commit as rewritten, all we do is return from the\n + function.\n \n As the conditional blocks are all mutually exclusive (either the\n conditions are mutually exclusive, or an earlier conditional block\n 9: 4fb641afb3c ! 8: 0a146d57266 sequencer: use an enum to represent result of picking a commit\n @@ Metadata\n ## Commit message ##\n sequencer: use an enum to represent result of picking a commit\n \n - Rather than using an integer where -1 is an error, 0 is success and\n - 1 means there were conflicts use an enum. This is clearer and lets\n + Rather than using an integer where -1 is an error, 0 is success and 1\n + indicates there were conflicts, use an enum. This is clearer and lets\n us add a separate return value for commits that are dropped because\n they become empty in the next commit.\n \n10: c89234dd949 ! 9: 2ef36b9ee5a sequencer: do not record dropped commits as rewritten\n @@ Commit message\n \n If a commit gets dropped because its changes are already upstream\n then we should not record it as rewritten. As well as confusing any\n - post-rewrite hooks this means we end up copying the notes from the\n + post-rewrite hooks, it means we end up copying the notes from the\n dropped commit to the commit that was picked immediately before the\n one that was dropped.\n \n - While we do not want to record the dropped commit is rewritten, if\n + While we do not want to record the dropped commit as rewritten, if\n it is the final commit in a chain of fixups then we need to flush\n the list of rewritten commits. The behavior of an \"edit\" command\n where the commit is dropped is changed so that \"rebase --continue\"\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"548291","messageId":"bff5f319e91b2b5ea13a32906d0d76bd688183fa.1784128921.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1784128921.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v3 3/9] sequencer: never reschedule on failed commit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-15T15:21:57Z","receivedAt":"2026-07-15T15:22:22Z","isPatch":true,"body":"From: Phillip Wood \n\nIf \"git commit\" fails to run then run_git_commit() returns -1 which\ncauses the current command to be rescheduled. This is incorrect as\nwe have successfully picked the commit and have written all the state\nfiles we need to successfully commit when the user continues. Fix this\nby converting -1 to 1 which matches what do_merge() does.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex eaffa8ebb84..1db844100ad 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2542,6 +2542,12 @@ static int do_pick_commit(struct repository *r,\n \t\t\tres = run_git_commit(NULL, reflog_action, opts, flags);\n \t\t\t*check_todo = 1;\n \t\t}\n+\t\t/*\n+\t\t * If \"git commit\" failed to run then res == -1, but we don't\n+\t\t * want reschedule the last command because the picking the\n+\t\t * commit was successful.\n+\t\t */\n+\t\tres = !!res;\n \t}\n \n \n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"548292","messageId":"947bb77e44f2087c5f148e887c280bb54d648b03.1784128921.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1784128921.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v3 2/9] sequencer: be more careful with external merge","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-15T15:21:56Z","receivedAt":"2026-07-15T15:22:23Z","isPatch":true,"body":"From: Phillip Wood \n\nIf an external merge strategy cannot merge (for example because it\nwould overwrite an untracked file) it exits with a non-zero exit\ncode other than 1. This should be treated differently from a merge\nwith conflicts, which is signaled by an exit code of 1, because, as\nthe merge failed, we need to reschedule the last pick. The caller\nexpects us to return -1 in this case. Also reschedule without trying\nto merge if the commit message cannot be written as that prevents us\nfrom successfully picking the commit.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 19 +++++++++++++++----\n t/t3404-rebase-interactive.sh | 11 +++++++++++\n 2 files changed, 26 insertions(+), 4 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 57855b0066a..eaffa8ebb84 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2453,14 +2453,25 @@ static int do_pick_commit(struct repository *r,\n \t\tstruct commit_list *common = NULL;\n \t\tstruct commit_list *remotes = NULL;\n \n-\t\tres = write_message(ctx->message.buf, ctx->message.len,\n-\t\t\t\t git_path_merge_msg(r), 0);\n+\t\tif (write_message(ctx->message.buf, ctx->message.len,\n+\t\t\t\t git_path_merge_msg(r), 0)) {\n+\t\t\tres = -1;\n+\t\t\tgoto leave;\n+\t\t}\n \n \t\tcommit_list_insert(base, &common);\n \t\tcommit_list_insert(next, &remotes);\n-\t\tres |= try_merge_command(r, opts->strategy,\n-\t\t\t\t\t opts->xopts.nr, opts->xopts.v,\n+\t\tres = try_merge_command(r, opts->strategy,\n+\t\t\t\t\topts->xopts.nr, opts->xopts.v,\n \t\t\t\t\tcommon, oid_to_hex(&head), remotes);\n+\t\t/*\n+\t\t * If there were conflicts, try_merge_command() returns 1,\n+\t\t * any other no-zero return code means that either the merge\n+\t\t * command could not be run, or it failed to merge.\n+\t\t */\n+\t\tif (res && res != 1)\n+\t\t\tres = -1;\n+\n \t\tcommit_list_free(common);\n \t\tcommit_list_free(remotes);\n \t}\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 58b3bb0c271..297b84e60d5 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1249,6 +1249,17 @@ test_expect_success 'interrupted rebase -i with --strategy and -X' '\n \tgit rebase --continue &&\n \ttest $(git show conflict-branch:conflict) = $(cat conflict) &&\n \ttest $(cat file1) = Z\n+'\n+\n+test_expect_success 'failing pick with --strategy is rescheduled' '\n+\ttest_when_finished \"rm -rf bin; test_might_fail git rebase --abort\" &&\n+\tmkdir bin &&\n+\techo exit 2 | write_script bin/git-merge-fail &&\n+\tgit log -1 --format=\"pick %H # %s\" HEAD >expect &&\n+\ttest_must_fail env PATH=\"$PWD/bin:$PATH\" \\\n+\t\tgit rebase --no-ff --strategy fail HEAD^ &&\n+\ttest_cmp expect .git/rebase-merge/git-rebase-todo &&\n+\ttest_cmp expect .git/rebase-merge/done\n '\n \n test_expect_success 'rebase -i error on commits with \\ in message' '\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"548293","messageId":"e785433ad3d77945c6eea7c732b0df5d9d04774d.1784128921.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1784128921.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v3 4/9] sequencer: remove unnecessary \"or\" in pick_one_commit()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-15T15:21:58Z","receivedAt":"2026-07-15T15:22:23Z","isPatch":true,"body":"From: Phillip Wood \n\nIf error_with_patch(..., res, ...) succeeds then it returns \"res\", if\nit fails then it returns -1. This means that or-ing the return value\nwith \"res\" is pointless as the result is the same as the return value.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 1db844100ad..70e12eab0ec 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -5007,9 +5007,8 @@ static int pick_one_commit(struct repository *r,\n \t\t oideq(&opts->squash_onto, &oid))))\n \t\t\tto_amend = 1;\n \n-\t\treturn res | error_with_patch(r, item->commit,\n-\t\t\t\t\t arg, item->arg_len, opts,\n-\t\t\t\t\t res, to_amend);\n+\t\treturn error_with_patch(r, item->commit, arg, item->arg_len,\n+\t\t\t\t\topts, res, to_amend);\n \t}\n \treturn res;\n }\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"548294","messageId":"134d8f7e935f28da90ddd78883c8f0aa8705230f.1784128921.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1784128921.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v3 5/9] sequencer: simplify handling of fixup with conflicts","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-15T15:21:59Z","receivedAt":"2026-07-15T15:22:25Z","isPatch":true,"body":"From: Phillip Wood \n\nCommit e032abd5a0 (rebase: fix rewritten list for failed pick,\n2023-09-06) introduced an early return when res == -1, so if\nwe enter this conditional block then res is positive. After the\nlast couple of commits the only possible positive value is 1. That\nmeans we can simplify the code by removing the conditional call to\nintend_to_amend() and have error_failed_squash() request that it is\ncalled in error_with_patch() instead.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 70e12eab0ec..a00e3622c87 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3874,7 +3874,7 @@ static int error_failed_squash(struct repository *r,\n \t\treturn error(_(\"could not copy '%s' to '%s'\"),\n \t\t\t rebase_path_message(),\n \t\t\t git_path_merge_msg(r));\n-\treturn error_with_patch(r, commit, subject, subject_len, opts, 1, 0);\n+\treturn error_with_patch(r, commit, subject, subject_len, opts, 1, 1);\n }\n \n static int do_exec(struct repository *r, const char *command_line, int quiet)\n@@ -4986,8 +4986,6 @@ static int pick_one_commit(struct repository *r,\n \t\trecord_in_rewritten(&item->commit->object.oid,\n \t\t\t\t peek_command(todo_list, 1));\n \tif (res && is_fixup(item->command)) {\n-\t\tif (res == 1)\n-\t\t\tintend_to_amend();\n \t\treturn error_failed_squash(r, item->commit, opts,\n \t\t\t\t\t item->arg_len, arg);\n \t} else if (res && is_rebase_i(opts) && item->commit) {\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"548295","messageId":"e3091dee633ea59d1ed853f4f0ca0fca29d0ec13.1784128921.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1784128921.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v3 6/9] sequencer: remove unnecessary condition in pick_one_commit()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-15T15:22:00Z","receivedAt":"2026-07-15T15:22:25Z","isPatch":true,"body":"From: Phillip Wood \n\nitem->commit holds the commit to be picked and so it must be non-NULL\notherwise pick_one_commit() would not know which commit to pick.\nIt is also unconditionally dereferenced in do_pick_commit() which is\ncalled at the top of this function. Therefore the check to see if it\nis non-NULL is superfluous.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex a00e3622c87..8f3eed205e7 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4988,7 +4988,7 @@ static int pick_one_commit(struct repository *r,\n \tif (res && is_fixup(item->command)) {\n \t\treturn error_failed_squash(r, item->commit, opts,\n \t\t\t\t\t item->arg_len, arg);\n-\t} else if (res && is_rebase_i(opts) && item->commit) {\n+\t} else if (res && is_rebase_i(opts)) {\n \t\tint to_amend = 0;\n \t\tstruct object_id oid;\n \n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"548296","messageId":"7c1642b0a49027a8851aa5985f4020d8e76414d1.1784128921.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1784128921.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v3 7/9] sequencer: simplify pick_one_commit()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-15T15:22:01Z","receivedAt":"2026-07-15T15:22:26Z","isPatch":true,"body":"From: Phillip Wood \n\nUnless we're rebasing, all we do in pick_one_commit() is call\ndo_pick_commit() and return its result. Simplify the code by returning\nearly if we're not rebasing so that we don't have to repeatedly call\nis_rebase_i() in the rest of the function. Note that there are a couple\nof conditions that do not call is_rebase_i() but they check for either\nan \"edit\" or a \"fixup\" command, both of which imply we're rebasing.\n\nThe only block that does not return early is the one guarded by\n\"!res\". Move the return into that block to make it clear that after\nrecording the commit as rewritten, all we do is return from the\nfunction.\n\nAs the conditional blocks are all mutually exclusive (either the\nconditions are mutually exclusive, or an earlier conditional block\nthat would match a later one contains a \"return\" statement) chain\nthem together with \"else if\" to make that clear.\n\nWhile we could remove \"res\" from the conditions below \"if (!res)\"\nthey are left alone because, when we start using an enum in the next\ncommit, it makes it clear that these clauses are handling cases where\nthere are conflicts.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 19 +++++++++++--------\n 1 file changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 8f3eed205e7..9016af9b5d7 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4966,12 +4966,14 @@ static int pick_one_commit(struct repository *r,\n \n \tres = do_pick_commit(r, item, opts, is_final_fixup(todo_list),\n \t\t\t check_todo);\n-\tif (is_rebase_i(opts) && res < 0) {\n+\tif (!is_rebase_i(opts))\n+\t\treturn res;\n+\n+\tif (res < 0) {\n \t\t/* Reschedule */\n \t\t*reschedule = 1;\n \t\treturn -1;\n-\t}\n-\tif (item->command == TODO_EDIT) {\n+\t} else if (item->command == TODO_EDIT) {\n \t\tstruct commit *commit = item->commit;\n \t\tif (!res) {\n \t\t\tif (!opts->verbose)\n@@ -4981,14 +4983,14 @@ static int pick_one_commit(struct repository *r,\n \t\t}\n \t\treturn error_with_patch(r, commit,\n \t\t\t\t\targ, item->arg_len, opts, res, !res);\n-\t}\n-\tif (is_rebase_i(opts) && !res)\n+\t} else if (!res) {\n \t\trecord_in_rewritten(&item->commit->object.oid,\n \t\t\t\t peek_command(todo_list, 1));\n-\tif (res && is_fixup(item->command)) {\n+\t\treturn 0;\n+\t} else if (res && is_fixup(item->command)) {\n \t\treturn error_failed_squash(r, item->commit, opts,\n \t\t\t\t\t item->arg_len, arg);\n-\t} else if (res && is_rebase_i(opts)) {\n+\t} else if (res) {\n \t\tint to_amend = 0;\n \t\tstruct object_id oid;\n \n@@ -5008,7 +5010,8 @@ static int pick_one_commit(struct repository *r,\n \t\treturn error_with_patch(r, item->commit, arg, item->arg_len,\n \t\t\t\t\topts, res, to_amend);\n \t}\n-\treturn res;\n+\n+\tBUG(\"Unhandled return value from do_pick_commit()\");\n }\n \n static int pick_commits(struct repository *r,\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"548297","messageId":"0a146d57266bdb54bbccdf714967c42ab0a2ecfc.1784128921.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1784128921.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v3 8/9] sequencer: use an enum to represent result of picking a commit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-15T15:22:02Z","receivedAt":"2026-07-15T15:22:27Z","isPatch":true,"body":"From: Phillip Wood \n\nRather than using an integer where -1 is an error, 0 is success and 1\nindicates there were conflicts, use an enum. This is clearer and lets\nus add a separate return value for commits that are dropped because\nthey become empty in the next commit.\n\nNote we continue to use \"return error(...)\" to return errors and\ntake advantage of C's lax typing of enums\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 61 +++++++++++++++++++++++++++++++++++++++--------------\n 1 file changed, 45 insertions(+), 16 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 9016af9b5d7..4b3092dc9bb 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2260,10 +2260,16 @@ static const char *reflog_message(struct replay_opts *opts,\n \treturn buf.buf;\n }\n \n-static int do_pick_commit(struct repository *r,\n-\t\t\t struct todo_item *item,\n-\t\t\t struct replay_opts *opts,\n-\t\t\t int final_fixup, int *check_todo)\n+enum pick_result {\n+\tPICK_RESULT_ERROR = -1,\n+\tPICK_RESULT_OK,\n+\tPICK_RESULT_CONFLICTS,\n+};\n+\n+static enum pick_result do_pick_commit(struct repository *r,\n+\t\t\t\t struct todo_item *item,\n+\t\t\t\t struct replay_opts *opts,\n+\t\t\t\t int final_fixup, int *check_todo)\n {\n \tstruct replay_ctx *ctx = opts->ctx;\n \tunsigned int flags = should_edit(opts) ? EDIT_MSG : 0;\n@@ -2564,7 +2570,12 @@ static int do_pick_commit(struct repository *r,\n \tfree(author);\n \tupdate_abort_safety_file();\n \n-\treturn res;\n+\tif (res < 0)\n+\t\treturn PICK_RESULT_ERROR;\n+\telse if (res > 0)\n+\t\treturn PICK_RESULT_CONFLICTS;\n+\telse\n+\t\treturn PICK_RESULT_OK;\n }\n \n static int prepare_revs(struct replay_opts *opts)\n@@ -4960,37 +4971,47 @@ static int pick_one_commit(struct repository *r,\n \t\t\t struct replay_opts *opts,\n \t\t\t int *check_todo, int* reschedule)\n {\n-\tint res;\n+\tenum pick_result pick_res;\n \tstruct todo_item *item = todo_list->items + todo_list->current;\n \tconst char *arg = todo_item_get_arg(todo_list, item);\n \n-\tres = do_pick_commit(r, item, opts, is_final_fixup(todo_list),\n-\t\t\t check_todo);\n+\tpick_res = do_pick_commit(r, item, opts, is_final_fixup(todo_list),\n+\t\t\t\t check_todo);\n \tif (!is_rebase_i(opts))\n-\t\treturn res;\n+\t\tswitch (pick_res) {\n+\t\tcase PICK_RESULT_ERROR:\n+\t\t\treturn -1;\n+\t\tcase PICK_RESULT_CONFLICTS:\n+\t\t\treturn 1;\n+\t\tdefault:\n+\t\t\treturn 0;\n+\t\t}\n \n-\tif (res < 0) {\n+\tif (pick_res == PICK_RESULT_ERROR) {\n \t\t/* Reschedule */\n \t\t*reschedule = 1;\n \t\treturn -1;\n \t} else if (item->command == TODO_EDIT) {\n \t\tstruct commit *commit = item->commit;\n-\t\tif (!res) {\n+\t\tint res = pick_res == PICK_RESULT_CONFLICTS;\n+\n+\t\tif (pick_res == PICK_RESULT_OK) {\n \t\t\tif (!opts->verbose)\n \t\t\t\tterm_clear_line();\n \t\t\tfprintf(stderr, _(\"Stopped at %s... %.*s\\n\"),\n \t\t\t\tshort_commit_name(r, commit), item->arg_len, arg);\n \t\t}\n \t\treturn error_with_patch(r, commit,\n \t\t\t\t\targ, item->arg_len, opts, res, !res);\n-\t} else if (!res) {\n+\t} else if (pick_res == PICK_RESULT_OK) {\n \t\trecord_in_rewritten(&item->commit->object.oid,\n \t\t\t\t peek_command(todo_list, 1));\n \t\treturn 0;\n-\t} else if (res && is_fixup(item->command)) {\n+\t} else if (pick_res == PICK_RESULT_CONFLICTS &&\n+\t\t is_fixup(item->command)) {\n \t\treturn error_failed_squash(r, item->commit, opts,\n \t\t\t\t\t item->arg_len, arg);\n-\t} else if (res) {\n+\t} else if (pick_res == PICK_RESULT_CONFLICTS) {\n \t\tint to_amend = 0;\n \t\tstruct object_id oid;\n \n@@ -5008,7 +5029,7 @@ static int pick_one_commit(struct repository *r,\n \t\t\tto_amend = 1;\n \n \t\treturn error_with_patch(r, item->commit, arg, item->arg_len,\n-\t\t\t\t\topts, res, to_amend);\n+\t\t\t\t\topts, 1, to_amend);\n \t}\n \n \tBUG(\"Unhandled return value from do_pick_commit()\");\n@@ -5547,7 +5568,15 @@ static int single_pick(struct repository *r,\n \t\t\tTODO_PICK : TODO_REVERT;\n \titem.commit = cmit;\n \n-\treturn do_pick_commit(r, &item, opts, 0, &check_todo);\n+\tswitch (do_pick_commit(r, &item, opts, 0, &check_todo)) {\n+\tcase PICK_RESULT_ERROR:\n+\t\treturn -1;\n+\tcase PICK_RESULT_CONFLICTS:\n+\t\treturn 1;\n+\tdefault:\n+\t\treturn 0;\n+\t}\n+\n }\n \n int sequencer_pick_revisions(struct repository *r,\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"548298","messageId":"2ef36b9ee5a399e1922e9b4620b04d33d0b10f02.1784128921.git.phillip.wood@dunelm.org.uk","threadId":"65820","inReplyTo":"cover.1784128921.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v3 9/9] sequencer: do not record dropped commits as rewritten","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-15T15:22:03Z","receivedAt":"2026-07-15T15:22:29Z","isPatch":true,"body":"From: Phillip Wood \n\nIf a commit gets dropped because its changes are already upstream\nthen we should not record it as rewritten. As well as confusing any\npost-rewrite hooks, it means we end up copying the notes from the\ndropped commit to the commit that was picked immediately before the\none that was dropped.\n\nWhile we do not want to record the dropped commit as rewritten, if\nit is the final commit in a chain of fixups then we need to flush\nthe list of rewritten commits. The behavior of an \"edit\" command\nwhere the commit is dropped is changed so that \"rebase --continue\"\nwill not amend the previous pick. However, as the code comment notes\nit will still be erroneously recorded as rewritten when the rebase\ncontinues. That will need to be addressed separately along with not\nrecording skipped commits as rewritten.\n\nThe initialization of \"drop_commit\" is moved to ensure it is initialized\nwhen rewording a fast-forwarded commit.\n\nReported-by: Uwe Kleine-König \nTested-by: Uwe Kleine-König \nSigned-off-by: Phillip Wood \n---\n sequencer.c | 24 +++++++++++++++++++-----\n t/t3400-rebase.sh | 12 ++++++++++++\n t/t5407-post-rewrite-hook.sh | 23 +++++++++++++++++++++++\n 3 files changed, 54 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 4b3092dc9bb..7a5898b215d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2264,6 +2264,7 @@ enum pick_result {\n \tPICK_RESULT_ERROR = -1,\n \tPICK_RESULT_OK,\n \tPICK_RESULT_CONFLICTS,\n+\tPICK_RESULT_DROPPED,\n };\n \n static enum pick_result do_pick_commit(struct repository *r,\n@@ -2279,7 +2280,7 @@ static enum pick_result do_pick_commit(struct repository *r,\n \tconst char *base_label, *next_label, *reflog_action;\n \tchar *author = NULL;\n \tstruct commit_message msg = { NULL, NULL, NULL, NULL };\n-\tint res, unborn = 0, reword = 0, allow, drop_commit;\n+\tint res, unborn = 0, reword = 0, allow, drop_commit = 0;\n \tenum todo_command command = item->command;\n \tstruct commit *commit = item->commit;\n \n@@ -2509,7 +2510,6 @@ static enum pick_result do_pick_commit(struct repository *r,\n \t\tgoto leave;\n \t}\n \n-\tdrop_commit = 0;\n \tallow = allow_empty(r, opts, commit);\n \tif (allow < 0) {\n \t\tres = allow;\n@@ -2574,6 +2574,8 @@ static enum pick_result do_pick_commit(struct repository *r,\n \t\treturn PICK_RESULT_ERROR;\n \telse if (res > 0)\n \t\treturn PICK_RESULT_CONFLICTS;\n+\telse if (drop_commit)\n+\t\treturn PICK_RESULT_DROPPED;\n \telse\n \t\treturn PICK_RESULT_OK;\n }\n@@ -4994,18 +4996,30 @@ static int pick_one_commit(struct repository *r,\n \t} else if (item->command == TODO_EDIT) {\n \t\tstruct commit *commit = item->commit;\n \t\tint res = pick_res == PICK_RESULT_CONFLICTS;\n+\t\tint to_amend = pick_res != PICK_RESULT_CONFLICTS &&\n+\t\t\t\tpick_res != PICK_RESULT_DROPPED;\n \n-\t\tif (pick_res == PICK_RESULT_OK) {\n+\t\t/*\n+\t\t * NEEDSWORK: Do not record the commit as rewritten when\n+\t\t * continuing if it was dropped. Does it even make sense\n+\t\t * to stop if the commit was dropped?\n+\t\t */\n+\t\tif (pick_res == PICK_RESULT_OK ||\n+\t\t pick_res == PICK_RESULT_DROPPED) {\n \t\t\tif (!opts->verbose)\n \t\t\t\tterm_clear_line();\n \t\t\tfprintf(stderr, _(\"Stopped at %s... %.*s\\n\"),\n \t\t\t\tshort_commit_name(r, commit), item->arg_len, arg);\n \t\t}\n-\t\treturn error_with_patch(r, commit,\n-\t\t\t\t\targ, item->arg_len, opts, res, !res);\n+\t\treturn error_with_patch(r, commit, arg, item->arg_len, opts,\n+\t\t\t\t\tres, to_amend);\n \t} else if (pick_res == PICK_RESULT_OK) {\n \t\trecord_in_rewritten(&item->commit->object.oid,\n \t\t\t\t peek_command(todo_list, 1));\n+\t\treturn 0;\n+\t} else if (pick_res == PICK_RESULT_DROPPED) {\n+\t\tif (is_final_fixup(todo_list))\n+\t\t\tflush_rewritten_pending();\n \t\treturn 0;\n \t} else if (pick_res == PICK_RESULT_CONFLICTS &&\n \t\t is_fixup(item->command)) {\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex f0e7fcf649a..1d09886ea35 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -274,6 +274,18 @@ test_expect_success 'rebase --apply can copy notes' '\n \tgit reset --hard n3 &&\n \tgit rebase --apply --onto n1 n2 &&\n \ttest \"a note\" = \"$(git notes show HEAD)\"\n+'\n+\n+test_expect_success 'rebase drops notes of dropped commits' '\n+\tgit checkout n1 &&\n+\techo n3 >n3.t &&\n+\techo n4 >n4.t &&\n+\tgit add n3.t n4.t &&\n+\tgit commit -m n34 &&\n+\tgit rebase HEAD n3 &&\n+\ttest_commit_message HEAD -m n2 &&\n+\ttest_must_fail git notes list HEAD >actual &&\n+\ttest_must_be_empty actual\n '\n \n test_expect_success 'rebase commit with an ancient timestamp' '\ndiff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh\nindex ad7f8c6f002..51991956d1d 100755\n--- a/t/t5407-post-rewrite-hook.sh\n+++ b/t/t5407-post-rewrite-hook.sh\n@@ -306,6 +306,29 @@ test_expect_success 'git rebase -i (exec)' '\n \tcat >expected.data <<-EOF &&\n \t$(git rev-parse C) $(git rev-parse HEAD^)\n \t$(git rev-parse D) $(git rev-parse HEAD)\n+\tEOF\n+\tverify_hook_input\n+'\n+\n+test_expect_success 'rebase with commits that become empty' '\n+\tcat >todo <<-\\EOF &&\n+\tpick H\n+\tpick E\n+\tfixup I\n+\tfixup H\n+\tpick G\n+\tpick I\n+\tEOF\n+\t(\n+\t\tset_replace_editor todo &&\n+\t\tgit rebase -i --empty=drop A A\n+\t) &&\n+\techo rebase >expected.args &&\n+\tcat >expected.data <<-EOF &&\n+\t$(git rev-parse H) $(git rev-parse HEAD~2)\n+\t$(git rev-parse E) $(git rev-parse HEAD~1)\n+\t$(git rev-parse I) $(git rev-parse HEAD~1)\n+\t$(git rev-parse G) $(git rev-parse HEAD)\n \tEOF\n \tverify_hook_input\n '\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"548318","messageId":"xmqqo6g8m6da.fsf@gitster.g","threadId":"65820","inReplyTo":"6cdccc2b-c0b4-497f-8408-a18bd0981505@gmail.com","subject":"Re: [PATCH v2 03/10] sequencer: be more careful with external merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-15T18:53:53Z","receivedAt":"2026-07-15T18:53:56Z","isPatch":true,"body":"Phillip Wood writes:\n\n> On 15/07/2026 10:35, Phillip Wood wrote:\n>> Hi Oswald\n>> \n>> On 13/07/2026 15:01, Oswald Buddenhagen wrote:\n>>> On Mon, Jul 13, 2026 at 02:17:20PM +0100, Phillip Wood wrote:\n>>>> If an external merge strategy cannot merge (for example because it\n>>>> would overwrite an untracked file) it exits with a non-zero exit\n>>>> code other than 1. This should be treated differently to a merge\n>>>>\n>>> s/to/from/, i think?\n>> \n>> Both are valid - the internet tells be \"different to\" is more common it \n>\n> sigh s/be/me/\n>\n> Phillip\n\nsigh s/it/in/ ;-)\n\n>\n>> British English, whereas \"different from\" is more common in American \n>> English. I guess for an international audience \"from\" would be the \n>> better choice.\n"},{"id":"548575","messageId":"als4huLvpnHsl_Mi@monoceros","threadId":"65820","inReplyTo":"akSuP-IWiH2wPd6S@monoceros","subject":"Re: [PATCH 00/11] sequencer: do not record dropped commits as rewritten","fromName":"Uwe Kleine-König","fromEmail":"u.kleine-koenig@baylibre.com","sentAt":"2026-07-18T08:37:54Z","receivedAt":"2026-07-18T08:37:59Z","isPatch":true,"body":"Hello,\n\nOn Wed, Jul 01, 2026 at 11:38:27AM +0200, Uwe Kleine-König wrote:\n> On Tue, Jun 30, 2026 at 04:28:50PM +0100, Phillip Wood wrote:\n> > On 19/06/2026 11:13, Phillip Wood wrote:\n> > > I'm happy to take this forward and try and fix at least some of the\n> > > other bugs I've listed above. Uwe - if I don't cc you on some patches\n> > > within the next couple of weeks please feel free to send a reminder.\n> > \n> > Here is the first batch that fixes the same problem as Uwe's patch. I've\n> > taken a slightly different approach that uses the return value from\n> > do_pick_commit() to signal that a commit was dropped rather than\n> > adding another function argument. That involves a number of preparatory\n> > patches, but they are hopefully reasonably small and easy to follow.\n> > \n> > If a commit gets dropped because its changes are already upstream\n> > then we should not record it as rewritten. As well as confusing any\n> > post-rewrite hooks this means we end up copying the notes from the\n> > dropped commit to the commit that was picked immediately before the\n> > one that was dropped.\n> > \n> > This series is structured as follows:\n> > \n> > Patch 1 restores some test coverage that was lost when the default\n> > rebase backend was changed.\n> > \n> > Patch 2 moves a function so it can be called without a forward\n> > declaration in Patch 11.\n> > \n> > Patches 3 & 4 fix the return value of do_pick_commit() when an external\n> > command fails (this is in preparation for patch 10).\n> > \n> > Patches 5-9 try and simplify the control flow in pick_one_commit()\n> > in preparation for patch 10.\n> > \n> > Patch 10 changes the return type of do_pick_commit() to an enum.\n> > \n> > Patch 11 adds a new member to the enum from patch 10 for commits that\n> > are dropped when they become empty and uses that to stop them from\n> > being recorded as rewritten.\n> \n> With my very little knowledge about git internals, this looks\n> reasonable, and it behaves as I expect in my test case. I installed a\n> local \n> \n> Tested-by: Uwe Kleine-König \n\nWhile it works fine in my test case, it doesn't in my real-life\nworkflow.\n\nI have a big branch of changes that I maintain on top of next/master, on\ntodays rebase I experience:\n\n\tuwe@monoceros:~/gsrc/linux-2nd$ git rebase --onto=next-20260717 next-20260716 -r -i device_id^{}\n\t... handling commits that get empty using `git rebase --skip` ...\n\n\tuwe@monoceros:~/gsrc/linux-2nd$ git range-diff next-20260716..device_id next-20260717..\n\t...\n\t 24: 901ca5f67bc5 ! 24: 9f3e8813f6b4 mtd: nand-omap2: Move omap_nand_ids[] to raw nand driver\n\t @@ Commit message\n\t ## Notes ##\n\t\t Forwarded: id:901ca5f67bc57219a9222115fabe1a1729b87e25.1784229863.git.ukleinek@kernel.org\n\n\t + Forwarded: id:20260716123646.1933293-2-u.kleine-koenig@baylibre.com\n\t +\n\t ## drivers/memory/omap-gpmc.c ##\n\t @@ drivers/memory/omap-gpmc.c: static void __maybe_unused gpmc_read_timings_dt(struct device_node *np,\n\t\t\tof_property_read_bool(np, \"gpmc,time-para-granularity\");\n\t 25: 69be5d4f9f13 < -: ------------ drm/radeon: Only define radeon_acpi_vfct_match when actually used\n\t...\n\nwith:\n\n\tuwe@monoceros:~/gsrc/linux-2nd$ git notes show 69be5d4f9f13\n\tForwarded: id:20260716123646.1933293-2-u.kleine-koenig@baylibre.com\n\nWhen I rebase without -i, the rebase happens without hitting empty\ncommits that I have to manually skip and then the notes for 69be5d4f9f13\ndoesn't make it into the neighbour commit after rebase.\n\nSo it seems there is still something fishy with interactive rebase.\n\nBest regards\nUwe\n"},{"id":"548580","messageId":"82527cd3-b3b3-4cc6-80c6-b5833b262c83@gmail.com","threadId":"65820","inReplyTo":"als4huLvpnHsl_Mi@monoceros","subject":"Re: [PATCH 00/11] sequencer: do not record dropped commits as rewritten","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-18T09:22:23Z","receivedAt":"2026-07-18T09:22:30Z","isPatch":true,"body":"Hi Uwe\n\nOn 18/07/2026 09:37, Uwe Kleine-König wrote:\n> \n> While it works fine in my test case, it doesn't in my real-life\n> workflow.\n> \n> I have a big branch of changes that I maintain on top of next/master, on\n> todays rebase I experience:\n> \n> \tuwe@monoceros:~/gsrc/linux-2nd$ git rebase --onto=next-20260717 next-20260716 -r -i device_id^{}\n> \t... handling commits that get empty using `git rebase --skip` ...\n> \n> \tuwe@monoceros:~/gsrc/linux-2nd$ git range-diff next-20260716..device_id next-20260717..\n> \t...\n> \t 24: 901ca5f67bc5 ! 24: 9f3e8813f6b4 mtd: nand-omap2: Move omap_nand_ids[] to raw nand driver\n> \t @@ Commit message\n> \t ## Notes ##\n> \t\t Forwarded: id:901ca5f67bc57219a9222115fabe1a1729b87e25.1784229863.git.ukleinek@kernel.org\n> \n> \t + Forwarded: id:20260716123646.1933293-2-u.kleine-koenig@baylibre.com\n> \t +\n> \t ## drivers/memory/omap-gpmc.c ##\n> \t @@ drivers/memory/omap-gpmc.c: static void __maybe_unused gpmc_read_timings_dt(struct device_node *np,\n> \t\t\tof_property_read_bool(np, \"gpmc,time-para-granularity\");\n> \t 25: 69be5d4f9f13 < -: ------------ drm/radeon: Only define radeon_acpi_vfct_match when actually used\n> \t...\n> \n> with:\n> \n> \tuwe@monoceros:~/gsrc/linux-2nd$ git notes show 69be5d4f9f13\n> \tForwarded: id:20260716123646.1933293-2-u.kleine-koenig@baylibre.com\n> \n> When I rebase without -i, the rebase happens without hitting empty\n> commits that I have to manually skip and then the notes for 69be5d4f9f13\n> doesn't make it into the neighbour commit after rebase.\n> \n> So it seems there is still something fishy with interactive rebase.\n\nFor historic reasons \"-i\" implies \"--empty=ask\", without \"-i\" the \ndefault \"--empty=drop\" (the UI is a mess). This patch series only stops \ncommits that are dropped by \"--empty=drop\" from being recorded as \nrewritten, so it will only have an effect with \"-i\" if you add \n\"--empty=drop\". I'm still thinking about how to handle commits that are \ndropped by the user, for example when when they run \"git rebase --skip\" \nafter a conflict, or they run \"git rebase --continue\" without committing \nafter a commit that becomes empty with \"--empty=ask\". As an aside I \nreally wish \"--empty=ask\" kept the empty commit on \"git rebase \n--continue\" and dropped it on \"git rebase --skip\" but the current \nbehavior dates from the early days of git.\n\nThanks\n\nPhillip\n"},{"id":"548631","messageId":"xmqqecgyn5gk.fsf@gitster.g","threadId":"65820","inReplyTo":"cover.1784128921.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH v3 0/9] sequencer: do not record dropped commits as rewritten","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-19T19:29:31Z","receivedAt":"2026-07-19T19:29:33Z","isPatch":true,"body":"Phillip Wood writes:\n\n> Thanks to everyone who commented on v2. I've dropped patch 2 which\n> Andrei pointed out was pointless and tried to make the remaining\n> commit messages clearer as requested by Oswald.\n>\n> If a commit gets dropped because its changes are already upstream\n> then we should not record it as rewritten. As well as confusing any\n> post-rewrite hooks this means we end up copying the notes from the\n> dropped commit to the commit that was picked immediately before the\n> one that was dropped.\n>\n> This series is structured as follows:\n>\n> Patch 1 restores some test coverage that was lost when the default\n> rebase backend was changed.\n>\n> Patches 2 & 3 fix the return value of do_pick_commit() when an external\n> command fails (this is in preparation for patch 8).\n>\n> Patches 4-7 try and simplify the control flow in pick_one_commit()\n> in preparation for patch 8.\n>\n> Patch 8 changes the return type of do_pick_commit() to an enum.\n>\n> Patch 9 adds a new member to the enum from patch 8 for commits that\n> are dropped when they become empty and uses that to stop them from\n> being recorded as rewritten.\n\nI see Phillip Cc'ed everybody who participated in the review for the\nprevious iterations, which is very much appreciated.\n\nIt looks like this is now ready to go? Any further comments?\n\nThanks.\n"},{"id":"548677","messageId":"al4RYuWKqAr-IlFC@ugly.lan","threadId":"65820","inReplyTo":"xmqqecgyn5gk.fsf@gitster.g","subject":"Re: [PATCH v3 0/9] sequencer: do not record dropped commits as rewritten","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2026-07-20T12:15:30Z","receivedAt":"2026-07-20T12:15:36Z","isPatch":true,"body":"On Sun, Jul 19, 2026 at 12:29:31PM -0700, Junio C Hamano wrote:\n>It looks like this is now ready to go? Any further comments?\n>\nyou can add whatever footer is appropriate for \"i read it, it seems to \nmake sense, but i didn't double-check\" for me.\n\n(same for phillip's new 2-patch series.)\n\n(it feels silly to \"spam\" the list with such low-value verdicts. i \nreally miss gerrit code review here, where i'd leave a +1 in passing.)\n"},{"id":"548684","messageId":"xmqqy0f5d25g.fsf@gitster.g","threadId":"65820","inReplyTo":"al4RYuWKqAr-IlFC@ugly.lan","subject":"Re: [PATCH v3 0/9] sequencer: do not record dropped commits as rewritten","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-20T17:03:23Z","receivedAt":"2026-07-20T17:03:26Z","isPatch":true,"body":"Oswald Buddenhagen writes:\n\n> On Sun, Jul 19, 2026 at 12:29:31PM -0700, Junio C Hamano wrote:\n>>It looks like this is now ready to go? Any further comments?\n>>\n> you can add whatever footer is appropriate for \"i read it, it seems to \n> make sense, but i didn't double-check\" for me.\n>\n> (same for phillip's new 2-patch series.)\n>\n> (it feels silly to \"spam\" the list with such low-value verdicts. i \n> really miss gerrit code review here, where i'd leave a +1 in passing.)\n\nActually, reducing the signal to a single bit, 'did I or did I not\nsee a +1 from them?', means Gerrit users see less 'spam' but must\nmake decisions based on too little signal. I do not know whether\nthat is an advantage.\n\nWith your email, we can at least discern that your comment is much\ncloser to an 'Acked-by' than a 'Reviewed-by', and we can respect\nthat distinction when judging whether there is sufficient consensus\non the list to move the topic forward.\n\nIn any case, thank you for reading it over and letting us know that\nyou found nothing glaringly wrong. That is indeed valuable\ninformation.\n\nThanks.\n"},{"id":"548694","messageId":"al6UqtgUtZb3bqMi@ugly.lan","threadId":"65820","inReplyTo":"xmqqy0f5d25g.fsf@gitster.g","subject":"gerrit code review once more (was: Re: [PATCH v3 0/9] sequencer: do not record dropped commits as) rewritten","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2026-07-20T21:35:38Z","receivedAt":"2026-07-20T21:35:43Z","isPatch":true,"body":"On Mon, Jul 20, 2026 at 10:03:23AM -0700, Junio C Hamano wrote:\n>Actually, reducing the signal to a single bit, 'did I or did I not\n>see a +1 from them?', means Gerrit users see less 'spam' but must\n>make decisions based on too little signal. I do not know whether\n>that is an advantage.\n>\n>With your email, we can at least discern that your comment is much\n>closer to an 'Acked-by' than a 'Reviewed-by', and we can respect\n>that distinction when judging whether there is sufficient consensus\n>on the list to move the topic forward.\n>\ngerrit discerns from -2 to +2 (*), so that angle is covered (**).\nhttps://gerrit-review.googlesource.com/Documentation/config-labels.html#label_Code-Review\n\n(*) actually however many levels the project chooses to configure, \nthough things aren't as smooth when deviating from the defaults\n\n(**) mostly - https://issues.gerritcodereview.com/issues/40000793\n"},{"id":"548782","messageId":"2c76d406-62d2-4e55-8f14-5b5f8045ae10@gmail.com","threadId":"65820","inReplyTo":"al4RYuWKqAr-IlFC@ugly.lan","subject":"Re: [PATCH v3 0/9] sequencer: do not record dropped commits as rewritten","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-22T15:15:41Z","receivedAt":"2026-07-22T15:15:47Z","isPatch":true,"body":"Hi Oswald\n\nOn 20/07/2026 13:15, Oswald Buddenhagen wrote:\n> On Sun, Jul 19, 2026 at 12:29:31PM -0700, Junio C Hamano wrote:\n>> It looks like this is now ready to go?  Any further comments?\n>>\n> you can add whatever footer is appropriate for \"i read it, it seems to \n> make sense, but i didn't double-check\" for me.\n> \n> (same for phillip's new 2-patch series.)\n\nThanks for reading them through - I'm glad to hear the commit messages \nmake sense now.\n\nPhillip\n\n> (it feels silly to \"spam\" the list with such low-value verdicts. i \n> really miss gerrit code review here, where i'd leave a +1 in passing.)\n> \n\n"}]}