Re: [PATCHv5 4/4] rm: absorb a submodules git dir before deletion

[Stefan Beller <sbeller@google.com>]

On Tue, Dec 27, 2016 at 10:17 AM, Stefan Beller <sbeller@google.com> wrote:
> On Mon, Dec 26, 2016 at 5:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> @@ -342,6 +313,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>>>                       exit(0);
>>>       }
>>>
>>> +     submodules_absorb_gitdir_if_needed(prefix);
>>> +
>>>       /*
>>>        * If not forced, the file, the index and the HEAD (if exists)
>>>        * must match; but the file can already been removed, since
>>> @@ -358,9 +331,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>>>                       oidclr(&oid);
>>>               if (check_local_mod(&oid, index_only))
>>>                       exit(1);
>>> -     } else if (!index_only) {
>>> -             if (check_submodules_use_gitfiles())
>>> -                     exit(1);
>>>       }
>>>
>>
>> Hmph.  It may be a bit strange to see an "index-only" remove to
>> touch working tree, no?  Yet submodules_absorb_gitdir_if_needed() is
>> unconditionally called above, which feels somewhat unexpected.
>
> I agree. In a reroll I'll protect the call with
>
>     if (!index_only)
>         submodules_absorb_gitdir_if_needed(prefix);
>
>>
>>> @@ -389,32 +359,20 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>>>        */
>>>       if (!index_only) {
>>>               int removed = 0, gitmodules_modified = 0;
>>>               for (i = 0; i < list.nr; i++) {
>>>                       const char *path = list.entry[i].name;
>>>                       if (list.entry[i].is_submodule) {
>>> +                             struct strbuf buf = STRBUF_INIT;
>>> +
>>> +                             strbuf_addstr(&buf, path);
>>> +                             if (remove_dir_recursively(&buf, 0))
>>> +                                     die(_("could not remove '%s'"), path);
>>> +                             strbuf_release(&buf);
>>> +
>>> +                             removed = 1;
>>> +                             if (!remove_path_from_gitmodules(path))
>>> +                                     gitmodules_modified = 1;
>>> +                             continue;
>>>                       }
>>
>> I do not see any behaviour change from the original (not quoted
>> here), but it is somewhat surprising that "git rm ./submodule" does
>> not really check if the submodule has local modifications and files
>> that are not even added before remove_dir_recursively() is called.
>>
>> Or perhaps I am reading the code incorrectly and such a check is
>> done elsewhere?
>
> When determining if the entry is a submodule (i.e. setting
> the is_submodule bit) we have
>
>     list.entry[list.nr].is_submodule = S_ISGITLINK(ce->ce_mode);
>     if (list.entry[list.nr++].is_submodule &&
>       !is_staging_gitmodules_ok())
>         die (_("Please stage ..."));
>

Well scratch that.
is_staging_gitmodules_ok only checks for the .gitmodules file and not
for the submodule itself (the submodule is not an argument to that function)

The actual answer is found in check_local_mod called via

    if (!force) {
        struct object_id oid;
        if (get_oid("HEAD", &oid))
            oidclr(&oid);
        if (check_local_mod(&oid, index_only))
            exit(1);
    }

check_local_mod was touched in the previous patch as it has:

    /*
    * Is the index different from the file in the work tree?
    * If it's a submodule, is its work tree modified?
    */
    if (ce_match_stat(ce, &st, 0) ||
      (S_ISGITLINK(ce->ce_mode) &&
       bad_to_remove_submodule(ce->name,
            SUBMODULE_REMOVAL_DIE_ON_ERROR |
            SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED)))
                local_changes = 1;