[PATCHv5 0/4] git-rm absorbs submodule git directory before deletion

[PATCHv5 0/4] git-rm absorbs submodule git directory before deletion

[Stefan Beller]

v5:
* removed the patch for adding {run,start,finish}_or_die
* added one more flag to the function ok_to_remove_submodule
  (if die on error is ok)
* renamed ok_to_remove_submodule to bad_to_remove_submodule to signal
  the error case better.

v4:
* reworded commit messages of the last 2 patches
* introduced a new patch introducing {run,start,finish}_command_or_die
* found an existing function in dir.h to use to remove a directory
  which deals gracefully with the cornercases, that Junio pointed out.
  
v3:
* removed the patch to enhance ok_to_remove_submodule to absorb the submodule
  if needed
* Removed all the error reporting from git-rm that was related to submodule
  git directories not absorbed.
* instead just absorb the git repositories or let the absorb function die
  with an appropriate error message.

v2:
* new base where to apply the patch:
  sb/submodule-embed-gitdir merged with sb/t3600-cleanup.
  I got merge conflicts and resolved them this way:
#@@@ -709,9 -687,10 +687,9 @@@ test_expect_success 'checking out a com
#          git commit -m "submodule removal" submod &&
#          git checkout HEAD^ &&
#          git submodule update &&
#-         git checkout -q HEAD^ 2>actual &&
#+         git checkout -q HEAD^ &&
#          git checkout -q master 2>actual &&
# -        echo "warning: unable to rmdir submod: Directory not empty" >expected &&
# -        test_i18ncmp expected actual &&
# +        test_i18ngrep "^warning: unable to rmdir submod:" actual &&
#          git status -s submod >actual &&
#          echo "?? submod/" >expected &&
#          test_cmp expected actual &&
#

* improved commit message in "ok_to_remove_submodule: absorb the submodule git dir"
  (David Turner offered me some advice on how to write better English off list)
* simplified code in last patch:
  -> dropped wrong comment for fallthrough
  -> moved redundant code out of both bodies of an if-clause.
* Fixed last patchs commit message to have "or_die" instead of or_dir.

v1:
The "checkout --recurse-submodules" series got too large to comfortably send
it out for review, so I had to break it up into smaller series'; this is the
first subseries, but it makes sense on its own.

This series teaches git-rm to absorb the git directory of a submodule instead
of failing and complaining about the git directory preventing deletion.

It applies on origin/sb/submodule-embed-gitdir.

Any feedback welcome!

Thanks,
Stefan


Stefan Beller (4):
  submodule.h: add extern keyword to functions
  submodule: modernize ok_to_remove_submodule to use argv_array
  submodule: rename and add flags to ok_to_remove_submodule
  rm: absorb a submodules git dir before deletion

 builtin/rm.c  | 83 +++++++++++++++--------------------------------------------
 submodule.c   | 57 ++++++++++++++++++++++++++--------------
 submodule.h   | 59 ++++++++++++++++++++++++------------------
 t/t3600-rm.sh | 39 +++++++++++-----------------
 4 files changed, 108 insertions(+), 130 deletions(-)

-- 
2.11.0.rc2.53.gb7b3fba.dirty

[PATCHv5 1/4] submodule.h: add extern keyword to functions

[Stefan Beller]

As the upcoming series will add a lot of functions to the submodule
header, let's first make the header consistent to the rest of the project
by adding the extern keyword to functions.

As per the CodingGuidelines we try to stay below 80 characters per line,
so adapt all those functions to stay below 80 characters that are already
using more than one line.  Those function using just one line are better
kept in one line than breaking them up into multiple lines just for the
goal of staying below the character limit as it makes grepping
for functions easier if they are one liners.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.h | 55 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 30 insertions(+), 25 deletions(-)

diff --git a/submodule.h b/submodule.h
index 6229054b99..61fb610749 100644
--- a/submodule.h
+++ b/submodule.h
@@ -29,50 +29,55 @@ struct submodule_update_strategy {
 };
 #define SUBMODULE_UPDATE_STRATEGY_INIT {SM_UPDATE_UNSPECIFIED, NULL}
 
