On Mon, Dec 26, 2016 at 5:10 PM, Junio C Hamano <[email protected]> wrote:
> Stefan Beller <[email protected]> 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
