Spurious warning when moving a file in presence of submodules

Hi,

I'm getting this warning:

  warning: Could not find section in .gitmodules where path=XXX

whenever I use "git mv" to move a file in a repository containing a
submodule. The file is outside the submodule and is completely
unrelated, so I do not understand the intent of the warning.

My understanding (without looking at the code in detail) is that Git
tries to be clever about submodule renames, hence checks whether the
source file is a submodule. But then if the lookup fails, it should just
silently move on to "normal file move" mode I guess...

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Hi Matthieu,

Am 11.10.2013 16:29, schrieb Matthieu Moy:

I'm getting this warning:

  warning: Could not find section in .gitmodules where path=XXX

whenever I use "git mv" to move a file in a repository containing a
submodule. The file is outside the submodule and is completely
unrelated, so I do not understand the intent of the warning.

My understanding (without looking at the code in detail) is that Git
tries to be clever about submodule renames, hence checks whether the
source file is a submodule. But then if the lookup fails, it should just
silently move on to "normal file move" mode I guess...

Right. Thanks for reporting, I can reproduce that here and am currently
looking into that.

In commit 0656781fa "git mv" learned to update the submodule path in the
.gitmodules file when moving a submodule in the work tree. But since that
commit update_path_in_gitmodules() gets called no matter if we moved a
submodule or a regular file, which is wrong and leads to a bogus warning
when moving a regular file in a repo containing a .gitmodules file:

    warning: Could not find section in .gitmodules where path=<filename>

Fix that by only calling update_path_in_gitmodules() when moving a
submodule. To achieve that, we introduce the special SUBMODULE_WITH_GITDIR
define to distinguish the cases where we also have to connect work tree
and git directory from those where we only need to update the .gitmodules
setting.

A test for submodules using a .git directory together with a .gitmodules
file has been added to t7001. Even though newer git versions will always
use a gitfile when cloning submodules, repositories cloned with older git
versions will still use this layout.

Reported-by: Matthieu Moy <redacted>
Signed-off-by: Jens Lehmann <redacted>
---

Am 11.10.2013 19:53, schrieb Jens Lehmann:

Am 11.10.2013 16:29, schrieb Matthieu Moy:

quoted

I'm getting this warning:

  warning: Could not find section in .gitmodules where path=XXX

whenever I use "git mv" to move a file in a repository containing a
submodule. The file is outside the submodule and is completely
unrelated, so I do not understand the intent of the warning.

My understanding (without looking at the code in detail) is that Git
tries to be clever about submodule renames, hence checks whether the
source file is a submodule. But then if the lookup fails, it should just
silently move on to "normal file move" mode I guess...

Right. Thanks for reporting, I can reproduce that here and am currently
looking into that.

And this is the fix for it, which I believe is stuff for maint.


 builtin/mv.c  | 13 +++++++++----
 t/t7001-mv.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index aec79d1..2e0e61b 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c

@@ -55,6 +55,7 @@ static const char *add_slash(const char *path)
 }

 static struct lock_file lock_file;
+#define SUBMODULE_WITH_GITDIR ((const char *)1)

 int cmd_mv(int argc, const char **argv, const char *prefix)
 {

@@ -132,6 +133,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				submodule_gitfile[i] = read_gitfile(submodule_dotgit.buf);
 				if (submodule_gitfile[i])
 					submodule_gitfile[i] = xstrdup(submodule_gitfile[i]);
+				else
+					submodule_gitfile[i] = SUBMODULE_WITH_GITDIR;
 				strbuf_release(&submodule_dotgit);
 			} else {
 				const char *src_w_slash = add_slash(src);

@@ -230,10 +233,12 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		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 (!update_path_in_gitmodules(src, dst))
-				gitmodules_modified = 1;
+			if (submodule_gitfile[i]) {
+				if (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR)
+					connect_work_tree_and_git_dir(dst, submodule_gitfile[i]);
+				if (!update_path_in_gitmodules(src, dst))
+					gitmodules_modified = 1;
+			}
 		}

 		if (mode == WORKING_DIRECTORY)

diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index d432f42..b90e985 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh

@@ -293,6 +293,32 @@ 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 a .git directory and .gitmodules' '
+	rm -rf mod &&
+	git reset --hard &&
+	git submodule update &&
+	entry="$(git ls-files --stage sub | cut -f 1)" &&
+	(
+		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
+	) &&
+	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 'git mv moves a submodule with gitfile' '
 	rm -rf mod/sub &&
 	git reset --hard &&

-- 
1.8.4.474.g128a96c.dirty

Jens Lehmann wrote:

In commit 0656781fa "git mv" learned to update the submodule path in the
.gitmodules file when moving a submodule in the work tree. But since that
commit update_path_in_gitmodules() gets called no matter if we moved a
submodule or a regular file, which is wrong and leads to a bogus warning
when moving a regular file in a repo containing a .gitmodules file:

    warning: Could not find section in .gitmodules where path=<filename>

[...]

Reported-by: Matthieu Moy <redacted>
Signed-off-by: Jens Lehmann <redacted>
---

[...]

And this is the fix for it, which I believe is stuff for maint.

Thanks again, and sorry to leave this hanging.  I had some vague ideas
for improvement:

 * style nits: test is a little long, making it hard to take in at a
   glance; tests tend to try to avoid pipelines to avoid losing the
   exit code from git commands; usual style is to use "test" instead
   of "[" consistently

 * would be nice to have another test that makes sure the "move a
   file, not submodule" case keeps working

But those can easily happen as changes on top, and as you mentioned,
it's probably worth someone interested (e.g., me :)) making a pass to
clean up the style of the rest of the script anyway.

The patch as-is is obviously good.  Thanks again for the quick
diagnosis and fix.

Reviewed-by: Jonathan Nieder <redacted>