[PATCH/RFC 0/3] Teach mv to move submodules

[PATCH/RFC 0/3] Teach mv to move submodules

[Jens Lehmann]

This is the last topic I intend to finish before preparing the recursive
submodule update. The other prerequisites for that next step are Heiko's
"allow more sources for config values" and "fetch moved submodules
on-demand" topics, which are necessary to populate submodules that were
not present in the currently checked out commit and to have all commits
of moved or currently not present submodules fetched for later checkout
respectively. But these are other threads ...

Let's enable users to move submodules (just as easy as they can already
remove them with current Git with "git rm") by using a plain "git mv".
While using "git submodule update" on a moved submodule cloned by a
coworker will still leave the submodule's work tree lying around at the
old path, that will be fixed by the recursive submodule update (and is
not different from what Git currently does with removed submodules).
Replacing a directory containing files tracked by Git or vice versa is
still not possible, another issue to be fixed by the recursive submodule
update. I tried to CC all parties which showed interest in this topic,
hopefully I didn't forget anyone.

Jens Lehmann (3):
  Teach mv to move submodules together with their work trees
  Teach mv to move submodules using a gitfile
  Teach mv to update the path entry in .gitmodules for moved submodules

 builtin/mv.c  | 126 ++++++++++++++++++++++++++++++++++------------------------
 submodule.c   | 126 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 submodule.h   |   3 ++
 t/t7001-mv.sh |  94 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 297 insertions(+), 52 deletions(-)

-- 
1.8.2.377.g1bdb7d0

[PATCH/RFC 1/3] 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 034fec9..361028d 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -117,55 +117,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 a845b15..4c57f61 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -255,4 +255,38 @@ test_expect_success SYMLINKS 'git mv should overwrite file with a 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.2.377.g1bdb7d0

[PATCH/RFC 2/3] 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.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
 builtin/mv.c  | 19 ++++++++++++++----
 submodule.c   | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 submodule.h   |  1 +
 t/t7001-mv.sh | 19 ++++++++++++++++++
 4 files changed, 99 insertions(+), 4 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 361028d..609bbb8 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>"),
@@ -65,7 +66,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;
@@ -84,6 +85,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	source = copy_pathspec(prefix, argv, argc, 0);
 	modes = xcalloc(argc, sizeof(enum update_mode));
 	dest_path = copy_pathspec(prefix, argv + argc, 1, 0);
+	submodule_gitfile = xcalloc(argc, sizeof(char *));

 	if (dest_path[0][0] == '\0')
 		/* special case: "." was normalized to "" */
@@ -119,8 +121,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;
@@ -215,9 +223,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 975bc87..eba9b42 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1001,3 +1001,67 @@ 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 core_worktree_setting = STRBUF_INIT;
+	struct strbuf configfile_name = STRBUF_INIT;
+	struct strbuf gitfile_content = STRBUF_INIT;
+	struct strbuf gitfile_name = STRBUF_INIT;
+	const char *real_work_tree = real_path(work_tree);
+	const char *pathspec[] = { real_work_tree, git_dir, NULL };
+	const char *max_prefix = common_prefix(pathspec);
+	FILE *fp;
+
+	if (max_prefix) {       /* skip common prefix */
+		size_t max_prefix_len = strlen(max_prefix);
+		real_work_tree += max_prefix_len;
+		git_dir += max_prefix_len;
+	}
+
+	/*
+	 * Update gitfile
+	 */
+	strbuf_addstr(&gitfile_content, "gitdir: ");
+	if (real_work_tree[0]) {
+		const char *s = real_work_tree;
+		do {
+			strbuf_addstr(&gitfile_content, "../");
+			s++;
+		} while ((s = strchr(s, '/')));
+	}
+	strbuf_addstr(&gitfile_content, git_dir);
+	strbuf_addch(&gitfile_content, '\n');
+
+	strbuf_addf(&gitfile_name, "%s/.git", work_tree);
+	fp = fopen(gitfile_name.buf, "w");
+	if (!fp)
+		die(_("Could not create git link %s"), gitfile_name.buf);
+	fprintf(fp, gitfile_content.buf);
+	fclose(fp);
+
+	strbuf_release(&gitfile_content);
+	strbuf_release(&gitfile_name);
+
+	/*
+	 * Update core.worktree setting
+	 */
+	if (git_dir[0]) {
+		const char *s = git_dir;
+		do {
+			strbuf_addstr(&core_worktree_setting, "../");
+			s++;
+		} while ((s = strchr(s, '/')));
+	}
+	strbuf_addstr(&core_worktree_setting, real_work_tree);
+
+	strbuf_addf(&configfile_name, "%s/config", git_dir);
+	if (git_config_set_in_file(configfile_name.buf, "core.worktree",
+				   core_worktree_setting.buf))
+		die(_("Could not set core.worktree in %s"),
+		    configfile_name.buf);
+
+	strbuf_release(&core_worktree_setting);
+	strbuf_release(&configfile_name);
+}
diff --git a/submodule.h b/submodule.h
index 3dc1b3f..0c27c53 100644
--- a/submodule.h
+++ b/submodule.h
@@ -35,5 +35,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 4c57f61..d824464 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -289,4 +289,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.2.377.g1bdb7d0

[PATCH/RFC 3/3] 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.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
 builtin/mv.c  |  8 +++++++-
 submodule.c   | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 submodule.h   |  2 ++
 t/t7001-mv.sh | 41 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 112 insertions(+), 1 deletion(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 609bbb8..36e5605 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -57,7 +57,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")),
@@ -71,6 +71,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,
@@ -228,6 +229,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)
@@ -239,6 +242,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 eba9b42..fb742b4 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,67 @@ static struct sha1_array ref_tips_after_fetch;
  */
 static int gitmodules_is_unmerged;

+/*
+ * Try to update the "path" entry in the "submodule.<name>" section of the
+ * .gitmodules file.
+ */
+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 whithout .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;
+	struct stat st;
+	int pos;
+	struct cache_entry *ce;
+	int namelen = strlen(".gitmodules");
+
+	pos = cache_name_pos(".gitmodules", strlen(".gitmodules"));
+	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_file_from_cache(".gitmodules") < 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;
diff --git a/submodule.h b/submodule.h
index 0c27c53..39c0321 100644
--- a/submodule.h
+++ b/submodule.h
@@ -11,6 +11,8 @@ enum {
 	RECURSE_SUBMODULES_ON = 2
 };

+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);
 int submodule_config(const char *var, const char *value, void *cb);
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index d824464..c144d21 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -304,6 +304,47 @@ 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 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
 '
-- 
1.8.2.377.g1bdb7d0

Re: [PATCH/RFC 2/3] Teach mv to move submodules using a gitfile

[Junio C Hamano]

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

> diff --git a/submodule.c b/submodule.c
> index 975bc87..eba9b42 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1001,3 +1001,67 @@ int merge_submodule(unsigned char result[20], const char *path,
> ...
> +	if (!fp)
> +		die(_("Could not create git link %s"), gitfile_name.buf);
> +	fprintf(fp, gitfile_content.buf);

Perhaps.

	fprintf(fp, "%s", gitfile_content.buf);

Re: [PATCH/RFC 2/3] Teach mv to move submodules using a gitfile

[Jens Lehmann]

Am 10.04.2013 01:08, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
>> diff --git a/submodule.c b/submodule.c
>> index 975bc87..eba9b42 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -1001,3 +1001,67 @@ int merge_submodule(unsigned char result[20], const char *path,
>> ...
>> +	if (!fp)
>> +		die(_("Could not create git link %s"), gitfile_name.buf);
>> +	fprintf(fp, gitfile_content.buf);
> 
> Perhaps.
> 
> 	fprintf(fp, "%s", gitfile_content.buf);

Sure. Will fix in v2.

[PATCH v2 2/3] 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.

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

Am 10.04.2013 18:59, schrieb Jens Lehmann:
> Am 10.04.2013 01:08, schrieb Junio C Hamano:
>> Jens Lehmann <Jens.Lehmann@web.de> writes:
>>
>>> diff --git a/submodule.c b/submodule.c
>>> index 975bc87..eba9b42 100644
>>> --- a/submodule.c
>>> +++ b/submodule.c
>>> @@ -1001,3 +1001,67 @@ int merge_submodule(unsigned char result[20], const char *path,
>>> ...
>>> +	if (!fp)
>>> +		die(_("Could not create git link %s"), gitfile_name.buf);
>>> +	fprintf(fp, gitfile_content.buf);
>>
>> Perhaps.
>>
>> 	fprintf(fp, "%s", gitfile_content.buf);
> 
> Sure. Will fix in v2.

Here we go.


 builtin/mv.c  | 19 ++++++++++++++----
 submodule.c   | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 submodule.h   |  1 +
 t/t7001-mv.sh | 19 ++++++++++++++++++
 4 files changed, 99 insertions(+), 4 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 361028d..609bbb8 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>"),
@@ -65,7 +66,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;
@@ -84,6 +85,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	source = copy_pathspec(prefix, argv, argc, 0);
 	modes = xcalloc(argc, sizeof(enum update_mode));
 	dest_path = copy_pathspec(prefix, argv + argc, 1, 0);
+	submodule_gitfile = xcalloc(argc, sizeof(char *));

 	if (dest_path[0][0] == '\0')
 		/* special case: "." was normalized to "" */
@@ -119,8 +121,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;
@@ -215,9 +223,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 975bc87..9a3eb85 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1001,3 +1001,67 @@ 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 core_worktree_setting = STRBUF_INIT;
+	struct strbuf configfile_name = STRBUF_INIT;
+	struct strbuf gitfile_content = STRBUF_INIT;
+	struct strbuf gitfile_name = STRBUF_INIT;
+	const char *real_work_tree = real_path(work_tree);
+	const char *pathspec[] = { real_work_tree, git_dir, NULL };
+	const char *max_prefix = common_prefix(pathspec);
+	FILE *fp;
+
+	if (max_prefix) {       /* skip common prefix */
+		size_t max_prefix_len = strlen(max_prefix);
+		real_work_tree += max_prefix_len;
+		git_dir += max_prefix_len;
+	}
+
+	/*
+	 * Update gitfile
+	 */
+	strbuf_addstr(&gitfile_content, "gitdir: ");
+	if (real_work_tree[0]) {
+		const char *s = real_work_tree;
+		do {
+			strbuf_addstr(&gitfile_content, "../");
+			s++;
+		} while ((s = strchr(s, '/')));
+	}
+	strbuf_addstr(&gitfile_content, git_dir);
+	strbuf_addch(&gitfile_content, '\n');
+
+	strbuf_addf(&gitfile_name, "%s/.git", work_tree);
+	fp = fopen(gitfile_name.buf, "w");
+	if (!fp)
+		die(_("Could not create git link %s"), gitfile_name.buf);
+	fprintf(fp, "%s", gitfile_content.buf);
+	fclose(fp);
+
+	strbuf_release(&gitfile_content);
+	strbuf_release(&gitfile_name);
+
+	/*
+	 * Update core.worktree setting
+	 */
+	if (git_dir[0]) {
+		const char *s = git_dir;
+		do {
+			strbuf_addstr(&core_worktree_setting, "../");
+			s++;
+		} while ((s = strchr(s, '/')));
+	}
+	strbuf_addstr(&core_worktree_setting, real_work_tree);
+
+	strbuf_addf(&configfile_name, "%s/config", git_dir);
+	if (git_config_set_in_file(configfile_name.buf, "core.worktree",
+				   core_worktree_setting.buf))
+		die(_("Could not set core.worktree in %s"),
+		    configfile_name.buf);
+
+	strbuf_release(&core_worktree_setting);
+	strbuf_release(&configfile_name);
+}
diff --git a/submodule.h b/submodule.h
index 3dc1b3f..0c27c53 100644
--- a/submodule.h
+++ b/submodule.h
@@ -35,5 +35,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 4c57f61..d824464 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -289,4 +289,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.2.1.345.gb37ac0e

Re: [PATCH v2 2/3] Teach mv to move submodules using a gitfile

[Ramkumar Ramachandra]

Jens Lehmann wrote:
> 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.

Isn't it untrue that the git directory is stored in
.git/modules/<name>: it is stored in .git/modules/path/to/module.  I
thought the whole point of this complex scheme was to avoid name
conflicts with submodules with the same name in other directories.
Then why aren't you moving the object store as well?  What happens if
I try to create a submodule in the old path of this moved submodule in
the future?  How will you fail, and how will the user recover from
this?

A nit on the wording.  Perhaps: "the .git file in the worktree must be
adjusted to point to $GITDIR, and core.worktree must be set to point
to the worktree."

> 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.

Oh God.  Can't you figure out at mv-time if the submodule uses a
gitfile?  Why are you storing it?  Where are you storing it exactly?

> diff --git a/builtin/mv.c b/builtin/mv.c
> index 361028d..609bbb8 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>"),
> @@ -65,7 +66,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;
> @@ -84,6 +85,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>         source = copy_pathspec(prefix, argv, argc, 0);
>         modes = xcalloc(argc, sizeof(enum update_mode));
>         dest_path = copy_pathspec(prefix, argv + argc, 1, 0);
> +       submodule_gitfile = xcalloc(argc, sizeof(char *));

This is cmd_mv, and you're allocating argc number of char * pointers
to submodule_gitfile.  Why?  Are you guaranteed that there are argc
number of submodule_gitfiles?

>         if (dest_path[0][0] == '\0')
>                 /* special case: "." was normalized to "" */
> @@ -119,8 +121,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);

What?!  read_gitfile() returns the path to the git directory, if it is
found.  How are you assigning the path to a $GITDIR to a variable
named submodule_gitfile?

> +                               if (submodule_gitfile[i])
> +                                       submodule_gitfile[i] = xstrdup(submodule_gitfile[i]);

Doesn't read as smoothly, but saves you an extra char *.  Okay.

> +                               strbuf_release(&submodule_dotgit);
>                         } else {
>                                 const char *src_w_slash = add_slash(src);
>                                 int last, len_w_slash = length + 1;
> @@ -215,9 +223,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]);
> +               }

