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

[Junio C Hamano <gitster@pobox.com>]

Pushkar Singh <pushkarkumarsingh1970@gmail.com> writes:

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

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

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