Thread (119 messages) 119 messages, 6 authors, 2d ago

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.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.
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 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 :)
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" 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.
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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help