[PATCH] builtin/reflog: say "would prune" for named refs with --dry-run

[PATCH] builtin/reflog: say "would prune" for named refs with --dry-run

[Laveesh Bansal]

With --dry-run --verbose, "git reflog expire --all" reports the entries
it would remove as "would prune", but when refs are named on the command
line it reports them as "prune", which looks as if they were removed:

    $ git reflog expire --dry-run --verbose --expire=now HEAD
    prune commit (initial): first

Nothing is removed in either case, only the label is wrong. This was
reported at [1].

fcd2c3d9d8 (reflog + refs-backend: move "verbose" out of the backend,
2021-12-22) added dry_run to struct expire_reflog_policy_cb and set it
in the --all loop, but not in the loop for named refs. Set it there
too.

Add tests for named refs and --all. No test checked the keep/prune
wording before.

[1] https://lore.kernel.org/git/b46cbce8-d2bb-46d2-bdd3-545422813d6b@tngtech.com/

Reported-by: Sven Weiland <sven.weiland@tngtech.com>
Assisted-by: Claude Sonnet 5.5
Signed-off-by: Laveesh Bansal <laveeshb@laveeshbansal.com>
---

Notes:
    The dry_run bit just duplicates what is already in flags, so every
    caller has to copy it, and one got missed. Dropping it would mean
    changing the callback signature, so I left that alone.

 builtin/reflog.c  |  5 ++++-
 t/t1410-reflog.sh | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 1211c58fa4..49e5453dd9 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -279,7 +279,10 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix,
 
 	for (i = 0; i < argc; i++) {
 		char *ref;
-		struct expire_reflog_policy_cb cb = { .opts = opts };
+		struct expire_reflog_policy_cb cb = {
+			.opts = opts,
+			.dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
+		};
 
 		if (!repo_dwim_log(the_repository, argv[i], strlen(argv[i]), NULL, &ref)) {
 			status |= error(_("reflog could not be found: '%s'"), argv[i]);
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index 93b5b49e1d..998a7e8667 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -240,6 +240,40 @@ test_expect_success 'reflog expire --dry-run should not touch reflog' '
 	check_fsck "missing blob $F"
 '
 
+# A dry run only ever says "keep" or "would prune".
+check_dry_run_output () {
+	test_grep "^would prune " "$1" &&
+	sed -e "/^keep /d" -e "/^would prune /d" \
+		-e "/^Marking reachable objects/d" "$1" >unexpected &&
+	test_must_be_empty unexpected
+}
+
+test_expect_success 'reflog expire --dry-run --verbose with a named ref' '
+	git reflog refs/heads/main >before &&
+	git reflog expire --dry-run --verbose \
+		--expire=$(($test_tick - 10000)) \
+		--expire-unreachable=$(($test_tick - 10000)) \
+		--stale-fix \
+		refs/heads/main >output &&
+	check_dry_run_output output &&
+
+	git reflog refs/heads/main >after &&
+	test_cmp before after
+'
+
+test_expect_success 'reflog expire --dry-run --verbose --all' '
+	git reflog refs/heads/main >before &&
+	git reflog expire --dry-run --verbose \
+		--expire=$(($test_tick - 10000)) \
+		--expire-unreachable=$(($test_tick - 10000)) \
+		--stale-fix \
+		--all >output &&
+	check_dry_run_output output &&
+
+	git reflog refs/heads/main >after &&
+	test_cmp before after
+'
+
 test_expect_success 'reflog expire' '
 
 	git reflog expire --verbose \
-- 
2.54.0

Re: [PATCH] builtin/reflog: say "would prune" for named refs with --dry-run

[Junio C Hamano]

Laveesh Bansal <laveeshb@laveeshbansal.com> writes:

> With --dry-run --verbose, "git reflog expire --all" reports the entries
> it would remove as "would prune", but when refs are named on the command
> line it reports them as "prune", which looks as if they were removed:
>
>     $ git reflog expire --dry-run --verbose --expire=now HEAD
>     prune commit (initial): first
>
> Nothing is removed in either case, only the label is wrong. This was
> reported at [1].
>
> fcd2c3d9d8 (reflog + refs-backend: move "verbose" out of the backend,
> 2021-12-22) added dry_run to struct expire_reflog_policy_cb and set it
> in the --all loop, but not in the loop for named refs. Set it there
> too.
> ...
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 1211c58fa4..49e5453dd9 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -279,7 +279,10 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix,
>  
>  	for (i = 0; i < argc; i++) {
>  		char *ref;
> -		struct expire_reflog_policy_cb cb = { .opts = opts };
> +		struct expire_reflog_policy_cb cb = {
> +			.opts = opts,
> +			.dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
> +		};

This obviously mimics what happens in the "if (do_all)" side.

But I have to wonder why nobody finds it disgusting that what
happens inside the "for_each_string_list_item()" and the later "for
(i = 0; i < rgc; i++)" loop have striking similarity between them.

Would it make sense to retructure the code flow to

	if (do_all) {
		collect "all" to a strvec or string list
	}
	foreach arg in argv[]) {
		add arg to the same strvec or string list
	}
	foreach ref in collected {
		do the expiration of ref's reflog
	}

That will help prevent a similar bug from ever happening again, as
there would be only a single place that sets up how the expiration
is called, instead of two we currently have.

Re: [PATCH] builtin/reflog: say "would prune" for named refs with --dry-run

[Laveesh Bansal]

Junio C Hamano <gitster@pobox.com> writes:

> That will help prevent a similar bug from ever happening again, as
> there would be only a single place that sets up how the expiration
> is called, instead of two we currently have.

Agreed, and I think the consolidation is doable.

I suggest keeping this patch as it is, since it fixes the reported
bug. I'll look into what you describe as a separate patch after this
one, and the tests added here would stay useful for that patch too.

Re: [PATCH] builtin/reflog: say "would prune" for named refs with --dry-run

[Junio C Hamano]

Laveesh Bansal <laveeshb@laveeshbansal.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> That will help prevent a similar bug from ever happening again, as
>> there would be only a single place that sets up how the expiration
>> is called, instead of two we currently have.
>
> Agreed, and I think the consolidation is doable.
>
> I suggest keeping this patch as it is, since it fixes the reported
> bug. I'll look into what you describe as a separate patch after this
> one, and the tests added here would stay useful for that patch too.

Sounds like a good plan to me.