Okay, scratch my previous comment about allocating argc char *
pointers for submodule_gitfile().  Since your logic is in a loop that
loops argc times, you really have no choice.

> diff --git a/submodule.c b/submodule.c
> index 975bc87..9a3eb85 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1001,3 +1001,67 @@ 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 core_worktree_setting = STRBUF_INIT;
> +       struct strbuf configfile_name = STRBUF_INIT;
> +       struct strbuf gitfile_content = STRBUF_INIT;
> +       struct strbuf gitfile_name = STRBUF_INIT;
> +       const char *real_work_tree = real_path(work_tree);
> +       const char *pathspec[] = { real_work_tree, git_dir, NULL };
> +       const char *max_prefix = common_prefix(pathspec);
> +       FILE *fp;
> +
> +       if (max_prefix) {       /* skip common prefix */
> +               size_t max_prefix_len = strlen(max_prefix);
> +               real_work_tree += max_prefix_len;
> +               git_dir += max_prefix_len;
> +       }
> +
> +       /*
> +        * Update gitfile
> +        */
> +       strbuf_addstr(&gitfile_content, "gitdir: ");
> +       if (real_work_tree[0]) {
> +               const char *s = real_work_tree;
> +               do {
> +                       strbuf_addstr(&gitfile_content, "../");
> +                       s++;
> +               } while ((s = strchr(s, '/')));
> +       }
> +       strbuf_addstr(&gitfile_content, git_dir);
> +       strbuf_addch(&gitfile_content, '\n');

Yuck.  Just yuck.  Why don't you just fopen the gitfile and write one
line to it directly (ie. the absolute path of the worktree)?  Why do
you need a strbuf for this at all?  Ah, absolute paths would break if
you moved the entire superproject to a different location.  This is
reason #347 I don't like keeping GITDIRs in the superproject's
.git/modules/.  In any case, why don't you use relative_path() in
path.c instead of reinventing the wheel?

