Re: [PATCH v3] reflog: close leak of reflog expire entry

> On Mon, Jul 21, 2025 at 04:39:37PM -0700, Jacob Keller wrote:
> 
>> Changes in v3:
>> - Remove the incorrect call in reflog_expiry_cleanup()
>> - Add a call in reflog_expire_condition()
>> - Link to v2: https://lore.kernel.org/r/20250709-jk-fix-leak-reflog-expire-config-v2-1-f9af934be8c1@gmail.com
> 
> This looks correct to me except...
> 
>> diff --git a/builtin/gc.c b/builtin/gc.c
>> index 845876ff0286..37f543736599 100644
>> --- a/builtin/gc.c
>> +++ b/builtin/gc.c
>> @@ -346,6 +346,7 @@ static int reflog_expire_condition(struct gc_config *cfg UNUSED)
>>  				 count_reflog_entries, &data);
>>  
>>  	reflog_expiry_cleanup(&data.policy);
>> +	reflog_clear_expire_config(&data.policy);
>>  	return data.count >= data.limit;
>>  }
>>  
> 
> This needs to pass &data.policy.opts, no?
> 

> I think we might also want this test on top (or I'd be happy to see it
> squashed in). It shows off your fix when built with SANITIZE=leak, and
> also catches the bug that v2 of your patch had.
> 
> -Peff
> 

> -- >8 --
> Subject: [PATCH] t1410: add test of gc.<pattern>.reflogExpire config
> 
> We have long supported the ability to set reflog expiration config for
> individual, going back to 3cb22b8efe (Per-ref reflog expiry
> configuration, 2008-06-15). But we have never had any tests.
> 
> Let's add a very basic one that checks that we apply the config
> correctly to a subset of refs (and not elsewhere). This also
> triggers the leaky code fixed by the previous commit.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/t1410-reflog.sh | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
> index 42b501f163..362e90d7d6 100755
> --- a/t/t1410-reflog.sh
> +++ b/t/t1410-reflog.sh
> @@ -320,6 +320,34 @@ test_expect_success 'git reflog expire unknown reference' '
>  	test_grep "error: reflog could not be found: ${SQ}does-not-exist${SQ}" stderr
>  '
>  
> +test_expect_success 'expire with pattern config' '
> +	# Split refs/heads/ into two roots so we can apply config to each. Make
> +	# two branches per root to verify that config is applied correctly
> +	# multiple times.
> +	git branch root1/branch1 &&
> +	git branch root1/branch2 &&
> +	git branch root2/branch1 &&
> +	git branch root2/branch2 &&
> +
> +	test_config "gc.reflogexpire" "never" &&
> +	test_config "gc.refs/heads/root2/*.reflogExpire" "now" &&
> +	git reflog expire \
> +		root1/branch1 root1/branch2 \
> +		root2/branch1 root2/branch2 &&
> +
> +	cat >expect <<-\EOF &&
> +	root1/branch1@{0}
> +	root1/branch2@{0}
> +	EOF
> +	git log -g --branches="root*" --format=%gD >actual.raw &&
> +	# The sole reflog entry of each branch points to the same commit, so
> +	# the order in which they are shown is nondeterministic. We just care
> +	# about the what was expired (and what was not), so sort to get a known
> +	# order.
> +	sort <actual.raw >actual.sorted &&
> +	test_cmp expect actual.sorted
> +'
> +
>  test_expect_success 'checkout should not delete log for packed ref' '
>  	test $(git reflog main | wc -l) = 4 &&
>  	git branch foo &&