-int is_staging_gitmodules_ok(void);
-int update_path_in_gitmodules(const char *oldpath, const char *newpath);
-int remove_path_from_gitmodules(const char *path);
-void stage_updated_gitmodules(void);
-void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
+extern int is_staging_gitmodules_ok(void);
+extern int update_path_in_gitmodules(const char *oldpath, const char *newpath);
+extern int remove_path_from_gitmodules(const char *path);
+extern void stage_updated_gitmodules(void);
+extern void set_diffopt_flags_from_submodule_config(struct diff_options *,
 		const char *path);
-int submodule_config(const char *var, const char *value, void *cb);
-void gitmodules_config(void);
-int parse_submodule_update_strategy(const char *value,
+extern int submodule_config(const char *var, const char *value, void *cb);
+extern void gitmodules_config(void);
+extern int parse_submodule_update_strategy(const char *value,
 		struct submodule_update_strategy *dst);
-const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);
-void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);
-void show_submodule_summary(FILE *f, const char *path,
+extern const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);
+extern void handle_ignore_submodules_arg(struct diff_options *, const char *);
+extern void show_submodule_summary(FILE *f, const char *path,
 		const char *line_prefix,
 		struct object_id *one, struct object_id *two,
 		unsigned dirty_submodule, const char *meta,
 		const char *del, const char *add, const char *reset);
-void show_submodule_inline_diff(FILE *f, const char *path,
+extern void show_submodule_inline_diff(FILE *f, const char *path,
 		const char *line_prefix,
 		struct object_id *one, struct object_id *two,
 		unsigned dirty_submodule, const char *meta,
 		const char *del, const char *add, const char *reset,
 		const struct diff_options *opt);
-void set_config_fetch_recurse_submodules(int value);
-void check_for_new_submodule_commits(unsigned char new_sha1[20]);
-int fetch_populated_submodules(const struct argv_array *options,
+extern void set_config_fetch_recurse_submodules(int value);
+extern void check_for_new_submodule_commits(unsigned char new_sha1[20]);
+extern int fetch_populated_submodules(const struct argv_array *options,
 			       const char *prefix, int command_line_option,
 			       int quiet, int max_parallel_jobs);
-unsigned is_submodule_modified(const char *path, int ignore_untracked);
-int submodule_uses_gitfile(const char *path);
-int ok_to_remove_submodule(const char *path);
-int merge_submodule(unsigned char result[20], const char *path, const unsigned char base[20],
-		    const unsigned char a[20], const unsigned char b[20], int search);
-int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name,
-		struct string_list *needs_pushing);
-int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);
-int parallel_submodules(void);
+extern unsigned is_submodule_modified(const char *path, int ignore_untracked);
+extern int submodule_uses_gitfile(const char *path);
+extern int ok_to_remove_submodule(const char *path);
+extern int merge_submodule(unsigned char result[20], const char *path,
+			   const unsigned char base[20],
+			   const unsigned char a[20],
+			   const unsigned char b[20], int search);
+extern int find_unpushed_submodules(unsigned char new_sha1[20],
+				    const char *remotes_name,
+				    struct string_list *needs_pushing);
+extern int push_unpushed_submodules(unsigned char new_sha1[20],
+				    const char *remotes_name);
+extern void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
+extern int parallel_submodules(void);
 
 /*
  * Prepare the "env_array" parameter of a "struct child_process" for executing
  * a submodule by clearing any repo-specific envirionment variables, but
  * retaining any config in the environment.
  */
