[PATCH v3 0/5] Teach mv to move submodules

[PATCH v3 0/5] Teach mv to move submodules

[Jens Lehmann]

Here is my third iteration of this series.

Changes to v2 are:

- I resolved the conflict with Duy's pathspec series by replacing the
  use of common_prefix() with relative_path().

- I separated the functions checking for modified unstaged .gitmodules
  and staging the changes to that file into another commit, as they
  are used by both mv and rm.

- mv and rm now die with the message "Please, stage your changes to
  .gitmodules or stash them to proceed" instead of changing and
  staging a .gitmodules file containing other unstaged modifications.

- Man pages for mv and rm are updated to tell the user what they do
  with the gitlink and the .gitmodules file in case of submodules.

- Minor changes according to the last review (typos and a bit more
  efficient coding).

This series applies cleanly on current pu (and I also ran t3600 and
t7001 manually to make sure I don't hit the silent breakage my last
series showed when I ran the whole test suite).

Jens Lehmann (5):
  Teach mv to move submodules together with their work trees
  Teach mv to move submodules using a gitfile
  submodule.c: add .gitmodules staging helper functions
  Teach mv to update the path entry in .gitmodules for moved submodules
  rm: delete .gitmodules entry of submodules removed from the work tree

 Documentation/git-mv.txt   |  10 ++-
 Documentation/git-rm.txt   |   8 ++-
 builtin/mv.c               | 126 ++++++++++++++++++++++----------------
 builtin/rm.c               |  19 +++++-
 submodule.c                | 147 +++++++++++++++++++++++++++++++++++++++++++++
 submodule.h                |   5 ++
 t/t3600-rm.sh              |  98 ++++++++++++++++++++++++++++--
 t/t7001-mv.sh              | 128 +++++++++++++++++++++++++++++++++++++++
 t/t7400-submodule-basic.sh |  14 ++---
 t/t7610-mergetool.sh       |   6 +-
 10 files changed, 484 insertions(+), 77 deletions(-)

-- 
1.8.4.rc0.199.g7079aac

[PATCH v3 1/5] Teach mv to move submodules together with their work trees

[Jens Lehmann]

Currently the attempt to use "git mv" on a submodule errors out with:
  fatal: source directory is empty, source=<src>, destination=<dest>
The reason is that mv searches for the submodule with a trailing slash in
the index, which it doesn't find (because it is stored without a trailing
slash). As it doesn't find any index entries inside the submodule it
claims the directory would be empty even though it isn't.

