Re: [PATCH v2 08/15] unpack-trees: don't respect submodule.update

From: Stefan Beller <sbeller@google.com>
Date: Aug 3, 2017, 20:43 UTC
Message-ID: <CAGZ79ka8YeW0ChP3Z3xjfV0r0aqg2sGpLDU5m5LGWyG9QED0Uw@mail.gmail.com>
In-Reply-To: <xmqqpoccwfpl.fsf@gitster.mtv.corp.google.com>

On Thu, Aug 3, 2017 at 1:37 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 12 quoted lines

> Brandon Williams <bmwill@google.com> writes:
>
>> The 'submodule.update' config was historically used and respected by the
>> 'submodule update' command because update handled a variety of different
>> ways it updated a submodule.  As we begin teaching other commands about
>> submodules it makes more sense for the different settings of
>> 'submodule.update' to be handled by the individual commands themselves
>> (checkout, rebase, merge, etc) so it shouldn't be respected by the
>> native checkout command.
>
> Soooo... what's the externally observable effect of this change?  Is
> it something that can be illustrated in a set of new tests?

The illustration can be as follows

    git config submodule.NAME.update none
    git checkout -f --recurse-submodules HEAD
    git status
    # observe dirty submodule, which is
    # not what checkout -f promises

Show 6 quoted lines

> IOW does this commit by itself want to change the behaviour of
> "submodule update" and existing (indirect) users of unpack-trees?
> Or does it want to keep the documented behaviour of "submodule
> update" while correcting unintended triggering in other (indirect)
> users of unpack-trees of the same machinery that is being removed in
> this patch?

"submodule update" is unaffected, only the recently introduced submodule awareness of checkout/reset/read-tree are changed.

This option is documented 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).

which doesn't indicate that any other command apart from "submodule update" should respect it.

Show 28 quoted lines

>
>> -     switch (sub->update_strategy.type) {
>> -     case SM_UPDATE_UNSPECIFIED:
>> -     case SM_UPDATE_CHECKOUT:
>> -             if (submodule_move_head(ce->name, old_id, new_id, flags))
>> -                     return o->gently ? -1 :
>> -                             add_rejected_path(o, ERROR_WOULD_LOSE_SUBMODULE, ce->name);
>> -             return 0;
>> -     case SM_UPDATE_NONE:
>> -             return 0;
>> -     case SM_UPDATE_REBASE:
>> -     case SM_UPDATE_MERGE:
>> -     case SM_UPDATE_COMMAND:
>> -     default:
>> -             warning(_("submodule update strategy not supported for submodule '%s'"), ce->name);
>> -             return -1;
>> -     }
>> +     if (submodule_move_head(ce->name, old_id, new_id, flags))
>> +             return o->gently ? -1 :
>> +                                add_rejected_path(o, ERROR_WOULD_LOSE_SUBMODULE, ce->name);
>> +     return 0;
>
> With this update, we always behave as if update_strategy.type were
> either left unspecified or explicitly set to checkout.  Other arms
> in this switch (and the other switch too), especially "none", were
> not expecting a call to submodule_move_head() to be made, but now
> the call is unconditional.
>

Yes. This is because each command (reset/checkout) should provide one expected behavior. It is not that we can configure reset to omit certain (tracked) files from being reset?