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

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

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

Toon Claes <toon@iotcl.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>

[snip]

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

>> +
>> +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/branch
>
> Should 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.

>> +	)
>> +'
>> +
>> +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/branch
>
> Shall 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.

>> +	)
>> +'
>> +
>> +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 HEAD
>
> Naive question: why is `test-tool ref-store` used and not
> `git -C ../wt reflog exist`?
>

That should work too :)

>> +	)
>> +'
>> +
>> +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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]