Fix that by searching for the name without a trailing slash and continue
if it is a submodule. Then rename() will move the submodule work tree just
like it moves a file.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
 builtin/mv.c  | 99 +++++++++++++++++++++++++++++++----------------------------
 t/t7001-mv.sh | 34 ++++++++++++++++++++
 2 files changed, 86 insertions(+), 47 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 16ce99b..1d3ef63 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -118,55 +118,60 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				&& lstat(dst, &st) == 0)
 			bad = _("cannot move directory over file");
 		else if (src_is_dir) {
-			const char *src_w_slash = add_slash(src);
-			int len_w_slash = length + 1;
-			int first, last;
-
-			modes[i] = WORKING_DIRECTORY;
-
-			first = cache_name_pos(src_w_slash, len_w_slash);
-			if (first >= 0)
-				die (_("Huh? %.*s is in index?"),
-						len_w_slash, src_w_slash);
-
-			first = -1 - first;
-			for (last = first; last < active_nr; last++) {
-				const char *path = active_cache[last]->name;
-				if (strncmp(path, src_w_slash, len_w_slash))
-					break;
-			}
-			free((char *)src_w_slash);
-
-			if (last - first < 1)
-				bad = _("source directory is empty");
-			else {
-				int j, dst_len;
-
-				if (last - first > 0) {
-					source = xrealloc(source,
-							(argc + last - first)
-							* sizeof(char *));
-					destination = xrealloc(destination,
-							(argc + last - first)
-							* sizeof(char *));
-					modes = xrealloc(modes,
-							(argc + last - first)
-							* sizeof(enum update_mode));
+			int first = cache_name_pos(src, length);
+			if (first >= 0) {
+				if (!S_ISGITLINK(active_cache[first]->ce_mode))
+					die (_("Huh? Directory %s is in index and no submodule?"), src);
+			} else {
+				const char *src_w_slash = add_slash(src);
+				int last, len_w_slash = length + 1;
+
+				modes[i] = WORKING_DIRECTORY;
+
+				first = cache_name_pos(src_w_slash, len_w_slash);
+				if (first >= 0)
+					die (_("Huh? %.*s is in index?"),
+							len_w_slash, src_w_slash);
+
+				first = -1 - first;
+				for (last = first; last < active_nr; last++) {
+					const char *path = active_cache[last]->name;
+					if (strncmp(path, src_w_slash, len_w_slash))
+						break;
 				}
-
-				dst = add_slash(dst);
-				dst_len = strlen(dst);
-
-				for (j = 0; j < last - first; j++) {
-					const char *path =
-						active_cache[first + j]->name;
-					source[argc + j] = path;
-					destination[argc + j] =
-						prefix_path(dst, dst_len,
-							path + length + 1);
-					modes[argc + j] = INDEX;
+				free((char *)src_w_slash);
+
+				if (last - first < 1)
+					bad = _("source directory is empty");
+				else {
+					int j, dst_len;
+
+					if (last - first > 0) {
+						source = xrealloc(source,
+								(argc + last - first)
+								* sizeof(char *));
+						destination = xrealloc(destination,
+								(argc + last - first)
+								* sizeof(char *));
+						modes = xrealloc(modes,
+								(argc + last - first)
+								* sizeof(enum update_mode));
+					}
+
+					dst = add_slash(dst);
+					dst_len = strlen(dst);
+
+					for (j = 0; j < last - first; j++) {
+						const char *path =
+							active_cache[first + j]->name;
+						source[argc + j] = path;
+						destination[argc + j] =
+							prefix_path(dst, dst_len,
+								path + length + 1);
+						modes[argc + j] = INDEX;
+					}
+					argc += last - first;
 				}
-				argc += last - first;
 			}
 		} else if (cache_name_pos(src, length) < 0)
 			bad = _("not under version control");
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 101816e..15c18b6 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -259,4 +259,38 @@ test_expect_success SYMLINKS 'check moved symlink' '

 rm -f moved symlink

+test_expect_success 'setup submodule' '
+	git commit -m initial &&
+	git reset --hard &&
+	git submodule add ./. sub &&
+	echo content >file &&
+	git add file &&
+	git commit -m "added sub and file"
+'
+
+test_expect_success 'git mv cannot move a submodule in a file' '
+	test_must_fail git mv sub file
+'
+
+test_expect_success 'git mv moves a submodule with a .git directory and no .gitmodules' '
+	entry="$(git ls-files --stage sub | cut -f 1)" &&
+	git rm .gitmodules &&
+	(
+		cd sub &&
+		rm -f .git &&
+		cp -a ../.git/modules/sub .git &&
+		GIT_WORK_TREE=. git config --unset core.worktree
+	) &&
+	mkdir mod &&
+	git mv sub mod/sub &&
+	! test -e sub &&
+	[ "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" ] &&
+	(
+		cd mod/sub &&
+		git status
+	) &&
+	git update-index --refresh &&
+	git diff-files --quiet
+'
+
 test_done
-- 
1.8.4.rc0.199.g7079aac

[PATCH v3 2/5] Teach mv to move submodules using a gitfile

[Jens Lehmann]

When moving a submodule which uses a gitfile to point to the git directory
stored in .git/modules/<name> of the superproject two changes must be made
to make the submodule work: the .git file and the core.worktree setting
must be adjusted to point from work tree to git directory and back.

Achieve that by remembering which submodule uses a gitfile by storing the
result of read_gitfile() of each submodule. If that is not NULL the new
function connect_work_tree_and_git_dir() is called after renaming the
submodule's work tree which updates the two settings to the new values.

Extend the man page to inform the user about that feature (and while at it
change the description to not talk about a script anymore, as mv is a
builtin for quite some time now).

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
 Documentation/git-mv.txt |  8 +++++++-
 builtin/mv.c             | 19 +++++++++++++++----
 submodule.c              | 31 +++++++++++++++++++++++++++++++
 submodule.h              |  1 +
 t/t7001-mv.sh            | 19 +++++++++++++++++++
 5 files changed, 73 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-mv.txt b/Documentation/git-mv.txt
index e93fcb4..1f6fce0 100644
--- a/Documentation/git-mv.txt
+++ b/Documentation/git-mv.txt
@@ -13,7 +13,7 @@ SYNOPSIS

 DESCRIPTION
 -----------
-This script is used to move or rename a file, directory or symlink.
+Move or rename a file, directory or symlink.

  git mv [-v] [-f] [-n] [-k] <source> <destination>
  git mv [-v] [-f] [-n] [-k] <source> ... <destination directory>
@@ -44,6 +44,12 @@ OPTIONS
 --verbose::
 	Report the names of files as they are moved.

+SUBMODULES
+----------
+Moving a submodule using a gitfile (which means they were cloned
+with a Git version 1.7.8 or newer) will update the gitfile and
+core.worktree setting to make the submodule work in the new location.
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/builtin/mv.c b/builtin/mv.c
index 1d3ef63..68b7060 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -9,6 +9,7 @@
 #include "cache-tree.h"
 #include "string-list.h"
 #include "parse-options.h"
+#include "submodule.h"

 static const char * const builtin_mv_usage[] = {
 	N_("git mv [options] <source>... <destination>"),
@@ -66,7 +67,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		OPT_BOOLEAN('k', NULL, &ignore_errors, N_("skip move/rename errors")),
 		OPT_END(),
 	};
-	const char **source, **destination, **dest_path;
+	const char **source, **destination, **dest_path, **submodule_gitfile;
 	enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;
 	struct stat st;
 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
@@ -85,6 +86,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	source = internal_copy_pathspec(prefix, argv, argc, 0);
 	modes = xcalloc(argc, sizeof(enum update_mode));
 	dest_path = internal_copy_pathspec(prefix, argv + argc, 1, 0);
+	submodule_gitfile = xcalloc(argc, sizeof(char *));

 	if (dest_path[0][0] == '\0')
 		/* special case: "." was normalized to "" */
@@ -120,8 +122,14 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		else if (src_is_dir) {
 			int first = cache_name_pos(src, length);
 			if (first >= 0) {
+				struct strbuf submodule_dotgit = STRBUF_INIT;
 				if (!S_ISGITLINK(active_cache[first]->ce_mode))
 					die (_("Huh? Directory %s is in index and no submodule?"), src);
+				strbuf_addf(&submodule_dotgit, "%s/.git", src);
+				submodule_gitfile[i] = read_gitfile(submodule_dotgit.buf);
+				if (submodule_gitfile[i])
+					submodule_gitfile[i] = xstrdup(submodule_gitfile[i]);
+				strbuf_release(&submodule_dotgit);
 			} else {
 				const char *src_w_slash = add_slash(src);
 				int last, len_w_slash = length + 1;
@@ -216,9 +224,12 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		int pos;
 		if (show_only || verbose)
 			printf(_("Renaming %s to %s\n"), src, dst);
-		if (!show_only && mode != INDEX &&
-				rename(src, dst) < 0 && !ignore_errors)
-			die_errno (_("renaming '%s' failed"), src);
+		if (!show_only && mode != INDEX) {
+			if (rename(src, dst) < 0 && !ignore_errors)
+				die_errno (_("renaming '%s' failed"), src);
+			if (submodule_gitfile[i])
+				connect_work_tree_and_git_dir(dst, submodule_gitfile[i]);
+		}

 		if (mode == WORKING_DIRECTORY)
 			continue;
diff --git a/submodule.c b/submodule.c
index 3f0a3f9..d96d187 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1004,3 +1004,34 @@ int merge_submodule(unsigned char result[20], const char *path,
 	free(merges.objects);
 	return 0;
 }
+
+/* Update gitfile and core.worktree setting to connect work tree and git dir */
+void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
+{
+	struct strbuf file_name = STRBUF_INIT;
+	struct strbuf rel_path = STRBUF_INIT;
+	const char *real_work_tree = xstrdup(real_path(work_tree));
+	FILE *fp;
+
+	/* Update gitfile */
+	strbuf_addf(&file_name, "%s/.git", work_tree);
+	fp = fopen(file_name.buf, "w");
+	if (!fp)
+		die(_("Could not create git link %s"), file_name.buf);
+	fprintf(fp, "gitdir: %s\n", relative_path(git_dir, real_work_tree,
+						  &rel_path));
+	fclose(fp);
+
+	/* Update core.worktree setting */
+	strbuf_reset(&file_name);
+	strbuf_addf(&file_name, "%s/config", git_dir);
+	if (git_config_set_in_file(file_name.buf, "core.worktree",
+				   relative_path(real_work_tree, git_dir,
+						 &rel_path)))
+		die(_("Could not set core.worktree in %s"),
+		    file_name.buf);
+
+	strbuf_release(&file_name);
+	strbuf_release(&rel_path);
+	free((void *)real_work_tree);
+}
diff --git a/submodule.h b/submodule.h
index c7ffc7c..29e9658 100644
--- a/submodule.h
+++ b/submodule.h
@@ -36,5 +36,6 @@ int merge_submodule(unsigned char result[20], const char *path, const unsigned c
 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);
+void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);

 #endif
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 15c18b6..b99177f 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -293,4 +293,23 @@ test_expect_success 'git mv moves a submodule with a .git directory and no .gitm
 	git diff-files --quiet
 '

+test_expect_success 'git mv moves a submodule with gitfile' '
+	rm -rf mod/sub &&
+	git reset --hard &&
+	git submodule update &&
+	entry="$(git ls-files --stage sub | cut -f 1)" &&
+	(
+		cd mod &&
+		git mv ../sub/ .
+	) &&
+	! test -e sub &&
+	[ "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" ] &&
+	(
+		cd mod/sub &&
+		git status
+	) &&
+	git update-index --refresh &&
+	git diff-files --quiet
+'
+
 test_done
-- 
1.8.4.rc0.199.g7079aac

Re: [PATCH v3 2/5] Teach mv to move submodules using a gitfile

[Fredrik Gustafsson]

On Tue, Jul 30, 2013 at 09:50:03PM +0200, Jens Lehmann wrote:
> +/* Update gitfile and core.worktree setting to connect work tree and git dir */
> +void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
> +{
> +	struct strbuf file_name = STRBUF_INIT;
> +	struct strbuf rel_path = STRBUF_INIT;
> +	const char *real_work_tree = xstrdup(real_path(work_tree));
> +	FILE *fp;
> +
> +	/* Update gitfile */
> +	strbuf_addf(&file_name, "%s/.git", work_tree);
> +	fp = fopen(file_name.buf, "w");
> +	if (!fp)
> +		die(_("Could not create git link %s"), file_name.buf);
> +	fprintf(fp, "gitdir: %s\n", relative_path(git_dir, real_work_tree,
> +						  &rel_path));
> +	fclose(fp);
> +
> +	/* Update core.worktree setting */
> +	strbuf_reset(&file_name);
> +	strbuf_addf(&file_name, "%s/config", git_dir);
> +	if (git_config_set_in_file(file_name.buf, "core.worktree",
> +				   relative_path(real_work_tree, git_dir,
> +						 &rel_path)))
> +		die(_("Could not set core.worktree in %s"),
> +		    file_name.buf);
> +
> +	strbuf_release(&file_name);
> +	strbuf_release(&rel_path);
> +	free((void *)real_work_tree);
> +}

Would it make sense to return an int here and do the dying in
builtin/mv.c instead? Again thinking of "libgit" and the die_errno for
non-submodule errors are in mv.c and not in rename().

If that's the case the same applies for stage_updated_gitmodules() and
update_path_in_gitmodules().

update_path_in_gitmodules() also needs a strbuf_release() if you haven't
already found it.

-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iveqy@iveqy.com

[PATCH v3 3/5] submodule.c: add .gitmodules staging helper functions

[Jens Lehmann]

Add the new is_staging_gitmodules_ok() and stage_updated_gitmodules()
functions to submodule.c. The first makes it possible for call sites to
see if the .gitmodules file did contain any unstaged modifications they
would accidentally stage in addition to those they intend to stage
themselves. The second function stages all modifications to the
.gitmodules file, both will be used by subsequent patches for the mv
and rm commands.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
 submodule.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 submodule.h |  2 ++
 2 files changed, 53 insertions(+)

diff --git a/submodule.c b/submodule.c
index d96d187..584f7de 100644
--- a/submodule.c
+++ b/submodule.c
@@ -10,6 +10,7 @@
 #include "string-list.h"
 #include "sha1-array.h"
 #include "argv-array.h"
+#include "blob.h"

 static struct string_list config_name_for_path;
 static struct string_list config_fetch_recurse_submodules_for_name;
@@ -30,6 +31,51 @@ static struct sha1_array ref_tips_after_fetch;
  */
 static int gitmodules_is_unmerged;

+/*
+ * This flag is set if the .gitmodules file had unstaged modifications on
+ * startup. This must be checked before allowing modifications to the
+ * .gitmodules file with the intention to stage them later, because when
+ * continuing we would stage the modifications the user didn't stage herself
+ * too. That might change in a future version when we learn to stage the
+ * changes we do ourselves without staging any previous modifications.
+ */
+static int gitmodules_is_modified;
+
+
+int is_staging_gitmodules_ok()
+{
+	return !gitmodules_is_modified;
+}
+
+void stage_updated_gitmodules(void)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct stat st;
+	int pos;
+	struct cache_entry *ce;
+	int namelen = strlen(".gitmodules");
+
+	pos = cache_name_pos(".gitmodules", namelen);
+	if (pos < 0) {
+		warning(_("could not find .gitmodules in index"));
+		return;
+	}
+	ce = active_cache[pos];
+	ce->ce_flags = namelen;
+	if (strbuf_read_file(&buf, ".gitmodules", 0) < 0)
+		die(_("reading updated .gitmodules failed"));
+	if (lstat(".gitmodules", &st) < 0)
+		die_errno(_("unable to stat updated .gitmodules"));
+	fill_stat_cache_info(ce, &st);
+	ce->ce_mode = ce_mode_from_stat(ce, st.st_mode);
+	if (remove_cache_entry_at(pos) < 0)
+		die(_("unable to remove .gitmodules from index"));
+	if (write_sha1_file(buf.buf, buf.len, blob_type, ce->sha1))
+		die(_("adding updated .gitmodules failed"));
+	if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE))
+		die(_("staging updated .gitmodules failed"));
+}
+
 static int add_submodule_odb(const char *path)
 {
 	struct strbuf objects_directory = STRBUF_INIT;
@@ -116,6 +162,11 @@ void gitmodules_config(void)
 				    !memcmp(ce->name, ".gitmodules", 11))
 					gitmodules_is_unmerged = 1;
 			}
