From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 10 Dec 2025 09:51:33 GMT
Subject: Re: [PATCH v6 11/11] builtin/history: implement "split" subcommand
Message-ID: <48ba9303-45f4-43bf-a257-10d58474096c@gmail.com>
In-Reply-To: <aS81GAuKV79x_V2K@pks.im>

On 02/12/2025 18:51, Patrick Steinhardt wrote:
> On Fri, Nov 21, 2025 at 02:31:14PM +0000, Phillip Wood wrote:
>> On 27/10/2025 11:33, Patrick Steinhardt wrote:
>>> +	* 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.

Oh I see, but why does that mean we need to fork a subprocess rather 
than writing the index to disc in this process?

Thanks

Phillip

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