Subject: Re: [PATCH RFC v2 15/16] builtin/history: implement "split" subcommand

From: Patrick Steinhardt <[email-masked]>
Date: Wed, 3 Sep 2025 14:20:19 +0200

On Tue, Aug 26, 2025 at 09:14:49AM -0400, D. Ben Knoble wrote:
> On Sun, Aug 24, 2025 at 1:44 PM Patrick Steinhardt <[email-masked]> wrote:
> > diff --git a/builtin/history.c b/builtin/history.c
> > index 16b516856e..6d3f44152c 100644
> > --- a/builtin/history.c
> > +++ b/builtin/history.c
> > @@ -517,6 +527,285 @@ static int cmd_history_reorder(int argc,
[snip]
> > +               wt_status_collect_changes_trees(&s, old_tree, new_tree);
> > +               wt_status_print(&s);
> > +               wt_status_collect_free_buffers(&s);
> > +               string_list_clear_func(&s.change, change_data_free);
> 
> I think I'm supposed to see the changes between the old and new trees,
> right? Does this only happen if I use the interactive machinery to
> edit a hunk? When I try accepting some changes and leaving others for
> the next commit I get no diff in the template.

Yeah, it's supposed to show the diff between old and new tree indeed. So
in theory you should see something.

> I did try to add new diff lines to a hunk, and nothing showed up…
> maybe I'm holding it wrong? I'm pretty sure I compiled this version.

Do you maybe have a reproducer for this? It seems to work alright for
me, but I wouldn't be surprised if there was a bug here. The wt-status
interfaces are quite something and I was tearing my hair while trying to
figure them out.

> It doesn't look like it's triggered only on commit.verbose config, either.

Fixed now.

> > +
> > +               strbuf_reset(out);
> > +               if (launch_editor(path, out, NULL)) {
> > +                       fprintf(stderr, _("Please supply the message using either -m or -F option.\n"));
> 
> According to the usage, git history split only supports -m, not -F ;)

True. I didn't want to add too many options right from the start to keep
the series somewhat simple. We should eventually add it though.

> > +                       return -1;
> > +               }
> > +               strbuf_stripspace(out, comment_line_str);
> > +
> > +       } else {
> > +               strbuf_addstr(out, provided_message);
> > +       }
> > +
> > +       cleanup_message(out, COMMIT_MSG_CLEANUP_ALL, 0);
> > +
> > +       if (!out->len) {
> > +               fprintf(stderr, _("Aborting commit due to empty commit message.\n"));
> 
> It _would_ be nice if this and similar errors left me able to "try
> again" without losing staged changes—I think I mentioned this before,
> though. And with the in-memory indices vs. actual working state,
> presenting a UI here could be very difficult. So it's an
> understandable choice.

Yeah, I don't dare touching this yet, but certainly see that this might
be a worthwhile addition as we iterate on this command.

Patrick