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

From: Elijah Newren <newren@gmail.com>
Date: Nov 20, 2025, 07:03 UTC
Message-ID: <CABPp-BEm1QBP+CuSOn5FaE3XJVFg+Qbfzdp560u00ZERbNm6qQ@mail.gmail.com>
In-Reply-To: <20251027-b4-pks-history-builtin-v6-5-407dd3f57ad3@pks.im>

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:

Show 24 quoted lines

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

Show 14 quoted lines

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

Show 8 quoted lines

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

Show 9 quoted lines

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

Show 11 quoted lines

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

Show 14 quoted lines

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