+		} else if (pos < active_nr) {
+			struct stat st;
+			if (lstat(".gitmodules", &st) == 0 &&
+			    ce_match_stat(active_cache[pos], &st, 0) & DATA_CHANGED)
+				gitmodules_is_modified = 1;
 		}

 		if (!gitmodules_is_unmerged)
diff --git a/submodule.h b/submodule.h
index 29e9658..244d5f5 100644
--- a/submodule.h
+++ b/submodule.h
@@ -11,6 +11,8 @@ enum {
 	RECURSE_SUBMODULES_ON = 2
 };

+int is_staging_gitmodules_ok();
+void stage_updated_gitmodules(void);
 void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
 		const char *path);
 int submodule_config(const char *var, const char *value, void *cb);
-- 
1.8.4.rc0.199.g7079aac

Re: [PATCH v3 3/5] submodule.c: add .gitmodules staging helper functions

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> +int is_staging_gitmodules_ok()

Will tweak this to:

	int is_staging_gitmodules_ok(void)

and fix this as well:

> +int is_staging_gitmodules_ok();
> +void stage_updated_gitmodules(void);


before queuing.

Thanks.

Re: [PATCH v3 3/5] submodule.c: add .gitmodules staging helper functions

[Jens Lehmann]

Am 30.07.2013 23:37, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
>> +int is_staging_gitmodules_ok()
> 
> Will tweak this to:
> 
> 	int is_staging_gitmodules_ok(void)
> 
> and fix this as well:
> 
>> +int is_staging_gitmodules_ok();
>> +void stage_updated_gitmodules(void);
> 
> 
> before queuing.

Thanks. As Frederik already noticed, this series is missing at least
one strbuf_release(). Will fix both in v4.

[PATCH v3 4/5] Teach mv to update the path entry in .gitmodules for moved submodules

[Jens Lehmann]

Currently using "git mv" on a submodule moves the submodule's work tree in
that of the superproject. But the submodule's path setting in .gitmodules
is left untouched, which is now inconsistent with the work tree and makes
git commands that rely on the proper path -> name mapping (like status and
diff) behave strangely.

Let "git mv" help here by not only moving the submodule's work tree but
also updating the "submodule.<submodule name>.path" setting from the
.gitmodules file and stage both. This doesn't happen when no .gitmodules
file is found and only issues a warning when it doesn't have a section for
this submodule. This is because the user might just use plain gitlinks
without the .gitmodules file or has already updated the path setting by
hand before issuing the "git mv" command (in which case the warning
reminds him that mv would have done that for him). Only when .gitmodules
is found and contains merge conflicts the mv command will fail and tell
the user to resolve the conflict before trying again.

Also extend the man page to inform the user about this new feature.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
 Documentation/git-mv.txt |  2 ++
 builtin/mv.c             | 10 ++++++-
 submodule.c              | 33 +++++++++++++++++++++
 submodule.h              |  1 +
 t/t7001-mv.sh            | 75 ++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 120 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-mv.txt b/Documentation/git-mv.txt
index 1f6fce0..b1f7988 100644
--- a/Documentation/git-mv.txt
+++ b/Documentation/git-mv.txt
@@ -49,6 +49,8 @@ SUBMODULES
 Moving a submodule using a gitfile (which means they were cloned
 with a Git version 1.7.8 or newer) will update the gitfile and
 core.worktree setting to make the submodule work in the new location.
+It also will attempt to update the submodule.<name>.path setting in
+the linkgit:gitmodules[5] file and stage that file (unless -n is used).

 GIT
 ---
diff --git a/builtin/mv.c b/builtin/mv.c
index 68b7060..7dd6bb4 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -58,7 +58,7 @@ static struct lock_file lock_file;

 int cmd_mv(int argc, const char **argv, const char *prefix)
 {
-	int i, newfd;
+	int i, newfd, gitmodules_modified = 0;
 	int verbose = 0, show_only = 0, force = 0, ignore_errors = 0;
 	struct option builtin_mv_options[] = {
 		OPT__VERBOSE(&verbose, N_("be verbose")),
@@ -72,6 +72,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	struct stat st;
 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;

+	gitmodules_config();
 	git_config(git_default_config, NULL);

 	argc = parse_options(argc, argv, prefix, builtin_mv_options,
@@ -125,6 +126,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				struct strbuf submodule_dotgit = STRBUF_INIT;
 				if (!S_ISGITLINK(active_cache[first]->ce_mode))
 					die (_("Huh? Directory %s is in index and no submodule?"), src);
+				if (!is_staging_gitmodules_ok())
+					die (_("Please, stage your changes to .gitmodules or stash them to proceed"));
 				strbuf_addf(&submodule_dotgit, "%s/.git", src);
 				submodule_gitfile[i] = read_gitfile(submodule_dotgit.buf);
 				if (submodule_gitfile[i])
@@ -229,6 +232,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				die_errno (_("renaming '%s' failed"), src);
 			if (submodule_gitfile[i])
 				connect_work_tree_and_git_dir(dst, submodule_gitfile[i]);
+			if (!update_path_in_gitmodules(src, dst))
+				gitmodules_modified = 1;
 		}

 		if (mode == WORKING_DIRECTORY)
@@ -240,6 +245,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 			rename_cache_entry_at(pos, dst);
 	}

+	if (gitmodules_modified)
+		stage_updated_gitmodules();
+
 	if (active_cache_changed) {
 		if (write_cache(newfd, active_cache, active_nr) ||
 		    commit_locked_index(&lock_file))
diff --git a/submodule.c b/submodule.c
index 584f7de..b210685 100644
--- a/submodule.c
+++ b/submodule.c
@@ -47,6 +47,39 @@ int is_staging_gitmodules_ok()
 	return !gitmodules_is_modified;
 }

+/*
+ * Try to update the "path" entry in the "submodule.<name>" section of the
+ * .gitmodules file. Return 0 only if a .gitmodules file was found, a section
+ * with the correct path=<oldpath> setting was found and we could update it.
+ */
+int update_path_in_gitmodules(const char *oldpath, const char *newpath)
+{
+	struct strbuf entry = STRBUF_INIT;
+	struct string_list_item *path_option;
+
+	if (!file_exists(".gitmodules")) /* Do nothing without .gitmodules */
+		return -1;
+
+	if (gitmodules_is_unmerged)
+		die(_("Cannot change unmerged .gitmodules, resolve merge conflicts first"));
+
+	path_option = unsorted_string_list_lookup(&config_name_for_path, oldpath);
+	if (!path_option) {
+		warning(_("Could not find section in .gitmodules where path=%s"), oldpath);
+		return -1;
+	}
+	strbuf_addstr(&entry, "submodule.");
+	strbuf_addstr(&entry, path_option->util);
+	strbuf_addstr(&entry, ".path");
+	if (git_config_set_in_file(".gitmodules", entry.buf, newpath) < 0) {
+		/* Maybe the user already did that, don't error out here */
+		warning(_("Could not update .gitmodules entry %s"), entry.buf);
+		return -1;
+	}
+	strbuf_release(&entry);
+	return 0;
+}
+
 void stage_updated_gitmodules(void)
 {
 	struct strbuf buf = STRBUF_INIT;
diff --git a/submodule.h b/submodule.h
index 244d5f5..570d4d0 100644
--- a/submodule.h
+++ b/submodule.h
@@ -12,6 +12,7 @@ enum {
 };

 int is_staging_gitmodules_ok();
+int update_path_in_gitmodules(const char *oldpath, const char *newpath);
 void stage_updated_gitmodules(void);
 void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
 		const char *path);
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index b99177f..d432f42 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -308,8 +308,83 @@ test_expect_success 'git mv moves a submodule with gitfile' '
 		cd mod/sub &&
 		git status
 	) &&
+	echo mod/sub >expected &&
+	git config -f .gitmodules submodule.sub.path >actual &&
+	test_cmp expected actual &&
 	git update-index --refresh &&
 	git diff-files --quiet
 '

