* [PATCH v8 11/11] builtin/history: implement "drop" subcommand
From: Patrick Steinhardt @ 2026-07-01 11:35 UTC (permalink / raw)
To: git
Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood,
Christian Couder
In-Reply-To: <20260701-b4-pks-history-drop-v8-0-19b5cdf1facd@pks.im>
A common operation when editing the commit history is to drop a specific
commit from the history entirely, but this operation is not currently
covered by git-history(1).
A couple of noteworthy bits:
- This is the first git-history(1) command that will ultimately result
in changes to both the index and the working tree. We thus have to
add logic to merge resulting changes into those.
- It is still not possible to replay merge commits, so this limitation
is inherited for the new "drop" command.
- For now we refuse to drop root commits. While we _can_ indeed drop
root commits in the general case, there are edge cases where the
resulting history would become completely empty. This is thus left
to a subsequent patch series.
Other than that, most of the logic is rather straight-forward as we can
continue to build on the preexisting logic in git-history(1) for most of
the part.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/git-history.adoc | 38 ++-
builtin/history.c | 184 ++++++++++++++
t/meson.build | 1 +
t/t3454-history-drop.sh | 561 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 783 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-history.adoc b/Documentation/git-history.adoc
index 2ba8121795..28b477cd37 100644
--- a/Documentation/git-history.adoc
+++ b/Documentation/git-history.adoc
@@ -8,6 +8,7 @@ git-history - EXPERIMENTAL: Rewrite history
SYNOPSIS
--------
[synopsis]
+git history drop <commit> [--dry-run] [--update-refs=(branches|head)] [--empty=(drop|keep|abort)]
git history fixup <commit> [--dry-run] [--update-refs=(branches|head)] [--reedit-message] [--empty=(drop|keep|abort)]
git history reword <commit> [--dry-run] [--update-refs=(branches|head)]
git history split <commit> [--dry-run] [--update-refs=(branches|head)] [--] [<pathspec>...]
@@ -51,13 +52,28 @@ be stateful operations. The limitation can be lifted once (if) Git learns about
first-class conflicts.
When using `fixup` with `--empty=drop`, dropping the root commit is not yet
-supported.
+supported. Likewise, `drop` cannot remove the root commit or a merge commit.
COMMANDS
--------
The following commands are available to rewrite history in different ways:
+`drop <commit>`::
+ Remove the specified commit from the history. All descendants of the
+ commit are replayed directly onto its parent.
++
+The root commit cannot be dropped as that may lead to edge cases where refs
+end up with no commits anymore. Merge commits cannot be dropped either; see
+LIMITATIONS.
++
+If `HEAD` points at a commit that is to be rewritten, the index and working
+tree are updated to match the new `HEAD`. The command aborts before any
+references are updated in case local modifications would be overwritten.
++
+If replaying any descendant would result in a conflict, the command aborts
+with an error.
+
`fixup <commit>`::
Apply the currently staged changes to the specified commit. This is
similar in nature to `git commit --fixup=<commit>` followed by `git
@@ -170,6 +186,26 @@ The staged addition of `unrelated.txt` has been incorporated into the `first`
commit. All descendant commits have been replayed on top of the rewritten
history.
+Drop a commit
+~~~~~~~~~~~~~
+
+----------
+$ git log --oneline
+abc1234 (HEAD -> main) third
+def5678 second
+ghi9012 first
+
+$ git history drop 'main^{/second}'
+
+$ git log --oneline
+jkl3456 (HEAD -> main) third
+ghi9012 first
+----------
+
+The `second` commit has been removed from the history, and `third` has been
+replayed directly on top of `first`. All branches that pointed at the dropped
+commit have been moved to its parent.
+
Split a commit
~~~~~~~~~~~~~~
diff --git a/builtin/history.c b/builtin/history.c
index 22b9fcb4a4..dd11ce2fa8 100644
--- a/builtin/history.c
+++ b/builtin/history.c
@@ -17,13 +17,17 @@
#include "read-cache.h"
#include "refs.h"
#include "replay.h"
+#include "reset.h"
#include "revision.h"
#include "sequencer.h"
#include "strvec.h"
#include "tree.h"
+#include "tree-walk.h"
#include "unpack-trees.h"
#include "wt-status.h"
+#define GIT_HISTORY_DROP_USAGE \
+ N_("git history drop <commit> [--dry-run] [--update-refs=(branches|head)] [--empty=(drop|keep|abort)]")
#define GIT_HISTORY_FIXUP_USAGE \
N_("git history fixup <commit> [--dry-run] [--update-refs=(branches|head)] [--reedit-message] [--empty=(drop|keep|abort)]")
#define GIT_HISTORY_REWORD_USAGE \
@@ -999,12 +1003,191 @@ static int cmd_history_split(int argc,
return ret;
}
+static int update_worktree(struct repository *repo,
+ const struct commit *old_head,
+ const struct commit *new_head,
+ bool dry_run)
+{
+ struct reset_working_tree_options opts = {
+ .oid_from = &old_head->object.oid,
+ .oid = &new_head->object.oid,
+ };
+ if (dry_run)
+ opts.flags |= RESET_WORKING_TREE_DRY_RUN;
+ return reset_working_tree(repo, &opts);
+}
+
+static int find_head_tree_change(struct repository *repo,
+ const struct replay_result *result,
+ struct commit **old_head,
+ struct commit **new_head,
+ bool *changed)
+{
+ const struct replay_ref_update *head_update = NULL;
+ struct commit *old_head_commit, *new_head_commit;
+ struct tree *old_head_tree, *new_head_tree;
+ const char *head_target;
+ int head_flags;
+
+ *changed = false;
+
+ head_target = refs_resolve_ref_unsafe(get_main_ref_store(repo), "HEAD",
+ RESOLVE_REF_NO_RECURSE | RESOLVE_REF_READING,
+ NULL, &head_flags);
+ if (!head_target)
+ return error(_("cannot look up HEAD"));
+
+ for (size_t i = 0; i < result->updates_nr; i++) {
+ if (!strcmp(result->updates[i].refname, head_target)) {
+ head_update = &result->updates[i];
+ break;
+ }
+ }
+
+ if (!head_update)
+ return 0;
+
+ old_head_commit = lookup_commit_reference(repo, &head_update->old_oid);
+ new_head_commit = lookup_commit_reference(repo, &head_update->new_oid);
+ if (!old_head_commit || !new_head_commit)
+ return error(_("cannot resolve HEAD commit"));
+
+ old_head_tree = repo_get_commit_tree(repo, old_head_commit);
+ new_head_tree = repo_get_commit_tree(repo, new_head_commit);
+ if (!old_head_tree || !new_head_tree)
+ return error(_("cannot resolve tree for HEAD"));
+
+ if (oideq(&old_head_tree->object.oid, &new_head_tree->object.oid))
+ return 0;
+
+ *old_head = old_head_commit;
+ *new_head = new_head_commit;
+ *changed = true;
+
+ return 0;
+}
+
+static int cmd_history_drop(int argc,
+ const char **argv,
+ const char *prefix,
+ struct repository *repo)
+{
+ const char * const usage[] = {
+ GIT_HISTORY_DROP_USAGE,
+ NULL,
+ };
+ enum replay_empty_commit_action empty = REPLAY_EMPTY_COMMIT_DROP;
+ enum ref_action action = REF_ACTION_DEFAULT;
+ int dry_run = 0;
+ struct option options[] = {
+ OPT_CALLBACK_F(0, "update-refs", &action, "(branches|head)",
+ N_("control which refs should be updated"),
+ PARSE_OPT_NONEG, parse_ref_action),
+ OPT_BOOL('n', "dry-run", &dry_run,
+ N_("perform a dry-run without updating any refs")),
+ OPT_CALLBACK_F(0, "empty", &empty, "(drop|keep|abort)",
+ N_("how to handle descendants that become empty"),
+ PARSE_OPT_NONEG, parse_opt_empty),
+ OPT_END(),
+ };
+ struct strbuf reflog_msg = STRBUF_INIT;
+ struct commit *original, *rewritten;
+ struct rev_info revs = { 0 };
+ struct replay_result result = { 0 };
+ struct commit *old_head, *new_head;
+ bool head_moves = false;
+ int ret;
+
+ argc = parse_options(argc, argv, prefix, options, usage, 0);
+ if (argc != 1) {
+ ret = error(_("command expects a single revision"));
+ goto out;
+ }
+ repo_config(repo, git_default_config, NULL);
+
+ if (action == REF_ACTION_DEFAULT)
+ action = REF_ACTION_BRANCHES;
+
+ original = lookup_commit_reference_by_name(argv[0]);
+ if (!original) {
+ ret = error(_("commit cannot be found: %s"), argv[0]);
+ goto out;
+ }
+
+ if (!original->parents) {
+ ret = error(_("cannot drop root commit %s: "
+ "it has no parent to replay onto"),
+ argv[0]);
+ goto out;
+ } else if (original->parents->next) {
+ ret = error(_("cannot drop merge commit: %s"), argv[0]);
+ goto out;
+ }
+
+ ret = setup_revwalk(repo, action, original, &revs);
+ if (ret)
+ goto out;
+
+ rewritten = original->parents->item;
+
+ ret = compute_pending_ref_updates(&revs, action, original, rewritten,
+ empty, &result);
+ if (ret) {
+ ret = error(_("failed replaying descendants"));
+ goto out;
+ }
+
+ /*
+ * If HEAD will move as a result of the rewrite then we'll have to
+ * merge in the changes into the worktree and index. This merge can of
+ * course conflict, which will cause the whole operation to abort.
+ *
+ * If we had already updated the refs at that point then we'd have an
+ * inconsistent repository state. So we first perform a dry-run merge
+ * here before updating refs.
+ */
+ if (!is_bare_repository()) {
+ ret = find_head_tree_change(repo, &result, &old_head,
+ &new_head, &head_moves);
+ if (ret < 0)
+ goto out;
+
+ if (head_moves && update_worktree(repo, old_head, new_head, true) < 0) {
+ ret = error(_("dropping this commit would "
+ "overwrite local changes; aborting"));
+ goto out;
+ }
+ }
+
+ strbuf_addf(&reflog_msg, "drop: dropping %s", argv[0]);
+ ret = apply_pending_ref_updates(repo, &result, reflog_msg.buf, dry_run);
+ if (ret < 0) {
+ ret = error(_("failed to update references"));
+ goto out;
+ }
+
+ if (!dry_run && head_moves && update_worktree(repo, old_head, new_head, false) < 0) {
+ ret = error(_("could not update working tree to new commit %s"),
+ oid_to_hex(&new_head->object.oid));
+ goto out;
+ }
+
+ ret = 0;
+
+out:
+ replay_result_release(&result);
+ strbuf_release(&reflog_msg);
+ release_revisions(&revs);
+ return ret;
+}
+
int cmd_history(int argc,
const char **argv,
const char *prefix,
struct repository *repo)
{
const char * const usage[] = {
+ GIT_HISTORY_DROP_USAGE,
GIT_HISTORY_FIXUP_USAGE,
GIT_HISTORY_REWORD_USAGE,
GIT_HISTORY_SPLIT_USAGE,
@@ -1012,6 +1195,7 @@ int cmd_history(int argc,
};
parse_opt_subcommand_fn *fn = NULL;
struct option options[] = {
+ OPT_SUBCOMMAND("drop", &fn, cmd_history_drop),
OPT_SUBCOMMAND("fixup", &fn, cmd_history_fixup),
OPT_SUBCOMMAND("reword", &fn, cmd_history_reword),
OPT_SUBCOMMAND("split", &fn, cmd_history_split),
diff --git a/t/meson.build b/t/meson.build
index 2af8d01279..d5e71056b2 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -399,6 +399,7 @@ integration_tests = [
't3451-history-reword.sh',
't3452-history-split.sh',
't3453-history-fixup.sh',
+ 't3454-history-drop.sh',
't3500-cherry.sh',
't3501-revert-cherry-pick.sh',
't3502-cherry-pick-merge.sh',
diff --git a/t/t3454-history-drop.sh b/t/t3454-history-drop.sh
new file mode 100755
index 0000000000..68a86d1e37
--- /dev/null
+++ b/t/t3454-history-drop.sh
@@ -0,0 +1,561 @@
+#!/bin/sh
+
+test_description='tests for git-history drop subcommand'
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY/lib-log-graph.sh"
+
+expect_graph () {
+ cat >expect &&
+ lib_test_cmp_graph --format=%s "$@"
+}
+
+expect_log () {
+ git log --format="%s" "$@" >actual &&
+ cat >expect &&
+ test_cmp expect actual
+}
+
+test_expect_success 'errors on missing commit argument' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit initial &&
+ test_must_fail git history drop 2>err &&
+ test_grep "command expects a single revision" err
+ )
+'
+
+test_expect_success 'errors on too many arguments' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit initial &&
+ test_must_fail git history drop HEAD HEAD 2>err &&
+ test_grep "command expects a single revision" err
+ )
+'
+
+test_expect_success 'errors on unknown revision' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit initial &&
+ test_must_fail git history drop does-not-exist 2>err &&
+ test_grep "commit cannot be found: does-not-exist" err
+ )
+'
+
+test_expect_success 'errors with invalid --empty= value' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit initial &&
+ test_commit second &&
+ test_must_fail git history drop --empty=bogus HEAD 2>err &&
+ test_grep "unrecognized.*--empty.*bogus" err
+ )
+'
+
+test_expect_success 'drops a commit in the middle and replays descendants' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+ test_commit second &&
+ test_commit third &&
+
+ git symbolic-ref HEAD >expect &&
+ git history drop HEAD~ &&
+ git symbolic-ref HEAD >actual &&
+ test_cmp expect actual &&
+
+ expect_log <<-\EOF &&
+ third
+ first
+ EOF
+
+ test_must_fail git show HEAD:second.t &&
+ test_path_is_missing second.t &&
+
+ git reflog >reflog &&
+ test_grep "drop: dropping HEAD~" reflog
+ )
+'
+
+test_expect_success 'drops the HEAD commit' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+ test_commit second &&
+
+ git history drop HEAD &&
+
+ expect_log <<-\EOF
+ first
+ EOF
+ )
+'
+
+test_expect_success 'drops a commit on detached HEAD' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+ test_commit second &&
+ test_commit third &&
+ git checkout --detach HEAD &&
+
+ git history drop HEAD~ &&
+
+ expect_log <<-\EOF
+ third
+ first
+ EOF
+ )
+'
+
+# Note: in this case it would actually be fine to drop the root commit, as we
+# do have a descendant commit, and no reference points to the root commit
+# directly. So this is something that we may relax eventually.
+test_expect_success 'refuses to drop the root commit' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+ test_commit second &&
+
+ test_must_fail git history drop HEAD~ 2>err &&
+ test_grep "cannot drop root commit" err
+ )
+'
+
+# In contrast to the above case, we actually don't want to drop the root commit
+# here as that would cause us to end up with an empty commit graph.
+test_expect_success 'refuses to drop the root commit when branch becomes empty' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+
+ test_must_fail git history drop HEAD 2>err &&
+ test_grep "cannot drop root commit" err
+ )
+'
+
+test_expect_success 'refuses to drop a merge commit' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit base &&
+ git branch branch &&
+ test_commit ours &&
+ git switch branch &&
+ test_commit theirs &&
+ git switch - &&
+ git merge theirs &&
+
+ test_must_fail git history drop HEAD 2>err &&
+ test_grep "cannot drop merge commit" err
+ )
+'
+
+test_expect_success 'refuses when descendants contain a merge commit' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit base &&
+ test_commit middle &&
+ git branch branch &&
+ test_commit ours &&
+ git switch branch &&
+ test_commit theirs &&
+ git switch - &&
+ git merge theirs &&
+
+ test_must_fail git history drop middle 2>err &&
+ test_grep "replaying merge commits is not supported yet" err
+ )
+'
+
+test_expect_success 'works in a bare repository' '
+ test_when_finished "rm -rf repo repo.git" &&
+
+ git init repo &&
+ test_commit -C repo first &&
+ test_commit -C repo second &&
+ test_commit -C repo third &&
+
+ git clone --bare repo repo.git &&
+ (
+ cd repo.git &&
+
+ git history drop HEAD~ &&
+ expect_log <<-\EOF
+ third
+ first
+ EOF
+ )
+'
+
+test_expect_success 'updates branches on other lines of descent' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit base &&
+ test_commit target &&
+ git branch theirs &&
+ test_commit ours &&
+ git switch theirs &&
+ test_commit theirs &&
+
+ expect_graph --branches <<-\EOF &&
+ * theirs
+ | * ours
+ |/
+ * target
+ * base
+ EOF
+
+ git history drop target &&
+
+ expect_graph --branches <<-\EOF
+ * ours
+ | * theirs
+ |/
+ * base
+ EOF
+ )
+'
+
+test_expect_success 'moves branch pointing at dropped commit to its parent' '
+ test_when_finished "rm -rf repo" &&
+ git init repo --initial-branch=main &&
+ (
+ cd repo &&
+ test_commit first &&
+ test_commit second &&
+ git branch points-at-second &&
+ test_commit third &&
+
+ git rev-parse first >expect &&
+ git history drop second &&
+ git rev-parse points-at-second >actual &&
+ test_cmp expect actual &&
+
+ expect_log --format="%s %D" --branches <<-\EOF
+ third HEAD -> main
+ first tag: first, points-at-second
+ EOF
+ )
+'
+
+test_expect_success '--dry-run prints ref updates without modifying repo' '
+ test_when_finished "rm -rf repo" &&
+ git init repo --initial-branch=main &&
+ (
+ cd repo &&
+ test_commit base &&
+ git branch branch &&
+ test_commit middle &&
+ test_commit ours &&
+ git switch branch &&
+ test_commit theirs &&
+
+ git refs list >refs-expect &&
+ git history drop --dry-run main~ >updates &&
+ git refs list >refs-actual &&
+ test_cmp refs-expect refs-actual &&
+ test_grep "update refs/heads/main" updates &&
+
+ git update-ref --stdin <updates &&
+ expect_log main <<-\EOF
+ ours
+ base
+ EOF
+ )
+'
+
+test_expect_success '--dry-run detects conflicts with modified working tree' '
+ test_when_finished "rm -rf repo" &&
+ git init repo --initial-branch=main &&
+ (
+ cd repo &&
+ test_commit first &&
+ test_commit second modify-me &&
+ echo modified >modify-me &&
+
+ git refs list >refs-expect &&
+ git diff >diff-expect &&
+ test_must_fail git history drop --dry-run HEAD 2>err &&
+ test_grep "dropping this commit would overwrite local changes" err &&
+ git diff >diff-actual &&
+ git refs list >refs-actual &&
+
+ test_cmp diff-expect diff-actual &&
+ test_cmp refs-expect refs-actual
+ )
+'
+
+test_expect_success '--update-refs=head updates only HEAD' '
+ test_when_finished "rm -rf repo" &&
+ git init repo --initial-branch=main &&
+ (
+ cd repo &&
+ test_commit base &&
+ test_commit target &&
+ git branch theirs &&
+ test_commit ours &&
+ git switch theirs &&
+ test_commit theirs &&
+
+ # When told to update HEAD only, the command refuses to
+ # rewrite commits that are not an ancestor of HEAD.
+ test_must_fail git history drop --update-refs=head main 2>err &&
+ test_grep "rewritten commit must be an ancestor of HEAD" err &&
+
+ expect_graph --branches <<-\EOF &&
+ * theirs
+ | * ours
+ |/
+ * target
+ * base
+ EOF
+
+ git switch main &&
+ git history drop --update-refs=head target &&
+
+ expect_graph --branches <<-\EOF
+ * ours
+ | * theirs
+ | * target
+ |/
+ * base
+ EOF
+ )
+'
+
+test_expect_success '--update-refs=head can rewrite detached HEAD' '
+ test_when_finished "rm -rf repo" &&
+ git init repo --initial-branch=main &&
+ (
+ cd repo &&
+ test_commit first &&
+ test_commit second &&
+ test_commit third &&
+ git switch --detach HEAD &&
+
+ git history drop --update-refs=head second &&
+
+ expect_log HEAD <<-\EOF &&
+ third
+ first
+ EOF
+ expect_log main <<-\EOF
+ third
+ second
+ first
+ EOF
+ )
+'
+
+test_expect_success 'conflict with replayed commit aborts cleanly' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit base &&
+ test_commit conflict-a file &&
+ test_commit conflict-b file &&
+
+ git refs list >refs-expect &&
+ test_must_fail git history drop HEAD~ 2>err &&
+ test_grep "failed replaying descendants" err &&
+ git refs list >refs-actual &&
+ test_cmp refs-expect refs-actual
+ )
+'
+
+# Build a history where a descendant of the drop target reverts the change
+# introduced by the drop target. After dropping, the descendant's diff applies
+# against a tree that already lacks the change, so it becomes empty.
+setup_empty_descendant_repo () {
+ git init "$1" &&
+ (
+ cd "$1" &&
+ echo C1 >file &&
+ git add file &&
+ git commit -m "base" &&
+ git tag base &&
+ echo C2 >file &&
+ git add file &&
+ git commit -m "drop-me" &&
+ git tag drop-me &&
+ test_commit middle &&
+ echo C1 >file &&
+ git add file &&
+ git commit -m "revert-drop-me" &&
+ git tag revert-drop-me
+ )
+}
+
+test_expect_success '--empty=drop drops descendants that become empty' '
+ test_when_finished "rm -rf repo" &&
+ setup_empty_descendant_repo repo &&
+ (
+ cd repo &&
+
+ git history drop --empty=drop drop-me &&
+
+ expect_log <<-\EOF
+ middle
+ base
+ EOF
+ )
+'
+
+test_expect_success '--empty=keep keeps descendants that become empty' '
+ test_when_finished "rm -rf repo" &&
+ setup_empty_descendant_repo repo &&
+ (
+ cd repo &&
+
+ git history drop --empty=keep drop-me &&
+
+ expect_log <<-\EOF &&
+ revert-drop-me
+ middle
+ base
+ EOF
+ git diff HEAD~ HEAD >diff &&
+ test_must_be_empty diff
+ )
+'
+
+test_expect_success '--empty=abort errors out when a descendant becomes empty' '
+ test_when_finished "rm -rf repo" &&
+ setup_empty_descendant_repo repo &&
+ (
+ cd repo &&
+
+ test_must_fail git history drop --empty=abort drop-me 2>err &&
+ test_grep "became empty after replay" err
+ )
+'
+
+test_expect_success 'updates index and worktree when HEAD moves' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+ test_commit second &&
+ test_commit third &&
+
+ git history drop second &&
+
+ # Worktree should no longer contain second.t.
+ test_path_is_missing second.t &&
+ test_path_is_file first.t &&
+ test_path_is_file third.t &&
+
+ # Index and worktree should both match the new HEAD.
+ git status --porcelain --untracked-files=no >status &&
+ test_must_be_empty status
+ )
+'
+
+test_expect_success 'updates worktree when dropping HEAD itself' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+ test_commit second &&
+
+ git history drop HEAD &&
+
+ test_path_is_missing second.t &&
+ test_path_is_file first.t &&
+
+ git status --porcelain --untracked-files=no >status &&
+ test_must_be_empty status
+ )
+'
+
+test_expect_success 'preserves unrelated unstaged modifications' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+ echo first-content >unrelated.txt &&
+ git add unrelated.txt &&
+ git commit -m "add unrelated" &&
+ test_commit second &&
+ test_commit third &&
+
+ echo locally-modified >unrelated.txt &&
+
+ git diff >diff-expect &&
+ git history drop second &&
+ git diff >diff-actual &&
+ test_cmp diff-expect diff-actual &&
+ test_path_is_missing second.t
+ )
+'
+
+test_expect_success 'preserves unrelated staged changes' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit first &&
+ echo first-content >unrelated.txt &&
+ git add unrelated.txt &&
+ git commit -m "add unrelated" &&
+ test_commit second &&
+ test_commit third &&
+
+ echo staged-change >unrelated.txt &&
+ git add unrelated.txt &&
+
+ git diff --cached >diff-expect &&
+ git history drop second &&
+ git diff --cached >diff-actual &&
+ test_cmp diff-expect diff-actual &&
+ test_path_is_missing second.t
+ )
+'
+
+test_expect_success 'aborts when local modifications would be overwritten' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit base &&
+ test_commit conflict &&
+
+ echo local-edit >conflict.t &&
+ git diff >diff-expect &&
+ test_must_fail git history drop HEAD 2>err &&
+ test_grep "would overwrite local changes" err &&
+ git diff >diff-actual &&
+ test_cmp diff-expect diff-actual
+ )
+'
+
+test_done
--
2.55.0.795.g602f6c329a.dirty
^ permalink raw reply related
* [PATCH v8 10/11] builtin/history: split handling of ref updates into two phases
From: Patrick Steinhardt @ 2026-07-01 11:35 UTC (permalink / raw)
To: git
Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood,
Christian Couder
In-Reply-To: <20260701-b4-pks-history-drop-v8-0-19b5cdf1facd@pks.im>
The function `handle_reference_updates()` is used by git-history(1) to
update all references that refer to commits that have been rewritten. As
such, it performs two steps:
- It gathers the references that need to be updated in the first
place.
- It prepares and commits the reference transaction.
In a subsequent commit we'll want to handle those two steps separately.
Prepare for this by splitting up the function into two.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/history.c | 100 +++++++++++++++++++++++++++++++++---------------------
1 file changed, 62 insertions(+), 38 deletions(-)
diff --git a/builtin/history.c b/builtin/history.c
index 0fc06fb204..22b9fcb4a4 100644
--- a/builtin/history.c
+++ b/builtin/history.c
@@ -333,21 +333,17 @@ static int handle_ref_update(struct ref_transaction *transaction,
NULL, NULL, 0, reflog_msg, err);
}
-static int handle_reference_updates(struct rev_info *revs,
- enum ref_action action,
- struct commit *original,
- struct commit *rewritten,
- const char *reflog_msg,
- int dry_run,
- enum replay_empty_commit_action empty)
+static int compute_pending_ref_updates(struct rev_info *revs,
+ enum ref_action action,
+ struct commit *original,
+ struct commit *rewritten,
+ enum replay_empty_commit_action empty,
+ struct replay_result *result)
{
const struct name_decoration *decoration;
struct replay_revisions_options opts = {
.empty = empty,
};
- struct replay_result result = { 0 };
- struct ref_transaction *transaction = NULL;
- struct strbuf err = STRBUF_INIT;
char hex[GIT_MAX_HEXSZ + 1];
bool detached_head;
int head_flags = 0;
@@ -359,34 +355,13 @@ static int handle_reference_updates(struct rev_info *revs,
opts.onto = oid_to_hex_r(hex, &rewritten->object.oid);
- ret = replay_revisions(revs, &opts, &result);
+ ret = replay_revisions(revs, &opts, result);
if (ret)
- goto out;
+ return ret;
if (action != REF_ACTION_BRANCHES && action != REF_ACTION_HEAD)
BUG("unsupported ref action %d", action);
- if (!dry_run) {
- transaction = ref_store_transaction_begin(get_main_ref_store(revs->repo), 0, &err);
- if (!transaction) {
- ret = error(_("failed to begin ref transaction: %s"), err.buf);
- goto out;
- }
- }
-
- for (size_t i = 0; i < result.updates_nr; i++) {
- ret = handle_ref_update(transaction,
- result.updates[i].refname,
- &result.updates[i].new_oid,
- &result.updates[i].old_oid,
- reflog_msg, &err);
- if (ret) {
- ret = error(_("failed to update ref '%s': %s"),
- result.updates[i].refname, err.buf);
- goto out;
- }
- }
-
/*
* `replay_revisions()` only updates references that are
* ancestors of `rewritten`, so we need to manually
@@ -414,14 +389,41 @@ static int handle_reference_updates(struct rev_info *revs,
!detached_head)
continue;
+ replay_result_queue_update(result, decoration->name,
+ &original->object.oid,
+ &rewritten->object.oid);
+ }
+
+ return 0;
+}
+
+static int apply_pending_ref_updates(struct repository *repo,
+ const struct replay_result *result,
+ const char *reflog_msg,
+ int dry_run)
+{
+ struct ref_transaction *transaction = NULL;
+ struct strbuf err = STRBUF_INIT;
+ int ret;
+
+ if (!dry_run) {
+ transaction = ref_store_transaction_begin(get_main_ref_store(repo),
+ 0, &err);
+ if (!transaction) {
+ ret = error(_("failed to begin ref transaction: %s"), err.buf);
+ goto out;
+ }
+ }
+
+ for (size_t i = 0; i < result->updates_nr; i++) {
ret = handle_ref_update(transaction,
- decoration->name,
- &rewritten->object.oid,
- &original->object.oid,
+ result->updates[i].refname,
+ &result->updates[i].new_oid,
+ &result->updates[i].old_oid,
reflog_msg, &err);
if (ret) {
ret = error(_("failed to update ref '%s': %s"),
- decoration->name, err.buf);
+ result->updates[i].refname, err.buf);
goto out;
}
}
@@ -435,11 +437,33 @@ static int handle_reference_updates(struct rev_info *revs,
out:
ref_transaction_free(transaction);
- replay_result_release(&result);
strbuf_release(&err);
return ret;
}
+static int handle_reference_updates(struct rev_info *revs,
+ enum ref_action action,
+ struct commit *original,
+ struct commit *rewritten,
+ const char *reflog_msg,
+ int dry_run,
+ enum replay_empty_commit_action empty)
+{
+ struct replay_result result = { 0 };
+ int ret;
+
+ ret = compute_pending_ref_updates(revs, action, original, rewritten,
+ empty, &result);
+ if (ret)
+ goto out;
+
+ ret = apply_pending_ref_updates(revs->repo, &result, reflog_msg, dry_run);
+
+out:
+ replay_result_release(&result);
+ return ret;
+}
+
static int commit_became_empty(struct repository *repo,
struct commit *original,
struct tree *result)
--
2.55.0.795.g602f6c329a.dirty
^ permalink raw reply related
* [PATCH v8 09/11] replay: expose `replay_result_queue_update()`
From: Patrick Steinhardt @ 2026-07-01 11:35 UTC (permalink / raw)
To: git
Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood,
Christian Couder
In-Reply-To: <20260701-b4-pks-history-drop-v8-0-19b5cdf1facd@pks.im>
Expose `replay_result_queue_update()`, which is used to append another
reference update to the replay result. This function will be used in a
subsequent commit.
Suggested-by: Christian Couder <christian.couder@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
replay.c | 8 ++++----
replay.h | 5 +++++
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/replay.c b/replay.c
index 4ef8abb607..7c8433107b 100644
--- a/replay.c
+++ b/replay.c
@@ -351,10 +351,10 @@ void replay_result_release(struct replay_result *result)
free(result->updates);
}
-static void replay_result_queue_update(struct replay_result *result,
- const char *refname,
- const struct object_id *old_oid,
- const struct object_id *new_oid)
+void replay_result_queue_update(struct replay_result *result,
+ const char *refname,
+ const struct object_id *old_oid,
+ const struct object_id *new_oid)
{
ALLOC_GROW(result->updates, result->updates_nr + 1, result->updates_alloc);
result->updates[result->updates_nr].refname = xstrdup(refname);
diff --git a/replay.h b/replay.h
index 1851a07705..da83b65345 100644
--- a/replay.h
+++ b/replay.h
@@ -80,6 +80,11 @@ struct replay_result {
void replay_result_release(struct replay_result *result);
+void replay_result_queue_update(struct replay_result *result,
+ const char *refname,
+ const struct object_id *old_oid,
+ const struct object_id *new_oid);
+
/*
* Replay a set of commits onto a new location. Leaves both the working tree,
* index and references untouched. Reference updates caused by the replay will
--
2.55.0.795.g602f6c329a.dirty
^ permalink raw reply related
* [PATCH v8 08/11] reset: stop assuming that the caller passes in a clean index
From: Patrick Steinhardt @ 2026-07-01 11:35 UTC (permalink / raw)
To: git
Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood,
Christian Couder
In-Reply-To: <20260701-b4-pks-history-drop-v8-0-19b5cdf1facd@pks.im>
In 652bd0211d (rebase: use 'skip_cache_tree_update' option, 2022-11-10),
we updated `reset_working_tree()` to stop updating the index tree cache.
This was done as a performance optimization: the function is only called
by "sequencer.c" and "rebase.c", both of which assume a clean index
before they perform their operation, so we know that the end result will
be a clean index, too. Consequently, we can skip recomputing the cache
as we can instead use `prime_cache_tree()` directly.
In a subsequent commit we're about to add a new caller though where the
assumption doesn't hold anymore: the index may be dirty before calling
`reset_working_tree()`, and consequently we cannot prime the cache with
a given tree anymore as the index and tree will mismatch.
Adapt the logic so that we only skip the cache tree update in case we're
doing a hard reset. While we could introduce logic that only skips the
update in case the incoming index was dirty already, that doesn't really
feel worth it: after all, the mentioned commit says itself that the
performance improvement was negligible anyway.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reset.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/reset.c b/reset.c
index 06f375f296..ff87e3e357 100644
--- a/reset.c
+++ b/reset.c
@@ -167,10 +167,11 @@ int reset_working_tree(struct repository *r,
unpack_tree_opts.dry_run = dry_run;
unpack_tree_opts.merge = 1;
unpack_tree_opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */
- unpack_tree_opts.skip_cache_tree_update = 1;
init_checkout_metadata(&unpack_tree_opts.meta, switch_to_branch, oid, NULL);
- if (reset_hard)
+ if (reset_hard) {
+ unpack_tree_opts.skip_cache_tree_update = 1;
unpack_tree_opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;
+ }
if (!reset_hard && !fill_tree_descriptor(r, &desc[nr++], &head_oid)) {
ret = error(_("failed to find tree of %s"),
@@ -197,7 +198,8 @@ int reset_working_tree(struct repository *r,
goto leave_reset_head;
}
- prime_cache_tree(r, r->index, tree);
+ if (reset_hard)
+ prime_cache_tree(r, r->index, tree);
if (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0) {
ret = error(_("could not write index"));
--
2.55.0.795.g602f6c329a.dirty
^ permalink raw reply related
* [PATCH v8 07/11] reset: allow the caller to specify the current HEAD object
From: Patrick Steinhardt @ 2026-07-01 11:35 UTC (permalink / raw)
To: git
Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood,
Christian Couder
In-Reply-To: <20260701-b4-pks-history-drop-v8-0-19b5cdf1facd@pks.im>
When calling `reset_working_tree()` we automatically derive the commit
that the callers wants to move from by reading the HEAD commit. Some
callers may already have resolved it, or they may want to move from a
different commit that doesn't match HEAD.
Introduce a new `oid_from` option that lets the caller specify the
commit.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reset.c | 5 ++++-
reset.h | 5 +++++
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/reset.c b/reset.c
index 4bde5d8dc6..06f375f296 100644
--- a/reset.c
+++ b/reset.c
@@ -122,7 +122,10 @@ int reset_working_tree(struct repository *r,
goto leave_reset_head;
}
- if (!repo_get_oid(r, "HEAD", &head_oid)) {
+ if (opts->oid_from) {
+ oidcpy(&head_oid, opts->oid_from);
+ head = &head_oid;
+ } else if (!repo_get_oid(r, "HEAD", &head_oid)) {
head = &head_oid;
} else if (!oid || !reset_hard) {
ret = error(_("could not determine HEAD revision"));
diff --git a/reset.h b/reset.h
index 38b2891b53..4c992ba671 100644
--- a/reset.h
+++ b/reset.h
@@ -37,6 +37,11 @@ struct reset_working_tree_options {
* The commit to checkout/reset to. Defaults to HEAD.
*/
const struct object_id *oid;
+ /*
+ * The commit to checkout/reset from when doing a two-way merge. This
+ * is used as one of the sides to merge.
+ */
+ const struct object_id *oid_from;
/*
* Optional value to set ORIG_HEAD. Defaults to HEAD.
*/
--
2.55.0.795.g602f6c329a.dirty
^ permalink raw reply related
* [PATCH v8 06/11] reset: introduce ability to skip updating HEAD
From: Patrick Steinhardt @ 2026-07-01 11:35 UTC (permalink / raw)
To: git
Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood,
Christian Couder
In-Reply-To: <20260701-b4-pks-history-drop-v8-0-19b5cdf1facd@pks.im>
In a subsequent commit we'll introduce a new caller to
`reset_working_tree()` that really only wants to update the index and
working tree, without updating any references. Introduce a new flag that
makes the caller opt in to updating HEAD and adapt all callers to set
that flag.
Note that in a previous iteration we instead introduced a flag that made
callers opt out of updating any references. This was somewhat awkward
though because we already have the `UPDATE_ORIG_HEAD` flag, so the
result was somewhat inconsistent.
Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/rebase.c | 14 ++++++++++----
reset.c | 9 +++++++--
reset.h | 9 ++++++---
sequencer.c | 4 +++-
4 files changed, 26 insertions(+), 10 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 06dcbaf5e8..10a306310c 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -607,7 +607,8 @@ static int move_to_original_branch(struct rebase_options *opts)
strbuf_addf(&head_reflog, "%s (finish): returning to %s",
opts->reflog_action, opts->head_name);
ropts.branch = opts->head_name;
- ropts.flags = RESET_WORKING_TREE_REFS_ONLY;
+ ropts.flags = RESET_WORKING_TREE_REFS_ONLY |
+ RESET_WORKING_TREE_UPDATE_HEAD;
ropts.branch_msg = branch_reflog.buf;
ropts.head_msg = head_reflog.buf;
ret = reset_working_tree(the_repository, &ropts);
@@ -693,6 +694,7 @@ static int run_am(struct rebase_options *opts)
ropts.oid = &opts->orig_head->object.oid;
ropts.branch = opts->head_name;
ropts.default_reflog_action = opts->reflog_action;
+ ropts.flags = RESET_WORKING_TREE_UPDATE_HEAD;
reset_working_tree(the_repository, &ropts);
error(_("\ngit encountered an error while preparing the "
"patches to replay\n"
@@ -862,7 +864,8 @@ static int checkout_up_to_date(struct rebase_options *options)
options->reflog_action, options->switch_to);
ropts.oid = &options->orig_head->object.oid;
ropts.branch = options->head_name;
- ropts.flags = RESET_WORKING_TREE_RUN_POST_CHECKOUT_HOOK;
+ ropts.flags = RESET_WORKING_TREE_RUN_POST_CHECKOUT_HOOK |
+ RESET_WORKING_TREE_UPDATE_HEAD;
if (!ropts.branch)
ropts.flags |= RESET_WORKING_TREE_DETACH;
ropts.head_msg = buf.buf;
@@ -1384,7 +1387,8 @@ int cmd_rebase(int argc,
rerere_clear(the_repository, &merge_rr);
string_list_clear(&merge_rr, 1);
- ropts.flags = RESET_WORKING_TREE_HARD;
+ ropts.flags = RESET_WORKING_TREE_HARD |
+ RESET_WORKING_TREE_UPDATE_HEAD;
if (reset_working_tree(the_repository, &ropts) < 0)
die(_("could not discard worktree changes"));
remove_branch_state(the_repository, 0);
@@ -1409,7 +1413,8 @@ int cmd_rebase(int argc,
ropts.oid = &options.orig_head->object.oid;
ropts.head_msg = head_msg.buf;
ropts.branch = options.head_name;
- ropts.flags = RESET_WORKING_TREE_HARD;
+ ropts.flags = RESET_WORKING_TREE_HARD |
+ RESET_WORKING_TREE_UPDATE_HEAD;
if (reset_working_tree(the_repository, &ropts) < 0)
die(_("could not move back to %s"),
oid_to_hex(&options.orig_head->object.oid));
@@ -1877,6 +1882,7 @@ int cmd_rebase(int argc,
ropts.oid = &options.onto->object.oid;
ropts.orig_head = &options.orig_head->object.oid;
ropts.flags = RESET_WORKING_TREE_DETACH |
+ RESET_WORKING_TREE_UPDATE_HEAD |
RESET_WORKING_TREE_UPDATE_ORIG_HEAD |
RESET_WORKING_TREE_RUN_POST_CHECKOUT_HOOK;
ropts.head_msg = msg.buf;
diff --git a/reset.c b/reset.c
index 99f2c1b012..4bde5d8dc6 100644
--- a/reset.c
+++ b/reset.c
@@ -92,6 +92,7 @@ int reset_working_tree(struct repository *r,
const char *switch_to_branch = opts->branch;
unsigned reset_hard = opts->flags & RESET_WORKING_TREE_HARD;
unsigned refs_only = opts->flags & RESET_WORKING_TREE_REFS_ONLY;
+ unsigned update_head = opts->flags & RESET_WORKING_TREE_UPDATE_HEAD;
unsigned update_orig_head = opts->flags & RESET_WORKING_TREE_UPDATE_ORIG_HEAD;
unsigned dry_run = opts->flags & RESET_WORKING_TREE_DRY_RUN;
struct object_id *head = NULL, head_oid;
@@ -113,6 +114,9 @@ int reset_working_tree(struct repository *r,
if (opts->branch_msg && !opts->branch)
BUG("branch reflog message given without a branch");
+ if (update_orig_head && !update_head)
+ BUG("cannot update ORIG_HEAD without updating HEAD" );
+
if (!refs_only && !dry_run && repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) {
ret = -1;
goto leave_reset_head;
@@ -129,7 +133,7 @@ int reset_working_tree(struct repository *r,
oid = &head_oid;
if (refs_only) {
- if (!dry_run)
+ if (!dry_run && update_head)
return update_refs(r, opts, oid, head);
return 0;
}
@@ -197,7 +201,8 @@ int reset_working_tree(struct repository *r,
goto leave_reset_head;
}
- if (oid != &head_oid || update_orig_head || switch_to_branch)
+ if (update_head &&
+ (oid != &head_oid || update_orig_head || switch_to_branch))
ret = update_refs(r, opts, oid, head);
leave_reset_head:
diff --git a/reset.h b/reset.h
index 898e4a1e95..38b2891b53 100644
--- a/reset.h
+++ b/reset.h
@@ -19,14 +19,17 @@ enum reset_working_tree_flags {
/* Only update refs, do not touch the worktree */
RESET_WORKING_TREE_REFS_ONLY = (1 << 3),
- /* Update ORIG_HEAD as well as HEAD */
- RESET_WORKING_TREE_UPDATE_ORIG_HEAD = (1 << 4),
+ /* Update HEAD */
+ RESET_WORKING_TREE_UPDATE_HEAD = (1 << 4),
+
+ /* Update ORIG_HEAD */
+ RESET_WORKING_TREE_UPDATE_ORIG_HEAD = (1 << 5),
/*
* Perform a dry-run by performing the operation without updating
* any user-visible state.
*/
- RESET_WORKING_TREE_DRY_RUN = (1 << 5),
+ RESET_WORKING_TREE_DRY_RUN = (1 << 6),
};
struct reset_working_tree_options {
diff --git a/sequencer.c b/sequencer.c
index 4efe831178..e905b1b2d9 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4678,7 +4678,8 @@ static void create_autostash_internal(struct repository *r,
has_uncommitted_changes(r, 1)) {
struct child_process stash = CHILD_PROCESS_INIT;
struct reset_working_tree_options ropts = {
- .flags = RESET_WORKING_TREE_HARD,
+ .flags = RESET_WORKING_TREE_HARD |
+ RESET_WORKING_TREE_UPDATE_HEAD,
};
struct object_id oid;
@@ -4873,6 +4874,7 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,
.oid = onto,
.orig_head = orig_head,
.flags = RESET_WORKING_TREE_DETACH |
+ RESET_WORKING_TREE_UPDATE_HEAD |
RESET_WORKING_TREE_UPDATE_ORIG_HEAD |
RESET_WORKING_TREE_RUN_POST_CHECKOUT_HOOK,
.head_msg = reflog_message(opts, "start", "checkout %s",
--
2.55.0.795.g602f6c329a.dirty
^ permalink raw reply related
* [PATCH v8 05/11] reset: introduce dry-run mode
From: Patrick Steinhardt @ 2026-07-01 11:35 UTC (permalink / raw)
To: git
Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood,
Christian Couder
In-Reply-To: <20260701-b4-pks-history-drop-v8-0-19b5cdf1facd@pks.im>
In a subsequent commit we'll add another caller to `reset_working_tree()`
that wants to perform a dry-run check of whether it would be possible to
update the index and working tree when moving to a new commit. Introduce
a new flag that lets the caller perform this operation.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reset.c | 44 +++++++++++++++++++++++++++++++++-----------
reset.h | 6 ++++++
2 files changed, 39 insertions(+), 11 deletions(-)
diff --git a/reset.c b/reset.c
index 4ca7f23a25..99f2c1b012 100644
--- a/reset.c
+++ b/reset.c
@@ -93,11 +93,14 @@ int reset_working_tree(struct repository *r,
unsigned reset_hard = opts->flags & RESET_WORKING_TREE_HARD;
unsigned refs_only = opts->flags & RESET_WORKING_TREE_REFS_ONLY;
unsigned update_orig_head = opts->flags & RESET_WORKING_TREE_UPDATE_ORIG_HEAD;
+ unsigned dry_run = opts->flags & RESET_WORKING_TREE_DRY_RUN;
struct object_id *head = NULL, head_oid;
struct tree_desc desc[2] = { { NULL }, { NULL } };
struct lock_file lock = LOCK_INIT;
struct unpack_trees_options unpack_tree_opts = { 0 };
struct tree *tree;
+ struct index_state scratch_index = INDEX_STATE_INIT(r);
+ struct index_state *istate;
const char *action;
int ret = 0, nr = 0;
@@ -110,7 +113,7 @@ int reset_working_tree(struct repository *r,
if (opts->branch_msg && !opts->branch)
BUG("branch reflog message given without a branch");
- if (!refs_only && repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) {
+ if (!refs_only && !dry_run && repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) {
ret = -1;
goto leave_reset_head;
}
@@ -125,16 +128,36 @@ int reset_working_tree(struct repository *r,
if (!oid)
oid = &head_oid;
- if (refs_only)
- return update_refs(r, opts, oid, head);
+ if (refs_only) {
+ if (!dry_run)
+ return update_refs(r, opts, oid, head);
+ return 0;
+ }
+
+ if (dry_run) {
+ if (read_index_from(&scratch_index, r->index_file, r->gitdir) < 0 ||
+ index_state_unmerged_to_stage0(&scratch_index) < 0) {
+ ret = error(_("could not read index"));
+ goto leave_reset_head;
+ }
+
+ istate = &scratch_index;
+ } else {
+ if (repo_read_index_unmerged(r) < 0) {
+ ret = error(_("could not read index"));
+ goto leave_reset_head;
+ }
+ istate = r->index;
+ }
action = reset_hard ? "reset" : "checkout";
setup_unpack_trees_porcelain(&unpack_tree_opts, action);
unpack_tree_opts.head_idx = 1;
- unpack_tree_opts.src_index = r->index;
- unpack_tree_opts.dst_index = r->index;
+ unpack_tree_opts.src_index = istate;
+ unpack_tree_opts.dst_index = istate;
unpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge;
- unpack_tree_opts.update = 1;
+ unpack_tree_opts.update = !dry_run;
+ unpack_tree_opts.dry_run = dry_run;
unpack_tree_opts.merge = 1;
unpack_tree_opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */
unpack_tree_opts.skip_cache_tree_update = 1;
@@ -142,11 +165,6 @@ int reset_working_tree(struct repository *r,
if (reset_hard)
unpack_tree_opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;
- if (repo_read_index_unmerged(r) < 0) {
- ret = error(_("could not read index"));
- goto leave_reset_head;
- }
-
if (!reset_hard && !fill_tree_descriptor(r, &desc[nr++], &head_oid)) {
ret = error(_("failed to find tree of %s"),
oid_to_hex(&head_oid));
@@ -163,6 +181,9 @@ int reset_working_tree(struct repository *r,
goto leave_reset_head;
}
+ if (dry_run)
+ goto leave_reset_head;
+
tree = repo_parse_tree_indirect(r, oid);
if (!tree) {
ret = error(_("unable to read tree (%s)"), oid_to_hex(oid));
@@ -182,6 +203,7 @@ int reset_working_tree(struct repository *r,
leave_reset_head:
rollback_lock_file(&lock);
clear_unpack_trees_porcelain(&unpack_tree_opts);
+ release_index(&scratch_index);
while (nr)
free((void *)desc[--nr].buffer);
return ret;
diff --git a/reset.h b/reset.h
index 2e5826de99..898e4a1e95 100644
--- a/reset.h
+++ b/reset.h
@@ -21,6 +21,12 @@ enum reset_working_tree_flags {
/* Update ORIG_HEAD as well as HEAD */
RESET_WORKING_TREE_UPDATE_ORIG_HEAD = (1 << 4),
+
+ /*
+ * Perform a dry-run by performing the operation without updating
+ * any user-visible state.
+ */
+ RESET_WORKING_TREE_DRY_RUN = (1 << 5),
};
struct reset_working_tree_options {
--
2.55.0.795.g602f6c329a.dirty
^ permalink raw reply related
* [PATCH v8 04/11] reset: modernize flags passed to `reset_working_tree()`
From: Patrick Steinhardt @ 2026-07-01 11:35 UTC (permalink / raw)
To: git
Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood,
Christian Couder
In-Reply-To: <20260701-b4-pks-history-drop-v8-0-19b5cdf1facd@pks.im>
The flags passed to `reset_working_tree()` are declared as defines. This
has fallen a bit out of practice nowadays, where we instead prefer to
use enums. Furthermore, the prefix of those flags does not match the
function name anymore after the rename in the preceding commit.
Adapt the code to follow modern best practices and adapt the flag names.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/rebase.c | 15 ++++++++-------
reset.c | 12 ++++++------
reset.h | 31 +++++++++++++++++++------------
sequencer.c | 9 ++++++---
4 files changed, 39 insertions(+), 28 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 22fbba3c62..06dcbaf5e8 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -607,7 +607,7 @@ static int move_to_original_branch(struct rebase_options *opts)
strbuf_addf(&head_reflog, "%s (finish): returning to %s",
opts->reflog_action, opts->head_name);
ropts.branch = opts->head_name;
- ropts.flags = RESET_HEAD_REFS_ONLY;
+ ropts.flags = RESET_WORKING_TREE_REFS_ONLY;
ropts.branch_msg = branch_reflog.buf;
ropts.head_msg = head_reflog.buf;
ret = reset_working_tree(the_repository, &ropts);
@@ -862,9 +862,9 @@ static int checkout_up_to_date(struct rebase_options *options)
options->reflog_action, options->switch_to);
ropts.oid = &options->orig_head->object.oid;
ropts.branch = options->head_name;
- ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
+ ropts.flags = RESET_WORKING_TREE_RUN_POST_CHECKOUT_HOOK;
if (!ropts.branch)
- ropts.flags |= RESET_HEAD_DETACH;
+ ropts.flags |= RESET_WORKING_TREE_DETACH;
ropts.head_msg = buf.buf;
if (reset_working_tree(the_repository, &ropts) < 0)
ret = error(_("could not switch to %s"), options->switch_to);
@@ -1384,7 +1384,7 @@ int cmd_rebase(int argc,
rerere_clear(the_repository, &merge_rr);
string_list_clear(&merge_rr, 1);
- ropts.flags = RESET_HEAD_HARD;
+ ropts.flags = RESET_WORKING_TREE_HARD;
if (reset_working_tree(the_repository, &ropts) < 0)
die(_("could not discard worktree changes"));
remove_branch_state(the_repository, 0);
@@ -1409,7 +1409,7 @@ int cmd_rebase(int argc,
ropts.oid = &options.orig_head->object.oid;
ropts.head_msg = head_msg.buf;
ropts.branch = options.head_name;
- ropts.flags = RESET_HEAD_HARD;
+ ropts.flags = RESET_WORKING_TREE_HARD;
if (reset_working_tree(the_repository, &ropts) < 0)
die(_("could not move back to %s"),
oid_to_hex(&options.orig_head->object.oid));
@@ -1876,8 +1876,9 @@ int cmd_rebase(int argc,
options.reflog_action, options.onto_name);
ropts.oid = &options.onto->object.oid;
ropts.orig_head = &options.orig_head->object.oid;
- ropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |
- RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
+ ropts.flags = RESET_WORKING_TREE_DETACH |
+ RESET_WORKING_TREE_UPDATE_ORIG_HEAD |
+ RESET_WORKING_TREE_RUN_POST_CHECKOUT_HOOK;
ropts.head_msg = msg.buf;
ropts.default_reflog_action = options.reflog_action;
if (reset_working_tree(the_repository, &ropts)) {
diff --git a/reset.c b/reset.c
index 799596398b..4ca7f23a25 100644
--- a/reset.c
+++ b/reset.c
@@ -16,9 +16,9 @@ static int update_refs(struct repository *repo,
const struct object_id *oid,
const struct object_id *head)
{
- unsigned detach_head = opts->flags & RESET_HEAD_DETACH;
- unsigned run_hook = opts->flags & RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
- unsigned update_orig_head = opts->flags & RESET_ORIG_HEAD;
+ unsigned detach_head = opts->flags & RESET_WORKING_TREE_DETACH;
+ unsigned run_hook = opts->flags & RESET_WORKING_TREE_RUN_POST_CHECKOUT_HOOK;
+ unsigned update_orig_head = opts->flags & RESET_WORKING_TREE_UPDATE_ORIG_HEAD;
const struct object_id *orig_head = opts->orig_head;
const char *switch_to_branch = opts->branch;
const char *reflog_branch = opts->branch_msg;
@@ -90,9 +90,9 @@ int reset_working_tree(struct repository *r,
{
const struct object_id *oid = opts->oid;
const char *switch_to_branch = opts->branch;
- unsigned reset_hard = opts->flags & RESET_HEAD_HARD;
- unsigned refs_only = opts->flags & RESET_HEAD_REFS_ONLY;
- unsigned update_orig_head = opts->flags & RESET_ORIG_HEAD;
+ unsigned reset_hard = opts->flags & RESET_WORKING_TREE_HARD;
+ unsigned refs_only = opts->flags & RESET_WORKING_TREE_REFS_ONLY;
+ unsigned update_orig_head = opts->flags & RESET_WORKING_TREE_UPDATE_ORIG_HEAD;
struct object_id *head = NULL, head_oid;
struct tree_desc desc[2] = { { NULL }, { NULL } };
struct lock_file lock = LOCK_INIT;
diff --git a/reset.h b/reset.h
index f130152014..2e5826de99 100644
--- a/reset.h
+++ b/reset.h
@@ -6,16 +6,22 @@
#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION"
-/* Request a detached checkout */
-#define RESET_HEAD_DETACH (1<<0)
-/* Request a reset rather than a checkout */
-#define RESET_HEAD_HARD (1<<1)
-/* Run the post-checkout hook */
-#define RESET_HEAD_RUN_POST_CHECKOUT_HOOK (1<<2)
-/* Only update refs, do not touch the worktree */
-#define RESET_HEAD_REFS_ONLY (1<<3)
-/* Update ORIG_HEAD as well as HEAD */
-#define RESET_ORIG_HEAD (1<<4)
+enum reset_working_tree_flags {
+ /* Request a detached checkout */
+ RESET_WORKING_TREE_DETACH = (1 << 0),
+
+ /* Request a reset rather than a checkout */
+ RESET_WORKING_TREE_HARD = (1 << 1),
+
+ /* Run the post-checkout hook */
+ RESET_WORKING_TREE_RUN_POST_CHECKOUT_HOOK = (1 << 2),
+
+ /* Only update refs, do not touch the worktree */
+ RESET_WORKING_TREE_REFS_ONLY = (1 << 3),
+
+ /* Update ORIG_HEAD as well as HEAD */
+ RESET_WORKING_TREE_UPDATE_ORIG_HEAD = (1 << 4),
+};
struct reset_working_tree_options {
/*
@@ -33,7 +39,7 @@ struct reset_working_tree_options {
/*
* Flags defined above.
*/
- unsigned flags;
+ enum reset_working_tree_flags flags;
/*
* Optional reflog message for branch, defaults to head_msg.
*/
@@ -45,7 +51,8 @@ struct reset_working_tree_options {
const char *head_msg;
/*
* Optional reflog message for ORIG_HEAD, if this omitted and flags
- * contains RESET_ORIG_HEAD then default_reflog_action must be given.
+ * contains RESET_WORKING_TREE_UPDATE_ORIG_HEAD then
+ * default_reflog_action must be given.
*/
const char *orig_head_msg;
/*
diff --git a/sequencer.c b/sequencer.c
index d73ecf0384..4efe831178 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4677,7 +4677,9 @@ static void create_autostash_internal(struct repository *r,
if (has_unstaged_changes(r, 1) ||
has_uncommitted_changes(r, 1)) {
struct child_process stash = CHILD_PROCESS_INIT;
- struct reset_working_tree_options ropts = { .flags = RESET_HEAD_HARD };
+ struct reset_working_tree_options ropts = {
+ .flags = RESET_WORKING_TREE_HARD,
+ };
struct object_id oid;
strvec_pushl(&stash.args,
@@ -4870,8 +4872,9 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,
struct reset_working_tree_options ropts = {
.oid = onto,
.orig_head = orig_head,
- .flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |
- RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
+ .flags = RESET_WORKING_TREE_DETACH |
+ RESET_WORKING_TREE_UPDATE_ORIG_HEAD |
+ RESET_WORKING_TREE_RUN_POST_CHECKOUT_HOOK,
.head_msg = reflog_message(opts, "start", "checkout %s",
onto_name),
.default_reflog_action = sequencer_reflog_action(opts)
--
2.55.0.795.g602f6c329a.dirty
^ permalink raw reply related
* [PATCH v8 03/11] reset: rename `reset_head()`
From: Patrick Steinhardt @ 2026-07-01 11:35 UTC (permalink / raw)
To: git
Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood,
Christian Couder
In-Reply-To: <20260701-b4-pks-history-drop-v8-0-19b5cdf1facd@pks.im>
In a subsequent commit we're about to adapt `reset_head()` so that the
reference update to HEAD is optional, only. At this point the function
starts to feel misnamed, as it doesn't necessarily have anything to do
with the HEAD reference anymore. The gist of the function then is that
we reset the working tree to a specific new commit, updating both the
index and the checked-out files.
Rename it to `reset_working_tree()` to better reflect that.
Note that we don't adjust the flags yet. This will happen in a
subsequent commit.
Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/rebase.c | 20 ++++++++++----------
reset.c | 5 +++--
reset.h | 4 ++--
sequencer.c | 8 ++++----
4 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index fa4f5d9306..22fbba3c62 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -592,7 +592,7 @@ static int finish_rebase(struct rebase_options *opts)
static int move_to_original_branch(struct rebase_options *opts)
{
struct strbuf branch_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
- struct reset_head_opts ropts = { 0 };
+ struct reset_working_tree_options ropts = { 0 };
int ret;
if (!opts->head_name)
@@ -610,7 +610,7 @@ static int move_to_original_branch(struct rebase_options *opts)
ropts.flags = RESET_HEAD_REFS_ONLY;
ropts.branch_msg = branch_reflog.buf;
ropts.head_msg = head_reflog.buf;
- ret = reset_head(the_repository, &ropts);
+ ret = reset_working_tree(the_repository, &ropts);
strbuf_release(&branch_reflog);
strbuf_release(&head_reflog);
@@ -685,7 +685,7 @@ static int run_am(struct rebase_options *opts)
status = run_command(&format_patch);
if (status) {
- struct reset_head_opts ropts = { 0 };
+ struct reset_working_tree_options ropts = { 0 };
unlink(rebased_patches);
free(rebased_patches);
child_process_clear(&am);
@@ -693,7 +693,7 @@ static int run_am(struct rebase_options *opts)
ropts.oid = &opts->orig_head->object.oid;
ropts.branch = opts->head_name;
ropts.default_reflog_action = opts->reflog_action;
- reset_head(the_repository, &ropts);
+ reset_working_tree(the_repository, &ropts);
error(_("\ngit encountered an error while preparing the "
"patches to replay\n"
"these revisions:\n"
@@ -855,7 +855,7 @@ static int rebase_config(const char *var, const char *value,
static int checkout_up_to_date(struct rebase_options *options)
{
struct strbuf buf = STRBUF_INIT;
- struct reset_head_opts ropts = { 0 };
+ struct reset_working_tree_options ropts = { 0 };
int ret = 0;
strbuf_addf(&buf, "%s: checkout %s",
@@ -866,7 +866,7 @@ static int checkout_up_to_date(struct rebase_options *options)
if (!ropts.branch)
ropts.flags |= RESET_HEAD_DETACH;
ropts.head_msg = buf.buf;
- if (reset_head(the_repository, &ropts) < 0)
+ if (reset_working_tree(the_repository, &ropts) < 0)
ret = error(_("could not switch to %s"), options->switch_to);
strbuf_release(&buf);
@@ -1116,7 +1116,7 @@ int cmd_rebase(int argc,
int reschedule_failed_exec = -1;
int allow_preemptive_ff = 1;
int preserve_merges_selected = 0;
- struct reset_head_opts ropts = { 0 };
+ struct reset_working_tree_options ropts = { 0 };
struct option builtin_rebase_options[] = {
OPT_STRING(0, "onto", &options.onto_name,
N_("revision"),
@@ -1385,7 +1385,7 @@ int cmd_rebase(int argc,
rerere_clear(the_repository, &merge_rr);
string_list_clear(&merge_rr, 1);
ropts.flags = RESET_HEAD_HARD;
- if (reset_head(the_repository, &ropts) < 0)
+ if (reset_working_tree(the_repository, &ropts) < 0)
die(_("could not discard worktree changes"));
remove_branch_state(the_repository, 0);
if (read_basic_state(&options))
@@ -1410,7 +1410,7 @@ int cmd_rebase(int argc,
ropts.head_msg = head_msg.buf;
ropts.branch = options.head_name;
ropts.flags = RESET_HEAD_HARD;
- if (reset_head(the_repository, &ropts) < 0)
+ if (reset_working_tree(the_repository, &ropts) < 0)
die(_("could not move back to %s"),
oid_to_hex(&options.orig_head->object.oid));
strbuf_release(&head_msg);
@@ -1880,7 +1880,7 @@ int cmd_rebase(int argc,
RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
ropts.head_msg = msg.buf;
ropts.default_reflog_action = options.reflog_action;
- if (reset_head(the_repository, &ropts)) {
+ if (reset_working_tree(the_repository, &ropts)) {
ret = error(_("Could not detach HEAD"));
goto cleanup_autostash;
}
diff --git a/reset.c b/reset.c
index 3b3cb74dab..799596398b 100644
--- a/reset.c
+++ b/reset.c
@@ -12,7 +12,7 @@
#include "hook.h"
static int update_refs(struct repository *repo,
- const struct reset_head_opts *opts,
+ const struct reset_working_tree_options *opts,
const struct object_id *oid,
const struct object_id *head)
{
@@ -85,7 +85,8 @@ static int update_refs(struct repository *repo,
return ret;
}
-int reset_head(struct repository *r, const struct reset_head_opts *opts)
+int reset_working_tree(struct repository *r,
+ const struct reset_working_tree_options *opts)
{
const struct object_id *oid = opts->oid;
const char *switch_to_branch = opts->branch;
diff --git a/reset.h b/reset.h
index a28f81829d..f130152014 100644
--- a/reset.h
+++ b/reset.h
@@ -17,7 +17,7 @@
/* Update ORIG_HEAD as well as HEAD */
#define RESET_ORIG_HEAD (1<<4)
-struct reset_head_opts {
+struct reset_working_tree_options {
/*
* The commit to checkout/reset to. Defaults to HEAD.
*/
@@ -55,6 +55,6 @@ struct reset_head_opts {
const char *default_reflog_action;
};
-int reset_head(struct repository *r, const struct reset_head_opts *opts);
+int reset_working_tree(struct repository *r, const struct reset_working_tree_options *opts);
#endif
diff --git a/sequencer.c b/sequencer.c
index 1ee4b2875b..d73ecf0384 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4677,7 +4677,7 @@ static void create_autostash_internal(struct repository *r,
if (has_unstaged_changes(r, 1) ||
has_uncommitted_changes(r, 1)) {
struct child_process stash = CHILD_PROCESS_INIT;
- struct reset_head_opts ropts = { .flags = RESET_HEAD_HARD };
+ struct reset_working_tree_options ropts = { .flags = RESET_HEAD_HARD };
struct object_id oid;
strvec_pushl(&stash.args,
@@ -4707,7 +4707,7 @@ static void create_autostash_internal(struct repository *r,
if (!silent)
printf(_("Created autostash: %s\n"), buf.buf);
- if (reset_head(r, &ropts) < 0)
+ if (reset_working_tree(r, &ropts) < 0)
die(_("could not reset --hard"));
discard_index(r->index);
if (repo_read_index(r) < 0)
@@ -4867,7 +4867,7 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,
const char *onto_name, const struct object_id *onto,
const struct object_id *orig_head)
{
- struct reset_head_opts ropts = {
+ struct reset_working_tree_options ropts = {
.oid = onto,
.orig_head = orig_head,
.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |
@@ -4876,7 +4876,7 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,
onto_name),
.default_reflog_action = sequencer_reflog_action(opts)
};
- if (reset_head(r, &ropts)) {
+ if (reset_working_tree(r, &ropts)) {
apply_autostash(rebase_path_autostash());
sequencer_remove_state(opts);
return error(_("could not detach HEAD"));
--
2.55.0.795.g602f6c329a.dirty
^ permalink raw reply related
* [PATCH v8 02/11] reset: drop `USE_THE_REPOSITORY_VARIABLE`
From: Patrick Steinhardt @ 2026-07-01 11:35 UTC (permalink / raw)
To: git
Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood,
Christian Couder
In-Reply-To: <20260701-b4-pks-history-drop-v8-0-19b5cdf1facd@pks.im>
In "reset.c" we still have references to `the_repository`, even though
the only entry point into the file already receives a repository as
parameter.
Update all uses of `the_repository` to instead use the passed-in repo
and drop `USE_THE_REPOSITORY_VARIABLE`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reset.c | 27 +++++++++++++--------------
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/reset.c b/reset.c
index 46e30e6394..3b3cb74dab 100644
--- a/reset.c
+++ b/reset.c
@@ -1,5 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
-
#include "git-compat-util.h"
#include "cache-tree.h"
#include "gettext.h"
@@ -13,7 +11,8 @@
#include "unpack-trees.h"
#include "hook.h"
-static int update_refs(const struct reset_head_opts *opts,
+static int update_refs(struct repository *repo,
+ const struct reset_head_opts *opts,
const struct object_id *oid,
const struct object_id *head)
{
@@ -42,19 +41,19 @@ static int update_refs(const struct reset_head_opts *opts,
prefix_len = msg.len;
if (update_orig_head) {
- if (!repo_get_oid(the_repository, "ORIG_HEAD", &oid_old_orig))
+ if (!repo_get_oid(repo, "ORIG_HEAD", &oid_old_orig))
old_orig = &oid_old_orig;
if (head) {
if (!reflog_orig_head) {
strbuf_addstr(&msg, "updating ORIG_HEAD");
reflog_orig_head = msg.buf;
}
- refs_update_ref(get_main_ref_store(the_repository),
+ refs_update_ref(get_main_ref_store(repo),
reflog_orig_head, "ORIG_HEAD",
orig_head ? orig_head : head,
old_orig, 0, UPDATE_REFS_MSG_ON_ERR);
} else if (old_orig)
- refs_delete_ref(get_main_ref_store(the_repository),
+ refs_delete_ref(get_main_ref_store(repo),
NULL, "ORIG_HEAD", old_orig, 0);
}
@@ -64,23 +63,23 @@ static int update_refs(const struct reset_head_opts *opts,
reflog_head = msg.buf;
}
if (!switch_to_branch)
- ret = refs_update_ref(get_main_ref_store(the_repository),
+ ret = refs_update_ref(get_main_ref_store(repo),
reflog_head, "HEAD", oid, head,
detach_head ? REF_NO_DEREF : 0,
UPDATE_REFS_MSG_ON_ERR);
else {
- ret = refs_update_ref(get_main_ref_store(the_repository),
+ ret = refs_update_ref(get_main_ref_store(repo),
reflog_branch ? reflog_branch : reflog_head,
switch_to_branch, oid, NULL, 0,
UPDATE_REFS_MSG_ON_ERR);
if (!ret)
- ret = refs_update_symref(get_main_ref_store(the_repository),
+ ret = refs_update_symref(get_main_ref_store(repo),
"HEAD", switch_to_branch,
reflog_head);
}
if (!ret && run_hook)
- run_hooks_l(the_repository, "post-checkout",
- oid_to_hex(head ? head : null_oid(the_hash_algo)),
+ run_hooks_l(repo, "post-checkout",
+ oid_to_hex(head ? head : null_oid(repo->hash_algo)),
oid_to_hex(oid), "1", NULL);
strbuf_release(&msg);
return ret;
@@ -126,7 +125,7 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)
oid = &head_oid;
if (refs_only)
- return update_refs(opts, oid, head);
+ return update_refs(r, opts, oid, head);
action = reset_hard ? "reset" : "checkout";
setup_unpack_trees_porcelain(&unpack_tree_opts, action);
@@ -163,7 +162,7 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)
goto leave_reset_head;
}
- tree = repo_parse_tree_indirect(the_repository, oid);
+ tree = repo_parse_tree_indirect(r, oid);
if (!tree) {
ret = error(_("unable to read tree (%s)"), oid_to_hex(oid));
goto leave_reset_head;
@@ -177,7 +176,7 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)
}
if (oid != &head_oid || update_orig_head || switch_to_branch)
- ret = update_refs(opts, oid, head);
+ ret = update_refs(r, opts, oid, head);
leave_reset_head:
rollback_lock_file(&lock);
--
2.55.0.795.g602f6c329a.dirty
^ permalink raw reply related
* [PATCH v8 00/11] builtin/history: introduce "drop" subcommand
From: Patrick Steinhardt @ 2026-07-01 11:35 UTC (permalink / raw)
To: git
Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood,
Christian Couder
In-Reply-To: <20260601-b4-pks-history-drop-v1-0-643e32340d55@pks.im>
Hi,
this small patch series introduces the new "drop" subcommand for
git-history(1). As a reader might guess, the command does exactly that:
given a commit, it will drop that commit from the commit history and
replay descendant branches on top of it.
Changes in v8:
- Pass `RESOLVE_REF_READING` to make `refs_resolve_ref_unsafe()`
return a NULL pointer when it cannot resolve the reference.
- Drop unneeded code that sets `head_target = "HEAD"` on detached
HEAD.
- Add a test case that verifies that we can drop commits with
"--update-refs=head" and a detached HEAD.
- Link to v7: https://patch.msgid.link/20260629-b4-pks-history-drop-v7-0-6e9392a957d8@pks.im
Changes in v7:
- Expose `replay_result_queue_update()` so that we don't have to
duplicate its functionality.
- Add missing SOB.
- Link to v6: https://patch.msgid.link/20260615-b4-pks-history-drop-v6-0-2e329e536d78@pks.im
Changes in v6:
- Fix bad interactions of DRY_RUN with UPDATE_HEAD
- Link to v5: https://patch.msgid.link/20260611-b4-pks-history-drop-v5-0-34d35725559c@pks.im
Changes in v5:
- Reject UPDATE_ORIG_HEAD without UPDATE_HEAD.
- Link to v4: https://patch.msgid.link/20260610-b4-pks-history-drop-v4-0-70d5f0ae8c25@pks.im
Changes in v4:
- Remove the `SKIP_REF_UPDATES` flag in favor of a new `UPDATE_HEAD`
flag, as suggested by Phillip.
- Rename `reset_head()` to `reset_working_tree()`. This better matches
the new scope of the function, and it helps us to catch any
in-flight patches that would now have to set the `UPDATE_HEAD` flag.
- Link to v3: https://patch.msgid.link/20260608-b4-pks-history-drop-v3-0-84ca8e43e937@pks.im
Changes in v3:
- Fix commit message typos.
- Make `update_orig_head` and `skip_ref_updates` mutually exclusive.
- Use fancy revisions to specify the commit to drop in the example
section.
- Detect conflicting changes in the index/working tree in dry-run
mode.
- Consistently use a subshell.
- Rename `RESET_HEAD_ORIG_HEAD` to `RESET_HEAD_UPDATE_ORIG_HEAD`.
- Link to v2: https://patch.msgid.link/20260603-b4-pks-history-drop-v2-0-742cb5b5176d@pks.im
Changes in v2:
- Reworked `update_worktree()` to use `reset_head()`, which required a
bunch of changes to `reset_head()`.
- Consistently mention the commit that cannot be dropped as part of
error messages.
- Adapt error message to not use backticks anymore.
- Drop redundant "--graph" flag in a test helper.
- Link to v1: https://patch.msgid.link/20260601-b4-pks-history-drop-v1-0-643e32340d55@pks.im
Thanks!
Patrick
---
Patrick Steinhardt (11):
read-cache: split out function to drop unmerged entries to stage 0
reset: drop `USE_THE_REPOSITORY_VARIABLE`
reset: rename `reset_head()`
reset: modernize flags passed to `reset_working_tree()`
reset: introduce dry-run mode
reset: introduce ability to skip updating HEAD
reset: allow the caller to specify the current HEAD object
reset: stop assuming that the caller passes in a clean index
replay: expose `replay_result_queue_update()`
builtin/history: split handling of ref updates into two phases
builtin/history: implement "drop" subcommand
Documentation/git-history.adoc | 38 ++-
builtin/history.c | 284 ++++++++++++++++++---
builtin/rebase.c | 41 +--
read-cache-ll.h | 1 +
read-cache.c | 12 +-
replay.c | 8 +-
replay.h | 5 +
reset.c | 102 +++++---
reset.h | 51 ++--
sequencer.c | 17 +-
t/meson.build | 1 +
t/t3454-history-drop.sh | 561 +++++++++++++++++++++++++++++++++++++++++
12 files changed, 1000 insertions(+), 121 deletions(-)
Range-diff versus v7:
1: 4b8702dbff = 1: 8145bb1408 read-cache: split out function to drop unmerged entries to stage 0
2: 1794f27cc4 = 2: 022d65d3db reset: drop `USE_THE_REPOSITORY_VARIABLE`
3: 2cb6c21f6a = 3: c37abd69d3 reset: rename `reset_head()`
4: 55fb296c39 = 4: 84f80ff33b reset: modernize flags passed to `reset_working_tree()`
5: b57d52b5ca = 5: 2465dcf8ec reset: introduce dry-run mode
6: a54dbccb17 = 6: 65b1853ce4 reset: introduce ability to skip updating HEAD
7: f22a1c9b70 = 7: acfc768574 reset: allow the caller to specify the current HEAD object
8: 5ec7867767 = 8: 8e2d3fa7b3 reset: stop assuming that the caller passes in a clean index
9: d8c9548408 = 9: 80015c6cec replay: expose `replay_result_queue_update()`
10: d01ce11892 = 10: b518dcd5ef builtin/history: split handling of ref updates into two phases
11: 1ee3600b98 ! 11: 963636e72c builtin/history: implement "drop" subcommand
@@ builtin/history.c: static int cmd_history_split(int argc,
+
+ *changed = false;
+
-+ head_target = refs_resolve_ref_unsafe(get_main_ref_store(repo),
-+ "HEAD", RESOLVE_REF_NO_RECURSE,
++ head_target = refs_resolve_ref_unsafe(get_main_ref_store(repo), "HEAD",
++ RESOLVE_REF_NO_RECURSE | RESOLVE_REF_READING,
+ NULL, &head_flags);
+ if (!head_target)
+ return error(_("cannot look up HEAD"));
-+ if (!(head_flags & REF_ISSYMREF))
-+ head_target = "HEAD";
+
+ for (size_t i = 0; i < result->updates_nr; i++) {
+ if (!strcmp(result->updates[i].refname, head_target)) {
@@ t/t3454-history-drop.sh (new)
+ )
+'
+
++test_expect_success '--update-refs=head can rewrite detached HEAD' '
++ test_when_finished "rm -rf repo" &&
++ git init repo --initial-branch=main &&
++ (
++ cd repo &&
++ test_commit first &&
++ test_commit second &&
++ test_commit third &&
++ git switch --detach HEAD &&
++
++ git history drop --update-refs=head second &&
++
++ expect_log HEAD <<-\EOF &&
++ third
++ first
++ EOF
++ expect_log main <<-\EOF
++ third
++ second
++ first
++ EOF
++ )
++'
++
+test_expect_success 'conflict with replayed commit aborts cleanly' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
---
base-commit: 1666c1265231b0bc5f613fbbf3f0a9896cdef76e
change-id: 20260601-b4-pks-history-drop-28f6c6399e7b
^ permalink raw reply
* [PATCH v8 01/11] read-cache: split out function to drop unmerged entries to stage 0
From: Patrick Steinhardt @ 2026-07-01 11:35 UTC (permalink / raw)
To: git
Cc: Pablo Sabater, Junio C Hamano, Kristoffer Haugsbakk, Phillip Wood,
Christian Couder
In-Reply-To: <20260701-b4-pks-history-drop-v8-0-19b5cdf1facd@pks.im>
In `repo_read_index_unmerged()` we read the index and then drop any
unmerged entries to stage 0. In a subsequent commit we'll want to
perform this operation on arbitrary indexes, not only the one of the
given repository.
Prepare for this by splitting out the functionality into a new function
that can act on an arbitrary index.
While at it, fix a signedness mismatch when iterating through the index
cache entries.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
read-cache-ll.h | 1 +
read-cache.c | 12 +++++++-----
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/read-cache-ll.h b/read-cache-ll.h
index 2c8b4b21b1..71b87615eb 100644
--- a/read-cache-ll.h
+++ b/read-cache-ll.h
@@ -309,6 +309,7 @@ int write_locked_index(struct index_state *, struct lock_file *lock, unsigned fl
void discard_index(struct index_state *);
void move_index_extensions(struct index_state *dst, struct index_state *src);
int unmerged_index(const struct index_state *);
+int index_state_unmerged_to_stage0(struct index_state *istate);
/**
* Returns 1 if istate differs from tree, 0 otherwise. If tree is NULL,
diff --git a/read-cache.c b/read-cache.c
index 21829102ae..799a5bc719 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -3403,13 +3403,15 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
*/
int repo_read_index_unmerged(struct repository *repo)
{
- struct index_state *istate;
- int i;
+ repo_read_index(repo);
+ return index_state_unmerged_to_stage0(repo->index);
+}
+
+int index_state_unmerged_to_stage0(struct index_state *istate)
+{
int unmerged = 0;
- repo_read_index(repo);
- istate = repo->index;
- for (i = 0; i < istate->cache_nr; i++) {
+ for (unsigned int i = 0; i < istate->cache_nr; i++) {
struct cache_entry *ce = istate->cache[i];
struct cache_entry *new_ce;
int len;
--
2.55.0.795.g602f6c329a.dirty
^ permalink raw reply related
* Re: [PATCH v5 0/4] history: add squash subcommand to fold a range
From: Phillip Wood @ 2026-07-01 10:31 UTC (permalink / raw)
To: Harald Nordgren, phillip.wood
Cc: Patrick Steinhardt, Harald Nordgren via GitGitGadget, git
In-Reply-To: <CAHwyqnXoqZYHodWXHtwnk0_PiZcCYSVL+WgL3h5nWiYx_cSZLw@mail.gmail.com>
Hi Harald
On 30/06/2026 19:38, Harald Nordgren wrote:
> 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?
The reason we're introducing the history command is to experiment with
providing a better user interface for rewriting history without being
bound by the limitations of "git rebase". So I think it would entirely
appropriate to try a different format for the squash message here. If it
turns out to be a success then we can see if we want to use it in "git
rebase" as well.
Thanks
Phillip
^ permalink raw reply
* Re: [PATCH v7 11/11] builtin/history: implement "drop" subcommand
From: Patrick Steinhardt @ 2026-07-01 10:21 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Pablo Sabater, Kristoffer Haugsbakk, Phillip Wood,
Christian Couder
In-Reply-To: <xmqqv7b1unxl.fsf@gitster.g>
On Mon, Jun 29, 2026 at 12:49:42PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > +static int find_head_tree_change(struct repository *repo,
> > + const struct replay_result *result,
> > + struct commit **old_head,
> > + struct commit **new_head,
> > + bool *changed)
> > +{
> > + const struct replay_ref_update *head_update = NULL;
> > + struct commit *old_head_commit, *new_head_commit;
> > + struct tree *old_head_tree, *new_head_tree;
> > + const char *head_target;
> > + int head_flags;
> > +
> > + *changed = false;
> > +
> > + head_target = refs_resolve_ref_unsafe(get_main_ref_store(repo),
> > + "HEAD", RESOLVE_REF_NO_RECURSE,
> > + NULL, &head_flags);
> > + if (!head_target)
> > + return error(_("cannot look up HEAD"));
>
> Here head_target would be something like "refs/heads/master", or
> whatever the "HEAD" happens to point at.
>
> > + if (!(head_flags & REF_ISSYMREF))
> > + head_target = "HEAD";
>
> But if it is not a symref, then it is a detached HEAD. We manually
> set it to "HEAD" again. We know head_target was not NULL, so what
> did we receive in it from refs_resolve_ref_unsafe() call, before we
> overwrite it here?
True, this is redundant indeed, as the returned name would already be
"HEAD" in that case.
[snip]
> > +static int cmd_history_drop(int argc,
> > + const char **argv,
> > + const char *prefix,
> > + struct repository *repo)
> > +{
> > +...
> > + struct option options[] = {
> > + OPT_CALLBACK_F(0, "update-refs", &action, "(branches|head)",
> > + N_("control which refs should be updated"),
> > + PARSE_OPT_NONEG, parse_ref_action),
> > ...
> > + OPT_END(),
> > + };
> > ...
> > + ret = compute_pending_ref_updates(&revs, action, original, rewritten,
> > + empty, &result);
>
> Here we call the function. When action is "--update-refs=head",
> doesn't this code in compute_pending_ref_updates() ...
>
> if (action == REF_ACTION_HEAD &&
> decoration->type != DECORATION_REF_HEAD)
> continue;
>
> ... skip any reference name (loaded by load_ref_decorations() lazily
> by calling get_name_decoration()) that is not "HEAD", which would
> mean we end up not finding any hits in the find_head_tree_change()
> function call we make later ...
>
> > + if (ret) {
> > + ret = error(_("failed replaying descendants"));
> > + goto out;
> > + }
> > + ...
> > + if (!is_bare_repository()) {
> > + ret = find_head_tree_change(repo, &result, &old_head,
> > + &new_head, &head_moves);
>
> ... here?
No, because `replay_revisions()` populates the result with the resolved
branch name, and we're also searching for the same branch name inside
`find_head_tree_change()`.
Patrick
^ permalink raw reply
* Re: weird quadratic reftable behavior, was: Re: [PATCH 3/3] t5551: pack refs after creating many tags
From: Patrick Steinhardt @ 2026-07-01 10:07 UTC (permalink / raw)
To: Kristofer Karlsson; +Cc: Jeff King, Michael Montalbo, git, Junio C Hamano
In-Reply-To: <CAL71e4PfXA-ixKR6r7fu_7_QmdzK+rTRs29mOsUYKaq+_a5q5w@mail.gmail.com>
On Wed, Jul 01, 2026 at 11:04:45AM +0200, Kristofer Karlsson wrote:
> On Wed, 1 Jul 2026 at 10:00, Jeff King <peff@peff.net> wrote:
>
> > Yeah, it is (mostly) the same problem. About half the time is spent in
> > refs_verify_refnames_available().
> >
> > The other half is in reftable_be_transaction_prepare(). Looks like it
> > makes individual calls to prepare_single_update(), which reads each ref.
> > And those reads are expensive because of all of the tombstones. It might
> > be possible to do an iterator merge or similar between the sorted list
> > of transaction refs and the reftable contents.
>
> Hi, sorry for jumping in -- I found this interesting and started
> poking at the code. I think both halves may share the same root
> cause.
>
> The merged iterator's suppress_deletions flag filters out tombstones
> internally, which means higher-level code with prefix or refname
> bounds never gets a chance to stop iteration early. By letting
> tombstones pass through and filtering them one layer up in the
> reftable backend, the existing bounds checks can kick in before
> we scan through all the tombstones.
Ah, interesting. This kind of hits int o the direction that I proposed
of stopping iteration once we hit the first reference that has a
lexicographically-larget refname. But I thought about having to solve it
generically in the iterator somehow, and I wasn't really looking forward
to that.
Your solutions works around that issue indeed. It of course doesn't
solve reference iteration, which would still be expensive. But it does
solve the issue where we just want to look up a single reference, which
is what Peff noticed to be slow.
> So instead of doing full scans inside merged_iter_next_void()
> we can just delegate to merged_iter_next_entry() and instead
> add a loop to reftable_be_reflog_exists() that skips
> tombstones (but is amortized O(1)).
>
> Now multiple call sites would need to add something like this
> to compensate for returning tombstones:
>
> if (reftable_log_record_is_deletion(&iter->log))
> continue;
>
> but it may be worth it if it reduces cost when there are many refs.
I think it certainly might be.
> The key spot is reftable_ref_iterator_advance(), where the deletion
> skip goes right after the existing prefix check -- so a tombstone
> past the prefix stops iteration immediately instead of being
> silently consumed. The same idea applies to reftable_backend_read_ref()
> and the log iteration paths.
Yup.
> I have a local branch with this attempted fix. Rerunning the
> benchmark:
>
> Before:
> nr=1000 0.306s
> nr=2000 0.945s
> nr=4000 3.816s
> nr=8000 14.93s
>
> After:
> nr=1000 0.020s
> nr=2000 0.044s
> nr=4000 0.071s
> nr=8000 0.145s
> nr=16000 0.258s
> nr=32000 0.591s
>
> I can send a proper patch if needed/wanted, but I might have missed
> something silly here.
Nice gains. I certainly think it would make sense to polish this a bit
and then cast it into a patch.
Patrick
^ permalink raw reply
* Re: [PATCH 00/11] sequencer: do not record dropped commits as rewritten
From: Uwe Kleine-König @ 2026-07-01 9:38 UTC (permalink / raw)
To: Phillip Wood; +Cc: git, Junio C Hamano, Phillip Wood
In-Reply-To: <cover.1782833268.git.phillip.wood@dunelm.org.uk>
[-- Attachment #1: Type: text/plain, Size: 2216 bytes --]
Hello Phillip,
thanks a lot for addressing this, very appreciated!
On Tue, Jun 30, 2026 at 04:28:50PM +0100, Phillip Wood wrote:
> 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.
With my very little knowledge about git internals, this looks
reasonable, and it behaves as I expect in my test case. I installed a
local
Tested-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> Base-Commit: 6c3d7b73556db708feb3b16232fab1efc4353428
BTW, b4 didn't pick this up, for me it says:
Base: not specified
(and I applied it on top of 2.55.0).
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: weird quadratic reftable behavior, was: Re: [PATCH 3/3] t5551: pack refs after creating many tags
From: Kristofer Karlsson @ 2026-07-01 9:04 UTC (permalink / raw)
To: Jeff King; +Cc: Patrick Steinhardt, Michael Montalbo, git, Junio C Hamano
In-Reply-To: <20260701080014.GA3748390@coredump.intra.peff.net>
On Wed, 1 Jul 2026 at 10:00, Jeff King <peff@peff.net> wrote:
> Yeah, it is (mostly) the same problem. About half the time is spent in
> refs_verify_refnames_available().
>
> The other half is in reftable_be_transaction_prepare(). Looks like it
> makes individual calls to prepare_single_update(), which reads each ref.
> And those reads are expensive because of all of the tombstones. It might
> be possible to do an iterator merge or similar between the sorted list
> of transaction refs and the reftable contents.
Hi, sorry for jumping in -- I found this interesting and started
poking at the code. I think both halves may share the same root
cause.
The merged iterator's suppress_deletions flag filters out tombstones
internally, which means higher-level code with prefix or refname
bounds never gets a chance to stop iteration early. By letting
tombstones pass through and filtering them one layer up in the
reftable backend, the existing bounds checks can kick in before
we scan through all the tombstones.
So instead of doing full scans inside merged_iter_next_void()
we can just delegate to merged_iter_next_entry() and instead
add a loop to reftable_be_reflog_exists() that skips
tombstones (but is amortized O(1)).
Now multiple call sites would need to add something like this
to compensate for returning tombstones:
if (reftable_log_record_is_deletion(&iter->log))
continue;
but it may be worth it if it reduces cost when there are many refs.
The key spot is reftable_ref_iterator_advance(), where the deletion
skip goes right after the existing prefix check -- so a tombstone
past the prefix stops iteration immediately instead of being
silently consumed. The same idea applies to reftable_backend_read_ref()
and the log iteration paths.
I have a local branch with this attempted fix. Rerunning the
benchmark:
Before:
nr=1000 0.306s
nr=2000 0.945s
nr=4000 3.816s
nr=8000 14.93s
After:
nr=1000 0.020s
nr=2000 0.044s
nr=4000 0.071s
nr=8000 0.145s
nr=16000 0.258s
nr=32000 0.591s
I can send a proper patch if needed/wanted, but I might have missed
something silly here.
Thanks,
Kristofer
^ permalink raw reply
* Re: [PATCH 2/2] format-patch: fix leak of rev_info in prepare_bases()
From: Patrick Steinhardt @ 2026-07-01 9:01 UTC (permalink / raw)
To: Jeff King; +Cc: git, Karthik Nayak
In-Reply-To: <20260701084733.GA814472@coredump.intra.peff.net>
On Wed, Jul 01, 2026 at 04:47:33AM -0400, Jeff King wrote:
> On Wed, Jul 01, 2026 at 10:42:38AM +0200, Patrick Steinhardt wrote:
>
> > > We already run a special leak job for linux-reftables. Why not turn that
> > > job into "leaks plus reftables plus test-vars"? The only downside would
> > > be potentially hiding leaks found by linux-reftables-leaks if the
> > > test-vars features force us into a difference code path. But looking at
> > > the list, it doesn't seem likely to me. None of them is particularly
> > > ref-related.
> > >
> > > In fact, I kind of wonder if we could fold linux-reftables into the
> > > test-vars job completely.
> >
> > linux-reftable or linux-reftable-leaks? I think it would certainly make
> > sense to drop one of these and merge it into linux-TEST-vars. The
> > linux-reftable job doesn't provide any benefit over its -leak variant,
> > so that would be the candidate I'd personally merge.
>
> Both. Fold linux-reftable into linux-TEST-vars, and then drop
> linux-reftable-leaks in favor of a new linux-TEST-vars-leaks.
Hm, okay. I guess that should be fine. Do we also want to do a similar
thing for macOS and create a macos-TEST-vars job that exercises all of
this?
Also, while at it... I really think that job name is just plain awful.
While at it, we might rename it to something more sensible like
"linux-changed-defaults".
Patrick
^ permalink raw reply
* Re: [PATCH v5 3/3] replay: offer an option to linearize the commit topology
From: Toon Claes @ 2026-07-01 8:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Elijah Newren, Johannes Schindelin
In-Reply-To: <xmqq5x358byf.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Toon Claes <toon@iotcl.com> writes:
>
>> Documentation/git-replay.adoc | 8 ++++-
>> builtin/replay.c | 6 +++-
>> replay.c | 50 ++++++++++++++++----------
>> replay.h | 5 +++
>> t/t3650-replay-basics.sh | 84 ++++++++++++++++++++++++++++++++++++++++++-
>> 5 files changed, 132 insertions(+), 21 deletions(-)
>
> "replay --linearize" behaves differently from the flattening rebase
> in a case where X and Y that forked from A are merged at Z, and we
> ask to flatten the history leading to Z, doesn't it?
>
> A----X
> \ \
> Y----Z (tip)
>
> A typical flattening rebase would rewrite X to X', Y to Y', while
> dropping Z, and would leave us a flattened history, like
>
> A---X'---Y' (updated tip, the order of X' and Y' may be swapped)
>
> I may be misreading the logic, but doesn't "replay --linearize"
> instead produce
>
> A----X' (dangling)
> \
> Y' (tip -- Z is dropped and gets mapped)
>
> and leave X' dangling (or Y'; the point is that only one of them
> will survive), never incorporating it in the resulting history?
You bring up a good point here, and it is very similar to what Dscho
brought up[1].
When I started working on v5, I realized multiple tips can be passed to
git-replay(1) and the code in v1-v4 would replay all commits into a
single linear history. I assumed that's not what we want.
In my mind, replaying unrelated histories with --onto and --linearize
should remain unrelated. Also assuming the --linearize option would only
linearize merge commits.
Looking at less obvious situations, like your example above, things
aren't really that simple. And I agree the new Y' tip is not correct and
X' shouldn't be dangling.
On the other hand though, if there was a branch pointing to X, we still
need a piece of history that has X', but doesn't have Y'. In your
flattened history X' isn't a descendant of Y', but the order may be
swapped and we would need to create something like:
A----X' (other tip)
\
Y'----X' (tip)
That's quite complex trying to achieve something like that in code. In
short, --linearize will change the topology of the (merge) commits, and
we have to do this in a predictable way. Thus I'm currently leaning
toward bringing v1-v4 behavior back and linearize all commits in to a
single line when using --linearize. Meaning:
B----C (other tip)
/
A----X
\
Y----Z (tip)
$ git replay --onto X --linearize Z C
Would result into:
A----X----Y----Z----B----C
^ (new other tip)
^ (new tip)
This might not always be the expected behavior, especially when
replaying multiple branches at once. But to those I would suggest: don't
replay multiple branches at once.
But then again: Given the above example, you want to replay Z and C on
top of eachother, but don't want to rewrite the "(other tip)"? They have
I think two options:
$ git replay --onto X --linearize --ref=tip Z C
By passing --ref we could tell git-replay(1) to only update that ref.
(sidenote: --ref currently cannot be combined with multiple revision
ranges, because that normally produces multiple tips and it would be
ambiguous which one --ref should point at. But --linearize collapses
everything into a single tip, so that ambiguity goes away; maybe we
should loosen the constraint in that case.)
Or:
$ git replay --onto X --linearize Z^{commit} C
By peeling Z to a commit, git-replay(1) doesn't see it as a ref to
update.
Anyhow, a lot to unpack and I'll try to do my best in the next version
to cover that in the commit message, docs and test cases.
[1]: <f8b520d1-edeb-9e45-c503-025c8b5833c3@gmx.de>
>> + if (commit->parents && commit->parents->next) {
>> + if (!opts->linearize)
>> + die(_("replaying merge commits is not supported yet!"));
>> + /*
>> + * Drop the merge commit: do not pick it, leave
>> + * `last_commit` unchanged, and fall through to the
>> + * rest of the loop. As a result:
>> + * - the merge commit is mapped to `last_commit` in
>> + * `replayed_commits`, this will become the parent for
>> + * the child commits.
>> + * - refs previously pointing to the merge commit are
>> + * rewritten to point to the previous non-merge commit.
>> + */
>> + } else {
>> + /*
>> + * pick_regular_commit() looks up the parent of `commit` in
>> + * `replayed_commits` to determine the ancestor to replay onto.
>> + * The `default_base` parameter is used when no ancestor is found,
>> + * which happens for the first commit in the revision range.
>> + * When reverting, commits are replayed in reverse order, so the
>> + * lookup never succeeds, and we need to pass `last_commit`.
>> + */
>> + struct commit *base = onto;
>> + if (mode == REPLAY_MODE_REVERT)
>> + base = last_commit;
>> +
>> + last_commit = pick_regular_commit(revs->repo, commit, base,
>> + replayed_commits,
>> + &merge_opt, &result,
>> + mode, opts->empty);
>> + }
>> +
>> if (!last_commit)
>> break;
>
> Immediately after this hunk beyond the post-context are these lines.
>
> /* Record commit -> last_commit mapping */
> put_mapped_commit(replayed_commits, commit, last_commit);
>
> Let's imagine X gets processed first. X (and other commits on its
> branch) gets replayed, last_commit is set to X' (which is the
> rewritten X). replayed_commits mapping holds X->X' mapping.
>
> Then let's imagine the history leading to Y is replayed next.
> last_commit becomes Y', and Y->Y' mapping is stored in
> replayed_commits.
>
> Finally, we see Z. We are going to _drop_ it. last_commit is left
> unchanged, pointing at Y'. Then last_commit (i.e., Y') is used as
> the merge commit Z maps to (i.e., correctly dropping Z).
>
> Any descendants of Z, if any, will be grafted as descendants of Y'.
> If X did not have any descendants other than Z in the rewritten part
> of the history, then X' (and commits leading to it) would be lost,
> no?
I appreciate you're breaking down the code here.
> This "loss of the other branch" may be an inherent characteristic of
> this feature (i.e., I do not think it is necessarily a bug, and it
> may even be that the "bug" is in the way I am reading the patch),
> but then I wonder if the user may want to have control over which
> side branch should survive, perhaps? It would probably need to be
> documented, and a test or two to cast this behaviour in stone.
>
>> diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
>> index 3353bc4a4d..34c038eab9 100755
>> --- a/t/t3650-replay-basics.sh
>> +++ b/t/t3650-replay-basics.sh
>> @@ -52,8 +52,12 @@ test_expect_success 'setup' '
>
> The pre-context here has
>
> git switch --detach topic4 &&
> test_commit N &&
> test_commit O &&
> git switch -c topic-with-merge topic4 &&
>
>> test_merge P O --no-ff &&
>> git switch main &&
>
> The above does prepare topic-with-merge branch, but ...
>
>> +test_expect_success 'replay to rebase merge commit with --linearize' '
>> + git replay --ref-action=print --linearize \
>> + --onto main I..topic-with-merge >result &&
>
> ... this does not really exersize linearizing replay in a typical
> mergy history. P merges O with --no-ff because otherwise there
> won't be a merge, since O is a descendant of the commit "test_merge
> P O" runs on (i.e., topic4 == topic-with-merge).
>
> topic4 --- N --- O
> \ \
> .-----------P
>
> So, as long as O is replayed later than the parent of N (which is
> true), O' will be the surviving tip (corresponds to Y' that the
> dropped Z was mapped to in the earlier example), and nothing gets
> orphaned, I think.
>
> Perhaps a test to try a real merge may look something like this.
>
> diff --git c/t/t3650-replay-basics.sh w/t/t3650-replay-basics.sh
> index 34c038eab9..bb737f729a 100755
> --- c/t/t3650-replay-basics.sh
> +++ w/t/t3650-replay-basics.sh
> @@ -647,4 +647,37 @@ test_expect_success 'replay with --linearize to rebase multiple divergent branch
> test_cmp expect actual
> '
>
> +test_expect_success 'replay with --linearize of a divergent merge drops one branch' '
> + git switch -c topic-divergent-base main &&
> + test_commit base &&
> + # Fork 1: base -> X
> + git switch -c topic-divergent-x &&
> + test_commit X &&
> + # Fork 2: base -> Y
> + git switch topic-divergent-base &&
> + git switch -c topic-divergent-y &&
> + test_commit Y &&
> + # Merge them at Z
> + git switch topic-divergent-x &&
> + test_merge Z topic-divergent-y --no-ff &&
> +
> + # History is now:
> + #
> + # X - Z (topic-divergent-x)
> + # / /
> + # base - Y
> + #
> +
> + git replay --ref-action=print --linearize \
> + --onto main topic-divergent-base..topic-divergent-x >result &&
> + test_line_count = 1 result &&
> + tip=$(cut -f 3 -d " " result) &&
> + # Get the commits replayed onto main
> + git log --format=%s main..$tip >actual &&
> + # We expect exactly one commit to be replayed (either X or Y)
> + # because the other one is left dangling due to the merge being dropped.
> + test_line_count = 1 actual &&
> + test_grep "^[XY]$" actual
> +'
> +
> test_done
Thank you for providing this test case.
--
Cheers,
Toon
^ permalink raw reply
* Re: [PATCH 2/2] format-patch: fix leak of rev_info in prepare_bases()
From: Jeff King @ 2026-07-01 8:47 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Karthik Nayak
In-Reply-To: <akTS_rPV7JaGHKRq@pks.im>
On Wed, Jul 01, 2026 at 10:42:38AM +0200, Patrick Steinhardt wrote:
> > We already run a special leak job for linux-reftables. Why not turn that
> > job into "leaks plus reftables plus test-vars"? The only downside would
> > be potentially hiding leaks found by linux-reftables-leaks if the
> > test-vars features force us into a difference code path. But looking at
> > the list, it doesn't seem likely to me. None of them is particularly
> > ref-related.
> >
> > In fact, I kind of wonder if we could fold linux-reftables into the
> > test-vars job completely.
>
> linux-reftable or linux-reftable-leaks? I think it would certainly make
> sense to drop one of these and merge it into linux-TEST-vars. The
> linux-reftable job doesn't provide any benefit over its -leak variant,
> so that would be the candidate I'd personally merge.
Both. Fold linux-reftable into linux-TEST-vars, and then drop
linux-reftable-leaks in favor of a new linux-TEST-vars-leaks.
-Peff
^ permalink raw reply
* Re: git-blame vs. abbrev
From: Laszlo Ersek @ 2026-07-01 8:45 UTC (permalink / raw)
To: René Scharfe, Junio C Hamano; +Cc: git
In-Reply-To: <f9761389-8c31-4928-babf-8218e9393376@web.de>
On 6/30/26 23:38, René Scharfe wrote:
> On 6/30/26 9:49 PM, Junio C Hamano wrote:
>> 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;
>>
>> 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.
> Here's a patch to reserve a column only if marks are actually shown.
>
> I strongly suspect that any line can only have a single mark, but I
> didn't bother checking and proving whether that's actually true; the
> new code should be able to handle multiple marks just fine.
>
> Misses tests.
>
> René
>
>
> ---
> builtin/blame.c | 61 ++++++++++++++++++++++++++++++++++---------------
> 1 file changed, 42 insertions(+), 19 deletions(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index ffbd3ce5c5..0e747a43f2 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -453,6 +453,39 @@ static void determine_line_heat(struct commit_info *ci, const char **dest_color)
> *dest_color = colorfield[i].col;
> }
>
> +static size_t handle_marks(const struct blame_entry *ent, int opt, bool print)
> +{
> + size_t len = 0;
> +
> + if ((ent->suspect->commit->object.flags & UNINTERESTING) &&
> + !blank_boundary && !(opt & OUTPUT_ANNOTATE_COMPAT)) {
> + if (print)
> + putchar('^');
> + len++;
> + }
> + if (mark_unblamable_lines && ent->unblamable) {
> + if (print)
> + putchar('*');
> + len++;
> + }
> + if (mark_ignored_lines && ent->ignored) {
> + if (print)
> + putchar('?');
> + len++;
> + }
> + return len;
> +}
> +
> +static size_t print_marks(const struct blame_entry *ent, int opt)
> +{
> + return handle_marks(ent, opt, true);
> +}
> +
> +static size_t count_marks(const struct blame_entry *ent, int opt)
> +{
> + return handle_marks(ent, opt, false);
> +}
> +
> static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent,
> int opt, struct blame_entry *prev_ent)
> {
> @@ -499,23 +532,10 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent,
> if (color)
> fputs(color, stdout);
>
> - if (suspect->commit->object.flags & UNINTERESTING) {
> - if (blank_boundary) {
> - memset(hex, ' ', strlen(hex));
> - } else if (!(opt & OUTPUT_ANNOTATE_COMPAT)) {
> - length--;
> - putchar('^');
> - }
> - }
> -
> - if (mark_unblamable_lines && ent->unblamable) {
> - length--;
> - putchar('*');
> - }
> - if (mark_ignored_lines && ent->ignored) {
> - length--;
> - putchar('?');
> - }
> + if ((suspect->commit->object.flags & UNINTERESTING) &&
> + blank_boundary)
> + memset(hex, ' ', strlen(hex));
> + length -= print_marks(ent, opt);
>
> printf("%.*s", (int)(length < GIT_MAX_HEXSZ ? length : GIT_MAX_HEXSZ), hex);
> if (opt & OUTPUT_ANNOTATE_COMPAT) {
> @@ -647,11 +667,15 @@ static void find_alignment(struct blame_scoreboard *sb, int *option)
> struct blame_entry *e;
> int compute_auto_abbrev = (abbrev < 0);
> int auto_abbrev = DEFAULT_ABBREV;
> + size_t max_marks_count = 0;
>
> for (e = sb->ent; e; e = e->next) {
> struct blame_origin *suspect = e->suspect;
> int num;
> + size_t marks_count = count_marks(e, *option);
>
> + if (max_marks_count < marks_count)
> + max_marks_count = marks_count;
> if (compute_auto_abbrev)
> auto_abbrev = update_auto_abbrev(auto_abbrev, suspect);
> if (strcmp(suspect->path, sb->path))
> @@ -685,8 +709,7 @@ static void find_alignment(struct blame_scoreboard *sb, int *option)
> max_score_digits = decimal_width(largest_score);
>
> if (compute_auto_abbrev)
> - /* one more abbrev length is needed for the boundary commit */
> - abbrev = auto_abbrev + 1;
> + abbrev = auto_abbrev + max_marks_count;
> }
>
> static void sanity_check_on_fail(struct blame_scoreboard *sb, int baa)
This way, I didn't even need to pass "-b". :)
I didn't try to test the patch exhaustively, but it definitely does what
I'm after.
Tested-by: Laszlo Ersek <laszlo.ersek@posteo.net>
Thank you!
Laszlo
^ permalink raw reply
* Re: git-blame vs. abbrev
From: Laszlo Ersek @ 2026-07-01 8:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqcxx7raan.fsf@gitster.g>
On 6/30/26 23:24, Junio C Hamano wrote:
> Laszlo Ersek <laszlo.ersek@posteo.net> writes:
>
>>> ... 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 ;-).
>>
>> That's it precisely! :) I do *not* notice that the hash from git-blame
>> has one more hexadecimal character than the central abbrev setting; so I
>> just go ahead and blindly cut n' paste it, using the mouse, from the
>> terminal, to the editor search box ...
>
> Which means that there does not need any configuration variable to
> trigger the new behaviour to protect your workflow from breaking, I
> think, and instead we can just go ahead with the "-b then no extra
> column for '^'" change, right? ;-)
Yes, certainly!
Thank you!
Laszlo
^ permalink raw reply
* Re: [PATCH 2/2] format-patch: fix leak of rev_info in prepare_bases()
From: Patrick Steinhardt @ 2026-07-01 8:42 UTC (permalink / raw)
To: Jeff King; +Cc: git, Karthik Nayak
In-Reply-To: <20260701081358.GB813310@coredump.intra.peff.net>
On Wed, Jul 01, 2026 at 04:13:58AM -0400, Jeff King wrote:
> On Tue, Jun 30, 2026 at 12:26:19PM +0200, Patrick Steinhardt wrote:
>
> > > make SANITIZE=leak
> > > cd t
> > > GIT_TEST_COMMIT_GRAPH=1 ./t4014-format-patch.sh
> > >
> > > which yields many entries like:
> > >
> > > ==git==3687620==ERROR: LeakSanitizer: detected memory leaks
> > > Direct leak of 200 byte(s) in 1 object(s) allocated from:
> > > #0 0x7f4ccba185cb in malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:74
> > > #1 0x55cd452cdd0b in do_xmalloc wrapper.c:55
> > > #2 0x55cd452cdd9d in xmalloc wrapper.c:76
> > > #3 0x55cd45255473 in init_topo_walk revision.c:3845
> > > #4 0x55cd45255bef in prepare_revision_walk revision.c:4017
> > > #5 0x55cd44ffec40 in prepare_bases builtin/log.c:1872
> > > #6 0x55cd450010ec in cmd_format_patch builtin/log.c:2439
> >
> > Interesting. Makes me wonder whether we should modify linux-TEST-vars to
> > also run with the leak checker enabled. Ideally we'd of course just do
> > this for all jobs, but the overhead is probably way too high... yes,
> > doing a simple benchmark shows a ~3x hit.
> >
> > So this is definitely nothing we want to do for all jobs. But for the
> > linux-TEST-vars job it might make sense, as it exercises a bunch of
> > non-default code paths.
>
> We already run a special leak job for linux-reftables. Why not turn that
> job into "leaks plus reftables plus test-vars"? The only downside would
> be potentially hiding leaks found by linux-reftables-leaks if the
> test-vars features force us into a difference code path. But looking at
> the list, it doesn't seem likely to me. None of them is particularly
> ref-related.
>
> In fact, I kind of wonder if we could fold linux-reftables into the
> test-vars job completely.
linux-reftable or linux-reftable-leaks? I think it would certainly make
sense to drop one of these and merge it into linux-TEST-vars. The
linux-reftable job doesn't provide any benefit over its -leak variant,
so that would be the candidate I'd personally merge.
> > One thing worth noting: there are still six test suites that are failing
> > with this patch: t0095, t3451, t3452, t3453, t4013 and t4211. The t345x
> > failures are because of the missing call to `repo_unuse_commit_buffer()`
> > in git-history(1), which we already noted elsewhere.
> >
> > All of the remaining leaks in t0095, t4013 and t4211 seem to be related
> > to bloom filters.
>
> I sent some patches to fix the bloom-filter cases.
I saw them already, thanks for your work here!
Patrick
^ permalink raw reply
* Re: [PATCH 2/3] revision: avoid leaking bloom keyvecs with multiple traversals
From: Patrick Steinhardt @ 2026-07-01 8:26 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20260701064052.GB2580331@coredump.intra.peff.net>
On Wed, Jul 01, 2026 at 02:40:52AM -0400, Jeff King wrote:
[snip]
> There's an alternative fix, which is that prepare_to_use_bloom_filter()
> could notice that we already have keyvec entries and just reuse them.
> But this is less safe; the keyvec depends on the pruning pathspec, and
> we don't know if that has changed.
Right. We could of course start to record the pruning pathspec so that
we're able to tell these cases apart, and if so we could reuse the bloom
keyvec entries safely. But as you mention...
> I think it would _probably_ work in practice, since any caller using a
> rev_info for multiple traversals is probably doing so with the same
> pathspec. But it would also create a very subtle bug if that assumption
> is violated. So we'll do the safer thing here, and generate fresh keyvec
> entries for each traversal. The efficiency difference is probably not
> noticeable, and this is what was happening already (we just weren't
> bothering to free the old ones!).
... we haven't been doing that beforehand, either, so it's fine to not
care about that for now and just plug the memory leak.
Patrick
^ permalink raw reply
* Re: [PATCH 2/2] format-patch: fix leak of rev_info in prepare_bases()
From: Jeff King @ 2026-07-01 8:13 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Karthik Nayak
In-Reply-To: <akOZy-BygZS8fqPM@pks.im>
On Tue, Jun 30, 2026 at 12:26:19PM +0200, Patrick Steinhardt wrote:
> > make SANITIZE=leak
> > cd t
> > GIT_TEST_COMMIT_GRAPH=1 ./t4014-format-patch.sh
> >
> > which yields many entries like:
> >
> > ==git==3687620==ERROR: LeakSanitizer: detected memory leaks
> > Direct leak of 200 byte(s) in 1 object(s) allocated from:
> > #0 0x7f4ccba185cb in malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:74
> > #1 0x55cd452cdd0b in do_xmalloc wrapper.c:55
> > #2 0x55cd452cdd9d in xmalloc wrapper.c:76
> > #3 0x55cd45255473 in init_topo_walk revision.c:3845
> > #4 0x55cd45255bef in prepare_revision_walk revision.c:4017
> > #5 0x55cd44ffec40 in prepare_bases builtin/log.c:1872
> > #6 0x55cd450010ec in cmd_format_patch builtin/log.c:2439
>
> Interesting. Makes me wonder whether we should modify linux-TEST-vars to
> also run with the leak checker enabled. Ideally we'd of course just do
> this for all jobs, but the overhead is probably way too high... yes,
> doing a simple benchmark shows a ~3x hit.
>
> So this is definitely nothing we want to do for all jobs. But for the
> linux-TEST-vars job it might make sense, as it exercises a bunch of
> non-default code paths.
We already run a special leak job for linux-reftables. Why not turn that
job into "leaks plus reftables plus test-vars"? The only downside would
be potentially hiding leaks found by linux-reftables-leaks if the
test-vars features force us into a difference code path. But looking at
the list, it doesn't seem likely to me. None of them is particularly
ref-related.
In fact, I kind of wonder if we could fold linux-reftables into the
test-vars job completely.
> One thing worth noting: there are still six test suites that are failing
> with this patch: t0095, t3451, t3452, t3453, t4013 and t4211. The t345x
> failures are because of the missing call to `repo_unuse_commit_buffer()`
> in git-history(1), which we already noted elsewhere.
>
> All of the remaining leaks in t0095, t4013 and t4211 seem to be related
> to bloom filters.
I sent some patches to fix the bloom-filter cases.
Building with OPENSSL_SHA1_UNSAFE turns up more. The core issue is that
recent versions of openssl require an allocation to open a sha1 context,
and we free it in git_hash_final(). So code paths that abort mid-hash
will leak the allocation, and we need a git_hash_discard().
It comes up mostly with csum-file.[ch], since that's where we use the
unsafe variant.
If you further build with OPENSSL_SHA1 (using it for _all_ hash
computations), there are a few more cases. It's hard to care too much
since that isn't a recommended build (and we've even discussed dropping
support for non-dc sha1 totally). But sha256 has the same issue, so
we'll want to fix it eventually (I didn't try leak-checking the
linux-sha256 build, but I expect it would complain a lot).
I have some patches but they need a bit of polish. In particular I think
we'll have to tweak the hash.h #define mess to expose a "discard"
primitive from each implementation (otherwise we have to finalize the
hash to discard, which is a little inefficient). I didn't quite have the
stomach for that tonight.
-Peff
^ 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