Re: [PATCH 2/2] reflog: implement subcommand to drop reflogs

[Karthik Nayak <karthik.188@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6951 bytes --]

Patrick Steinhardt <ps@pks.im> 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@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`?
>

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]