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

> 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`)?

> > +
> > +       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.

> > +       }
> > +
> > +       /*
> > +        * 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...

> > +
> > +       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.

> > +       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.)

> > +       /* 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?