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

[Patrick Steinhardt <ps@pks.im>]

On Fri, Nov 21, 2025 at 02:31:14PM +0000, Phillip Wood wrote:
> Hi Patrick
> 
> On 27/10/2025 11:33, Patrick Steinhardt wrote:
> > It is quite a common use case that one wants to split up one commit into
> > multiple commits by moving parts of the changes of the original commit
> > out into a separate commit. This is quite an involved operation though:
> > 
> >    1. Identify the commit in question that is to be dropped.
> > 
> >    2. Perform an interactive rebase on top of that commit's parent.
> > 
> >    3. Modify the instruction sheet to "edit" the commit that is to be
> >       split up.
> > 
> >    4. Drop the commit via "git reset HEAD~".
> > 
> >    5. Stage changes that should go into the first commit and commit it.
> > 
> >    6. Stage changes that should go into the second commit and commit it.
> > 
> >    7. Finalize the rebase.
> > 
> > This is quite complex, and overall I would claim that most people who
> > are not experts in Git would struggle with this flow.
> 
> If they want to test the split commit it's even more complicated because
> they need to stash the unstaged changes. We should think about how we can
> add support for testing split commits to this command in the future.
> 
> > Introduce a new "split" subcommand for git-history(1) to make this way
> > easier. All the user needs to do is to say `git history split $COMMIT`.
> >  From hereon, Git asks the user which parts of the commit shall be moved
> > out into a separate commit and, once done, asks the user for the commit
> > message. Git then creates that split-out commit and applies the original
> > commit on top of it.
> 
> As others have said (and I thought we'd agreed c.f.
> <aMfdR3JE4zq-2j9b@pks.im>) I think it would be better to prompt the user to
> edit the existing commit message when creating both commits. Elsewhere
> Elijah mention being able to split a commit into more than two commits. I
> wonder if we could loop running run_add_p_index() and committing the result
> until there are no more changes left. It does mean that the user has to
> actively select changes for the final commit though which might be annoying.
> We can always play with that later.

Ah, right. Changing this now to prompt for both commit messages.

> Looking below I do wonder if we can share more code between subcommands when
> it comes to checking the commit we're given on the command line and
> re-creating a commit and having the user edit the message.
> 
> > +static int split_commit(struct repository *repo,
> > +			struct commit *original_commit,
> > +			struct pathspec *pathspec,
> > +			struct object_id *out)
> > {
> > [...]> +	/*
> > +	* Construct the first commit. This is done by taking the original
> > +	* commit parent's tree and selectively patching changes from the diff
> > +	* between that parent and its child.
> > +	*/
> > +	repo_git_path_replace(repo, &index_file, "%s", "history-split.index");
> > +
> > +	read_tree_cmd.git_cmd = 1;
> > +	strvec_pushf(&read_tree_cmd.env, "GIT_INDEX_FILE=%s", index_file.buf);
> > +	strvec_push(&read_tree_cmd.args, "read-tree");
> > +	strvec_push(&read_tree_cmd.args, oid_to_hex(&parent_tree_oid));
> > +	ret = run_command(&read_tree_cmd);
> 
> Why do we need to fork "read-tree" here rather than call unpack_trees()
> ourselves?

This is an artifact of how the `run_add_p()` interfaces work. They
unfortunately do not work on top of an in-memory index, but they work on
an on-disk index.

> > [...]
> > +	/* 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);
> > +
> > +	ret = fill_commit_message(repo, &parent_tree_oid, &split_tree->object.oid,
> > +				  "", "split-out", &split_message);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	ret = commit_tree(split_message.buf, split_message.len, &split_tree->object.oid,
> > +			  original_commit->parents, &out[0], original_author, NULL);
> > +	if (ret < 0) {
> > +		ret = error(_("failed writing split-out commit"));
> > +		goto out;
> > +	}
> 
> Don't we have the same code for rewording a commit, maybe we should package
> this up into a shared helper function.

Hm, indeed, there's a bit of non-trivial logic here. I'll refactor this.

> > +static int cmd_history_split(int argc,
> > +			     const char **argv,
> > +			     const char *prefix,
> > +			     struct repository *repo)
> > +{
> > +	const char * const usage[] = {
> > +		GIT_HISTORY_SPLIT_USAGE,
> > +		NULL,
> > +	};
> > +	struct option options[] = {
> > +		OPT_END(),
> > +	};
> > +	struct oidmap rewritten_commits = OIDMAP_INIT;
> > +	struct commit *original_commit, *parent, *head;
> > +	struct strvec commits = STRVEC_INIT;
> > +	struct commit_list *from_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;
> > +	}
> > +
> > +	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, &from_list);
> > +	if (!repo_is_descendant_of(repo, head, from_list)) {
> > +		ret = error(_("split commit must be reachable from current HEAD commit"));
> > +		goto out;
> > +	}
> 
> This is very similar to cmd_history_reword() up to this point. When we add
> the "drop" and "amend" subcommands they're going to want to do the same
> checks.
> 
> > +	parse_pathspec(&pathspec, 0,
> > +		PATHSPEC_PREFER_FULL | PATHSPEC_SYMLINK_LEADING_PATH | PATHSPEC_PREFIX_ORIGIN,
> > +		prefix, argv + 1);
> 
> This and calling split_commit() below are the only real differences with
> cmd_history_reword(), is it worth trying to share some more code between the
> two?

Yup, done.

Thanks!

Patrick