Re: [PATCH v2] reflog: implement subcommand to drop reflogs

> On Mon, Mar 10, 2025 at 01:36:25PM +0100, Karthik Nayak wrote:
>> 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.
>

>> 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.
>

>> +	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.
>

>> +			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)`?
>

>> 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.
>