Re: [PATCH] builtin/reflog: expire --all and named refs in one loop

[Patrick Steinhardt <ps@pks.im>]

On Sat, Nov 07, 2026 at 05:22:13PM -0800, Laveesh Bansal wrote:
> "git reflog expire" sets up the call to refs_reflog_expire() twice,
> once for --all and once for refs named on the command line.
> d1536ac3e2 (builtin/reflog: say "would prune" for named refs with
> --dry-run, 2026-10-23) fixed the named-ref copy, which was missing
> dry_run. Junio suggested a single loop in the review of that fix [1].

We don't typically refer to suggestions in our commit messages like
this. You should instead explain what the underlying issue is in your
own words and why that issue should be fixed so that readers can read
this message without having to hunt for context in other mailing list
threads.

That being said, it can be useful to refer to this mail after the "---"
line or additional context.

> Collect the refs from both sources into one list and expire them in a
> single loop. The names given on the command line are still looked up
> in that loop, so the output and the exit status stay the same.

You say what you do, but you don't say why you do it or what it buys us.

> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 49e5453dd9..42d37a3724 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -259,44 +259,42 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix,
>  					     collect_reflog, &collected);
>  		}
>  		free_worktrees(worktrees);
> -
> -		for_each_string_list_item(item, &collected.reflogs) {
> -			struct expire_reflog_policy_cb cb = {
> -				.opts = opts,
> -				.dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
> -			};
> -
> -			reflog_expire_options_set_refname(&cb.opts,  item->string);
> -			status |= refs_reflog_expire(get_main_ref_store(the_repository),
> -						     item->string, flags,
> -						     reflog_expiry_prepare,
> -						     should_prune_fn,
> -						     reflog_expiry_cleanup,
> -						     &cb);
> -		}
> -		string_list_clear(&collected.reflogs, 0);
>  	}
>  
> +	/* util marks the names that still have to be looked up */
>  	for (i = 0; i < argc; i++) {
> -		char *ref;
> +		item = string_list_append(&collected.reflogs, argv[i]);
> +		item->util = (void *)1;
> +	}

Okay, so instead of carrying the logic around `refs_reflog_expire()`
twice you instead now first gather all reflog names that we want to
expire, and then we do that in a single step. That seems like a useful
simplification to me.

One thing I wonder though: now that we first gather reflog names, can't
we also easily deduplicate them? For example if the same reflog name was
given multiple times on the command line, or in case the reflog is
included both via a command line argument and via "--all".

So I wonder whether we should both sort and deduplicate entries here.

> +	for_each_string_list_item(item, &collected.reflogs) {
>  		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]);
> +		const char *refname = item->string;
> +		char *ref = NULL;
> +
> +		if (item->util &&
> +		    !repo_dwim_log(the_repository, item->string,
> +				   strlen(item->string), NULL, &ref)) {
> +			status |= error(_("reflog could not be found: '%s'"),
> +					item->string);
>  			continue;
>  		}
> -		reflog_expire_options_set_refname(&cb.opts, ref);
> +		if (ref)
> +			refname = ref;

Okay, we only apply per-reflog options in the case where the reflog name
was explicitly specified by the user, becaues only in that case do we
call `repo_dwim_log()`. This retains the status quo, but now that one
sees this single loop it makes you wonder whether that's a sensible
thing to do.

Patrick