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

[Patrick Steinhardt <ps@pks.im>]

On Wed, Aug 20, 2025 at 05:27:32PM -0400, D. Ben Knoble wrote:
> On Wed, Aug 20, 2025 at 5:05 AM Patrick Steinhardt <ps@pks.im> wrote:
> > diff --git a/Documentation/git-history.adoc b/Documentation/git-history.adoc
> > index 6e8b4e1326..f0f1f2a093 100644
> > --- a/Documentation/git-history.adoc
> > +++ b/Documentation/git-history.adoc
> > @@ -47,6 +48,26 @@ reorder <revision> (--before=<revision>|--after=<revision>)::
> >         commit. The commits must be related to one another and must be
> >         reachable from the current `HEAD` commit.
> >
> > +split <revision> [--message=<message>] [--] [<pathspec>...]::
> > +       Interactively split up the commit 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.
> > ++
> > +The commit message of the new commit will be asked for by launching the
> > +configured editor. Authorship of the commit will be the same as for the
> > +original commit.
> > ++
> > +If passed, _<pathspec>_ can be used to limit which changes shall be split out
> > +of the original commit. Files not matching any of the pathspecs will remain
> > +part of the original commit. For more details about the _<pathspec>_ syntax,
> > +see the 'pathspec' entry.
> 
> Glossary entry?

Yup.

> > +       /*
> > +        * But we do ask the user for a new commit message. This is in contrast
> > +        * to the second commit, where we'll retain the original commit
> > +        * message.
> > +        */
> 
> Interesting. I can see using the original as the template for _both_,
> or the first instead of the second. jj's split works a little
> differently (especially with their notion of descriptions), so I can't
> use them as a reference for the behavior.
> 
> I suppose this is one of those "everybody has their preference"
> things, but I think giving the message in both new commits as the
> template gives splitters the most information available when writing
> the message. (Of course, in my editor, I can presumably do something
> like ":Git show -s <split-commit-ish>" if I want.)

I think giving only the split-out changes is a reasonable default, but I
can totally see that we might eventually want to add a command line
option to change the behaviour.

> > +       if (!commit_message) {
> > +               split_message_path = repo_git_path(repo, "SPLIT_MSG");
> > +               strbuf_addch(&split_message, '\n');
> > +               strbuf_commented_addf(&split_message, comment_line_str,
> > +                                     _("Please enter a commit message for the split-out changes."));
> > +               write_file_buf(split_message_path, split_message.buf, split_message.len);
> 
> I also noticed the commented template differs substantially from the
> regular commit template, and my editor doesn't recognize "SPLIT_MSG"
> as a commit message file.
> 
> The latter can be fixed elsewhere, but for the former: perhaps it's
> worth using the usual template with the wording here prepended?
> Respecting commit.verbose / commit.status, too.

Yeah, that's something I wanted to get around to, but haven't yet. I
also noticed that it's not exactly easy to figure out what you're
currently editing without that lack of context.

I'll include that in v2.

> BTW, if I quit the editor with an error here, I'm left back where I
> started. So I'd have to re-stage changes if I wanted to split again,
> which is a bit different from how interactive rebase will leave me
> with the partially staged changes. Obviously that's harder to do with
> the in-memory index + automatic re-application of remaining patch when
> finished, so maybe a note in the docs about this being "all or
> nothing"?

Yeah, fair. I guess adding a note for now is the best way to go about
it, but this is certainly something we can and should iterate on in the
future.

Patrick