From: Glen Choo Date: Wed, 16 Mar 2022 22:37:07 GMT Subject: Re: [PATCH v5 00/10] fetch --recurse-submodules: fetch unpopulated submodules Message-ID: In-Reply-To: Junio C Hamano writes: > Glen Choo writes: > >> Glen Choo writes: >> >>> Junio C Hamano writes: >>> >>>> Glen Choo writes: >>>> >>>>> To clarify, does this opinion of "don't use config values that aren't >>>>> copied into .git/config" extend to in-tree .gitmodules? Prior to this >>>>> series, we always read the in-tree .gitmodules to get the config - the >>>>> user does not need to copy the settings to .git/config, but we don't >>>>> pick a commit to read .gitmodules from. >>>> >>>> I think we do, but I also think it was a huge mistake to allow >>>> repository data to directly affect the behaviour of local checkout. >>> >>> I'm inclined to agree. >>> >>>> Fixing that is most likely outside the scope of this series, though. >>> >>> Agree. Thanks! >> >> I thought that this would have been the end of the discussion, but after >> reading , I guess I had the wrong impression >> (oops). >> >> If I am reading everything correctly, we both agree that it's not >> good to read _any_ config values from .gitmodules (even if it's >> in-tree), and that we should clean it up outside of this topic. So for >> this topic to be merged into 'next', is it enough to say that I will fix >> this behavior in a follow up topic? > > At least we should remember that is something to be fixed. It may > not be you personally who addresses that issue, though ;-) Perhaps squashing in a NEEDSWORK comment into [PATCH v5 09/10] will suffice? I can also resend this series if preferred. ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ---- diff --git a/submodule.c b/submodule.c index 6e6b2d04e4..93c78a4dc3 100644 --- a/submodule.c +++ b/submodule.c @@ -795,6 +795,21 @@ static const char *default_name_or_path(const char *path_or_name) * superproject commit that points to the submodule, but this is * arbitrary - we can choose any (super_oid, path) that matches the * submodule's name. + * + * NEEDSWORK: Storing an arbitrary commit is undesirable because we can't + * guarantee that we're reading the commit that the user would expect. A better + * scheme would be to just fetch a submodule by its name. This requires two + * steps: + * - Create a function that behaves like repo_submodule_init(), but accepts a + * submodule name instead of treeish_name and path. This should be easy + * because repo_submodule_init() internally uses the submodule's name. + * + * - Replace most instances of 'struct submodule' (which is the .gitmodules + * config) with just the submodule name. This is OK because we expect + * submodule settings to be stored in .git/config (via "git submodule init"), + * not .gitmodules. This also lets us delete get_non_gitmodules_submodule(), + * which constructs a bogus 'struct submodule' for the sake of giving a + * placeholder name to a gitlink. */ struct changed_submodule_data { /*