* [PATCH 11/11] sequencer: do not record dropped commits as rewritten
From: Phillip Wood @ 2026-06-30 15:29 UTC (permalink / raw)
To: git; +Cc: Uwe Kleine-König, Junio C Hamano, Phillip Wood
In-Reply-To: <cover.1782833268.git.phillip.wood@dunelm.org.uk>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
If a commit gets dropped because its changes are already upstream
then we should not record it as rewritten. As well as confusing any
post-rewrite hooks this means we end up copying the notes from the
dropped commit to the commit that was picked immediately before the
one that was dropped.
While we do not want to record the dropped commit is rewritten, if
it is the final commit in a chain of fixups then we need to flush
the list of rewritten commits. The behavior of an "edit" command
where the commit is dropped is changed so that "rebase --continue"
will not amend the previous pick. However, as the code comment notes
it will still be erroneously recorded as rewritten when the rebase
continues. That will need to be addressed separately along with not
recording skipped commits as rewritten.
The initialization of "drop_commit" is moved to ensure it is initialized
when rewording a fast-forwarded commit.
Reported-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 24 +++++++++++++++++++-----
t/t3400-rebase.sh | 12 ++++++++++++
t/t5407-post-rewrite-hook.sh | 23 +++++++++++++++++++++++
3 files changed, 54 insertions(+), 5 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index ca005b969c4..a85f9e8b77d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2264,6 +2264,7 @@ enum pick_result {
PICK_RESULT_ERROR = -1,
PICK_RESULT_OK,
PICK_RESULT_CONFLICTS,
+ PICK_RESULT_DROPPED,
};
static enum pick_result do_pick_commit(struct repository *r,
@@ -2279,7 +2280,7 @@ static enum pick_result do_pick_commit(struct repository *r,
const char *base_label, *next_label, *reflog_action;
char *author = NULL;
struct commit_message msg = { NULL, NULL, NULL, NULL };
- int res, unborn = 0, reword = 0, allow, drop_commit;
+ int res, unborn = 0, reword = 0, allow, drop_commit = 0;
enum todo_command command = item->command;
struct commit *commit = item->commit;
@@ -2509,7 +2510,6 @@ static enum pick_result do_pick_commit(struct repository *r,
goto leave;
}
- drop_commit = 0;
allow = allow_empty(r, opts, commit);
if (allow < 0) {
res = allow;
@@ -2574,6 +2574,8 @@ static enum pick_result do_pick_commit(struct repository *r,
return PICK_RESULT_ERROR;
else if (res > 0)
return PICK_RESULT_CONFLICTS;
+ else if (drop_commit)
+ return PICK_RESULT_DROPPED;
else
return PICK_RESULT_OK;
}
@@ -4994,18 +4996,30 @@ static int pick_one_commit(struct repository *r,
} else if (item->command == TODO_EDIT) {
struct commit *commit = item->commit;
int res = pick_res == PICK_RESULT_CONFLICTS;
+ int to_amend = pick_res != PICK_RESULT_CONFLICTS &&
+ pick_res != PICK_RESULT_DROPPED;
- if (pick_res == PICK_RESULT_OK) {
+ /*
+ * NEEDSWORK: Do not record the commit as rewritten when
+ * continuing if it was dropped. Does it even make sense
+ * to stop if the commit was dropped?
+ */
+ if (pick_res == PICK_RESULT_OK ||
+ pick_res == PICK_RESULT_DROPPED) {
if (!opts->verbose)
term_clear_line();
fprintf(stderr, _("Stopped at %s... %.*s\n"),
short_commit_name(r, commit), item->arg_len, arg);
}
- return error_with_patch(r, commit,
- arg, item->arg_len, opts, res, !res);
+ return error_with_patch(r, commit, arg, item->arg_len, opts,
+ res, to_amend);
} else if (pick_res == PICK_RESULT_OK) {
record_in_rewritten(&item->commit->object.oid,
peek_command(todo_list, 1));
+ return 0;
+ } else if (pick_res == PICK_RESULT_DROPPED) {
+ if (is_final_fixup(todo_list))
+ flush_rewritten_pending();
return 0;
} else if (pick_res == PICK_RESULT_CONFLICTS &&
is_fixup(item->command)) {
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index f0e7fcf649a..1d09886ea35 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -274,6 +274,18 @@ test_expect_success 'rebase --apply can copy notes' '
git reset --hard n3 &&
git rebase --apply --onto n1 n2 &&
test "a note" = "$(git notes show HEAD)"
+'
+
+test_expect_success 'rebase drops notes of dropped commits' '
+ git checkout n1 &&
+ echo n3 >n3.t &&
+ echo n4 >n4.t &&
+ git add n3.t n4.t &&
+ git commit -m n34 &&
+ git rebase HEAD n3 &&
+ test_commit_message HEAD -m n2 &&
+ test_must_fail git notes list HEAD >actual &&
+ test_must_be_empty actual
'
test_expect_success 'rebase commit with an ancient timestamp' '
diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
index ad7f8c6f002..51991956d1d 100755
--- a/t/t5407-post-rewrite-hook.sh
+++ b/t/t5407-post-rewrite-hook.sh
@@ -306,6 +306,29 @@ test_expect_success 'git rebase -i (exec)' '
cat >expected.data <<-EOF &&
$(git rev-parse C) $(git rev-parse HEAD^)
$(git rev-parse D) $(git rev-parse HEAD)
+ EOF
+ verify_hook_input
+'
+
+test_expect_success 'rebase with commits that become empty' '
+ cat >todo <<-\EOF &&
+ pick H
+ pick E
+ fixup I
+ fixup H
+ pick G
+ pick I
+ EOF
+ (
+ set_replace_editor todo &&
+ git rebase -i --empty=drop A A
+ ) &&
+ echo rebase >expected.args &&
+ cat >expected.data <<-EOF &&
+ $(git rev-parse H) $(git rev-parse HEAD~2)
+ $(git rev-parse E) $(git rev-parse HEAD~1)
+ $(git rev-parse I) $(git rev-parse HEAD~1)
+ $(git rev-parse G) $(git rev-parse HEAD)
EOF
verify_hook_input
'
--
2.54.0.200.gfd8d68259e3
^ permalink raw reply related
* [PATCH 10/11] sequencer: use an enum to represent result of picking a commit
From: Phillip Wood @ 2026-06-30 15:29 UTC (permalink / raw)
To: git; +Cc: Uwe Kleine-König, Junio C Hamano, Phillip Wood
In-Reply-To: <cover.1782833268.git.phillip.wood@dunelm.org.uk>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
Rather than using an integer where -1 is an error, 0 is success and
1 means there were conflicts use an enum. This is clearer and lets
us add a separate return value for commits that are dropped because
they become empty in the next commit.
Note we continue to use "return error(...)" to return errors and
take advantage of C's lax typing of enums
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 61 +++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 45 insertions(+), 16 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 655a2e84bef..ca005b969c4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2260,10 +2260,16 @@ static const char *reflog_message(struct replay_opts *opts,
return buf.buf;
}
-static int do_pick_commit(struct repository *r,
- struct todo_item *item,
- struct replay_opts *opts,
- int final_fixup, int *check_todo)
+enum pick_result {
+ PICK_RESULT_ERROR = -1,
+ PICK_RESULT_OK,
+ PICK_RESULT_CONFLICTS,
+};
+
+static enum pick_result do_pick_commit(struct repository *r,
+ struct todo_item *item,
+ struct replay_opts *opts,
+ int final_fixup, int *check_todo)
{
struct replay_ctx *ctx = opts->ctx;
unsigned int flags = should_edit(opts) ? EDIT_MSG : 0;
@@ -2564,7 +2570,12 @@ static int do_pick_commit(struct repository *r,
free(author);
update_abort_safety_file();
- return res;
+ if (res < 0)
+ return PICK_RESULT_ERROR;
+ else if (res > 0)
+ return PICK_RESULT_CONFLICTS;
+ else
+ return PICK_RESULT_OK;
}
static int prepare_revs(struct replay_opts *opts)
@@ -4960,37 +4971,47 @@ static int pick_one_commit(struct repository *r,
struct replay_opts *opts,
int *check_todo, int* reschedule)
{
- int res;
+ enum pick_result pick_res;
struct todo_item *item = todo_list->items + todo_list->current;
const char *arg = todo_item_get_arg(todo_list, item);
- res = do_pick_commit(r, item, opts, is_final_fixup(todo_list),
- check_todo);
+ pick_res = do_pick_commit(r, item, opts, is_final_fixup(todo_list),
+ check_todo);
if (!is_rebase_i(opts))
- return res;
+ switch (pick_res) {
+ case PICK_RESULT_ERROR:
+ return -1;
+ case PICK_RESULT_CONFLICTS:
+ return 1;
+ default:
+ return 0;
+ }
- if (res < 0) {
+ if (pick_res == PICK_RESULT_ERROR) {
/* Reschedule */
*reschedule = 1;
return -1;
} else if (item->command == TODO_EDIT) {
struct commit *commit = item->commit;
- if (!res) {
+ int res = pick_res == PICK_RESULT_CONFLICTS;
+
+ if (pick_res == PICK_RESULT_OK) {
if (!opts->verbose)
term_clear_line();
fprintf(stderr, _("Stopped at %s... %.*s\n"),
short_commit_name(r, commit), item->arg_len, arg);
}
return error_with_patch(r, commit,
arg, item->arg_len, opts, res, !res);
- } else if (!res) {
+ } else if (pick_res == PICK_RESULT_OK) {
record_in_rewritten(&item->commit->object.oid,
peek_command(todo_list, 1));
return 0;
- } else if (res && is_fixup(item->command)) {
+ } else if (pick_res == PICK_RESULT_CONFLICTS &&
+ is_fixup(item->command)) {
return error_failed_squash(r, item->commit, opts,
item->arg_len, arg);
- } else if (res) {
+ } else if (pick_res == PICK_RESULT_CONFLICTS) {
int to_amend = 0;
struct object_id oid;
@@ -5008,7 +5029,7 @@ static int pick_one_commit(struct repository *r,
to_amend = 1;
return error_with_patch(r, item->commit, arg, item->arg_len,
- opts, res, to_amend);
+ opts, 1, to_amend);
}
BUG("Unhandled return value from do_pick_commit()");
@@ -5547,7 +5568,15 @@ static int single_pick(struct repository *r,
TODO_PICK : TODO_REVERT;
item.commit = cmit;
- return do_pick_commit(r, &item, opts, 0, &check_todo);
+ switch (do_pick_commit(r, &item, opts, 0, &check_todo)) {
+ case PICK_RESULT_ERROR:
+ return -1;
+ case PICK_RESULT_CONFLICTS:
+ return 1;
+ default:
+ return 0;
+ }
+
}
int sequencer_pick_revisions(struct repository *r,
--
2.54.0.200.gfd8d68259e3
^ permalink raw reply related
* Re: [PATCH v4 1/1] environment: move excludes_file into repo_config_values
From: Tian Yuchen @ 2026-06-30 16:20 UTC (permalink / raw)
To: Junio C Hamano, Christian Couder
Cc: git, cirnovskyv, szeder.dev, Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <xmqqbjctz9mh.fsf@gitster.g>
Hi Christian and Junio,
On 6/29/26 22:47, Junio C Hamano wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> I agree that the best end state would be to have no `if (!repo ||
>> !repo->initialized)` check, but we shouldn't have to get there right
>> away. I think it's fine to proceed in several steps:
>>
>> 1) `if (!repo || !repo->initialized) return NULL;` allows us to remove
>> the global variable and use getters which will help us in the next
>> step.
>>
>> 2) `if (!repo || !repo->initialized) return BUG("repo must be an
>> initialized repository");` now we want to find and fix callers
>> (including tests) that haven't properly initialized things.
>>
>> 3) No `if (!repo || !repo->initialized)` check, as we are sure that
>> all the callers that didn't properly initialized things have been
>> found and fixed.
>>
>> So I think 1) is fine for now as long as we properly explain in the
>> commit messages and in code comments (maybe using NEEDSWORK comments)
>> that we know there is more work to do on this in the future.
>
> I am OK with the progressive improvements, but if "wean us away from
> global variables" topic stops at step 1 I would have to say that is
> a failed experiment. Not doing (2) means you made the system bug-to-bug
> compatible from the old world where these things weren't in repo-config
> but were still globals, which is code churn for nothing good to show
> in the end result. We need to get to (2) before declaring victory.
>
> But of course, we need to start somewhere. (1) with in-code
> comments sprinkled to such places that say that we'd need to revisit
> would be a good place to start.
>
> Thanks.
Thank you both for paving a clear way forward.
For the upcoming V5 patch, I will implement:
1. Revert to the shields ('return NULL' for the getter, and bypassing
'repo != repository' for the _clear()).
2. Add 'NEEDSWORK' comments above them documenting that these are
temporary changes.
3. Update the commit message to reflect this.
Once this initial migration safely lands, the next goal will be to
investigate those failing CI tests.
Will send out V5 shortly.
Regards, yuchen
^ permalink raw reply
page: | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox