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

[Junio C Hamano <gitster@pobox.com>]

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.