Re: [PATCH v2 9/9] builtin/history: implement "drop" subcommand

[Patrick Steinhardt <ps@pks.im>]

On Wed, Jun 03, 2026 at 09:04:33PM +0200, Kristoffer Haugsbakk wrote:
> On Wed, Jun 3, 2026, at 18:14, Patrick Steinhardt wrote:
> > diff --git a/Documentation/git-history.adoc
> > b/Documentation/git-history.adoc
> > index 2ba8121795..4eac732fd2 100644
> > --- a/Documentation/git-history.adoc
> > +++ b/Documentation/git-history.adoc
> > @@ -51,13 +52,28 @@ be stateful operations. The limitation can be
> > lifted once (if) Git learns about
> >  first-class conflicts.
> >
> >  When using `fixup` with `--empty=drop`, dropping the root commit is not yet
> > -supported.
> > +supported. Likewise, `drop` cannot remove the root commit or a merge commit.
> >
> >  COMMANDS
> >  --------
> >
> >  The following commands are available to rewrite history in different ways:
> >
> > +`drop <commit>`::
> > +	Remove the specified commit from the history. All descendants of the
> > +	commit are replayed directly onto its parent.
> > ++
> > +The root commit cannot be dropped as that may lead to edge cases where refs
> > +end up with no commits anymore. Merge commits cannot be dropped either; see
> > +LIMITATIONS.
> 
> Should section names be “bare” or quoted like "LIMITATIONS"?
> I don’t know.
> 
> Maybe add “above” since it’s a previous section.

Hm. I think I'd prefer to keep this as-is, as we already have
preexisting documentation in the same manpage that does it exactly like
the above.

> > ++
> > +If `HEAD` points at a commit that is to be rewritten, the index and working
> >[snip]
> > +Drop a commit
> > +~~~~~~~~~~~~~
> > +
> > +----------
> > +$ git log --oneline
> > +abc1234 (HEAD -> main) third
> > +def5678 second
> > +ghi9012 first
> > +
> > +$ git history drop def5678
> 
> I know this is only the most simple example. And I might be dragging in
> something beyond the scope of this example. But I recall one
> demonstration on the first git-history(1) series which used a lot of
> revision expressions and someone saying that they couldn’t imagine a
> workflow where this would be more interactive than bringing up the
> git-rebase(1) todo editor.
> 
> (I couldn’t find back to this right now.)

Yeah, I remember this discussion.

> Although it is slower in terms of machine cycles, the keyboard instinct
> for dropping a nearby commit might be to do `git rebase -i @~10`
> (sufficiently high number) and navigating quickly in the configured
> editor, deleting the line or using the keybind for `drop`. This example
> which by implication brings up the log in order to paste the abbreviated
> hash isn’t as ergonomic in comparison.
> 
> But using a revision expression like searching the subject with
> `main^{/second}`, while not quicker probably, does distinguish itself
> from git-rebase(1) by being a pretty fast ad hoc invocation that can be
> done in one command without futzing with some weird sed(1) editor in
> order to navigate to the `second` line and deleting it, or
> something. And that’s a small win in isolation, but it segues much more
> naturally into letting you script, say, dropping the last commit that
> starts with the subject `TEMP`.
> 
> Or maybe revision expressions is too much in this context?

No, I think that's actually a good idea to demonstrate how this can be
used without being too unergomonic.

Eventually I still have the idea in my mind to implement a TUI around
git-history(1) that allows to drive its several subcommands without
having to laboriously select the right commits. Ideally, you'd simply
select the commits you care about and then pick specific actions you
want to do on them, with a nice visual graph that updates after the
operation. But that's a bit into the future, I guess :)

> > diff --git a/t/t3454-history-drop.sh b/t/t3454-history-drop.sh
> > new file mode 100755
> > index 0000000000..37d8413e7e
> > --- /dev/null
> > +++ b/t/t3454-history-drop.sh
> > @@ -0,0 +1,513 @@
> > +#!/bin/sh
> > +
> > +test_description='tests for git-history drop subcommand'
> > +
> > +. ./test-lib.sh
> > +. "$TEST_DIRECTORY/lib-log-graph.sh"
> > +
> > +expect_graph () {
> > +	cat >expect &&
> > +	lib_test_cmp_graph --format=%s "$@"
> > +}
> > +
> > +expect_log () {
> > +	git log --format="%s" "$@" >actual &&
> > +	cat >expect &&
> > +	test_cmp expect actual
> > +}
> > +
> > +test_expect_success 'errors on missing commit argument' '
> > +	test_when_finished "rm -rf repo" &&
> > +	git init repo &&
> > +	(
> > +		cd repo &&
> > +		test_commit initial &&
> > +		test_must_fail git history drop 2>err &&
> > +		test_grep "command expects a single revision" err
> 
> Why not `test_cmp` since it’s a fixed error?
> 
> Same for a few other tests like `errors on unknown revision`.

We tend to use test_grep for cases like this, even for static error
messages. I haven't really figured out myself when exactly we tend to
use one form over the other, to be honest.

> >[snip]
> > +test_expect_success 'errors with invalid --empty= value' '
> > +	test_when_finished "rm -rf repo" &&
> > +	git init repo &&
> > +	test_commit -C repo initial &&
> > +	test_commit -C repo second &&
> > +	test_must_fail git -C repo history drop --empty=bogus HEAD 2>err &&
> > +	test_grep "unrecognized.*--empty.*bogus" err
> > +'
> 
> Style related I guess. Most tests here use a subshell but this one uses
> `git -C`? Why is that?

No good reason, will fix.

Thanks!

Patrick