Re: [PATCH 2/2] reflog: implement subcommand to drop reflogs
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Mar 7, 2025, 11:50 UTC
- Message-ID
- <Z8rdg90kxmKHHbyh@pks.im>
- In-Reply-To
- <20250307-493-add-command-to-purge-reflog-entries-v1-2-84ab8529cf9e@gmail.com>
On Fri, Mar 07, 2025 at 12:17:26PM +0100, Karthik Nayak wrote:
Show 9 quoted lines
> Add a new 'drop' subcommand to git-reflog that allows users to delete > the entire reflog for a specified reference. Include a '--all' flag to > enable dropping all reflogs in a repository. > > While 'git-reflog(1)' currently allows users to expire reflogs and > delete individual entries, it lacks functionality to completely remove > reflogs for specific references. This becomes problematic in > repositories where reflogs are not needed but continue to accumulate > entries despite setting 'core.logAllRefUpdates=false'.
I think the order of the two paragraphs should be switched: we tend to first explain the problem before explaining how to address it.
> While here, remove an erranous newline in the file.
I suspet this should either be "extraneous" or "erroneous"? I cannot quite tell which of both it shuld be :)
Show 17 quoted lines
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> Documentation/git-reflog.adoc | 6 +++++
> builtin/reflog.c | 58 ++++++++++++++++++++++++++++++++++++++++++-
> t/t1410-reflog.sh | 55 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 118 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-reflog.adoc b/Documentation/git-reflog.adoc
> index a929c52982..4ecee297de 100644
> --- a/Documentation/git-reflog.adoc
> +++ b/Documentation/git-reflog.adoc
> @@ -17,6 +17,7 @@ SYNOPSIS
> 'git reflog delete' [--rewrite] [--updateref]
> [--dry-run | -n] [--verbose] <ref>@{<specifier>}...
> 'git reflog exists' <ref>
> +'git reflog drop' [--all | <refs>...]Should we put the command next to `delete`?
Show 10 quoted lines
> DESCRIPTION > ----------- > @@ -57,6 +58,11 @@ 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 removes the reflog for the specified references. > +In contrast, "expire" can be used to prune all entries from a reflog, > +but the reflog itself will still exist for that reference. To fully > +remove the reflog for specific references, use the "drop" subcommand.
The last sentence feels like pointless duplication to me. We should likely also point out how it is different from "delete". How about:
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.It might also be useful to add a comment to "delete" to say that it deletes entries, but not the reflog.
Show 50 quoted lines
> OPTIONS
> -------
>
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index f92258f6b6..232602c1a6 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -27,6 +27,9 @@
> #define BUILTIN_REFLOG_EXISTS_USAGE \
> N_("git reflog exists <ref>")
>
> +#define BUILTIN_REFLOG_DROP_USAGE \
> + N_("git reflog drop [--all | <refs>...]")
> +
> static const char *const reflog_show_usage[] = {
> BUILTIN_REFLOG_SHOW_USAGE,
> NULL,
> @@ -52,12 +55,18 @@ static const char *const reflog_exists_usage[] = {
> NULL,
> };
>
> +static const char *const reflog_drop_usage[] = {
> + BUILTIN_REFLOG_DROP_USAGE,
> + NULL,
> +};
> +
> static const char *const reflog_usage[] = {
> BUILTIN_REFLOG_SHOW_USAGE,
> BUILTIN_REFLOG_LIST_USAGE,
> BUILTIN_REFLOG_EXPIRE_USAGE,
> BUILTIN_REFLOG_DELETE_USAGE,
> BUILTIN_REFLOG_EXISTS_USAGE,
> + BUILTIN_REFLOG_DROP_USAGE,
> NULL
> };
>
> @@ -447,10 +456,56 @@ 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 i, ret, do_all;
> + const struct option options[] = {
> + OPT_BOOL(0, "all", &do_all, N_("process the reflogs of all references")),
> + OPT_END()
> + };
> +
> + do_all = ret = 0;Can't we initiailize the variables directly when declaring them?
> + argc = parse_options(argc, argv, prefix, options, reflog_drop_usage, 0);
> +
> + if (do_all) {`do_all` and `argc > 0` should be mutually exclusive from my point of view, as the combination does not make any sense. We should likely die if we see both to be non-zero. Similarly, I think we should abort on `!do_all && !argc`.
Show 5 quoted lines
> + struct worktree_reflogs collected = {
> + .reflogs = STRING_LIST_INIT_DUP,
> + };
> + struct string_list_item *item;
> + struct worktree **worktrees, **p;Would it be useful to point out in the docs that we also prune logs of worktrees?
Show 20 quoted lines
> + 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);
> + }
> +
> + for (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]);
> + continue;
> + }Is there a particular reason why we have to double check that the reflog that we just enumerated really exists? It feels rather unnecessary to me.
Show 15 quoted lines
> diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh > index 388fdf9ae5..b6e44ce6b9 100755 > --- a/t/t1410-reflog.sh > +++ b/t/t1410-reflog.sh > @@ -551,4 +551,59 @@ 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
Do we want to check the error message of the latter command?
Patrick