On 7/9/2025 8:42 PM, Jeff King wrote:
> On Thu, Jul 10, 2025 at 11:00:38AM +0800, Lidong Yan wrote:
>
>> In builtin/reflog.c, we have code like
>>
>> ---
>> for (i = 0; i < argc; i++) {
>> char *ref;
>> struct expire_reflog_policy_cb cb = { .opts = opts };
>>
>> if (!repo_dwim_log(the_repository, argv[i], strlen(argv[i]), NULL, &ref)) {
>> status |= error(_("reflog could not be found: '%s'"), argv[i]);
>> continue;
>> }
>> reflog_expire_options_set_refname(&cb.opts, ref);
>> status |= refs_reflog_expire(get_main_ref_store(the_repository),
>> ref, flags,
>> reflog_expiry_prepare,
>> should_prune_fn,
>> reflog_expiry_cleanup,
>> &cb);
>> free(ref);
>> }
>> + reflog_clear_expire_config(&opts);
>> ---
>>
>> I think allowing reblog_expiry_cleanup() to free all opt->entries might
>> cause reblog_expire_options_set_refname() to behave incorrectly.
>
> Hmm, yeah. We are calling this in a loop, so we'd want the config to
> persist until the loop ends. I didn't test, but I'd guess that:
>
> git -c 'gc.refs/heads/*.reflogExpire=now' \
> reflog expire refs/heads/foo refs/heads/bar
>
> would apply the config for "foo" but not for "bar". So I think
> reflog_expiry_cleanup() has to just clean up per-traversal data, not the
> config.
>
> So the call at the end here looks reasonable, but the call in
> reflog_expiry_cleanup() is wrong. I guess it was trying to cover the
> call in reflog_expire_condition(). That probably just needs a manual:
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 845876ff02..37f5437365 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;
Ya, you're right. I just thought the reflog_expiry_cleanup would only be
called by this function. It did pass the tests... I'll see if I can add
a test case covering this since its caused a bit more trouble than I
thought it would.
> }
>
>
> -Peff
Attachment:
OpenPGP_signature.asc
Description: OpenPGP digital signature