+test_expect_success 'mv does not complain when no .gitmodules file is found' '
+	rm -rf mod/sub &&
+	git reset --hard &&
+	git submodule update &&
+	git rm .gitmodules &&
+	entry="$(git ls-files --stage sub | cut -f 1)" &&
+	git mv sub mod/sub 2>actual.err &&
+	! test -s actual.err &&
+	! test -e sub &&
+	[ "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" ] &&
+	(
+		cd mod/sub &&
+		git status
+	) &&
+	git update-index --refresh &&
+	git diff-files --quiet
+'
+
+test_expect_success 'mv will error out on a modified .gitmodules file unless staged' '
+	rm -rf mod/sub &&
+	git reset --hard &&
+	git submodule update &&
+	git config -f .gitmodules foo.bar true &&
+	entry="$(git ls-files --stage sub | cut -f 1)" &&
+	test_must_fail git mv sub mod/sub 2>actual.err &&
+	test -s actual.err &&
+	test -e sub &&
+	git diff-files --quiet -- sub &&
+	git add .gitmodules &&
+	git mv sub mod/sub 2>actual.err &&
+	! test -s actual.err &&
+	! test -e sub &&
+	[ "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" ] &&
+	(
+		cd mod/sub &&
+		git status
+	) &&
+	git update-index --refresh &&
+	git diff-files --quiet
+'
+
+test_expect_success 'mv issues a warning when section is not found in .gitmodules' '
+	rm -rf mod/sub &&
+	git reset --hard &&
+	git submodule update &&
+	git config -f .gitmodules --remove-section submodule.sub &&
+	git add .gitmodules &&
+	entry="$(git ls-files --stage sub | cut -f 1)" &&
+	echo "warning: Could not find section in .gitmodules where path=sub" >expect.err &&
+	git mv sub mod/sub 2>actual.err &&
+	test_i18ncmp expect.err actual.err &&
+	! test -e sub &&
+	[ "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" ] &&
+	(
+		cd mod/sub &&
+		git status
+	) &&
+	git update-index --refresh &&
+	git diff-files --quiet
+'
+
+test_expect_success 'mv --dry-run does not touch the submodule or .gitmodules' '
+	rm -rf mod/sub &&
+	git reset --hard &&
+	git submodule update &&
+	git mv -n sub mod/sub 2>actual.err &&
+	test -f sub/.git &&
+	git diff-index --exit-code HEAD &&
+	git update-index --refresh &&
+	git diff-files --quiet -- sub .gitmodules
+'
+
 test_done
-- 
1.8.4.rc0.199.g7079aac

Re: [PATCH v4 4/5] Teach mv to update the path entry in .gitmodules for moved submodules

[Jens Lehmann]

Currently using "git mv" on a submodule moves the submodule's work tree in
that of the superproject. But the submodule's path setting in .gitmodules
is left untouched, which is now inconsistent with the work tree and makes
git commands that rely on the proper path -> name mapping (like status and
diff) behave strangely.

Let "git mv" help here by not only moving the submodule's work tree but
also updating the "submodule.<submodule name>.path" setting from the
.gitmodules file and stage both. This doesn't happen when no .gitmodules
file is found and only issues a warning when it doesn't have a section for
this submodule. This is because the user might just use plain gitlinks
without the .gitmodules file or has already updated the path setting by
hand before issuing the "git mv" command (in which case the warning
reminds him that mv would have done that for him). Only when .gitmodules
is found and contains merge conflicts the mv command will fail and tell
the user to resolve the conflict before trying again.

Also extend the man page to inform the user about this new feature.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---

This version fixes the missing strbuf_release() noticed by Frederik.


 Documentation/git-mv.txt |  2 ++
 builtin/mv.c             | 10 ++++++-
 submodule.c              | 34 ++++++++++++++++++++++
 submodule.h              |  1 +
 t/t7001-mv.sh            | 75 ++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 121 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-mv.txt b/Documentation/git-mv.txt
index 1f6fce0..b1f7988 100644
--- a/Documentation/git-mv.txt
+++ b/Documentation/git-mv.txt
@@ -49,6 +49,8 @@ SUBMODULES
 Moving a submodule using a gitfile (which means they were cloned
 with a Git version 1.7.8 or newer) will update the gitfile and
 core.worktree setting to make the submodule work in the new location.
+It also will attempt to update the submodule.<name>.path setting in
+the linkgit:gitmodules[5] file and stage that file (unless -n is used).

 GIT
 ---
diff --git a/builtin/mv.c b/builtin/mv.c
index 68b7060..7dd6bb4 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -58,7 +58,7 @@ static struct lock_file lock_file;

 int cmd_mv(int argc, const char **argv, const char *prefix)
 {
-	int i, newfd;
+	int i, newfd, gitmodules_modified = 0;
 	int verbose = 0, show_only = 0, force = 0, ignore_errors = 0;
 	struct option builtin_mv_options[] = {
 		OPT__VERBOSE(&verbose, N_("be verbose")),
@@ -72,6 +72,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	struct stat st;
 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;

+	gitmodules_config();
 	git_config(git_default_config, NULL);

 	argc = parse_options(argc, argv, prefix, builtin_mv_options,
@@ -125,6 +126,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				struct strbuf submodule_dotgit = STRBUF_INIT;
 				if (!S_ISGITLINK(active_cache[first]->ce_mode))
 					die (_("Huh? Directory %s is in index and no submodule?"), src);
+				if (!is_staging_gitmodules_ok())
+					die (_("Please, stage your changes to .gitmodules or stash them to proceed"));
 				strbuf_addf(&submodule_dotgit, "%s/.git", src);
 				submodule_gitfile[i] = read_gitfile(submodule_dotgit.buf);
 				if (submodule_gitfile[i])
@@ -229,6 +232,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				die_errno (_("renaming '%s' failed"), src);
 			if (submodule_gitfile[i])
 				connect_work_tree_and_git_dir(dst, submodule_gitfile[i]);
+			if (!update_path_in_gitmodules(src, dst))
+				gitmodules_modified = 1;
 		}

 		if (mode == WORKING_DIRECTORY)
@@ -240,6 +245,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 			rename_cache_entry_at(pos, dst);
 	}

+	if (gitmodules_modified)
+		stage_updated_gitmodules();
+
 	if (active_cache_changed) {
 		if (write_cache(newfd, active_cache, active_nr) ||
 		    commit_locked_index(&lock_file))
diff --git a/submodule.c b/submodule.c
index 5874d08..1c2714f 100644
--- a/submodule.c
+++ b/submodule.c
@@ -47,6 +47,40 @@ int is_staging_gitmodules_ok(void)
 	return !gitmodules_is_modified;
 }

+/*
+ * Try to update the "path" entry in the "submodule.<name>" section of the
+ * .gitmodules file. Return 0 only if a .gitmodules file was found, a section
+ * with the correct path=<oldpath> setting was found and we could update it.
+ */
+int update_path_in_gitmodules(const char *oldpath, const char *newpath)
+{
+	struct strbuf entry = STRBUF_INIT;
+	struct string_list_item *path_option;
+
+	if (!file_exists(".gitmodules")) /* Do nothing without .gitmodules */
+		return -1;
+
+	if (gitmodules_is_unmerged)
+		die(_("Cannot change unmerged .gitmodules, resolve merge conflicts first"));
+
+	path_option = unsorted_string_list_lookup(&config_name_for_path, oldpath);
+	if (!path_option) {
+		warning(_("Could not find section in .gitmodules where path=%s"), oldpath);
+		return -1;
+	}
+	strbuf_addstr(&entry, "submodule.");
+	strbuf_addstr(&entry, path_option->util);
+	strbuf_addstr(&entry, ".path");
+	if (git_config_set_in_file(".gitmodules", entry.buf, newpath) < 0) {
+		/* Maybe the user already did that, don't error out here */
+		warning(_("Could not update .gitmodules entry %s"), entry.buf);
+		strbuf_release(&entry);
+		return -1;
+	}
+	strbuf_release(&entry);
+	return 0;
+}
+
 void stage_updated_gitmodules(void)
 {
 	struct strbuf buf = STRBUF_INIT;
diff --git a/submodule.h b/submodule.h
index 5501354..e339891 100644
--- a/submodule.h
+++ b/submodule.h
@@ -12,6 +12,7 @@ enum {
 };

 int is_staging_gitmodules_ok(void);
+int update_path_in_gitmodules(const char *oldpath, const char *newpath);
 void stage_updated_gitmodules(void);
 void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
 		const char *path);
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index b99177f..d432f42 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -308,8 +308,83 @@ test_expect_success 'git mv moves a submodule with gitfile' '
 		cd mod/sub &&
 		git status
 	) &&
+	echo mod/sub >expected &&
+	git config -f .gitmodules submodule.sub.path >actual &&
+	test_cmp expected actual &&
 	git update-index --refresh &&
 	git diff-files --quiet
 '

+test_expect_success 'mv does not complain when no .gitmodules file is found' '
+	rm -rf mod/sub &&
+	git reset --hard &&
+	git submodule update &&
+	git rm .gitmodules &&
+	entry="$(git ls-files --stage sub | cut -f 1)" &&
+	git mv sub mod/sub 2>actual.err &&
+	! test -s actual.err &&
+	! test -e sub &&
+	[ "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" ] &&
+	(
+		cd mod/sub &&
+		git status
+	) &&
+	git update-index --refresh &&
+	git diff-files --quiet
+'
+
+test_expect_success 'mv will error out on a modified .gitmodules file unless staged' '
+	rm -rf mod/sub &&
+	git reset --hard &&
+	git submodule update &&
+	git config -f .gitmodules foo.bar true &&
+	entry="$(git ls-files --stage sub | cut -f 1)" &&
+	test_must_fail git mv sub mod/sub 2>actual.err &&
+	test -s actual.err &&
+	test -e sub &&
+	git diff-files --quiet -- sub &&
+	git add .gitmodules &&
+	git mv sub mod/sub 2>actual.err &&
+	! test -s actual.err &&
+	! test -e sub &&
+	[ "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" ] &&
+	(
+		cd mod/sub &&
+		git status
+	) &&
+	git update-index --refresh &&
+	git diff-files --quiet
+'
+
+test_expect_success 'mv issues a warning when section is not found in .gitmodules' '
+	rm -rf mod/sub &&
+	git reset --hard &&
+	git submodule update &&
+	git config -f .gitmodules --remove-section submodule.sub &&
+	git add .gitmodules &&
+	entry="$(git ls-files --stage sub | cut -f 1)" &&
+	echo "warning: Could not find section in .gitmodules where path=sub" >expect.err &&
+	git mv sub mod/sub 2>actual.err &&
+	test_i18ncmp expect.err actual.err &&
+	! test -e sub &&
+	[ "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" ] &&
+	(
+		cd mod/sub &&
+		git status
+	) &&
+	git update-index --refresh &&
+	git diff-files --quiet
+'
+
+test_expect_success 'mv --dry-run does not touch the submodule or .gitmodules' '
+	rm -rf mod/sub &&
+	git reset --hard &&
+	git submodule update &&
+	git mv -n sub mod/sub 2>actual.err &&
+	test -f sub/.git &&
+	git diff-index --exit-code HEAD &&
+	git update-index --refresh &&
+	git diff-files --quiet -- sub .gitmodules
+'
+
 test_done
