Patrick Steinhardt [off-list ref] writes:

On Mon, Mar 10, 2025 at 01:36:25PM +0100, Karthik Nayak wrote:

quoted

diff --git a/Documentation/git-reflog.adoc b/Documentation/git-reflog.adoc
index a929c52982..6ed98ddaef 100644
--- a/Documentation/git-reflog.adoc
+++ b/Documentation/git-reflog.adoc

@@ -16,6 +16,7 @@ SYNOPSIS
 	[--dry-run | -n] [--verbose] [--all [--single-worktree] | <refs>...]
 'git reflog delete' [--rewrite] [--updateref]
 	[--dry-run | -n] [--verbose] <ref>@{<specifier>}...
+'git reflog drop' [--all | <refs>...]
 'git reflog exists' <ref>

 DESCRIPTION

@@ -48,15 +49,19 @@ and not reachable from the current tip, are removed from the reflog.
 This is typically not used directly by end users -- instead, see
 linkgit:git-gc[1].

-The "delete" subcommand deletes single entries from the reflog. Its
-argument must be an _exact_ entry (e.g. "`git reflog delete
-master@{2}`"). This subcommand is also typically not used directly by
-end users.
+The "delete" subcommand deletes single entries from the reflog, but
+not the reflog itself. Its argument must be an _exact_ entry (e.g. "`git
+reflog delete master@{2}`"). This subcommand is also typically not used
+directly by end users.

 The "exists" subcommand checks whether a ref has a reflog.  It exits
 with zero status if the reflog exists, and non-zero status if it does
 not.

+The "drop" subcommand completely removes the reflog for the specified
+references. This is in contrast to "expire" and "delete", both of which
+can be used to delete reflog entries, but not the reflog itself.
+

I guess this paragraph should also moved between "delete" and "exists"
now.

Yeah, make sense.

quoted

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 95f264989b..cd93a0bef9 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c

@@ -449,10 +458,58 @@ static int cmd_reflog_exists(int argc, const char **argv, const char *prefix,
 				   refname);
 }

+static int cmd_reflog_drop(int argc, const char **argv, const char *prefix,
+			   struct repository *repo)
+{
+	int ret = 0, do_all = 0;
+	const struct option options[] = {
+		OPT_BOOL(0, "all", &do_all, N_("process the reflogs of all references")),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options, reflog_drop_usage, 0);
+
+	if (argc && do_all)
+		die(_("references specified along with --all"));

We should probably use `usage()` instead of `die()` here.

Good point.

quoted

+	if (do_all) {
+		struct worktree_reflogs collected = {
+			.reflogs = STRING_LIST_INIT_DUP,
+		};
+		struct string_list_item *item;
+		struct worktree **worktrees, **p;
+
+		worktrees = get_worktrees();
+		for (p = worktrees; *p; p++) {
+			collected.worktree = *p;
+			refs_for_each_reflog(get_worktree_ref_store(*p),
+					     collect_reflog, &collected);
+		}
+		free_worktrees(worktrees);
+
+		for_each_string_list_item(item, &collected.reflogs)
+			ret |= refs_delete_reflog(get_main_ref_store(repo),
+						     item->string);
+		string_list_clear(&collected.reflogs, 0);
+	}

I noticed that `git reflog expire` has the same arguments to specify
which reflogs to expire:

    [--all [--single-worktree] | <refs>...]

The only exception is that they also support `--single-worktree` to only
expire relfogs from the current worktree. Supporting it should probably
not be too much work, so do we want to do so to have feature parity
regarding the reflog selection?

I think it would make sense to add support for `--single-worktree`. Let
me add that in, since it mostly a single-line change.

quoted

+	for (int i = 0; i < argc; i++) {
+		char *ref;
+		if (!repo_dwim_log(repo, argv[i], strlen(argv[i]), NULL, &ref)) {
+			ret |= error(_("%s points nowhere!"), argv[i]);

As a user I wouldn't know what this error is trying to tell me. Does the
reflog exist but it's a symreflog that points to another reflog that
does not exist? Do its entries point nowhere?

How about: `error(_("reflog could not be found: '%s'"))` instead? And
seeing that you copied the error message from the "expire" subcommand
we could also adapt it in a preparatory commit.

Thanks! let change it in both places.

quoted

+			continue;
+		}
+
+		ret |= refs_delete_reflog(get_main_ref_store(repo), ref);
+		free(ref);
+	}

The code is correct, but do we want to maybe wrap this loop in the
`else` branch to guide the reader and make it blindingly obvious that
the loop does nothing `if (do_all)`?

Wouldn't it be simpler to return at the end of the `if (do_all)`? I've
added that, but if feel strongly about this form, happy to change it.

quoted

diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index 388fdf9ae5..251caaf9a4 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh

@@ -551,4 +551,71 @@ test_expect_success 'reflog with invalid object ID can be listed' '
 	)
 '

+test_expect_success 'reflog drop non-existent ref' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_must_fail git reflog exists refs/heads/non-existent &&
+		test_must_fail git reflog drop refs/heads/non-existent 2>stderr &&
+		test_grep "error: refs/heads/non-existent points nowhere!" stderr
+	)
+'

One edge case that I haven't seen is to try and drop multiple
references, some of which exist and some of which don't. The loops you
have seem to explicitly allow for deletion of only a subset, so it would
be nice to verify that the logic works as expected.

Good catch, I also noticed that I didn't have a test for worktrees. So
will add that in too.

Patrick