builtin/reflog: expire --all and named refs in one loop
PATCH 1/1 builtin/reflog: expire --all and named refs in one loop
"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/[email protected]/
---
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(-)
builtin/reflog.c +24 -26
@@ -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);
Signature · 1 line
2.54.0
Quoted 5 lines · 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 missingdry_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.
Quoted 3 lines
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.
Quoted 22 lines
builtin/reflog.c +0
-14@@ -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);
builtin/reflog.c static int cmd_reflog_expire(int argc, const char **argv, const char *prefix, · line 268
Quoted 10 lines
builtin/reflog.c +4
-3- } -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.
Quoted 11 lines
builtin/reflog.c +4
-3+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 ; +
builtin/reflog.c static int cmd_reflog_expire(int argc, const char **argv, const char *prefix, · line 286
Quoted 10 lines
builtin/reflog.c +7
-1+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
Quoted 4 lines · Patrick Steinhardt
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.
Quoted 3 lines
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.
Quoted 11 lines · Laveesh Bansal
Quoted 4 lines · Patrick Steinhardt
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.
Quoted 11 lines
Quoted 3 lines
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. Onlyrepo_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
Patrick Steinhardt <[email protected]> writes:
2 Okay, so instead of carrying the logic around refs_reflog_expire()
Quoted 10 lines
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.
v2 cover builtin/reflog: expire each reflog only once
In reply to Laveesh BansalThis 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.
[1] https://lore.kernel.org/git/[email protected]/
[2] https://lore.kernel.org/git/[email protected]/
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(-)
Signature · 1 line
2.54.0
PATCH v2 1/2 builtin/reflog: expire --all and named refs in one loop
"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.
--- builtin/reflog.c | 50 +++++++++++++++++++++++------------------------- 1 file changed, 24 insertions(+), 26 deletions(-)
builtin/reflog.c +24 -26
@@ -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);
Signature · 1 line
2.54.0
PATCH v2 2/2 builtin/reflog: expire each reflog only once
In reply to 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): oneThe 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.
---
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(+)
builtin/reflog.c +8 -0
@@ -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);
t/t1410-reflog.sh +14 -0
@@ -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 \
Signature · 1 line
2.54.0
Re: [PATCH v2 2/2] builtin/reflog: expire each reflog only once
Quoted 12 lines · 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): oneThe 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.
Quoted 7 lines
Signed-off-by Laveesh Bansal <[email protected]>---
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.
Quoted 13 lines
builtin/reflog.c +1
-0@@ -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"),
builtin/reflog.c static int cmd_reflog_expire(int argc, const char **argv, const char *prefix, · line 293
Quoted 8 lines
builtin/reflog.c +5
-0@@ -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.
Quoted 11 lines
t/t1410-reflog.sh +3
-0@@ -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)) \
t/t1410-reflog.sh test_expect_success 'reflog expire --dry-run --verbose --all' ' · line 289
Quoted 10 lines
t/t1410-reflog.sh +10
-0+ --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