-- 
1.8.4.rc0.199.g3f237fc

[PATCH v3 5/5] rm: delete .gitmodules entry of submodules removed from the work tree

[Jens Lehmann]

Currently using "git rm" on a submodule removes the submodule's work tree
from that of the superproject and the gitlink from the index. But the
submodule's section in .gitmodules is left untouched, which is a leftover
of the now removed submodule and might irritate users (as opposed to the
setting in .git/config, this must stay as a reminder that the user showed
interest in this submodule so it will be repopulated later when an older
commit is checked out).

Let "git rm" help the user by not only removing the submodule from the
work tree but by also removing the "submodule.<submodule name>" section
from the .gitmodules file and stage both. This doesn't happen when the
"--cached" option is used, as it would modify the work tree. This also
silently does nothing when no .gitmodules file is found and only issues a
warning when it doesn't have a section for this submodule. This is because
the user might just use plain gitlinks without the .gitmodules file or has
already removed the section by hand before issuing the "git rm" command
(in which case the warning reminds him that rm would have done that for
him). Only when .gitmodules is found and contains merge conflicts the rm
command will fail and tell the user to resolve the conflict before trying
again.

Also extend the man page to inform the user about this new feature. While
at it promote the submodule sub-section to a chapter as it made not much
sense under "REMOVING FILES THAT HAVE DISAPPEARED FROM THE FILESYSTEM".

In t7610 three uses of "git rm submod" had to be replaced with "git rm
--cached submod" because that test expects .gitmodules and the work tree
to stay untouched. Also in t7400 the tests for the remaining settings in
the .gitmodules file had to be changed to assert that these settings are
missing.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
 Documentation/git-rm.txt   |  8 ++--
 builtin/rm.c               | 19 +++++++--
 submodule.c                | 32 +++++++++++++++
 submodule.h                |  1 +
 t/t3600-rm.sh              | 98 +++++++++++++++++++++++++++++++++++++++++++---
 t/t7400-submodule-basic.sh | 14 ++-----
 t/t7610-mergetool.sh       |  6 +--
 7 files changed, 153 insertions(+), 25 deletions(-)

diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt
index 1d876c2..9d731b4 100644
--- a/Documentation/git-rm.txt
+++ b/Documentation/git-rm.txt
@@ -134,14 +134,16 @@ use the following command:
 git diff --name-only --diff-filter=D -z | xargs -0 git rm --cached
 ----------------

-Submodules
-~~~~~~~~~~
+SUBMODULES
+----------
 Only submodules using a gitfile (which means they were cloned
 with a Git version 1.7.8 or newer) will be removed from the work
 tree, as their repository lives inside the .git directory of the
 superproject. If a submodule (or one of those nested inside it)
 still uses a .git directory, `git rm` will fail - no matter if forced
-or not - to protect the submodule's history.
+or not - to protect the submodule's history. If it exists the
+submodule.<name> section in the linkgit:gitmodules[5] file will also
+be removed and that file will be staged (unless --cached or -n are used).

 A submodule is considered up-to-date when the HEAD is the same as
 recorded in the index, no tracked files are modified and no untracked
diff --git a/builtin/rm.c b/builtin/rm.c
index df85f98..a9d45dd 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -283,6 +283,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	struct pathspec pathspec;
 	char *seen;

+	gitmodules_config();
 	git_config(git_default_config, NULL);

 	argc = parse_options(argc, argv, prefix, builtin_rm_options,
@@ -324,7 +325,10 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 			continue;
 		ALLOC_GROW(list.entry, list.nr + 1, list.alloc);
 		list.entry[list.nr].name = ce->name;
-		list.entry[list.nr++].is_submodule = S_ISGITLINK(ce->ce_mode);
+		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 your changes to .gitmodules or stash them to proceed"));
 	}

 	if (pathspec.nr) {
@@ -396,13 +400,15 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	 * in the middle)
 	 */
 	if (!index_only) {
-		int removed = 0;
+		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) {
 				if (is_empty_dir(path)) {
 					if (!rmdir(path)) {
 						removed = 1;
+						if (!remove_path_from_gitmodules(path))
+							gitmodules_modified = 1;
 						continue;
 					}
 				} else {
@@ -410,9 +416,14 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 					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;
 					strbuf_release(&buf);
 					/* Fallthrough and let remove_path() fail. */
 				}
@@ -424,6 +435,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 			if (!removed)
 				die_errno("git rm: '%s'", path);
 		}
+		if (gitmodules_modified)
+			stage_updated_gitmodules();
 	}

 	if (active_cache_changed) {
diff --git a/submodule.c b/submodule.c
index b210685..5cd8845 100644
--- a/submodule.c
+++ b/submodule.c
@@ -80,6 +80,38 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)
 	return 0;
 }

+/*
+ * Try to remove the "submodule.<name>" section from .gitmodules where the given
+ * path is configured. Return 0 only if a .gitmodules file was found, a section
+ * with the correct path=<path> setting was found and we could remove it.
+ */
+int remove_path_from_gitmodules(const char *path)
+{
+	struct strbuf sect = STRBUF_INIT;
+	struct string_list_item *path_option;
+
+	if (!file_exists(".gitmodules")) /* Do nothing without .gitmodules */
+		return -1;
+
+	if (gitmodules_is_unmerged)
+		die(_("Cannot change unmerged .gitmodules, resolve merge conflicts first"));
+
+	path_option = unsorted_string_list_lookup(&config_name_for_path, path);
+	if (!path_option) {
+		warning(_("Could not find section in .gitmodules where path=%s"), path);
+		return -1;
+	}
+	strbuf_addstr(&sect, "submodule.");
+	strbuf_addstr(&sect, path_option->util);
+	if (git_config_rename_section_in_file(".gitmodules", sect.buf, NULL) < 0) {
+		/* Maybe the user already did that, don't error out here */
+		warning(_("Could not remove .gitmodules entry for %s"), path);
+		return -1;
+	}
+	strbuf_release(&sect);
+	return 0;
+}
+
 void stage_updated_gitmodules(void)
 {
 	struct strbuf buf = STRBUF_INIT;
diff --git a/submodule.h b/submodule.h
index 570d4d0..eca1d11 100644
--- a/submodule.h
+++ b/submodule.h
@@ -13,6 +13,7 @@ enum {

 int is_staging_gitmodules_ok();
 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,
 		const char *path);
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 5c87b55..639cb70 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -263,6 +263,7 @@ test_expect_success 'rm removes subdirectories recursively' '
 '

 cat >expect <<EOF
+M  .gitmodules
 D  submod
 EOF

@@ -270,6 +271,15 @@ cat >expect.modified <<EOF
  M submod
 EOF

+cat >expect.cached <<EOF
+D  submod
+EOF
+
+cat >expect.both_deleted<<EOF
+D  .gitmodules
+D  submod
+EOF
+
 test_expect_success 'rm removes empty submodules from work tree' '
 	mkdir submod &&
 	git update-index --add --cacheinfo 160000 $(git rev-parse HEAD) submod &&
@@ -281,16 +291,20 @@ test_expect_success 'rm removes empty submodules from work tree' '
 	git rm submod &&
 	test ! -e submod &&
 	git status -s -uno --ignore-submodules=none > actual &&
-	test_cmp expect actual
+	test_cmp expect actual &&
+	test_must_fail git config -f .gitmodules submodule.sub.url &&
+	test_must_fail git config -f .gitmodules submodule.sub.path
 '

-test_expect_success 'rm removes removed submodule from index' '
+test_expect_success 'rm removes removed submodule from index and .gitmodules' '
 	git reset --hard &&
 	git submodule update &&
 	rm -rf submod &&
 	git rm submod &&
 	git status -s -uno --ignore-submodules=none > actual &&
-	test_cmp expect actual
+	test_cmp expect actual &&
+	test_must_fail git config -f .gitmodules submodule.sub.url &&
+	test_must_fail git config -f .gitmodules submodule.sub.path
 '

 test_expect_success 'rm removes work tree of unmodified submodules' '
@@ -299,7 +313,9 @@ test_expect_success 'rm removes work tree of unmodified submodules' '
 	git rm submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none > actual &&
-	test_cmp expect actual
+	test_cmp expect actual &&
+	test_must_fail git config -f .gitmodules submodule.sub.url &&
+	test_must_fail git config -f .gitmodules submodule.sub.path
 '

 test_expect_success 'rm removes a submodule with a trailing /' '
@@ -333,6 +349,72 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none > actual &&
+	test_cmp expect actual &&
+	test_must_fail git config -f .gitmodules submodule.sub.url &&
+	test_must_fail git config -f .gitmodules submodule.sub.path
+'
+
+test_expect_success 'rm --cached leaves work tree of populated submodules and .gitmodules alone' '
+	git reset --hard &&
+	git submodule update &&
+	git rm --cached submod &&
+	test -d submod &&
+	test -f submod/.git &&
+	git status -s -uno >actual &&
+	test_cmp expect.cached actual &&
+	git config -f .gitmodules submodule.sub.url &&
+	git config -f .gitmodules submodule.sub.path
+'
+
+test_expect_success 'rm --dry-run does not touch the submodule or .gitmodules' '
+	git reset --hard &&
+	git submodule update &&
+	git rm -n submod &&
+	test -f submod/.git &&
+	git diff-index --exit-code HEAD
+'
+
+test_expect_success 'rm does not complain when no .gitmodules file is found' '
+	git reset --hard &&
+	git submodule update &&
+	git rm .gitmodules &&
+	git rm submod >actual 2>actual.err &&
+	! test -s actual.err &&
+	! test -d submod &&
+	! test -f submod/.git &&
+	git status -s -uno >actual &&
+	test_cmp expect.both_deleted actual
+'
+
+test_expect_success 'rm will error out on a modified .gitmodules file unless staged' '
+	git reset --hard &&
+	git submodule update &&
+	git config -f .gitmodules foo.bar true &&
+	test_must_fail git rm submod >actual 2>actual.err &&
+	test -s actual.err &&
+	test -d submod &&
+	test -f submod/.git &&
+	git diff-files --quiet -- submod &&
+	git add .gitmodules &&
+	git rm submod >actual 2>actual.err &&
+	! test -s actual.err &&
+	! test -d submod &&
+	! test -f submod/.git &&
+	git status -s -uno >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'rm issues a warning when section is not found in .gitmodules' '
+	git reset --hard &&
+	git submodule update &&
+	git config -f .gitmodules --remove-section submodule.sub &&
+	git add .gitmodules &&
+	echo "warning: Could not find section in .gitmodules where path=submod" >expect.err &&
+	git rm submod >actual 2>actual.err &&
+	test_i18ncmp expect.err actual.err &&
+	! test -d submod &&
+	! test -f submod/.git &&
+	git status -s -uno >actual &&
 	test_cmp expect actual
 '

@@ -427,7 +509,9 @@ test_expect_success 'rm of a conflicted populated submodule with different HEAD
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none > actual &&
-	test_cmp expect actual
+	test_cmp expect actual &&
+	test_must_fail git config -f .gitmodules submodule.sub.url &&
+	test_must_fail git config -f .gitmodules submodule.sub.path
 '

 test_expect_success 'rm of a conflicted populated submodule with modifications fails unless forced' '
@@ -446,7 +530,9 @@ test_expect_success 'rm of a conflicted populated submodule with modifications f
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none > actual &&
-	test_cmp expect actual
+	test_cmp expect actual &&
+	test_must_fail git config -f .gitmodules submodule.sub.url &&
+	test_must_fail git config -f .gitmodules submodule.sub.path
 '

 test_expect_success 'rm of a conflicted populated submodule with untracked files fails unless forced' '
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 5ee97b0..88293e6 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -773,13 +773,11 @@ test_expect_success 'submodule add --name allows to replace a submodule with ano
 			test_cmp expect .git
 		) &&
 		echo "repo" >expect &&
