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

Hi!

El mar, 2 jun 2026 a las 8:16, Patrick Steinhardt ([off-list ref]) escribió:

+
+static int cmd_history_drop(int argc,
+                           const char **argv,
+                           const char *prefix,
+                           struct repository *repo)
+{
+       const char * const usage[] = {
+               GIT_HISTORY_DROP_USAGE,
+               NULL,
+       };
+       enum replay_empty_commit_action empty = REPLAY_EMPTY_COMMIT_DROP;
+       enum ref_action action = REF_ACTION_DEFAULT;
+       int dry_run = 0;
+       struct option options[] = {
+               OPT_CALLBACK_F(0, "update-refs", &action, "(branches|head)",
+                              N_("control which refs should be updated"),
+                              PARSE_OPT_NONEG, parse_ref_action),
+               OPT_BOOL('n', "dry-run", &dry_run,
+                        N_("perform a dry-run without updating any refs")),
+               OPT_CALLBACK_F(0, "empty", &empty, "(drop|keep|abort)",
+                              N_("how to handle descendants that become empty"),
+                              PARSE_OPT_NONEG, parse_opt_empty),
+               OPT_END(),
+       };
+       struct strbuf reflog_msg = STRBUF_INIT;
+       struct commit *original, *rewritten;
+       struct rev_info revs = { 0 };
+       struct replay_result result = { 0 };
+       struct commit *old_head, *new_head;
+       bool head_moves = false;
+       int ret;
+
+       argc = parse_options(argc, argv, prefix, options, usage, 0);
+       if (argc != 1) {
+               ret = error(_("command expects a single revision"));
+               goto out;
+       }
+       repo_config(repo, git_default_config, NULL);
+
+       if (action == REF_ACTION_DEFAULT)
+               action = REF_ACTION_BRANCHES;
+
+       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?

+               goto out;
+       }

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?

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()

I can't say much more, from what I tested it worked fine but I haven't
tested very exhaustively tho,

--
Pablo