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

On Tue, Dec 27, 2016 at 10:17 AM, Stefan Beller [off-list ref] wrote:

On Mon, Dec 26, 2016 at 5:10 PM, Junio C Hamano [off-list ref] wrote:

quoted

Stefan Beller [off-list ref] writes:

quoted

@@ -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);

quoted

quoted

@@ -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;