From: D. Ben Knoble Date: Thu, 12 Feb 2026 22:20:13 GMT Subject: Re: [PATCH 3/4] builtin/history: replace "--ref-action=print" with "--dry-run" Message-ID: In-Reply-To: <20260212-b4-pks-history-dry-run-v1-3-1ce03d631c1b@pks.im> On Thu, Feb 12, 2026 at 7:45 AM Patrick Steinhardt wrote: > > The git-history(1) command has the ability to perform a dry-run > that will not end up modifying any references. Instead, we'll only print > any ref updates that would happen as a consequence of performing the > operation. > > This mode is somewhat hidden though behind the "--ref-action=print" > option. This command line option has its origin in git-replay(1), where > it's probably an okayish interface as this command is sitting more on > the plumbing side of tools. But git-history(1) is a user-facing tool, > and this way of achieving a dry-run is way too technical and thus not > very discoverable. > > Besides usability issues, it also has another issue: the dry-run mode > will always operate as if the user wanted to rewrite all branches. But > in fact, the user also has the option to only update the HEAD reference, > and they might want to perform a dry-run of such an operation, too. We > could of course introduce "--ref-actoin=print-head", but that would > become even less ergonomic. > > Replace "--ref-action=print" with a new "--dry-run" toggle. This new > toggle works with both "--ref-action={head,branches}" and is way more > discoverable. > > Add a test to verify that both "--ref-action=" values behave as > expected. > > Signed-off-by: Patrick Steinhardt > --- > Documentation/git-history.adoc | 12 ++-- > builtin/history.c | 150 ++++++++++++++++++++++------------------- > t/t3451-history-reword.sh | 12 +++- > 3 files changed, 96 insertions(+), 78 deletions(-) > > diff --git a/Documentation/git-history.adoc b/Documentation/git-history.adoc > index 154e262b76..33353815cb 100644 > --- a/Documentation/git-history.adoc > +++ b/Documentation/git-history.adoc > @@ -8,7 +8,7 @@ git-history - EXPERIMENTAL: Rewrite history > SYNOPSIS > -------- > [synopsis] > -git history reword [--ref-action=(branches|head|print)] > +git history reword [--dry-run] [--ref-action=(branches|head)] > > DESCRIPTION > ----------- > @@ -60,13 +60,15 @@ The following commands are available to rewrite history in different ways: > OPTIONS > ------- > > -`--ref-action=(branches|head|print)`:: > +`--dry-run`:: > + Do not update any references, but instead print any ref updates in a > + format that can be consumed by linkgit:git-update-ref[1]. > + > +`--ref-action=(branches|head)`:: > 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]. > + the current `HEAD` reference will be rewritten. Unrelated to this series, I've just realized I can't tell from these docs what the default --ref-action is. The code says "branches." > > GIT > --- > diff --git a/builtin/history.c b/builtin/history.c > index ca0cdb6a58..40e2925cea 100644 > --- a/builtin/history.c > +++ b/builtin/history.c > @@ -18,7 +18,7 @@ > #include "wt-status.h" > > #define GIT_HISTORY_REWORD_USAGE \ > - N_("git history reword [--ref-action=(branches|head|print)]") > + N_("git history reword [--dry-run] [--ref-action=(branches|head)]") > > static void change_data_free(void *util, const char *str UNUSED) > { > @@ -155,7 +155,6 @@ 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) > @@ -167,10 +166,8 @@ static int parse_ref_action(const struct option *opt, const char *value, int uns > *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'"), > + return error(_("%s expects one of 'branches' or 'head'"), > opt->long_name); > } > > @@ -287,11 +284,29 @@ static int setup_revwalk(struct repository *repo, > return ret; > } > > +static int handle_ref_update(struct ref_transaction *transaction, > + const char *refname, > + const struct object_id *new_oid, > + const struct object_id *old_oid, > + const char *reflog_msg, > + struct strbuf *err) > +{ > + if (!transaction) { > + printf("update %s %s %s\n", > + refname, oid_to_hex(new_oid), oid_to_hex(old_oid)); > + return 0; > + } > + > + return ref_transaction_update(transaction, refname, new_oid, old_oid, > + NULL, NULL, 0, reflog_msg, err); > +} > + > static int handle_reference_updates(struct rev_info *revs, > enum ref_action action, > struct commit *original, > struct commit *rewritten, > - const char *reflog_msg) > + const char *reflog_msg, > + int dry_run) > { > const struct name_decoration *decoration; > struct replay_revisions_options opts = { 0 }; > @@ -313,82 +328,72 @@ static int handle_reference_updates(struct rev_info *revs, > if (ret) > goto out; > > - switch (action) { > - case REF_ACTION_BRANCHES: > - case REF_ACTION_HEAD: > + if (action != REF_ACTION_BRANCHES && action != REF_ACTION_HEAD) > + BUG("unsupported ref action %d", action); > + > + if (!dry_run) { > transaction = ref_store_transaction_begin(get_main_ref_store(revs->repo), 0, &err); > if (!transaction) { > ret = error(_("failed to begin ref transaction: %s"), err.buf); > goto out; > } > + } It took me longer than I'd like to admit to realize that only initializing the transaction this way is safe, because we change the handler below: > > - 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; > - } > + for (size_t i = 0; i < result.updates_nr; i++) { > + ret = handle_ref_update(transaction, > + result.updates[i].refname, > + &result.updates[i].new_oid, > + &result.updates[i].old_oid, > + reflog_msg, &err); > + if (ret) { > + ret = error(_("failed to update ref '%s': %s"), > + result.updates[i].refname, err.buf); > + goto out; > } > + } To use our new "NULL-safe transaction update" function. Phew. Using "-b" with git-diff/show/etc. helps quite a bit. > + > + /* > + * `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; > > /* > - * `replay_revisions()` only updates references that are > - * ancestors of `rewritten`, so we need to manually > - * handle updating references that point to `original`. > + * 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. > */ > - 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); > + if (action == REF_ACTION_BRANCHES && > + decoration->type == DECORATION_REF_HEAD && > + !detached_head) > + continue; > + > + ret = handle_ref_update(transaction, > + decoration->name, > + &rewritten->object.oid, > + &original->object.oid, > + reflog_msg, &err); > + if (ret) { > + ret = error(_("failed to update ref '%s': %s"), > + decoration->name, 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); > + if (transaction && ref_transaction_commit(transaction, &err)) { > + ret = error(_("failed to commit ref transaction: %s"), err.buf); > + goto out; > } All effectively whitespace change, except for the new transaction check, which is sensible. > > ret = 0; > @@ -410,10 +415,13 @@ static int cmd_history_reword(int argc, > NULL, > }; > enum ref_action action = REF_ACTION_DEFAULT; > + int dry_run = 0; > struct option options[] = { > OPT_CALLBACK_F(0, "ref-action", &action, N_(""), > - N_("control ref update behavior (branches|head|print)"), > + N_("control ref update behavior (branches|head)"), > PARSE_OPT_NONEG, parse_ref_action), > + OPT_BOOL('n', "dry-run", &dry_run, > + N_("perform a dry-run without updating any refs")), > OPT_END(), > }; > struct strbuf reflog_msg = STRBUF_INIT; > @@ -450,7 +458,7 @@ static int cmd_history_reword(int argc, > strbuf_addf(&reflog_msg, "reword: updating %s", argv[0]); > > ret = handle_reference_updates(&revs, action, original, rewritten, > - reflog_msg.buf); > + reflog_msg.buf, dry_run); > if (ret < 0) { > ret = error(_("failed replaying descendants")); > goto out; > diff --git a/t/t3451-history-reword.sh b/t/t3451-history-reword.sh > index 12a9a7d051..702d40dc06 100755 > --- a/t/t3451-history-reword.sh > +++ b/t/t3451-history-reword.sh > @@ -221,7 +221,7 @@ test_expect_success 'can reword a merge commit' ' > ) > ' > > -test_expect_success '--ref-action=print prints ref updates without modifying repo' ' > +test_expect_success '--dry-run prints ref updates without modifying repo' ' > test_when_finished "rm -rf repo" && > git init repo --initial-branch=main && > ( > @@ -233,7 +233,15 @@ test_expect_success '--ref-action=print prints ref updates without modifying rep > test_commit theirs && > > git refs list >refs-expect && > - reword_with_message --ref-action=print base >updates <<-\EOF && > + reword_with_message --dry-run --ref-action=head 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 && > + > + reword_with_message --dry-run base >updates <<-\EOF && > reworded commit > EOF > git refs list >refs-actual && > > -- > 2.53.0.295.g64333814d3.dirty > > -- D. Ben Knoble