Re: [PATCH v2] reflog: implement subcommand to drop reflogs
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Mar 12, 2025, 07:15 UTC
- Message-ID
- <Z9E0oDgGOM1R-4mI@pks.im>
- In-Reply-To
- <20250310-493-add-command-to-purge-reflog-entries-v2-1-05caa92e0bfa@gmail.com>
On Mon, Mar 10, 2025 at 01:36:25PM +0100, Karthik Nayak wrote:
Show 33 quoted lines
> 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.
Show 21 quoted lines
> 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.
Show 20 quoted lines
> + 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?
> + 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.
Show 6 quoted lines
> + 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)`?
Show 18 quoted lines
> 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.
Patrick