Re: [PATCH v6 05/11] builtin/history: implement "reword" subcommand

[Patrick Steinhardt <ps@pks.im>]

On Wed, Nov 19, 2025 at 11:03:20PM -0800, Elijah Newren wrote:
> Phillip responded in good detail, but I wanted to comment on a few
> additional things...
> 
> On Mon, Oct 27, 2025 at 4:34 AM Patrick Steinhardt <ps@pks.im> wrote:
> 
> > +static int collect_commits(struct repository *repo,
> > +                          struct commit *old_commit,
> > +                          struct commit *new_commit,
> > +                          struct strvec *out)
> > +{
> > +       struct setup_revision_opt revision_opts = {
> > +               .assume_dashdash = 1,
> > +       };
> > +       struct strvec revisions = STRVEC_INIT;
> > +       struct commit *child;
> > +       struct rev_info rev = { 0 };
> > +       int ret;
> > +
> > +       repo_init_revisions(repo, &rev, NULL);
> > +       strvec_push(&revisions, "");
> > +       strvec_push(&revisions, oid_to_hex(&new_commit->object.oid));
> > +       if (old_commit)
> > +               strvec_pushf(&revisions, "^%s", oid_to_hex(&old_commit->object.oid));
> > +
> > +       setup_revisions_from_strvec(&revisions, &rev, &revision_opts);
> > +       if (revisions.nr != 1 || prepare_revision_walk(&rev)) {
> > +               ret = error(_("revision walk setup failed"));
> > +               goto out;
> > +       }
> 
> Don't we want to restrict the revision walk to descendants of
> old_commit (which can be done with `--ancestry-path`)?

We verify that both commits have direct ancestry and that there are no
merge commits in the history, so this shouldn't be needed. But indeed,
this makes the logic a bit easier to reason about.

> > +
> > +       while ((child = get_revision(&rev))) {
> > +               if (old_commit && !child->parents)
> > +                       BUG("revision walk did not find child commit");
> > +               if (child->parents && child->parents->next) {
> > +                       ret = error(_("cannot rearrange commit history with merges"));
> > +                       goto out;
> > +               }
> > +
> > +               strvec_push(out, oid_to_hex(&child->object.oid));
> > +
> > +               if (child->parents && old_commit &&
> > +                   commit_list_contains(old_commit, child->parents))
> > +                       break;
> 
> Is this last if-check basically a workaround to not providing
> --ancestry-path to the revision walk?  And won't it sometimes still
> get non-descendants of old_commit before reaching old_commit?  Or, I
> guess that's not an issue since you error out when you hit merges, but
> once replay supports merges, there's more logic that needs changing
> than one expects with the way this is coded.

Yeah, this is not currently an issue as we explicitly rule out merges.
Anyway, I'm using the flag now, so this isn't needed anymore.

> > +       }
> > +
> > +       /*
> > +        * Revisions are in newest-order-first. We have to reverse the
> > +        * array though so that we pick the oldest commits first.
> > +        */
> > +       for (size_t i = 0, j = out->nr - 1; i < j; i++, j--)
> > +               SWAP(out->v[i], out->v[j]);
> 
> Setting rev.reverse would obviate the need for this...

Yup, true. I couldn't use 'reverse' before due to the way the loop was
handled.

> > +
> > +       ret = 0;
> > +
> > +out:
> > +       strvec_clear(&revisions);
> > +       release_revisions(&rev);
> > +       reset_revision_walk();
> > +       return ret;
> > +}
> 
> You've pulled out some functions from builtin/replay, but you've
> decided to hand re-roll all the revision walking.  Is that because you
> first implemented on top of sequencer, and then transliterated to
> replay?  If so, I think we could restructure this; I think what you
> need is:
>   * Create a new commit with an altered commit message.
>   * Invoking whatever function(s) would be invoked by "git replay
> --onto ${NEW_COMMIT_ID} --ancestry-path ^${OLD_COMMIT_ID} --branches"
> (or as a first cut, even shelling out to that subprocess).
> 
> The first bullet point would be your fill_commit_message().
> 
> The second bullet point would allow you to perhaps drop your
> collect_commits(), replace_commits(), and apply_commits(), which feel
> like they are just re-implementing replay logic, and replace them with
> something like:
> 
> void replay_descendants(struct repository *repo,
>                         const struct object_id *prev_head,
>                         const struct object_id *new_head)
> {
>         struct strvec args = STRVEC_INIT;
> 
>         strvec_pushl(&args, "replay", "--onto", NULL);
>         strvec_push(&args, oid_to_hex(new_head));
>         strvec_push(&args, "--ancestry-path");
>         strvec_pushf(&args, "^%s", oid_to_hex(prev_head));
>         strvec_push(&args, "--branches");
> 
>         reset_revision_walk();
>         cmd_replay(args.nr, args.v, NULL, repo);
> }
> 
> ...although maybe it's a little ugly to invoke cmd_replay() this way
> and maybe we want to restructure that out.
> 
> But, I am really late in providing my review, so if you want to go
> forward with your existing three functions and then perhaps we
> restructure later, that's fine too.  The command is experimental,
> after all.

I guess it's a combination of the transliteration and that I couldn't
figure out how to easily do some things without shelling out. I plan on
introducing features eventually that also allow for example to reorder
commits, and I'm not clear that this is easy to do with the replay
infra.

So for now I think I'd like to retain the current infra. But I certainly
agree that we should revisit and see whether we can further refactor the
interfaces provided by "replay.c" to cover more cases without shelling
out.

> > +       head = lookup_commit_reference_by_name("HEAD");
> > +       if (!head) {
> > +               ret = error(_("could not resolve HEAD to a commit"));
> > +               goto out;
> > +       }
> > +
> > +       commit_list_append(original_commit, &from_list);
> > +       if (!repo_is_descendant_of(repo, head, from_list)) {
> > +               ret = error (_("split commit must be reachable from current HEAD commit"));
> > +               goto out;
> > +       }
> 
> Why should it be required to be reachable from HEAD?  Shouldn't it be
> possible to reword a commit from another branch?
> 
> Also, what about when a commit is reachable from both HEAD and other
> branches?  I know you started by basing on git-rebase, and git-rebase
> restricts things to just one branch, but that was perhaps its biggest
> design flaw that couldn't be backward compatibly fixed without
> creating a new command.  I'd rather avoid copying that flaw.  (Maybe
> the user needs an error by default if more than one branch is
> affected, or they need to provide an additional flag to rewrite
> multiple branches, but only rewriting one branch when more than one is
> affected is just wrong to me unless the user explicitly specifies
> that's what they want.)

For now we the commands really only care about a single branch, the case
where a commit exists on multiple branches is not considered. I'm not
really sure whether I'd call this a flaw -- I think it's as easy way to
think about the command for the user.

That being said, I certainly think that we can eventually introduce a
flag to alter the behaviour so that it considers multiple branches in
case the commit exists on more than one branch.

> > +       /* We retain authorship of the original commit. */
> > +       original_message = repo_logmsg_reencode(repo, original_commit, 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_commit_tree_oid,
> > +                                 original_body, "reworded", &final_message);
> > +       if (ret < 0)
> > +               goto out;
> > +
> > +       ret = commit_tree(final_message.buf, final_message.len, &original_commit_tree_oid,
> > +                         original_commit->parents, &rewritten_commit, original_author, NULL);
> 
> Does the use of commit_tree() instead of commit_tree_extended() mean
> you discard additional headers on the reworded commit, such as
> encoding?

Good point, let me fix this.

Patrick