-		git config -f .gitmodules submodule.repo.path >actual &&
-		test_cmp expect actual &&
+		test_must_fail git config -f .gitmodules submodule.repo.path &&
 		git config -f .gitmodules submodule.repo_new.path >actual &&
 		test_cmp expect actual&&
 		echo "$submodurl/repo" >expect &&
-		git config -f .gitmodules submodule.repo.url >actual &&
-		test_cmp expect actual &&
+		test_must_fail git config -f .gitmodules submodule.repo.url &&
 		echo "$submodurl/bare.git" >expect &&
 		git config -f .gitmodules submodule.repo_new.url >actual &&
 		test_cmp expect actual &&
@@ -799,12 +797,8 @@ test_expect_success 'submodule add with an existing name fails unless forced' '
 		git rm repo &&
 		test_must_fail git submodule add -q --name repo_new "$submodurl/repo.git" repo &&
 		test ! -d repo &&
-		echo "repo" >expect &&
-		git config -f .gitmodules submodule.repo_new.path >actual &&
-		test_cmp expect actual&&
-		echo "$submodurl/bare.git" >expect &&
-		git config -f .gitmodules submodule.repo_new.url >actual &&
-		test_cmp expect actual &&
+		test_must_fail git config -f .gitmodules submodule.repo_new.path &&
+		test_must_fail git config -f .gitmodules submodule.repo_new.url &&
 		echo "$submodurl/bare.git" >expect &&
 		git config submodule.repo_new.url >actual &&
 		test_cmp expect actual &&
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index d526b1d..05d9db0 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -253,7 +253,7 @@ test_expect_success 'deleted vs modified submodule' '
     git checkout -b test6 branch1 &&
     git submodule update -N &&
     mv submod submod-movedaside &&
-    git rm submod &&
+    git rm --cached submod &&
     git commit -m "Submodule deleted from branch" &&
     git checkout -b test6.a test6 &&
     test_must_fail git merge master &&
@@ -322,7 +322,7 @@ test_expect_success 'file vs modified submodule' '
     git checkout -b test7 branch1 &&
     git submodule update -N &&
     mv submod submod-movedaside &&
-    git rm submod &&
+    git rm --cached submod &&
     echo not a submodule >submod &&
     git add submod &&
     git commit -m "Submodule path becomes file" &&
@@ -453,7 +453,7 @@ test_expect_success 'submodule in subdirectory' '
 test_expect_success 'directory vs modified submodule' '
     git checkout -b test11 branch1 &&
     mv submod submod-movedaside &&
-    git rm submod &&
+    git rm --cached submod &&
     mkdir submod &&
     echo not a submodule >submod/file16 &&
     git add submod/file16 &&
-- 
1.8.4.rc0.199.g7079aac

Re: [PATCH v3 5/5] rm: delete .gitmodules entry of submodules removed from the work tree

[Fredrik Gustafsson]

On Tue, Jul 30, 2013 at 09:51:51PM +0200, Jens Lehmann wrote:
> +/*
> + * Try to remove the "submodule.<name>" section from .gitmodules where the given
> + * path is configured. Return 0 only if a .gitmodules file was found, a section
> + * with the correct path=<path> setting was found and we could remove it.
> + */
> +int remove_path_from_gitmodules(const char *path)
> +{
> +	struct strbuf sect = STRBUF_INIT;
> +	struct string_list_item *path_option;
> +
> +	if (!file_exists(".gitmodules")) /* Do nothing without .gitmodules */
> +		return -1;
> +
> +	if (gitmodules_is_unmerged)
> +		die(_("Cannot change unmerged .gitmodules, resolve merge conflicts first"));
> +
> +	path_option = unsorted_string_list_lookup(&config_name_for_path, path);
> +	if (!path_option) {
> +		warning(_("Could not find section in .gitmodules where path=%s"), path);
> +		return -1;
> +	}
> +	strbuf_addstr(&sect, "submodule.");
> +	strbuf_addstr(&sect, path_option->util);
> +	if (git_config_rename_section_in_file(".gitmodules", sect.buf, NULL) < 0) {
> +		/* Maybe the user already did that, don't error out here */
> +		warning(_("Could not remove .gitmodules entry for %s"), path);
> +		return -1;
> +	}
> +	strbuf_release(&sect);
> +	return 0;
> +}

This question applies for this function and a few more functions in this
patch that has the same characteristics.

If we're in a state when we need to return non-zero, we don't do any
cleaning (that is strbuf_release()). Since this file is in the part
called libgit AFAIK, shouldn't we always clean after us?

Would it make sense to have different return values for different
errors?

I do like the comments above the function, more functions (at least
non-static ones) should follow this good style IMHO.
-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iveqy@iveqy.com

Re: [PATCH v3 5/5] rm: delete .gitmodules entry of submodules removed from the work tree

[Jens Lehmann]

Am 30.07.2013 22:15, schrieb Fredrik Gustafsson:
> On Tue, Jul 30, 2013 at 09:51:51PM +0200, Jens Lehmann wrote:
>> +/*
>> + * Try to remove the "submodule.<name>" section from .gitmodules where the given
>> + * path is configured. Return 0 only if a .gitmodules file was found, a section
>> + * with the correct path=<path> setting was found and we could remove it.
>> + */
>> +int remove_path_from_gitmodules(const char *path)
>> +{
>> +	struct strbuf sect = STRBUF_INIT;
>> +	struct string_list_item *path_option;
>> +
>> +	if (!file_exists(".gitmodules")) /* Do nothing without .gitmodules */
>> +		return -1;
>> +
>> +	if (gitmodules_is_unmerged)
>> +		die(_("Cannot change unmerged .gitmodules, resolve merge conflicts first"));
>> +
>> +	path_option = unsorted_string_list_lookup(&config_name_for_path, path);
>> +	if (!path_option) {
>> +		warning(_("Could not find section in .gitmodules where path=%s"), path);
>> +		return -1;
>> +	}
>> +	strbuf_addstr(&sect, "submodule.");
>> +	strbuf_addstr(&sect, path_option->util);
>> +	if (git_config_rename_section_in_file(".gitmodules", sect.buf, NULL) < 0) {
>> +		/* Maybe the user already did that, don't error out here */
>> +		warning(_("Could not remove .gitmodules entry for %s"), path);
>> +		return -1;
>> +	}
>> +	strbuf_release(&sect);
>> +	return 0;
>> +}
> 
> This question applies for this function and a few more functions in this
> patch that has the same characteristics.
> 
> If we're in a state when we need to return non-zero, we don't do any
> cleaning (that is strbuf_release()). Since this file is in the part
> called libgit AFAIK, shouldn't we always clean after us?

