Re: [PATCH v2 9/9] builtin/history: implement "drop" subcommand
From: Patrick Steinhardt <hidden>
Date: 2026-06-04 09:02:12
On Wed, Jun 03, 2026 at 09:04:33PM +0200, Kristoffer Haugsbakk wrote:
On Wed, Jun 3, 2026, at 18:14, Patrick Steinhardt wrote:quoted
diff --git a/Documentation/git-history.adocb/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 belifted 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.
quoted
++ +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 def5678I 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 :)
quoted
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" errWhy 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.
quoted
[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