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

[Stefan Beller <sbeller@xxxxxxxxxx>]

On Mon, Dec 26, 2016 at 5:10 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Stefan Beller <sbeller@xxxxxxxxxx> 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 ..."));

I think for clarity we could drop the is_submodule bit and only
and directly do

    if (S_ISGITLINK(ce->ce_mode) &&
      !is_staging_gitmodules_ok())
        die (_("Please stage ..."));

and below (the code that you quoted) also just check via
    S_ISGITLINK(ce->ce_mode)

Thanks,
Stefan