Re: [PATCH v2 2/2] builtin/reflog: expire each reflog only once

[Patrick Steinhardt <ps@pks.im>]

On Tue, Nov 10, 2026 at 07:26:41AM -0800, Laveesh Bansal wrote:
> "git reflog expire" processes a reflog once for every time it is named,
> and reports it every time:
> 
>     $ git reflog expire --dry-run --verbose --expire=now main main
>     would prune commit (initial): one
>     would prune commit (initial): one
> 
> The same happens for two names of the same ref, such as "main" and
> "refs/heads/main", and for a name that --all already covers.
> 
> Skip a reflog that this run has already processed.
> 
> Signed-off-by: Laveesh Bansal <laveeshb@laveeshbansal.com>
> ---
> 
> Notes:
>     With --updateref, the first pass can move the ref, so a second pass
>     over the same reflog could prune entries that the new tip no longer
>     reaches.  It now ends as if the name had been given once.

Isn't this note here important information to point out in the commit
message? It sounds like a bug to me that we behave differently depending
on how many times a refname is passed.

> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 42d37a3724..4221136298 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -190,6 +190,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix,
>  	struct worktree_reflogs collected = {
>  		.reflogs = STRING_LIST_INIT_DUP,
>  	};
> +	struct string_list seen = STRING_LIST_INIT_DUP;
>  	struct string_list_item *item;
>  	const struct option options[] = {
>  		OPT_BIT('n', "dry-run", &flags, N_("do not actually prune any entries"),
> @@ -285,6 +286,12 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix,
>  		if (ref)
>  			refname = ref;
>  
> +		if (string_list_has_string(&seen, refname)) {
> +			free(ref);
> +			continue;
> +		}
> +		string_list_insert(&seen, refname);

Hmm. This here retain the current ordering. But is there a reason why we
have to maintain it?

The reason I'm asking is that the behaviour here is O(n * 2 log n), as
for every string you add to the list you both search whether it exists
already and then you search for the insert position. And we could easily
speed this up by using `string_list_append()` and then sorting and
deduping once after the loop.

> diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
> index 998a7e8667..d22216c36d 100755
> --- a/t/t1410-reflog.sh
> +++ b/t/t1410-reflog.sh
> @@ -274,6 +274,20 @@ test_expect_success 'reflog expire --dry-run --verbose --all' '
>  	test_cmp before after
>  '
>  
> +test_expect_success 'reflog expire does not process a reflog twice' '
> +	git reflog expire --dry-run --verbose \
> +		--expire=$(($test_tick - 10000)) \
> +		--expire-unreachable=$(($test_tick - 10000)) \
> +		--stale-fix \
> +		--all >once &&
> +	git reflog expire --dry-run --verbose \
> +		--expire=$(($test_tick - 10000)) \
> +		--expire-unreachable=$(($test_tick - 10000)) \
> +		--stale-fix \
> +		--all main refs/heads/main main >twice &&
> +	test_cmp once twice
> +'

Arguably, if the above is a bug, we should also have a test for it here.

Patrick