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

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

[Laveesh Bansal]

"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].

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.

[1] https://lore.kernel.org/git/xmqqqzhdy7pi.fsf@gitster.g/

Assisted-by: Claude Sonnet 5.5
Signed-off-by: Laveesh Bansal <laveeshb@laveeshbansal.com>
---

Notes:
    The lookup stays in the loop. Doing it while collecting would report an
    unknown name before any expiry output, and make "expire --expire=now
    HEAD @{-1}" succeed.
    
    I ran the full test suite, and the reflog tests with reftable, sha256
    and SANITIZE=leak. Output, exit status and the resulting reflogs match
    the base for the cases I tried, including @{-1} and unknown names.

 builtin/reflog.c | 50 +++++++++++++++++++++++-------------------------
 1 file changed, 24 insertions(+), 26 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 49e5453dd9..42d37a3724 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -187,6 +187,10 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix,
 	unsigned int flags = 0;
 	int verbose = 0;
 	reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;
+	struct worktree_reflogs collected = {
+		.reflogs = 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"),
 			EXPIRE_REFLOGS_DRY_RUN),
@@ -244,10 +248,6 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix,
 	}
 
 	if (do_all) {
-		struct worktree_reflogs collected = {
-			.reflogs = STRING_LIST_INIT_DUP,
-		};
-		struct string_list_item *item;
 		struct worktree **worktrees, **p;
 
 		worktrees = get_worktrees(the_repository);
@@ -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;
+	}
+
+	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;
+
+		reflog_expire_options_set_refname(&cb.opts, refname);
 		status |= refs_reflog_expire(get_main_ref_store(the_repository),
-					     ref, flags,
+					     refname, flags,
 					     reflog_expiry_prepare,
 					     should_prune_fn,
 					     reflog_expiry_cleanup,
 					     &cb);
 		free(ref);
 	}
+	string_list_clear(&collected.reflogs, 0);
 
 	reflog_clear_expire_config(&opts);
 
-- 
2.54.0

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

[Patrick Steinhardt]

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

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

[Laveesh Bansal]

Patrick Steinhardt <ps@pks.im> writes:

> 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".

I want this patch to change no behavior, and deduplicating would change
the output: with "--verbose", a name given twice lists its entries
twice today, and sorting would reorder them.  It could be done in the
loop, by skipping a name already seen, but that is a separate change.

> 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()`.

reflog_expire_options_set_refname() is called for every entry in the
loop, as it was in both loops before, so the per-reflog options apply
to the --all entries too.  Only repo_dwim_log() is conditional: the
names from --all come straight from the ref store and need no lookup.

I'll reword the commit message as you suggest and send a v2 once others
have had a chance to comment.

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

[Patrick Steinhardt]

On Mon, Nov 09, 2026 at 08:13:13AM -0800, Laveesh Bansal wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > 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".
> 
> I want this patch to change no behavior, and deduplicating would change
> the output: with "--verbose", a name given twice lists its entries
> twice today, and sorting would reorder them.  It could be done in the
> loop, by skipping a name already seen, but that is a separate change.

You could add it as a second commit to this patch series. Like that, the
refactoring and the change in behaviour would still be separate from one
another. And that second commit would be a good point in case vouching
for why the refactoring is a good idea.

> > 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()`.
> 
> reflog_expire_options_set_refname() is called for every entry in the
> loop, as it was in both loops before, so the per-reflog options apply
> to the --all entries too.  Only repo_dwim_log() is conditional: the
> names from --all come straight from the ref store and need no lookup.
> 
> I'll reword the commit message as you suggest and send a v2 once others
> have had a chance to comment.

Thanks!

Patrick

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

2 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.

Very good point.  I cannot think of a reason why we should not
consider it a bug that the current two-loop model processes and
reports about the same thing twice, and the collect-then-process
model allows deduplication almost for free to address that bug.

Thanks.

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

