On Tue, Dec 27, 2016 at 10:17 AM, Stefan Beller <[email protected]> wrote:
> 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 ..."));
>
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;
