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

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 3, 2025, 12:20 UTC
Message-ID: <aLgygyUuQCmR25qU@pks.im>
In-Reply-To: <CALnO6CBRv-Jvm1FsUAkDSyaNNRXtMukReKzefvCw5-vj=-pnCA@mail.gmail.com>

On Tue, Aug 26, 2025 at 09:14:49AM -0400, D. Ben Knoble wrote:

Show 6 quoted lines

> On Sun, Aug 24, 2025 at 1:44 PM Patrick Steinhardt <ps@pks.im> 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]

Show 9 quoted lines

> > +               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.

Show 6 quoted lines

> > +
> > +               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.

Show 18 quoted lines

> > +                       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