Right you are, thanks for bringing that up. The last return needs a
strbuf_release(), will fix that in v4.

> Would it make sense to have different return values for different
> errors?

I don't think so. The caller only wants to know if the modification
of the .gitmodules file happened or not, so he can decide if that
needs to be staged.

Re: [PATCH v4 5/5] rm: delete .gitmodules entry of submodules removed from the work tree

[Jens Lehmann]

Currently using "git rm" on a submodule removes the submodule's work tree
from that of the superproject and the gitlink from the index. But the
submodule's section in .gitmodules is left untouched, which is a leftover
of the now removed submodule and might irritate users (as opposed to the
setting in .git/config, this must stay as a reminder that the user showed
interest in this submodule so it will be repopulated later when an older
commit is checked out).

Let "git rm" help the user by not only removing the submodule from the
work tree but by also removing the "submodule.<submodule name>" section
from the .gitmodules file and stage both. This doesn't happen when the
"--cached" option is used, as it would modify the work tree. This also
silently does nothing when no .gitmodules file is found and only issues a
warning when it doesn't have a section for this submodule. This is because
the user might just use plain gitlinks without the .gitmodules file or has
already removed the section by hand before issuing the "git rm" command
(in which case the warning reminds him that rm would have done that for
him). Only when .gitmodules is found and contains merge conflicts the rm
command will fail and tell the user to resolve the conflict before trying
again.

Also extend the man page to inform the user about this new feature. While
at it promote the submodule sub-section to a chapter as it made not much
sense under "REMOVING FILES THAT HAVE DISAPPEARED FROM THE FILESYSTEM".

In t7610 three uses of "git rm submod" had to be replaced with "git rm
--cached submod" because that test expects .gitmodules and the work tree
to stay untouched. Also in t7400 the tests for the remaining settings in
the .gitmodules file had to be changed to assert that these settings are
missing.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---

This version fixes the missing strbuf_release() noticed by Frederik.


 Documentation/git-rm.txt   |  8 ++--
 builtin/rm.c               | 19 +++++++--
 submodule.c                | 33 ++++++++++++++++
 submodule.h                |  1 +
 t/t3600-rm.sh              | 98 +++++++++++++++++++++++++++++++++++++++++++---
 t/t7400-submodule-basic.sh | 14 ++-----
 t/t7610-mergetool.sh       |  6 +--
 7 files changed, 154 insertions(+), 25 deletions(-)

diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt
index 1d876c2..9d731b4 100644
--- a/Documentation/git-rm.txt
+++ b/Documentation/git-rm.txt
@@ -134,14 +134,16 @@ use the following command:
 git diff --name-only --diff-filter=D -z | xargs -0 git rm --cached
 ----------------

-Submodules
-~~~~~~~~~~
+SUBMODULES
+----------
 Only submodules using a gitfile (which means they were cloned
 with a Git version 1.7.8 or newer) will be removed from the work
 tree, as their repository lives inside the .git directory of the
 superproject. If a submodule (or one of those nested inside it)
 still uses a .git directory, `git rm` will fail - no matter if forced
-or not - to protect the submodule's history.
+or not - to protect the submodule's history. If it exists the
+submodule.<name> section in the linkgit:gitmodules[5] file will also
+be removed and that file will be staged (unless --cached or -n are used).

 A submodule is considered up-to-date when the HEAD is the same as
 recorded in the index, no tracked files are modified and no untracked
diff --git a/builtin/rm.c b/builtin/rm.c
index df85f98..a9d45dd 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -283,6 +283,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	struct pathspec pathspec;
 	char *seen;

+	gitmodules_config();
 	git_config(git_default_config, NULL);

 	argc = parse_options(argc, argv, prefix, builtin_rm_options,
@@ -324,7 +325,10 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 			continue;
 		ALLOC_GROW(list.entry, list.nr + 1, list.alloc);
 		list.entry[list.nr].name = ce->name;
-		list.entry[list.nr++].is_submodule = S_ISGITLINK(ce->ce_mode);
+		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 your changes to .gitmodules or stash them to proceed"));
 	}

 	if (pathspec.nr) {
@@ -396,13 +400,15 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	 * in the middle)
 	 */
 	if (!index_only) {
-		int removed = 0;
+		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) {
 				if (is_empty_dir(path)) {
 					if (!rmdir(path)) {
 						removed = 1;
+						if (!remove_path_from_gitmodules(path))
+							gitmodules_modified = 1;
 						continue;
 					}
 				} else {
@@ -410,9 +416,14 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 					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;
 					strbuf_release(&buf);
 					/* Fallthrough and let remove_path() fail. */
 				}
@@ -424,6 +435,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 			if (!removed)
 				die_errno("git rm: '%s'", path);
 		}
+		if (gitmodules_modified)
+			stage_updated_gitmodules();
 	}

 	if (active_cache_changed) {
diff --git a/submodule.c b/submodule.c
index 1c2714f..35c745e 100644
--- a/submodule.c
+++ b/submodule.c
@@ -81,6 +81,39 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)
 	return 0;
 }

+/*
+ * Try to remove the "submodule.<name>" section from .gitmodules where the given
+ * path is configured. Return 0 only if a .gitmodules file was found, a section
+ * with the correct path=<path> setting was found and we could remove it.
+ */
+int remove_path_from_gitmodules(const char *path)
+{
+	struct strbuf sect = STRBUF_INIT;
+	struct string_list_item *path_option;
+
+	if (!file_exists(".gitmodules")) /* Do nothing without .gitmodules */
+		return -1;
+
+	if (gitmodules_is_unmerged)
+		die(_("Cannot change unmerged .gitmodules, resolve merge conflicts first"));
+
+	path_option = unsorted_string_list_lookup(&config_name_for_path, path);
+	if (!path_option) {
+		warning(_("Could not find section in .gitmodules where path=%s"), path);
+		return -1;
+	}
+	strbuf_addstr(&sect, "submodule.");
+	strbuf_addstr(&sect, path_option->util);
+	if (git_config_rename_section_in_file(".gitmodules", sect.buf, NULL) < 0) {
+		/* Maybe the user already did that, don't error out here */
+		warning(_("Could not remove .gitmodules entry for %s"), path);
+		strbuf_release(&sect);
+		return -1;
+	}
+	strbuf_release(&sect);
+	return 0;
+}
+
 void stage_updated_gitmodules(void)
 {
 	struct strbuf buf = STRBUF_INIT;
diff --git a/submodule.h b/submodule.h
index e339891..7beec48 100644
--- a/submodule.h
+++ b/submodule.h
@@ -13,6 +13,7 @@ enum {

 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,
 		const char *path);
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 5c87b55..639cb70 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -263,6 +263,7 @@ test_expect_success 'rm removes subdirectories recursively' '
 '

 cat >expect <<EOF
+M  .gitmodules
 D  submod
 EOF

@@ -270,6 +271,15 @@ cat >expect.modified <<EOF
  M submod
 EOF

+cat >expect.cached <<EOF
+D  submod
+EOF
+
+cat >expect.both_deleted<<EOF
+D  .gitmodules
+D  submod
+EOF
+
 test_expect_success 'rm removes empty submodules from work tree' '
 	mkdir submod &&
 	git update-index --add --cacheinfo 160000 $(git rev-parse HEAD) submod &&
@@ -281,16 +291,20 @@ test_expect_success 'rm removes empty submodules from work tree' '
 	git rm submod &&
 	test ! -e submod &&
 	git status -s -uno --ignore-submodules=none > actual &&
-	test_cmp expect actual
+	test_cmp expect actual &&
+	test_must_fail git config -f .gitmodules submodule.sub.url &&
+	test_must_fail git config -f .gitmodules submodule.sub.path
 '

-test_expect_success 'rm removes removed submodule from index' '
+test_expect_success 'rm removes removed submodule from index and .gitmodules' '
 	git reset --hard &&
 	git submodule update &&
 	rm -rf submod &&
 	git rm submod &&
 	git status -s -uno --ignore-submodules=none > actual &&
-	test_cmp expect actual
+	test_cmp expect actual &&
+	test_must_fail git config -f .gitmodules submodule.sub.url &&
+	test_must_fail git config -f .gitmodules submodule.sub.path
 '

 test_expect_success 'rm removes work tree of unmodified submodules' '
@@ -299,7 +313,9 @@ test_expect_success 'rm removes work tree of unmodified submodules' '
 	git rm submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none > actual &&
-	test_cmp expect actual
+	test_cmp expect actual &&
+	test_must_fail git config -f .gitmodules submodule.sub.url &&
+	test_must_fail git config -f .gitmodules submodule.sub.path
 '

 test_expect_success 'rm removes a submodule with a trailing /' '
@@ -333,6 +349,72 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none > actual &&
+	test_cmp expect actual &&
+	test_must_fail git config -f .gitmodules submodule.sub.url &&
+	test_must_fail git config -f .gitmodules submodule.sub.path
+'
+
+test_expect_success 'rm --cached leaves work tree of populated submodules and .gitmodules alone' '
+	git reset --hard &&
+	git submodule update &&
+	git rm --cached submod &&
+	test -d submod &&
+	test -f submod/.git &&
+	git status -s -uno >actual &&
+	test_cmp expect.cached actual &&
+	git config -f .gitmodules submodule.sub.url &&
+	git config -f .gitmodules submodule.sub.path
+'
+
+test_expect_success 'rm --dry-run does not touch the submodule or .gitmodules' '
+	git reset --hard &&
+	git submodule update &&
+	git rm -n submod &&
+	test -f submod/.git &&
+	git diff-index --exit-code HEAD
+'
+
+test_expect_success 'rm does not complain when no .gitmodules file is found' '
+	git reset --hard &&
+	git submodule update &&
+	git rm .gitmodules &&
+	git rm submod >actual 2>actual.err &&
+	! test -s actual.err &&
+	! test -d submod &&
+	! test -f submod/.git &&
+	git status -s -uno >actual &&
+	test_cmp expect.both_deleted actual
+'
+
+test_expect_success 'rm will error out on a modified .gitmodules file unless staged' '
+	git reset --hard &&
+	git submodule update &&
+	git config -f .gitmodules foo.bar true &&
+	test_must_fail git rm submod >actual 2>actual.err &&
+	test -s actual.err &&
+	test -d submod &&
+	test -f submod/.git &&
+	git diff-files --quiet -- submod &&
+	git add .gitmodules &&
+	git rm submod >actual 2>actual.err &&
+	! test -s actual.err &&
+	! test -d submod &&
+	! test -f submod/.git &&
+	git status -s -uno >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'rm issues a warning when section is not found in .gitmodules' '
+	git reset --hard &&
+	git submodule update &&
+	git config -f .gitmodules --remove-section submodule.sub &&
+	git add .gitmodules &&
+	echo "warning: Could not find section in .gitmodules where path=submod" >expect.err &&
+	git rm submod >actual 2>actual.err &&
+	test_i18ncmp expect.err actual.err &&
+	! test -d submod &&
+	! test -f submod/.git &&
+	git status -s -uno >actual &&
 	test_cmp expect actual
 '

@@ -427,7 +509,9 @@ test_expect_success 'rm of a conflicted populated submodule with different HEAD
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none > actual &&
-	test_cmp expect actual
+	test_cmp expect actual &&
+	test_must_fail git config -f .gitmodules submodule.sub.url &&
+	test_must_fail git config -f .gitmodules submodule.sub.path
 '

 test_expect_success 'rm of a conflicted populated submodule with modifications fails unless forced' '
@@ -446,7 +530,9 @@ test_expect_success 'rm of a conflicted populated submodule with modifications f
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none > actual &&
-	test_cmp expect actual
+	test_cmp expect actual &&
+	test_must_fail git config -f .gitmodules submodule.sub.url &&
+	test_must_fail git config -f .gitmodules submodule.sub.path
 '

 test_expect_success 'rm of a conflicted populated submodule with untracked files fails unless forced' '
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 5ee97b0..88293e6 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -773,13 +773,11 @@ test_expect_success 'submodule add --name allows to replace a submodule with ano
 			test_cmp expect .git
 		) &&
 		echo "repo" >expect &&
