Re: [PATCH v5 00/10] fetch --recurse-submodules: fetch unpopulated submodules

From

Glen Choo <chooglen@google.com>

Date

Mar 16, 2022, 22:37 UTC

Message-ID

<kl6l7d8tle24.fsf@chooglen-macbookpro.roam.corp.google.com>

In-Reply-To

<xmqqfsnhk0x7.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

> Glen Choo <chooglen@google.com> writes:
>
>> Glen Choo <chooglen@google.com> writes:
>>
>>> Junio C Hamano <gitster@pobox.com> writes:
>>>
>>>> Glen Choo <chooglen@google.com> 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 <xmqqa6dpllmc.fsf@gitster.g>, 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 ;-)

----- >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 {
 	/*