Re: [PATCH v4 12/12] builtin/history: implement "split" subcommand

[Patrick Steinhardt <ps@pks.im>]

On Tue, Oct 14, 2025 at 09:38:51AM -0400, Karthik Nayak wrote:
> 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.

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

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