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

From: Phillip Wood <[email-masked]>
Date: Fri, 21 Nov 2025 14:31:14 +0000

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. 
<[email-masked]>) 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.

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?

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

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

> diff --git a/t/t3452-history-split.sh b/t/t3452-history-split.sh
> [...]
> +test_expect_success 'refuses to work with merge commits' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_commit base &&
> +		git branch branch &&
> +		test_commit ours &&
> +		git switch branch &&
> +		test_commit theirs &&
> +		git switch - &&
> +		git merge theirs &&
> +		test_must_fail git history split HEAD 2>err &&
> +		test_grep "cannot rearrange commit history with merges" err &&
> +		test_must_fail git history split HEAD~ 2>err &&
> +		test_grep "cannot rearrange commit history with merges" err
> +	)
> +'

My comments from the reword tests apply here as well.

Thanks

Phillip

> +
> +test_expect_success 'refuses to work with unrelated commits' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_commit base &&
> +		git branch branch &&
> +		test_commit ours &&
> +		git switch branch &&
> +		test_commit theirs &&
> +		test_must_fail git history split ours 2>err &&
> +		test_grep "split commit must be reachable from current HEAD commit" err
> +	)
> +'
> +
> +test_expect_success 'can split up tip commit' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_commit initial &&
> +		touch bar foo &&
> +		git add . &&
> +		git commit -m split-me &&
> +
> +		git symbolic-ref HEAD >expect &&
> +		set_fake_editor "split-out commit" &&
> +		git history split HEAD <<-EOF &&
> +		y
> +		n
> +		EOF
> +		git symbolic-ref HEAD >actual &&
> +		test_cmp expect actual &&
> +
> +		expect_log <<-EOF &&
> +		split-me
> +		split-out commit
> +		initial
> +		EOF
> +
> +		expect_tree_entries HEAD~ <<-EOF &&
> +		bar
> +		initial.t
> +		EOF
> +
> +		expect_tree_entries HEAD <<-EOF
> +		bar
> +		foo
> +		initial.t
> +		EOF
> +	)
> +'
> +
> +test_expect_success 'can split up root commit' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		touch bar foo &&
> +		git add . &&
> +		git commit -m root &&
> +		test_commit tip &&
> +
> +		set_fake_editor "split-out commit" &&
> +		git history split HEAD~ <<-EOF &&
> +		y
> +		n
> +		EOF
> +
> +		expect_log <<-EOF &&
> +		tip
> +		root
> +		split-out commit
> +		EOF
> +
> +		expect_tree_entries HEAD~2 <<-EOF &&
> +		bar
> +		EOF
> +
> +		expect_tree_entries HEAD~ <<-EOF &&
> +		bar
> +		foo
> +		EOF
> +
> +		expect_tree_entries HEAD <<-EOF
> +		bar
> +		foo
> +		tip.t
> +		EOF
> +	)
> +'
> +
> +test_expect_success 'can split up in-between commit' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_commit initial &&
> +		touch bar foo &&
> +		git add . &&
> +		git commit -m split-me &&
> +		test_commit tip &&
> +
> +		set_fake_editor "split-out commit" &&
> +		git history split HEAD~ <<-EOF &&
> +		y
> +		n
> +		EOF
> +
> +		expect_log <<-EOF &&
> +		tip
> +		split-me
> +		split-out commit
> +		initial
> +		EOF
> +
> +		expect_tree_entries HEAD~2 <<-EOF &&
> +		bar
> +		initial.t
> +		EOF
> +
> +		expect_tree_entries HEAD~ <<-EOF &&
> +		bar
> +		foo
> +		initial.t
> +		EOF
> +
> +		expect_tree_entries HEAD <<-EOF
> +		bar
> +		foo
> +		initial.t
> +		tip.t
> +		EOF
> +	)
> +'
> +
> +test_expect_success 'can pick multiple hunks' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		touch bar baz foo qux &&
> +		git add . &&
> +		git commit -m split-me &&
> +
> +		set_fake_editor "split-out-commit" &&
> +		git history split HEAD <<-EOF &&
> +		y
> +		n
> +		y
> +		n
> +		EOF
> +
> +		expect_tree_entries HEAD~ <<-EOF &&
> +		bar
> +		foo
> +		EOF
> +
> +		expect_tree_entries HEAD <<-EOF
> +		bar
> +		baz
> +		foo
> +		qux
> +		EOF
> +	)
> +'
> +
> +
> +test_expect_success 'can use only last hunk' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		touch bar foo &&
> +		git add . &&
> +		git commit -m split-me &&
> +
> +		set_fake_editor "split-out commit" &&
> +		git history split HEAD <<-EOF &&
> +		n
> +		y
> +		EOF
> +
> +		expect_log <<-EOF &&
> +		split-me
> +		split-out commit
> +		EOF
> +
> +		expect_tree_entries HEAD~ <<-EOF &&
> +		foo
> +		EOF
> +
> +		expect_tree_entries HEAD <<-EOF
> +		bar
> +		foo
> +		EOF
> +	)
> +'
> +
> +test_expect_success 'aborts with empty commit message' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		touch bar foo &&
> +		git add . &&
> +		git commit -m split-me &&
> +
> +		set_fake_editor "" &&
> +		test_must_fail git history split HEAD <<-EOF 2>err &&
> +		y
> +		n
> +		EOF
> +		test_grep "Aborting commit due to empty commit message." err
> +	)
> +'
> +
> +test_expect_success 'commit message editor sees split-out changes' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		touch bar foo &&
> +		git add . &&
> +		git commit -m split-me &&
> +
> +		write_script fake-editor.sh <<-\EOF &&
> +		cp "$1" . &&
> +		echo "some commit message" >>"$1"
> +		EOF
> +		test_set_editor "$(pwd)"/fake-editor.sh &&
> +
> +		git history split HEAD <<-EOF &&
> +		y
> +		n
> +		EOF
> +
> +		cat >expect <<-EOF &&
> +
> +		# Please enter the commit message for the split-out changes. Lines starting
> +		# with ${SQ}#${SQ} will be ignored.
> +		# Changes to be committed:
> +		#	new file:   bar
> +		#
> +		EOF
> +		test_cmp expect COMMIT_EDITMSG &&
> +
> +		expect_log <<-EOF
> +		split-me
> +		some commit message
> +		EOF
> +	)
> +'
> +
> +test_expect_success 'can use pathspec to limit what gets split' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		touch bar foo &&
> +		git add . &&
> +		git commit -m split-me &&
> +
> +		set_fake_editor "split-out commit" &&
> +		git history split HEAD -- foo <<-EOF &&
> +		y
> +		EOF
> +
> +		expect_tree_entries HEAD~ <<-EOF &&
> +		foo
> +		EOF
> +
> +		expect_tree_entries HEAD <<-EOF
> +		bar
> +		foo
> +		EOF
> +	)
> +'
> +
> +test_expect_success 'refuses to create empty split-out commit' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_commit base &&
> +		touch bar foo &&
> +		git add . &&
> +		git commit -m split-me &&
> +
> +		test_must_fail git history split HEAD 2>err <<-EOF &&
> +		n
> +		n
> +		EOF
> +		test_grep "split commit is empty" err
> +	)
> +'
> +
> +test_expect_success 'hooks are executed for rewritten commits' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		touch bar foo &&
> +		git add . &&
> +		git commit -m split-me &&
> +		old_head=$(git rev-parse HEAD) &&
> +
> +		write_script .git/hooks/prepare-commit-msg <<-EOF &&
> +		touch "$(pwd)/hooks.log"
> +		EOF
> +		write_script .git/hooks/post-commit <<-EOF &&
> +		touch "$(pwd)/hooks.log"
> +		EOF
> +		write_script .git/hooks/post-rewrite <<-EOF &&
> +		touch "$(pwd)/hooks.log"
> +		EOF
> +
> +		set_fake_editor "split-out commit" &&
> +		git history split HEAD <<-EOF &&
> +		y
> +		n
> +		EOF
> +
> +		expect_log <<-EOF &&
> +		split-me
> +		split-out commit
> +		EOF
> +
> +		test_path_is_missing hooks.log
> +	)
> +'
> +
> +test_expect_success 'refuses to create empty original commit' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		touch bar foo &&
> +		git add . &&
> +		git commit -m split-me &&
> +
> +		test_must_fail git history split HEAD 2>err <<-EOF &&
> +		y
> +		y
> +		EOF
> +		test_grep "split commit tree matches original commit" err
> +	)
> +'
> +
> +test_expect_success 'retains changes in the worktree and index' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		echo a >a &&
> +		echo b >b &&
> +		git add . &&
> +		git commit -m "initial commit" &&
> +		echo a-modified >a &&
> +		echo b-modified >b &&
> +		git add b &&
> +		set_fake_editor "a-only" &&
> +		git history split HEAD <<-EOF &&
> +		y
> +		n
> +		EOF
> +
> +		expect_tree_entries HEAD~ <<-EOF &&
> +		a
> +		EOF
> +		expect_tree_entries HEAD <<-EOF &&
> +		a
> +		b
> +		EOF
> +
> +		cat >expect <<-\EOF &&
> +		 M a
> +		M  b
> +		?? actual
> +		?? expect
> +		?? fake-editor.sh
> +		EOF
> +		git status --porcelain >actual &&
> +		test_cmp expect actual
> +	)
> +'
> +
> +test_done
>