Re: [RFC v2 PATCH] Teach rm to remove submodules unless they contain a git directory

Am 27.08.2012 22:59, schrieb Junio C Hamano:

Jens Lehmann [off-list ref] writes:

quoted

+{
+	int i;
+	int errs = 0;
+
+	for (i = 0; i < list.nr; i++) {
+		const char *name = list.entry[i].name;
+		int pos;
+		struct cache_entry *ce;
+		struct stat st;
+
+		pos = cache_name_pos(name, strlen(name));
+		if (pos < 0)
+			continue; /* ignore unmerged entry */

Would this cause "git rm -f path" for an unmerged submodule bypass
the safety check?

Oops, thanks for spotting that. So replacing the "continue;" with
"pos = -pos-1;" should do the trick here, right? Will add some
tests for unmerged submodules ...

quoted

+		ce = active_cache[pos];
+
+		if (!S_ISGITLINK(ce->ce_mode) ||
+		    (lstat(ce->name, &st) < 0) ||
+		    is_empty_dir(name))
+			continue;
+
+		if (!submodule_uses_gitfile(name))
+			errs = error(_("submodule '%s' (or one of its nested "
+				     "submodules) uses a .git directory\n"
+				     "(use 'rm -rf' if you really want to remove "
+				     "it including all of its history)"), name);
+	}
+
+	return errs;
+}

The call to this function comes at the very end and gives us yes/no
for the entire set of paths.  After getting this error for one
submodule and bunch of other non-submodule paths, what is the
procedure for the user to remove it that we want to recommend in our
documentation?  Would it go like this?

	$ git rm path1 path2 sub path3
	... get the above error ...
	$ git submodule --to-gitfile sub
        $ rm -fr sub
        $ git rm sub
        ... then finally ...
        $ git rm path1 path2 path3

With current git I'd recommend:

 	$ git rm path1 path2 sub path3
 	... get the above error ...
        $ rm -fr sub
 	... try again ...
        $ git rm path1 path2 sub path3

Maybe I should add the hint to repeat the git rm after removing the
submodule to the error output?

Once we implemented "git submodule --to-gitfile" it could be used
instead of "rm -fr sub" to preserve the submodule's repo if the user
wants to.

BTW: I added the same message twice, here for the forced case and in
check_local_mod() when not forced. Is there a recommended way to assign
a localized message to a static variable, so I could define it only once
and reuse it?

quoted

@@ -80,8 +116,11 @@ static int check_local_mod(unsigned char *head, int index_only)

 		/*
 		 * 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))
+		if (ce_match_stat(ce, &st, 0) ||
+		    (S_ISGITLINK(ce->ce_mode) &&
+		     !ok_to_remove_submodule(ce->name)))
 			local_changes = 1;

As noted before, because we also skip these "does it match the
index?  does it match the HEAD?" checks for unmerged paths in this
function, a submodule that has local changes or new files is
eligible for removal during a conflicted merge.  I have a feeling
that this should be tightened a bit; wouldn't we want to check at
least in the checked out version (i.e. stage #2 in the index) if the
path were a submodule, even if we are in the middle of a conflicted
merge?  After all, the top level merge shouldn't have touched the
submodule working tree, so the local modes and new files must have
come from the end user action that was done _before_ the conflicted
merge started, and not expendable, no?

Right, I'll change that.