-		git config -f .gitmodules submodule.repo.path >actual &&
-		test_cmp expect actual &&
+		test_must_fail git config -f .gitmodules submodule.repo.path &&
 		git config -f .gitmodules submodule.repo_new.path >actual &&
 		test_cmp expect actual&&
 		echo "$submodurl/repo" >expect &&
-		git config -f .gitmodules submodule.repo.url >actual &&
-		test_cmp expect actual &&
+		test_must_fail git config -f .gitmodules submodule.repo.url &&
 		echo "$submodurl/bare.git" >expect &&
 		git config -f .gitmodules submodule.repo_new.url >actual &&
 		test_cmp expect actual &&
@@ -799,12 +797,8 @@ test_expect_success 'submodule add with an existing name fails unless forced' '
 		git rm repo &&
 		test_must_fail git submodule add -q --name repo_new "$submodurl/repo.git" repo &&
 		test ! -d repo &&
-		echo "repo" >expect &&
-		git config -f .gitmodules submodule.repo_new.path >actual &&
-		test_cmp expect actual&&
-		echo "$submodurl/bare.git" >expect &&
-		git config -f .gitmodules submodule.repo_new.url >actual &&
-		test_cmp expect actual &&
+		test_must_fail git config -f .gitmodules submodule.repo_new.path &&
+		test_must_fail git config -f .gitmodules submodule.repo_new.url &&
 		echo "$submodurl/bare.git" >expect &&
 		git config submodule.repo_new.url >actual &&
 		test_cmp expect actual &&
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index d526b1d..05d9db0 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -253,7 +253,7 @@ test_expect_success 'deleted vs modified submodule' '
     git checkout -b test6 branch1 &&
     git submodule update -N &&
     mv submod submod-movedaside &&
-    git rm submod &&
+    git rm --cached submod &&
     git commit -m "Submodule deleted from branch" &&
     git checkout -b test6.a test6 &&
     test_must_fail git merge master &&
@@ -322,7 +322,7 @@ test_expect_success 'file vs modified submodule' '
     git checkout -b test7 branch1 &&
     git submodule update -N &&
     mv submod submod-movedaside &&
-    git rm submod &&
+    git rm --cached submod &&
     echo not a submodule >submod &&
     git add submod &&
     git commit -m "Submodule path becomes file" &&
@@ -453,7 +453,7 @@ test_expect_success 'submodule in subdirectory' '
 test_expect_success 'directory vs modified submodule' '
     git checkout -b test11 branch1 &&
     mv submod submod-movedaside &&
-    git rm submod &&
+    git rm --cached submod &&
     mkdir submod &&
     echo not a submodule >submod/file16 &&
     git add submod/file16 &&
-- 
1.8.4.rc0.199.g3f237fc

Re: [PATCH v4 5/5] rm: delete .gitmodules entry of submodules removed from the work tree

[Junio C Hamano]

Thanks, will replace the top two commits and queue.  Looks like we
are getting ready for 'next'?

Re: [PATCH v4 5/5] rm: delete .gitmodules entry of submodules removed from the work tree

[Jens Lehmann]

Am 06.08.2013 23:11, schrieb Junio C Hamano:
> Thanks, will replace the top two commits and queue.  Looks like we
> are getting ready for 'next'?

I hope so, I'm not aware of any open issues.

Re: [PATCH v4 5/5] rm: delete .gitmodules entry of submodules removed from the work tree

[Fredrik Gustafsson]

On Tue, Aug 06, 2013 at 02:11:56PM -0700, Junio C Hamano wrote:
> Thanks, will replace the top two commits and queue.  Looks like we
> are getting ready for 'next'?

I'm a bit curious about if we should move towards a reentrent libgit
(which would for example make multithreading easier) or not.

If so, I suggest that this patch only use die() in builtin/. However I
know that there's a lot of die() all over libgit today, I'm curious
about what direction we're heading.

-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iveqy@iveqy.com

Re: [PATCH v4 5/5] rm: delete .gitmodules entry of submodules removed from the work tree

[Jens Lehmann]

Am 07.08.2013 20:28, schrieb Fredrik Gustafsson:
> On Tue, Aug 06, 2013 at 02:11:56PM -0700, Junio C Hamano wrote:
>> Thanks, will replace the top two commits and queue.  Looks like we
>> are getting ready for 'next'?
> 
> I'm a bit curious about if we should move towards a reentrent libgit
> (which would for example make multithreading easier) or not.

I'm not aware of such an effort in core Git (I always thought that
libgit2 is the project doing what you seem to aim for).

> If so, I suggest that this patch only use die() in builtin/. However I
> know that there's a lot of die() all over libgit today, I'm curious
> about what direction we're heading.

The die() calls are just one part. Global variables are another issue,
we have memory which is implicitly freed on exit ... so unless we
commit ourselves to fix all those issues I see no point in moving the
die() calls into builtin/ in my series.

Re: [PATCH v4 5/5] rm: delete .gitmodules entry of submodules removed from the work tree

[Fredrik Gustafsson]

On Thu, Aug 08, 2013 at 07:11:04PM +0200, Jens Lehmann wrote:
> Am 07.08.2013 20:28, schrieb Fredrik Gustafsson:
> > On Tue, Aug 06, 2013 at 02:11:56PM -0700, Junio C Hamano wrote:
> >> Thanks, will replace the top two commits and queue.  Looks like we
> >> are getting ready for 'next'?
> > 
> > I'm a bit curious about if we should move towards a reentrent libgit
> > (which would for example make multithreading easier) or not.
> 
> I'm not aware of such an effort in core Git (I always thought that
> libgit2 is the project doing what you seem to aim for).
> 
> > If so, I suggest that this patch only use die() in builtin/. However I
> > know that there's a lot of die() all over libgit today, I'm curious
> > about what direction we're heading.
> 
> The die() calls are just one part. Global variables are another issue,
> we have memory which is implicitly freed on exit ... so unless we
> commit ourselves to fix all those issues I see no point in moving the
> die() calls into builtin/ in my series.

Okay, thanks for your answer.

-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iveqy@iveqy.com