Re: [PATCH v2] reflog: fix default expiry periods

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 23, 2026, 19:26 UTC
Message-ID: <xmqqpky3ahvo.fsf@gitster.g>
In-Reply-To: <20260923102140.25475-2-pushkarkumarsingh1970@gmail.com>

Pushkar Singh <pushkarkumarsingh1970@gmail.com> writes:

Show 10 quoted lines

> The default reflog expiry periods were swapped when they were moved to
> REFLOG_EXPIRE_OPTIONS_INIT() by 85658275702b (builtin/reflog: stop storing
> default reflog expiry dates globally).
>
> This caused reachable entries to expire after 30 days instead of 90 days,
> and unreachable entries after 90 days instead of 30 days.
>
> Reported-by: r.norouzi <r.norouzi@proton.me>
> Signed-off-by: Pushkar Singh <pushkarkumarsingh1970@gmail.com>
> ---

The above reads very well.

Show 6 quoted lines

>  #define REFLOG_EXPIRE_OPTIONS_INIT(now) { \
> -	.default_expire_total = now - 30 * 24 * 3600, \
> -	.default_expire_unreachable = now - 90 * 24 * 3600, \
> +	.default_expire_total = now - 90 * 24 * 3600, \
> +	.default_expire_unreachable = now - 30 * 24 * 3600, \
>  }

and the fix is very straight-forward.

Show 52 quoted lines

> diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
> index 8f78cf4b01..c494aa5ef0 100755
> --- a/t/t1410-reflog.sh
> +++ b/t/t1410-reflog.sh
> @@ -153,6 +153,51 @@ test_expect_success 'reflog expire should not barf on an annotated tag' '
>  	test_grep ! "error: [Oo]bject .* not a commit" err
>  '
>  
> +test_expect_success 'reflog expire uses the correct default expiry periods' '
> +	test_when_finished "rm -rf reachable-keep reachable-expire unreachable" &&
> +	git init reachable-keep &&
> +	(
> +		cd reachable-keep &&
> +		timestamp=$(test-tool date timestamp "60.days.ago") &&
> +		timestamp=${timestamp#* -> } &&
> +		test_commit --no-tag --date "$timestamp +0000" old &&
> +		git reflog expire --all &&
> +		test_stdout_line_count = 1 git reflog refs/heads/main
> +	) &&
> +	git init reachable-expire &&
> +	(
> +		cd reachable-expire &&
> +		timestamp=$(test-tool date timestamp "100.days.ago") &&
> +		timestamp=${timestamp#* -> } &&
> +		test_commit --no-tag --date "$timestamp +0000" old &&
> +		git reflog expire --all &&
> +		test_stdout_line_count = 0 git reflog refs/heads/main
> +	) &&
> +	git init unreachable &&
> +	(
> +		cd unreachable &&
> +		test_commit --no-tag base &&
> +		base=$(git rev-parse HEAD) &&
> +		timestamp=$(test-tool date timestamp "20.days.ago") &&
> +		timestamp=${timestamp#* -> } &&
> +		test_commit --no-tag --date "$timestamp +0000" old-20 &&
> +		old20=$(git rev-parse HEAD) &&
> +		git update-ref refs/heads/main "$base" &&
> +		timestamp=$(test-tool date timestamp "40.days.ago") &&
> +		timestamp=${timestamp#* -> } &&
> +		test_commit --no-tag --date "$timestamp +0000" old-40 &&
> +		old40=$(git rev-parse HEAD) &&
> +		git update-ref refs/heads/main "$base" &&
> +		git rev-list --all --objects >reachable &&
> +		test_grep ! "$old20" reachable &&
> +		test_grep ! "$old40" reachable &&
> +		git reflog expire --all &&
> +		git reflog --format='%H' refs/heads/main >actual &&
> +		test_grep "$old20" actual &&
> +		test_grep ! "$old40" actual
> +	)
> +'

This one is curious in a few ways.

For reachable ones before and after the cut-off timestamp, we have separate blocks to test them independently, but for unreachable ones, we dedicatge only one block. Is there a good reason for this distinction?

As some people worry about repository set-up and tear-down cost, it may please them more if you create a single test repository, prepare four cases in it, and test them with a single "reflog expire --all".

On the other hand, it makes it easier to debug these tests if you create one test repository for each of the four cases and test them independently, but if we are going that route, we would rather want to have one "test_expect_success" block for each of these four cases.

This "one test_expect_success block that has three repositories, one is used to test two cases and each of the other two is used to test the remaining two cases separately" arrangement looks puzzling.