* [PATCH 00/11] sequencer: do not record dropped commits as rewritten
From: Phillip Wood @ 2026-06-30 15:28 UTC (permalink / raw)
To: git; +Cc: Uwe Kleine-König, Junio C Hamano, Phillip Wood
In-Reply-To: <67dbfb5c-5f07-49b8-aa32-a4635c585028@gmail.com>
On 19/06/2026 11:13, Phillip Wood wrote:
> I'm happy to take this forward and try and fix at least some of the
> other bugs I've listed above. Uwe - if I don't cc you on some patches
> within the next couple of weeks please feel free to send a reminder.
Here is the first batch that fixes the same problem as Uwe's patch. I've
taken a slightly different approach that uses the return value from
do_pick_commit() to signal that a commit was dropped rather than
adding another function argument. That involves a number of preparatory
patches, but they are hopefully reasonably small and easy to follow.
If a commit gets dropped because its changes are already upstream
then we should not record it as rewritten. As well as confusing any
post-rewrite hooks this means we end up copying the notes from the
dropped commit to the commit that was picked immediately before the
one that was dropped.
This series is structured as follows:
Patch 1 restores some test coverage that was lost when the default
rebase backend was changed.
Patch 2 moves a function so it can be called without a forward
declaration in Patch 11.
Patches 3 & 4 fix the return value of do_pick_commit() when an external
command fails (this is in preparation for patch 10).
Patches 5-9 try and simplify the control flow in pick_one_commit()
in preparation for patch 10.
Patch 10 changes the return type of do_pick_commit() to an enum.
Patch 11 adds a new member to the enum from patch 10 for commits that
are dropped when they become empty and uses that to stop them from
being recorded as rewritten.
Base-Commit: 6c3d7b73556db708feb3b16232fab1efc4353428
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Frebase-drop-notes-with-commit%2Fv1
View-Changes-At: https://github.com/phillipwood/git/compare/6c3d7b735...26551f268
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/rebase-drop-notes-with-commit/v1
Phillip Wood (11):
t3400: restore coverage for note copying with apply backend
sequencer: move definition of is_final_fixup()
sequencer: be more careful with external merge
sequencer: never reschedule on failed commit
sequencer: remove unnecessary "or" in pick_one_commit()
sequencer: simplify handing of fixup with conflicts
sequencer: remove unnecessary condition in pick_one_commit()
sequencer: simplify pick_one_commit()
sequencer: return early from pick_one_commit() on success
sequencer: use an enum to represent result of picking a commit
sequencer: do not record dropped commits as rewritten
sequencer.c | 154 +++++++++++++++++++++++-----------
t/t3400-rebase.sh | 16 +++-
t/t3404-rebase-interactive.sh | 11 +++
t/t5407-post-rewrite-hook.sh | 23 +++++
4 files changed, 155 insertions(+), 49 deletions(-)
--
2.54.0.200.gfd8d68259e3
^ permalink raw reply
* [PATCH 01/11] t3400: restore coverage for note copying with apply backend
From: Phillip Wood @ 2026-06-30 15:28 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>
Now that the merge backend is the default we have lost coverage for
"git rebase --apply" copying notes. Fix this by replacing "-m" with
"--apply" as the previous test which uses the default backend now
checks the merge backend.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
t/t3400-rebase.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index c0c00fbb7b1..f0e7fcf649a 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -270,9 +270,9 @@ test_expect_success 'rebase can copy notes' '
test "a note" = "$(git notes show HEAD)"
'
-test_expect_success 'rebase -m can copy notes' '
+test_expect_success 'rebase --apply can copy notes' '
git reset --hard n3 &&
- git rebase -m --onto n1 n2 &&
+ git rebase --apply --onto n1 n2 &&
test "a note" = "$(git notes show HEAD)"
'
--
2.54.0.200.gfd8d68259e3
^ permalink raw reply related
* [PATCH 02/11] sequencer: move definition of is_final_fixup()
From: Phillip Wood @ 2026-06-30 15:28 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>
Move this function earlier in the file in preparation for adding a
new caller in a later commit.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 57855b0066a..32a09b6e87d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4627,21 +4627,6 @@ static int do_update_refs(struct repository *r, int quiet)
strbuf_release(&update_msg);
strbuf_release(&error_msg);
return res;
-}
-
-static int is_final_fixup(struct todo_list *todo_list)
-{
- int i = todo_list->current;
-
- if (!is_fixup(todo_list->items[i].command))
- return 0;
-
- while (++i < todo_list->nr)
- if (is_fixup(todo_list->items[i].command))
- return 0;
- else if (!is_noop(todo_list->items[i].command))
- break;
- return 1;
}
static enum todo_command peek_command(struct todo_list *todo_list, int offset)
@@ -4925,6 +4910,21 @@ static int reread_todo_if_changed(struct repository *r,
strbuf_release(&buf);
return 0;
+}
+
+static int is_final_fixup(struct todo_list *todo_list)
+{
+ int i = todo_list->current;
+
+ if (!is_fixup(todo_list->items[i].command))
+ return 0;
+
+ while (++i < todo_list->nr)
+ if (is_fixup(todo_list->items[i].command))
+ return 0;
+ else if (!is_noop(todo_list->items[i].command))
+ break;
+ return 1;
}
static const char rescheduled_advice[] =
--
2.54.0.200.gfd8d68259e3
^ permalink raw reply related
* [PATCH 03/11] sequencer: be more careful with external merge
From: Phillip Wood @ 2026-06-30 15:28 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 an external merge strategy cannot merge (for example because it
would overwrite an untracked file) it exits with a non-zero exit
code other than 1. This should be treated differently to a merge
with conflicts which is signalled by an exit code of 1 because as
the merge failed we need to reschedule the last pick. The caller
expects us to return -1 in this case. Also reschedule without trying
to merge if the commit message cannot be written as that prevents us
from successfully picking the commit.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 19 +++++++++++++++----
t/t3404-rebase-interactive.sh | 11 +++++++++++
2 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 32a09b6e87d..e6626c4db4e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2453,14 +2453,25 @@ static int do_pick_commit(struct repository *r,
struct commit_list *common = NULL;
struct commit_list *remotes = NULL;
- res = write_message(ctx->message.buf, ctx->message.len,
- git_path_merge_msg(r), 0);
+ if (write_message(ctx->message.buf, ctx->message.len,
+ git_path_merge_msg(r), 0)) {
+ res = -1;
+ goto leave;
+ }
commit_list_insert(base, &common);
commit_list_insert(next, &remotes);
- res |= try_merge_command(r, opts->strategy,
- opts->xopts.nr, opts->xopts.v,
+ res = try_merge_command(r, opts->strategy,
+ opts->xopts.nr, opts->xopts.v,
common, oid_to_hex(&head), remotes);
+ /*
+ * If the there were conflicts, try_merge_command() returns 1,
+ * any other no-zero return code means that either the merge
+ * command could not be run, or it failed to merge.
+ */
+ if (res && res != 1)
+ res = -1;
+
commit_list_free(common);
commit_list_free(remotes);
}
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 58b3bb0c271..297b84e60d5 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1249,6 +1249,17 @@ test_expect_success 'interrupted rebase -i with --strategy and -X' '
git rebase --continue &&
test $(git show conflict-branch:conflict) = $(cat conflict) &&
test $(cat file1) = Z
+'
+
+test_expect_success 'failing pick with --strategy is rescheduled' '
+ test_when_finished "rm -rf bin; test_might_fail git rebase --abort" &&
+ mkdir bin &&
+ echo exit 2 | write_script bin/git-merge-fail &&
+ git log -1 --format="pick %H # %s" HEAD >expect &&
+ test_must_fail env PATH="$PWD/bin:$PATH" \
+ git rebase --no-ff --strategy fail HEAD^ &&
+ test_cmp expect .git/rebase-merge/git-rebase-todo &&
+ test_cmp expect .git/rebase-merge/done
'
test_expect_success 'rebase -i error on commits with \ in message' '
--
2.54.0.200.gfd8d68259e3
^ permalink raw reply related
* [PATCH 04/11] sequencer: never reschedule on failed commit
From: Phillip Wood @ 2026-06-30 15:28 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 "git commit" fails to run then run_git_commit() returns -1 which
causes the current command to be rescheduled. This is incorrect as
we have successfully picked the commit and have written all the state
files we need to successfully commit when the user continues. Fix this
by converting -1 to 1 which matches what do_merge() does.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index e6626c4db4e..d7e439b1feb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2542,6 +2542,12 @@ static int do_pick_commit(struct repository *r,
res = run_git_commit(NULL, reflog_action, opts, flags);
*check_todo = 1;
}
+ /*
+ * If "git commit" failed to run than res == -1 but we dont
+ * want reschedule the last command because the picking the
+ * commit was successful.
+ */
+ res = !!res;
}
--
2.54.0.200.gfd8d68259e3
^ permalink raw reply related
* [PATCH 05/11] sequencer: remove unnecessary "or" in pick_one_commit()
From: Phillip Wood @ 2026-06-30 15:28 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 error_with_patch(..., res, ...) succeeds then it returns "res", if
it fails then it returns -1. This means that or-ing the return value
with "res" is pointless the result is the same as the return value.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index d7e439b1feb..39cbb7b6e3e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5007,9 +5007,8 @@ static int pick_one_commit(struct repository *r,
oideq(&opts->squash_onto, &oid))))
to_amend = 1;
- return res | error_with_patch(r, item->commit,
- arg, item->arg_len, opts,
- res, to_amend);
+ return error_with_patch(r, item->commit, arg, item->arg_len,
+ opts, res, to_amend);
}
return res;
}
--
2.54.0.200.gfd8d68259e3
^ permalink raw reply related
* [PATCH 06/11] sequencer: simplify handing of fixup with conflicts
From: Phillip Wood @ 2026-06-30 15:28 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>
Commit e032abd5a0 (rebase: fix rewritten list for failed pick,
2023-09-06) introduced an early return when res == -1, so if we enter
this conditional block then res is positive. After the last couple
of commits the only possible positive value is 1 so we can simplify
the code by removing the conditional call to intend_to_amend() and
call it error_with_patch() instead.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 39cbb7b6e3e..bcfbda018a7 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3874,7 +3874,7 @@ static int error_failed_squash(struct repository *r,
return error(_("could not copy '%s' to '%s'"),
rebase_path_message(),
git_path_merge_msg(r));
- return error_with_patch(r, commit, subject, subject_len, opts, 1, 0);
+ return error_with_patch(r, commit, subject, subject_len, opts, 1, 1);
}
static int do_exec(struct repository *r, const char *command_line, int quiet)
@@ -4986,8 +4986,6 @@ static int pick_one_commit(struct repository *r,
record_in_rewritten(&item->commit->object.oid,
peek_command(todo_list, 1));
if (res && is_fixup(item->command)) {
- if (res == 1)
- intend_to_amend();
return error_failed_squash(r, item->commit, opts,
item->arg_len, arg);
} else if (res && is_rebase_i(opts) && item->commit) {
--
2.54.0.200.gfd8d68259e3
^ permalink raw reply related
* [PATCH 07/11] sequencer: remove unnecessary condition in pick_one_commit()
From: Phillip Wood @ 2026-06-30 15:28 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>
item->commit holds the commit to be picked and so it must be non-NULL
otherwise pick_one_commit() would not know which commit to pick.
It is also unconditionally dereferenced in do_pick_commit() which is
called at the top of this function. Therefore the check to see if it
is non-NULL is superfluous.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index bcfbda018a7..ff28873d21c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4988,7 +4988,7 @@ static int pick_one_commit(struct repository *r,
if (res && is_fixup(item->command)) {
return error_failed_squash(r, item->commit, opts,
item->arg_len, arg);
- } else if (res && is_rebase_i(opts) && item->commit) {
+ } else if (res && is_rebase_i(opts)) {
int to_amend = 0;
struct object_id oid;
--
2.54.0.200.gfd8d68259e3
^ permalink raw reply related
* [PATCH 08/11] sequencer: simplify pick_one_commit()
From: Phillip Wood @ 2026-06-30 15:28 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>
Unless we're rebasing all we do in pick_one_commit() is call
do_pick_commit() and return its result. Simplify the code by returing
early if we're not rebasing so that we don't have to continually call
is_rebase_i() in the rest of the function. Note that there are a couple
of conditions that do not call is_rebase_i() but they check for either
an "edit" or a "fixup" command, both of which imply we're rebasing.
As the conditional blocks are all mutually exclusive (either the
conditions are mutually exclusive, or an earlier conditional block
that would match a later one contains a "return" statement) chain
them together with "else if" to make that clear.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index ff28873d21c..416729f30a7 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4966,12 +4966,14 @@ static int pick_one_commit(struct repository *r,
res = do_pick_commit(r, item, opts, is_final_fixup(todo_list),
check_todo);
- if (is_rebase_i(opts) && res < 0) {
+ if (!is_rebase_i(opts))
+ return res;
+
+ if (res < 0) {
/* Reschedule */
*reschedule = 1;
return -1;
- }
- if (item->command == TODO_EDIT) {
+ } else if (item->command == TODO_EDIT) {
struct commit *commit = item->commit;
if (!res) {
if (!opts->verbose)
@@ -4981,14 +4983,13 @@ static int pick_one_commit(struct repository *r,
}
return error_with_patch(r, commit,
arg, item->arg_len, opts, res, !res);
- }
- if (is_rebase_i(opts) && !res)
+ } else if (!res) {
record_in_rewritten(&item->commit->object.oid,
peek_command(todo_list, 1));
- if (res && is_fixup(item->command)) {
+ } else if (res && is_fixup(item->command)) {
return error_failed_squash(r, item->commit, opts,
item->arg_len, arg);
- } else if (res && is_rebase_i(opts)) {
+ } else if (res) {
int to_amend = 0;
struct object_id oid;
--
2.54.0.200.gfd8d68259e3
^ permalink raw reply related
* [PATCH 09/11] sequencer: return early from pick_one_commit() on success
From: Phillip Wood @ 2026-06-30 15:28 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>
The only block that does not return early is the one guarded by
"!res". Move the return into that block to make it clear that after
recording the commit as rewritten all we do is return from the function.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
sequencer.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index 416729f30a7..655a2e84bef 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4986,6 +4986,7 @@ static int pick_one_commit(struct repository *r,
} else if (!res) {
record_in_rewritten(&item->commit->object.oid,
peek_command(todo_list, 1));
+ return 0;
} else if (res && is_fixup(item->command)) {
return error_failed_squash(r, item->commit, opts,
item->arg_len, arg);
@@ -5009,7 +5010,8 @@ static int pick_one_commit(struct repository *r,
return error_with_patch(r, item->commit, arg, item->arg_len,
opts, res, to_amend);
}
- return res;
+
+ BUG("Unhandled return value from do_pick_commit()");
}
static int pick_commits(struct repository *r,
--
2.54.0.200.gfd8d68259e3
^ permalink raw reply related
* [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
* [PATCH v5 0/1] environment: move excludes_file into repo_config_values
From: Tian Yuchen @ 2026-06-30 16:44 UTC (permalink / raw)
To: git
Cc: cirnovskyv, szeder.dev, Tian Yuchen, Christian Couder,
Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <20260627160813.1074201-1-cat@malon.dev>
This patch continues the libification effort by migrating the global
string variable 'excludes_file' into 'struct repo_config_values'. Since
this is a dynamically allocated variable, the migration requires proper
heap memory management.
This patch mainly does three things:
- Abstract the XDG fallback lazy-loading logic out of dir.c into a proper
getter.
- Move the variables into the struct repo_config_values.
- Introduce the memory destructor 'repo_config_values_clear()'.
Changes since V4:
Defensive checks are retained in both the getter (returning NULL if
uninitialized) and the destructor (bypassing non-the_repository instances)
to maintain bug-to-bug compatibility. These are marked with 'NEEDSWORK'
comments.
Future work will track down and fix the offending callers to eventually
replace these shields with stricter BUG() assertions. [1]
THANKS!
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
Signed-off-by: Tian Yuchen <cat@malon.dev>
[1] https://lore.kernel.org/git/054b3fb6-0e69-473d-9778-b1b11ea82b3a@malon.dev/T/#mca0730b6735298316ec00a53d4719935d18837a6
Tian Yuchen (1):
environment: move excludes_file into repo_config_values
dir.c | 4 ++--
environment.c | 43 ++++++++++++++++++++++++++++++++++++++++---
environment.h | 13 ++++++++++++-
repository.c | 1 +
4 files changed, 55 insertions(+), 6 deletions(-)
--
2.43.0
^ permalink raw reply
* [PATCH v5 1/1] environment: move excludes_file into repo_config_values
From: Tian Yuchen @ 2026-06-30 16:44 UTC (permalink / raw)
To: git
Cc: cirnovskyv, szeder.dev, Tian Yuchen, Christian Couder,
Ayush Chandekar, Olamide Caleb Bello
In-Reply-To: <20260630164401.2906091-1-cat@malon.dev>
The global variable 'excludes_file' is used to track the path to the
global ignore file. If this variable is NULL, 'setup_standard_excludes()'
in 'dir.c' forcefully evaluates and assigns the XDG default path to it.
Continue the libification effort by encapsulating this lazy-loading
fallback logic into a proper getter and moving the variable into
'struct repo_config_values'.
Since 'excludes_file' is a dynamically allocated string, it requires
proper heap memory management. Introduce repo_config_values_clear()
and wire it up in 'repo_clear()' to safely free this memory when a
repository instance is destroyed. Also clean up the heap-allocated
'attributes_file' in this new destructor while we are at it.
Note on transition:
Defensive checks are temporarily retained in both the getter (returning
NULL if uninitialized) and the destructor (bypassing non-the_repository
instances) to maintain bug-to-bug compatibility. These are marked with
NEEDSWORK comments. Future work will track down and fix the offending
callers to eventually replace these shields with stricter BUG()
assertions.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
Signed-off-by: Tian Yuchen <cat@malon.dev>
---
dir.c | 4 ++--
environment.c | 43 ++++++++++++++++++++++++++++++++++++++++---
environment.h | 13 ++++++++++++-
repository.c | 1 +
4 files changed, 55 insertions(+), 6 deletions(-)
diff --git a/dir.c b/dir.c
index 7a73690fbc..4f87a52b3c 100644
--- a/dir.c
+++ b/dir.c
@@ -3481,11 +3481,11 @@ static GIT_PATH_FUNC(git_path_info_exclude, "info/exclude")
void setup_standard_excludes(struct dir_struct *dir)
{
+ const char *excludes_file = repo_excludes_file(the_repository);
+
dir->exclude_per_dir = ".gitignore";
/* core.excludesfile defaulting to $XDG_CONFIG_HOME/git/ignore */
- if (!excludes_file)
- excludes_file = xdg_config_home("ignore");
if (excludes_file && !access_or_warn(excludes_file, R_OK, 0))
add_patterns_from_file_1(dir, excludes_file,
dir->untracked ? &dir->internal.ss_excludes_file : NULL);
diff --git a/environment.c b/environment.c
index ba2c60103f..5fc13d47c2 100644
--- a/environment.c
+++ b/environment.c
@@ -57,7 +57,6 @@ enum fsync_method fsync_method = FSYNC_METHOD_DEFAULT;
enum fsync_component fsync_components = FSYNC_COMPONENTS_DEFAULT;
char *editor_program;
char *askpass_program;
-char *excludes_file;
enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
enum eol core_eol = EOL_UNSET;
int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;
@@ -134,6 +133,24 @@ int is_bare_repository(void)
return is_bare_repository_cfg && !repo_get_work_tree(the_repository);
}
+const char *repo_excludes_file(struct repository *repo)
+{
+ /*
+ * NEEDSWORK: This is a temporary shield to maintain bug-to-bug
+ * compatibility during the libification transition.
+ *
+ * Once offending callers are properly fixed, this check should
+ * be upgraded to a BUG() assertion and eventually removed entirely.
+ */
+ if (!repo || !repo->initialized)
+ return NULL;
+
+ if (!repo_config_values(repo)->excludes_file)
+ repo_config_values(repo)->excludes_file = xdg_config_home("ignore");
+
+ return repo_config_values(repo)->excludes_file;
+}
+
int have_git_dir(void)
{
return startup_info->have_repository
@@ -461,8 +478,8 @@ int git_default_core_config(const char *var, const char *value,
}
if (!strcmp(var, "core.excludesfile")) {
- FREE_AND_NULL(excludes_file);
- return git_config_pathname(&excludes_file, var, value);
+ FREE_AND_NULL(cfg->excludes_file);
+ return git_config_pathname(&cfg->excludes_file, var, value);
}
if (!strcmp(var, "core.whitespace")) {
@@ -715,6 +732,7 @@ int git_default_config(const char *var, const char *value,
void repo_config_values_init(struct repo_config_values *cfg)
{
cfg->attributes_file = NULL;
+ cfg->excludes_file = NULL;
cfg->apply_sparse_checkout = 0;
cfg->branch_track = BRANCH_TRACK_REMOTE;
cfg->trust_ctime = 1;
@@ -726,3 +744,22 @@ void repo_config_values_init(struct repo_config_values *cfg)
cfg->sparse_expect_files_outside_of_patterns = 0;
cfg->warn_on_object_refname_ambiguity = 1;
}
+
+void repo_config_values_clear(struct repository *repo)
+{
+ struct repo_config_values *cfg;
+
+ /*
+ * NEEDSWORK: Temporary shield to prevent temporary/uninitialized
+ * submodules from triggering the BUG() at repository.c:59
+ * during repo_clear(). This should be removed once submodule
+ * lifecycle and per-repo config support are fully resolved.
+ */
+ if (repo != the_repository)
+ return;
+
+ cfg = repo_config_values(repo);
+
+ FREE_AND_NULL(cfg->attributes_file);
+ FREE_AND_NULL(cfg->excludes_file);
+}
diff --git a/environment.h b/environment.h
index 6f18286955..2e8352de7f 100644
--- a/environment.h
+++ b/environment.h
@@ -90,6 +90,7 @@ struct repository;
struct repo_config_values {
/* section "core" config values */
char *attributes_file;
+ char *excludes_file;
int apply_sparse_checkout;
int trust_ctime;
int check_stat;
@@ -133,8 +134,19 @@ int git_default_config(const char *, const char *,
int git_default_core_config(const char *var, const char *value,
const struct config_context *ctx, void *cb);
+const char *repo_excludes_file(struct repository *repo);
+
void repo_config_values_init(struct repo_config_values *cfg);
+/*
+ * Frees memory allocated for dynamically loaded configuration values
+ * inside `repo_config_values`.
+ *
+ * As dynamically allocated variables are migrated into this struct,
+ * their FREE_AND_NULL() calls should be appended here.
+ */
+void repo_config_values_clear(struct repository *repo);
+
/*
* TODO: All the below state either explicitly or implicitly relies on
* `the_repository`. We should eventually get rid of these and make the
@@ -208,7 +220,6 @@ extern char *git_log_output_encoding;
extern char *editor_program;
extern char *askpass_program;
-extern char *excludes_file;
/*
* The character that begins a commented line in user-editable file
diff --git a/repository.c b/repository.c
index 187dd471c4..b31f1b7852 100644
--- a/repository.c
+++ b/repository.c
@@ -388,6 +388,7 @@ void repo_clear(struct repository *repo)
FREE_AND_NULL(repo->parsed_objects);
repo_settings_clear(repo);
+ repo_config_values_clear(repo);
if (repo->config) {
git_configset_clear(repo->config);
--
2.43.0
^ permalink raw reply related
* Re: [PATCH 3/6] odb: add `source` field to struct object_info_source
From: Junio C Hamano @ 2026-06-30 16:54 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Justin Tobler, git
In-Reply-To: <akOod6X1a2axIXKZ@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> On Mon, Jun 29, 2026 at 01:47:41PM -0700, Junio C Hamano wrote:
>> Justin Tobler <jltobler@gmail.com> writes:
>>
>> >> @@ -1424,6 +1424,10 @@ int packed_object_info_with_index_pos(struct odb_source_packed *source UNUSED,
>> >> oi->whence = OI_PACKED;
>> >>
>> >> if (oi->sourcep) {
>> >> + if (!source)
>> >> + BUG("cannot request source without an owning source");
>> >> + oi->sourcep->source = &source->base;
>> >
>> > And here it is set for the packed backend. Looks good.
>> >
>> > Naive question: I understand that some `packed_info_object()` callers
>> > may not have the `struct odb_source` on hand, but when the `struct
>> > packed_git` is intially setup, is it not always known the ODB source it
>> > comes from? It makes me wonder if the ODB source should also be recorded
>> > when `struct packed_git` is initialized.
>
> I've addressed this comment on patch 1.
>
>> As with your reaction to [PATCH 1/6], I do share this puzzlement: if
>> the source can almost always be NULL, what is it good for and isn't
>> it something that can be computed from the available information?
>
> It's not almost always NULL, even though it looks like this because we
> ended up adapting more callers to pass `NULL` than we adapted callers to
> pass an actual source. But in the end it's rather the opposite: there
> are very few low-level callers that don't have the source info
> available, and everyone else instead uses `odb_read_object_info()`,
> where we do have it available. But those callers don't need to be
> adjusted, so they weren't visible in the diff.
>
>> Perhaps it is the naming?
>
> Yeah, as Justin pointed out, calling this `sourcep` is confusing.
OK, everything makes sense.
>> I am confused what the above quoted code actually is doing ("if you
>> have a source, then grab its base and set it to .source member of
>> the struct the out parameter points at", makes it sound like the out
>> parameter sourcep should be pointing at a structure with .base
>> member, not .source member, or perhaps the caller should be passing
>> &oi->sourcep->source as *base to be assigned to, or something).
>
> We have to return the generic source here, not the specialized source,
> so that this interface can be used by every implementation. Other sites
> would end up storing their own source, which of course would have a
> different specialized backend.
>
> So an alternative to write this would have been:
>
> oi->sourcep->source = (struct odb_source *) source;
>
> But by assigning the base we avoid having to cast.
Yuck.
I guess it may be OK as the caller or whategver the caller calls
later may have to downcast this pointer the usual way to access what
we return here anyway. As a pointer to a struct object, when
suitably converted, points to its first member, this upcast should
always be safe. And taking the address of the .base member is far
more explicit.
While it hides the fact that there is such an upcast involved, which
might be confusing to readers not familiar with this corner of the
codebase, I am fine with the way it was written.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 05/12] t/unit-tests: introduce test helper to write reftable blocks
From: Junio C Hamano @ 2026-06-30 17:25 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, oxsignal, Christian Couder
In-Reply-To: <20260629-pks-reftable-hardening-v2-5-b0228e7d908d@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> Introduce a new test helper that allows us to write reftable blocks.
> This helper will be used by subsequent commits.
>
> Suggested-by: Christian Couder <christian.couder@gmail.com>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> t/unit-tests/u-reftable-block.c | 47 ++++++++++++++++++++++++-----------------
> 1 file changed, 28 insertions(+), 19 deletions(-)
Nicely reduces boilerplate/setup code. Thanks, both.
^ permalink raw reply
* Re: [PATCH] submodule absorbgitdirs tests: use test_* helper functions
From: Junio C Hamano @ 2026-06-30 18:00 UTC (permalink / raw)
To: Bryan B. Lima; +Cc: git, gustavoscorrea, Ævar Arnfjörð Bjarmason
In-Reply-To: <20260630020220.1559190-1-bblima@usp.br>
"Bryan B. Lima" <bblima@usp.br> writes:
> Use modern helper functions from test-lib-functions.sh to provide nice error messages.
>
> Signed-off-by: Bryan B. Lima <bblima@usp.br>
> Co-authored-by: Gustavo S. Correa <gustavoscorrea@usp.br>
> Signed-off-by: Gustavo S. Correa <gustavoscorrea@usp.br>
> ---
> t/t7412-submodule-absorbgitdirs.sh | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
Welcome to the Git development community.
It is rare, but it happens from time to time, that we see a patch by
somebody we haven't seen on this list, and the patch looks perfect.
Very delighted.
Will queue. Thanks.
> diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh
> index 0490499573..bd1c684480 100755
> --- a/t/t7412-submodule-absorbgitdirs.sh
> +++ b/t/t7412-submodule-absorbgitdirs.sh
> @@ -34,8 +34,8 @@ test_expect_success 'absorb the git dir' '
> git submodule absorbgitdirs 2>actual &&
> test_cmp expect actual &&
> git fsck &&
> - test -f sub1/.git &&
> - test -d .git/modules/sub1 &&
> + test_path_is_file sub1/.git &&
> + test_path_is_dir .git/modules/sub1 &&
> git status >actual.1 &&
> git -C sub1 rev-parse HEAD >actual.2 &&
> test_cmp expect.1 actual.1 &&
> @@ -47,9 +47,9 @@ test_expect_success 'absorbing does not fail for deinitialized submodules' '
> git submodule deinit --all &&
> git submodule absorbgitdirs 2>err &&
> test_must_be_empty err &&
> - test -d .git/modules/sub1 &&
> - test -d sub1 &&
> - ! test -e sub1/.git
> + test_path_is_dir .git/modules/sub1 &&
> + test_path_is_dir sub1 &&
> + test_path_is_missing sub1/.git
> '
>
> test_expect_success 'setup nested submodule' '
> @@ -72,8 +72,8 @@ test_expect_success 'absorb the git dir in a nested submodule' '
> EOF
> git submodule absorbgitdirs 2>actual &&
> test_cmp expect actual &&
> - test -f sub1/nested/.git &&
> - test -d .git/modules/sub1/modules/nested &&
> + test_path_is_file sub1/nested/.git &&
> + test_path_is_dir .git/modules/sub1/modules/nested &&
> git status >actual.1 &&
> git -C sub1/nested rev-parse HEAD >actual.2 &&
> test_cmp expect.1 actual.1 &&
> @@ -109,9 +109,9 @@ test_expect_success 'absorb the git dir in a nested submodule' '
> EOF
> git submodule absorbgitdirs 2>actual &&
> test_cmp expect actual &&
> - test -f sub1/.git &&
> - test -f sub1/nested/.git &&
> - test -d .git/modules/sub1/modules/nested &&
> + test_path_is_file sub1/.git &&
> + test_path_is_file sub1/nested/.git &&
> + test_path_is_dir .git/modules/sub1/modules/nested &&
> git status >actual.1 &&
> git -C sub1/nested rev-parse HEAD >actual.2 &&
> test_cmp expect.1 actual.1 &&
> @@ -155,7 +155,7 @@ test_expect_success 'absorbing the git dir fails for incomplete submodules' '
> test_must_fail git submodule absorbgitdirs 2>actual &&
> test_cmp expect actual &&
> git -C sub2 fsck &&
> - test -d sub2/.git &&
> + test_path_is_dir sub2/.git &&
> git status >actual &&
> git -C sub2 rev-parse HEAD >actual.2 &&
> test_cmp expect.1 actual.1 &&
>
> base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
^ permalink raw reply
* Re: [PATCH 02/13] setup: mark bogus worktree in `apply_repository_format()`
From: Junio C Hamano @ 2026-06-30 18:26 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <20260630-pks-setup-split-discovery-and-setup-v1-2-13864eb5a032@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> + * we have to exlicitly unset the configuration.
explicitly (will amend while queuing).
> + */
^ permalink raw reply
* Re: [PATCH v5 0/4] history: add squash subcommand to fold a range
From: Harald Nordgren @ 2026-06-30 18:38 UTC (permalink / raw)
To: phillip.wood; +Cc: Patrick Steinhardt, Harald Nordgren via GitGitGadget, git
In-Reply-To: <4b505228-4846-4a48-9255-e249f4e70a1f@gmail.com>
I want to avoid creating drift between this and the format of 'git
rebase -i', so if we want to change this, maybe better to change both
at a later point instead?
Harald
^ permalink raw reply
* Re: [PATCH v3 0/5] builtin/refs: add ability to write references
From: Junio C Hamano @ 2026-06-30 19:16 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <20260630-pks-refs-writing-subcommands-v3-0-deb04de1ecef@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> @@ builtin/refs.c: static int cmd_refs_optimize(int argc, const char **argv, const
> + if (repo_get_oid_with_flags(repo, argv[1], &newoid, GET_OID_SKIP_AMBIGUITY_CHECK))
> + die(_("invalid object ID: '%s'"), argv[1]);
> + if (is_null_oid(&newoid))
> -+ die(_("cannot create reference with null old object ID"));
> ++ die(_("cannot create reference with null new object ID"));
> +
> + ret = refs_update_ref(get_main_ref_store(repo), message, refname,
> + &newoid, null_oid(repo->hash_algo), flags,
> @@ t/t1466-refs-create.sh (new)
> + (
> + cd repo &&
> + test_must_fail git refs create refs/heads/foo $ZERO_OID 2>err &&
> -+ test_grep "null old object ID" err &&
> ++ test_grep "null new object ID" err &&
> + test_must_fail git refs exists refs/heads/foo
> + )
> +'
Looks obviously correct. Thanks.
^ permalink raw reply
* Re: [PATCH 0/3] fixing expensive http test timeouts
From: Junio C Hamano @ 2026-06-30 19:21 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Jeff King, Michael Montalbo, git
In-Reply-To: <akOGzAq8Is7ghgIM@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> The pipelines of the official mirror can be found at [1]. We might for
> example add something like the below patch to our README.md to make it
> more discoverable.
>
> Patrick
>
> [1]: https://gitlab.com/git-scm/git/-/pipelines
>
> diff --git a/README.md b/README.md
> index d87bca1b8c..9ad77fdf7e 100644
> --- a/README.md
> +++ b/README.md
> @@ -1,4 +1,5 @@
> -[](https://github.com/git/git/actions?query=branch%3Amaster+event%3Apush)
> +[](https://github.com/git/git/actions?query=branch%3Amaster+event%3Apush)
> +[](https://gitlab.com/git-scm/git/-/pipelines?ref=master)
>
> Git - fast, scalable, distributed revision control system
> =========================================================
Oh, nice. We of course do not want to be heavily involved in
advertising offerings by commercial entities but I think these two
sites deserve one line each for their continued service to the
community ;-)
^ permalink raw reply
* Re: What's cooking in git.git (Jun 2026, #10)
From: Junio C Hamano @ 2026-06-30 19:27 UTC (permalink / raw)
To: Toon Claes; +Cc: git
In-Reply-To: <87cxx84exj.fsf@emacs.iotcl.com>
Toon Claes <toon@iotcl.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> * ps/odb-generalize-prepare (2026-06-22) 3 commits
>> - odb: introduce `odb_prepare()`
>> - odb/source: generalize `reprepare()` callback
>> - Merge branch 'ps/odb-source-packed' into ps/odb-generalize-prepare
>> (this branch uses ps/odb-source-packed.)
>>
>> The `reprepare()` callback for object database sources has been
>> generalized into a `prepare()` callback with an optional flush cache
>> flag, and a new `odb_prepare()` wrapper has been introduced to
>> allow pre-opening object database sources.
>>
>> Needs review.
>> source: <20260622-b4-pks-odb-generalize-prepare-v1-0-d2a5c5d13144@pks.im>
>
> I did have some questions/remarks, but Patrick answered them, and with
> those answers I'm happy about this series.
Yeah, I am also happy with these patches.
Thanks.
^ permalink raw reply
* Re: git-blame vs. abbrev
From: Junio C Hamano @ 2026-06-30 19:49 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: git
In-Reply-To: <b500479b-14c1-4fbb-a672-1d2cd4852601@posteo.net>
Laszlo Ersek <laszlo.ersek@posteo.net> writes:
> Hi,
>
> when git-blame is passed the "-b" option ("Show blank SHA-1 for boundary
> commits"), shouldn't git-blame *stop* reserving a commit hash nibble for
> the caret that otherwise marks boundary commits?
>
> More directly, I find it inconvenient that git-blame shows commit hashes
> that are one nibble longer (13) than my "core.abbrev" (12) setting; that
> makes cutting and pasting commit hashes from the git-blame output into a
> git-rebase TODO list cumbersome.
I never knew that the parser in rebase did not want to see a longer
abbreviation; shouldn't it take 16 hexadecimal abbreviation from the
result of letting the user edit the list, even if it initially gave
12 hexadecimal abbreviation, as long as these extra 4 hexdigits do
not break the commit object name? That is a more serious usability
bug that needs to be fixed, if it is the case, I would think.
FWIW, even if your core.abbrev says you want 12, if two objects
share the same 12 hexdigits as the prefix, you do end up getting 13
or more, so a parser that insists on exact 12 hexdigits sounds like
a bug.
Just for the sake of aesthetics, I agree that when we are not
showing the boundary mark, it would make sense not to reserve one
column that we know we will never use. But unless there is a
mistaken parser that insists on 12 hexdigits when 13 hexdigits you
give uniquely identify the same object, I suspect you wouldn't even
notice that the hexadecimal digits you see on the screen have one
digit longer than usual ;-).
^ permalink raw reply
* Re: [PATCH 00/11] sequencer: do not record dropped commits as rewritten
From: Junio C Hamano @ 2026-06-30 19:57 UTC (permalink / raw)
To: Phillip Wood; +Cc: git, Uwe Kleine-König, Konstantin Ryabitsev
In-Reply-To: <cover.1782833268.git.phillip.wood@dunelm.org.uk>
Phillip Wood <phillip.wood123@gmail.com> writes:
> On 19/06/2026 11:13, Phillip Wood wrote:
>> I'm happy to take this forward and try and fix at least some of the
>> other bugs I've listed above. Uwe - if I don't cc you on some patches
>> within the next couple of weeks please feel free to send a reminder.
>
> Here is the first batch that fixes the same problem as Uwe's patch. I've
> taken a slightly different approach that uses the return value from
> do_pick_commit() to signal that a commit was dropped rather than
> adding another function argument. That involves a number of preparatory
> patches, but they are hopefully reasonably small and easy to follow.
>
> If a commit gets dropped because its changes are already upstream
> then we should not record it as rewritten. As well as confusing any
> post-rewrite hooks this means we end up copying the notes from the
> dropped commit to the commit that was picked immediately before the
> one that was dropped.
>
> This series is structured as follows:
>
> Patch 1 restores some test coverage that was lost when the default
> rebase backend was changed.
>
> Patch 2 moves a function so it can be called without a forward
> declaration in Patch 11.
>
> Patches 3 & 4 fix the return value of do_pick_commit() when an external
> command fails (this is in preparation for patch 10).
>
> Patches 5-9 try and simplify the control flow in pick_one_commit()
> in preparation for patch 10.
>
> Patch 10 changes the return type of do_pick_commit() to an enum.
>
> Patch 11 adds a new member to the enum from patch 10 for commits that
> are dropped when they become empty and uses that to stop them from
> being recorded as rewritten.
>
> Base-Commit: 6c3d7b73556db708feb3b16232fab1efc4353428
> Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Frebase-drop-notes-with-commit%2Fv1
> View-Changes-At: https://github.com/phillipwood/git/compare/6c3d7b735...26551f268
> Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/rebase-drop-notes-with-commit/v1
Thanks.
A tangent (I Cc'ed Konstantin for this), but
$ b4 am -o- '<cover.1782833268.git.phillip.wood@dunelm.org.uk>' >b4am.mbx
failed to produce a usable mailbox. It somehow did not think [2/11]
existed. I manually examined the References and In-Reply-To headers
of that particular message and compared them with those from other
messages but did not find anything suspicious X-<.
I have a bunch of typofixes queued on top of these 11 patches (made
with "git commit --fixup reword:<sha1>"); please double check when
you reroll after seeing more substantial reviews than mere typofixes,
possibly from others.
Thanks.
Here is the transcript of failed b4 am invocation.
---- >8 ----
Looking up https://lore.kernel.org/all/cover.1782833268.git.phillip.wood@dunelm.org.uk/
Grabbing thread from lore.kernel.org/all/cover.1782833268.git.phillip.wood@dunelm.org.uk/t.mbox.gz
Analyzing 17 messages in the thread
WARNING: duplicate messages found at index 1
Subject 1: sequencer: Skip copying notes for commits that disappear during rebase
Subject 2: t3400: restore coverage for note copying with apply backend
2 is not a reply... assume additional patch
Looking for additional code-review trailers on lore.kernel.org
Analyzing 0 code-review messages
Checking attestation on all messages, may take a moment...
---
✗ [PATCH] sequencer: Skip copying notes for commits that disappear during rebase
✗ No key: openpgp/u.kleine-koenig@baylibre.com
✗ BADSIG: DKIM/baylibre.com
✓ [PATCH 1/11] t3400: restore coverage for note copying with apply backend
✓ Signed: DKIM/gmail.com
✓ [PATCH 3/11] sequencer: be more careful with external merge
✓ Signed: DKIM/gmail.com
✓ [PATCH 4/11] sequencer: never reschedule on failed commit
✓ Signed: DKIM/gmail.com
✓ [PATCH 5/11] sequencer: remove unnecessary "or" in pick_one_commit()
✓ Signed: DKIM/gmail.com
✓ [PATCH 6/11] sequencer: simplify handing of fixup with conflicts
✓ Signed: DKIM/gmail.com
✓ [PATCH 7/11] sequencer: remove unnecessary condition in pick_one_commit()
✓ Signed: DKIM/gmail.com
✓ [PATCH 8/11] sequencer: simplify pick_one_commit()
✓ Signed: DKIM/gmail.com
✓ [PATCH 9/11] sequencer: return early from pick_one_commit() on success
✓ Signed: DKIM/gmail.com
✓ [PATCH 10/11] sequencer: use an enum to represent result of picking a commit
✓ Signed: DKIM/gmail.com
✓ [PATCH 11/11] sequencer: do not record dropped commits as rewritten
✓ Signed: DKIM/gmail.com
ERROR: missing [12/2]!
---
Total patches: 11
---
WARNING: Thread incomplete!
Link: https://patch.msgid.link/cover.1782833268.git.phillip.wood@dunelm.org.uk
:
^ permalink raw reply
page: next (older) | 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