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

[Stefan Beller <sbeller@google.com>]

On Tue, Oct 4, 2016 at 10:34 AM, Jeff King <peff@peff.net> wrote:
> 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.

>>
>> +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