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

[Stefan Beller <sbeller@google.com>]

On Tue, Oct 4, 2016 at 12:39 PM, Jeff King <peff@peff.net> wrote:
> 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.

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


>                 *(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