From: Karthik Nayak Date: Tue, 14 Oct 2025 13:38:51 GMT Subject: Re: [PATCH v4 12/12] builtin/history: implement "split" subcommand Message-ID: In-Reply-To: <20251001-b4-pks-history-builtin-v4-12-8e61ddb86317@pks.im> Patrick Steinhardt writes: > 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. > > 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. > > Signed-off-by: Patrick Steinhardt > --- > Documentation/git-history.adoc | 62 ++++++ > builtin/history.c | 225 +++++++++++++++++++++ > t/meson.build | 1 + > t/t3452-history-split.sh | 432 +++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 720 insertions(+) > > 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 > @@ -10,6 +10,7 @@ SYNOPSIS > [synopsis] > git history [] > git history reword [] > +git history split [] [--] [...] > > DESCRIPTION > ----------- > @@ -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=] [--] [...]`:: > + Interactively split up 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 :) [snip] > +EXAMPLES > +-------- > + > +Split a commit > +~~~~~~~~~~~~~~ > + > +---------- > +$ git log --stat --oneline > +3f81232 (HEAD -> main) original > + bar | 1 + > + foo | 1 + > + 2 files changed, 2 insertions(+) > + > +$ git history split HEAD --message="split-out commit" > +diff --git a/bar b/bar > +new file mode 100644 > +index 0000000..5716ca5 > +--- /dev/null > ++++ b/bar > +@@ -0,0 +1 @@ > ++bar > +(1/1) Stage addition [y,n,q,a,d,e,p,?]? y > + > +diff --git a/foo b/foo > +new file mode 100644 > +index 0000000..257cc56 > +--- /dev/null > ++++ b/foo > +@@ -0,0 +1 @@ > ++foo > +(1/1) Stage addition [y,n,q,a,d,e,p,?]? n > + > +$ git log --stat --oneline > +7cebe64 (HEAD -> main) original > + foo | 1 + > + 1 file changed, 1 insertion(+) > +d1582f3 split-out commit > + bar | 1 + > + 1 file changed, 1 insertion(+) > +---------- > + It's really nice to have examples. [snip] > + > +static int cmd_history_split(int argc, > + const char **argv, > + const char *prefix, > + struct repository *repo) > +{ > + const char * const usage[] = { > + N_("git history split [] "), > + NULL, > + }; should be '*const' here. > + 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. > + 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? > + parse_pathspec(&pathspec, 0, > + PATHSPEC_PREFER_FULL | PATHSPEC_SYMLINK_LEADING_PATH | PATHSPEC_PREFIX_ORIGIN, > + prefix, argv + 1); > + > + /* > + * Collect the list of commits that we'll have to reapply now already. > + * This ensures that we'll abort early on in case the range of commits > + * contains merges, which we do not yet handle. > + */ Comment spacing is off here. > + ret = collect_commits(repo, parent, head, &commits); > + if (ret < 0) > + goto out; > + > + /* > + * Then we split up the commit and replace the original commit with the > + * new new ones. > + */ s/new/new > + ret = split_commit(repo, original_commit, &pathspec, > + commit_message, split_commits); > + if (ret < 0) > + goto out; > + This one was straight forward, it handles adding the hunks to the index creating the two commits and linking them. > + replace_commits(&commits, &original_commit->object.oid, > + split_commits, ARRAY_SIZE(split_commits)); > + Nice. We use the function introduced earlier to replace the replace the commits with the new two commits. > + ret = apply_commits(repo, &commits, parent, head, "split"); > + if (ret < 0) > + goto out; > + > + ret = 0; > + > +out: > + oidmap_clear(&rewritten_commits, 0); > + clear_pathspec(&pathspec); > + strvec_clear(&commits); > + free_commit_list(list); > + return ret; > +} [snip] The tests look good. Nothing to add there.