> +       strbuf_addf(&gitfile_name, "%s/.git", work_tree);

Why work_tree, not real_work_tree?
strbuf is an overkill for this: why not mkpath()?

> +       fp = fopen(gitfile_name.buf, "w");
> +       if (!fp)
> +               die(_("Could not create git link %s"), gitfile_name.buf);
> +       fprintf(fp, "%s", gitfile_content.buf);

Not checking the return value of fprintf() for possible failure?

> +       fclose(fp);
> +
> +       strbuf_release(&gitfile_content);
> +       strbuf_release(&gitfile_name);
> +
> +       /*
> +        * Update core.worktree setting
> +        */
> +       if (git_dir[0]) {
> +               const char *s = git_dir;
> +               do {
> +                       strbuf_addstr(&core_worktree_setting, "../");
> +                       s++;
> +               } while ((s = strchr(s, '/')));
> +       }

Can't use relative_path()?

> +       strbuf_addstr(&core_worktree_setting, real_work_tree);
> +
> +       strbuf_addf(&configfile_name, "%s/config", git_dir);

mkpath() to avoid the strbuf.  If you have to use strbufs, why aren't
you re-using a "pathbuf" strbuf?

Re: [PATCH/RFC 1/3] Teach mv to move submodules together with their work trees

[Ramkumar Ramachandra]

Jens Lehmann wrote:
> 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.

Why does it search for a submodule with a trailing slash in the index?
 You make it sound like it's doing something unnatural; in reality, it
does this because it executes lstat() on the filesystem path
specified, and the stat mode matches S_ISDIR (because it _is_ a
directory on the filesystem).  It is stored in the index as an entry
(without a trailing slash) with the mode 160000, gitlink.

What happens if I attempt to git mv oldpath/ newpath/ (with the
literal slashes, probably because I'm using a stupid shell
completion)?

> Fix that by searching for the name without a trailing slash and continue
> if it is a submodule.

So, the correct solution is not to "search for a name without a
trailing slash", but rather to handle the gitlink entry in the index
appropriately.

> Then rename() will move the submodule work tree just
> like it moves a file.

What is this rename() function you're talking about?  I don't see it anywhere.

> diff --git a/builtin/mv.c b/builtin/mv.c
> index 034fec9..361028d 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -117,55 +117,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);

I didn't understand this.  Why does it have to be a gitlink if it is
stored at index position >= 0?
I'm assuming the else-case has nothing to do with the actual moving
but rather something specific to directories (enumerating entries in
it?), which is why it doesn't get executed when we find a gitlink.

> +                       } 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;
>                                 }

Mostly unchanged, but I didn't understand the line first = -1 - first
in the original.  What is it doing?  So, I'm guessing first is the
cache position of the directory itself, and last stores the index of
the "last" entry in the cache?  What does that even mean?

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

This is exactly what was tripping us up earlier.  Can you explain what
last - first < 1 means?

> +                               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;
>                                 }

Mostly unchanged, but hard to review because I can't easily see what
changed and what didn't.

> -                               argc += last - first;

Why did you remove this line?

Re: [PATCH/RFC 3/3] Teach mv to update the path entry in .gitmodules for moved submodules

[Ramkumar Ramachandra]

Jens Lehmann wrote:
> Currently using "git mv" on a submodule moves the submodule's work tree in
> that of the superproject.

It's not "Currently": it's the result of your last two patches.  The
wording is very unclear.  Perhaps: As a result of the last two patches
in this series, a 'git mv' moves the submodule directory to a new path
in the superproject's worktree, and updates the cache entry in the
index appropriately.

> 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.

I'm frankly a little surprised that your previous two patches didn't
break any tests.  That's probably because we don't have enough tests
to exercise git-core with submodules.  I'm not saying that it's a bad
thing though: submodules are still immature, and tight tests would get
in the way of new patches.

> 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

How will git-core work without a .gitmodules?  Shouldn't we create a
fresh .gitmodules here?

> 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).

