From: Patrick Steinhardt Date: Mon, 12 Jan 2026 13:03:31 GMT Subject: Re: [PATCH v8 7/7] builtin/history: implement "reword" subcommand Message-ID: In-Reply-To: On Fri, Jan 09, 2026 at 05:20:04PM -0800, Elijah Newren wrote: > On Wed, Jan 7, 2026 at 2:10 AM Patrick Steinhardt wrote: > > diff --git a/Documentation/git-history.adoc b/Documentation/git-history.adoc > > index 5a9d931efc..4eea317e5c 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 > > ----------- > > @@ -32,8 +32,9 @@ 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. > > Ah, was the previous sentence here from the former patch just a bad > splitting when you were rewriting? Dunno what happened here, to be honest. > > LIMITATIONS > > ----------- > > @@ -51,6 +52,22 @@ COMMANDS > > > > Several 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. > > One isn't exactly "several"; I know you'll add more later, but since > this series ends here, should that word be changed? We can just say "The following commands", there is no need to be specific. > > + > > +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 > > + decendants of the original commit will be rewritten. With `head`, only > > decendants -> descendants . Or maybe double down on the typo and > extend it a bit into either 'decedent' or 'decadent'. That could be > fun. Thanks, this made me laugh. :) > > diff --git a/builtin/history.c b/builtin/history.c > > index f6fe32610b..59011ea517 100644 > > --- a/builtin/history.c > > +++ b/builtin/history.c > > @@ -1,22 +1,404 @@ [snip] > > +static int commit_tree_with_edited_message(struct repository *repo, > > + const char *action, > > + struct commit *original, > > + struct commit **out) > > +{ > > + const char *exclude_gpgsig[] = { "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); > > Does this grab encoding? If so, should it be excluded as well given > the repo_logmsg_reencode() call? Hm, good question indeed. I think that makes sense. [snip] > > + } else { > > + strvec_push(&args, "--branches"); > > + } > > + > > + 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(repo, &revs, &opts, &updates); > > + if (ret) > > + goto out; > > + > > + switch (action) { > > + case REF_ACTION_DEFAULT: > > + case REF_ACTION_BRANCHES: > > + 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 < updates.nr; i++) { > > + ret = ref_transaction_update(transaction, > > + updates.items[i].refname, > > + &updates.items[i].new_oid, > > + &updates.items[i].old_oid, > > + NULL, NULL, 0, reflog_msg, &err); > > + if (ret) { > > + ret = error(_("failed to update ref '%s': %s"), > > + updates.items[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`. > > + */ > > This is a good catch; I was wondering if there was a way to put this > logic into replay_revisions() so that other callers need not duplicate > it, but since it just takes the revisions to walk over and that list > is empty, it'd somehow need to know about original->object.oid; it > doesn't have that info. Hmmm... Yeah, exactly. I was also thinking about whether this can be part of git-replay(1), but ultimately it didn't really seem to make sense. After all this is about a commit that we're _not_ replaying at all, but that we have manually edited. > > diff --git a/replay.c b/replay.c > > index 8c2f2d3710..5203f9db4c 100644 > > --- a/replay.c > > +++ b/replay.c > > @@ -254,7 +254,9 @@ 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, > > + }; > > Wait, what? Why is this being initialized this way? > > Same as I said over in > https://lore.kernel.org/git/CABPp-BEh7VEM6UQjkK3CxJcv54vEmueTmh9+-SyTKUxgy7Mkcg@mail.gmail.com/, > why is this change here? Was this due to hitting an empty range? Yeah, exactly. > Actually, while supporting empty ranges didn't make sense back when I > mentioned it to Siddharth (because users always specified the ranges), > I think it actually does make sense now that ranges are implicit. > Someone could use "git history reword HEAD" (even if "git commit > --amend" already exists), and that'd result in an empty range. So, I > think the change makes sense now, but I think this particular change > really ought to be documented and motivated in a separate commit > message rather than lumped in with the other changes in this commit. That's fair, will do. Thanks! Patrick