Re: [PATCH 12/15] unpack-trees: check if we can perform the operation for submodules

[Stefan Beller <sbeller@google.com>]

On Tue, Feb 21, 2017 at 3:35 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Tue, Feb 21, 2017 at 2:16 PM, Stefan Beller <sbeller@google.com> wrote:
>> On Fri, Feb 17, 2017 at 10:42 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
>>> On Wed, Feb 15, 2017 at 4:38 PM, Stefan Beller <sbeller@google.com> wrote:
>>>> +       if (is_active_submodule_with_strategy(ce, SM_UPDATE_UNSPECIFIED))
>>>
>>> Here, and in other cases where we use
>>> is_active_submodule_with_strategy(), why do we only ever check
>>> SM_UPDATE_UNSPECIFIED? It seems really weird that we're only going to
>>> check submodules who's strategy is unspecified, when that defaults to
>>> checkout if I recall correctly? Shouldn't we check both? This applies
>>> to pretty much everywhere that you call this function that I noticed,
>>> which is why I removed the context.
>>
>> I am torn between this.
>>
>> submodule.<name>.update = {rebase, merge, checkout, none !command}
>> is currently documented in GIT-CONFIG(1) as
>>
>>        submodule.<name>.update
>>            The default update procedure for a submodule. This variable is
>>            populated by git submodule init from the gitmodules(5) file. See
>>            description of update command in git-submodule(1).
>>
>> and in GIT-SUBMODULE(1) as
>>
>>        update
>>            [...] can be done in several ways
>>            depending on command line options and the value of
>>            submodule.<name>.update configuration variable. Supported update
>>            procedures are:
>>
>>            checkout
>>                [...] or no option is given, and
>>                submodule.<name>.update is unset, or if it is set to checkout.
>>
>> So the "update" config clearly only applies to the "submodule update"
>> command, right?
>>
>> Well no, "checkout --recurse-submodules" is very similar
>> to running "submodule update", except with a bit more checks, so you could
>> think that such an option applies to checkout as well. (and eventually
>> rebase/merge etc. are supported as well.)
>>
>> So initially I assumed both "unspecified" as well as "checkout"
>> are good matches to support in the first round.
>>
>> Then I flip flopped to think that we should not interfere with these
>> settings at all (The checkout command does checkout and checkout only;
>> no implicit rebase/merge ever in the future, because that would be
>> confusing). So ignoring that option seemed like the way to go.
>
> Hmm. So it's a bit complicated.
>
>>
>> But ignoring that option is also not the right approach.
>> What if you have set it to "none" and really *expect* Git to not touch
>> that submodule?
>
> Or set it to "rebase" and suddenly git-checkout is ignoring you and
> just checking things out anyways.
>
>>
>> So I dunno. Maybe it is a documentation issue, we need to spell out
>> in the man page for checkout that --recurse-submodules is
>> following one of these models. Now which is the best default model here?
>
> Personally, I would go with that the config option sets the general
> strategy used by the submodule whenever its updated, regardless of
> how.
>
> So, for example, setting it to none, means that recurse-submoduls will
> ignore it when checking out. Setting it to rebase, or merge, and the
> checkout will try to do those things?

That is generally a sound idea when it comes to git-checkout.

What about other future things like git-revert?
(Ok I already brought up this example too many times; it should have
a revert-submodules as well switch, which is neither of the current strategies,
so we'd have to invent a new strategy and make that the default for
revert. That strategy would make no sense in any other command though)

What about "git-rebase --recurse-submodules"?
Should git-rebase merge the submodules when it is configured to "merge"
Or just "checkout" (the possibly non-fast-forward-y old sha1) ?

The only sane option IMO is "rebase" as well in the submodules, rewriting
the submodule pointers in the rebased commits in the superproject.

>
> Or, if that's not really feasible, have the checkout go "hey.. you
> asked me to recurse, but uhhh these submodules don't allow me to do
> checkout, so I'm gonna fail"? I think that's the best approach for
> now.

So you'd propose to generally use the submodule.<name>.update
strategies with aggressive error-out but also keeping in mind
that the strategies might grow by a lot in the future (well only revert
comes to mind here).

ok, let's do that then.

Thanks,
Stefan