-void prepare_submodule_repo_env(struct argv_array *out);
+extern void prepare_submodule_repo_env(struct argv_array *out);
 
 #define ABSORB_GITDIR_RECURSE_SUBMODULES (1<<0)
 extern void absorb_git_dir_into_superproject(const char *prefix,
-- 
2.11.0.rc2.53.gb7b3fba.dirty

[PATCHv5 2/4] submodule: modernize ok_to_remove_submodule to use argv_array

[Stefan Beller]

Instead of constructing the NULL terminated array ourselves, we
should make use of the argv_array infrastructure.

While at it, adapt the error messages to reflect the actual invocation.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/submodule.c b/submodule.c
index 45ccfb7ab4..9f0b544ebe 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1023,13 +1023,6 @@ int ok_to_remove_submodule(const char *path)
 {
 	ssize_t len;
 	struct child_process cp = CHILD_PROCESS_INIT;
-	const char *argv[] = {
-		"status",
-		"--porcelain",
-		"-u",
-		"--ignore-submodules=none",
-		NULL,
-	};
 	struct strbuf buf = STRBUF_INIT;
 	int ok_to_remove = 1;
 
@@ -1039,14 +1032,15 @@ int ok_to_remove_submodule(const char *path)
 	if (!submodule_uses_gitfile(path))
 		return 0;
 
-	cp.argv = argv;
+	argv_array_pushl(&cp.args, "status", "--porcelain", "-u",
+				   "--ignore-submodules=none", NULL);
 	prepare_submodule_repo_env(&cp.env_array);
 	cp.git_cmd = 1;
 	cp.no_stdin = 1;
 	cp.out = -1;
 	cp.dir = path;
 	if (start_command(&cp))
-		die("Could not run 'git status --porcelain -uall --ignore-submodules=none' in submodule %s", path);
+		die(_("could not run 'git status --porcelain -u --ignore-submodules=none' in submodule %s"), path);
 
 	len = strbuf_read(&buf, cp.out, 1024);
 	if (len > 2)
@@ -1054,7 +1048,7 @@ int ok_to_remove_submodule(const char *path)
 	close(cp.out);
 
 	if (finish_command(&cp))
-		die("'git status --porcelain -uall --ignore-submodules=none' failed in submodule %s", path);
+		die(_("'git status --porcelain -u --ignore-submodules=none' failed in submodule %s"), path);
 
 	strbuf_release(&buf);
 	return ok_to_remove;
-- 
2.11.0.rc2.53.gb7b3fba.dirty

[PATCHv5 3/4] submodule: rename and add flags to ok_to_remove_submodule

[Stefan Beller]

In different contexts the question "Is it ok to delete a submodule?"
may be answered differently.

In 293ab15eea (submodule: teach rm to remove submodules unless they
contain a git directory, 2012-09-26) a case was made that we can safely
ignore ignored untracked files for removal as we explicitely ask for the
removal of the submodule.

In a later patch we want to remove submodules even when the user doesn't
explicitly ask for it (e.g. checking out a tree-ish in which the submodule
doesn't exist).  In that case we want to be more careful when it comes
to deletion of untracked files. As of this patch it is unclear how this
will be implemented exactly, so we'll offer flags in which the caller
can specify how the different untracked files ought to be handled.

As the flags allow the function to not die on an error when spawning
a child process, we need to find an appropriate return code for the
case when the child process could not be started. As in that case we
cannot tell if the submodule is ok to remove, we'd want to return 'false'.

As only 0 is understood as false, rename the function to invert the
meaning, i.e. the return code of 0 signals the removal of the submodule
is fine, and other values can be used to return a more precise answer
what went wrong.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/rm.c |  4 +++-
 submodule.c  | 49 +++++++++++++++++++++++++++++++++++++------------
 submodule.h  |  6 +++++-
 3 files changed, 45 insertions(+), 14 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index 3f3e24eb36..5a5a66272b 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -187,7 +187,9 @@ static int check_local_mod(struct object_id *head, int index_only)
 		 */
 		if (ce_match_stat(ce, &st, 0) ||
 		    (S_ISGITLINK(ce->ce_mode) &&
-		     !ok_to_remove_submodule(ce->name)))
+		     bad_to_remove_submodule(ce->name,
+				SUBMODULE_REMOVAL_DIE_ON_ERROR |
+				SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED)))
 			local_changes = 1;
 
 		/*
diff --git a/submodule.c b/submodule.c
index 9f0b544ebe..1cc04d24e5 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1019,39 +1019,64 @@ int submodule_uses_gitfile(const char *path)
 	return 1;
 }
 
-int ok_to_remove_submodule(const char *path)
+/*
+ * Check if it is a bad idea to remove a submodule, i.e. if we'd lose data
+ * when doing so.
+ *
+ * Return 1 if we'd lose data, return 0 if the removal is fine,
+ * and negative values for errors.
+ */
+int bad_to_remove_submodule(const char *path, unsigned flags)
 {
 	ssize_t len;
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf buf = STRBUF_INIT;
-	int ok_to_remove = 1;
+	int ret = 0;
 
 	if (!file_exists(path) || is_empty_dir(path))
-		return 1;
+		return 0;
 
 	if (!submodule_uses_gitfile(path))
-		return 0;
+		return 1;
 
-	argv_array_pushl(&cp.args, "status", "--porcelain", "-u",
+	argv_array_pushl(&cp.args, "status", "--porcelain",
 				   "--ignore-submodules=none", NULL);
+
+	if (flags & SUBMODULE_REMOVAL_IGNORE_UNTRACKED)
+		argv_array_push(&cp.args, "-uno");
+	else
+		argv_array_push(&cp.args, "-uall");
+
+	if (!(flags & SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED))
+		argv_array_push(&cp.args, "--ignored");
+
 	prepare_submodule_repo_env(&cp.env_array);
 	cp.git_cmd = 1;
 	cp.no_stdin = 1;
 	cp.out = -1;
 	cp.dir = path;
-	if (start_command(&cp))
-		die(_("could not run 'git status --porcelain -u --ignore-submodules=none' in submodule %s"), path);
+	if (start_command(&cp)) {
+		if (flags & SUBMODULE_REMOVAL_DIE_ON_ERROR)
+			die(_("could not start 'git status in submodule '%s'"),
+				path);
+		ret = -1;
+		goto out;
+	}
 
 	len = strbuf_read(&buf, cp.out, 1024);
 	if (len > 2)
-		ok_to_remove = 0;
+		ret = 1;
 	close(cp.out);
 
-	if (finish_command(&cp))
-		die(_("'git status --porcelain -u --ignore-submodules=none' failed in submodule %s"), path);
-
+	if (finish_command(&cp)) {
+		if (flags & SUBMODULE_REMOVAL_DIE_ON_ERROR)
+			die(_("could not run 'git status in submodule '%s'"),
+				path);
+		ret = -1;
+	}
+out:
 	strbuf_release(&buf);
-	return ok_to_remove;
+	return ret;
 }
 
 static int find_first_merges(struct object_array *result, const char *path,
diff --git a/submodule.h b/submodule.h
index 61fb610749..21b1569413 100644
--- a/submodule.h
+++ b/submodule.h
@@ -59,7 +59,11 @@ extern int fetch_populated_submodules(const struct argv_array *options,
 			       int quiet, int max_parallel_jobs);
 extern unsigned is_submodule_modified(const char *path, int ignore_untracked);
 extern int submodule_uses_gitfile(const char *path);
-extern int ok_to_remove_submodule(const char *path);
+
+#define SUBMODULE_REMOVAL_DIE_ON_ERROR (1<<0)
+#define SUBMODULE_REMOVAL_IGNORE_UNTRACKED (1<<1)
+#define SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED (1<<2)
+extern int bad_to_remove_submodule(const char *path, unsigned flags);
 extern int merge_submodule(unsigned char result[20], const char *path,
 			   const unsigned char base[20],
 			   const unsigned char a[20],
-- 
2.11.0.rc2.53.gb7b3fba.dirty

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

[Stefan Beller]

When deleting a submodule, we need to keep the actual git directory around,
such that we do not lose local changes in there and at a later checkout
of the submodule we don't need to clone it again.

Now that the functionality is available to absorb the git directory of a
submodule, rewrite the checking in git-rm to not complain, but rather
relocate the git directories inside the superproject.

An alternative solution was discussed to have a function
`depopulate_submodule`. That would couple the check for its git directory
and possible relocation before the the removal, such that it is less
likely to miss the check in the future.  But the indirection with such
a function added seemed also complex. The reason for that was that this
possible move of the git directory was also implemented in
`ok_to_remove_submodule`, such that this function could truthfully
answer whether it is ok to remove the submodule.

The solution proposed here defers all these checks to the caller.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/rm.c  | 79 ++++++++++++++---------------------------------------------
 t/t3600-rm.sh | 39 ++++++++++++-----------------
 2 files changed, 33 insertions(+), 85 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index 5a5a66272b..6f2001b0eb 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -59,27 +59,9 @@ static void print_error_files(struct string_list *files_list,
 	}
 }
 
-static void error_removing_concrete_submodules(struct string_list *files, int *errs)
-{
-	print_error_files(files,
-			  Q_("the following submodule (or one of its nested "
-			     "submodules)\n"
-			     "uses a .git directory:",
-			     "the following submodules (or one of their nested "
-			     "submodules)\n"
-			     "use a .git directory:", files->nr),
-			  _("\n(use 'rm -rf' if you really want to remove "
-			    "it including all of its history)"),
-			  errs);
-	string_list_clear(files, 0);
-}
-
-static int check_submodules_use_gitfiles(void)
+static void submodules_absorb_gitdir_if_needed(const char *prefix)
 {
 	int i;
-	int errs = 0;
-	struct string_list files = STRING_LIST_INIT_NODUP;
-
 	for (i = 0; i < list.nr; i++) {
 		const char *name = list.entry[i].name;
 		int pos;
@@ -99,12 +81,9 @@ static int check_submodules_use_gitfiles(void)
 			continue;
 
 		if (!submodule_uses_gitfile(name))
-			string_list_append(&files, name);
+			absorb_git_dir_into_superproject(prefix, name,
+				ABSORB_GITDIR_RECURSE_SUBMODULES);
 	}
-
-	error_removing_concrete_submodules(&files, &errs);
-
-	return errs;
 }
 
 static int check_local_mod(struct object_id *head, int index_only)
@@ -120,7 +99,6 @@ static int check_local_mod(struct object_id *head, int index_only)
 	int errs = 0;
 	struct string_list files_staged = STRING_LIST_INIT_NODUP;
 	struct string_list files_cached = STRING_LIST_INIT_NODUP;
-	struct string_list files_submodule = STRING_LIST_INIT_NODUP;
 	struct string_list files_local = STRING_LIST_INIT_NODUP;
 
 	no_head = is_null_oid(head);
@@ -219,13 +197,8 @@ static int check_local_mod(struct object_id *head, int index_only)
 		else if (!index_only) {
 			if (staged_changes)
 				string_list_append(&files_cached, name);
-			if (local_changes) {
-				if (S_ISGITLINK(ce->ce_mode) &&
-				    !submodule_uses_gitfile(name))
-					string_list_append(&files_submodule, name);
-				else
-					string_list_append(&files_local, name);
-			}
+			if (local_changes)
+				string_list_append(&files_local, name);
 		}
 	}
 	print_error_files(&files_staged,
@@ -247,8 +220,6 @@ static int check_local_mod(struct object_id *head, int index_only)
 			  &errs);
 	string_list_clear(&files_cached, 0);
 
-	error_removing_concrete_submodules(&files_submodule, &errs);
-
 	print_error_files(&files_local,
 			  Q_("the following file has local modifications:",
 			     "the following files have local modifications:",
@@ -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);
 	}
 
 	/*
@@ -389,32 +359,20 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	 */
 	if (!index_only) {
 		int removed = 0, gitmodules_modified = 0;
-		struct strbuf buf = STRBUF_INIT;
 		for (i = 0; i < list.nr; i++) {
 			const char *path = list.entry[i].name;
 			if (list.entry[i].is_submodule) {
-				if (is_empty_dir(path)) {
-					if (!rmdir(path)) {
-						removed = 1;
-						if (!remove_path_from_gitmodules(path))
-							gitmodules_modified = 1;
-						continue;
-					}
-				} else {
-					strbuf_reset(&buf);
-					strbuf_addstr(&buf, path);
-					if (!remove_dir_recursively(&buf, 0)) {
-						removed = 1;
-						if (!remove_path_from_gitmodules(path))
-							gitmodules_modified = 1;
-						strbuf_release(&buf);
-						continue;
-					} else if (!file_exists(path))
-						/* Submodule was removed by user */
-						if (!remove_path_from_gitmodules(path))
-							gitmodules_modified = 1;
-					/* Fallthrough and let remove_path() fail. */
-				}
+				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;
 			}
 			if (!remove_path(path)) {
 				removed = 1;
@@ -423,7 +381,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 			if (!removed)
 				die_errno("git rm: '%s'", path);
 		}
-		strbuf_release(&buf);
 		if (gitmodules_modified)
 			stage_updated_gitmodules();
 	}
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index bcbb680651..5aa6db584c 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -569,26 +569,22 @@ test_expect_success 'rm of a conflicted unpopulated submodule succeeds' '
 	test_cmp expect actual
 '
 
-test_expect_success 'rm of a populated submodule with a .git directory fails even when forced' '
+test_expect_success 'rm of a populated submodule with a .git directory migrates git dir' '
 	git checkout -f master &&
 	git reset --hard &&
 	git submodule update &&
 	(cd submod &&
 		rm .git &&
 		cp -R ../.git/modules/sub .git &&
-		GIT_WORK_TREE=. git config --unset core.worktree
+		GIT_WORK_TREE=. git config --unset core.worktree &&
+		rm -r ../.git/modules/sub
 	) &&
-	test_must_fail git rm submod &&
-	test -d submod &&
-	test -d submod/.git &&
-	git status -s -uno --ignore-submodules=none >actual &&
-	! test -s actual &&
-	test_must_fail git rm -f submod &&
-	test -d submod &&
-	test -d submod/.git &&
+	git rm submod 2>output.err &&
+	! test -d submod &&
+	! test -d submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	! test -s actual &&
-	rm -rf submod
+	test -s actual &&
+	test_i18ngrep Migrating output.err
 '
 
 cat >expect.deepmodified <<EOF
@@ -667,24 +663,19 @@ test_expect_success 'rm of a populated nested submodule with a nested .git direc
 	git submodule update --recursive &&
 	(cd submod/subsubmod &&
 		rm .git &&
-		cp -R ../../.git/modules/sub/modules/sub .git &&
+		mv ../../.git/modules/sub/modules/sub .git &&
 		GIT_WORK_TREE=. git config --unset core.worktree
 	) &&
-	test_must_fail git rm submod &&
-	test -d submod &&
-	test -d submod/subsubmod/.git &&
-	git status -s -uno --ignore-submodules=none >actual &&
-	! test -s actual &&
-	test_must_fail git rm -f submod &&
-	test -d submod &&
-	test -d submod/subsubmod/.git &&
+	git rm submod 2>output.err &&
+	! test -d submod &&
+	! test -d submod/subsubmod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	! test -s actual &&
-	rm -rf submod
+	test -s actual &&
+	test_i18ngrep Migrating output.err
 '
 
 test_expect_success 'checking out a commit after submodule removal needs manual updates' '
-	git commit -m "submodule removal" submod &&
+	git commit -m "submodule removal" submod .gitmodules &&
 	git checkout HEAD^ &&
 	git submodule update &&
 	git checkout -q HEAD^ &&
-- 
2.11.0.rc2.53.gb7b3fba.dirty

Re: [PATCHv5 3/4] submodule: rename and add flags to ok_to_remove_submodule

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> As only 0 is understood as false, rename the function to invert the
> meaning, i.e. the return code of 0 signals the removal of the submodule
> is fine, and other values can be used to return a more precise answer
> what went wrong.

Makes sense to rename it as that will catch all the callers that
depend on the old semantics and name.

> -	if (start_command(&cp))
> -		die(_("could not run 'git status --porcelain -u --ignore-submodules=none' in submodule %s"), path);
> +	if (start_command(&cp)) {
> +		if (flags & SUBMODULE_REMOVAL_DIE_ON_ERROR)
> +			die(_("could not start 'git status in submodule '%s'"),
> +				path);
> +		ret = -1;
> +		goto out;
> +	}

This new codepath that does not die will not leak anything, as
a failed start_command() should release its argv[] and env[].

>  	len = strbuf_read(&buf, cp.out, 1024);
>  	if (len > 2)
> -		ok_to_remove = 0;
> +		ret = 1;

Not a new problem but is it obvious why the comparison of "len" is
against "2"?  This may deserve a one-liner comment.

Otherwise looks good to me.

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> 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. 

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

Re: [PATCHv5 1/4] submodule.h: add extern keyword to functions

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> As the upcoming series will add a lot of functions to the submodule
> header, let's first make the header consistent to the rest of the project
> by adding the extern keyword to functions.

This may be the right thing to do in the longer term but a patch
like this adds a lot of unnecessary work on the integrator when
there are _other_ topics that are in flight.  I'll see how bad the
conflicts are while merging the topic to 'pu'.

Thanks.

Re: [PATCHv5 3/4] submodule: rename and add flags to ok_to_remove_submodule

[Stefan Beller]

On Mon, Dec 26, 2016 at 4:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> As only 0 is understood as false, rename the function to invert the
>> meaning, i.e. the return code of 0 signals the removal of the submodule
>> is fine, and other values can be used to return a more precise answer
>> what went wrong.
>
> Makes sense to rename it as that will catch all the callers that
> depend on the old semantics and name.
>
>> -     if (start_command(&cp))
>> -             die(_("could not run 'git status --porcelain -u --ignore-submodules=none' in submodule %s"), path);
>> +     if (start_command(&cp)) {
>> +             if (flags & SUBMODULE_REMOVAL_DIE_ON_ERROR)
>> +                     die(_("could not start 'git status in submodule '%s'"),
>> +                             path);
>> +             ret = -1;
>> +             goto out;
>> +     }
>
> This new codepath that does not die will not leak anything, as
> a failed start_command() should release its argv[] and env[].
>
>>       len = strbuf_read(&buf, cp.out, 1024);
>>       if (len > 2)
>> -             ok_to_remove = 0;
>> +             ret = 1;
>
> Not a new problem but is it obvious why the comparison of "len" is
> against "2"?  This may deserve a one-liner comment.
>
> Otherwise looks good to me.

I took the check "len > 2" from the other occurrence in submodule.c.
When thinking about it (and inspecting the man page for
porcelain status), I think just checking != 0 is sufficient.

So in a reroll I'll change that to just

    if (len)
        ...

I'll look at the other occurrences and see if we can reuse code
as well.

Thanks,
Stefan

Re: [PATCHv5 1/4] submodule.h: add extern keyword to functions

[Stefan Beller]

On Mon, Dec 26, 2016 at 5:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> As the upcoming series will add a lot of functions to the submodule
>> header, let's first make the header consistent to the rest of the project
>> by adding the extern keyword to functions.
>
> This may be the right thing to do in the longer term but a patch
> like this adds a lot of unnecessary work on the integrator when
> there are _other_ topics that are in flight.  I'll see how bad the
> conflicts are while merging the topic to 'pu'.
>
> Thanks.

Maybe we can roll this patch on its own,
so in case the latter patches need rerolls we still have this one and can
build on top?

Thanks,
Stefan

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

[Stefan Beller]

On Mon, Dec 26, 2016 at 5:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> 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

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

[Stefan Beller]

On Tue, Dec 27, 2016 at 10:17 AM, Stefan Beller <sbeller@google.com> wrote:
> On Mon, Dec 26, 2016 at 5:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> 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;

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

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

OK, that happens way before this loop starts finding submodules and
removing them, so we can say that the latter is well protected.

Thanks.