On Tue, Feb 21, 2017 at 3:35 PM, Jacob Keller [off-list ref] wrote:

On Tue, Feb 21, 2017 at 2:16 PM, Stefan Beller [off-list ref] wrote:

quoted

On Fri, Feb 17, 2017 at 10:42 AM, Jacob Keller [off-list ref] wrote:

quoted

On Wed, Feb 15, 2017 at 4:38 PM, Stefan Beller [off-list ref] wrote:

quoted

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

quoted

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.

quoted

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