From: Toon Claes <toon@iotcl.com>
Date: Wed, 18 Feb 2026 14:21:02 GMT
Subject: Re: [PATCH v6 4/6] refs: move out stub modification to generic layer
Message-ID: <87o6lmceup.fsf@iotcl.com>
In-Reply-To: <CAOLa=ZQwrOGpZfVtfTfPFhnkJ_qnEhv8mxO3Ot7nQXusbkJkYw@mail.gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

> Patrick Steinhardt <ps@pks.im> writes:
>
>> On Sat, Feb 14, 2026 at 11:34:17PM +0100, Karthik Nayak wrote:
>>> When creating the reftable reference backend on disk, we create stubs to
>>> ensure that the directory can be recognized as a Git repository. This is
>>> done by calling `refs_create_refdir_stubs()`. Move this to the generic
>>> layer as this is needed for all backends excluding from the files
>>> backends. In an upcoming commit, we'll also need to extend this logic to
>>> create stubs when using alternate reference directories.
>>>
>>> Similarly, move the logic for deletion of stubs to the generic layer.
>>> The files backend recursively calls the remove function of the
>>> 'packed-backend', here skip calling the generic function since that
>>> would try to delete stubs.
>>
>> Tiniest nit: it might make sense to reorder patches a bit so that the
>> creation of `refs_create_refdir_stubs()` and this patch here sit next to
>> each other.
>>
>
> I think that would be nice, let me do that.

Thanks, I was thinking the same, but I wasn't going to comment on that.
Happy to see you've agreed on this already.

>> What's missing a bit in the commit message is the motivation. What does
>> this step enable us to do that we couldn't do before?
>>
>
> I did add a line
>
>   In an upcoming commit, we'll also need to extend this logic to create
>   stubs when using alternate reference directories.
>
> I'll expand a little on that.

<3

>>> diff --git a/refs.c b/refs.c
>>> index 11d028232b..a24602c9bf 100644
>>> --- a/refs.c
>>> +++ b/refs.c
>>> @@ -2190,12 +2190,59 @@ void refs_create_refdir_stubs(struct repository *repo, const char *refdir,
>>>  /* backend functions */
>>>  int ref_store_create_on_disk(struct ref_store *refs, int flags, struct strbuf *err)
>>>  {
>>> -	return refs->be->create_on_disk(refs, flags, err);
>>> +	int ret = refs->be->create_on_disk(refs, flags, err);
>>> +
>>> +	if (!ret &&
>>> +	    ref_storage_format_by_name(refs->be->name) != REF_STORAGE_FORMAT_FILES) {
>>> +		struct strbuf msg = STRBUF_INIT;
>>> +
>>> +		strbuf_addf(&msg, "this repository uses the %s format", refs->be->name);
>>> +		refs_create_refdir_stubs(refs->repo, refs->gitdir, msg.buf);
>>> +		strbuf_release(&msg);
>>> +	}
>>> +
>>> +	return ret;
>>>  }
>>
>> This makes me wonder: if we called `refs_create_refdir_stubs()` before
>> we call `->create_on_disk()`, could we even do it for the "files"
>> backend? Just a thought though.
>>
>
> Well, there is some nuance there
>
> 1. 'refs/heads', 'refs/tags' is not created for linked worktrees.

I'm a little bit confused what you mean here? Would it be a problem if
it *is* created?

> 2. 'HEAD' is only created lazily, not in `create_on_disk()`.

Okay, seems like a valid argument to me. You don't want to have
`refs/HEAD` created with `ref: refs/heads/.invalid`?

> Also the intent is totally different, the stubs are for backward
> compatibility. So I think its better to let that logic stay within the
> files-backend.

That's mainly because you named the function like this, but it doesn't
have to be named like that.

>> For symmetry it would be nice to not have an early return here, but also
>> format the condition for this block in the same way as we have it for
>> `ref_store_create_on_disk()`.
>>
>> Patrick
>
> Yeah sure, we can do that here, in the last commit, we'll have to modify
> that anyway back to something like this. But it definitely would be
> easier to review this commit. Will add.

:+1:

-- 
Cheers,
Toon