{"thread":{"id":"35105","subject":"Spurious warning when moving a file in presence of submodules","startedAt":"2013-10-11T14:29:10Z","lastAt":"2013-10-17T18:24:05Z","messageCount":8,"participants":["Matthieu Moy","Jens Lehmann","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"228738","messageId":"vpq38o7nao9.fsf@anie.imag.fr","threadId":"35105","inReplyTo":null,"subject":"Spurious warning when moving a file in presence of submodules","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-10-11T14:29:10Z","receivedAt":"2013-10-11T14:29:10Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Hi,\n\nI'm getting this warning:\n\n warning: Could not find section in .gitmodules where path=XXX\n\nwhenever I use \"git mv\" to move a file in a repository containing a\nsubmodule. The file is outside the submodule and is completely\nunrelated, so I do not understand the intent of the warning.\n\nMy understanding (without looking at the code in detail) is that Git\ntries to be clever about submodule renames, hence checks whether the\nsource file is a submodule. But then if the lookup fails, it should just\nsilently move on to \"normal file move\" mode I guess...\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"228745","messageId":"52583B00.8040700@web.de","threadId":"35105","inReplyTo":"vpq38o7nao9.fsf@anie.imag.fr","subject":"Re: Spurious warning when moving a file in presence of submodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-10-11T17:53:04Z","receivedAt":"2013-10-11T17:53:04Z","isPatch":false,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Hi Matthieu,\n\nAm 11.10.2013 16:29, schrieb Matthieu Moy:\n> I'm getting this warning:\n> \n> warning: Could not find section in .gitmodules where path=XXX\n> \n> whenever I use \"git mv\" to move a file in a repository containing a\n> submodule. The file is outside the submodule and is completely\n> unrelated, so I do not understand the intent of the warning.\n> \n> My understanding (without looking at the code in detail) is that Git\n> tries to be clever about submodule renames, hence checks whether the\n> source file is a submodule. But then if the lookup fails, it should just\n> silently move on to \"normal file move\" mode I guess...\n\nRight. Thanks for reporting, I can reproduce that here and am currently\nlooking into that.\n"},{"id":"228903","messageId":"525A8965.3040407@web.de","threadId":"35105","inReplyTo":"52583B00.8040700@web.de","subject":"[PATCH] mv: Fix spurious warning when moving a file in presence of submodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-10-13T11:52:05Z","receivedAt":"2013-10-13T11:52:05Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"In commit 0656781fa \"git mv\" learned to update the submodule path in the\n.gitmodules file when moving a submodule in the work tree. But since that\ncommit update_path_in_gitmodules() gets called no matter if we moved a\nsubmodule or a regular file, which is wrong and leads to a bogus warning\nwhen moving a regular file in a repo containing a .gitmodules file:\n\n warning: Could not find section in .gitmodules where path=\n\nFix that by only calling update_path_in_gitmodules() when moving a\nsubmodule. To achieve that, we introduce the special SUBMODULE_WITH_GITDIR\ndefine to distinguish the cases where we also have to connect work tree\nand git directory from those where we only need to update the .gitmodules\nsetting.\n\nA test for submodules using a .git directory together with a .gitmodules\nfile has been added to t7001. Even though newer git versions will always\nuse a gitfile when cloning submodules, repositories cloned with older git\nversions will still use this layout.\n\nReported-by: Matthieu Moy \nSigned-off-by: Jens Lehmann \n---\n\nAm 11.10.2013 19:53, schrieb Jens Lehmann:\n> Am 11.10.2013 16:29, schrieb Matthieu Moy:\n>> I'm getting this warning:\n>>\n>> warning: Could not find section in .gitmodules where path=XXX\n>>\n>> whenever I use \"git mv\" to move a file in a repository containing a\n>> submodule. The file is outside the submodule and is completely\n>> unrelated, so I do not understand the intent of the warning.\n>>\n>> My understanding (without looking at the code in detail) is that Git\n>> tries to be clever about submodule renames, hence checks whether the\n>> source file is a submodule. But then if the lookup fails, it should just\n>> silently move on to \"normal file move\" mode I guess...\n> \n> Right. Thanks for reporting, I can reproduce that here and am currently\n> looking into that.\n\nAnd this is the fix for it, which I believe is stuff for maint.\n\n\n builtin/mv.c | 13 +++++++++----\n t/t7001-mv.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 35 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex aec79d1..2e0e61b 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -55,6 +55,7 @@ static const char *add_slash(const char *path)\n }\n\n static struct lock_file lock_file;\n+#define SUBMODULE_WITH_GITDIR ((const char *)1)\n\n int cmd_mv(int argc, const char **argv, const char *prefix)\n {\n@@ -132,6 +133,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\t\tsubmodule_gitfile[i] = read_gitfile(submodule_dotgit.buf);\n \t\t\t\tif (submodule_gitfile[i])\n \t\t\t\t\tsubmodule_gitfile[i] = xstrdup(submodule_gitfile[i]);\n+\t\t\t\telse\n+\t\t\t\t\tsubmodule_gitfile[i] = SUBMODULE_WITH_GITDIR;\n \t\t\t\tstrbuf_release(&submodule_dotgit);\n \t\t\t} else {\n \t\t\t\tconst char *src_w_slash = add_slash(src);\n@@ -230,10 +233,12 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\tif (!show_only && mode != INDEX) {\n \t\t\tif (rename(src, dst) < 0 && !ignore_errors)\n \t\t\t\tdie_errno (_(\"renaming '%s' failed\"), src);\n-\t\t\tif (submodule_gitfile[i])\n-\t\t\t\tconnect_work_tree_and_git_dir(dst, submodule_gitfile[i]);\n-\t\t\tif (!update_path_in_gitmodules(src, dst))\n-\t\t\t\tgitmodules_modified = 1;\n+\t\t\tif (submodule_gitfile[i]) {\n+\t\t\t\tif (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR)\n+\t\t\t\t\tconnect_work_tree_and_git_dir(dst, submodule_gitfile[i]);\n+\t\t\t\tif (!update_path_in_gitmodules(src, dst))\n+\t\t\t\t\tgitmodules_modified = 1;\n+\t\t\t}\n \t\t}\n\n \t\tif (mode == WORKING_DIRECTORY)\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex d432f42..b90e985 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -293,6 +293,32 @@ test_expect_success 'git mv moves a submodule with a .git directory and no .gitm\n \tgit diff-files --quiet\n '\n\n+test_expect_success 'git mv moves a submodule with a .git directory and .gitmodules' '\n+\trm -rf mod &&\n+\tgit reset --hard &&\n+\tgit submodule update &&\n+\tentry=\"$(git ls-files --stage sub | cut -f 1)\" &&\n+\t(\n+\t\tcd sub &&\n+\t\trm -f .git &&\n+\t\tcp -a ../.git/modules/sub .git &&\n+\t\tGIT_WORK_TREE=. git config --unset core.worktree\n+\t) &&\n+\tmkdir mod &&\n+\tgit mv sub mod/sub &&\n+\t! test -e sub &&\n+\t[ \"$entry\" = \"$(git ls-files --stage mod/sub | cut -f 1)\" ] &&\n+\t(\n+\t\tcd mod/sub &&\n+\t\tgit status\n+\t) &&\n+\techo mod/sub >expected &&\n+\tgit config -f .gitmodules submodule.sub.path >actual &&\n+\ttest_cmp expected actual &&\n+\tgit update-index --refresh &&\n+\tgit diff-files --quiet\n+'\n+\n test_expect_success 'git mv moves a submodule with gitfile' '\n \trm -rf mod/sub &&\n \tgit reset --hard &&\n-- \n1.8.4.474.g128a96c.dirty\n"},{"id":"228905","messageId":"vpqr4bp6wkh.fsf@anie.imag.fr","threadId":"35105","inReplyTo":"525A8965.3040407@web.de","subject":"Re: [PATCH] mv: Fix spurious warning when moving a file in presence of submodules","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-10-13T15:05:02Z","receivedAt":"2013-10-13T15:05:02Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jens Lehmann writes:\n\n> static struct lock_file lock_file;\n> +#define SUBMODULE_WITH_GITDIR ((const char *)1)\n\nI don't like very much hardcoded addresses like this. Are you 100% sure\naddress 1 will never be returned by xstrdup on any platform? The risk is\nsmall if not negligible, but I'm unconfortable with this. Isn't there an\nalternative (NULL, or empty string) that is guaranteed to never happen?\n\n> +test_expect_success 'git mv moves a submodule with a .git directory and .gitmodules' '\n\nThis doesn't seem to test the problem I was having (move a file, not a\nsubmodule). Is this intentional?\n\nIn any case, this fixes my problem, thanks!\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"228906","messageId":"525AE868.9050207@web.de","threadId":"35105","inReplyTo":"vpqr4bp6wkh.fsf@anie.imag.fr","subject":"Re: [PATCH] mv: Fix spurious warning when moving a file in presence of submodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-10-13T18:37:28Z","receivedAt":"2013-10-13T18:37:28Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 13.10.2013 17:05, schrieb Matthieu Moy:\n> Jens Lehmann writes:\n> \n>> static struct lock_file lock_file;\n>> +#define SUBMODULE_WITH_GITDIR ((const char *)1)\n> \n> I don't like very much hardcoded addresses like this. Are you 100% sure\n> address 1 will never be returned by xstrdup on any platform? The risk is\n> small if not negligible, but I'm unconfortable with this. Isn't there an\n> alternative (NULL, or empty string) that is guaranteed to never happen?\n\nAll alternatives I could think of would require an extra array storing\nthat information, which I dismissed for performance and memory footprint\nreasons (NULL is already taken for not being a submodule). I think 1 is\none of the safest hard coded addresses as on sane systems accessing the\nzeropage will trigger a segfault. And if someday someone wants to free\nthe memory I expect the special casing of 1 to be rather obvious. But\nI'm open to alternatives and would change that if people disagree.\n\n>> +test_expect_success 'git mv moves a submodule with a .git directory and .gitmodules' '\n> \n> This doesn't seem to test the problem I was having (move a file, not a\n> submodule). Is this intentional?\n\nYes. The first idea was to simply move the update_path_in_gitmodules()\ninto the \"if (submodule_gitfile[i])\"-block, but that would have resulted\nin not updating .gitmodules for submodules with a .git directory, which\nI would consider a bug. So I thought this was worth an extra test case,\nwhile I wasn't sure testing mv of a regular file to not issue a warning\nis a very useful test case in submodule context.\n\n> In any case, this fixes my problem, thanks!\n\nSure, glad to help and thanks for testing!\n"},{"id":"228923","messageId":"20131014054048.GC25344@google.com","threadId":"35105","inReplyTo":"vpqr4bp6wkh.fsf@anie.imag.fr","subject":"Re: [PATCH] mv: Fix spurious warning when moving a file in presence of submodules","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-10-14T05:40:48Z","receivedAt":"2013-10-14T05:40:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Matthieu Moy wrote:\n> Jens Lehmann writes:\n\n>> static struct lock_file lock_file;\n>> +#define SUBMODULE_WITH_GITDIR ((const char *)1)\n>\n> I don't like very much hardcoded addresses like this. Are you 100% sure\n> address 1 will never be returned by xstrdup on any platform? The risk is\n> small if not negligible, but I'm unconfortable with this.\n\nI haven't checked what the standards say, but in practice I think it's\nokay. An alternative would be to do something like\n\n\tstatic const char SUBMODULE_WITH_GITDIR[] = \"*** (submodule with gitdir) ***\";\n\nwhich is a bit more error-prone because attempts to use it as a string\nwouldn't crash. We use (void *) 1 in the same way a few places currently.\n\nThanks, both.\n"},{"id":"228926","messageId":"vpqfvs42fre.fsf@anie.imag.fr","threadId":"35105","inReplyTo":"20131014054048.GC25344@google.com","subject":"Re: [PATCH] mv: Fix spurious warning when moving a file in presence of submodules","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-10-14T12:33:57Z","receivedAt":"2013-10-14T12:33:57Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jonathan Nieder writes:\n\n> Matthieu Moy wrote:\n>> Jens Lehmann writes:\n>\n>>> static struct lock_file lock_file;\n>>> +#define SUBMODULE_WITH_GITDIR ((const char *)1)\n>>\n>> I don't like very much hardcoded addresses like this. Are you 100% sure\n>> address 1 will never be returned by xstrdup on any platform? The risk is\n>> small if not negligible, but I'm unconfortable with this.\n>\n> I haven't checked what the standards say, but in practice I think it's\n> okay. [...] We use (void *) 1 in the same way a few places currently.\n\nOK, fine with me.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"229126","messageId":"20131017182405.GR9464@google.com","threadId":"35105","inReplyTo":"525A8965.3040407@web.de","subject":"Re: [PATCH] mv: Fix spurious warning when moving a file in presence of submodules","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-10-17T18:24:05Z","receivedAt":"2013-10-17T18:24:05Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jens Lehmann wrote:\n\n> In commit 0656781fa \"git mv\" learned to update the submodule path in the\n> .gitmodules file when moving a submodule in the work tree. But since that\n> commit update_path_in_gitmodules() gets called no matter if we moved a\n> submodule or a regular file, which is wrong and leads to a bogus warning\n> when moving a regular file in a repo containing a .gitmodules file:\n>\n> warning: Could not find section in .gitmodules where path=\n[...]\n> Reported-by: Matthieu Moy \n> Signed-off-by: Jens Lehmann \n> ---\n[...]\n> And this is the fix for it, which I believe is stuff for maint.\n\nThanks again, and sorry to leave this hanging. I had some vague ideas\nfor improvement:\n\n * style nits: test is a little long, making it hard to take in at a\n glance; tests tend to try to avoid pipelines to avoid losing the\n exit code from git commands; usual style is to use \"test\" instead\n of \"[\" consistently\n\n * would be nice to have another test that makes sure the \"move a\n file, not submodule\" case keeps working\n\nBut those can easily happen as changes on top, and as you mentioned,\nit's probably worth someone interested (e.g., me :)) making a pass to\nclean up the style of the rest of the script anyway.\n\nThe patch as-is is obviously good. Thanks again for the quick\ndiagnosis and fix.\n\nReviewed-by: Jonathan Nieder \n"}]}