Shouldn't these two cases issue different warnings?

Besides, why is this explanation missing in your rm patch's commit message?

> diff --git a/builtin/mv.c b/builtin/mv.c
> index 609bbb8..36e5605 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -239,6 +242,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>                         rename_cache_entry_at(pos, dst);
>         }
>
> +       if (gitmodules_modified)
> +               stage_updated_gitmodules();
> +

I'm unhappy with this, but we I'll have to live with my
dissatisfaction until we get rid of ".gitmodules" (if that ever
happens).  mv has a clear task: it should perform a filesystem mv,
manipulate the index, and write out the changed index.  Adding an
unrelated file to the index has nothing to do with its primary task.

> diff --git a/submodule.c b/submodule.c
> index eba9b42..fb742b4 100644
> --- a/submodule.c
> +++ b/submodule.c

All my comments in the rm review apply here too, so I won't repeat myself.

> @@ -10,6 +10,7 @@
>  #include "string-list.h"
>  #include "sha1-array.h"
>  #include "argv-array.h"
> +#include "blob.h"

Interesting.  Let's see why you need blob.h.

>  static struct string_list config_name_for_path;
>  static struct string_list config_fetch_recurse_submodules_for_name;
> @@ -30,6 +31,67 @@ static struct sha1_array ref_tips_after_fetch;
>   */
>  static int gitmodules_is_unmerged;
>
> +/*
> + * Try to update the "path" entry in the "submodule.<name>" section of the
> + * .gitmodules file.
> + */
> +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 whithout .gitmodules */
> +               return -1;

s/whithout/without.  Why are you not returning an error()?  Don't you
want to tell the user that no ".gitmodules" was found?

> +void stage_updated_gitmodules(void)

void return?  Don't you want the caller to know whether we were
successful or not?

Why does the name contain "updated"?  Why does the function care if I
updated the .gitmodules or not?  It's just staging .gitmodules, as it
is on the filesystem.

> +{
> +       struct strbuf buf = STRBUF_INIT;
> +       struct stat st;
> +       int pos;
> +       struct cache_entry *ce;
> +       int namelen = strlen(".gitmodules");
> +
> +       pos = cache_name_pos(".gitmodules", strlen(".gitmodules"));

And you forgot about namelen just like that.

> +       if (pos < 0) {
> +               warning(_("could not find .gitmodules in index"));
> +               return;
> +       }

What could this mean?  I might have an untracked .gitmodules file in
my worktree, or I might not have a .gitmodules file at all.  Since
you're already checking the latter case in the previous function,
can't you persist the result, and return a more sensible error?

> +       ce = active_cache[pos];
> +       ce->ce_flags = namelen;
> +       if (strbuf_read_file(&buf, ".gitmodules", 0) < 0)
> +               die(_("reading updated .gitmodules failed"));

You're reading it because?  What does "_updated_ .gitmodules" mean here?

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

I don't understand why you're taking the pains to fill out a cache_entry.

> +       if (remove_file_from_cache(".gitmodules") < 0)
> +               die(_("unable to remove .gitmodules from index"));

You already have pos, so why not just remove_cache_entry_at()?

> +       if (write_sha1_file(buf.buf, buf.len, blob_type, ce->sha1))
> +               die(_("adding updated .gitmodules failed"));

Ah, you need blob.h for blob_type.

Wait, why are you just reading and writing .gitmodules?  What changed?
 And why are you manually writing a blob to the object store?  Doesn't
git-core already do this if you just add it to the index? See the
S_IFLNK case in index_path().

What happens if there's a race?  Shouldn't you be locking .gitmodules
before updating it, like we do with the index and just about
everything else?

> +       if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE))
> +               die(_("staging updated .gitmodules failed"));

This is all you need: it calls index_path() and writes your blob
object to the database.

Re: [PATCH/RFC 1/3] Teach mv to move submodules together with their work trees

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

>> Then rename() will move the submodule work tree just
>> like it moves a file.
>
> What is this rename() function you're talking about?  I don't see it anywhere.

"man 2 rename"; it is called from a generic part of builtin/mv.c to
rename one path to another and can move both files and directories.

