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

[Patrick Steinhardt <ps@pks.im>]

On Tue, Jun 02, 2026 at 09:31:17AM +0200, Pablo Sabater wrote:
> El mar, 2 jun 2026 a las 8:16, Patrick Steinhardt (<ps@pks.im>) escribió:
[snip]
> > +       original = lookup_commit_reference_by_name(argv[0]);
> > +       if (!original) {
> > +               ret = error(_("commit cannot be found: %s"), argv[0]);
> > +               goto out;
> > +       }
> > +
> > +       if (!original->parents) {
> > +               ret = error(_("cannot drop root commit %s: "
> > +                             "it has no parent to replay onto"),
> > +                           argv[0]);
> > +               goto out;
> > +       } else if (original->parents->next) {
> > +               ret = error(_("cannot drop merge commit"));
> 
> Why the if block adds which commit context, but not on the else if block?

True indeed, will adapt.

> > diff --git a/t/t3454-history-drop.sh b/t/t3454-history-drop.sh
> > new file mode 100755
> > index 0000000000..b320ff09b3
> > --- /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 --graph --format=%s "$@"
> > +}
> 
> This function appears exactly the same at t6016 and t4215 but named as
> check_graph. I was gonna do a cleanup for a commit series I'm working
> on to bring that function to `lib-log-graph.sh` because all these test
> files share that they import graph functions from `lib-log-graph.c`,
> maybe you could do it?

I'd rather keep this series focussed, but I wouldn't mind a follow up
that deduplicates these call sites.

> Also:
> 
> lib_test_cmp_graph () {
>         git log --graph "$@" >output &&
>         sed 's/ *$//' >output.sanitized <output &&
>         test_cmp expect output.sanitized
> }
> 
> Already uses `--graph` you can drop it from expect_graph()

True indeed, dropped the "--graph" argument.

Thanks!

Patrick