Patrick Steinhardt <ps@xxxxxx> writes:
> On Fri, Mar 07, 2025 at 12:17:26PM +0100, Karthik Nayak wrote:
>> 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.
>
That makes sense, let me do that.
>> 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 :)
>
I was thinking similar but decided to go with the latter (sans typo).
Will fix it.
>
>>
>> Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx>
>> ---
>> 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`?
>
I would have like it if they were actually alphabetically sorted, but I
guess this is good alternative to cluster similar sub-commands.
>> 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.
>
Thanks, this does look better and I agree, we should call out "delete"
too.
>> 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?
>
We can, let me fix it! I'll also move the initialization of 'i' down to
the loop while we're here.
>> + 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`.
>
Makes sense, let me add a 'die()' there.
>> + 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?
>
Yes it would, will add.
>> + 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.
>
You mean we could directly do `(get_main_ref_store(repo), argv[i]);` ?
The issue is that this returns '0', even when the reflog doesn't exist.
So to notify the user correctly, we do this check.
>> 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?
>
That would be nice addittion, will add.
> Patrick
Attachment:
signature.asc
Description: PGP signature