>> +                       if (first >= 0) {
>> +                               if (!S_ISGITLINK(active_cache[first]->ce_mode))
>> +                                       die (_("Huh? Directory %s is in index and no submodule?"), src);
>
> I didn't understand this.  Why does it have to be a gitlink if it is
> stored at index position >= 0?

The path is not in the middle of the conflict, but the index records
something that is not a gitlink.  E.g. you start from A (regular file)
in the index, "rm A && mkdir A" and make it the top of a working
tree of a separate project.  "git mv A elsewhere" will say "src_is_dir"
but the index still thinks A should be a regular file blob.

> I'm assuming the else-case has nothing to do with the actual moving
> but rather something specific to directories (enumerating entries in
> it?), which is why it doesn't get executed when we find a gitlink.

It wants to move all the paths in the directory to a new
destination, e.g. "git mv srcdir dstdir", and update 


>> +                       } 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;
>>                                 }
>
> Mostly unchanged, but I didn't understand the line first = -1 - first
> in the original.  What is it doing?  So, I'm guessing first is the
> cache position of the directory itself, and last stores the index of
> the "last" entry in the cache?  What does that even mean?

cache_name_pos() either returns the position for a path at stage #0
that exists in the index, or the position the given path _would_ be
inserted if you were to add it to the index for a path that does not
exist in the index at stage #0 (I think this part of the code does
not consider that the given path is unmerged, either by being sloppy
or detecting that case much earlier---I didn't check), and when
doing the latter, it encodes the position by negating it and
offsetting it by 1 (otherwise you cannot tell "it would come at the
very beginning" and "it is at the very beginning", because negated
zero is still zero).  The "-1 - first" is an idiom used everywhere
by callers of cache_name_pos() to recover the latter from the
returned value.

If you start at a position "src/" would have been inserted, and
iterate over the index while the entry's path prefix-matches with
"src/", you will find where the entries in the "src/" directory
ends.

>> +                               if (last - first < 1)
>> +                                       bad = _("source directory is empty");
>
> This is exactly what was tripping us up earlier.  Can you explain what
> last - first < 1 means?

I think the above covers it.

Asking questions to learn the basic part of Git internals on this
list, e.g. "I found this existing code, and I do not understand what
it is doing. Can somebody shed a light on it?", is perfectly fine,
but can you do so outside the review discussion?

It clutters the review discussions when such a request for education
is labeled as if it were a review or mixed with a message with
genuine review comments.

Re: [PATCH/RFC 1/3] Teach mv to move submodules together with their work trees

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Why does it search for a submodule with a trailing slash in the index?
>  You make it sound like it's doing something unnatural; in reality, it
> does this because it executes lstat() on the filesystem path
> specified, and the stat mode matches S_ISDIR (because it _is_ a
> directory on the filesystem).  It is stored in the index as an entry
> (without a trailing slash) with the mode 160000, gitlink.
>
> What happens if I attempt to git mv oldpath/ newpath/ (with the
> literal slashes, probably because I'm using a stupid shell
> completion)?

I think it should work.

	mkdir a && >a/f && git add a/f && git mv a/ b/

>> Fix that by searching for the name without a trailing slash and continue
>> if it is a submodule.
>
> So, the correct solution is not to "search for a name without a
> trailing slash", but rather to handle the gitlink entry in the index
> appropriately.

For moving an entire directory's contents, because the index is
flat, you would look for "name/", because you know all of the paths
contained in it will share that prefix.

But when dealing with a submodule, you need to see if the path that
found to be a directory in the working tree is a gitlink in the
index.  And the way to do so is to look for it without trailing
slash, because there is nothing hanging under it in the index.

So the right implementation of "handle appropriately" is to "search
without slash".  They are saying the same thing, and the latter is a
more specific way to point out in what way the existing code that
wanted to delegate moving to "submodule mv" and not having to worry
about gitlinks was unprepared for it, and what change is needed to
make it "handle appropriately".

