Re: [PATCH v3 2/2] reflog: implement subcommand to drop reflogs
From: Karthik Nayak <hidden>
Date: 2025-03-19 09:16:11
Toon Claes [off-list ref] writes:
Karthik Nayak [off-list ref] writes:
[snip]
quoted
+static int cmd_reflog_drop(int argc, const char **argv, const char *prefix, + struct repository *repo) +{ + int ret = 0, do_all = 0, single_worktree = 0; + const struct option options[] = { + OPT_BOOL(0, "all", &do_all, N_("drop the reflogs of all references")), + OPT_BOOL(0, "single-worktree", &single_worktree, + N_("drop reflogs from the current worktree only")), + OPT_END() + }; + + argc = parse_options(argc, argv, prefix, options, reflog_drop_usage, 0); + + if (argc && do_all) + usage(_("references specified along with --all"));What is the intended behavior when both `--all` and `<refs>` are omitted? It seems nothing happens at the moment. And no error nor warning is printed, that feels a bit odd to me. Now, when you do `git reflog expire --expire=all` it also seems to be doing nothing at all. I also think this is weird. And I don't see any test coverage for `git reflog expire` without `--all`. But what is the expected behavior when you omit `--all` and `<refs>`? Should it give an error or warning? Should it use HEAD, just like `git reflog show` does?
As discussed in the other thread [1], ideally this should be raised as an error. I'm leaving it for now. [snip]
quoted
+ +test_expect_success 'reflog drop --all' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + test_commit A && + test_commit_bulk --ref=refs/heads/branch 1 && + git reflog exists refs/heads/main && + git reflog exists refs/heads/branch && + git reflog drop --all && + test_must_fail git reflog exists refs/heads/main && + test_must_fail git reflog exists refs/heads/branchShould we test output of `git reflog list`?
I don't see why, we're concerned with individual reflogs and 'exists' help check against those individual reflogs.
quoted
+ ) +' + +test_expect_success 'reflog drop --all multiple worktrees' ' + test_when_finished "rm -rf repo" && + test_when_finished "rm -rf wt" && + git init repo && + ( + cd repo && + test_commit A && + git worktree add ../wt && + test_commit_bulk -C ../wt --ref=refs/heads/branch 1 && + git reflog exists refs/heads/main && + git reflog exists refs/heads/branch && + git reflog drop --all && + test_must_fail git reflog exists refs/heads/main && + test_must_fail git reflog exists refs/heads/branchShall we test HEAD in both worktrees does not exists?
I think it would be a good addition, but I'm not sure if its worthy of a re-roll.
quoted
+ ) +' + +test_expect_success 'reflog drop --all --single-worktree' ' + test_when_finished "rm -rf repo" && + test_when_finished "rm -rf wt" && + git init repo && + ( + cd repo && + test_commit A && + git worktree add ../wt && + test_commit -C ../wt foobar && + git reflog exists refs/heads/main && + git reflog exists refs/heads/wt && + test-tool ref-store worktree:wt reflog-exists HEAD && + git reflog drop --all --single-worktree && + test_must_fail git reflog exists refs/heads/main && + test_must_fail git reflog exists refs/heads/wt && + test_must_fail test-tool ref-store worktree:main reflog-exists HEAD && + test-tool ref-store worktree:wt reflog-exists HEADNaive question: why is `test-tool ref-store` used and not `git -C ../wt reflog exist`?
That should work too :)
quoted
+ ) +' + +test_expect_success 'reflog drop --all with reference' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + test_commit A && + test_must_fail git reflog drop --all refs/heads/main 2>stderr && + test_grep "usage: references specified along with --all" stderr + ) +' + test_done -- 2.48.1
[1]: CAOLa=ZSj11TSTs6CywSX1Q9AAfW28zssS2yrGf8PmBOgd06Etg@mail.gmail.com
Attachments
- signature.asc [application/pgp-signature] 690 bytes