Re: [PATCH v4 12/12] builtin/history: implement "split" subcommand
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Oct 21, 2025, 11:44 UTC
- Message-ID
- <aPdyBpuLfuYOLe7q@pks.im>
- In-Reply-To
- <CAOLa=ZTnsHxK2+rRsAErY_zc1Rg5WNrmcDBiPgDk38zb9TNfkQ@mail.gmail.com>
On Tue, Oct 14, 2025 at 09:38:51AM -0400, Karthik Nayak wrote:
Show 40 quoted lines
> Patrick Steinhardt <ps@pks.im> writes: > > diff --git a/Documentation/git-history.adoc b/Documentation/git-history.adoc > > index b55babe206..83d675afea 100644 > > --- a/Documentation/git-history.adoc > > +++ b/Documentation/git-history.adoc > > @@ -40,6 +41,26 @@ rewrite history in different ways: > > provided, then this command will spawn an editor with the current > > message of that commit. > > > > +`split [--message=<message>] <commit> [--] [<pathspec>...]`:: > > + Interactively split up <commit> into two commits by choosing > > + hunks introduced by it that will be moved into the new split-out > > + commit. These hunks will then be written into a new commit that > > + becomes the parent of the previous commit. The original commit > > + stays intact, except that its parent will be the newly split-out > > + commit. > > > > So in essence we do this: > > Before split: > P1 ── C0 ── C1 ── ... ── CN > └─(target) └─(HEAD) > > After split: > P1 ── S0 ── C0' ── C1 ── ...... ── CN > │ └─(modified original) └─(HEAD) > └─(split-out hunks) > > I do wonder if S0 should contain the existing message and the new > message should go to C0'. So perhaps more like > > After split: > P1 ── C0' ── S0 ── C1 ── ..... ── CN > │ └─(split-out hunks) └─(HEAD) > └─(modified original) > > Mostly because when you say split, I would assume we keep the original > as is and add on top of it. I don't really have a strong argument though > :)
Yeah, this has already caused some discussion beforehand. I guess you can argue either way, and the suggestion from others was to simply allow the user to edit both commit messages.
I don't at all mind going into that direction, but I wonder how to call the "--message" switch in that case. We could of course just call these "--first-message" and "--second-message", but that feels somewhat awkward.
Also, I already have it in my mind that it would be cool to extend this command so that you can split into arbitrary many commits. That is, after you have split out the first commit we simply go back into interactive mode to create a second commit tree. Rinse and repeat until we have no chunks left anymore. But if we had such a mode though, then numbered parameters don't make much sense anymore.
An alternative could be to just accept multiple "-m" arguments, and we then apply the messages to the respective commits? Dunno.
Show 32 quoted lines
> > + const char *commit_message = NULL;
> > + struct option options[] = {
> > + OPT_STRING('m', "message", &commit_message, N_("message"), N_("commit message")),
> > + OPT_END(),
> > + };
> > + struct oidmap rewritten_commits = OIDMAP_INIT;
> > + struct commit *original_commit, *parent, *head;
> > + struct strvec commits = STRVEC_INIT;
> > + struct commit_list *list = NULL;
> > + struct object_id split_commits[2];
> > + struct pathspec pathspec = { 0 };
> > + int ret;
> > +
> > + argc = parse_options(argc, argv, prefix, options, usage, 0);
> > + if (argc < 1) {
> > + ret = error(_("command expects a revision"));
> > + goto out;
> > + }
> > + repo_config(repo, git_default_config, NULL);
> > +
> > + original_commit = lookup_commit_reference_by_name(argv[0]);
> > + if (!original_commit) {
> > + ret = error(_("commit to be split cannot be found: %s"), argv[0]);
> > + goto out;
> > + }
> > +
> > + if (original_commit->parents && original_commit->parents->next) {
> > + ret = error(_("commit to be split must not be a merge commit"));
> > + goto out;
> > + }
>
> Do we need this? Since we also check for merges in `collect_commits()` below.Yeah, we don't indeed.
Show 24 quoted lines
> > + parent = original_commit->parents ? original_commit->parents->item : NULL;
> > + if (parent && repo_parse_commit(repo, parent)) {
> > + ret = error(_("unable to parse commit %s"),
> > + oid_to_hex(&parent->object.oid));
> > + goto 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, &list);
> > + if (!repo_is_descendant_of(repo, original_commit, list)) {
> > + ret = error (_("split commit must be reachable from current HEAD commit"));
>
> s/error /error/
>
> > + goto out;
> > + }
> > +
>
> This is also checked within collect_commits(), no?The problem is rather that the check is wrong: we check whether the original commit is a descendent of itself, which is always true. What we actually want to check though is whether HEAD is a descendant of the original commit.
The check in `collect_commits()` is slightly different, as we verify that the range provided to us is actually the same. But that check shouldn't ever hit if the above check actually triggers.
I'll fix the check here and drop the one in `collect_commits()`.
Patrick