So I think there is no need to rephrase here, but your comment makes
me wonder if the patch covers the case where you have a submodule at
"a/x" and the user does "git mv a/ b/".  The src_is_dir thing will
notice "a/" is a directory, finds all the paths inside a/ including
"a/x" (and we won't see any paths inside the submodule's working
tree) to "b/", and update the cache entries and the working tree.
Does the adjusting of the path for that moved submodule, which is a
theme for [PATCH 3/3], cover that case, too?

Another thing to wonder is if we correctly reject "git mv a/x/f here"
in the same example where a/x is a directory.  The path is beyond
the lower boundary of our working tree and should not be touched.

Re: [PATCH/RFC 3/3] Teach mv to update the path entry in .gitmodules for moved submodules

[Junio C Hamano]

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

> 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.

Should it happen when the user has other changes to .gitmodules that
hasn't been updated to the index?

As this is done in the same final "per-path" loop as adjusting the
gitfile links, the worry I expressed in an earlier message about
"git mv a/ b/" when "a/x" is a submodule turns out to be unfounded,
which is good ;-)

Re: [PATCH v2 2/3] Teach mv to move submodules using a gitfile

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Jens Lehmann wrote:
>> 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.
>
> Isn't it untrue that the git directory is stored in
> .git/modules/<name>: it is stored in .git/modules/path/to/module.
>
> I thought the whole point of this complex scheme was to avoid name
> conflicts with submodules with the same name in other directories.

I think Jens is right on this one.  There are three things the code needs
to know about a submodule: its name, path and URL.

A canonical use case to think about is a project that builds an
appliance:

 * Its zero-th version only has the sources to the userspace for the
   appliance.

 * The first version adds the Linux kernel as a submodule bound at
   kernel/, taking it from git://k.org/linux-2.6.git/.

 * The second version adds a choice to build the appliance with the
   BSD kernel, and the project reorganizes the source tree to have
   Linux kernel at path linux/ and adds the bsd kernel at path bsd/.

 * By the time the third version is released, the URL to the Linux
   has migrated to git://k.org/linux.git/; it is still logically
   the same (i.e. continuation of) the old 2.6.git repository [*1*].

We would want to make it possible to "git checkout" smoothly between
these four versions.  Switching from v1 to v0 would have to remove
the submodule working tree at kernel/ but the user may want to
switch back to v1 without having to re-download the kernel
submodule, so the kernel/.git repository needs to be stashed away
somewhere.  Somewhere in $GIT_DIR of the superproject, but where?
Switching from v1 to v2 would need to move kernel/ to linux/ and
move kernel/.git to linux/.git.  The design choice made long time
ago (if you recall the collection of old threads I gave you some
time ago, this is what was called "Steven's three-level thing") was
to give a stable "logical" name for the Linux kernel component, so
that no matter where in the working tree the version that happens to
be at the tip of the current branch has it, we know where in the
superproject's .git/modules it is found.

So at the second version when we move the submodule that used to be
at kernel/ to linux/, we move the working tree of it, adjust the
"path" of the submodule, but keep the name.  And that name gives an
identity to the submodule, and that is what is used as a key inside
$GIT_DIR of the superproject to decide where the repository
(together with its object store) of the submodule is stashed away.


[Footnote]

*1* I mentioned the URL thing only for completeness; it does not
come into play in the "checkout" scenario, but when you start
thinking about remote interactions, you need to be aware of how that
value and the one copied to the configuration upon "submodule init"
need to be managed. Which is a separate topic but is an integral
part of the canonical example.

Ideally, a user who has followed along the life of this project
should:

 * first encounter git://k.org/linux-2.6.git/ in v1; "git submodule
   init" would copy it to her .git/config in the superproject.

 * later notice that .gitmodules has git://k.org/linux.git/ location
   that she hasn't seen for the submodule, and is given a chance to
   have the URL entry updated in her .git/config.

This is becuase even when she checks out an older branch that has
2.6 in .gitmodules, "git submodule update" _should_ go to the new
URL, not to the defunct 2.6 URL.  We do the "copy initially", but do
not do the latter "offer a chance to update when seeing a new one"
(at least, not yet).