Re: [PATCH] push: change submodule default to check

From: Stefan Beller <sbeller@google.com>
Date: Oct 4, 2016, 17:48 UTC
Message-ID: <CAGZ79kaKOjqLBsNVSmudzLUCkOJf_CsFGE8OZZHsTmuXCfiVeg@mail.gmail.com>
In-Reply-To: <20161004173430.eax4ptohyonc5bw2@sigill.intra.peff.net>

On Tue, Oct 4, 2016 at 10:34 AM, Jeff King <peff@peff.net> wrote:

Show 15 quoted lines

> On Tue, Oct 04, 2016 at 09:40:36AM -0700, Stefan Beller wrote:
>
>> >> Why should we even have a default different from today's?  If most
>> >> repositories don't have submodules enabled at all, we can just let
>> >> those working with submodules enabled to toggle their configuration
>> >> and that is an very easy to understand solution, no?
>> >
>> > You will not see any complaint from me on that. I was taking for granted
>> > that the current default is inconvenient to submodule users, but I don't
>> > have any experience myself.
>> >
>>
>> And there I was trying to help submodule users not shoot in their foot.
>
> Sorry if my reply came off as snarky.

Yeah, sorry about starting being snarky here.

Show 13 quoted lines

>>
>> +static void preset_submodule_default(void)
>> +{
>> +     if (file_exists(".gitmodules"))
>> +             recurse_submodules = RECURSE_SUBMODULES_CHECK;
>> +     else
>> +             recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
>> +}
>
> This does seem like a reasonable heuristic. I wonder if you want to
> confirm that we actually have a worktree (and are in it) before looking
> at file_exists(). It's unlikely that looking at ".gitmodules" in a bare
> repo would trigger in practice, but it does not hurt to be careful.

In a bare repo we'd rather want to check for an entry of .gitmodules in HEAD ?

I considered it a non issue, as I don't think many people push from bare repositories. So if we were to check in the HEAD tree instead of the file system, why would we apply different rules for bare and non bare repositories? We probably would not want to do that, so is it reasonable to check for the .gitmodules in the HEAD tree in general in the non bare case? I dunno, it sounds like an equally cheap heuristic covering most cases.

Here is another thought: .gitmodules may not exist (either in working dir or in HEADs git tree), so maybe the "correct" heuristic is to check for directories in $GIT_DIR/modules/ That is "more correct", because it is inconceivable to change the submodule pointers without having the submodules checked out. (Who would do that? Why?)

>
> -Peff