Re: [PATCH v4 14/15] files-backend: remove submodule_allowed from files_downcast()

From: Duy Nguyen <pclouds@gmail.com>
Date: Feb 20, 2017, 12:21 UTC
Message-ID: <CACsJy8AZ27O-pxTqHOzYXRBuyv8dkxdGJ_5Z0u3eaxkNdnaEYA@mail.gmail.com>
In-Reply-To: <25fcb527-595a-7865-41e3-ee7c4c1ad668@alum.mit.edu>

On Mon, Feb 20, 2017 at 7:11 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:

Show 26 quoted lines

> On 02/18/2017 02:33 PM, Nguyễn Thái Ngọc Duy wrote:
>> Since submodule or not is irrelevant to files-backend after the last
>> patch, remove the parameter from files_downcast() and kill
>> files_assert_main_repository().
>>
>> PS. This implies that all ref operations are allowed for submodules. But
>> we may need to look more closely to see if that's really true...
>
> I think you are jumping the gun here.
>
> Even after your changes, there is still a significant difference between
> the main repository and submodules: we have access to the object
> database for the main repository, but not for submodules.
>
> So, for example, the following don't work for submodules:
>
> * `parse_object()` is used to ensure that references are only pointed at
> valid objects, and that branches are only pointed at commit objects.
>
> * `peel_object()` is used to write the peeled version of references in
> `packed-refs`, and to peel references while they are being iterated
> over. Since this doesn't work, I think you can't write `packed-refs` in
> a submodule.
>
> These limitations, I think, imply that submodule references have to be
> treated as read-only.

Behind the scene submodule does add_submodule_odb(), which basically makes the submodule's odb an alternate of in-core odb. So odb access works. I was puzzled how submodules could by pass odb access at the beginning only to find that out. technical/api-ref-iteration.txt also mentions that you need to add_submodule_odb(), so I think it's deliberate (albeit hacky) design.

Show 5 quoted lines

> When I was working on a patch series similar to yours, I introduced a
> boolean "main_repository" flag in `struct ref_store`, and use that
> member to implement `files_assert_main_repository()`. That way
> `files_ref_store::submodule` can still be removed, which is the more
> important goal from a design standpoint.

I could keep the submodule check back (and replace the submodule string in files_ref_store with just a flag). But I really think all backend functions work with submodule. Perhaps add some tests to exercise/verify that files-backend-on-submodule works?

-- 
Duy