Patrick Steinhardt <ps@xxxxxx> writes:
> 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.
>
Yeah, make sense.
>> 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.
>
Good point.
>> + 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?
>
I think it would make sense to add support for `--single-worktree`. Let
me add that in, since it mostly a single-line change.
>> + 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.
>
Thanks! let change it in both places.
>> + 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)`?
>
Wouldn't it be simpler to return at the end of the `if (do_all)`? I've
added that, but if feel strongly about this form, happy to change it.
>> 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.
>
Good catch, I also noticed that I didn't have a test for worktrees. So
will add that in too.
> Patrick
Attachment:
signature.asc
Description: PGP signature
