Re: [PATCH v8 7/7] builtin/history: implement "reword" subcommand

[Patrick Steinhardt <ps@pks.im>]

On Fri, Jan 09, 2026 at 05:20:04PM -0800, Elijah Newren wrote:
> On Wed, Jan 7, 2026 at 2:10 AM Patrick Steinhardt <ps@pks.im> 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 [<options>]
> > +git history reword <commit> [--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 <commit>`::
> > +       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