Spurious warning when moving a file in presence of submodules

Spurious warning when moving a file in presence of submodules

[Matthieu Moy]

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/

Re: Spurious warning when moving a file in presence of submodules

[Jens Lehmann]

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.

[PATCH] mv: Fix spurious warning when moving a file in presence of submodules

[Jens Lehmann]

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 <Matthieu.Moy@grenoble-inp.fr>
Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---

Am 11.10.2013 19:53, schrieb Jens Lehmann:
> 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.

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

Re: [PATCH] mv: Fix spurious warning when moving a file in presence of submodules

[Matthieu Moy]

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

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

I don't like very much hardcoded addresses like this. Are you 100% sure
address 1 will never be returned by xstrdup on any platform? The risk is
small if not negligible, but I'm unconfortable with this. Isn't there an
alternative (NULL, or empty string) that is guaranteed to never happen?

> +test_expect_success 'git mv moves a submodule with a .git directory and .gitmodules' '

This doesn't seem to test the problem I was having (move a file, not a
submodule). Is this intentional?

In any case, this fixes my problem, thanks!

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

Re: [PATCH] mv: Fix spurious warning when moving a file in presence of submodules

[Jens Lehmann]

Am 13.10.2013 17:05, schrieb Matthieu Moy:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
>>  static struct lock_file lock_file;
>> +#define SUBMODULE_WITH_GITDIR ((const char *)1)
> 
> I don't like very much hardcoded addresses like this. Are you 100% sure
> address 1 will never be returned by xstrdup on any platform? The risk is
> small if not negligible, but I'm unconfortable with this. Isn't there an
> alternative (NULL, or empty string) that is guaranteed to never happen?

All alternatives I could think of would require an extra array storing
that information, which I dismissed for performance and memory footprint
reasons (NULL is already taken for not being a submodule). I think 1 is
one of the safest hard coded addresses as on sane systems accessing the
zeropage will trigger a segfault. And if someday someone wants to free
the memory I expect the special casing of 1 to be rather obvious. But
I'm open to alternatives and would change that if people disagree.

>> +test_expect_success 'git mv moves a submodule with a .git directory and .gitmodules' '
> 
> This doesn't seem to test the problem I was having (move a file, not a
> submodule). Is this intentional?

Yes. The first idea was to simply move the update_path_in_gitmodules()
into the "if (submodule_gitfile[i])"-block, but that would have resulted
in not updating .gitmodules for submodules with a .git directory, which
I would consider a bug. So I thought this was worth an extra test case,
while I wasn't sure testing mv of a regular file to not issue a warning
is a very useful test case in submodule context.

> In any case, this fixes my problem, thanks!

Sure, glad to help and thanks for testing!

Re: [PATCH] mv: Fix spurious warning when moving a file in presence of submodules

[Jonathan Nieder]

Matthieu Moy wrote:
> Jens Lehmann <Jens.Lehmann@web.de> writes:

>>  static struct lock_file lock_file;
>> +#define SUBMODULE_WITH_GITDIR ((const char *)1)
>
> I don't like very much hardcoded addresses like this. Are you 100% sure
> address 1 will never be returned by xstrdup on any platform? The risk is
> small if not negligible, but I'm unconfortable with this.

I haven't checked what the standards say, but in practice I think it's
okay.  An alternative would be to do something like

	static const char SUBMODULE_WITH_GITDIR[] = "*** (submodule with gitdir) ***";

which is a bit more error-prone because attempts to use it as a string
wouldn't crash.  We use (void *) 1 in the same way a few places currently.

Thanks, both.

Re: [PATCH] mv: Fix spurious warning when moving a file in presence of submodules

[Matthieu Moy]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Matthieu Moy wrote:
>> Jens Lehmann <Jens.Lehmann@web.de> writes:
>
>>>  static struct lock_file lock_file;
>>> +#define SUBMODULE_WITH_GITDIR ((const char *)1)
>>
>> I don't like very much hardcoded addresses like this. Are you 100% sure
>> address 1 will never be returned by xstrdup on any platform? The risk is
>> small if not negligible, but I'm unconfortable with this.
>
> I haven't checked what the standards say, but in practice I think it's
> okay.  [...]  We use (void *) 1 in the same way a few places currently.

OK, fine with me.

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

Re: [PATCH] mv: Fix spurious warning when moving a file in presence of submodules

[Jonathan Nieder]

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 <Matthieu.Moy@grenoble-inp.fr>
> Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
> ---
[...]
> 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 <jrnieder@gmail.com>