From: Patrick Steinhardt Date: Tue, 02 Dec 2025 18:51:04 GMT Subject: Re: [PATCH v6 11/11] builtin/history: implement "split" subcommand Message-ID: In-Reply-To: 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. > ) 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