Re: [PATCHv2 1/2] push: change submodule default to check when submodules exist

From: Stefan Beller <sbeller@google.com>
Date: Oct 4, 2016, 19:51 UTC
Message-ID: <CAGZ79kbt+SZoogKTV_-rVfOOFzf6xrhWytrBo2H3r6NQw34WTw@mail.gmail.com>
In-Reply-To: <20161004193926.32w7yivkakqoadm2@sigill.intra.peff.net>

On Tue, Oct 4, 2016 at 12:39 PM, Jeff King <peff@peff.net> wrote:

Show 16 quoted lines

> On Tue, Oct 04, 2016 at 12:29:09PM -0700, Stefan Beller wrote:
>
>> Jeff wrote:
>> > Consulting .git/config is fine, I think. It's not like we don't read it
>> > (sometimes multiple times!) during the normal course of the program
>> > anyway. It's just a question of whether it makes more sense for the
>> > heuristic to kick in after "init", or only after "update". I don't know
>> > enough to have an opinion.
>>
>> I think there is no difference in practice, however the "after update"
>> is way easier to implement and hence more maintainable (one lstat instead of
>> fiddeling with the config; that can go wrong easily).
>
> Hmm, I would have thought it is the opposite; can't submodules exist in
> the working tree in their own ".git" directory? I know that's the "old"
> way of doing it, but I didn't know if it was totally deprecated.

Oo, right. :(

The proposed patch would not change the current behavior for a layout of .git directories inside the submodule working trees, actually. It would however also not have the desired effect of enabling the check for push.

However these not-yet-deprecated layouts are likely in use by people who know what they are doing, so maybe we can punt on that.

Show 6 quoted lines

>
> Anyway, the config version is probably just:
>
>   int config_check_submodule(const char *var, const char *value, void *data)
>   {
>         if (starts_with(var, "submodule.") && ends_with(var, ".path"))

s/.path/.url/ but I get the point. I do dislike this solution for another reasons though:

In the future when worktree supports submodules we either have the url per worktree,so we'd need to process all working tree configs as well or we do it the proper way, which is replacing the submodule.$name.url variable with 2 options, one is purely used to configure the URL and the other is purely used to indicate the existence of a submodule. Piling on the mixed use case of urls today feels sad.

Show 24 quoted lines

>                 *(int *)data = 1;
>         return 0;
>   }
>
>   ...
>   int have_submodule = 0;
>   git_config(config_check_submodule, &have_submodule);
>
> But I don't care too much either way. that's just for reference.
>
>> @@ -31,6 +32,19 @@ static const char **refspec;
>>  static int refspec_nr;
>>  static int refspec_alloc;
>>
>> +static void preset_submodule_default(void)
>> +{
>> +     struct strbuf sb = STRBUF_INIT;
>> +     strbuf_addf(&sb, "%s/modules", get_git_dir());
>> +
>> +     if (file_exists(sb.buf))
>
> Maybe just:
>
>   if (file_exists(git_path("modules"))

Sounds good.

So I'll see if I can get the version running you propose here, otherwise I'll resend with these changes.

>
> ?
>
> -Peff