[Laveesh Bansal]

This series makes "git reflog expire" expire each reflog only once.  1/2
puts the --all and the named-ref paths in one loop without changing
behavior.  2/2 skips a reflog that was already expired in this run, so
a name given twice, or one that --all also covers, is no longer expired
and reported twice.

v1 was the refactor alone.  I had kept it free of behavior changes and
planned to handle repeated reflogs in a later patch.  After the
discussion on v1 [1][2], I am bringing that change into v2 as a second
patch, which keeps the refactor separate.

Changes since v1: 1/2 only has a new commit message, and 2/2 is new.

I ran the full test suite, and the reflog tests with reftable, sha256
and SANITIZE=leak.  Against the base, the output differs only where a
reflog is reached twice.

Assisted-by: Claude Sonnet 5.5

[1] https://lore.kernel.org/git/avLDE1gGsjjy2ryd@pks.im/
[2] https://lore.kernel.org/git/xmqqwlqlydo2.fsf@gitster.g/


Laveesh Bansal (2):
  builtin/reflog: expire --all and named refs in one loop
  builtin/reflog: expire each reflog only once

 builtin/reflog.c  | 56 ++++++++++++++++++++++++++---------------------
 t/t1410-reflog.sh | 14 ++++++++++++
 2 files changed, 45 insertions(+), 25 deletions(-)

-- 
2.54.0

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

[Laveesh Bansal]

"git reflog expire" calls refs_reflog_expire() from two places, one
for --all and one for the refs named on the command line, each with
its own copy of the callback setup. The copies can drift apart:
d1536ac3e2 (builtin/reflog: say "would prune" for named refs with
--dry-run, 2026-10-23) fixed a copy that was missing dry_run.

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

Signed-off-by: Laveesh Bansal <laveeshb@laveeshbansal.com>
---
 builtin/reflog.c | 50 +++++++++++++++++++++++-------------------------
 1 file changed, 24 insertions(+), 26 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 49e5453dd9..42d37a3724 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -187,6 +187,10 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix,
 	unsigned int flags = 0;
 	int verbose = 0;
 	reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;
+	struct worktree_reflogs collected = {
+		.reflogs = 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"),
 			EXPIRE_REFLOGS_DRY_RUN),
@@ -244,10 +248,6 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix,
 	}
 
 	if (do_all) {
-		struct worktree_reflogs collected = {
-			.reflogs = STRING_LIST_INIT_DUP,
-		};
-		struct string_list_item *item;
 		struct worktree **worktrees, **p;
 
 		worktrees = get_worktrees(the_repository);
@@ -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;
+	}
+
+	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;
+
+		reflog_expire_options_set_refname(&cb.opts, refname);
 		status |= refs_reflog_expire(get_main_ref_store(the_repository),
-					     ref, flags,
+					     refname, flags,
 					     reflog_expiry_prepare,
 					     should_prune_fn,
 					     reflog_expiry_cleanup,
 					     &cb);
 		free(ref);
 	}
+	string_list_clear(&collected.reflogs, 0);
 
 	reflog_clear_expire_config(&opts);
 
-- 
2.54.0

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

[Laveesh Bansal]

"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.

 builtin/reflog.c  |  8 ++++++++
 t/t1410-reflog.sh | 14 ++++++++++++++
 2 files changed, 22 insertions(+)

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);
+
 		reflog_expire_options_set_refname(&cb.opts, refname);
 		status |= refs_reflog_expire(get_main_ref_store(the_repository),
 					     refname, flags,
@@ -295,6 +302,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix,
 		free(ref);
 	}
 	string_list_clear(&collected.reflogs, 0);
+	string_list_clear(&seen, 0);
 
 	reflog_clear_expire_config(&opts);
 
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
+'
+
 test_expect_success 'reflog expire' '
 
 	git reflog expire --verbose \
-- 
2.54.0

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

[Patrick Steinhardt]

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