# [PATCH v10 1/8] builtin/replay: extract core logic to replay revisions 19 messages from 2026-01-12 to 2026-01-13. Participants: Patrick Steinhardt, Junio C Hamano, Elijah Newren. Thread: https://gitlist.dev/t/64783 ## Patrick Steinhardt, 2026-01-12 14:15 Subject: [PATCH v10 0/8] Introduce git-history(1) command for easy history editing Message-ID: <20260112-b4-pks-history-builtin-v10-0-e3c6aa5b4cec@pks.im> URL: https://gitlist.dev/e/20260112-b4-pks-history-builtin-v10-0-e3c6aa5b4cec%40pks.im In-Reply-To: <20250819-b4-pks-history-builtin-v1-0-9b77c32688fe@pks.im> ``` Hi, over recent months I've been playing around with Jujutsu quite frequently. While I still prefer using Git, there's been a couple features in it that I really like and that I'd like to have in Git, as well. A copule of these features relate to history editing. Most importantly, I really dig the following commands: - jj-abandon(1) to drop a specific commit from your history. - jj-absorb(1) to take some changes and automatically apply them to commits in your history that last modified the respective hunks. - jj-split(1) to split a commit into two. - jj-new(1) to insert a new commit after or before a specific other commit. Not all of these commands can be ported directly into Git. jj-new(1) for example doesn't really make a ton of sense for us, I'd claim. But some of these commands _do_ make sense. This patch series is a starting point for such a command. For now, it only implements rewording of commits. I'll continue introducing new commands once this patch series has been merged though. Changes in v10: - Split out changes that make `replay_revisions()` handle empty revision ranges properly. - Drop redundant `struct repository` parameter in `replay_revisions()`. - Move around `die_for_incompatible_opt2()`. - Fix up some of the headers in "replay.c". - Split out the changes that make `replay_revisions()` handle empty revision ranges properly. - Create a separate commit that fixes updating detached HEAD. - Strip "encoding" header as we're reencoding commit messages. - Merge the logic to update refs in in `REF_ACTION_HEAD` and `REF_ACTION_BRANCHES` modes. - Link to v9: https://lore.kernel.org/r/20260109-b4-pks-history-builtin-v9-0-8766101814c6@pks.im Changes in v9: - Rename `struct replay_ref_updates` to `struct replay_result` to make its semantics less focussed on ref updates, only. - Clarify and fix return codes of git-replay(1) so that we return 1 on conflict, 128 on an error and 0 on success. - The usual small improvements to commit messages. - Link to v8: https://lore.kernel.org/r/20260107-b4-pks-history-builtin-v8-0-18e9779e3a26@pks.im Changes in v8: - The series is now built on top of e0bfec3dfc (The 15th batch, 2026-01-06) with kh/replay-invalid-onto-advance at 56b77a687e (t3650: add more regression tests for failure conditions, 2026-01-05) merged into it. The latter is expected to hit 'next' soonish. - Another biggish revamp to build almost completely on top of the git-replay(1) logic. - We now advance all branches descending from the rewritten commit by default. - I've dropped the patches introducing `git history split` and will send this as a follow-up patch series once this once has been merged. This was done to focus attention on the underlying mechanics as much as possible (and to keep my own sanity with the frequent revamps). - Link to v7: https://lore.kernel.org/r/20251203-b4-pks-history-builtin-v7-0-9e9f849bfd0e@pks.im Changes in v7: - I've rebased the patch series on top of f0ef5b6d9b (The fifth batch, 2025-11-30) with pw/replay-exclude-gpgsig-fix at 9f3a115087 (replay: do not copy "gpgsign-sha256" header, 2025-11-26) merged into it to fix some conflicts. - I refactored a bunch of code to be shared between split and reword. - I am now using "--ancestry-path" and "--reverse" to enumerate commits. - `git history split` now asks for commit messages for both commits. - `git history split` no longer allows the user to edit hunks so that conflicts become impossible. This may be reintroduced at a later point in time. - A bunch of test improvements. - Several commit message and documentation improvements. - Link to v6: https://lore.kernel.org/r/20251027-b4-pks-history-builtin-v6-0-407dd3f57ad3@pks.im Changes in v6: - I've rebased the patch series again to pull in the latest updates from sa/replay-atomic-ref-updates and fix conflicts. It is now based on 4e98b730f1 (The twenty-fourth batch, 2025-10-24) with ab661bb1bb (replay: add replay.refAction config option, 2025-10-23) merged into it. - I've dropped the "-m" options for now, so commit messages are always asked for via the editor. These can be introduced in a subsequent patch series once discussion around them has settled. - We don't use the merge machinery anymore to pick the commits. - Drop the commit to parse commits in the replay machinery. It didn't seem to be necessary in v5 anymore, and now that we don't use the merge machinery at all we don't ever take that code path in the first place. - Link to v5: https://lore.kernel.org/r/20251021-b4-pks-history-builtin-v5-0-78d23f578fe6@pks.im Changes in v5: - I've changed the patch series to be based on top of 133d151831 (The twenty-first batch, 2025-10-20) with sa/replay-atomic-ref-updates at a1c22e627e (SQAUASH??? t0450 band-aid, 2025-10-14) merged into it. This is one the one hand to fix a conflict, but also to get some of the CI updates to make GitLab CI work again. - Some slight commit message improvements. - Deduplicate subcommand usage strings by using defines. - Fix the desendancy checks to properly verify that HEAD is a descendant of the commit to be rewritten. Also add some tests for this. - Fix the hint that mentions that lines starting with the comment character will be tripped after having written the commit message. - Move an include to the correct commit. - Link to v4: https://lore.kernel.org/r/20251001-b4-pks-history-builtin-v4-0-8e61ddb86317@pks.im Changes in v4: - I've rebuilt the patch series. It is now based on 821f583da6 (The thirteenth batcn, 2025-09-29) with sa/replay-atomic-ref-updates at 665c66a743 (replay: make atomic ref updates the default behavior, 2025-09-27) merged into it. This should fix all conflicts with seen. - I've reworked this patch series to use the same infra as git-replay(1), as discussed during the Contributor's Summit. - I've slimmed down the patch series to only tackle those commands that cannot result in a conflict to keep it simple. I also learned that Elijah has been working on a "git replay edit" command, so I dropped that command so that we can instead use his version. - During the Contributor's Summit we have agreed that for now, we won't care about hook execution just yet. This may be backfilled at a later point in time. - I dropped "commit.verbose" handling for now, as my understanding of it was wrong at first. This is something we should backfill. - Link to v3: https://lore.kernel.org/r/20250904-b4-pks-history-builtin-v3-0-509053514755@pks.im Changes in v3: - Add logic to drive the "post-rewrite" hook and add tests to verify that all hooks are executed as expected. - Deduplicate logic to turn a replay action into a todo command. - Move the addition of tests for the top-level git-history(1) command to the correct commit. - Some smaller commit message fixes. - Honor "commit.verbose". - Fix copy-paste error with an error message. - Link to v2: https://lore.kernel.org/r/20250824-b4-pks-history-builtin-v2-0-964ac12f65bd@pks.im Changes in v2: - Add a new "reword" subcommand. - List git-history(1) in "command-list.txt". - Add some missing error handling. - Simplify calling convention of `apply_commits()` to handle root commits internally instead of requiring every caller to do so. - Add tests to verify that git-history(1) refuses to work with changes in the worktree or index. - Mark git-history(1) as experimental. - Introduce commands to manage interrupted history edits. - A bunch of improvements to the manpage. - Link to v1: https://lore.kernel.org/r/20250819-b4-pks-history-builtin-v1-0-9b77c32688fe@pks.im Thanks! Patrick --- Patrick Steinhardt (8): builtin/replay: extract core logic to replay revisions builtin/replay: move core logic into "libgit.a" replay: small set of cleanups replay: support empty commit ranges replay: support updating detached HEAD wt-status: provide function to expose status for trees builtin: add new "history" command builtin/history: implement "reword" subcommand .gitignore | 1 + Documentation/git-history.adoc | 73 +++++++ Documentation/meson.build | 1 + Makefile | 2 + builtin.h | 1 + builtin/history.c | 427 +++++++++++++++++++++++++++++++++++++++++ builtin/replay.c | 382 ++++-------------------------------- command-list.txt | 1 + git.c | 1 + meson.build | 2 + replay.c | 372 +++++++++++++++++++++++++++++++++++ replay.h | 64 ++++++ t/meson.build | 2 + t/t3450-history.sh | 17 ++ t/t3451-history-reword.sh | 391 +++++++++++++++++++++++++++++++++++++ t/t3650-replay-basics.sh | 9 + wt-status.c | 24 +++ wt-status.h | 9 + 18 files changed, 1434 insertions(+), 345 deletions(-) Range-diff versus v9: 1: 0cae2364ee ! 1: 99b47e1af2 builtin/replay: extract core logic to replay revisions @@ Commit message Signed-off-by: Patrick Steinhardt ## builtin/replay.c ## -@@ builtin/replay.c: static enum ref_action_mode get_ref_action_mode(struct repository *repo, const c - return REF_ACTION_UPDATE; +@@ builtin/replay.c: static void set_up_replay_mode(struct repository *repo, + if (!rinfo.positive_refexprs) + die(_("need some commits to replay")); + +- die_for_incompatible_opt2(!!onto_name, "--onto", +- !!*advance_name, "--advance"); ++ if (!(!!onto_name ^ !!*advance_name)) ++ BUG("expected either onto_name or *advance_name in this function"); ++ + if (onto_name) { + *onto = peel_committish(repo, onto_name, "--onto"); + if (rinfo.positive_refexprs < +@@ builtin/replay.c: static struct commit *pick_regular_commit(struct repository *repo, + return create_commit(repo, result->tree, pickme, replayed_base); } +struct replay_revisions_options { @@ builtin/replay.c: static enum ref_action_mode get_ref_action_mode(struct reposit + result->updates_nr++; +} + -+static int replay_revisions(struct repository *repo, struct rev_info *revs, ++static int replay_revisions(struct rev_info *revs, + struct replay_revisions_options *opts, + struct replay_result *out) +{ @@ builtin/replay.c: static enum ref_action_mode get_ref_action_mode(struct reposit + int ret; + + advance = xstrdup_or_null(opts->advance); -+ set_up_replay_mode(repo, &revs->cmdline, opts->onto, &advance, ++ set_up_replay_mode(revs->repo, &revs->cmdline, opts->onto, &advance, + &onto, &update_refs); + + /* FIXME: Should allow replaying commits with the first as a root commit */ @@ builtin/replay.c: static enum ref_action_mode get_ref_action_mode(struct reposit + goto out; + } + -+ init_basic_merge_options(&merge_opt, repo); ++ init_basic_merge_options(&merge_opt, revs->repo); + memset(&result, 0, sizeof(result)); + merge_opt.show_rename_progress = 0; + last_commit = onto; @@ builtin/replay.c: static enum ref_action_mode get_ref_action_mode(struct reposit + if (commit->parents->next) + die(_("replaying merge commits is not supported yet!")); + -+ last_commit = pick_regular_commit(repo, commit, replayed_commits, ++ last_commit = pick_regular_commit(revs->repo, commit, replayed_commits, + onto, &merge_opt, &result); + if (!last_commit) + break; @@ builtin/replay.c: static enum ref_action_mode get_ref_action_mode(struct reposit + return ret; +} + - static int handle_ref_update(enum ref_action_mode mode, - struct ref_transaction *transaction, - const char *refname, + static enum ref_action_mode parse_ref_action_mode(const char *ref_action, const char *source) + { + if (!ref_action || !strcmp(ref_action, "update")) @@ builtin/replay.c: int cmd_replay(int argc, const char *prefix, struct repository *repo) @@ builtin/replay.c: int cmd_replay(int argc, - contained, "--contained"); + die_for_incompatible_opt2(!!opts.advance, "--advance", + opts.contained, "--contained"); ++ die_for_incompatible_opt2(!!opts.advance, "--advance", ++ !!opts.onto, "--onto"); /* Parse ref action mode from command line or config */ ref_mode = get_ref_action_mode(repo, ref_action); @@ builtin/replay.c: int cmd_replay(int argc, - &onto, &update_refs); - - /* FIXME: Should allow replaying commits with the first as a root commit */ -+ ret = replay_revisions(repo, &revs, &opts, &result); ++ ret = replay_revisions(&revs, &opts, &result); + if (ret) + goto cleanup; 2: c5fca546e5 ! 2: 0945b53ecb builtin/replay: move core logic into "libgit.a" @@ builtin/replay.c - if (!rinfo.positive_refexprs) - die(_("need some commits to replay")); - -- die_for_incompatible_opt2(!!onto_name, "--onto", -- !!*advance_name, "--advance"); +- if (!(!!onto_name ^ !!*advance_name)) +- BUG("expected either onto_name or *advance_name in this function"); +- - if (onto_name) { - *onto = peel_committish(repo, onto_name, "--onto"); - if (rinfo.positive_refexprs < @@ builtin/replay.c - return create_commit(repo, result->tree, pickme, replayed_base); -} - - static enum ref_action_mode parse_ref_action_mode(const char *ref_action, const char *source) - { - if (!ref_action || !strcmp(ref_action, "update")) -@@ builtin/replay.c: static enum ref_action_mode get_ref_action_mode(struct repository *repo, const c - return REF_ACTION_UPDATE; - } - -struct replay_revisions_options { - const char *advance; - const char *onto; @@ builtin/replay.c: static enum ref_action_mode get_ref_action_mode(struct reposit - result->updates_nr++; -} - --static int replay_revisions(struct repository *repo, struct rev_info *revs, +-static int replay_revisions(struct rev_info *revs, - struct replay_revisions_options *opts, - struct replay_result *out) -{ @@ builtin/replay.c: static enum ref_action_mode get_ref_action_mode(struct reposit - int ret; - - advance = xstrdup_or_null(opts->advance); -- set_up_replay_mode(repo, &revs->cmdline, opts->onto, &advance, +- set_up_replay_mode(revs->repo, &revs->cmdline, opts->onto, &advance, - &onto, &update_refs); - - /* FIXME: Should allow replaying commits with the first as a root commit */ @@ builtin/replay.c: static enum ref_action_mode get_ref_action_mode(struct reposit - goto out; - } - -- init_basic_merge_options(&merge_opt, repo); +- init_basic_merge_options(&merge_opt, revs->repo); - memset(&result, 0, sizeof(result)); - merge_opt.show_rename_progress = 0; - last_commit = onto; @@ builtin/replay.c: static enum ref_action_mode get_ref_action_mode(struct reposit - if (commit->parents->next) - die(_("replaying merge commits is not supported yet!")); - -- last_commit = pick_regular_commit(repo, commit, replayed_commits, +- last_commit = pick_regular_commit(revs->repo, commit, replayed_commits, - onto, &merge_opt, &result); - if (!last_commit) - break; @@ builtin/replay.c: static enum ref_action_mode get_ref_action_mode(struct reposit - return ret; -} - - static int handle_ref_update(enum ref_action_mode mode, - struct ref_transaction *transaction, - const char *refname, + static enum ref_action_mode parse_ref_action_mode(const char *ref_action, const char *source) + { + if (!ref_action || !strcmp(ref_action, "update")) ## meson.build ## @@ meson.build: libgit_sources = [ @@ replay.c (new) +#include "hex.h" +#include "merge-ort.h" +#include "object-name.h" -+#include "oidset.h" -+#include "parse-options.h" +#include "refs.h" +#include "replay.h" +#include "revision.h" ++#include "strmap.h" +#include "tree.h" + +static const char *short_commit_name(struct repository *repo, @@ replay.c (new) + if (!rinfo.positive_refexprs) + die(_("need some commits to replay")); + -+ die_for_incompatible_opt2(!!onto_name, "--onto", -+ !!*advance_name, "--advance"); ++ if (!(!!onto_name ^ !!*advance_name)) ++ BUG("expected either onto_name or *advance_name in this function"); ++ + if (onto_name) { + *onto = peel_committish(repo, onto_name, "--onto"); + if (rinfo.positive_refexprs < @@ replay.c (new) + result->updates_nr++; +} + -+int replay_revisions(struct repository *repo, struct rev_info *revs, ++int replay_revisions(struct rev_info *revs, + struct replay_revisions_options *opts, + struct replay_result *out) +{ @@ replay.c (new) + int ret; + + advance = xstrdup_or_null(opts->advance); -+ set_up_replay_mode(repo, &revs->cmdline, opts->onto, &advance, ++ set_up_replay_mode(revs->repo, &revs->cmdline, opts->onto, &advance, + &onto, &update_refs); + + /* FIXME: Should allow replaying commits with the first as a root commit */ @@ replay.c (new) + goto out; + } + -+ init_basic_merge_options(&merge_opt, repo); ++ init_basic_merge_options(&merge_opt, revs->repo); + memset(&result, 0, sizeof(result)); + merge_opt.show_rename_progress = 0; + last_commit = onto; @@ replay.c (new) + if (commit->parents->next) + die(_("replaying merge commits is not supported yet!")); + -+ last_commit = pick_regular_commit(repo, commit, replayed_commits, ++ last_commit = pick_regular_commit(revs->repo, commit, replayed_commits, + onto, &merge_opt, &result); + if (!last_commit) + break; @@ replay.h (new) + * + * Returns 0 on success, a negative error code otherwise. + */ -+int replay_revisions(struct repository *repo, struct rev_info *revs, ++int replay_revisions(struct rev_info *revs, + struct replay_revisions_options *opts, + struct replay_result *out); + 3: 1d55aab054 = 3: 85ebe5853f replay: small set of cleanups 4: 451717f951 < -: ---------- replay: yield the object ID of the final rewritten commit -: ---------- > 4: a24641dbfb replay: support empty commit ranges -: ---------- > 5: e0ab803a46 replay: support updating detached HEAD 5: 7cde288c8d = 6: abc33b25ca wt-status: provide function to expose status for trees 6: b315c04169 ! 7: 387d1ded10 builtin: add new "history" command @@ Documentation/git-history.adoc (new) +your commit history that is simpler to use compared to linkgit:git-rebase[1] in +general. + -+If you want to reapply a range of commits onto a different base, or interactive -+rebases if you want to edit a range of commits. ++Use linkgit:git-rebase[1] if you want to reapply a range of commits onto a ++different base, or interactive rebases if you want to edit a range of commits ++at once. + +LIMITATIONS +----------- @@ Documentation/git-history.adoc (new) +COMMANDS +-------- + -+Several commands are available to rewrite history in different ways: ++No commands are supported yet. + +GIT +--- 7: f71911586d ! 8: aeb895475b builtin/history: implement "reword" subcommand @@ Commit message rewritten. This especially helps with workflows that use stacked branches. + Helped-by: Elijah Newren Signed-off-by: Patrick Steinhardt ## Documentation/git-history.adoc ## @@ Documentation/git-history.adoc: git-history - EXPERIMENTAL: Rewrite history DESCRIPTION ----------- -@@ Documentation/git-history.adoc: Overall, linkgit:git-history[1] aims to provide a more opinionated way to modify - your commit history that is simpler to use compared to linkgit:git-rebase[1] in - general. - --If you want to reapply a range of commits onto a different base, or interactive --rebases if you want to edit a range of commits. -+Use linkgit:git-rebase[1] if you want to reapply a range of commits onto a -+different base, or interactive rebases if you want to edit a range of commits -+at once. - - LIMITATIONS - ----------- -@@ Documentation/git-history.adoc: COMMANDS - - Several commands are available to rewrite history in different ways: +@@ Documentation/git-history.adoc: first-class conflicts. + COMMANDS + -------- +-No commands are supported yet. ++The following commands are available to rewrite history in different ways: ++ +`reword `:: + Rewrite the commit message of the specified commit. All the other + details of this commit remain unchanged. This command will spawn an @@ Documentation/git-history.adoc: COMMANDS +`--ref-action=(branches|head|print)`:: + Control which references will be updated by the command, if any. With + `branches`, all local branches that point to commits which are -+ decendants of the original commit will be rewritten. With `head`, only ++ descendants of the original commit will be rewritten. With `head`, only + the current `HEAD` reference will be rewritten. With `print`, all + updates as they would be performed with `branches` are printed in a + format that can be consumed by linkgit:git-update-ref[1]. -+ + GIT --- - Part of the linkgit:git[1] suite ## builtin/history.c ## @@ @@ builtin/history.c + struct commit *original, + struct commit **out) +{ -+ const char *exclude_gpgsig[] = { "gpgsig", "gpgsig-sha256", NULL }; ++ const char *exclude_gpgsig[] = { ++ /* We reencode the message, so the encoding needs to be stripped. */ ++ "encoding", ++ /* We need to strip signatures as those will become invalid. */ ++ "gpgsig", ++ "gpgsig-sha256", ++ NULL, ++ }; + const char *original_message, *original_body, *ptr; + struct commit_extra_header *original_extra_headers = NULL; + struct strbuf commit_message = STRBUF_INIT; @@ builtin/history.c +{ + const struct name_decoration *decoration; + struct replay_revisions_options opts = { 0 }; -+ struct replay_result result = { -+ .final_oid = rewritten->object.oid, -+ }; ++ struct replay_result result = { 0 }; + struct ref_transaction *transaction = NULL; + struct strvec args = STRVEC_INIT; + struct strbuf err = STRBUF_INIT; + struct commit *head = NULL; + struct rev_info revs; + char hex[GIT_MAX_HEXSZ + 1]; ++ bool detached_head; ++ int head_flags = 0; + int ret; + ++ refs_read_ref_full(get_main_ref_store(repo), "HEAD", ++ RESOLVE_REF_NO_RECURSE, NULL, &head_flags); ++ detached_head = !(head_flags & REF_ISSYMREF); ++ + repo_init_revisions(repo, &revs, NULL); + strvec_push(&args, "ignored"); + strvec_push(&args, "--reverse"); @@ builtin/history.c + goto out; + } + -+ strvec_push(&args, oid_to_hex(&head->object.oid)); ++ strvec_push(&args, "HEAD"); + } else { + strvec_push(&args, "--branches"); ++ strvec_push(&args, "HEAD"); + } + + setup_revisions_from_strvec(&args, &revs, NULL); @@ builtin/history.c + + opts.onto = oid_to_hex_r(hex, &rewritten->object.oid); + -+ ret = replay_revisions(repo, &revs, &opts, &result); ++ ret = replay_revisions(&revs, &opts, &result); + if (ret) + goto out; + + switch (action) { -+ case REF_ACTION_DEFAULT: + case REF_ACTION_BRANCHES: ++ case REF_ACTION_HEAD: + transaction = ref_store_transaction_begin(get_main_ref_store(repo), 0, &err); + if (!transaction) { + ret = error(_("failed to begin ref transaction: %s"), err.buf); @@ builtin/history.c + decoration; + decoration = decoration->next) + { -+ if (decoration->type != DECORATION_REF_LOCAL) ++ if (decoration->type != DECORATION_REF_LOCAL && ++ decoration->type != DECORATION_REF_HEAD) ++ continue; ++ ++ if (action == REF_ACTION_HEAD && ++ decoration->type != DECORATION_REF_HEAD) ++ continue; ++ ++ /* ++ * We only need to update HEAD separately in case it's ++ * detached. If it's not we'd already update the branch ++ * it is pointing to. ++ */ ++ if (action == REF_ACTION_BRANCHES && ++ decoration->type == DECORATION_REF_HEAD && ++ !detached_head) + continue; + + ret = ref_transaction_update(transaction, @@ builtin/history.c + } + + break; -+ case REF_ACTION_HEAD: -+ ret = refs_update_ref(get_main_ref_store(repo), reflog_msg, "HEAD", -+ &result.final_oid, &head->object.oid, 0, -+ UPDATE_REFS_MSG_ON_ERR); -+ if (ret) -+ goto out; -+ break; + case REF_ACTION_PRINT: + for (size_t i = 0; i < result.updates_nr; i++) + printf("update %s %s %s\n", @@ builtin/history.c + } + 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]); @@ builtin/history.c + return fn(argc, argv, prefix, repo); } - ## replay.c ## -@@ replay.c: int replay_revisions(struct repository *repo, struct rev_info *revs, - struct commit *commit; - struct commit *onto = NULL; - struct merge_options merge_opt; -- struct merge_result result; -+ struct merge_result result = { -+ .clean = 1, -+ }; - char *advance; - int ret; - -@@ replay.c: int replay_revisions(struct repository *repo, struct rev_info *revs, - } - - init_basic_merge_options(&merge_opt, repo); -- memset(&result, 0, sizeof(result)); - merge_opt.show_rename_progress = 0; - last_commit = onto; - replayed_commits = kh_init_oid_map(); - ## t/meson.build ## @@ t/meson.build: integration_tests = [ 't3437-rebase-fixup-options.sh', @@ t/t3451-history-reword.sh (new) + ) +' + ++test_expect_success 'can reword commit in the middle even on detached head' ' ++ test_when_finished "rm -rf repo" && ++ git init repo && ++ ( ++ cd repo && ++ test_commit first && ++ test_commit second && ++ test_commit third_on_main && ++ git checkout --detach HEAD^ && ++ test_commit third_on_head && ++ ++ reword_with_message HEAD~ <<-EOF && ++ second reworded ++ EOF ++ ++ expect_graph HEAD --branches <<-\EOF ++ * third_on_head ++ | * third_on_main ++ |/ ++ * second reworded ++ * first ++ EOF ++ ) ++' ++ ++test_expect_success 'can reword the detached head' ' ++ test_when_finished "rm -rf repo" && ++ git init repo && ++ ( ++ cd repo && ++ test_commit first && ++ test_commit second && ++ git checkout --detach HEAD && ++ test_commit third && ++ ++ reword_with_message HEAD <<-EOF && ++ third reworded ++ EOF ++ ++ expect_log <<-\EOF ++ third reworded ++ second ++ first ++ EOF ++ ) ++' ++ +test_expect_success 'can reword root commit' ' + test_when_finished "rm -rf repo" && + git init repo && --- base-commit: 02bdc12118c43e0a65258eb742492965859bffb6 change-id: 20250819-b4-pks-history-builtin-83398f9a05f0 ``` ## Patrick Steinhardt, 2026-01-12 14:15 Subject: [PATCH v10 1/8] builtin/replay: extract core logic to replay revisions Message-ID: <20260112-b4-pks-history-builtin-v10-1-e3c6aa5b4cec@pks.im> URL: https://gitlist.dev/e/20260112-b4-pks-history-builtin-v10-1-e3c6aa5b4cec%40pks.im In-Reply-To: <20260112-b4-pks-history-builtin-v10-0-e3c6aa5b4cec@pks.im> ``` We're about to move the core logic used to replay revisions onto a new base into the "libgit.a" library. Prepare for this by pulling out the logic into a new function `replay_revisions()` that: 1. Takes a set of revisions to replay and some options that tell it how it ought to replay the revisions. 2. Replays the commits. 3. Records any reference updates that would be caused by replaying the commits in a structure that is owned by the caller. The logic itself will be moved into a separate file in the next commit. This change is not expected to cause user-visible change in behaviour. Signed-off-by: Patrick Steinhardt --- builtin/replay.c | 281 +++++++++++++++++++++++++++++++++---------------------- 1 file changed, 170 insertions(+), 111 deletions(-) diff --git a/builtin/replay.c b/builtin/replay.c index 1960bbbee8..df0dc04d02 100644 --- a/builtin/replay.c +++ b/builtin/replay.c @@ -177,8 +177,9 @@ static void set_up_replay_mode(struct repository *repo, if (!rinfo.positive_refexprs) die(_("need some commits to replay")); - die_for_incompatible_opt2(!!onto_name, "--onto", - !!*advance_name, "--advance"); + if (!(!!onto_name ^ !!*advance_name)) + BUG("expected either onto_name or *advance_name in this function"); + if (onto_name) { *onto = peel_committish(repo, onto_name, "--onto"); if (rinfo.positive_refexprs < @@ -253,6 +254,137 @@ static struct commit *pick_regular_commit(struct repository *repo, return create_commit(repo, result->tree, pickme, replayed_base); } +struct replay_revisions_options { + const char *advance; + const char *onto; + int contained; +}; + +struct replay_result { + struct replay_ref_update { + char *refname; + struct object_id old_oid; + struct object_id new_oid; + } *updates; + size_t updates_nr, updates_alloc; + + bool merge_conflict; +}; + +static void replay_result_release(struct replay_result *result) +{ + for (size_t i = 0; i < result->updates_nr; i++) + free(result->updates[i].refname); + 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) +{ + ALLOC_GROW(result->updates, result->updates_nr + 1, result->updates_alloc); + result->updates[result->updates_nr].refname = xstrdup(refname); + result->updates[result->updates_nr].old_oid = *old_oid; + result->updates[result->updates_nr].new_oid = *new_oid; + result->updates_nr++; +} + +static int replay_revisions(struct rev_info *revs, + struct replay_revisions_options *opts, + struct replay_result *out) +{ + kh_oid_map_t *replayed_commits = NULL; + struct strset *update_refs = NULL; + struct commit *last_commit = NULL; + struct commit *commit; + struct commit *onto = NULL; + struct merge_options merge_opt; + struct merge_result result; + char *advance; + int ret; + + advance = xstrdup_or_null(opts->advance); + set_up_replay_mode(revs->repo, &revs->cmdline, opts->onto, &advance, + &onto, &update_refs); + + /* FIXME: Should allow replaying commits with the first as a root commit */ + + if (prepare_revision_walk(revs) < 0) { + ret = error(_("error preparing revisions")); + goto out; + } + + init_basic_merge_options(&merge_opt, revs->repo); + memset(&result, 0, sizeof(result)); + merge_opt.show_rename_progress = 0; + last_commit = onto; + replayed_commits = kh_init_oid_map(); + while ((commit = get_revision(revs))) { + const struct name_decoration *decoration; + khint_t pos; + int hr; + + if (!commit->parents) + die(_("replaying down from root commit is not supported yet!")); + if (commit->parents->next) + die(_("replaying merge commits is not supported yet!")); + + last_commit = pick_regular_commit(revs->repo, commit, replayed_commits, + onto, &merge_opt, &result); + if (!last_commit) + break; + + /* Record commit -> last_commit mapping */ + pos = kh_put_oid_map(replayed_commits, commit->object.oid, &hr); + if (hr == 0) + BUG("Duplicate rewritten commit: %s\n", + oid_to_hex(&commit->object.oid)); + kh_value(replayed_commits, pos) = last_commit; + + /* Update any necessary branches */ + if (advance) + continue; + decoration = get_name_decoration(&commit->object); + if (!decoration) + continue; + while (decoration) { + if (decoration->type == DECORATION_REF_LOCAL && + (opts->contained || strset_contains(update_refs, + decoration->name))) { + replay_result_queue_update(out, decoration->name, + &commit->object.oid, + &last_commit->object.oid); + } + decoration = decoration->next; + } + } + + if (!result.clean) { + out->merge_conflict = true; + ret = -1; + goto out; + } + + /* In --advance mode, advance the target ref */ + if (advance) + replay_result_queue_update(out, advance, + &onto->object.oid, + &last_commit->object.oid); + + ret = 0; + +out: + if (update_refs) { + strset_clear(update_refs); + free(update_refs); + } + kh_destroy_oid_map(replayed_commits); + merge_finalize(&merge_opt, &result); + free(advance); + return ret; +} + static enum ref_action_mode parse_ref_action_mode(const char *ref_action, const char *source) { if (!ref_action || !strcmp(ref_action, "update")) @@ -306,21 +438,11 @@ int cmd_replay(int argc, const char *prefix, struct repository *repo) { - const char *advance_name_opt = NULL; - char *advance_name = NULL; - struct commit *onto = NULL; - const char *onto_name = NULL; - int contained = 0; + struct replay_revisions_options opts = { 0 }; + struct replay_result result = { 0 }; const char *ref_action = NULL; enum ref_action_mode ref_mode; - struct rev_info revs; - struct commit *last_commit = NULL; - struct commit *commit; - struct merge_options merge_opt; - struct merge_result result; - struct strset *update_refs = NULL; - kh_oid_map_t *replayed_commits; struct ref_transaction *transaction = NULL; struct strbuf transaction_err = STRBUF_INIT; struct strbuf reflog_msg = STRBUF_INIT; @@ -333,13 +455,13 @@ int cmd_replay(int argc, NULL }; struct option replay_options[] = { - OPT_STRING(0, "advance", &advance_name_opt, + OPT_STRING(0, "advance", &opts.advance, N_("branch"), N_("make replay advance given branch")), - OPT_STRING(0, "onto", &onto_name, + OPT_STRING(0, "onto", &opts.onto, N_("revision"), N_("replay onto given commit")), - OPT_BOOL(0, "contained", &contained, + OPT_BOOL(0, "contained", &opts.contained, N_("update all branches that point at commits in ")), OPT_STRING(0, "ref-action", &ref_action, N_("mode"), @@ -350,19 +472,19 @@ int cmd_replay(int argc, argc = parse_options(argc, argv, prefix, replay_options, replay_usage, PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN_OPT); - if (!onto_name && !advance_name_opt) { + if (!opts.onto && !opts.advance) { error(_("option --onto or --advance is mandatory")); usage_with_options(replay_usage, replay_options); } - die_for_incompatible_opt2(!!advance_name_opt, "--advance", - contained, "--contained"); + die_for_incompatible_opt2(!!opts.advance, "--advance", + opts.contained, "--contained"); + die_for_incompatible_opt2(!!opts.advance, "--advance", + !!opts.onto, "--onto"); /* Parse ref action mode from command line or config */ ref_mode = get_ref_action_mode(repo, ref_action); - advance_name = xstrdup_or_null(advance_name_opt); - repo_init_revisions(repo, &revs, prefix); /* @@ -414,18 +536,19 @@ int cmd_replay(int argc, revs.simplify_history = 0; } - set_up_replay_mode(repo, &revs.cmdline, - onto_name, &advance_name, - &onto, &update_refs); - - /* FIXME: Should allow replaying commits with the first as a root commit */ + ret = replay_revisions(&revs, &opts, &result); + if (ret) + goto cleanup; /* Build reflog message */ - if (advance_name_opt) - strbuf_addf(&reflog_msg, "replay --advance %s", advance_name_opt); - else - strbuf_addf(&reflog_msg, "replay --onto %s", - oid_to_hex(&onto->object.oid)); + if (opts.advance) { + strbuf_addf(&reflog_msg, "replay --advance %s", opts.advance); + } else { + struct object_id oid; + if (repo_get_oid_committish(repo, opts.onto, &oid)) + BUG("--onto commit should have been resolved beforehand already"); + strbuf_addf(&reflog_msg, "replay --onto %s", oid_to_hex(&oid)); + } /* Initialize ref transaction if using update mode */ if (ref_mode == REF_ACTION_UPDATE) { @@ -438,78 +561,19 @@ int cmd_replay(int argc, } } - if (prepare_revision_walk(&revs) < 0) { - ret = error(_("error preparing revisions")); - goto cleanup; - } - - init_basic_merge_options(&merge_opt, repo); - memset(&result, 0, sizeof(result)); - merge_opt.show_rename_progress = 0; - last_commit = onto; - replayed_commits = kh_init_oid_map(); - while ((commit = get_revision(&revs))) { - const struct name_decoration *decoration; - khint_t pos; - int hr; - - if (!commit->parents) - die(_("replaying down from root commit is not supported yet!")); - if (commit->parents->next) - die(_("replaying merge commits is not supported yet!")); - - last_commit = pick_regular_commit(repo, commit, replayed_commits, - onto, &merge_opt, &result); - if (!last_commit) - break; - - /* Record commit -> last_commit mapping */ - pos = kh_put_oid_map(replayed_commits, commit->object.oid, &hr); - if (hr == 0) - BUG("Duplicate rewritten commit: %s\n", - oid_to_hex(&commit->object.oid)); - kh_value(replayed_commits, pos) = last_commit; - - /* Update any necessary branches */ - if (advance_name) - continue; - decoration = get_name_decoration(&commit->object); - if (!decoration) - continue; - while (decoration) { - if (decoration->type == DECORATION_REF_LOCAL && - (contained || strset_contains(update_refs, - decoration->name))) { - if (handle_ref_update(ref_mode, transaction, - decoration->name, - &last_commit->object.oid, - &commit->object.oid, - reflog_msg.buf, - &transaction_err) < 0) { - ret = error(_("failed to update ref '%s': %s"), - decoration->name, transaction_err.buf); - goto cleanup; - } - } - decoration = decoration->next; - } - } - - /* In --advance mode, advance the target ref */ - if (result.clean == 1 && advance_name) { - if (handle_ref_update(ref_mode, transaction, advance_name, - &last_commit->object.oid, - &onto->object.oid, - reflog_msg.buf, - &transaction_err) < 0) { + for (size_t i = 0; i < result.updates_nr; i++) { + ret = handle_ref_update(ref_mode, transaction, result.updates[i].refname, + &result.updates[i].new_oid, &result.updates[i].old_oid, + reflog_msg.buf, &transaction_err); + if (ret) { ret = error(_("failed to update ref '%s': %s"), - advance_name, transaction_err.buf); + result.updates[i].refname, transaction_err.buf); goto cleanup; } } /* Commit the ref transaction if we have one */ - if (transaction && result.clean == 1) { + if (transaction) { if (ref_transaction_commit(transaction, &transaction_err)) { ret = error(_("failed to commit ref transaction: %s"), transaction_err.buf); @@ -517,24 +581,19 @@ int cmd_replay(int argc, } } - merge_finalize(&merge_opt, &result); - kh_destroy_oid_map(replayed_commits); - if (update_refs) { - strset_clear(update_refs); - free(update_refs); - } - ret = result.clean; - cleanup: if (transaction) ref_transaction_free(transaction); + replay_result_release(&result); strbuf_release(&transaction_err); strbuf_release(&reflog_msg); release_revisions(&revs); - free(advance_name); - /* Return */ - if (ret < 0) - exit(128); - return ret ? 0 : 1; + if (ret) { + if (result.merge_conflict) + return 1; + return 128; + } + + return 0; } -- 2.52.0.590.g1f87b77810.dirty ``` ## Patrick Steinhardt, 2026-01-12 14:15 Subject: [PATCH v10 2/8] builtin/replay: move core logic into "libgit.a" Message-ID: <20260112-b4-pks-history-builtin-v10-2-e3c6aa5b4cec@pks.im> URL: https://gitlist.dev/e/20260112-b4-pks-history-builtin-v10-2-e3c6aa5b4cec%40pks.im In-Reply-To: <20260112-b4-pks-history-builtin-v10-0-e3c6aa5b4cec@pks.im> ``` Move the core logic used to replay commits into "libgit.a" so that it can be easily reused by other commands. It will be used in a subsequent commit where we're about to introduce a new git-history(1) command. Note that with this change we have no sign-comparison warnings anymore, and neither do we depend on `the_repository`. Signed-off-by: Patrick Steinhardt --- Makefile | 1 + builtin/replay.c | 369 +------------------------------------------------------ meson.build | 1 + replay.c | 355 ++++++++++++++++++++++++++++++++++++++++++++++++++++ replay.h | 64 ++++++++++ 5 files changed, 422 insertions(+), 368 deletions(-) diff --git a/Makefile b/Makefile index b7eba509c6..1c64a5d2ae 100644 --- a/Makefile +++ b/Makefile @@ -1285,6 +1285,7 @@ LIB_OBJS += repack-geometry.o LIB_OBJS += repack-midx.o LIB_OBJS += repack-promisor.o LIB_OBJS += replace-object.o +LIB_OBJS += replay.o LIB_OBJS += repo-settings.o LIB_OBJS += repository.o LIB_OBJS += rerere.o diff --git a/builtin/replay.c b/builtin/replay.c index df0dc04d02..e27889d536 100644 --- a/builtin/replay.c +++ b/builtin/replay.c @@ -2,389 +2,22 @@ * "git replay" builtin command */ -#define USE_THE_REPOSITORY_VARIABLE -#define DISABLE_SIGN_COMPARE_WARNINGS - #include "git-compat-util.h" #include "builtin.h" #include "config.h" -#include "environment.h" #include "hex.h" -#include "lockfile.h" -#include "merge-ort.h" #include "object-name.h" #include "parse-options.h" #include "refs.h" +#include "replay.h" #include "revision.h" -#include "strmap.h" -#include -#include enum ref_action_mode { REF_ACTION_UPDATE, REF_ACTION_PRINT, }; -static const char *short_commit_name(struct repository *repo, - struct commit *commit) -{ - return repo_find_unique_abbrev(repo, &commit->object.oid, - DEFAULT_ABBREV); -} - -static struct commit *peel_committish(struct repository *repo, - const char *name, - const char *mode) -{ - struct object *obj; - struct object_id oid; - - if (repo_get_oid(repo, name, &oid)) - die(_("'%s' is not a valid commit-ish for %s"), name, mode); - obj = parse_object_or_die(repo, &oid, name); - return (struct commit *)repo_peel_to_type(repo, name, 0, obj, - OBJ_COMMIT); -} - -static char *get_author(const char *message) -{ - size_t len; - const char *a; - - a = find_commit_header(message, "author", &len); - if (a) - return xmemdupz(a, len); - - return NULL; -} - -static struct commit *create_commit(struct repository *repo, - struct tree *tree, - struct commit *based_on, - struct commit *parent) -{ - struct object_id ret; - struct object *obj = NULL; - struct commit_list *parents = NULL; - char *author; - char *sign_commit = NULL; /* FIXME: cli users might want to sign again */ - struct commit_extra_header *extra = NULL; - struct strbuf msg = STRBUF_INIT; - const char *out_enc = get_commit_output_encoding(); - const char *message = repo_logmsg_reencode(repo, based_on, - NULL, out_enc); - const char *orig_message = NULL; - const char *exclude_gpgsig[] = { "gpgsig", "gpgsig-sha256", NULL }; - - commit_list_insert(parent, &parents); - extra = read_commit_extra_headers(based_on, exclude_gpgsig); - find_commit_subject(message, &orig_message); - strbuf_addstr(&msg, orig_message); - author = get_author(message); - reset_ident_date(); - if (commit_tree_extended(msg.buf, msg.len, &tree->object.oid, parents, - &ret, author, NULL, sign_commit, extra)) { - error(_("failed to write commit object")); - goto out; - } - - obj = parse_object(repo, &ret); - -out: - repo_unuse_commit_buffer(the_repository, based_on, message); - free_commit_extra_headers(extra); - free_commit_list(parents); - strbuf_release(&msg); - free(author); - return (struct commit *)obj; -} - -struct ref_info { - struct commit *onto; - struct strset positive_refs; - struct strset negative_refs; - int positive_refexprs; - int negative_refexprs; -}; - -static void get_ref_information(struct repository *repo, - struct rev_cmdline_info *cmd_info, - struct ref_info *ref_info) -{ - int i; - - ref_info->onto = NULL; - strset_init(&ref_info->positive_refs); - strset_init(&ref_info->negative_refs); - ref_info->positive_refexprs = 0; - ref_info->negative_refexprs = 0; - - /* - * When the user specifies e.g. - * git replay origin/main..mybranch - * git replay ^origin/next mybranch1 mybranch2 - * we want to be able to determine where to replay the commits. In - * these examples, the branches are probably based on an old version - * of either origin/main or origin/next, so we want to replay on the - * newest version of that branch. In contrast we would want to error - * out if they ran - * git replay ^origin/master ^origin/next mybranch - * git replay mybranch~2..mybranch - * the first of those because there's no unique base to choose, and - * the second because they'd likely just be replaying commits on top - * of the same commit and not making any difference. - */ - for (i = 0; i < cmd_info->nr; i++) { - struct rev_cmdline_entry *e = cmd_info->rev + i; - struct object_id oid; - const char *refexpr = e->name; - char *fullname = NULL; - int can_uniquely_dwim = 1; - - if (*refexpr == '^') - refexpr++; - if (repo_dwim_ref(repo, refexpr, strlen(refexpr), &oid, &fullname, 0) != 1) - can_uniquely_dwim = 0; - - if (e->flags & BOTTOM) { - if (can_uniquely_dwim) - strset_add(&ref_info->negative_refs, fullname); - if (!ref_info->negative_refexprs) - ref_info->onto = lookup_commit_reference_gently(repo, - &e->item->oid, 1); - ref_info->negative_refexprs++; - } else { - if (can_uniquely_dwim) - strset_add(&ref_info->positive_refs, fullname); - ref_info->positive_refexprs++; - } - - free(fullname); - } -} - -static void set_up_replay_mode(struct repository *repo, - struct rev_cmdline_info *cmd_info, - const char *onto_name, - char **advance_name, - struct commit **onto, - struct strset **update_refs) -{ - struct ref_info rinfo; - - get_ref_information(repo, cmd_info, &rinfo); - if (!rinfo.positive_refexprs) - die(_("need some commits to replay")); - - if (!(!!onto_name ^ !!*advance_name)) - BUG("expected either onto_name or *advance_name in this function"); - - if (onto_name) { - *onto = peel_committish(repo, onto_name, "--onto"); - if (rinfo.positive_refexprs < - strset_get_size(&rinfo.positive_refs)) - die(_("all positive revisions given must be references")); - *update_refs = xcalloc(1, sizeof(**update_refs)); - **update_refs = rinfo.positive_refs; - memset(&rinfo.positive_refs, 0, sizeof(**update_refs)); - } else { - struct object_id oid; - char *fullname = NULL; - - if (!*advance_name) - BUG("expected either onto_name or *advance_name in this function"); - - if (repo_dwim_ref(repo, *advance_name, strlen(*advance_name), - &oid, &fullname, 0) == 1) { - free(*advance_name); - *advance_name = fullname; - } else { - die(_("argument to --advance must be a reference")); - } - *onto = peel_committish(repo, *advance_name, "--advance"); - if (rinfo.positive_refexprs > 1) - die(_("cannot advance target with multiple sources because ordering would be ill-defined")); - } - strset_clear(&rinfo.negative_refs); - strset_clear(&rinfo.positive_refs); -} - -static struct commit *mapped_commit(kh_oid_map_t *replayed_commits, - struct commit *commit, - struct commit *fallback) -{ - khint_t pos = kh_get_oid_map(replayed_commits, commit->object.oid); - if (pos == kh_end(replayed_commits)) - return fallback; - return kh_value(replayed_commits, pos); -} - -static struct commit *pick_regular_commit(struct repository *repo, - struct commit *pickme, - kh_oid_map_t *replayed_commits, - struct commit *onto, - struct merge_options *merge_opt, - struct merge_result *result) -{ - struct commit *base, *replayed_base; - struct tree *pickme_tree, *base_tree; - - base = pickme->parents->item; - replayed_base = mapped_commit(replayed_commits, base, onto); - - result->tree = repo_get_commit_tree(repo, replayed_base); - pickme_tree = repo_get_commit_tree(repo, pickme); - base_tree = repo_get_commit_tree(repo, base); - - merge_opt->branch1 = short_commit_name(repo, replayed_base); - merge_opt->branch2 = short_commit_name(repo, pickme); - merge_opt->ancestor = xstrfmt("parent of %s", merge_opt->branch2); - - merge_incore_nonrecursive(merge_opt, - base_tree, - result->tree, - pickme_tree, - result); - - free((char*)merge_opt->ancestor); - merge_opt->ancestor = NULL; - if (!result->clean) - return NULL; - return create_commit(repo, result->tree, pickme, replayed_base); -} - -struct replay_revisions_options { - const char *advance; - const char *onto; - int contained; -}; - -struct replay_result { - struct replay_ref_update { - char *refname; - struct object_id old_oid; - struct object_id new_oid; - } *updates; - size_t updates_nr, updates_alloc; - - bool merge_conflict; -}; - -static void replay_result_release(struct replay_result *result) -{ - for (size_t i = 0; i < result->updates_nr; i++) - free(result->updates[i].refname); - 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) -{ - ALLOC_GROW(result->updates, result->updates_nr + 1, result->updates_alloc); - result->updates[result->updates_nr].refname = xstrdup(refname); - result->updates[result->updates_nr].old_oid = *old_oid; - result->updates[result->updates_nr].new_oid = *new_oid; - result->updates_nr++; -} - -static int replay_revisions(struct rev_info *revs, - struct replay_revisions_options *opts, - struct replay_result *out) -{ - kh_oid_map_t *replayed_commits = NULL; - struct strset *update_refs = NULL; - struct commit *last_commit = NULL; - struct commit *commit; - struct commit *onto = NULL; - struct merge_options merge_opt; - struct merge_result result; - char *advance; - int ret; - - advance = xstrdup_or_null(opts->advance); - set_up_replay_mode(revs->repo, &revs->cmdline, opts->onto, &advance, - &onto, &update_refs); - - /* FIXME: Should allow replaying commits with the first as a root commit */ - - if (prepare_revision_walk(revs) < 0) { - ret = error(_("error preparing revisions")); - goto out; - } - - init_basic_merge_options(&merge_opt, revs->repo); - memset(&result, 0, sizeof(result)); - merge_opt.show_rename_progress = 0; - last_commit = onto; - replayed_commits = kh_init_oid_map(); - while ((commit = get_revision(revs))) { - const struct name_decoration *decoration; - khint_t pos; - int hr; - - if (!commit->parents) - die(_("replaying down from root commit is not supported yet!")); - if (commit->parents->next) - die(_("replaying merge commits is not supported yet!")); - - last_commit = pick_regular_commit(revs->repo, commit, replayed_commits, - onto, &merge_opt, &result); - if (!last_commit) - break; - - /* Record commit -> last_commit mapping */ - pos = kh_put_oid_map(replayed_commits, commit->object.oid, &hr); - if (hr == 0) - BUG("Duplicate rewritten commit: %s\n", - oid_to_hex(&commit->object.oid)); - kh_value(replayed_commits, pos) = last_commit; - - /* Update any necessary branches */ - if (advance) - continue; - decoration = get_name_decoration(&commit->object); - if (!decoration) - continue; - while (decoration) { - if (decoration->type == DECORATION_REF_LOCAL && - (opts->contained || strset_contains(update_refs, - decoration->name))) { - replay_result_queue_update(out, decoration->name, - &commit->object.oid, - &last_commit->object.oid); - } - decoration = decoration->next; - } - } - - if (!result.clean) { - out->merge_conflict = true; - ret = -1; - goto out; - } - - /* In --advance mode, advance the target ref */ - if (advance) - replay_result_queue_update(out, advance, - &onto->object.oid, - &last_commit->object.oid); - - ret = 0; - -out: - if (update_refs) { - strset_clear(update_refs); - free(update_refs); - } - kh_destroy_oid_map(replayed_commits); - merge_finalize(&merge_opt, &result); - free(advance); - return ret; -} - static enum ref_action_mode parse_ref_action_mode(const char *ref_action, const char *source) { if (!ref_action || !strcmp(ref_action, "update")) diff --git a/meson.build b/meson.build index dd52efd1c8..a5a4e99b25 100644 --- a/meson.build +++ b/meson.build @@ -471,6 +471,7 @@ libgit_sources = [ 'repack-midx.c', 'repack-promisor.c', 'replace-object.c', + 'replay.c', 'repo-settings.c', 'repository.c', 'rerere.c', diff --git a/replay.c b/replay.c new file mode 100644 index 0000000000..f7c61dd83f --- /dev/null +++ b/replay.c @@ -0,0 +1,355 @@ +#define USE_THE_REPOSITORY_VARIABLE +#define DISABLE_SIGN_COMPARE_WARNINGS + +#include "git-compat-util.h" +#include "environment.h" +#include "hex.h" +#include "merge-ort.h" +#include "object-name.h" +#include "refs.h" +#include "replay.h" +#include "revision.h" +#include "strmap.h" +#include "tree.h" + +static const char *short_commit_name(struct repository *repo, + struct commit *commit) +{ + return repo_find_unique_abbrev(repo, &commit->object.oid, + DEFAULT_ABBREV); +} + +static struct commit *peel_committish(struct repository *repo, + const char *name, + const char *mode) +{ + struct object *obj; + struct object_id oid; + + if (repo_get_oid(repo, name, &oid)) + die(_("'%s' is not a valid commit-ish for %s"), name, mode); + obj = parse_object_or_die(repo, &oid, name); + return (struct commit *)repo_peel_to_type(repo, name, 0, obj, + OBJ_COMMIT); +} + +static char *get_author(const char *message) +{ + size_t len; + const char *a; + + a = find_commit_header(message, "author", &len); + if (a) + return xmemdupz(a, len); + + return NULL; +} + +static struct commit *create_commit(struct repository *repo, + struct tree *tree, + struct commit *based_on, + struct commit *parent) +{ + struct object_id ret; + struct object *obj = NULL; + struct commit_list *parents = NULL; + char *author; + char *sign_commit = NULL; /* FIXME: cli users might want to sign again */ + struct commit_extra_header *extra = NULL; + struct strbuf msg = STRBUF_INIT; + const char *out_enc = get_commit_output_encoding(); + const char *message = repo_logmsg_reencode(repo, based_on, + NULL, out_enc); + const char *orig_message = NULL; + const char *exclude_gpgsig[] = { "gpgsig", "gpgsig-sha256", NULL }; + + commit_list_insert(parent, &parents); + extra = read_commit_extra_headers(based_on, exclude_gpgsig); + find_commit_subject(message, &orig_message); + strbuf_addstr(&msg, orig_message); + author = get_author(message); + reset_ident_date(); + if (commit_tree_extended(msg.buf, msg.len, &tree->object.oid, parents, + &ret, author, NULL, sign_commit, extra)) { + error(_("failed to write commit object")); + goto out; + } + + obj = parse_object(repo, &ret); + +out: + repo_unuse_commit_buffer(the_repository, based_on, message); + free_commit_extra_headers(extra); + free_commit_list(parents); + strbuf_release(&msg); + free(author); + return (struct commit *)obj; +} + +struct ref_info { + struct commit *onto; + struct strset positive_refs; + struct strset negative_refs; + int positive_refexprs; + int negative_refexprs; +}; + +static void get_ref_information(struct repository *repo, + struct rev_cmdline_info *cmd_info, + struct ref_info *ref_info) +{ + int i; + + ref_info->onto = NULL; + strset_init(&ref_info->positive_refs); + strset_init(&ref_info->negative_refs); + ref_info->positive_refexprs = 0; + ref_info->negative_refexprs = 0; + + /* + * When the user specifies e.g. + * git replay origin/main..mybranch + * git replay ^origin/next mybranch1 mybranch2 + * we want to be able to determine where to replay the commits. In + * these examples, the branches are probably based on an old version + * of either origin/main or origin/next, so we want to replay on the + * newest version of that branch. In contrast we would want to error + * out if they ran + * git replay ^origin/master ^origin/next mybranch + * git replay mybranch~2..mybranch + * the first of those because there's no unique base to choose, and + * the second because they'd likely just be replaying commits on top + * of the same commit and not making any difference. + */ + for (i = 0; i < cmd_info->nr; i++) { + struct rev_cmdline_entry *e = cmd_info->rev + i; + struct object_id oid; + const char *refexpr = e->name; + char *fullname = NULL; + int can_uniquely_dwim = 1; + + if (*refexpr == '^') + refexpr++; + if (repo_dwim_ref(repo, refexpr, strlen(refexpr), &oid, &fullname, 0) != 1) + can_uniquely_dwim = 0; + + if (e->flags & BOTTOM) { + if (can_uniquely_dwim) + strset_add(&ref_info->negative_refs, fullname); + if (!ref_info->negative_refexprs) + ref_info->onto = lookup_commit_reference_gently(repo, + &e->item->oid, 1); + ref_info->negative_refexprs++; + } else { + if (can_uniquely_dwim) + strset_add(&ref_info->positive_refs, fullname); + ref_info->positive_refexprs++; + } + + free(fullname); + } +} + +static void set_up_replay_mode(struct repository *repo, + struct rev_cmdline_info *cmd_info, + const char *onto_name, + char **advance_name, + struct commit **onto, + struct strset **update_refs) +{ + struct ref_info rinfo; + + get_ref_information(repo, cmd_info, &rinfo); + if (!rinfo.positive_refexprs) + die(_("need some commits to replay")); + + if (!(!!onto_name ^ !!*advance_name)) + BUG("expected either onto_name or *advance_name in this function"); + + if (onto_name) { + *onto = peel_committish(repo, onto_name, "--onto"); + if (rinfo.positive_refexprs < + strset_get_size(&rinfo.positive_refs)) + die(_("all positive revisions given must be references")); + *update_refs = xcalloc(1, sizeof(**update_refs)); + **update_refs = rinfo.positive_refs; + memset(&rinfo.positive_refs, 0, sizeof(**update_refs)); + } else { + struct object_id oid; + char *fullname = NULL; + + if (!*advance_name) + BUG("expected either onto_name or *advance_name in this function"); + + if (repo_dwim_ref(repo, *advance_name, strlen(*advance_name), + &oid, &fullname, 0) == 1) { + free(*advance_name); + *advance_name = fullname; + } else { + die(_("argument to --advance must be a reference")); + } + *onto = peel_committish(repo, *advance_name, "--advance"); + if (rinfo.positive_refexprs > 1) + die(_("cannot advance target with multiple sources because ordering would be ill-defined")); + } + strset_clear(&rinfo.negative_refs); + strset_clear(&rinfo.positive_refs); +} + +static struct commit *mapped_commit(kh_oid_map_t *replayed_commits, + struct commit *commit, + struct commit *fallback) +{ + khint_t pos = kh_get_oid_map(replayed_commits, commit->object.oid); + if (pos == kh_end(replayed_commits)) + return fallback; + return kh_value(replayed_commits, pos); +} + +static struct commit *pick_regular_commit(struct repository *repo, + struct commit *pickme, + kh_oid_map_t *replayed_commits, + struct commit *onto, + struct merge_options *merge_opt, + struct merge_result *result) +{ + struct commit *base, *replayed_base; + struct tree *pickme_tree, *base_tree; + + base = pickme->parents->item; + replayed_base = mapped_commit(replayed_commits, base, onto); + + result->tree = repo_get_commit_tree(repo, replayed_base); + pickme_tree = repo_get_commit_tree(repo, pickme); + base_tree = repo_get_commit_tree(repo, base); + + merge_opt->branch1 = short_commit_name(repo, replayed_base); + merge_opt->branch2 = short_commit_name(repo, pickme); + merge_opt->ancestor = xstrfmt("parent of %s", merge_opt->branch2); + + merge_incore_nonrecursive(merge_opt, + base_tree, + result->tree, + pickme_tree, + result); + + free((char*)merge_opt->ancestor); + merge_opt->ancestor = NULL; + if (!result->clean) + return NULL; + return create_commit(repo, result->tree, pickme, replayed_base); +} + +void replay_result_release(struct replay_result *result) +{ + for (size_t i = 0; i < result->updates_nr; i++) + free(result->updates[i].refname); + 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) +{ + ALLOC_GROW(result->updates, result->updates_nr + 1, result->updates_alloc); + result->updates[result->updates_nr].refname = xstrdup(refname); + result->updates[result->updates_nr].old_oid = *old_oid; + result->updates[result->updates_nr].new_oid = *new_oid; + result->updates_nr++; +} + +int replay_revisions(struct rev_info *revs, + struct replay_revisions_options *opts, + struct replay_result *out) +{ + kh_oid_map_t *replayed_commits = NULL; + struct strset *update_refs = NULL; + struct commit *last_commit = NULL; + struct commit *commit; + struct commit *onto = NULL; + struct merge_options merge_opt; + struct merge_result result; + char *advance; + int ret; + + advance = xstrdup_or_null(opts->advance); + set_up_replay_mode(revs->repo, &revs->cmdline, opts->onto, &advance, + &onto, &update_refs); + + /* FIXME: Should allow replaying commits with the first as a root commit */ + + if (prepare_revision_walk(revs) < 0) { + ret = error(_("error preparing revisions")); + goto out; + } + + init_basic_merge_options(&merge_opt, revs->repo); + memset(&result, 0, sizeof(result)); + merge_opt.show_rename_progress = 0; + last_commit = onto; + replayed_commits = kh_init_oid_map(); + while ((commit = get_revision(revs))) { + const struct name_decoration *decoration; + khint_t pos; + int hr; + + if (!commit->parents) + die(_("replaying down from root commit is not supported yet!")); + if (commit->parents->next) + die(_("replaying merge commits is not supported yet!")); + + last_commit = pick_regular_commit(revs->repo, commit, replayed_commits, + onto, &merge_opt, &result); + if (!last_commit) + break; + + /* Record commit -> last_commit mapping */ + pos = kh_put_oid_map(replayed_commits, commit->object.oid, &hr); + if (hr == 0) + BUG("Duplicate rewritten commit: %s\n", + oid_to_hex(&commit->object.oid)); + kh_value(replayed_commits, pos) = last_commit; + + /* Update any necessary branches */ + if (advance) + continue; + decoration = get_name_decoration(&commit->object); + if (!decoration) + continue; + while (decoration) { + if (decoration->type == DECORATION_REF_LOCAL && + (opts->contained || strset_contains(update_refs, + decoration->name))) { + replay_result_queue_update(out, decoration->name, + &commit->object.oid, + &last_commit->object.oid); + } + decoration = decoration->next; + } + } + + if (!result.clean) { + out->merge_conflict = true; + ret = -1; + goto out; + } + + /* In --advance mode, advance the target ref */ + if (advance) + replay_result_queue_update(out, advance, + &onto->object.oid, + &last_commit->object.oid); + + ret = 0; + +out: + if (update_refs) { + strset_clear(update_refs); + free(update_refs); + } + kh_destroy_oid_map(replayed_commits); + merge_finalize(&merge_opt, &result); + free(advance); + return ret; +} diff --git a/replay.h b/replay.h new file mode 100644 index 0000000000..09e0a4e640 --- /dev/null +++ b/replay.h @@ -0,0 +1,64 @@ +#ifndef REPLAY_H +#define REPLAY_H + +#include "hash.h" + +struct repository; +struct rev_info; + +/* + * A set of options that can be passed to `replay_revisions()`. + */ +struct replay_revisions_options { + /* + * Starting point at which to create the new commits; must be a branch + * name. The branch will be updated to point to the rewritten commits. + * This option is mutually exclusive with `onto`. + */ + const char *advance; + + /* + * Starting point at which to create the new commits; must be a + * committish. References pointing at decendants of `onto` will be + * updated to point to the new commits. + */ + const char *onto; + + /* + * Update branches that point at commits in the given revision range. + * Requires `onto` to be set. + */ + int contained; +}; + +/* This struct is used as an out-parameter by `replay_revisions()`. */ +struct replay_result { + /* + * The set of reference updates that are caused by replaying the + * commits. + */ + struct replay_ref_update { + char *refname; + struct object_id old_oid; + struct object_id new_oid; + } *updates; + size_t updates_nr, updates_alloc; + + /* Set to true in case the replay failed with a merge conflict. */ + bool merge_conflict; +}; + +void replay_result_release(struct replay_result *result); + +/* + * 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 + * be recorded in the `updates` out pointer. + * + * Returns 0 on success, a negative error code otherwise. + */ +int replay_revisions(struct rev_info *revs, + struct replay_revisions_options *opts, + struct replay_result *out); + +#endif -- 2.52.0.590.g1f87b77810.dirty ``` ## Patrick Steinhardt, 2026-01-12 14:15 Subject: [PATCH v10 3/8] replay: small set of cleanups Message-ID: <20260112-b4-pks-history-builtin-v10-3-e3c6aa5b4cec@pks.im> URL: https://gitlist.dev/e/20260112-b4-pks-history-builtin-v10-3-e3c6aa5b4cec%40pks.im In-Reply-To: <20260112-b4-pks-history-builtin-v10-0-e3c6aa5b4cec@pks.im> ``` Perform a small set of cleanups so that the "replay" logic compiles with "-Wsign-compare" and doesn't use `the_repository` anymore. Note that there are still some implicit dependencies on `the_repository`, e.g. because we use `get_commit_output_encoding()`. Signed-off-by: Patrick Steinhardt --- replay.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/replay.c b/replay.c index f7c61dd83f..1e660171d2 100644 --- a/replay.c +++ b/replay.c @@ -1,5 +1,4 @@ #define USE_THE_REPOSITORY_VARIABLE -#define DISABLE_SIGN_COMPARE_WARNINGS #include "git-compat-util.h" #include "environment.h" @@ -78,7 +77,7 @@ static struct commit *create_commit(struct repository *repo, obj = parse_object(repo, &ret); out: - repo_unuse_commit_buffer(the_repository, based_on, message); + repo_unuse_commit_buffer(repo, based_on, message); free_commit_extra_headers(extra); free_commit_list(parents); strbuf_release(&msg); @@ -90,16 +89,14 @@ struct ref_info { struct commit *onto; struct strset positive_refs; struct strset negative_refs; - int positive_refexprs; - int negative_refexprs; + size_t positive_refexprs; + size_t negative_refexprs; }; static void get_ref_information(struct repository *repo, struct rev_cmdline_info *cmd_info, struct ref_info *ref_info) { - int i; - ref_info->onto = NULL; strset_init(&ref_info->positive_refs); strset_init(&ref_info->negative_refs); @@ -121,7 +118,7 @@ static void get_ref_information(struct repository *repo, * the second because they'd likely just be replaying commits on top * of the same commit and not making any difference. */ - for (i = 0; i < cmd_info->nr; i++) { + for (size_t i = 0; i < cmd_info->nr; i++) { struct rev_cmdline_entry *e = cmd_info->rev + i; struct object_id oid; const char *refexpr = e->name; -- 2.52.0.590.g1f87b77810.dirty ``` ## Patrick Steinhardt, 2026-01-12 14:15 Subject: [PATCH v10 4/8] replay: support empty commit ranges Message-ID: <20260112-b4-pks-history-builtin-v10-4-e3c6aa5b4cec@pks.im> URL: https://gitlist.dev/e/20260112-b4-pks-history-builtin-v10-4-e3c6aa5b4cec%40pks.im In-Reply-To: <20260112-b4-pks-history-builtin-v10-0-e3c6aa5b4cec@pks.im> ``` In a subsequent commit we're about to introduce a new user of the replay subsystem. With that new user, the range of commits that we'll want to replay will be identified implicitly via "HEAD". With such implicit ranges it becomes likely that the range of revisions that we're asked to replay becomes empty. This case does not make sense with git-replay(1), but with the new command it will. This case is not currently supported by `replay_revisions()` though because we zero-initialize `struct merge_result`. This includes its `.clean` member, which indicates whether the merge ran into a conflict or not. But given that we don't have any revision to replay, we won't ever perform any merge at all, and consequently that member will never be set to `1`. We thus later think that there's been a merge conflict and return an error from `replay_commits()`. Address this issue by initializing the `.clean` member to `1`. Signed-off-by: Patrick Steinhardt --- replay.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/replay.c b/replay.c index 1e660171d2..a8e6d5b30b 100644 --- a/replay.c +++ b/replay.c @@ -266,7 +266,9 @@ int replay_revisions(struct rev_info *revs, struct commit *commit; struct commit *onto = NULL; struct merge_options merge_opt; - struct merge_result result; + struct merge_result result = { + .clean = 1, + }; char *advance; int ret; @@ -282,7 +284,6 @@ int replay_revisions(struct rev_info *revs, } init_basic_merge_options(&merge_opt, revs->repo); - memset(&result, 0, sizeof(result)); merge_opt.show_rename_progress = 0; last_commit = onto; replayed_commits = kh_init_oid_map(); -- 2.52.0.590.g1f87b77810.dirty ``` ## Patrick Steinhardt, 2026-01-12 14:15 Subject: [PATCH v10 5/8] replay: support updating detached HEAD Message-ID: <20260112-b4-pks-history-builtin-v10-5-e3c6aa5b4cec@pks.im> URL: https://gitlist.dev/e/20260112-b4-pks-history-builtin-v10-5-e3c6aa5b4cec%40pks.im In-Reply-To: <20260112-b4-pks-history-builtin-v10-0-e3c6aa5b4cec@pks.im> ``` In a subsequent commit we're about to introduce a new git-history(1) command, which will by default work on all local branches and HEAD. This is already well-supported by the replay machinery for most of the part: updating branches is one of its prime use cases, and the HEAD ref is also updated in case it points to any of the branches. However, what's not supported yet is to update HEAD in case it is not a symbolic ref. We determine the refs that need to be updated by iterating through the decorations of the original commit, but we only update those refs that are `DECORATION_REF_LOCAL`, which covers local branches. Address this gap by also handling `DECORATION_REF_HEAD`. Note though that this needs to only happen in case we're working on a detached HEAD. If HEAD is pointing to a branch, then we'd already update that branch via `DECORATION_REF_LOCAL`. Refactor the loop that iterates through the decorations a bit to make the individual conditions easier to understand. Based-on-patch-by: Elijah Newren Signed-off-by: Patrick Steinhardt --- replay.c | 47 +++++++++++++++++++++++++++++++++-------------- t/t3650-replay-basics.sh | 9 +++++++++ 2 files changed, 42 insertions(+), 14 deletions(-) diff --git a/replay.c b/replay.c index a8e6d5b30b..6a8cd2f72d 100644 --- a/replay.c +++ b/replay.c @@ -150,11 +150,17 @@ static void get_ref_information(struct repository *repo, static void set_up_replay_mode(struct repository *repo, struct rev_cmdline_info *cmd_info, const char *onto_name, + bool *detached_head, char **advance_name, struct commit **onto, struct strset **update_refs) { struct ref_info rinfo; + int head_flags = 0; + + refs_read_ref_full(get_main_ref_store(repo), "HEAD", + RESOLVE_REF_NO_RECURSE, NULL, &head_flags); + *detached_head = !(head_flags & REF_ISSYMREF); get_ref_information(repo, cmd_info, &rinfo); if (!rinfo.positive_refexprs) @@ -269,12 +275,13 @@ int replay_revisions(struct rev_info *revs, struct merge_result result = { .clean = 1, }; + bool detached_head; char *advance; int ret; advance = xstrdup_or_null(opts->advance); - set_up_replay_mode(revs->repo, &revs->cmdline, opts->onto, &advance, - &onto, &update_refs); + set_up_replay_mode(revs->repo, &revs->cmdline, opts->onto, + &detached_head, &advance, &onto, &update_refs); /* FIXME: Should allow replaying commits with the first as a root commit */ @@ -312,18 +319,30 @@ int replay_revisions(struct rev_info *revs, /* Update any necessary branches */ if (advance) continue; - decoration = get_name_decoration(&commit->object); - if (!decoration) - continue; - while (decoration) { - if (decoration->type == DECORATION_REF_LOCAL && - (opts->contained || strset_contains(update_refs, - decoration->name))) { - replay_result_queue_update(out, decoration->name, - &commit->object.oid, - &last_commit->object.oid); - } - decoration = decoration->next; + + for (decoration = get_name_decoration(&commit->object); + decoration; + decoration = decoration->next) + { + if (decoration->type != DECORATION_REF_LOCAL && + decoration->type != DECORATION_REF_HEAD) + continue; + + /* + * We only need to update HEAD separately in case it's + * detached. If it's not we'd already update the branch + * it is pointing to. + */ + if (decoration->type == DECORATION_REF_HEAD && !detached_head) + continue; + + if (!opts->contained && + !strset_contains(update_refs, decoration->name)) + continue; + + replay_result_queue_update(out, decoration->name, + &commit->object.oid, + &last_commit->object.oid); } } diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh index 307101eeb9..c862aa39f3 100755 --- a/t/t3650-replay-basics.sh +++ b/t/t3650-replay-basics.sh @@ -249,6 +249,15 @@ test_expect_success 'using replay on bare repo to rebase multiple divergent bran done ' +test_expect_success 'using replay to update detached HEAD' ' + current_head=$(git branch --show-current) && + test_when_finished git switch "$current_head" && + git switch --detach && + test_commit something && + git replay --ref-action=print --onto HEAD~2 --ref-action=print HEAD~..HEAD >updates && + test_grep "update HEAD " updates +' + test_expect_success 'merge.directoryRenames=false' ' # create a test case that stress-tests the rename caching git switch -c rename-onto && -- 2.52.0.590.g1f87b77810.dirty ``` ## Patrick Steinhardt, 2026-01-12 14:15 Subject: [PATCH v10 6/8] wt-status: provide function to expose status for trees Message-ID: <20260112-b4-pks-history-builtin-v10-6-e3c6aa5b4cec@pks.im> URL: https://gitlist.dev/e/20260112-b4-pks-history-builtin-v10-6-e3c6aa5b4cec%40pks.im In-Reply-To: <20260112-b4-pks-history-builtin-v10-0-e3c6aa5b4cec@pks.im> ``` The "wt-status" subsystem is responsible for printing status information around the current state of the working tree. This most importantly includes information around whether the working tree or the index have any changes. We're about to introduce a new command where the changes in neither of them are actually relevant to us. Instead, what we want is to format the changes between two different trees. While it is a little bit of a stretch to add this as functionality to _working tree_ status, it doesn't make any sense to open-code this functionality, either. Implement a new function `wt_status_collect_changes_trees()` that diffs two trees and formats the status accordingly. This function is not yet used, but will be in a subsequent commit. Signed-off-by: Patrick Steinhardt --- wt-status.c | 24 ++++++++++++++++++++++++ wt-status.h | 9 +++++++++ 2 files changed, 33 insertions(+) diff --git a/wt-status.c b/wt-status.c index e12adb26b9..95942399f8 100644 --- a/wt-status.c +++ b/wt-status.c @@ -612,6 +612,30 @@ static void wt_status_collect_updated_cb(struct diff_queue_struct *q, } } +void wt_status_collect_changes_trees(struct wt_status *s, + const struct object_id *old_treeish, + const struct object_id *new_treeish) +{ + struct diff_options opts = { 0 }; + + repo_diff_setup(s->repo, &opts); + opts.output_format = DIFF_FORMAT_CALLBACK; + opts.format_callback = wt_status_collect_updated_cb; + opts.format_callback_data = s; + opts.detect_rename = s->detect_rename >= 0 ? s->detect_rename : opts.detect_rename; + opts.rename_limit = s->rename_limit >= 0 ? s->rename_limit : opts.rename_limit; + opts.rename_score = s->rename_score >= 0 ? s->rename_score : opts.rename_score; + opts.flags.recursive = 1; + diff_setup_done(&opts); + + diff_tree_oid(old_treeish, new_treeish, "", &opts); + diffcore_std(&opts); + diff_flush(&opts); + wt_status_get_state(s->repo, &s->state, 0); + + diff_free(&opts); +} + static void wt_status_collect_changes_worktree(struct wt_status *s) { struct rev_info rev; diff --git a/wt-status.h b/wt-status.h index e40a27214a..e9fe32e98c 100644 --- a/wt-status.h +++ b/wt-status.h @@ -153,6 +153,15 @@ void wt_status_add_cut_line(struct wt_status *s); void wt_status_prepare(struct repository *r, struct wt_status *s); void wt_status_print(struct wt_status *s); void wt_status_collect(struct wt_status *s); + +/* + * Collect all changes between the two trees. Changes will be displayed as if + * they were staged into the index. + */ +void wt_status_collect_changes_trees(struct wt_status *s, + const struct object_id *old_treeish, + const struct object_id *new_treeish); + /* * Frees the buffers allocated by wt_status_collect. */ -- 2.52.0.590.g1f87b77810.dirty ``` ## Patrick Steinhardt, 2026-01-12 14:15 Subject: [PATCH v10 7/8] builtin: add new "history" command Message-ID: <20260112-b4-pks-history-builtin-v10-7-e3c6aa5b4cec@pks.im> URL: https://gitlist.dev/e/20260112-b4-pks-history-builtin-v10-7-e3c6aa5b4cec%40pks.im In-Reply-To: <20260112-b4-pks-history-builtin-v10-0-e3c6aa5b4cec@pks.im> ``` When rewriting history via git-rebase(1) there are a few very common use cases: - The ordering of two commits should be reversed. - A commit should be split up into two commits. - A commit should be dropped from the history completely. - Multiple commits should be squashed into one. - Editing an existing commit that is not the tip of the current branch. While these operations are all doable, it often feels needlessly kludgey to do so by doing an interactive rebase, using the editor to say what one wants, and then perform the actions. Also, some operations like splitting up a commit into two are way more involved than that and require a whole series of commands. Rebases also do not update dependent branches. The use of stacked branches has grown quite common with competing version control systems like Jujutsu though, so it clearly is a need that users have. While rebases _can_ serve this use case if one always works on the latest stacked branch, it is somewhat awkward and very easy to get wrong. Add a new "history" command to plug these gaps. This command will have several different subcommands to imperatively rewrite history for common use cases like the above. Signed-off-by: Patrick Steinhardt --- .gitignore | 1 + Documentation/git-history.adoc | 57 ++++++++++++++++++++++++++++++++++++++++++ Documentation/meson.build | 1 + Makefile | 1 + builtin.h | 1 + builtin/history.c | 22 ++++++++++++++++ command-list.txt | 1 + git.c | 1 + meson.build | 1 + t/meson.build | 1 + t/t3450-history.sh | 17 +++++++++++++ 11 files changed, 104 insertions(+) diff --git a/.gitignore b/.gitignore index 78a45cb5be..24635cf2d6 100644 --- a/.gitignore +++ b/.gitignore @@ -79,6 +79,7 @@ /git-grep /git-hash-object /git-help +/git-history /git-hook /git-http-backend /git-http-fetch diff --git a/Documentation/git-history.adoc b/Documentation/git-history.adoc new file mode 100644 index 0000000000..68c35f36b9 --- /dev/null +++ b/Documentation/git-history.adoc @@ -0,0 +1,57 @@ +git-history(1) +============== + +NAME +---- +git-history - EXPERIMENTAL: Rewrite history + +SYNOPSIS +-------- +[synopsis] +git history [] + +DESCRIPTION +----------- + +Rewrite history by rearranging or modifying specific commits in the +history. + +THIS COMMAND IS EXPERIMENTAL. THE BEHAVIOR MAY CHANGE. + +This command is related to linkgit:git-rebase[1] in that both commands can be +used to rewrite history. There are a couple of major differences though: + +* linkgit:git-history[1] can work in a bare repository as it does not need to + touch either the index or the worktree. +* linkgit:git-history[1] does not execute any linkgit:githooks[5] at the + current point in time. This may change in the future. +* linkgit:git-history[1] by default updates all branches that are descendants + of the original commit to point to the rewritten commit. + +Overall, linkgit:git-history[1] aims to provide a more opinionated way to modify +your commit history that is simpler to use compared to linkgit:git-rebase[1] in +general. + +Use linkgit:git-rebase[1] if you want to reapply a range of commits onto a +different base, or interactive rebases if you want to edit a range of commits +at once. + +LIMITATIONS +----------- + +This command does not (yet) work with histories that contain merges. You +should use linkgit:git-rebase[1] with the `--rebase-merges` flag instead. + +Furthermore, the command does not support operations that can result in merge +conflicts. This limitation is by design as history rewrites are not intended to +be stateful operations. The limitation can be lifted once (if) Git learns about +first-class conflicts. + +COMMANDS +-------- + +No commands are supported yet. + +GIT +--- +Part of the linkgit:git[1] suite diff --git a/Documentation/meson.build b/Documentation/meson.build index f02dbc20cb..fd2e8cc02d 100644 --- a/Documentation/meson.build +++ b/Documentation/meson.build @@ -64,6 +64,7 @@ manpages = { 'git-gui.adoc' : 1, 'git-hash-object.adoc' : 1, 'git-help.adoc' : 1, + 'git-history.adoc' : 1, 'git-hook.adoc' : 1, 'git-http-backend.adoc' : 1, 'git-http-fetch.adoc' : 1, diff --git a/Makefile b/Makefile index 1c64a5d2ae..c0569ed8e4 100644 --- a/Makefile +++ b/Makefile @@ -1418,6 +1418,7 @@ BUILTIN_OBJS += builtin/get-tar-commit-id.o BUILTIN_OBJS += builtin/grep.o BUILTIN_OBJS += builtin/hash-object.o BUILTIN_OBJS += builtin/help.o +BUILTIN_OBJS += builtin/history.o BUILTIN_OBJS += builtin/hook.o BUILTIN_OBJS += builtin/index-pack.o BUILTIN_OBJS += builtin/init-db.o diff --git a/builtin.h b/builtin.h index 1b35565fbd..93c91d07d4 100644 --- a/builtin.h +++ b/builtin.h @@ -172,6 +172,7 @@ int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix, struc int cmd_grep(int argc, const char **argv, const char *prefix, struct repository *repo); int cmd_hash_object(int argc, const char **argv, const char *prefix, struct repository *repo); int cmd_help(int argc, const char **argv, const char *prefix, struct repository *repo); +int cmd_history(int argc, const char **argv, const char *prefix, struct repository *repo); int cmd_hook(int argc, const char **argv, const char *prefix, struct repository *repo); int cmd_index_pack(int argc, const char **argv, const char *prefix, struct repository *repo); int cmd_init_db(int argc, const char **argv, const char *prefix, struct repository *repo); diff --git a/builtin/history.c b/builtin/history.c new file mode 100644 index 0000000000..f6fe32610b --- /dev/null +++ b/builtin/history.c @@ -0,0 +1,22 @@ +#include "builtin.h" +#include "gettext.h" +#include "parse-options.h" + +int cmd_history(int argc, + const char **argv, + const char *prefix, + struct repository *repo UNUSED) +{ + const char * const usage[] = { + N_("git history []"), + NULL, + }; + struct option options[] = { + OPT_END(), + }; + + argc = parse_options(argc, argv, prefix, options, usage, 0); + if (argc) + usagef("unrecognized argument: %s", argv[0]); + return 0; +} diff --git a/command-list.txt b/command-list.txt index accd3d0c4b..f9005cf459 100644 --- a/command-list.txt +++ b/command-list.txt @@ -115,6 +115,7 @@ git-grep mainporcelain info git-gui mainporcelain git-hash-object plumbingmanipulators git-help ancillaryinterrogators complete +git-history mainporcelain history git-hook purehelpers git-http-backend synchingrepositories git-http-fetch synchelpers diff --git a/git.c b/git.c index c5fad56813..744cb6527e 100644 --- a/git.c +++ b/git.c @@ -586,6 +586,7 @@ static struct cmd_struct commands[] = { { "grep", cmd_grep, RUN_SETUP_GENTLY }, { "hash-object", cmd_hash_object }, { "help", cmd_help }, + { "history", cmd_history, RUN_SETUP }, { "hook", cmd_hook, RUN_SETUP }, { "index-pack", cmd_index_pack, RUN_SETUP_GENTLY | NO_PARSEOPT }, { "init", cmd_init_db }, diff --git a/meson.build b/meson.build index a5a4e99b25..3a1d12caa4 100644 --- a/meson.build +++ b/meson.build @@ -610,6 +610,7 @@ builtin_sources = [ 'builtin/grep.c', 'builtin/hash-object.c', 'builtin/help.c', + 'builtin/history.c', 'builtin/hook.c', 'builtin/index-pack.c', 'builtin/init-db.c', diff --git a/t/meson.build b/t/meson.build index 459c52a489..73006b095a 100644 --- a/t/meson.build +++ b/t/meson.build @@ -387,6 +387,7 @@ integration_tests = [ 't3436-rebase-more-options.sh', 't3437-rebase-fixup-options.sh', 't3438-rebase-broken-files.sh', + 't3450-history.sh', 't3500-cherry.sh', 't3501-revert-cherry-pick.sh', 't3502-cherry-pick-merge.sh', diff --git a/t/t3450-history.sh b/t/t3450-history.sh new file mode 100755 index 0000000000..417c343d43 --- /dev/null +++ b/t/t3450-history.sh @@ -0,0 +1,17 @@ +#!/bin/sh + +test_description='tests for git-history command' + +. ./test-lib.sh + +test_expect_success 'does nothing without any arguments' ' + git history >out 2>&1 && + test_must_be_empty out +' + +test_expect_success 'raises an error with unknown argument' ' + test_must_fail git history garbage 2>err && + test_grep "unrecognized argument: garbage" err +' + +test_done -- 2.52.0.590.g1f87b77810.dirty ``` ## Patrick Steinhardt, 2026-01-12 14:15 Subject: [PATCH v10 8/8] builtin/history: implement "reword" subcommand Message-ID: <20260112-b4-pks-history-builtin-v10-8-e3c6aa5b4cec@pks.im> URL: https://gitlist.dev/e/20260112-b4-pks-history-builtin-v10-8-e3c6aa5b4cec%40pks.im In-Reply-To: <20260112-b4-pks-history-builtin-v10-0-e3c6aa5b4cec@pks.im> ``` Implement a new "reword" subcommand for git-history(1). This subcommand is similar to the user performing an interactive rebase with a single commit changed to use the "reword" instruction. The "reword" subcommand is built on top of the replay subsystem instead of the sequencer. This leads to some major differences compared to git-rebase(1): - We do not check out the commit that is to be reworded and instead perform the operation in-memory. This has the obvious benefit of being significantly faster compared to git-rebase(1), but even more importantly it allows the user to rewrite history even if there are local changes in the working tree or in the index. - We do not execute any hooks, even though we leave some room for changing this in the future. - By default, all local branches that contain the commit will be rewritten. This especially helps with workflows that use stacked branches. Helped-by: Elijah Newren Signed-off-by: Patrick Steinhardt --- Documentation/git-history.adoc | 20 +- builtin/history.c | 415 ++++++++++++++++++++++++++++++++++++++++- t/meson.build | 1 + t/t3450-history.sh | 6 +- t/t3451-history-reword.sh | 391 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 823 insertions(+), 10 deletions(-) diff --git a/Documentation/git-history.adoc b/Documentation/git-history.adoc index 68c35f36b9..154e262b76 100644 --- a/Documentation/git-history.adoc +++ b/Documentation/git-history.adoc @@ -8,7 +8,7 @@ git-history - EXPERIMENTAL: Rewrite history SYNOPSIS -------- [synopsis] -git history [] +git history reword [--ref-action=(branches|head|print)] DESCRIPTION ----------- @@ -50,7 +50,23 @@ first-class conflicts. COMMANDS -------- -No commands are supported yet. +The following commands are available to rewrite history in different ways: + +`reword `:: + Rewrite the commit message of the specified commit. All the other + details of this commit remain unchanged. This command will spawn an + editor with the current message of that commit. + +OPTIONS +------- + +`--ref-action=(branches|head|print)`:: + Control which references will be updated by the command, if any. With + `branches`, all local branches that point to commits which are + descendants of the original commit will be rewritten. With `head`, only + the current `HEAD` reference will be rewritten. With `print`, all + updates as they would be performed with `branches` are printed in a + format that can be consumed by linkgit:git-update-ref[1]. GIT --- diff --git a/builtin/history.c b/builtin/history.c index f6fe32610b..77ad0294dd 100644 --- a/builtin/history.c +++ b/builtin/history.c @@ -1,22 +1,427 @@ +#define USE_THE_REPOSITORY_VARIABLE + #include "builtin.h" +#include "commit.h" +#include "commit-reach.h" +#include "config.h" +#include "editor.h" +#include "environment.h" #include "gettext.h" +#include "hex.h" #include "parse-options.h" +#include "refs.h" +#include "replay.h" +#include "revision.h" +#include "sequencer.h" +#include "strvec.h" +#include "tree.h" +#include "wt-status.h" + +#define GIT_HISTORY_REWORD_USAGE \ + N_("git history reword [--ref-action=(branches|head|print)]") + +static void change_data_free(void *util, const char *str UNUSED) +{ + struct wt_status_change_data *d = util; + free(d->rename_source); + free(d); +} + +static int fill_commit_message(struct repository *repo, + const struct object_id *old_tree, + const struct object_id *new_tree, + const char *default_message, + const char *action, + struct strbuf *out) +{ + const char *path = git_path_commit_editmsg(); + const char *hint = + _("Please enter the commit message for the %s changes." + " Lines starting\nwith '%s' will be ignored, and an" + " empty message aborts the commit.\n"); + struct wt_status s; + + strbuf_addstr(out, default_message); + strbuf_addch(out, '\n'); + strbuf_commented_addf(out, comment_line_str, hint, action, comment_line_str); + write_file_buf(path, out->buf, out->len); + + wt_status_prepare(repo, &s); + FREE_AND_NULL(s.branch); + s.ahead_behind_flags = AHEAD_BEHIND_QUICK; + s.commit_template = 1; + s.colopts = 0; + s.display_comment_prefix = 1; + s.hints = 0; + s.use_color = 0; + s.whence = FROM_COMMIT; + s.committable = 1; + + s.fp = fopen(git_path_commit_editmsg(), "a"); + if (!s.fp) + return error_errno(_("could not open '%s'"), git_path_commit_editmsg()); + + wt_status_collect_changes_trees(&s, old_tree, new_tree); + wt_status_print(&s); + wt_status_collect_free_buffers(&s); + string_list_clear_func(&s.change, change_data_free); + + strbuf_reset(out); + if (launch_editor(path, out, NULL)) { + fprintf(stderr, _("Aborting commit as launching the editor failed.\n")); + return -1; + } + strbuf_stripspace(out, comment_line_str); + + cleanup_message(out, COMMIT_MSG_CLEANUP_ALL, 0); + + if (!out->len) { + fprintf(stderr, _("Aborting commit due to empty commit message.\n")); + return -1; + } + + return 0; +} + +static int commit_tree_with_edited_message(struct repository *repo, + const char *action, + struct commit *original, + struct commit **out) +{ + const char *exclude_gpgsig[] = { + /* We reencode the message, so the encoding needs to be stripped. */ + "encoding", + /* We need to strip signatures as those will become invalid. */ + "gpgsig", + "gpgsig-sha256", + NULL, + }; + const char *original_message, *original_body, *ptr; + struct commit_extra_header *original_extra_headers = NULL; + struct strbuf commit_message = STRBUF_INIT; + struct object_id rewritten_commit_oid; + struct object_id original_tree_oid; + struct object_id parent_tree_oid; + char *original_author = NULL; + struct commit *parent; + size_t len; + int ret; + + original_tree_oid = repo_get_commit_tree(repo, original)->object.oid; + + parent = original->parents ? original->parents->item : NULL; + if (parent) { + if (repo_parse_commit(repo, parent)) { + ret = error(_("unable to parse parent commit %s"), + oid_to_hex(&parent->object.oid)); + goto out; + } + + parent_tree_oid = repo_get_commit_tree(repo, parent)->object.oid; + } else { + oidcpy(&parent_tree_oid, repo->hash_algo->empty_tree); + } + + /* We retain authorship of the original commit. */ + original_message = repo_logmsg_reencode(repo, original, NULL, NULL); + ptr = find_commit_header(original_message, "author", &len); + if (ptr) + original_author = xmemdupz(ptr, len); + find_commit_subject(original_message, &original_body); + + ret = fill_commit_message(repo, &parent_tree_oid, &original_tree_oid, + original_body, action, &commit_message); + if (ret < 0) + goto out; + + original_extra_headers = read_commit_extra_headers(original, exclude_gpgsig); + + ret = commit_tree_extended(commit_message.buf, commit_message.len, &original_tree_oid, + original->parents, &rewritten_commit_oid, original_author, + NULL, NULL, original_extra_headers); + if (ret < 0) + goto out; + + *out = lookup_commit_or_die(&rewritten_commit_oid, "rewritten commit"); + +out: + free_commit_extra_headers(original_extra_headers); + strbuf_release(&commit_message); + free(original_author); + return ret; +} + +enum ref_action { + REF_ACTION_DEFAULT, + REF_ACTION_BRANCHES, + REF_ACTION_HEAD, + REF_ACTION_PRINT, +}; + +static int parse_ref_action(const struct option *opt, const char *value, int unset) +{ + enum ref_action *action = opt->value; + + BUG_ON_OPT_NEG_NOARG(unset, value); + if (!strcmp(value, "branches")) { + *action = REF_ACTION_BRANCHES; + } else if (!strcmp(value, "head")) { + *action = REF_ACTION_HEAD; + } else if (!strcmp(value, "print")) { + *action = REF_ACTION_PRINT; + } else { + return error(_("%s expects one of 'branches', 'head' or 'print'"), + opt->long_name); + } + + return 0; +} + +static int handle_reference_updates(enum ref_action action, + struct repository *repo, + struct commit *original, + struct commit *rewritten, + const char *reflog_msg) +{ + const struct name_decoration *decoration; + struct replay_revisions_options opts = { 0 }; + struct replay_result result = { 0 }; + struct ref_transaction *transaction = NULL; + struct strvec args = STRVEC_INIT; + struct strbuf err = STRBUF_INIT; + struct commit *head = NULL; + struct rev_info revs; + char hex[GIT_MAX_HEXSZ + 1]; + bool detached_head; + int head_flags = 0; + int ret; + + refs_read_ref_full(get_main_ref_store(repo), "HEAD", + RESOLVE_REF_NO_RECURSE, NULL, &head_flags); + detached_head = !(head_flags & REF_ISSYMREF); + + repo_init_revisions(repo, &revs, NULL); + strvec_push(&args, "ignored"); + strvec_push(&args, "--reverse"); + strvec_push(&args, "--topo-order"); + strvec_push(&args, "--full-history"); + + /* We only want to see commits that are descendants of the old commit. */ + strvec_pushf(&args, "--ancestry-path=%s", + oid_to_hex(&original->object.oid)); + + /* + * Ancestry path may also show ancestors of the old commit, but we + * don't want to see those, either. + */ + strvec_pushf(&args, "^%s", oid_to_hex(&original->object.oid)); + + /* + * When we're asked to update HEAD we need to verify that the commit + * that we want to rewrite is actually an ancestor of it and, if so, + * update it. Otherwise we'll update (or print) all descendant + * branches. + */ + if (action == REF_ACTION_HEAD) { + struct commit_list *from_list = NULL; + + head = lookup_commit_reference_by_name("HEAD"); + if (!head) { + ret = error(_("cannot look up HEAD")); + goto out; + } + + commit_list_insert(original, &from_list); + ret = repo_is_descendant_of(repo, head, from_list); + free_commit_list(from_list); + + if (ret < 0) { + ret = error(_("cannot determine descendance")); + goto out; + } else if (!ret) { + ret = error(_("rewritten commit must be an ancestor " + "of HEAD when using --ref-action=head")); + goto out; + } + + strvec_push(&args, "HEAD"); + } else { + strvec_push(&args, "--branches"); + strvec_push(&args, "HEAD"); + } + + setup_revisions_from_strvec(&args, &revs, NULL); + if (revs.nr) + BUG("revisions were set up with invalid argument '%s'", args.v[0]); + + opts.onto = oid_to_hex_r(hex, &rewritten->object.oid); + + ret = replay_revisions(&revs, &opts, &result); + if (ret) + goto out; + + switch (action) { + case REF_ACTION_BRANCHES: + case REF_ACTION_HEAD: + 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 = ref_transaction_update(transaction, + result.updates[i].refname, + &result.updates[i].new_oid, + &result.updates[i].old_oid, + NULL, NULL, 0, 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 + * handle updating references that point to `original`. + */ + for (decoration = get_name_decoration(&original->object); + decoration; + decoration = decoration->next) + { + if (decoration->type != DECORATION_REF_LOCAL && + decoration->type != DECORATION_REF_HEAD) + continue; + + if (action == REF_ACTION_HEAD && + decoration->type != DECORATION_REF_HEAD) + continue; + + /* + * We only need to update HEAD separately in case it's + * detached. If it's not we'd already update the branch + * it is pointing to. + */ + if (action == REF_ACTION_BRANCHES && + decoration->type == DECORATION_REF_HEAD && + !detached_head) + continue; + + ret = ref_transaction_update(transaction, + decoration->name, + &rewritten->object.oid, + &original->object.oid, + NULL, NULL, 0, reflog_msg, &err); + if (ret) { + ret = error(_("failed to update ref '%s': %s"), + decoration->name, err.buf); + goto out; + } + } + + if (ref_transaction_commit(transaction, &err)) { + ret = error(_("failed to commit ref transaction: %s"), err.buf); + goto out; + } + + break; + case REF_ACTION_PRINT: + for (size_t i = 0; i < result.updates_nr; i++) + printf("update %s %s %s\n", + result.updates[i].refname, + oid_to_hex(&result.updates[i].new_oid), + oid_to_hex(&result.updates[i].old_oid)); + break; + default: + BUG("unsupported ref action %d", action); + } + + ret = 0; + +out: + ref_transaction_free(transaction); + replay_result_release(&result); + release_revisions(&revs); + strbuf_release(&err); + strvec_clear(&args); + return ret; +} + +static int cmd_history_reword(int argc, + const char **argv, + const char *prefix, + struct repository *repo) +{ + const char * const usage[] = { + GIT_HISTORY_REWORD_USAGE, + NULL, + }; + enum ref_action action = REF_ACTION_DEFAULT; + struct option options[] = { + OPT_CALLBACK_F(0, "ref-action", &action, N_(""), + N_("control ref update behavior (branches|head|print)"), + PARSE_OPT_NONEG, parse_ref_action), + OPT_END(), + }; + struct strbuf reflog_msg = STRBUF_INIT; + struct commit *original, *rewritten; + 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; + } + + ret = commit_tree_with_edited_message(repo, "reworded", original, &rewritten); + if (ret < 0) { + ret = error(_("failed writing reworded commit")); + goto out; + } + + strbuf_addf(&reflog_msg, "reword: updating %s", argv[0]); + + ret = handle_reference_updates(action, repo, original, rewritten, + reflog_msg.buf); + if (ret < 0) { + ret = error(_("failed replaying descendants")); + goto out; + } + + ret = 0; + +out: + strbuf_release(&reflog_msg); + return ret; +} int cmd_history(int argc, const char **argv, const char *prefix, - struct repository *repo UNUSED) + struct repository *repo) { const char * const usage[] = { - N_("git history []"), + GIT_HISTORY_REWORD_USAGE, NULL, }; + parse_opt_subcommand_fn *fn = NULL; struct option options[] = { + OPT_SUBCOMMAND("reword", &fn, cmd_history_reword), OPT_END(), }; argc = parse_options(argc, argv, prefix, options, usage, 0); - if (argc) - usagef("unrecognized argument: %s", argv[0]); - return 0; + return fn(argc, argv, prefix, repo); } diff --git a/t/meson.build b/t/meson.build index 73006b095a..c9f92450dc 100644 --- a/t/meson.build +++ b/t/meson.build @@ -388,6 +388,7 @@ integration_tests = [ 't3437-rebase-fixup-options.sh', 't3438-rebase-broken-files.sh', 't3450-history.sh', + 't3451-history-reword.sh', 't3500-cherry.sh', 't3501-revert-cherry-pick.sh', 't3502-cherry-pick-merge.sh', diff --git a/t/t3450-history.sh b/t/t3450-history.sh index 417c343d43..f513463b92 100755 --- a/t/t3450-history.sh +++ b/t/t3450-history.sh @@ -5,13 +5,13 @@ test_description='tests for git-history command' . ./test-lib.sh test_expect_success 'does nothing without any arguments' ' - git history >out 2>&1 && - test_must_be_empty out + test_must_fail git history 2>err && + test_grep "need a subcommand" err ' test_expect_success 'raises an error with unknown argument' ' test_must_fail git history garbage 2>err && - test_grep "unrecognized argument: garbage" err + test_grep "unknown subcommand: .garbage." err ' test_done diff --git a/t/t3451-history-reword.sh b/t/t3451-history-reword.sh new file mode 100755 index 0000000000..3594421b68 --- /dev/null +++ b/t/t3451-history-reword.sh @@ -0,0 +1,391 @@ +#!/bin/sh + +test_description='tests for git-history reword subcommand' + +. ./test-lib.sh +. "$TEST_DIRECTORY/lib-log-graph.sh" + +reword_with_message () { + cat >message && + write_script fake-editor.sh <<-\EOF && + cp message "$1" + EOF + test_set_editor "$(pwd)"/fake-editor.sh && + git history reword "$@" && + rm fake-editor.sh message +} + +expect_graph () { + cat >expect && + lib_test_cmp_graph --graph --format=%s "$@" +} + +expect_log () { + git log --format="%s" "$@" >actual && + cat >expect && + test_cmp expect actual +} + +test_expect_success 'can reword tip of a branch' ' + 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 && + reword_with_message HEAD <<-EOF && + third reworded + EOF + git symbolic-ref HEAD >actual && + test_cmp expect actual && + + expect_log <<-\EOF && + third reworded + second + first + EOF + + git reflog >reflog && + test_grep "reword: updating HEAD" reflog + ) +' + +test_expect_success 'can reword commit in the middle' ' + 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 && + reword_with_message HEAD~ <<-EOF && + second reworded + EOF + git symbolic-ref HEAD >actual && + test_cmp expect actual && + + expect_log <<-\EOF + third + second reworded + first + EOF + ) +' + +test_expect_success 'can reword commit in the middle even on detached head' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + test_commit first && + test_commit second && + test_commit third_on_main && + git checkout --detach HEAD^ && + test_commit third_on_head && + + reword_with_message HEAD~ <<-EOF && + second reworded + EOF + + expect_graph HEAD --branches <<-\EOF + * third_on_head + | * third_on_main + |/ + * second reworded + * first + EOF + ) +' + +test_expect_success 'can reword the detached head' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + test_commit first && + test_commit second && + git checkout --detach HEAD && + test_commit third && + + reword_with_message HEAD <<-EOF && + third reworded + EOF + + expect_log <<-\EOF + third reworded + second + first + EOF + ) +' + +test_expect_success 'can reword root commit' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + test_commit first && + test_commit second && + test_commit third && + reword_with_message HEAD~2 <<-EOF && + first reworded + EOF + + expect_log <<-\EOF + third + second + first reworded + EOF + ) +' + +test_expect_success 'can reword in a bare repo' ' + test_when_finished "rm -rf repo repo.git" && + git init repo && + test_commit -C repo first && + git clone --bare repo repo.git && + ( + cd repo.git && + reword_with_message HEAD <<-EOF && + reworded + EOF + + expect_log <<-\EOF + reworded + EOF + ) +' + +test_expect_success 'can reword a commit on a different branch' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + test_commit base && + git branch theirs && + test_commit ours && + git switch theirs && + test_commit theirs && + + git rev-parse ours >ours-before && + reword_with_message theirs <<-EOF && + Reworded theirs + EOF + git rev-parse ours >ours-after && + test_cmp ours-before ours-after && + + expect_graph --branches <<-\EOF + * Reworded theirs + | * ours + |/ + * base + EOF + ) +' + +test_expect_success 'can reword 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 && + + # It is not possible to replay merge commits embedded in the + # history (yet). + test_must_fail git history reword HEAD~ 2>err && + test_grep "replaying merge commits is not supported yet" err && + + # But it is possible to reword a merge commit directly. + reword_with_message HEAD <<-EOF && + Reworded merge commit + EOF + expect_graph <<-\EOF + * Reworded merge commit + |\ + | * theirs + * | ours + |/ + * base + EOF + ) +' + +test_expect_success '--ref-action=print 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 ours && + git switch branch && + test_commit theirs && + + git refs list >refs-expect && + reword_with_message --ref-action=print base >updates <<-\EOF && + reworded commit + EOF + git refs list >refs-actual && + test_cmp refs-expect refs-actual && + + test_grep "update refs/heads/branch" updates && + test_grep "update refs/heads/main" updates && + git update-ref --stdin err && + test_grep "rewritten commit must be an ancestor of HEAD" err && + + reword_with_message --ref-action=head base >updates <<-\EOF && + reworded base + EOF + expect_log HEAD <<-\EOF && + ours + reworded base + EOF + expect_log main <<-\EOF + theirs + base + EOF + ) +' + +test_expect_success 'editor shows proper status' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + test_commit first && + + write_script fake-editor.sh <<-\EOF && + cp "$1" . && + printf "\namend a comment\n" >>"$1" + EOF + test_set_editor "$(pwd)"/fake-editor.sh && + git history reword HEAD && + + cat >expect <<-EOF && + first + + # Please enter the commit message for the reworded changes. Lines starting + # with ${SQ}#${SQ} will be ignored, and an empty message aborts the commit. + # Changes to be committed: + # new file: first.t + # + EOF + test_cmp expect COMMIT_EDITMSG && + + test_commit_message HEAD <<-\EOF + first + + amend a comment + EOF + ) +' + +# For now, git-history(1) does not yet execute any hooks. This is subject to +# change in the future, and if it does this test here is expected to start +# failing. In other words, this test is not an endorsement of the current +# status quo. +test_expect_success 'hooks are not executed for rewritten commits' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + test_commit first && + test_commit second && + test_commit third && + + ORIG_PATH="$(pwd)" && + export ORIG_PATH && + for hook in prepare-commit-msg pre-commit post-commit post-rewrite commit-msg + do + write_script .git/hooks/$hook <<-\EOF || exit 1 + touch "$ORIG_PATH/hooks.log + EOF + done && + + reword_with_message HEAD~ <<-EOF && + second reworded + EOF + + cat >expect <<-EOF && + third + second reworded + first + EOF + git log --format=%s >actual && + test_cmp expect actual && + + test_path_is_missing hooks.log + ) +' + +test_expect_success 'aborts with empty commit message' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + test_commit first && + + ! reword_with_message HEAD 2>err a && + echo bar >b && + git add b && + reword_with_message HEAD <<-EOF && + message + EOF + cat >expect <<-\EOF && + M a + M b + ?? actual + ?? expect + EOF + git status --porcelain >actual && + test_cmp expect actual + ) +' + +test_done -- 2.52.0.590.g1f87b77810.dirty ``` ## Junio C Hamano, 2026-01-12 15:08 Subject: Re: [PATCH v10 1/8] builtin/replay: extract core logic to replay revisions Message-ID: URL: https://gitlist.dev/e/xmqqv7h6na0s.fsf%40gitster.g In-Reply-To: <20260112-b4-pks-history-builtin-v10-1-e3c6aa5b4cec@pks.im> ``` Patrick Steinhardt writes: > - die_for_incompatible_opt2(!!onto_name, "--onto", > - !!*advance_name, "--advance"); > + if (!(!!onto_name ^ !!*advance_name)) > + BUG("expected either onto_name or *advance_name in this function"); This brings our crypticness to a whole new level. onto_name not being NULL is a sign that "--onto" was given, while *advance_name pointer points at the string that "--advance" option has received. We are saying that only one of these two must be non-NULL, and the other must be NULL. I know !!VAR is an idiom to turn any pointer into 0 (=NULL) or 1 (!=NULL), but isn't the latter (i.e., normalizing all non-NULL pointer to 1) a bit overkill, which becomes only necessary because the construction wants to use "^" as "sides of this operator are different Boolean values" operator. And then to add on top, the whole thing is !(negated). I wonder if if (!onto_name != !*advance_name) BUG("one and only one of --onto/--advance must be given"); is easier to follow without being overly cute? Thanks. ``` ## Patrick Steinhardt, 2026-01-12 15:37 Subject: Re: [PATCH v10 1/8] builtin/replay: extract core logic to replay revisions Message-ID: URL: https://gitlist.dev/e/aWUVHKMfG0vRb8_G%40pks.im In-Reply-To: ``` On Mon, Jan 12, 2026 at 07:08:51AM -0800, Junio C Hamano wrote: > Patrick Steinhardt writes: > > > - die_for_incompatible_opt2(!!onto_name, "--onto", > > - !!*advance_name, "--advance"); > > + if (!(!!onto_name ^ !!*advance_name)) > > + BUG("expected either onto_name or *advance_name in this function"); > > This brings our crypticness to a whole new level. onto_name not > being NULL is a sign that "--onto" was given, while *advance_name > pointer points at the string that "--advance" option has received. > We are saying that only one of these two must be non-NULL, and the > other must be NULL. > > I know !!VAR is an idiom to turn any pointer into 0 (=NULL) or 1 > (!=NULL), but isn't the latter (i.e., normalizing all non-NULL > pointer to 1) a bit overkill, which becomes only necessary because > the construction wants to use "^" as "sides of this operator are > different Boolean values" operator. And then to add on top, the > whole thing is !(negated). I wonder if > > if (!onto_name != !*advance_name) > BUG("one and only one of --onto/--advance must be given"); > > is easier to follow without being overly cute? It should be: if (!onto_name == !*advance_name) BUG(...); But other than that it reads way better indeed. Fixed up locally, thanks! Patrick ``` ## Elijah Newren, 2026-01-13 06:00 Subject: Re: [PATCH v10 1/8] builtin/replay: extract core logic to replay revisions Message-ID: URL: https://gitlist.dev/e/CABPp-BGOcMRerGpH5HGkUR4-DKPx%2BVmkWzqRt8qideZoJBrvHg%40mail.gmail.com In-Reply-To: <20260112-b4-pks-history-builtin-v10-1-e3c6aa5b4cec@pks.im> ``` On Mon, Jan 12, 2026 at 6:17 AM Patrick Steinhardt wrote: [...] > - die_for_incompatible_opt2(!!onto_name, "--onto", > - !!*advance_name, "--advance"); > + if (!(!!onto_name ^ !!*advance_name)) > + BUG("expected either onto_name or *advance_name in this function"); > + I liked Junio's comments on this section and your response. > @@ -253,6 +254,137 @@ static struct commit *pick_regular_commit(struct repository *repo, [...] > + if (!result.clean) { > + out->merge_conflict = true; > + ret = -1; Even if you keep the special merge_conflict field for other purposes, setting ret to -1 here still feels very wrong. Negative return codes, and especially -1, is used throughout the merge machinery to signal unexpected errors like failure to read/write to disk. Further, it's inconsistent with how builtin/merge-tree.c works, where both in code and in documentation merge result code is 0 == clean, 1 == merge conflicts. I'm worried using -1 here could cause some nasty future maintenance headaches trying to understand the field if left this way, at least for me. As mentioned in the last round, the ret value here should be 1. > @@ -306,21 +438,11 @@ int cmd_replay(int argc, [...] > - die_for_incompatible_opt2(!!advance_name_opt, "--advance", > - contained, "--contained"); > + die_for_incompatible_opt2(!!opts.advance, "--advance", > + opts.contained, "--contained"); This predates your patch, but I'm wondering if there's anything we should do to clarify and/or simplify the first check. The original form of the check + die_for_incompatible_opt2(!!opts.advance, "--advance", + opts.contained, "--contained"); was created because (a) I had code that allowed --onto to be implicit in some cases, and (b) I was thinking only of --onto and --advance modes. However: (a) we got rid of the implicit mode selection from my private branch, and (b) Siddharth added patches which added a --revert mode. Those patches caused confusion around the interplay of --contained with the new mode (https://lore.kernel.org/git/xmqq3460ocv7.fsf@gitster.g/). I thought the synopsis: "([--contained] --onto | --advance ) " implied clearly enough that --contained is a sub-mode of --onto, but apparently that wasn't the case. Perhaps we can strengthen that understanding if we change the check here to instead be something like if (opts.contained && !opts.onto) die("--onto must be specified if --contained is") Definitely not critical; but might be a nice cleanup. > + die_for_incompatible_opt2(!!opts.advance, "--advance", > + !!opts.onto, "--onto"); Yeah, and Siddharth can convert this to a die_for_incompatible_opt3() call, adding "--revert" to it when he rerolls his series on top of yours. > - /* Return */ > - if (ret < 0) > - exit(128); > - return ret ? 0 : 1; > + if (ret) { > + if (result.merge_conflict) > + return 1; > + return 128; > + } > + > + return 0; > } You mentioned that you wanted to keep the merge_conflict field due to some future patches beyond the currently submitted series. I wonder if it'd make more sense to introduce that field once you introduce the new patches, but i don't feel too strongly about that. I do feel strongly that the place where you set ret to -1 is problematic and should be changed to 1. ``` ## Elijah Newren, 2026-01-13 06:00 Subject: Re: [PATCH v10 4/8] replay: support empty commit ranges Message-ID: URL: https://gitlist.dev/e/CABPp-BGhtPyiVT%3D32NXz3k8m%3D%2BZgPziXueM4Y8%2Bg4dAUtN9osw%40mail.gmail.com In-Reply-To: <20260112-b4-pks-history-builtin-v10-4-e3c6aa5b4cec@pks.im> ``` On Mon, Jan 12, 2026 at 6:17 AM Patrick Steinhardt wrote: > > In a subsequent commit we're about to introduce a new user of the replay > subsystem. With that new user, the range of commits that we'll want to > replay will be identified implicitly via "HEAD". With such implicit > ranges it becomes likely that the range of revisions that we're asked to > replay becomes empty. This case does not make sense with git-replay(1), > but with the new command it will. I think I know what you were trying to say, but this feels misleading; it could be the commit at the tip of any branch, not just HEAD. Perhaps: In a subsequent commit we're about to introduce a new user of the replay subsystem. With that new user, the range of commits that we'll want to replay will be identified implicitly via a single commit, and will include all descendants of that commit to any branch. If that commit has no descendants (because it's the tip of some branch), then the range of revisions that we're asked to replay becomes empty. This case does not make sense with git-replay(1), but with the new command it will. > This case is not currently supported by `replay_revisions()` though > because we zero-initialize `struct merge_result`. This includes its > `.clean` member, which indicates whether the merge ran into a conflict > or not. But given that we don't have any revision to replay, we won't > ever perform any merge at all, and consequently that member will never > be set to `1`. We thus later think that there's been a merge conflict > and return an error from `replay_commits()`. > > Address this issue by initializing the `.clean` member to `1`. > > Signed-off-by: Patrick Steinhardt > --- > replay.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/replay.c b/replay.c > index 1e660171d2..a8e6d5b30b 100644 > --- a/replay.c > +++ b/replay.c > @@ -266,7 +266,9 @@ int replay_revisions(struct rev_info *revs, > struct commit *commit; > struct commit *onto = NULL; > struct merge_options merge_opt; > - struct merge_result result; > + struct merge_result result = { > + .clean = 1, > + }; > char *advance; > int ret; > > @@ -282,7 +284,6 @@ int replay_revisions(struct rev_info *revs, > } > > init_basic_merge_options(&merge_opt, revs->repo); > - memset(&result, 0, sizeof(result)); > merge_opt.show_rename_progress = 0; > last_commit = onto; > replayed_commits = kh_init_oid_map(); > > -- > 2.52.0.590.g1f87b77810.dirty Looks good otherwise; thanks for splitting this out. ``` ## Elijah Newren, 2026-01-13 06:00 Subject: Re: [PATCH v10 5/8] replay: support updating detached HEAD Message-ID: URL: https://gitlist.dev/e/CABPp-BFvi0xmpP1jcU0qc7c-KhYXgzpCVbXyto3FMSFSSwrKoA%40mail.gmail.com In-Reply-To: <20260112-b4-pks-history-builtin-v10-5-e3c6aa5b4cec@pks.im> ``` On Mon, Jan 12, 2026 at 6:17 AM Patrick Steinhardt wrote: [...] > + refs_read_ref_full(get_main_ref_store(repo), "HEAD", > + RESOLVE_REF_NO_RECURSE, NULL, &head_flags); > + *detached_head = !(head_flags & REF_ISSYMREF); Ah, I like this simpler way to determine if we have a detached HEAD. Out of curiosity, though, is there a need to specify RESOLVE_REF_NO_RECURSE? I waffled on that for a bit and just left it out. > @@ -312,18 +319,30 @@ int replay_revisions(struct rev_info *revs, [...] > + for (decoration = get_name_decoration(&commit->object); > + decoration; > + decoration = decoration->next) > + { > + if (decoration->type != DECORATION_REF_LOCAL && > + decoration->type != DECORATION_REF_HEAD) > + continue; > + > + /* > + * We only need to update HEAD separately in case it's > + * detached. If it's not we'd already update the branch > + * it is pointing to. > + */ > + if (decoration->type == DECORATION_REF_HEAD && !detached_head) > + continue; > + > + if (!opts->contained && > + !strset_contains(update_refs, decoration->name)) > + continue; I like this refactoring to make the conditions clearer; very nice. > diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh > index 307101eeb9..c862aa39f3 100755 > --- a/t/t3650-replay-basics.sh > +++ b/t/t3650-replay-basics.sh > @@ -249,6 +249,15 @@ test_expect_success 'using replay on bare repo to rebase multiple divergent bran > done > ' > > +test_expect_success 'using replay to update detached HEAD' ' > + current_head=$(git branch --show-current) && > + test_when_finished git switch "$current_head" && > + git switch --detach && > + test_commit something && > + git replay --ref-action=print --onto HEAD~2 --ref-action=print HEAD~..HEAD >updates && > + test_grep "update HEAD " updates > +' > + > test_expect_success 'merge.directoryRenames=false' ' > # create a test case that stress-tests the rename caching > git switch -c rename-onto && > > -- > 2.52.0.590.g1f87b77810.dirty Good extra test too. ``` ## Elijah Newren, 2026-01-13 06:01 Subject: Re: [PATCH v10 8/8] builtin/history: implement "reword" subcommand Message-ID: URL: https://gitlist.dev/e/CABPp-BHq35ySWhud-_53BXE1ed%3DV7MqjrpWDzgWkcs7o_Lum-A%40mail.gmail.com In-Reply-To: <20260112-b4-pks-history-builtin-v10-8-e3c6aa5b4cec@pks.im> ``` On Mon, Jan 12, 2026 at 6:17 AM Patrick Steinhardt wrote: > > Implement a new "reword" subcommand for git-history(1). This subcommand > is similar to the user performing an interactive rebase with a single > commit changed to use the "reword" instruction. > > The "reword" subcommand is built on top of the replay subsystem > instead of the sequencer. This leads to some major differences compared > to git-rebase(1): > > - We do not check out the commit that is to be reworded and instead > perform the operation in-memory. This has the obvious benefit of > being significantly faster compared to git-rebase(1), but even more > importantly it allows the user to rewrite history even if there are > local changes in the working tree or in the index. > > - We do not execute any hooks, even though we leave some room for > changing this in the future. > > - By default, all local branches that contain the commit will be > rewritten. This especially helps with workflows that use stacked > branches. > > Helped-by: Elijah Newren > Signed-off-by: Patrick Steinhardt This patch is now a mixture of my patch and yours; all the changes since your v9 & my patch look good to me. ``` ## Elijah Newren, 2026-01-13 06:01 Subject: Re: [PATCH v10 0/8] Introduce git-history(1) command for easy history editing Message-ID: URL: https://gitlist.dev/e/CABPp-BEDpjfxLrB5nWS%2BDkkTBKR%2BLe%2BjJhQiJCio%3DWkruchioA%40mail.gmail.com In-Reply-To: <20260112-b4-pks-history-builtin-v10-0-e3c6aa5b4cec@pks.im> ``` On Mon, Jan 12, 2026 at 6:17 AM Patrick Steinhardt wrote: [...] > Changes in v10: > - Split out changes that make `replay_revisions()` handle empty > revision ranges properly. > - Drop redundant `struct repository` parameter in > `replay_revisions()`. > - Move around `die_for_incompatible_opt2()`. > - Fix up some of the headers in "replay.c". > - Split out the changes that make `replay_revisions()` handle empty > revision ranges properly. > - Create a separate commit that fixes updating detached HEAD. > - Strip "encoding" header as we're reencoding commit messages. > - Merge the logic to update refs in in `REF_ACTION_HEAD` and > `REF_ACTION_BRANCHES` modes. > - Link to v9: https://lore.kernel.org/r/20260109-b4-pks-history-builtin-v9-0-8766101814c6@pks.im Both Junio and I highlighted some changes in patch 1 (you already indicated your agreement with Junio), and I had some minor rewording suggestions for the commit message of patch 4. All other patches in v10 look good to me. ``` ## Patrick Steinhardt, 2026-01-13 07:30 Subject: Re: [PATCH v10 5/8] replay: support updating detached HEAD Message-ID: URL: https://gitlist.dev/e/aWX0nsiqdOJMwGHH%40pks.im In-Reply-To: ``` On Mon, Jan 12, 2026 at 10:00:53PM -0800, Elijah Newren wrote: > On Mon, Jan 12, 2026 at 6:17 AM Patrick Steinhardt wrote: > [...] > > + refs_read_ref_full(get_main_ref_store(repo), "HEAD", > > + RESOLVE_REF_NO_RECURSE, NULL, &head_flags); > > + *detached_head = !(head_flags & REF_ISSYMREF); > > Ah, I like this simpler way to determine if we have a detached HEAD. > Out of curiosity, though, is there a need to specify > RESOLVE_REF_NO_RECURSE? I waffled on that for a bit and just left it > out. There isn't, but I felt like it clarifies that we really only care about the immediate ref's value. It's also more efficient, even though that does not matter at all in the grand scheme of things. Patrick ``` ## Patrick Steinhardt, 2026-01-13 07:30 Subject: Re: [PATCH v10 4/8] replay: support empty commit ranges Message-ID: URL: https://gitlist.dev/e/aWX0qcDrBoeN64WI%40pks.im In-Reply-To: ``` On Mon, Jan 12, 2026 at 10:00:27PM -0800, Elijah Newren wrote: > On Mon, Jan 12, 2026 at 6:17 AM Patrick Steinhardt wrote: > > > > In a subsequent commit we're about to introduce a new user of the replay > > subsystem. With that new user, the range of commits that we'll want to > > replay will be identified implicitly via "HEAD". With such implicit > > ranges it becomes likely that the range of revisions that we're asked to > > replay becomes empty. This case does not make sense with git-replay(1), > > but with the new command it will. > > I think I know what you were trying to say, but this feels misleading; > it could be the commit at the tip of any branch, not just HEAD. > Perhaps: > > In a subsequent commit we're about to introduce a new user of the replay > subsystem. With that new user, the range of commits that we'll want to > replay will be identified implicitly via a single commit, and will > include all descendants of that commit to any branch. If that commit has > no descendants (because it's the tip of some branch), then the range of > revisions that we're asked to replay becomes empty. This case does not > make sense with git-replay(1), but with the new command it will. That reads better indeed, thanks! Patrick ``` ## Patrick Steinhardt, 2026-01-13 07:31 Subject: Re: [PATCH v10 1/8] builtin/replay: extract core logic to replay revisions Message-ID: URL: https://gitlist.dev/e/aWX0055xT2PKzgfa%40pks.im In-Reply-To: ``` On Mon, Jan 12, 2026 at 10:00:06PM -0800, Elijah Newren wrote: > On Mon, Jan 12, 2026 at 6:17 AM Patrick Steinhardt wrote: > > @@ -253,6 +254,137 @@ static struct commit *pick_regular_commit(struct repository *repo, > [...] > > + if (!result.clean) { > > + out->merge_conflict = true; > > + ret = -1; > > Even if you keep the special merge_conflict field for other purposes, > setting ret to -1 here still feels very wrong. Negative return codes, > and especially -1, is used throughout the merge machinery to signal > unexpected errors like failure to read/write to disk. Further, it's > inconsistent with how builtin/merge-tree.c works, where both in code > and in documentation merge result code is 0 == clean, 1 == merge > conflicts. I'm worried using -1 here could cause some nasty future > maintenance headaches trying to understand the field if left this way, > at least for me. As mentioned in the last round, the ret value here > should be 1. To expand a bit why I prefer my style: it's extremely easy to get the return code wrong. We have a bunch of callsites that set the return code directly, and in theory we now have to vet every single such call site to verify that it will never return an error code of 1. If any function does, then exiting with 1 now becomes ambiguous. This is an error waiting to happen. With being explicit about the error code and the specific condition we care about we make that issue go away. Furthermore, it also helps to document intent in the code as it is very explicit now. In any case, I don't want this disagreement to block the patch series, so I'll adapt to make it return a positive value on a conflict. > > @@ -306,21 +438,11 @@ int cmd_replay(int argc, > [...] > > - die_for_incompatible_opt2(!!advance_name_opt, "--advance", > > - contained, "--contained"); > > + die_for_incompatible_opt2(!!opts.advance, "--advance", > > + opts.contained, "--contained"); > > This predates your patch, but I'm wondering if there's anything we > should do to clarify and/or simplify the first check. The original > form of the check > + die_for_incompatible_opt2(!!opts.advance, "--advance", > + opts.contained, "--contained"); > was created because (a) I had code that allowed --onto to be implicit > in some cases, and (b) I was thinking only of --onto and --advance > modes. > > However: (a) we got rid of the implicit mode selection from my private > branch, and (b) Siddharth added patches which added a --revert mode. > Those patches caused confusion around the interplay of --contained > with the new mode > (https://lore.kernel.org/git/xmqq3460ocv7.fsf@gitster.g/). I thought > the synopsis: > "([--contained] --onto | --advance ) " > implied clearly enough that --contained is a sub-mode of --onto, but > apparently that wasn't the case. Perhaps we can strengthen that > understanding if we change the check here to instead be something like > if (opts.contained && !opts.onto) > die("--onto must be specified if --contained is") > > Definitely not critical; but might be a nice cleanup. > > > + die_for_incompatible_opt2(!!opts.advance, "--advance", > > + !!opts.onto, "--onto"); > > Yeah, and Siddharth can convert this to a die_for_incompatible_opt3() > call, adding "--revert" to it when he rerolls his series on top of > yours. Fair, but this feels somewhat unrelated to my changes. So I'll not touch this part and leave it for a future patch series to clean up, if that's alright with you. > > - /* Return */ > > - if (ret < 0) > > - exit(128); > > - return ret ? 0 : 1; > > + if (ret) { > > + if (result.merge_conflict) > > + return 1; > > + return 128; > > + } > > + > > + return 0; > > } > > You mentioned that you wanted to keep the merge_conflict field due to > some future patches beyond the currently submitted series. I wonder > if it'd make more sense to introduce that field once you introduce the > new patches, but i don't feel too strongly about that. I do feel > strongly that the place where you set ret to -1 is problematic and > should be changed to 1. I'll drop the field for now. Patrick ```