{"thread":{"id":"54746","subject":"BUG in fetching non-checked out submodule","startedAt":"2020-12-02T15:57:34Z","lastAt":"2020-12-09T14:02:07Z","messageCount":36,"participants":["Ralf Thielow","Philippe Blain","Junio C Hamano","Peter Kästle","Peter Kaestle","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"411166","messageId":"CAN0XMOLiS_8JZKF_wW70BvRRxkDHyUoa=Z3ODtB_Bd6f5Y=7JQ@mail.gmail.com","threadId":"54746","inReplyTo":null,"subject":"BUG in fetching non-checked out submodule","fromName":"Ralf Thielow","fromEmail":"ralf.thielow@gmail.com","sentAt":"2020-12-02T15:56:01Z","receivedAt":"2020-12-02T15:57:34Z","isPatch":false,"sender":{"key":"ralf.thielow@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1275832?v=4"},"body":"Hi,\n\nI have the current 'master' branch of git installed and get\nthe following error when fetching a submodule that is not\nchecked out.\n\nI've bisected this error down to commit\n1b7ac4e6d4 (submodules: fix of regression on fetching of\nnon-init subsub-repo, 2020-11-12)\n\n$ git version\ngit version 2.29.2.435.g72ffeb997e\n\n$ git config --get submodule.recurse\ntrue\n\n$ git submodule status\n-855827c583bc30645ba427885caa40c5b81764d2 sha1collisiondetection\n\n$ git fetch\nFetching submodule sha1collisiondetection\nFetching submodule sha1collisiondetection/sha1collisiondetection\nFetching submodule\nsha1collisiondetection/sha1collisiondetection/sha1collisiondetection\nFetching submodule\nsha1collisiondetection/sha1collisiondetection/sha1collisiondetection/sha1collisiondetection\n...\n\n$ git submodule update --checkout\nSubmodule path 'sha1collisiondetection': checked out\n'855827c583bc30645ba427885caa40c5b81764d2'\n\n$ git fetch\nFetching submodule sha1collisiondetection\n$\n\nRalf\n"},{"id":"411173","messageId":"CC0FA973-E37A-4BD3-B5A2-1436DD8DF16F@gmail.com","threadId":"54746","inReplyTo":"CAN0XMOLiS_8JZKF_wW70BvRRxkDHyUoa=Z3ODtB_Bd6f5Y=7JQ@mail.gmail.com","subject":"Re: BUG in fetching non-checked out submodule","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-12-02T17:19:59Z","receivedAt":"2020-12-02T17:20:59Z","isPatch":false,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Ralf,\n\n> Le 2 déc. 2020 à 10:56, Ralf Thielow a écrit :\n> \n> Hi,\n> \n> I have the current 'master' branch of git installed and get\n> the following error when fetching a submodule that is not\n> checked out.\n> \n> I've bisected this error down to commit\n> 1b7ac4e6d4 (submodules: fix of regression on fetching of\n> non-init subsub-repo, 2020-11-12)\n\nThanks for bisecting it. That commit wanted to fix a different bug\nrelated to nested submodules, and the route taken was simply\nreverting an earlier commit (a62387b (submodule.c: fetch in\nsubmodules git directory instead of in worktree, 2018-11-28).\n\nAs you discovered, it breaks other scenarios.\n\n> \n> $ git version\n> git version 2.29.2.435.g72ffeb997e\n> \n> $ git config --get submodule.recurse\n> true\n\nYeah, I think the test suite could make more efforts\nto run more tests with that setting turned 'on', but\nit would require significants efforts since it changes \nthe behaviour of several commands.\n\nMeta question: is there an easy way to run the whole test\nsuite with specific config options turned on ?\n\n> \n> $ git submodule status\n> -855827c583bc30645ba427885caa40c5b81764d2 sha1collisiondetection\n> \n> $ git fetch\n> Fetching submodule sha1collisiondetection\n> Fetching submodule sha1collisiondetection/sha1collisiondetection\n> Fetching submodule\n> sha1collisiondetection/sha1collisiondetection/sha1collisiondetection\n> Fetching submodule\n> sha1collisiondetection/sha1collisiondetection/sha1collisiondetection/sha1collisiondetection\n> ...\n> \n> $ git submodule update --checkout\n> Submodule path 'sha1collisiondetection': checked out\n> '855827c583bc30645ba427885caa40c5b81764d2'\n\nOk, you don't add '--init' but the submodule gets checked out,\nso it looks like you have 'submodule.active' set to a pathspec \nthat matches 'sha1collisiondetection'. Did you clone the git repo\nwith '--recurse-submodules', which would add '.' as the value of\n'submodule.active' ? Or maybe you manually configured that value\nin your global gitconfig ?\n\nThanks for the report,\n\nPhilippe.\n\n"},{"id":"411185","messageId":"xmqqzh2vkdu8.fsf@gitster.c.googlers.com","threadId":"54746","inReplyTo":"CC0FA973-E37A-4BD3-B5A2-1436DD8DF16F@gmail.com","subject":"Re: BUG in fetching non-checked out submodule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-02T23:06:23Z","receivedAt":"2020-12-02T23:07:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philippe Blain writes:\n\n> Thanks for bisecting it. That commit wanted to fix a different bug\n> related to nested submodules, and the route taken was simply\n> reverting an earlier commit (a62387b (submodule.c: fetch in\n> submodules git directory instead of in worktree, 2018-11-28).\n>\n> As you discovered, it breaks other scenarios.\n>\n>> \n>> $ git version\n>> git version 2.29.2.435.g72ffeb997e\n>> \n>> $ git config --get submodule.recurse\n>> true\n\nI think the current situation is probably worse. \n\nAs a short-term fix, we should revert 1b7ac4e6d4 until we can come\nup with a real fix, probably.\n\n> Yeah, I think the test suite could make more efforts\n> to run more tests with that setting turned 'on', but\n> it would require significants efforts since it changes \n> the behaviour of several commands.\n\nI am not sure if the question is about amount of efforts.\n\nA configuration variable is there to change the behaviour of\ncommands, so a test of a command that has been running happily and\nproducing a set of expected outcome with a configuration unset\nshould break the expectation when the configuration is set ---\notherwise there is no point in having a configuration variable.\n\n> Meta question: is there an easy way to run the whole test\n> suite with specific config options turned on ?\n\nHence, I do not think it even makes sense to have such an \"easy\nway\". If the \"fetch\" command, for example, is expected to change\nbehaviour depending on the value of submodule.recurse, a test\nwritten for the case where the variable is not set should produce\ndifferent outcome when the variable is set.\n\nWhat we need may be a better test coverage. submodule.recurse is a\nlater addition, and all tests written earlier do test how the commands\nbehave without the configuration being set. If one wants to change\nthe behaviour of these commands when the configuration is set, new\ntests to specify what the expected behaviour need to be added.\n\n> Thanks for the report,\n\nYup, thanks for helping out.\n"},{"id":"411206","messageId":"CAN0XMOKEG9HLuzf9ZRXyUs_uHTXagyCdghtP98rLVoPj_75UYQ@mail.gmail.com","threadId":"54746","inReplyTo":"CC0FA973-E37A-4BD3-B5A2-1436DD8DF16F@gmail.com","subject":"Re: BUG in fetching non-checked out submodule","fromName":"Ralf Thielow","fromEmail":"ralf.thielow@gmail.com","sentAt":"2020-12-03T07:45:16Z","receivedAt":"2020-12-03T07:46:49Z","isPatch":false,"sender":{"key":"ralf.thielow@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1275832?v=4"},"body":"Hi Philippe,\n\nAm Mi., 2. Dez. 2020 um 18:20 Uhr schrieb Philippe Blain\n:\n>\n> Ok, you don't add '--init' but the submodule gets checked out,\n> so it looks like you have 'submodule.active' set to a pathspec\n> that matches 'sha1collisiondetection'. Did you clone the git repo\n> with '--recurse-submodules', which would add '.' as the value of\n> 'submodule.active' ? Or maybe you manually configured that value\n> in your global gitconfig ?\n\nSry, I did not mention that I started with the submodule already\ninitialized and checked out. IIRC I deinitialized the submodule with\n'git submodule deinit -f sha1collisiondetection/' and ran 'git fetch'\nto see if it happens on the git repo, too, since I encountered the\nissue on another repo first.\n\nI can see the same misbehaviour when I run\n$ git -c submodule.recurse=false fetch --recurse-submodules\ntoo, so I think it doesn't have necessarily something to do with\nthe 'submodule.recursive' config, but with 'recurse submodules'\nin general.\n\nRalf\n"},{"id":"411208","messageId":"04968f5c-c8bd-c57e-d646-7c9f7691e1a8@nokia.com","threadId":"54746","inReplyTo":"xmqqzh2vkdu8.fsf@gitster.c.googlers.com","subject":"Re: BUG in fetching non-checked out submodule","fromName":"Peter Kästle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-12-03T07:54:47Z","receivedAt":"2020-12-03T07:56:01Z","isPatch":false,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"Hi,\n\nOn 03.12.20 00:06, Junio C Hamano wrote:\n> Philippe Blain writes:\n> \n>> Thanks for bisecting it. That commit wanted to fix a different bug\n>> related to nested submodules, and the route taken was simply\n>> reverting an earlier commit (a62387b (submodule.c: fetch in\n>> submodules git directory instead of in worktree, 2018-11-28).\n>>\n>> As you discovered, it breaks other scenarios.\n>>\n>>>\n>>> $ git version\n>>> git version 2.29.2.435.g72ffeb997e\n>>>\n>>> $ git config --get submodule.recurse\n>>> true\n> \n> I think the current situation is probably worse.\n> \n> As a short-term fix, we should revert 1b7ac4e6d4 until we can come\n> up with a real fix, probably.\n\nJunio: This is why I originally intended to commit the test case for the \ntestsuite separated from the revert and wanted to start a discussion \nabout the actual real fix for the issue:\nhttps://public-inbox.org/git/1604413399-63090-1-git-send-email-peter.kaestle@nokia.com/\n\nMy proposal would be to revert 1b7ac4e6d4 and isolate the test case \n\"test_expect_success 'setup nested submodule fetch test' '\" make it \n\"test_expect_failure\" and apply it instead, until we come up with a real \nsolution.\n\n>> Yeah, I think the test suite could make more efforts\n>> to run more tests with that setting turned 'on', but\n>> it would require significants efforts since it changes\n>> the behaviour of several commands.\n> \n> I am not sure if the question is about amount of efforts.\n> \n> A configuration variable is there to change the behaviour of\n> commands, so a test of a command that has been running happily and\n> producing a set of expected outcome with a configuration unset\n> should break the expectation when the configuration is set ---\n> otherwise there is no point in having a configuration variable.\n> \n>> Meta question: is there an easy way to run the whole test\n>> suite with specific config options turned on ?\n> \n> Hence, I do not think it even makes sense to have such an \"easy\n> way\". If the \"fetch\" command, for example, is expected to change\n> behaviour depending on the value of submodule.recurse, a test\n> written for the case where the variable is not set should produce\n> different outcome when the variable is set.\n> \n> What we need may be a better test coverage. submodule.recurse is a\n> later addition, and all tests written earlier do test how the commands\n> behave without the configuration being set. If one wants to change\n> the behaviour of these commands when the configuration is set, new\n> tests to specify what the expected behaviour need to be added.\n\nCan we start a new test suite for tests on which this variable is set? \nThe case of Ralf can be used as first test case.\n\nRalf: could you please send a command sequence, which recreates the \nrequired repository setup from scratch and is able to trigger your \nobservation? Then we can convert this into a test case.\n\nThanks.\n\n-- \nbest regards\n--peter;\n"},{"id":"411212","messageId":"034d340f-ae62-2c22-3c6c-c9b14e0de662@nokia.com","threadId":"54746","inReplyTo":"CAN0XMOKEG9HLuzf9ZRXyUs_uHTXagyCdghtP98rLVoPj_75UYQ@mail.gmail.com","subject":"Re: BUG in fetching non-checked out submodule","fromName":"Peter Kästle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-12-03T08:20:19Z","receivedAt":"2020-12-03T08:21:31Z","isPatch":false,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"Hi Ralf,\n\nOn 03.12.20 08:45, Ralf Thielow wrote:\n> Am Mi., 2. Dez. 2020 um 18:20 Uhr schrieb Philippe Blain\n> :\n>>\n>> Ok, you don't add '--init' but the submodule gets checked out,\n>> so it looks like you have 'submodule.active' set to a pathspec\n>> that matches 'sha1collisiondetection'. Did you clone the git repo\n>> with '--recurse-submodules', which would add '.' as the value of\n>> 'submodule.active' ? Or maybe you manually configured that value\n>> in your global gitconfig ?\n> \n> Sry, I did not mention that I started with the submodule already\n> initialized and checked out. IIRC I deinitialized the submodule with\n> 'git submodule deinit -f sha1collisiondetection/' and ran 'git fetch'\n> to see if it happens on the git repo, too, since I encountered the\n> issue on another repo first.\n> \n> I can see the same misbehaviour when I run\n> $ git -c submodule.recurse=false fetch --recurse-submodules\n> too, so I think it doesn't have necessarily something to do with\n> the 'submodule.recursive' config, but with 'recurse submodules'\n> in general.\nThis is interesting. There are several testcases using \n\"--recurse-submodules\". Have you tried, whether any of these trigger?\n\n-- \nkind regards\n--peter;\n\n\n"},{"id":"411215","messageId":"CAN0XMOLHM0mFvXcdiXJS_yD59rSTuyJpp9N9MLvcZ5LCC1-yZA@mail.gmail.com","threadId":"54746","inReplyTo":"034d340f-ae62-2c22-3c6c-c9b14e0de662@nokia.com","subject":"Re: BUG in fetching non-checked out submodule","fromName":"Ralf Thielow","fromEmail":"ralf.thielow@gmail.com","sentAt":"2020-12-03T09:38:28Z","receivedAt":"2020-12-03T09:39:46Z","isPatch":false,"sender":{"key":"ralf.thielow@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1275832?v=4"},"body":"Hi Peter,\n\nAm Do., 3. Dez. 2020 um 09:20 Uhr schrieb Peter Kästle\n:\n> > I can see the same misbehaviour when I run\n> > $ git -c submodule.recurse=false fetch --recurse-submodules\n> > too, so I think it doesn't have necessarily something to do with\n> > the 'submodule.recursive' config, but with 'recurse submodules'\n> > in general.\n> This is interesting. There are several testcases using\n> \"--recurse-submodules\". Have you tried, whether any of these trigger?\n>\n\nThe test suite passes.\n\nRalf\n"},{"id":"411216","messageId":"168be31c-f913-4bda-cf05-adcc07c51ec8@nokia.com","threadId":"54746","inReplyTo":"CAN0XMOLHM0mFvXcdiXJS_yD59rSTuyJpp9N9MLvcZ5LCC1-yZA@mail.gmail.com","subject":"Re: BUG in fetching non-checked out submodule","fromName":"Peter Kästle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-12-03T09:43:34Z","receivedAt":"2020-12-03T09:44:33Z","isPatch":false,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"Hi Ralf,\nOn 03.12.20 10:38, Ralf Thielow wrote:\n> Hi Peter,\n> \n> Am Do., 3. Dez. 2020 um 09:20 Uhr schrieb Peter Kästle\n> :\n>>> I can see the same misbehaviour when I run\n>>> $ git -c submodule.recurse=false fetch --recurse-submodules\n>>> too, so I think it doesn't have necessarily something to do with\n>>> the 'submodule.recursive' config, but with 'recurse submodules'\n>>> in general.\n>> This is interesting. There are several testcases using\n>> \"--recurse-submodules\". Have you tried, whether any of these trigger?\n>>\n> \n> The test suite passes.\n\nThis is what I experienced. Thus it is important to get your case into \nthe test suite. Could you please send us a command sequence how to \ncreate your repository structure from scratch, so that a test case can \nbe created? - Of course, if you want, you can also create a test case \non your own.\n\n-- \nkind regards\n--peter;\n"},{"id":"411218","messageId":"CAN0XMOLdz3bk+wehy-+0_XGLX6722jb71vvzNPXrryeeFuxd0w@mail.gmail.com","threadId":"54746","inReplyTo":"168be31c-f913-4bda-cf05-adcc07c51ec8@nokia.com","subject":"Re: BUG in fetching non-checked out submodule","fromName":"Ralf Thielow","fromEmail":"ralf.thielow@gmail.com","sentAt":"2020-12-03T12:30:56Z","receivedAt":"2020-12-03T12:32:14Z","isPatch":false,"sender":{"key":"ralf.thielow@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1275832?v=4"},"body":"Hi Peter,\n\nAm Do., 3. Dez. 2020 um 10:43 Uhr schrieb Peter Kästle\n:\n> >\n> > The test suite passes.\n>\n> This is what I experienced. Thus it is important to get your case into\n> the test suite. Could you please send us a command sequence how to\n> create your repository structure from scratch, so that a test case can\n> be created? - Of course, if you want, you can also create a test case\n> on your own.\n>\n\nIt can be reproduced with the following sequence of commands:\n\ngit init sub\ncd sub\ntouch file\ngit add file\ngit commit -m \"add file\"\ncd ..\ngit init main\ncd main\ngit submodule add ../sub\ngit submodule init\ngit submodule update --checkout\ngit submodule deinit -f sub/\ngit fetch --recurse-submodules\n\nRalf\n"},{"id":"411222","messageId":"5869c005-468b-8686-3022-3fa18f28f96e@nokia.com","threadId":"54746","inReplyTo":"CAN0XMOLdz3bk+wehy-+0_XGLX6722jb71vvzNPXrryeeFuxd0w@mail.gmail.com","subject":"Re: BUG in fetching non-checked out submodule","fromName":"Peter Kästle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-12-03T15:10:28Z","receivedAt":"2020-12-03T15:11:32Z","isPatch":false,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"Hi Ralf,\n\nOn 03.12.20 13:30, Ralf Thielow wrote:\n> It can be reproduced with the following sequence of commands:\n> \n> git init sub\n> cd sub\n> touch file\n> git add file\n> git commit -m \"add file\"\n> cd ..\n> git init main\n> cd main\n> git submodule add ../sub\n> git submodule init\n> git submodule update --checkout\n> git submodule deinit -f sub/\n> git fetch --recurse-submodules\n\nWith git from master state the \"git fetch --recurse-submodules\" results \nin an infinite recurse call.\n\nI translated your sequence into a bash script, which can then be easily \nconverted into a test case for git. Problematic was the infinite \nrecurse loop of the git fetch command, which I solved by grep'ing for \nthe second recursion output and abort using -m1.\nCould you please confirm, that you see \"passed\" for the good git \nversions and \"failed\" for the bad ones?\n\n\n#!/bin/bash\n\ntestcase () {\n rm -Rf main sub &&\n\n git init main &&\n git init sub &&\n\n touch sub/file &&\n git -C sub add file &&\n git -C sub commit -m \"add file\" &&\n git -C sub rev-parse HEAD >expect &&\n\n git -C main submodule add ../sub &&\n git -C main submodule init &&\n git -C main submodule update --checkout &&\n\n git -C main submodule deinit -f sub &&\n ! git -C main fetch --recurse-submodules |&\n grep -v -m1 \"Fetching submodule sub$\" &&\n git -C main submodule status |\n sed -e \"s/^-//\" -e \"s/ sub$//\" >actual &&\n cmp expect actual\n}\n\nif testcase\nthen\n echo \"passed\"\nelse\n echo \"failed\"\nfi\n\n\n-- \n--peter;\n"},{"id":"411225","messageId":"CADtb9DxTgEWfOF7jDGGt3eQSCaaqeiyJfS4V-e0SyPenE2SXWA@mail.gmail.com","threadId":"54746","inReplyTo":"04968f5c-c8bd-c57e-d646-7c9f7691e1a8@nokia.com","subject":"Re: BUG in fetching non-checked out submodule","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-12-03T15:25:40Z","receivedAt":"2020-12-03T15:26:56Z","isPatch":false,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hello Peter,\n\nLe jeu. 3 déc. 2020, à 02 h 54, Peter Kästle a écrit :\n>\n> Hi,\n>\n> On 03.12.20 00:06, Junio C Hamano wrote:\n> > Philippe Blain writes:\n> >\n> >> Thanks for bisecting it. That commit wanted to fix a different bug\n> >> related to nested submodules, and the route taken was simply\n> >> reverting an earlier commit (a62387b (submodule.c: fetch in\n> >> submodules git directory instead of in worktree, 2018-11-28).\n> >>\n> >> As you discovered, it breaks other scenarios.\n> >>\n> >>>\n> >>> $ git version\n> >>> git version 2.29.2.435.g72ffeb997e\n> >>>\n> >>> $ git config --get submodule.recurse\n> >>> true\n> >\n> > I think the current situation is probably worse.\n> >\n> > As a short-term fix, we should revert 1b7ac4e6d4 until we can come\n> > up with a real fix, probably.\n>\n> Junio: This is why I originally intended to commit the test case for the\n> testsuite separated from the revert and wanted to start a discussion\n> about the actual real fix for the issue:\n> https://public-inbox.org/git/1604413399-63090-1-git-send-email-peter.kaestle@nokia.com/\n>\n> My proposal would be to revert 1b7ac4e6d4 and isolate the test case\n> \"test_expect_success 'setup nested submodule fetch test' '\" make it\n> \"test_expect_failure\" and apply it instead, until we come up with a real\n> solution.\n\n\nI think I have the real solution. I did some debugging and I think it\nis quite easy:\nIn 'get_next_submodule', 'get_submodule_repo_for(spf->r, task->sub)'\nfails to get a repo pointer for the submodule repository, since it is\nnot initialized. That\nis normal. Then we go in the \"else\" branch, and hit this code:\n\n/*\n* An empty directory is normal,\n* the submodule is not initialized\n*/\nif (S_ISGITLINK(ce->ce_mode) &&\n!is_empty_dir(ce->name)) {\n\n'is_empty_dir' receives ce->name, but the current working directory is the\nGit directory of 'middle', so clearly is_empty_dir returns false, as\n\n/path/to/git/t/trash\ndirectory.t5526-fetch-submodules/B/.git/modules/middle/inner\n\nis a non-existent path. The path that we should send is the worktree\nof inner, ie.\nthe concatenation of spf->r->worktree and ce->name. This would give\n\n/path/to/git/t/trash directory.t5526-fetch-submodules/B/middle/inner,\n\nwhich is an empty directory since the inner submodule is not initialized,\nand we would not get the \"Could not access submodule inner\" error\nthat you wanted to solve.\n\n\nCheers,\n\nPhilippe.\n"},{"id":"411226","messageId":"0b6a34a0-428e-5fc4-307d-1217b112659c@nokia.com","threadId":"54746","inReplyTo":"CADtb9DxTgEWfOF7jDGGt3eQSCaaqeiyJfS4V-e0SyPenE2SXWA@mail.gmail.com","subject":"Re: BUG in fetching non-checked out submodule","fromName":"Peter Kästle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-12-03T15:33:37Z","receivedAt":"2020-12-03T15:34:46Z","isPatch":false,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"Hi Philippe,\n\nOn 03.12.20 16:25, Philippe Blain wrote:\n>> On 03.12.20 00:06, Junio C Hamano wrote:\n>>> Philippe Blain writes:\n>>>\n>>>> Thanks for bisecting it. That commit wanted to fix a different bug\n>>>> related to nested submodules, and the route taken was simply\n>>>> reverting an earlier commit (a62387b (submodule.c: fetch in\n>>>> submodules git directory instead of in worktree, 2018-11-28).\n>>>>\n>>>> As you discovered, it breaks other scenarios.\n>>>>\n>>>>>\n>>>>> $ git version\n>>>>> git version 2.29.2.435.g72ffeb997e\n>>>>>\n>>>>> $ git config --get submodule.recurse\n>>>>> true\n>>>\n>>> I think the current situation is probably worse.\n>>>\n>>> As a short-term fix, we should revert 1b7ac4e6d4 until we can come\n>>> up with a real fix, probably.\n>>\n>> Junio: This is why I originally intended to commit the test case for the\n>> testsuite separated from the revert and wanted to start a discussion\n>> about the actual real fix for the issue:\n>> https://public-inbox.org/git/1604413399-63090-1-git-send-email-peter.kaestle@nokia.com/\n>>\n>> My proposal would be to revert 1b7ac4e6d4 and isolate the test case\n>> \"test_expect_success 'setup nested submodule fetch test' '\" make it\n>> \"test_expect_failure\" and apply it instead, until we come up with a real\n>> solution.\n> \n> \n> I think I have the real solution. I did some debugging and I think it\n> is quite easy:\n> In 'get_next_submodule', 'get_submodule_repo_for(spf->r, task->sub)'\n> fails to get a repo pointer for the submodule repository, since it is\n> not initialized. That\n> is normal. Then we go in the \"else\" branch, and hit this code:\n> \n> /*\n> * An empty directory is normal,\n> * the submodule is not initialized\n> */\n> if (S_ISGITLINK(ce->ce_mode) &&\n> !is_empty_dir(ce->name)) {\n> \n> 'is_empty_dir' receives ce->name, but the current working directory is the\n> Git directory of 'middle', so clearly is_empty_dir returns false, as\n> \n> /path/to/git/t/trash\n> directory.t5526-fetch-submodules/B/.git/modules/middle/inner\n> \n> is a non-existent path. The path that we should send is the worktree\n> of inner, ie.\n> the concatenation of spf->r->worktree and ce->name. This would give\n> \n> /path/to/git/t/trash directory.t5526-fetch-submodules/B/middle/inner,\n> \n> which is an empty directory since the inner submodule is not initialized,\n> and we would not get the \"Could not access submodule inner\" error\n> that you wanted to solve.\n\n\nOn quick glance this sounds plausible, but to fully understand it I need \nto put some effort in reading this code again. I hope to do so \ntomorrow. We can then compile a new set of patches including this real \nfix and Ralf's and my test case.\n\nThanks for digging into it.\n\n-- \nkind regards\n--peter;\n"},{"id":"411256","messageId":"CAN0XMOKYKkKPj0rpE-b-6pvzJ7Tcr3Ycpu+ynunge8Yz4yNWcg@mail.gmail.com","threadId":"54746","inReplyTo":"5869c005-468b-8686-3022-3fa18f28f96e@nokia.com","subject":"Re: BUG in fetching non-checked out submodule","fromName":"Ralf Thielow","fromEmail":"ralf.thielow@gmail.com","sentAt":"2020-12-03T16:45:03Z","receivedAt":"2020-12-03T16:46:28Z","isPatch":false,"sender":{"key":"ralf.thielow@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1275832?v=4"},"body":"Hi Peter,\n\nAm Do., 3. Dez. 2020 um 16:10 Uhr schrieb Peter Kästle\n:\n>\n> I translated your sequence into a bash script, which can then be easily\n> converted into a test case for git. Problematic was the infinite\n> recurse loop of the git fetch command, which I solved by grep'ing for\n> the second recursion output and abort using -m1.\n> Could you please confirm, that you see \"passed\" for the good git\n> versions and \"failed\" for the bad ones?\n>\n>\n\nI can confirm that. It \"failed\" on current master version\n2.29.2.435.g72ffeb997e\nand \"passed\" on the latest tag 2.29.2.\n\nThanks,\nRalf\n"},{"id":"411262","messageId":"xmqqzh2uhi6s.fsf@gitster.c.googlers.com","threadId":"54746","inReplyTo":"0b6a34a0-428e-5fc4-307d-1217b112659c@nokia.com","subject":"Re: BUG in fetching non-checked out submodule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-03T18:12:59Z","receivedAt":"2020-12-03T18:14:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Kästle writes:\n\n> On quick glance this sounds plausible, but to fully understand it I\n> need to put some effort in reading this code again. I hope to do so \n> tomorrow. We can then compile a new set of patches including this\n> real fix and Ralf's and my test case.\n>\n> Thanks for digging into it.\n\nYeah, thanks, both for noticing problem so quickly and started\ndigging to find the real solution.\n\nBy the way, as to the \"if this were originally two patches, we could\nhave saved the test that expects failure\" you raised, I do not think\nit is a good idea to optimize for cases where changes turn out\nproblematic and have to get reverted.\n"},{"id":"411332","messageId":"1607095412-40109-1-git-send-email-peter.kaestle@nokia.com","threadId":"54746","inReplyTo":"0b6a34a0-428e-5fc4-307d-1217b112659c@nokia.com","subject":"[PATCH] submodules: fix of regression on fetching of non-init subsub-repo","fromName":"Peter Kaestle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-12-04T15:23:32Z","receivedAt":"2020-12-04T15:24:34Z","isPatch":true,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"A regression has been introduced by a62387b (submodule.c: fetch in\nsubmodules git directory instead of in worktree, 2018-11-28).\n\nThe scenario in which it triggers is when one has a remote repository\nwith a subrepository inside a subrepository like this:\nsuperproject/middle_repo/inner_repo\n\nPerson A and B have both a clone of it, while Person B is not working\nwith the inner_repo and thus does not have it initialized in his working\ncopy.\n\nNow person A introduces a change to the inner_repo and propagates it\nthrough the middle_repo and the superproject.\n\nOnce person A pushed the changes and person B wants to fetch them using\n\"git fetch\" on superproject level, B's git call will return with error\nsaying:\n\nCould not access submodule 'inner_repo'\nErrors during submodule fetch:\n middle_repo\n\nExpectation is that in this case the inner submodule will be recognized\nas uninitialized subrepository and skipped by the git fetch command.\n\nThis used to work correctly before 'a62387b (submodule.c: fetch in\nsubmodules git directory instead of in worktree, 2018-11-28)'.\n\nStarting with a62387b the code wants to evaluate \"is_empty_dir()\" inside\n.git/modules for a directory only existing in the worktree, delivering\nthen of course wrong return value.\n\nThis patch ensures is_empty_dir() is getting the correct path of the\nuninitialized submodule by concatenation of the actual worktree and the\nname of the uninitialized submodule.\n\nFurthermore a regression test case is added, which tests for recursive\nfetches on a superproject with uninitialized sub repositories. This\nissue was leading to an infinite loop when doing a revert of a62387b.\n\nSigned-off-by: Peter Kaestle \nCC: Junio C Hamano \nCC: Philippe Blain \nCC: Ralf Thielow \n---\n submodule.c | 7 ++-\n t/t5526-fetch-submodules.sh | 94 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 100 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex b3bb59f066..b561445329 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1477,6 +1477,7 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\tstrbuf_release(&submodule_prefix);\n \t\t\treturn 1;\n \t\t} else {\n+\t\t\tstruct strbuf empty_submodule_path = STRBUF_INIT;\n \n \t\t\tfetch_task_release(task);\n \t\t\tfree(task);\n@@ -1485,13 +1486,17 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\t * An empty directory is normal,\n \t\t\t * the submodule is not initialized\n \t\t\t */\n+\t\t\tstrbuf_addf(&empty_submodule_path, \"%s/%s/\",\n+\t\t\t\t\t\t\tspf->r->worktree,\n+\t\t\t\t\t\t\tce->name);\n \t\t\tif (S_ISGITLINK(ce->ce_mode) &&\n-\t\t\t !is_empty_dir(ce->name)) {\n+\t\t\t !is_empty_dir(empty_submodule_path.buf)) {\n \t\t\t\tspf->result = 1;\n \t\t\t\tstrbuf_addf(err,\n \t\t\t\t\t _(\"Could not access submodule '%s'\\n\"),\n \t\t\t\t\t ce->name);\n \t\t\t}\n+\t\t\tstrbuf_release(&empty_submodule_path);\n \t\t}\n \t}\n \ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex dd8e423d25..04eb587862 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -719,4 +719,98 @@ test_expect_success 'fetch new submodule commit intermittently referenced by sup\n \t)\n '\n \n+add_commit_push () {\n+\tdir=\"$1\"\n+\tmsg=\"$2\"\n+\tshift 2\n+\tgit -C \"$dir\" add \"$@\" &&\n+\tgit -C \"$dir\" commit -a -m \"$msg\" &&\n+\tgit -C \"$dir\" push\n+}\n+\n+compare_refs_in_dir () {\n+\tfail= &&\n+\tif test \"x$1\" = 'x!'\n+\tthen\n+\t\tfail='!' &&\n+\t\tshift\n+\tfi &&\n+\tgit -C \"$1\" rev-parse --verify \"$2\" >expect &&\n+\tgit -C \"$3\" rev-parse --verify \"$4\" >actual &&\n+\teval $fail test_cmp expect actual\n+}\n+\n+\n+test_expect_success 'setup nested submodule fetch test' '\n+\t# does not depend on any previous test setups\n+\n+\tfor repo in outer middle inner\n+\tdo\n+\t\t(\n+\t\t\tgit init --bare $repo &&\n+\t\t\tgit clone $repo ${repo}_content &&\n+\t\t\techo \"$repo\" >\"${repo}_content/file\" &&\n+\t\t\tadd_commit_push ${repo}_content \"initial\" file\n+\t\t) || return 1\n+\tdone &&\n+\n+\tgit clone outer A &&\n+\tgit -C A submodule add \"$pwd/middle\" &&\n+\tgit -C A/middle/ submodule add \"$pwd/inner\" &&\n+\tadd_commit_push A/middle/ \"adding inner sub\" .gitmodules inner &&\n+\tadd_commit_push A/ \"adding middle sub\" .gitmodules middle &&\n+\n+\tgit clone outer B &&\n+\tgit -C B/ submodule update --init middle &&\n+\n+\tcompare_refs_in_dir A HEAD B HEAD &&\n+\tcompare_refs_in_dir A/middle HEAD B/middle HEAD &&\n+\ttest -f B/file &&\n+\ttest -f B/middle/file &&\n+\t! test -f B/middle/inner/file &&\n+\n+\techo \"change on inner repo of A\" >\"A/middle/inner/file\" &&\n+\tadd_commit_push A/middle/inner \"change on inner\" file &&\n+\tadd_commit_push A/middle \"change on inner\" inner &&\n+\tadd_commit_push A \"change on inner\" middle\n+'\n+\n+test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '\n+\t# depends on previous test for setup\n+\n+\tgit -C B/ fetch &&\n+\tcompare_refs_in_dir A origin/master B origin/master\n+'\n+\n+\n+test_expect_success 'setup recursive fetch with uninit submodule' '\n+\t# does not depend on any previous test setups\n+\n+\tgit init main &&\n+\tgit init sub &&\n+\n+\ttouch sub/file &&\n+\tgit -C sub add file &&\n+\tgit -C sub commit -m \"add file\" &&\n+\tgit -C sub rev-parse HEAD >expect &&\n+\n+\tgit -C main submodule add ../sub &&\n+\tgit -C main submodule init &&\n+\tgit -C main submodule update --checkout &&\n+\tgit -C main submodule status |\n+\t\tsed -e \"s/^ //\" -e \"s/ sub .*$//\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'recursive fetch with uninit submodule' '\n+\t# depends on previous test for setup\n+\n+\tgit -C main submodule deinit -f sub &&\n+\t! git -C main fetch --recurse-submodules |&\n+\t\tgrep -v -m1 \"Fetching submodule sub$\" &&\n+\tgit -C main submodule status |\n+\t\tsed -e \"s/^-//\" -e \"s/ sub$//\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.29.2\n\n"},{"id":"411339","messageId":"CAPig+cR69HJefRMfH_5-dHOMVY-VmVgbqQuWV90ednDEjrnExw@mail.gmail.com","threadId":"54746","inReplyTo":"1607095412-40109-1-git-send-email-peter.kaestle@nokia.com","subject":"Re: [PATCH] submodules: fix of regression on fetching of non-init subsub-repo","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-04T18:06:50Z","receivedAt":"2020-12-04T18:07:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Dec 4, 2020 at 10:25 AM Peter Kaestle wrote:\n> [...]\n> Furthermore a regression test case is added, which tests for recursive\n> fetches on a superproject with uninitialized sub repositories. This\n> issue was leading to an infinite loop when doing a revert of a62387b.\n\nJust a few small comments (nothing comprehensive) from a quick scan of\nthe patch...\n\nMostly they are just minor style issues, not necessarily worth a\nre-roll, but there is one actionable item.\n\n> Signed-off-by: Peter Kaestle \n> ---\n> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n> @@ -719,4 +719,98 @@ test_expect_success 'fetch new submodule commit intermittently referenced by sup\n> +add_commit_push () {\n> + dir=\"$1\"\n> + msg=\"$2\"\n> + shift 2\n\nWe typically recommend including these assignments in the &&-chain to\nfuture-proof against someone later inserting code above them and not\nrealizing that that code is not part of the &&-chain, in which case if\nthe new code fails, the failure might go unnoticed.\n\n> + git -C \"$dir\" add \"$@\" &&\n> + git -C \"$dir\" commit -a -m \"$msg\" &&\n> + git -C \"$dir\" push\n> +}\n> +\n> +compare_refs_in_dir () {\n> + fail= &&\n> + if test \"x$1\" = 'x!'\n> + then\n> + fail='!' &&\n> + shift\n> + fi &&\n> + git -C \"$1\" rev-parse --verify \"$2\" >expect &&\n> + git -C \"$3\" rev-parse --verify \"$4\" >actual &&\n> + eval $fail test_cmp expect actual\n> +}\n\nWe have a test_cmp_rev() similar to this but it doesn't support -C as\nsome of our other test functions do. I briefly wondered if it would\nmake sense to extend it to understand -C, but even that wouldn't help\nthis case since compare_refs_in_dir() introduced here involves two\ndistinct directories. The need here is so special-purpose that it\nlikely would not make sense to upgrade test_cmp_rev() to accommodate\nit. Okay.\n\n> +test_expect_success 'setup nested submodule fetch test' '\n> + # does not depend on any previous test setups\n> +\n> + for repo in outer middle inner\n> + do\n> + (\n> + git init --bare $repo &&\n> + git clone $repo ${repo}_content &&\n> + echo \"$repo\" >\"${repo}_content/file\" &&\n> + add_commit_push ${repo}_content \"initial\" file\n> + ) || return 1\n> + done &&\n\nWhat is the purpose of the subshell here? Is it to ensure that commits\nin each repo have identical timestamps? Or is it just for making the\n&& and || expression more clear? If the latter, we normally don't\nbother with the parentheses.\n\n> + git clone outer A &&\n> + git -C A submodule add \"$pwd/middle\" &&\n> + git -C A/middle/ submodule add \"$pwd/inner\" &&\n> + add_commit_push A/middle/ \"adding inner sub\" .gitmodules inner &&\n> + add_commit_push A/ \"adding middle sub\" .gitmodules middle &&\n> +\n> + git clone outer B &&\n> + git -C B/ submodule update --init middle &&\n> +\n> + compare_refs_in_dir A HEAD B HEAD &&\n> + compare_refs_in_dir A/middle HEAD B/middle HEAD &&\n> + test -f B/file &&\n> + test -f B/middle/file &&\n> + ! test -f B/middle/inner/file &&\n\nThese days we typically use test_path_exists() (or\ntest_path_is_file()) and test_path_is_missing() rather than bare\n`test`.\n\n> +test_expect_success 'setup recursive fetch with uninit submodule' '\n> + # does not depend on any previous test setups\n> +\n> + git init main &&\n> + git init sub &&\n> +\n> + touch sub/file &&\n\nUnless the timestamp of the file is significant to the test, in which\ncase `touch` is used, we normally create empty files like this:\n\n >sub/file &&\n\n> +test_expect_success 'recursive fetch with uninit submodule' '\n> + git -C main submodule deinit -f sub &&\n> + ! git -C main fetch --recurse-submodules |&\n> + grep -v -m1 \"Fetching submodule sub$\" &&\n\nWe want the test scripts to be portable, thus avoid Bashisms such as `|&`.\n\nWe also avoid placing a Git command upstream in a pipe since doing so\ncauses the exit code of the Git command to be lost. Instead, we would\nnormally send the Git output to a file and then send that file to\nwhatever would be downstream of the Git command in the pipe. So, a\nmechanical rewrite of the above (without thinking too hard about it)\nmight be:\n\n git -C main fetch --recurse-submodules >out 2>&1 &&\n ! grep -v -m1 \"Fetching submodule sub$\" &&\n\n> + git -C main submodule status |\n> + sed -e \"s/^-//\" -e \"s/ sub$//\" >actual &&\n\nSame comment about avoiding Git upstream in a pipe, so perhaps:\n\n git -C main submodule status >out &&\n sed -e \"s/^-//\" -e \"s/ sub$//\" out >actual &&\n\n> + test_cmp expect actual\n> +'\n"},{"id":"411591","messageId":"0deeeabe-9590-db7a-4a07-447d43df7a24@nokia.com","threadId":"54746","inReplyTo":"CAPig+cR69HJefRMfH_5-dHOMVY-VmVgbqQuWV90ednDEjrnExw@mail.gmail.com","subject":"Re: [PATCH] submodules: fix of regression on fetching of non-init subsub-repo","fromName":"Peter Kästle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-12-07T08:28:39Z","receivedAt":"2020-12-07T08:29:56Z","isPatch":true,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"Hi Eric,\n\nOn 04.12.20 19:06, Eric Sunshine wrote:\n> On Fri, Dec 4, 2020 at 10:25 AM Peter Kaestle wrote:\n>> [...]\n>> Furthermore a regression test case is added, which tests for recursive\n>> fetches on a superproject with uninitialized sub repositories. This\n>> issue was leading to an infinite loop when doing a revert of a62387b.\n> \n> Just a few small comments (nothing comprehensive) from a quick scan of\n> the patch...\n> \n> Mostly they are just minor style issues, not necessarily worth a\n> re-roll, but there is one actionable item.\n\nthanks for your comments. A new patch will follow soon.\n\n\n>> Signed-off-by: Peter Kaestle \n>> ---\n>> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n>> @@ -719,4 +719,98 @@ test_expect_success 'fetch new submodule commit intermittently referenced by sup\n>> +add_commit_push () {\n>> + dir=\"$1\"\n>> + msg=\"$2\"\n>> + shift 2\n> \n> We typically recommend including these assignments in the &&-chain to\n> future-proof against someone later inserting code above them and not\n> realizing that that code is not part of the &&-chain, in which case if\n> the new code fails, the failure might go unnoticed.\n\nok\n\n> \n>> + git -C \"$dir\" add \"$@\" &&\n>> + git -C \"$dir\" commit -a -m \"$msg\" &&\n>> + git -C \"$dir\" push\n>> +}\n>> +\n>> +compare_refs_in_dir () {\n>> + fail= &&\n>> + if test \"x$1\" = 'x!'\n>> + then\n>> + fail='!' &&\n>> + shift\n>> + fi &&\n>> + git -C \"$1\" rev-parse --verify \"$2\" >expect &&\n>> + git -C \"$3\" rev-parse --verify \"$4\" >actual &&\n>> + eval $fail test_cmp expect actual\n>> +}\n> \n> We have a test_cmp_rev() similar to this but it doesn't support -C as\n> some of our other test functions do. I briefly wondered if it would\n> make sense to extend it to understand -C, but even that wouldn't help\n> this case since compare_refs_in_dir() introduced here involves two\n> distinct directories. The need here is so special-purpose that it\n> likely would not make sense to upgrade test_cmp_rev() to accommodate\n> it. Okay.\n\nYes, I saw that there's a similar function and I tried to modify this \none first. Unfortunately this didn't work without touching much \nunaffected test code. So I propose to continue with this additional \nfunction.\n\n\n>> +test_expect_success 'setup nested submodule fetch test' '\n>> + # does not depend on any previous test setups\n>> +\n>> + for repo in outer middle inner\n>> + do\n>> + (\n>> + git init --bare $repo &&\n>> + git clone $repo ${repo}_content &&\n>> + echo \"$repo\" >\"${repo}_content/file\" &&\n>> + add_commit_push ${repo}_content \"initial\" file\n>> + ) || return 1\n>> + done &&\n> \n> What is the purpose of the subshell here? Is it to ensure that commits\n> in each repo have identical timestamps? Or is it just for making the\n> && and || expression more clear? If the latter, we normally don't\n> bother with the parentheses.\n\nIt was intended to make the correlation of && and || clear. I have \nexperienced many cases in the past where things were screwed because it \nwas not clearly understood by everybody. I'll propose next patch \nwithout this subshell.\n\n>> + git clone outer A &&\n>> + git -C A submodule add \"$pwd/middle\" &&\n>> + git -C A/middle/ submodule add \"$pwd/inner\" &&\n>> + add_commit_push A/middle/ \"adding inner sub\" .gitmodules inner &&\n>> + add_commit_push A/ \"adding middle sub\" .gitmodules middle &&\n>> +\n>> + git clone outer B &&\n>> + git -C B/ submodule update --init middle &&\n>> +\n>> + compare_refs_in_dir A HEAD B HEAD &&\n>> + compare_refs_in_dir A/middle HEAD B/middle HEAD &&\n>> + test -f B/file &&\n>> + test -f B/middle/file &&\n>> + ! test -f B/middle/inner/file &&\n> \n> These days we typically use test_path_exists() (or\n> test_path_is_file()) and test_path_is_missing() rather than bare\n> `test`.\n\nok.\n\n\n>> +test_expect_success 'setup recursive fetch with uninit submodule' '\n>> + # does not depend on any previous test setups\n>> +\n>> + git init main &&\n>> + git init sub &&\n>> +\n>> + touch sub/file &&\n> \n> Unless the timestamp of the file is significant to the test, in which\n> case `touch` is used, we normally create empty files like this:\n> \n> >sub/file &&\n\nok.\n\n\n> \n>> +test_expect_success 'recursive fetch with uninit submodule' '\n>> + git -C main submodule deinit -f sub &&\n>> + ! git -C main fetch --recurse-submodules |&\n>> + grep -v -m1 \"Fetching submodule sub$\" &&\n> \n> We want the test scripts to be portable, thus avoid Bashisms such as `|&`. > We also avoid placing a Git command upstream in a pipe since doing so\n> causes the exit code of the Git command to be lost. Instead, we would\n> normally send the Git output to a file and then send that file to\n> whatever would be downstream of the Git command in the pipe. So, a\n> mechanical rewrite of the above (without thinking too hard about it)\n> might be:\n> \n> git -C main fetch --recurse-submodules >out 2>&1 &&\n> ! grep -v -m1 \"Fetching submodule sub$\" &&\n\nIn general I agree, but for this special test case, it's required to \nhave the two commands connected by a pipe, as the grep needs to kill the \ngit call in error case. Otherwise for this regression git would go for \nan infinite recursion loop.\n\nOf course, we can go for a \"git 2>&1 | grep\" solution.\n\n\n>> + git -C main submodule status |\n>> + sed -e \"s/^-//\" -e \"s/ sub$//\" >actual &&\n> \n> Same comment about avoiding Git upstream in a pipe, so perhaps:\n> \n> git -C main submodule status >out &&\n> sed -e \"s/^-//\" -e \"s/ sub$//\" out >actual &&\n> \n>> + test_cmp expect actual\n>> +'\n\nok.\n\n-- \nkind regards\n--peter;\n"},{"id":"411592","messageId":"CAPig+cQ8VC2q4nuzgM9QxmddH4cMezbZdRZDxX1PqfW6XKcC_A@mail.gmail.com","threadId":"54746","inReplyTo":"0deeeabe-9590-db7a-4a07-447d43df7a24@nokia.com","subject":"Re: [PATCH] submodules: fix of regression on fetching of non-init subsub-repo","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-07T08:40:34Z","receivedAt":"2020-12-07T08:41:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 7, 2020 at 3:29 AM Peter Kästle wrote:\n> On 04.12.20 19:06, Eric Sunshine wrote:\n> > On Fri, Dec 4, 2020 at 10:25 AM Peter Kaestle wrote:\n> >> + ! git -C main fetch --recurse-submodules |&\n> >> + grep -v -m1 \"Fetching submodule sub$\" &&\n> >\n> > We want the test scripts to be portable, thus avoid Bashisms such as `|&`.\n> > git -C main fetch --recurse-submodules >out 2>&1 &&\n> > ! grep -v -m1 \"Fetching submodule sub$\" &&\n>\n> In general I agree, but for this special test case, it's required to\n> have the two commands connected by a pipe, as the grep needs to kill the\n> git call in error case. Otherwise for this regression git would go for\n> an infinite recursion loop.\n>\n> Of course, we can go for a \"git 2>&1 | grep\" solution.\n\nIn that case, an in-code comment explaining why the output of `git`\nmust be piped to `grep` would be helpful since it is not uncommon for\npeople to \"modernize\" test code as they are working in the vicinity,\nand removing such pipes is one of the modernizations often made. A\ncomment would help avoid such a change.\n"},{"id":"411601","messageId":"1607348819-61355-1-git-send-email-peter.kaestle@nokia.com","threadId":"54746","inReplyTo":"CAPig+cQ8VC2q4nuzgM9QxmddH4cMezbZdRZDxX1PqfW6XKcC_A@mail.gmail.com","subject":"[PATCH v2] submodules: fix of regression on fetching of non-init subsub-repo","fromName":"Peter Kaestle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-12-07T13:46:59Z","receivedAt":"2020-12-07T13:48:23Z","isPatch":true,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"A regression has been introduced by a62387b (submodule.c: fetch in\nsubmodules git directory instead of in worktree, 2018-11-28).\n\nThe scenario in which it triggers is when one has a remote repository\nwith a subrepository inside a subrepository like this:\nsuperproject/middle_repo/inner_repo\n\nPerson A and B have both a clone of it, while Person B is not working\nwith the inner_repo and thus does not have it initialized in his working\ncopy.\n\nNow person A introduces a change to the inner_repo and propagates it\nthrough the middle_repo and the superproject.\n\nOnce person A pushed the changes and person B wants to fetch them using\n\"git fetch\" on superproject level, B's git call will return with error\nsaying:\n\nCould not access submodule 'inner_repo'\nErrors during submodule fetch:\n middle_repo\n\nExpectation is that in this case the inner submodule will be recognized\nas uninitialized subrepository and skipped by the git fetch command.\n\nThis used to work correctly before 'a62387b (submodule.c: fetch in\nsubmodules git directory instead of in worktree, 2018-11-28)'.\n\nStarting with a62387b the code wants to evaluate \"is_empty_dir()\" inside\n.git/modules for a directory only existing in the worktree, delivering\nthen of course wrong return value.\n\nThis patch ensures is_empty_dir() is getting the correct path of the\nuninitialized submodule by concatenation of the actual worktree and the\nname of the uninitialized submodule.\n\nFurthermore a regression test case is added, which tests for recursive\nfetches on a superproject with uninitialized sub repositories. This\nissue was leading to an infinite loop when doing a revert of a62387b.\n\nSigned-off-by: Peter Kaestle \nCC: Junio C Hamano \nCC: Philippe Blain \nCC: Ralf Thielow \nCC: Eric Sunshine \n---\n submodule.c | 7 ++-\n t/t5526-fetch-submodules.sh | 104 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 110 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex b3bb59f066..b561445329 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1477,6 +1477,7 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\tstrbuf_release(&submodule_prefix);\n \t\t\treturn 1;\n \t\t} else {\n+\t\t\tstruct strbuf empty_submodule_path = STRBUF_INIT;\n \n \t\t\tfetch_task_release(task);\n \t\t\tfree(task);\n@@ -1485,13 +1486,17 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\t * An empty directory is normal,\n \t\t\t * the submodule is not initialized\n \t\t\t */\n+\t\t\tstrbuf_addf(&empty_submodule_path, \"%s/%s/\",\n+\t\t\t\t\t\t\tspf->r->worktree,\n+\t\t\t\t\t\t\tce->name);\n \t\t\tif (S_ISGITLINK(ce->ce_mode) &&\n-\t\t\t !is_empty_dir(ce->name)) {\n+\t\t\t !is_empty_dir(empty_submodule_path.buf)) {\n \t\t\t\tspf->result = 1;\n \t\t\t\tstrbuf_addf(err,\n \t\t\t\t\t _(\"Could not access submodule '%s'\\n\"),\n \t\t\t\t\t ce->name);\n \t\t\t}\n+\t\t\tstrbuf_release(&empty_submodule_path);\n \t\t}\n \t}\n \ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex dd8e423d25..666dd1e2b7 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -719,4 +719,108 @@ test_expect_success 'fetch new submodule commit intermittently referenced by sup\n \t)\n '\n \n+add_commit_push () {\n+\tdir=\"$1\" &&\n+\tmsg=\"$2\" &&\n+\tshift 2 &&\n+\tgit -C \"$dir\" add \"$@\" &&\n+\tgit -C \"$dir\" commit -a -m \"$msg\" &&\n+\tgit -C \"$dir\" push\n+}\n+\n+compare_refs_in_dir () {\n+\tfail= &&\n+\tif test \"x$1\" = 'x!'\n+\tthen\n+\t\tfail='!' &&\n+\t\tshift\n+\tfi &&\n+\tgit -C \"$1\" rev-parse --verify \"$2\" >expect &&\n+\tgit -C \"$3\" rev-parse --verify \"$4\" >actual &&\n+\teval $fail test_cmp expect actual\n+}\n+\n+\n+test_expect_success 'setup nested submodule fetch test' '\n+\t# does not depend on any previous test setups\n+\n+\tfor repo in outer middle inner\n+\tdo\n+\t\tgit init --bare $repo &&\n+\t\tgit clone $repo ${repo}_content &&\n+\t\techo \"$repo\" >\"${repo}_content/file\" &&\n+\t\tadd_commit_push ${repo}_content \"initial\" file ||\n+\t\treturn 1\n+\tdone &&\n+\n+\tgit clone outer A &&\n+\tgit -C A submodule add \"$pwd/middle\" &&\n+\tgit -C A/middle/ submodule add \"$pwd/inner\" &&\n+\tadd_commit_push A/middle/ \"adding inner sub\" .gitmodules inner &&\n+\tadd_commit_push A/ \"adding middle sub\" .gitmodules middle &&\n+\n+\tgit clone outer B &&\n+\tgit -C B/ submodule update --init middle &&\n+\n+\tcompare_refs_in_dir A HEAD B HEAD &&\n+\tcompare_refs_in_dir A/middle HEAD B/middle HEAD &&\n+\ttest_path_is_file B/file &&\n+\ttest_path_is_file B/middle/file &&\n+\ttest_path_is_missing B/middle/inner/file &&\n+\n+\techo \"change on inner repo of A\" >\"A/middle/inner/file\" &&\n+\tadd_commit_push A/middle/inner \"change on inner\" file &&\n+\tadd_commit_push A/middle \"change on inner\" inner &&\n+\tadd_commit_push A \"change on inner\" middle\n+'\n+\n+test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '\n+\t# depends on previous test for setup\n+\n+\tgit -C B/ fetch &&\n+\tcompare_refs_in_dir A origin/master B origin/master\n+'\n+\n+\n+test_expect_success 'setup recursive fetch with uninit submodule' '\n+\t# does not depend on any previous test setups\n+\n+\tgit init main &&\n+\tgit init sub &&\n+\n+\t>sub/file &&\n+\tgit -C sub add file &&\n+\tgit -C sub commit -m \"add file\" &&\n+\tgit -C sub rev-parse HEAD >expect &&\n+\n+\tgit -C main submodule add ../sub &&\n+\tgit -C main submodule init &&\n+\tgit -C main submodule update --checkout &&\n+\tgit -C main submodule status >out &&\n+\tsed -e \"s/^ //\" -e \"s/ sub .*$//\" out >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'recursive fetch with uninit submodule' '\n+\t# depends on previous test for setup\n+\n+\tgit -C main submodule deinit -f sub &&\n+\n+\t# In a regression the following git call will run into infinite recursion.\n+\t# To handle that, we connect the grep command to the git call by a pipe\n+\t# so that grep can kill the infinite recusion when detected.\n+\t# The recursion creates git output like:\n+\t# Fetching submodule sub\n+\t# Fetching submodule sub/sub <-- [1]\n+\t# Fetching submodule sub/sub/sub\n+\t# ...\n+\t# [1] grep will trigger here and kill git by exiting and closing its stdin\n+\n+\t! git -C main fetch --recurse-submodules 2>&1 |\n+\t\tgrep -v -m1 \"Fetching submodule sub$\" &&\n+\tgit -C main submodule status >out &&\n+\tsed -e \"s/^-//\" -e \"s/ sub$//\" out >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.29.2\n\n"},{"id":"411615","messageId":"613FAD04-0D5A-4DE0-8FE8-0C5C5619B7BC@gmail.com","threadId":"54746","inReplyTo":"1607348819-61355-1-git-send-email-peter.kaestle@nokia.com","subject":"Re: [PATCH v2] submodules: fix of regression on fetching of non-init subsub-repo","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-12-07T18:42:08Z","receivedAt":"2020-12-07T18:42:54Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Peter,\n\n> Le 7 déc. 2020 à 08:46, Peter Kaestle a écrit :\n> \n> A regression has been introduced by a62387b (submodule.c: fetch in\n> submodules git directory instead of in worktree, 2018-11-28).\n> \n> The scenario in which it triggers is when one has a remote repository\n> with a subrepository inside a subrepository like this:\n> superproject/middle_repo/inner_repo\n\nThe correct terminology is \"submodule\", not \"subrepository\". \n\nAlso, (minor point) I would just write \"when one has a repository\", \nas its simpler (the repository by itself is not \"remote\", it is only \"remote\" \nin relation the repositories that are cloned from it).\n\n> Person A and B have both a clone of it, while Person B is not working\n> with the inner_repo and thus does not have it initialized in his working\n> copy.\n> \n> Now person A introduces a change to the inner_repo and propagates it\n> through the middle_repo and the superproject.\n> \n> Once person A pushed the changes and person B wants to fetch them using\n> \"git fetch\" on superproject level,\n\ns/on/at the/\n\n> B's git call will return with error\n> saying:\n> \n> Could not access submodule 'inner_repo'\n> Errors during submodule fetch:\n> middle_repo\n> \n> Expectation is that in this case the inner submodule will be recognized\n> as uninitialized subrepository and skipped by the git fetch command.\n\nhere again, terminology: \"as an uninitialized submodule\" \n\n> This used to work correctly before 'a62387b (submodule.c: fetch in\n> submodules git directory instead of in worktree, 2018-11-28)'.\n> \n> Starting with a62387b the code wants to evaluate \"is_empty_dir()\" inside\n> .git/modules for a directory only existing in the worktree, delivering\n> then of course wrong return value.\n> \n> This patch ensures is_empty_dir() is getting the correct path of the\n> uninitialized submodule by concatenation of the actual worktree and the\n> name of the uninitialized submodule.\n> \n> Furthermore a regression test case is added, which tests for recursive\n> fetches on a superproject with uninitialized sub repositories.\n> This\n> issue was leading to an infinite loop when doing a revert of a62387b.\n\nI would maybe add more details here, something like the following \n(we can cite your previous attempt, because it was merged to 'master'):\n\nThe first attempt to fix this regression, in 1b7ac4e6d4 (submodules: \nfix of regression on fetching of non-init subsub-repo, 2020-11-12), by simply\nreverting a62387b, resulted in\nan infinite loop of submodule fetches in the simpler case of a recursive fetch of a superproject with\nuninitialized submodules, and so this commit was reverted in 7091499bc0 (Revert \n\"submodules: fix of regression on fetching of non-init subsub-repo\", 2020-12-02).\nTo prevent future breakages, also add a regression test for this scenario.\n\n> \n> Signed-off-by: Peter Kaestle \n> CC: Junio C Hamano \n> CC: Philippe Blain \n> CC: Ralf Thielow \n> CC: Eric Sunshine \n> ---\n> submodule.c | 7 ++-\n> t/t5526-fetch-submodules.sh | 104 ++++++++++++++++++++++++++++++++++++\n> 2 files changed, 110 insertions(+), 1 deletion(-)\n> \n> diff --git a/submodule.c b/submodule.c\n> index b3bb59f066..b561445329 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1477,6 +1477,7 @@ static int get_next_submodule(struct child_process *cp,\n> \t\t\tstrbuf_release(&submodule_prefix);\n> \t\t\treturn 1;\n> \t\t} else {\n> +\t\t\tstruct strbuf empty_submodule_path = STRBUF_INIT;\n> \n> \t\t\tfetch_task_release(task);\n> \t\t\tfree(task);\n> @@ -1485,13 +1486,17 @@ static int get_next_submodule(struct child_process *cp,\n> \t\t\t * An empty directory is normal,\n> \t\t\t * the submodule is not initialized\n> \t\t\t */\n> +\t\t\tstrbuf_addf(&empty_submodule_path, \"%s/%s/\",\n> +\t\t\t\t\t\t\tspf->r->worktree,\n> +\t\t\t\t\t\t\tce->name);\n> \t\t\tif (S_ISGITLINK(ce->ce_mode) &&\n> -\t\t\t !is_empty_dir(ce->name)) {\n> +\t\t\t !is_empty_dir(empty_submodule_path.buf)) {\n> \t\t\t\tspf->result = 1;\n> \t\t\t\tstrbuf_addf(err,\n> \t\t\t\t\t _(\"Could not access submodule '%s'\\n\"),\n> \t\t\t\t\t ce->name);\n> \t\t\t}\n> +\t\t\tstrbuf_release(&empty_submodule_path);\n> \t\t}\n> \t}\n\n\nMaybe a personal preference, but I would have gone for something a little simpler, like the following:\n\n\ndiff --git a/submodule.c b/submodule.c\nindex b3bb59f066..4200865174 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1486,7 +1486,7 @@ static int get_next_submodule(struct child_process *cp,\n * the submodule is not initialized\n */\n if (S_ISGITLINK(ce->ce_mode) &&\n- !is_empty_dir(ce->name)) {\n+ !is_empty_dir(repo_worktree_path(spf->r, \"%s\", ce->name))) {\n spf->result = 1;\n strbuf_addf(err,\n _(\"Could not access submodule '%s'\\n\"),\n\n> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n> index dd8e423d25..666dd1e2b7 100755\n> --- a/t/t5526-fetch-submodules.sh\n> +++ b/t/t5526-fetch-submodules.sh\n> @@ -719,4 +719,108 @@ test_expect_success 'fetch new submodule commit intermittently referenced by sup\n> \t)\n> '\n> \n> +add_commit_push () {\n> +\tdir=\"$1\" &&\n> +\tmsg=\"$2\" &&\n> +\tshift 2 &&\n> +\tgit -C \"$dir\" add \"$@\" &&\n> +\tgit -C \"$dir\" commit -a -m \"$msg\" &&\n> +\tgit -C \"$dir\" push\n> +}\n> +\n> +compare_refs_in_dir () {\n> +\tfail= &&\n> +\tif test \"x$1\" = 'x!'\n> +\tthen\n> +\t\tfail='!' &&\n> +\t\tshift\n> +\tfi &&\n> +\tgit -C \"$1\" rev-parse --verify \"$2\" >expect &&\n> +\tgit -C \"$3\" rev-parse --verify \"$4\" >actual &&\n> +\teval $fail test_cmp expect actual\n> +}\n> +\n> +\n> +test_expect_success 'setup nested submodule fetch test' '\n> +\t# does not depend on any previous test setups\n> +\n> +\tfor repo in outer middle inner\n> +\tdo\n> +\t\tgit init --bare $repo &&\n> +\t\tgit clone $repo ${repo}_content &&\n> +\t\techo \"$repo\" >\"${repo}_content/file\" &&\n> +\t\tadd_commit_push ${repo}_content \"initial\" file ||\n> +\t\treturn 1\n> +\tdone &&\n> +\n> +\tgit clone outer A &&\n> +\tgit -C A submodule add \"$pwd/middle\" &&\n> +\tgit -C A/middle/ submodule add \"$pwd/inner\" &&\n> +\tadd_commit_push A/middle/ \"adding inner sub\" .gitmodules inner &&\n> +\tadd_commit_push A/ \"adding middle sub\" .gitmodules middle &&\n> +\n> +\tgit clone outer B &&\n> +\tgit -C B/ submodule update --init middle &&\n> +\n> +\tcompare_refs_in_dir A HEAD B HEAD &&\n> +\tcompare_refs_in_dir A/middle HEAD B/middle HEAD &&\n> +\ttest_path_is_file B/file &&\n> +\ttest_path_is_file B/middle/file &&\n> +\ttest_path_is_missing B/middle/inner/file &&\n> +\n> +\techo \"change on inner repo of A\" >\"A/middle/inner/file\" &&\n> +\tadd_commit_push A/middle/inner \"change on inner\" file &&\n> +\tadd_commit_push A/middle \"change on inner\" inner &&\n> +\tadd_commit_push A \"change on inner\" middle\n> +'\n> +\n> +test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '\n> +\t# depends on previous test for setup\n> +\n> +\tgit -C B/ fetch &&\n> +\tcompare_refs_in_dir A origin/master B origin/master\n> +'\n> +\n> +\n> +test_expect_success 'setup recursive fetch with uninit submodule' '\n> +\t# does not depend on any previous test setups\n> +\n> +\tgit init main &&\n> +\tgit init sub &&\n> +\n> +\t>sub/file &&\n> +\tgit -C sub add file &&\n> +\tgit -C sub commit -m \"add file\" &&\n> +\tgit -C sub rev-parse HEAD >expect &&\n> +\n> +\tgit -C main submodule add ../sub &&\n> +\tgit -C main submodule init &&\n> +\tgit -C main submodule update --checkout &&\n\nThese two steps are unnecessary as they are implicitly done by 'git submodule add'.\nI think we could reflect real life a little bit more by cloning the superproject, and running\nthe 'recursive fetch with uninit submodule' test below in the clone.\n\n> +\tgit -C main submodule status >out &&\n> +\tsed -e \"s/^ //\" -e \"s/ sub .*$//\" out >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'recursive fetch with uninit submodule' '\n> +\t# depends on previous test for setup\n> +\n> +\tgit -C main submodule deinit -f sub &&\n\nHere you are deiniting the submodule, such that \nthe Git directory will stay in .git/modules/sub. This is not the same thing\nas a submodule that was never initialized (\"uninitialized\"), for which .git/modules/sub\nwill not yet exist. So maybe we could harden the tests by also testing\nfor that scenario ? I don't know... maybe the infinite loop only happens\nif .git/modules/sub actually already exists. If so, the test name should be\n\"recursive fetch with deinitialized submodule\", I think.\n\n> +\n> +\t# In a regression the following git call will run into infinite recursion.\n> +\t# To handle that, we connect the grep command to the git call by a pipe\n> +\t# so that grep can kill the infinite recusion when detected.\n> +\t# The recursion creates git output like:\n> +\t# Fetching submodule sub\n> +\t# Fetching submodule sub/sub <-- [1]\n> +\t# Fetching submodule sub/sub/sub\n> +\t# ...\n> +\t# [1] grep will trigger here and kill git by exiting and closing its stdin\n> +\n> +\t! git -C main fetch --recurse-submodules 2>&1 |\n> +\t\tgrep -v -m1 \"Fetching submodule sub$\" &&\n> +\tgit -C main submodule status >out &&\n> +\tsed -e \"s/^-//\" -e \"s/ sub$//\" out >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n\n\nThanks for working on that, and sorry for not having the time to comment before\nyou sent v2.\n\nCheers,\n\nPhilippe."},{"id":"411636","messageId":"xmqq360hbev1.fsf@gitster.c.googlers.com","threadId":"54746","inReplyTo":"1607348819-61355-1-git-send-email-peter.kaestle@nokia.com","subject":"Re: [PATCH v2] submodules: fix of regression on fetching of non-init subsub-repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-07T19:22:42Z","receivedAt":"2020-12-07T19:23:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Kaestle writes:\n\n> +add_commit_push () {\n> +\tdir=\"$1\" &&\n> +\tmsg=\"$2\" &&\n> +\tshift 2 &&\n> +\tgit -C \"$dir\" add \"$@\" &&\n> +\tgit -C \"$dir\" commit -a -m \"$msg\" &&\n> +\tgit -C \"$dir\" push\n> +}\n> +\n> +compare_refs_in_dir () {\n> +\tfail= &&\n> +\tif test \"x$1\" = 'x!'\n> +\tthen\n> +\t\tfail='!' &&\n> +\t\tshift\n> +\tfi &&\n> +\tgit -C \"$1\" rev-parse --verify \"$2\" >expect &&\n> +\tgit -C \"$3\" rev-parse --verify \"$4\" >actual &&\n> +\teval $fail test_cmp expect actual\n> +}\n\n\n\n> +test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '\n> +\t# depends on previous test for setup\n> +\n> +\tgit -C B/ fetch &&\n> +\tcompare_refs_in_dir A origin/master B origin/master\n\nCan we do this without relying on the name of the default branch?\nPerhaps when outer, middle and inner are prepared, they can be\nforced to be on the 'sample' (not 'master' nor 'main') branch, or\nsomething like that?\n\n> +test_expect_success 'setup recursive fetch with uninit submodule' '\n> +\t# does not depend on any previous test setups\n> +\n> +\tgit init main &&\n> +\tgit init sub &&\n\n\"super vs sub\" would give us a better contrast than \"main vs sub\",\nand it would help reduce mistakes in the mechanical conversion of\n\"master\" to \"main\" happening in another topic.\n\n> +\t# In a regression the following git call will run into infinite recursion.\n> +\t# To handle that, we connect the grep command to the git call by a pipe\n> +\t# so that grep can kill the infinite recusion when detected.\n> +\t# The recursion creates git output like:\n> +\t# Fetching submodule sub\n> +\t# Fetching submodule sub/sub <-- [1]\n> +\t# Fetching submodule sub/sub/sub\n> +\t# ...\n> +\t# [1] grep will trigger here and kill git by exiting and closing its stdin\n\n\"trigger here and kill...\" -> \"stop reading and cause git to\neventually stop and die\"\n\nBut we probably cannot use 'grep -m1' so it is a moot point.\n\n> +\n> +\t! git -C main fetch --recurse-submodules 2>&1 |\n> +\t\tgrep -v -m1 \"Fetching submodule sub$\" &&\n\nUnfortunately, \"grep -m\" is not even in POSIX, I would think.\n\nWhat do we expect to happen in the correct case?\n\n - A line \"Fetching submodule sub\" and nothing else is given? That\n feels a bit brittle (how are we making sure, in the presence of\n \"2>&1\", that we will not get any other output, like progress?)\n\n - \"sub\" is the only thing that appears on lines that begin with\n \"Fetching submodule\" (i.e. \"Fetching submodule $something\" where\n $something is not 'sub' is an error), and we allow other garbage\n in the output? That would be a bit more robust than the above.\n\nAs you seem to be comfortable using \"sed\" below, perhaps use it to\nextract the first few lines that say \"^Fetching submodule \" from the\noutput and stop, and check that the output has only one such line\nabout 'sub' and nothing else?\n\n> +\tgit -C main submodule status >out &&\n> +\tsed -e \"s/^-//\" -e \"s/ sub$//\" out >actual &&\n> +\ttest_cmp expect actual\n"},{"id":"411640","messageId":"xmqqpn3l9zce.fsf@gitster.c.googlers.com","threadId":"54746","inReplyTo":"613FAD04-0D5A-4DE0-8FE8-0C5C5619B7BC@gmail.com","subject":"Re: [PATCH v2] submodules: fix of regression on fetching of non-init subsub-repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-07T19:43:13Z","receivedAt":"2020-12-07T19:44:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philippe Blain writes:\n\n> Maybe a personal preference, but I would have gone for something a\n> little simpler, like the following:\n>\n> diff --git a/submodule.c b/submodule.c\n> index b3bb59f066..4200865174 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1486,7 +1486,7 @@ static int get_next_submodule(struct child_process *cp,\n> * the submodule is not initialized\n> */\n> if (S_ISGITLINK(ce->ce_mode) &&\n> - !is_empty_dir(ce->name)) {\n> + !is_empty_dir(repo_worktree_path(spf->r, \"%s\", ce->name))) {\n\nBut then you leak the return value from repo_worktree_path(), no?\n\n>> +test_expect_success 'recursive fetch with uninit submodule' '\n>> +\t# depends on previous test for setup\n>> +\n>> +\tgit -C main submodule deinit -f sub &&\n>\n> Here you are deiniting the submodule, such that \n> the Git directory will stay in .git/modules/sub. This is not the same thing\n> as a submodule that was never initialized (\"uninitialized\"), for which .git/modules/sub\n> will not yet exist. So maybe we could harden the tests by also testing\n> for that scenario ? I don't know... maybe the infinite loop only happens\n> if .git/modules/sub actually already exists. If so, the test name should be\n> \"recursive fetch with deinitialized submodule\", I think.\n\nEven if the original breakage happens only for deinitialized case,\nit would be sensible to test uninitialized one as well, I would\nthink.\n\n> Thanks for working on that, and sorry for not having the time to comment before\n> you sent v2.\n\nThanks, all. \n\nIt's not like we corral everybody to a single place on a single day\nto work on a single thing. Reviews ov v2 by reviewers who have not\nseen v1 is totally expected and very much appreciated.\n"},{"id":"411642","messageId":"xmqqh7ox9ypl.fsf@gitster.c.googlers.com","threadId":"54746","inReplyTo":"613FAD04-0D5A-4DE0-8FE8-0C5C5619B7BC@gmail.com","subject":"Re: [PATCH v2] submodules: fix of regression on fetching of non-init subsub-repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-07T19:56:54Z","receivedAt":"2020-12-07T19:57:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philippe Blain writes:\n\n> I would maybe add more details here, something like the following \n> (we can cite your previous attempt, because it was merged to 'master'):\n>\n> The first attempt to fix this regression, in 1b7ac4e6d4\n> (submodules: fix of regression on fetching of non-init\n> subsub-repo, 2020-11-12), by simply reverting a62387b, resulted in\n> an infinite loop of submodule fetches in the simpler case of a\n> recursive fetch of a superproject with uninitialized submodules,\n> and so this commit was reverted in 7091499bc0 (Revert \"submodules:\n> fix of regression on fetching of non-init subsub-repo\",\n> 2020-12-02). To prevent future breakages, also add a regression\n> test for this scenario.\n\nForgot to mention in my other response, but I do find this a very\nsensible addition.\n\nThanks.\n"},{"id":"411643","messageId":"BF6B37E9-5AF9-4A81-90D8-0270D1269332@gmail.com","threadId":"54746","inReplyTo":"xmqq360hbev1.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] submodules: fix of regression on fetching of non-init subsub-repo","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-12-07T20:44:06Z","receivedAt":"2020-12-07T20:45:05Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Junio,\n\n> Le 7 déc. 2020 à 14:22, Junio C Hamano a écrit :\n> \n> Peter Kaestle writes:\n> \n>> +add_commit_push () {\n>> +\tdir=\"$1\" &&\n>> +\tmsg=\"$2\" &&\n>> +\tshift 2 &&\n>> +\tgit -C \"$dir\" add \"$@\" &&\n>> +\tgit -C \"$dir\" commit -a -m \"$msg\" &&\n>> +\tgit -C \"$dir\" push\n>> +}\n>> +\n>> +compare_refs_in_dir () {\n>> +\tfail= &&\n>> +\tif test \"x$1\" = 'x!'\n>> +\tthen\n>> +\t\tfail='!' &&\n>> +\t\tshift\n>> +\tfi &&\n>> +\tgit -C \"$1\" rev-parse --verify \"$2\" >expect &&\n>> +\tgit -C \"$3\" rev-parse --verify \"$4\" >actual &&\n>> +\teval $fail test_cmp expect actual\n>> +}\n> \n> \n> \n>> +test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '\n>> +\t# depends on previous test for setup\n>> +\n>> +\tgit -C B/ fetch &&\n>> +\tcompare_refs_in_dir A origin/master B origin/master\n> \n> Can we do this without relying on the name of the default branch?\n> Perhaps when outer, middle and inner are prepared, they can be\n> forced to be on the 'sample' (not 'master' nor 'main') branch, or\n> something like that?\n\nOr, simpler, we could call \"git remote set-head -a' \nin A and B in the setup script, which would make\norigin/HEAD in A and B point to the default branch, \nsuch that the call here could be :\n\ncompare_refs_in_dir A origin/HEAD B origin/HEAD\n\nPhilippe.\n\n"},{"id":"411647","messageId":"xmqqsg8h8h3d.fsf@gitster.c.googlers.com","threadId":"54746","inReplyTo":"BF6B37E9-5AF9-4A81-90D8-0270D1269332@gmail.com","subject":"Re: [PATCH v2] submodules: fix of regression on fetching of non-init subsub-repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-07T21:02:46Z","receivedAt":"2020-12-07T21:03:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philippe Blain writes:\n\n>>> +test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '\n>>> +\t# depends on previous test for setup\n>>> +\n>>> +\tgit -C B/ fetch &&\n>>> +\tcompare_refs_in_dir A origin/master B origin/master\n>> \n>> Can we do this without relying on the name of the default branch?\n>> Perhaps when outer, middle and inner are prepared, they can be\n>> forced to be on the 'sample' (not 'master' nor 'main') branch, or\n>> something like that?\n>\n> Or, simpler, we could call \"git remote set-head -a' \n> in A and B in the setup script, which would make\n> origin/HEAD in A and B point to the default branch, \n> such that the call here could be :\n\nThe set-up prepares A and B by cloning from elsewhere, no? Should\nwe even need a set-head call?\n\n> compare_refs_in_dir A origin/HEAD B origin/HEAD\n\nYes, using HEAD would be another simple way to avoid having to rely\non the default behaviour.\n\nTHanks.\n"},{"id":"411648","messageId":"xmqqo8j58gpq.fsf@gitster.c.googlers.com","threadId":"54746","inReplyTo":"xmqqsg8h8h3d.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] submodules: fix of regression on fetching of non-init subsub-repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-07T21:10:57Z","receivedAt":"2020-12-07T21:11:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Philippe Blain writes:\n>\n>>>> +test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '\n>>>> +\t# depends on previous test for setup\n>>>> +\n>>>> +\tgit -C B/ fetch &&\n>>>> +\tcompare_refs_in_dir A origin/master B origin/master\n>>> \n>>> Can we do this without relying on the name of the default branch?\n>>> Perhaps when outer, middle and inner are prepared, they can be\n>>> forced to be on the 'sample' (not 'master' nor 'main') branch, or\n>>> something like that?\n>>\n>> Or, simpler, we could call \"git remote set-head -a' \n>> in A and B in the setup script, which would make\n>> origin/HEAD in A and B point to the default branch, \n>> such that the call here could be :\n>\n> The set-up prepares A and B by cloning from elsewhere, no? Should\n> we even need a set-head call?\n\nAh, they are created by cloning an empty repository. That explains\nwhy. Thanks.\n\n>> compare_refs_in_dir A origin/HEAD B origin/HEAD\n>\n> Yes, using HEAD would be another simple way to avoid having to rely\n> on the default behaviour.\n>\n> THanks.\n"},{"id":"411713","messageId":"539b840a-51e7-5105-061b-64a7bfdee06a@nokia.com","threadId":"54746","inReplyTo":"xmqqpn3l9zce.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] submodules: fix of regression on fetching of non-init subsub-repo","fromName":"Peter Kästle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-12-08T08:46:01Z","receivedAt":"2020-12-08T08:48:15Z","isPatch":true,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"\nOn 07.12.20 20:43, Junio C Hamano wrote:\n> Philippe Blain writes:\n>> Thanks for working on that, and sorry for not having the time to comment before\n>> you sent v2.\n> \n> Thanks, all.\n> \n> It's not like we corral everybody to a single place on a single day\n> to work on a single thing. Reviews ov v2 by reviewers who have not\n> seen v1 is totally expected and very much appreciated.\n> \n\nThank you very much for all your comments, they're much appreciated. I \nprefer spending more time on it now for getting it right, than to take \nanother revert-rethink-refactor round.\nIt will take me some time today to work on them.\n\n-- \nkind regards\n--peter;\n\n\n"},{"id":"411716","messageId":"1f0422a1-f6e7-8044-ed53-c9496fbd1ccb@nokia.com","threadId":"54746","inReplyTo":"613FAD04-0D5A-4DE0-8FE8-0C5C5619B7BC@gmail.com","subject":"Re: [PATCH v2] submodules: fix of regression on fetching of non-init subsub-repo","fromName":"Peter Kästle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-12-08T14:06:08Z","receivedAt":"2020-12-08T14:07:09Z","isPatch":true,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"On 07.12.20 19:42, Philippe Blain wrote:\n> Hi Peter,\n> \n>> Le 7 déc. 2020 à 08:46, Peter Kaestle a écrit :\n>>\n>> A regression has been introduced by a62387b (submodule.c: fetch in\n>> submodules git directory instead of in worktree, 2018-11-28).\n>>\n>> The scenario in which it triggers is when one has a remote repository\n>> with a subrepository inside a subrepository like this:\n>> superproject/middle_repo/inner_repo\n> \n> The correct terminology is \"submodule\", not \"subrepository\".\n> \n> Also, (minor point) I would just write \"when one has a repository\",\n> as its simpler (the repository by itself is not \"remote\", it is only \"remote\"\n> in relation the repositories that are cloned from it).\n\nok.\n\n\n\n>> Person A and B have both a clone of it, while Person B is not working\n>> with the inner_repo and thus does not have it initialized in his working\n>> copy.\n>>\n>> Now person A introduces a change to the inner_repo and propagates it\n>> through the middle_repo and the superproject.\n>>\n>> Once person A pushed the changes and person B wants to fetch them using\n>> \"git fetch\" on superproject level,\n> \n> s/on/at the/\n\nok.\n\n\n>> B's git call will return with error\n>> saying:\n>>\n>> Could not access submodule 'inner_repo'\n>> Errors during submodule fetch:\n>> middle_repo\n>>\n>> Expectation is that in this case the inner submodule will be recognized\n>> as uninitialized subrepository and skipped by the git fetch command.\n> \n> here again, terminology: \"as an uninitialized submodule\"\n\nok.\n\n\n>> This used to work correctly before 'a62387b (submodule.c: fetch in\n>> submodules git directory instead of in worktree, 2018-11-28)'.\n>>\n>> Starting with a62387b the code wants to evaluate \"is_empty_dir()\" inside\n>> .git/modules for a directory only existing in the worktree, delivering\n>> then of course wrong return value.\n>>\n>> This patch ensures is_empty_dir() is getting the correct path of the\n>> uninitialized submodule by concatenation of the actual worktree and the\n>> name of the uninitialized submodule.\n>>\n>> Furthermore a regression test case is added, which tests for recursive\n>> fetches on a superproject with uninitialized sub repositories.\n>> This\n>> issue was leading to an infinite loop when doing a revert of a62387b.\n> \n> I would maybe add more details here, something like the following\n> (we can cite your previous attempt, because it was merged to 'master'):\n> \n> The first attempt to fix this regression, in 1b7ac4e6d4 (submodules:\n> fix of regression on fetching of non-init subsub-repo, 2020-11-12), by simply\n> reverting a62387b, resulted in\n> an infinite loop of submodule fetches in the simpler case of a recursive fetch of a superproject with\n> uninitialized submodules, and so this commit was reverted in 7091499bc0 (Revert\n> \"submodules: fix of regression on fetching of non-init subsub-repo\", 2020-12-02).\n> To prevent future breakages, also add a regression test for this scenario.\n\nJip, I like that.\n\n\n>>\n>> Signed-off-by: Peter Kaestle \n>> CC: Junio C Hamano \n>> CC: Philippe Blain \n>> CC: Ralf Thielow \n>> CC: Eric Sunshine \n>> ---\n>> submodule.c | 7 ++-\n>> t/t5526-fetch-submodules.sh | 104 ++++++++++++++++++++++++++++++++++++\n>> 2 files changed, 110 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/submodule.c b/submodule.c\n>> index b3bb59f066..b561445329 100644\n>> --- a/submodule.c\n>> +++ b/submodule.c\n>> @@ -1477,6 +1477,7 @@ static int get_next_submodule(struct child_process *cp,\n>> \t\t\tstrbuf_release(&submodule_prefix);\n>> \t\t\treturn 1;\n>> \t\t} else {\n>> +\t\t\tstruct strbuf empty_submodule_path = STRBUF_INIT;\n>>\n>> \t\t\tfetch_task_release(task);\n>> \t\t\tfree(task);\n>> @@ -1485,13 +1486,17 @@ static int get_next_submodule(struct child_process *cp,\n>> \t\t\t * An empty directory is normal,\n>> \t\t\t * the submodule is not initialized\n>> \t\t\t */\n>> +\t\t\tstrbuf_addf(&empty_submodule_path, \"%s/%s/\",\n>> +\t\t\t\t\t\t\tspf->r->worktree,\n>> +\t\t\t\t\t\t\tce->name);\n>> \t\t\tif (S_ISGITLINK(ce->ce_mode) &&\n>> -\t\t\t !is_empty_dir(ce->name)) {\n>> +\t\t\t !is_empty_dir(empty_submodule_path.buf)) {\n>> \t\t\t\tspf->result = 1;\n>> \t\t\t\tstrbuf_addf(err,\n>> \t\t\t\t\t _(\"Could not access submodule '%s'\\n\"),\n>> \t\t\t\t\t ce->name);\n>> \t\t\t}\n>> +\t\t\tstrbuf_release(&empty_submodule_path);\n>> \t\t}\n>> \t}\n> \n> \n> Maybe a personal preference, but I would have gone for something a little simpler, like the following:\n> \n> \n> diff --git a/submodule.c b/submodule.c\n> index b3bb59f066..4200865174 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1486,7 +1486,7 @@ static int get_next_submodule(struct child_process *cp,\n> * the submodule is not initialized\n> */\n> if (S_ISGITLINK(ce->ce_mode) &&\n> - !is_empty_dir(ce->name)) {\n> + !is_empty_dir(repo_worktree_path(spf->r, \"%s\", ce->name))) {\n\nI'm not deep enough into the git code to judge which approach is the \nbetter one. From my perspective, being a foreigner to the git code, I \nlike my proposed code more, as for me it's much easier to understand \nwhat's happening by having a meaningful variable name and without being \nforced to dig into outer functions first.\nAlso Junio C Hamano is having some concerns, which I \ncan't judge:\n\n> But then you leak the return value from repo_worktree_path(), no?\n\nThus for v3 I'll stick to my proposal and when you'll review it, please \ndiscuss with each other whether I should go for a v4 using \nrepo_worktree_path().\n\n[...]\n\n>> +\n>> +test_expect_success 'setup recursive fetch with uninit submodule' '\n>> +\t# does not depend on any previous test setups\n>> +\n>> +\tgit init main &&\n>> +\tgit init sub &&\n>> +\n>> +\t>sub/file &&\n>> +\tgit -C sub add file &&\n>> +\tgit -C sub commit -m \"add file\" &&\n>> +\tgit -C sub rev-parse HEAD >expect &&\n>> +\n>> +\tgit -C main submodule add ../sub &&\n>> +\tgit -C main submodule init &&\n>> +\tgit -C main submodule update --checkout &&\n> \n> These two steps are unnecessary as they are implicitly done by 'git submodule add'.\n> I think we could reflect real life a little bit more by cloning the superproject, and running\n> the 'recursive fetch with uninit submodule' test below in the clone.\n\nYes, you're right, \"...init\" and \"...update...\" can be removed.\n\n\n>> +\tgit -C main submodule status >out &&\n>> +\tsed -e \"s/^ //\" -e \"s/ sub .*$//\" out >actual &&\n>> +\ttest_cmp expect actual\n>> +'\n>> +\n>> +test_expect_success 'recursive fetch with uninit submodule' '\n>> +\t# depends on previous test for setup\n>> +\n>> +\tgit -C main submodule deinit -f sub &&\n> \n> Here you are deiniting the submodule, such that\n> the Git directory will stay in .git/modules/sub. This is not the same thing\n> as a submodule that was never initialized (\"uninitialized\"), for which .git/modules/sub\n> will not yet exist. So maybe we could harden the tests by also testing\n> for that scenario ? I don't know... maybe the infinite loop only happens\n> if .git/modules/sub actually already exists. If so, the test name should be\n> \"recursive fetch with deinitialized submodule\", I think.\n\nI added another test case for v3, which checks for this in case of never \ninitialized submodule. When executing the test, I can see that the \ninfinite loop regression only occurs after doing the init followed by a \ndeinit. Thus renaming the test accordingly.\n\n-- \nbest regards\n--peter;\n"},{"id":"411719","messageId":"d52db91d-af6d-c93c-2c4a-e2460905623d@nokia.com","threadId":"54746","inReplyTo":"xmqq360hbev1.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] submodules: fix of regression on fetching of non-init subsub-repo","fromName":"Peter Kästle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-12-08T14:58:39Z","receivedAt":"2020-12-08T14:59:46Z","isPatch":true,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"\nOn 07.12.20 20:22, Junio C Hamano wrote:\n> Peter Kaestle writes:\n> \n>> +add_commit_push () {\n>> +\tdir=\"$1\" &&\n>> +\tmsg=\"$2\" &&\n>> +\tshift 2 &&\n>> +\tgit -C \"$dir\" add \"$@\" &&\n>> +\tgit -C \"$dir\" commit -a -m \"$msg\" &&\n>> +\tgit -C \"$dir\" push\n>> +}\n>> +\n>> +compare_refs_in_dir () {\n>> +\tfail= &&\n>> +\tif test \"x$1\" = 'x!'\n>> +\tthen\n>> +\t\tfail='!' &&\n>> +\t\tshift\n>> +\tfi &&\n>> +\tgit -C \"$1\" rev-parse --verify \"$2\" >expect &&\n>> +\tgit -C \"$3\" rev-parse --verify \"$4\" >actual &&\n>> +\teval $fail test_cmp expect actual\n>> +}\n> \n> \n> \n>> +test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '\n>> +\t# depends on previous test for setup\n>> +\n>> +\tgit -C B/ fetch &&\n>> +\tcompare_refs_in_dir A origin/master B origin/master\n> \n> Can we do this without relying on the name of the default branch?\n> Perhaps when outer, middle and inner are prepared, they can be\n> forced to be on the 'sample' (not 'master' nor 'main') branch, or\n> something like that?\n\nUsing origin/HEAD for compare_refs_in_dir should be fine without \nadditional setup, as for the regression the \"git -C B/ fetch\" will fail \nand return with false (see description of the patch). This \ncompare_refs_in_dir is just for additional checking as you proposed in \nthe mail:\nhttps://public-inbox.org/git/xmqqk0uuct94.fsf@gitster.c.googlers.com/\n------------8<-------------\n> And from B that was an original copy of A with only the top and\n> middle layer instantiated, you run \"git fetch\". Are you happy as\n> long as \"git fetch\" does not exit with non-zero status? That is\n> hard to believe---it may be a necessary condition for the command to\n> exit with zero status, but you have other expectations, like what\n> commit the remote tracking branch refs/remotes/origin/HEAD ought to\n> be pointing at. I think we should check that, too.\n----------->8-------------\n\n\n> \n>> +test_expect_success 'setup recursive fetch with uninit submodule' '\n>> +\t# does not depend on any previous test setups\n>> +\n>> +\tgit init main &&\n>> +\tgit init sub &&\n> \n> \"super vs sub\" would give us a better contrast than \"main vs sub\",\n> and it would help reduce mistakes in the mechanical conversion of\n> \"master\" to \"main\" happening in another topic.\n> \n\nok.\n\n\n>> +\t# In a regression the following git call will run into infinite recursion.\n>> +\t# To handle that, we connect the grep command to the git call by a pipe\n>> +\t# so that grep can kill the infinite recusion when detected.\n>> +\t# The recursion creates git output like:\n>> +\t# Fetching submodule sub\n>> +\t# Fetching submodule sub/sub <-- [1]\n>> +\t# Fetching submodule sub/sub/sub\n>> +\t# ...\n>> +\t# [1] grep will trigger here and kill git by exiting and closing its stdin\n> \n> \"trigger here and kill...\" -> \"stop reading and cause git to\n> eventually stop and die\"\n> \n> But we probably cannot use 'grep -m1' so it is a moot point.\n> \n>> +\n>> +\t! git -C main fetch --recurse-submodules 2>&1 |\n>> +\t\tgrep -v -m1 \"Fetching submodule sub$\" &&\n> \n> Unfortunately, \"grep -m\" is not even in POSIX, I would think.\n> \n> What do we expect to happen in the correct case?\n\nsigh, we can't use grep -m1. Too bad, it was such a nice solution.\n\n> \n> - A line \"Fetching submodule sub\" and nothing else is given? That\n> feels a bit brittle (how are we making sure, in the presence of\n> \"2>&1\", that we will not get any other output, like progress?)\n> \n> - \"sub\" is the only thing that appears on lines that begin with\n> \"Fetching submodule\" (i.e. \"Fetching submodule $something\" where\n> $something is not 'sub' is an error), and we allow other garbage\n> in the output? That would be a bit more robust than the above.\n> \n> As you seem to be comfortable using \"sed\" below, perhaps use it to\n> extract the first few lines that say \"^Fetching submodule \" from the\n> output and stop, and check that the output has only one such line\n> about 'sub' and nothing else?\n\nAccording to [1] posix sed offers equal possibility to quit like grep \n-m1 and I'll adopt:\n$> yes posixgrepisnogoodforus | sed \"/posix/q\"\n\n[1] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html\n\n\nLooking at the other mails I think I processed over all open comments \nand will prepare for v3. Thanks again.\n\n-- \nkind regards\n--peter;\n\n"},{"id":"411721","messageId":"1607442126-34705-1-git-send-email-peter.kaestle@nokia.com","threadId":"54746","inReplyTo":"d52db91d-af6d-c93c-2c4a-e2460905623d@nokia.com","subject":"[PATCH v3] submodules: fix of regression on fetching of non-init subsub-repo","fromName":"Peter Kaestle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-12-08T15:42:06Z","receivedAt":"2020-12-08T15:44:32Z","isPatch":true,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"A regression has been introduced by a62387b (submodule.c: fetch in\nsubmodules git directory instead of in worktree, 2018-11-28).\n\nThe scenario in which it triggers is when one has a repository with a\nsubmodule inside a submodule like this:\nsuperproject/middle_repo/inner_repo\n\nPerson A and B have both a clone of it, while Person B is not working\nwith the inner_repo and thus does not have it initialized in his working\ncopy.\n\nNow person A introduces a change to the inner_repo and propagates it\nthrough the middle_repo and the superproject.\n\nOnce person A pushed the changes and person B wants to fetch them using\n\"git fetch\" at the superproject level, B's git call will return with\nerror saying:\n\nCould not access submodule 'inner_repo'\nErrors during submodule fetch:\n middle_repo\n\nExpectation is that in this case the inner submodule will be recognized\nas uninitialized submodule and skipped by the git fetch command.\n\nThis used to work correctly before 'a62387b (submodule.c: fetch in\nsubmodules git directory instead of in worktree, 2018-11-28)'.\n\nStarting with a62387b the code wants to evaluate \"is_empty_dir()\" inside\n.git/modules for a directory only existing in the worktree, delivering\nthen of course wrong return value.\n\nThis patch ensures is_empty_dir() is getting the correct path of the\nuninitialized submodule by concatenation of the actual worktree and the\nname of the uninitialized submodule.\n\nFurthermore a regression test case is added, which tests for recursive\nfetches on a superproject with uninitialized sub repositories. This\nissue was leading to an infinite loop when doing a revert of a62387b.\n\nThe first attempt to fix this regression, in 1b7ac4e6d4 (submodules:\nfix of regression on fetching of non-init subsub-repo, 2020-11-12), by\nsimply reverting a62387b, resulted in an infinite loop of submodule\nfetches in the simpler case of a recursive fetch of a superproject with\nuninitialized submodules, and so this commit was reverted in 7091499bc0\n(Revert \"submodules: fix of regression on fetching of non-init\nsubsub-repo\", 2020-12-02).\nTo prevent future breakages, also add a regression test for this\nscenario.\n\nSigned-off-by: Peter Kaestle \nCC: Junio C Hamano \nCC: Philippe Blain \nCC: Ralf Thielow \nCC: Philippe Blain \n---\n submodule.c | 7 ++-\n t/t5526-fetch-submodules.sh | 124 ++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 130 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex b3bb59f..b561445 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1477,6 +1477,7 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\tstrbuf_release(&submodule_prefix);\n \t\t\treturn 1;\n \t\t} else {\n+\t\t\tstruct strbuf empty_submodule_path = STRBUF_INIT;\n \n \t\t\tfetch_task_release(task);\n \t\t\tfree(task);\n@@ -1485,13 +1486,17 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\t * An empty directory is normal,\n \t\t\t * the submodule is not initialized\n \t\t\t */\n+\t\t\tstrbuf_addf(&empty_submodule_path, \"%s/%s/\",\n+\t\t\t\t\t\t\tspf->r->worktree,\n+\t\t\t\t\t\t\tce->name);\n \t\t\tif (S_ISGITLINK(ce->ce_mode) &&\n-\t\t\t !is_empty_dir(ce->name)) {\n+\t\t\t !is_empty_dir(empty_submodule_path.buf)) {\n \t\t\t\tspf->result = 1;\n \t\t\t\tstrbuf_addf(err,\n \t\t\t\t\t _(\"Could not access submodule '%s'\\n\"),\n \t\t\t\t\t ce->name);\n \t\t\t}\n+\t\t\tstrbuf_release(&empty_submodule_path);\n \t\t}\n \t}\n \ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex dd8e423..495348a 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -719,4 +719,128 @@ test_expect_success 'fetch new submodule commit intermittently referenced by sup\n \t)\n '\n \n+add_commit_push () {\n+\tdir=\"$1\" &&\n+\tmsg=\"$2\" &&\n+\tshift 2 &&\n+\tgit -C \"$dir\" add \"$@\" &&\n+\tgit -C \"$dir\" commit -a -m \"$msg\" &&\n+\tgit -C \"$dir\" push\n+}\n+\n+compare_refs_in_dir () {\n+\tfail= &&\n+\tif test \"x$1\" = 'x!'\n+\tthen\n+\t\tfail='!' &&\n+\t\tshift\n+\tfi &&\n+\tgit -C \"$1\" rev-parse --verify \"$2\" >expect &&\n+\tgit -C \"$3\" rev-parse --verify \"$4\" >actual &&\n+\teval $fail test_cmp expect actual\n+}\n+\n+\n+test_expect_success 'setup nested submodule fetch test' '\n+\t# does not depend on any previous test setups\n+\n+\tfor repo in outer middle inner\n+\tdo\n+\t\tgit init --bare $repo &&\n+\t\tgit clone $repo ${repo}_content &&\n+\t\techo \"$repo\" >\"${repo}_content/file\" &&\n+\t\tadd_commit_push ${repo}_content \"initial\" file ||\n+\t\treturn 1\n+\tdone &&\n+\n+\tgit clone outer A &&\n+\tgit -C A submodule add \"$pwd/middle\" &&\n+\tgit -C A/middle/ submodule add \"$pwd/inner\" &&\n+\tadd_commit_push A/middle/ \"adding inner sub\" .gitmodules inner &&\n+\tadd_commit_push A/ \"adding middle sub\" .gitmodules middle &&\n+\n+\tgit clone outer B &&\n+\tgit -C B/ submodule update --init middle &&\n+\n+\tcompare_refs_in_dir A HEAD B HEAD &&\n+\tcompare_refs_in_dir A/middle HEAD B/middle HEAD &&\n+\ttest_path_is_file B/file &&\n+\ttest_path_is_file B/middle/file &&\n+\ttest_path_is_missing B/middle/inner/file &&\n+\n+\techo \"change on inner repo of A\" >\"A/middle/inner/file\" &&\n+\tadd_commit_push A/middle/inner \"change on inner\" file &&\n+\tadd_commit_push A/middle \"change on inner\" inner &&\n+\tadd_commit_push A \"change on inner\" middle\n+'\n+\n+test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '\n+\t# depends on previous test for setup\n+\n+\tgit -C B/ fetch &&\n+\tcompare_refs_in_dir A origin/HEAD B origin/HEAD\n+'\n+\n+fetch_with_recusion_abort () {\n+\t# In a regression the following git call will run into infinite recursion.\n+\t# To handle that, we connect the sed command to the git call by a pipe\n+\t# so that sed can kill the infinite recusion when detected.\n+\t# The recursion creates git output like:\n+\t# Fetching submodule sub\n+\t# Fetching submodule sub/sub <-- [1]\n+\t# Fetching submodule sub/sub/sub\n+\t# ...\n+\t# [1] sed will stop reading and cause git to eventually stop and die\n+\n+\tgit -C \"$1\" fetch --recurse-submodules 2>&1 |\n+\t\tsed \"/Fetching submodule $2[^$]/q\" >out &&\n+\t! grep \"Fetching submodule $2[^$]\" out\n+}\n+\n+test_expect_success 'setup recursive fetch with uninit submodule' '\n+\t# does not depend on any previous test setups\n+\n+\t# setup a remote superproject to make git fetch work with an uninit submodule\n+\tgit init --bare super_bare &&\n+\tgit clone super_bare super &&\n+\tgit init sub &&\n+\n+\t>sub/file &&\n+\tgit -C sub add file &&\n+\tgit -C sub commit -m \"add file\" &&\n+\tgit -C sub rev-parse HEAD >expect &&\n+\n+\t# adding submodule without cloning\n+\techo \"[submodule \\\"sub\\\"]\" >super/.gitmodules &&\n+\techo \"path = sub\" >>super/.gitmodules &&\n+\techo \"url = ../sub\" >>super/.gitmodules &&\n+\tgit -C super update-index --add --cacheinfo 160000 $(cat expect) sub &&\n+\tmkdir super/sub &&\n+\n+\tgit -C super submodule status >out &&\n+\tsed -e \"s/^-//\" -e \"s/ sub.*$//\" out >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'recursive fetch with uninit submodule' '\n+\t# depends on previous test for setup\n+\n+\tfetch_with_recusion_abort super sub &&\n+\tgit -C super submodule status >out &&\n+\tsed -e \"s/^-//\" -e \"s/ sub$//\" out >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'recursive fetch after deinit a submodule' '\n+\t# depends on previous test for setup\n+\n+\tgit -C super submodule update --init sub &&\n+\tgit -C super submodule deinit -f sub &&\n+\n+\tfetch_with_recusion_abort super sub &&\n+\tgit -C super submodule status >out &&\n+\tsed -e \"s/^-//\" -e \"s/ sub$//\" out >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.6.2\n\n"},{"id":"411722","messageId":"2da143e1-d186-e87d-87e6-134c60f4428b@nokia.com","threadId":"54746","inReplyTo":"1607442126-34705-1-git-send-email-peter.kaestle@nokia.com","subject":"Re: [PATCH v3] submodules: fix of regression on fetching of non-init subsub-repo","fromName":"Peter Kästle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-12-08T15:51:07Z","receivedAt":"2020-12-08T15:52:23Z","isPatch":true,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"On 08.12.20 16:42, Peter Kaestle wrote:\n> A regression has been introduced by a62387b (submodule.c: fetch in\n> submodules git directory instead of in worktree, 2018-11-28).\n> \n> The scenario in which it triggers is when one has a repository with a\n> submodule inside a submodule like this:\n> superproject/middle_repo/inner_repo\n> \n> Person A and B have both a clone of it, while Person B is not working\n> with the inner_repo and thus does not have it initialized in his working\n> copy.\n> \n> Now person A introduces a change to the inner_repo and propagates it\n> through the middle_repo and the superproject.\n> \n> Once person A pushed the changes and person B wants to fetch them using\n> \"git fetch\" at the superproject level, B's git call will return with\n> error saying:\n> \n> Could not access submodule 'inner_repo'\n> Errors during submodule fetch:\n> middle_repo\n> \n> Expectation is that in this case the inner submodule will be recognized\n> as uninitialized submodule and skipped by the git fetch command.\n> \n> This used to work correctly before 'a62387b (submodule.c: fetch in\n> submodules git directory instead of in worktree, 2018-11-28)'.\n> \n> Starting with a62387b the code wants to evaluate \"is_empty_dir()\" inside\n> .git/modules for a directory only existing in the worktree, delivering\n> then of course wrong return value.\n> \n> This patch ensures is_empty_dir() is getting the correct path of the\n> uninitialized submodule by concatenation of the actual worktree and the\n> name of the uninitialized submodule.\n> \n> Furthermore a regression test case is added, which tests for recursive\n> fetches on a superproject with uninitialized sub repositories. This\n> issue was leading to an infinite loop when doing a revert of a62387b.\n> \n> The first attempt to fix this regression, in 1b7ac4e6d4 (submodules:\n> fix of regression on fetching of non-init subsub-repo, 2020-11-12), by\n> simply reverting a62387b, resulted in an infinite loop of submodule\n> fetches in the simpler case of a recursive fetch of a superproject with\n> uninitialized submodules, and so this commit was reverted in 7091499bc0\n> (Revert \"submodules: fix of regression on fetching of non-init\n> subsub-repo\", 2020-12-02).\n> To prevent future breakages, also add a regression test for this\n> scenario.\n> \n> Signed-off-by: Peter Kaestle \n> CC: Junio C Hamano \n> CC: Philippe Blain \n> CC: Ralf Thielow \n> CC: Philippe Blain \n> ---\n> submodule.c | 7 ++-\n> t/t5526-fetch-submodules.sh | 124 ++++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 130 insertions(+), 1 deletion(-)\n> \n> diff --git a/submodule.c b/submodule.c\n> index b3bb59f..b561445 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n\n[...]\n\n\nThese are my test run outputs:\nfinal_fix [PATCH v3]\n==========\n# passed all 41 test(s)\n\n\nrecursion_regression [1b7ac4e6d4]\n==========\nnot ok 41 - recursive fetch after deinit a submodule\n\n\nno_fix [3a0b884c - origin/master]\n==========\nnot ok 38 - fetching a superproject containing an uninitialized sub/sub \nproject\n\n\n-- \nkind regards\n--peter;\n\n"},{"id":"411738","messageId":"xmqqr1o06n5r.fsf@gitster.c.googlers.com","threadId":"54746","inReplyTo":"1607442126-34705-1-git-send-email-peter.kaestle@nokia.com","subject":"Re: [PATCH v3] submodules: fix of regression on fetching of non-init subsub-repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-08T20:46:56Z","receivedAt":"2020-12-08T20:47:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Kaestle writes:\n\n> A regression has been introduced by a62387b (submodule.c: fetch in\n> submodules git directory instead of in worktree, 2018-11-28).\n>\n> The scenario in which it triggers is when one has a repository with a\n> submodule inside a submodule like this:\n> superproject/middle_repo/inner_repo\n>\n> Person A and B have both a clone of it, while Person B is not working\n> with the inner_repo and thus does not have it initialized in his working\n> copy.\n>\n> Now person A introduces a change to the inner_repo and propagates it\n> through the middle_repo and the superproject.\n>\n> Once person A pushed the changes and person B wants to fetch them using\n> \"git fetch\" at the superproject level, B's git call will return with\n> error saying:\n>\n> Could not access submodule 'inner_repo'\n> Errors during submodule fetch:\n> middle_repo\n>\n> Expectation is that in this case the inner submodule will be recognized\n> as uninitialized submodule and skipped by the git fetch command.\n>\n> This used to work correctly before 'a62387b (submodule.c: fetch in\n> submodules git directory instead of in worktree, 2018-11-28)'.\n>\n> Starting with a62387b the code wants to evaluate \"is_empty_dir()\" inside\n> .git/modules for a directory only existing in the worktree, delivering\n> then of course wrong return value.\n>\n> This patch ensures is_empty_dir() is getting the correct path of the\n> uninitialized submodule by concatenation of the actual worktree and the\n> name of the uninitialized submodule.\n>\n> Furthermore a regression test case is added, which tests for recursive\n> fetches on a superproject with uninitialized sub repositories. This\n> issue was leading to an infinite loop when doing a revert of a62387b.\n>\n> The first attempt to fix this regression, in 1b7ac4e6d4 (submodules:\n> fix of regression on fetching of non-init subsub-repo, 2020-11-12), by\n> simply reverting a62387b, resulted in an infinite loop of submodule\n> fetches in the simpler case of a recursive fetch of a superproject with\n> uninitialized submodules, and so this commit was reverted in 7091499bc0\n> (Revert \"submodules: fix of regression on fetching of non-init\n> subsub-repo\", 2020-12-02).\n> To prevent future breakages, also add a regression test for this\n> scenario.\n>\n> Signed-off-by: Peter Kaestle \n> CC: Junio C Hamano \n> CC: Philippe Blain \n> CC: Ralf Thielow \n> CC: Philippe Blain \n> ---\n\nThanks, will replace.\n"},{"id":"411797","messageId":"CADtb9DwwfAG69YmM0+0FC6qkO361-95Uy8dNZ8jXnrVcxHSrMQ@mail.gmail.com","threadId":"54746","inReplyTo":"1607442126-34705-1-git-send-email-peter.kaestle@nokia.com","subject":"Re: [PATCH v3] submodules: fix of regression on fetching of non-init subsub-repo","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-12-08T23:25:44Z","receivedAt":"2020-12-08T23:26:40Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Peter,\n\nLe mar. 8 déc. 2020, à 10 h 43, Peter Kaestle\n a écrit :\n>\n> -- 8< --\n>\n> Furthermore a regression test case is added, which tests for recursive\n> fetches on a superproject with uninitialized sub repositories. This\n> issue was leading to an infinite loop when doing a revert of a62387b.\n\nI think this paragraph could be removed as it's saying the same thing as\nthe one below.\n\n>\n> The first attempt to fix this regression, in 1b7ac4e6d4 (submodules:\n> fix of regression on fetching of non-init subsub-repo, 2020-11-12), by\n> simply reverting a62387b, resulted in an infinite loop of submodule\n> fetches in the simpler case of a recursive fetch of a superproject with\n> uninitialized submodules, and so this commit was reverted in 7091499bc0\n> (Revert \"submodules: fix of regression on fetching of non-init\n> subsub-repo\", 2020-12-02).\n> To prevent future breakages, also add a regression test for this\n> scenario.\n>\n> Signed-off-by: Peter Kaestle \n> CC: Junio C Hamano \n> CC: Philippe Blain \n> CC: Ralf Thielow \n> CC: Philippe Blain \n> ---\n> submodule.c | 7 ++-\n> t/t5526-fetch-submodules.sh | 124 ++++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 130 insertions(+), 1 deletion(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index b3bb59f..b561445 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1477,6 +1477,7 @@ static int get_next_submodule(struct child_process *cp,\n> strbuf_release(&submodule_prefix);\n> return 1;\n> } else {\n> + struct strbuf empty_submodule_path = STRBUF_INIT;\n>\n> fetch_task_release(task);\n> free(task);\n> @@ -1485,13 +1486,17 @@ static int get_next_submodule(struct child_process *cp,\n> * An empty directory is normal,\n> * the submodule is not initialized\n> */\n> + strbuf_addf(&empty_submodule_path, \"%s/%s/\",\n> + spf->r->worktree,\n> + ce->name);\n> if (S_ISGITLINK(ce->ce_mode) &&\n> - !is_empty_dir(ce->name)) {\n> + !is_empty_dir(empty_submodule_path.buf)) {\n> spf->result = 1;\n> strbuf_addf(err,\n> _(\"Could not access submodule '%s'\\n\"),\n> ce->name);\n> }\n> + strbuf_release(&empty_submodule_path);\n> }\n> }\n>\n> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n> index dd8e423..495348a 100755\n> --- a/t/t5526-fetch-submodules.sh\n> +++ b/t/t5526-fetch-submodules.sh\n> @@ -719,4 +719,128 @@ test_expect_success 'fetch new submodule commit intermittently referenced by sup\n> )\n> '\n>\n> +add_commit_push () {\n> + dir=\"$1\" &&\n> + msg=\"$2\" &&\n> + shift 2 &&\n> + git -C \"$dir\" add \"$@\" &&\n> + git -C \"$dir\" commit -a -m \"$msg\" &&\n> + git -C \"$dir\" push\n> +}\n> +\n> +compare_refs_in_dir () {\n> + fail= &&\n> + if test \"x$1\" = 'x!'\n> + then\n> + fail='!' &&\n> + shift\n> + fi &&\n> + git -C \"$1\" rev-parse --verify \"$2\" >expect &&\n> + git -C \"$3\" rev-parse --verify \"$4\" >actual &&\n> + eval $fail test_cmp expect actual\n> +}\n> +\n> +\n> +test_expect_success 'setup nested submodule fetch test' '\n> + # does not depend on any previous test setups\n> +\n> + for repo in outer middle inner\n> + do\n> + git init --bare $repo &&\n> + git clone $repo ${repo}_content &&\n> + echo \"$repo\" >\"${repo}_content/file\" &&\n> + add_commit_push ${repo}_content \"initial\" file ||\n> + return 1\n> + done &&\n> +\n> + git clone outer A &&\n> + git -C A submodule add \"$pwd/middle\" &&\n> + git -C A/middle/ submodule add \"$pwd/inner\" &&\n> + add_commit_push A/middle/ \"adding inner sub\" .gitmodules inner &&\n> + add_commit_push A/ \"adding middle sub\" .gitmodules middle &&\n> +\n> + git clone outer B &&\n> + git -C B/ submodule update --init middle &&\n> +\n> + compare_refs_in_dir A HEAD B HEAD &&\n> + compare_refs_in_dir A/middle HEAD B/middle HEAD &&\n> + test_path_is_file B/file &&\n> + test_path_is_file B/middle/file &&\n> + test_path_is_missing B/middle/inner/file &&\n> +\n> + echo \"change on inner repo of A\" >\"A/middle/inner/file\" &&\n> + add_commit_push A/middle/inner \"change on inner\" file &&\n> + add_commit_push A/middle \"change on inner\" inner &&\n> + add_commit_push A \"change on inner\" middle\n> +'\n> +\n> +test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '\n> + # depends on previous test for setup\n> +\n> + git -C B/ fetch &&\n> + compare_refs_in_dir A origin/HEAD B origin/HEAD\n> +'\n> +\n> +fetch_with_recusion_abort () {\n\ns/recusion/recursion/\n\n> + # In a regression the following git call will run into infinite recursion.\n> + # To handle that, we connect the sed command to the git call by a pipe\n> + # so that sed can kill the infinite recusion when detected.\n\ns/recusion/recursion/\n\n> + # The recursion creates git output like:\n> + # Fetching submodule sub\n> + # Fetching submodule sub/sub <-- [1]\n> + # Fetching submodule sub/sub/sub\n> + # ...\n> + # [1] sed will stop reading and cause git to eventually stop and die\n> +\n> + git -C \"$1\" fetch --recurse-submodules 2>&1 |\n> + sed \"/Fetching submodule $2[^$]/q\" >out &&\n> + ! grep \"Fetching submodule $2[^$]\" out\n> +}\n> +\n> +test_expect_success 'setup recursive fetch with uninit submodule' '\n> + # does not depend on any previous test setups\n> +\n> + # setup a remote superproject to make git fetch work with an uninit submodule\n> + git init --bare super_bare &&\n> + git clone super_bare super &&\n> + git init sub &&\n> +\n> + >sub/file &&\n> + git -C sub add file &&\n> + git -C sub commit -m \"add file\" &&\n> + git -C sub rev-parse HEAD >expect &&\n> +\n> + # adding submodule without cloning\n> + echo \"[submodule \\\"sub\\\"]\" >super/.gitmodules &&\n> + echo \"path = sub\" >>super/.gitmodules &&\n> + echo \"url = ../sub\" >>super/.gitmodules &&\n> + git -C super update-index --add --cacheinfo 160000 $(cat expect) sub &&\n> + mkdir super/sub &&\n> +\n> + git -C super submodule status >out &&\n> + sed -e \"s/^-//\" -e \"s/ sub.*$//\" out >actual &&\n> + test_cmp expect actual\n> +'\n\nI think this is overly complicated, what I was hinting at was adding\nthe submodule\nin the superproject before cloning it, so something along these lines:\n\ntest_create_repo super &&\ntest_commit -C super initial &&\ntest_create_repo sub &&\ntest_commit -C sub initial &&\ngit -C sub rev-parse HEAD >expect &&\n\ngit -C super submodule add ../sub &&\ngit -C super commit -m \"add sub\" &&\n\ngit clone super superclone &&\ngit -C superclone submodule status >out &&\nsed -e \"s/^-//\" -e \"s/ sub.*$//\" out >actual &&\ntest_cmp expect actual\n\nAnd then running the two tests below in \"superclone\".\n\n\n> +\n> +test_expect_success 'recursive fetch with uninit submodule' '\n> + # depends on previous test for setup\n> +\n> + fetch_with_recusion_abort super sub &&\n\ns/recusion/recursion/\n\n> + git -C super submodule status >out &&\n> + sed -e \"s/^-//\" -e \"s/ sub$//\" out >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> +test_expect_success 'recursive fetch after deinit a submodule' '\n> + # depends on previous test for setup\n> +\n> + git -C super submodule update --init sub &&\n> + git -C super submodule deinit -f sub &&\n> +\n> + fetch_with_recusion_abort super sub &&\n\ns/recusion/recursion/\n\n> + git -C super submodule status >out &&\n> + sed -e \"s/^-//\" -e \"s/ sub$//\" out >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> test_done\n> --\n> 2.6.2\n>\n\nPhilippe.\n"},{"id":"411834","messageId":"c0971d1b-3bc5-8004-09f8-7ce10fb3df26@nokia.com","threadId":"54746","inReplyTo":"CADtb9DwwfAG69YmM0+0FC6qkO361-95Uy8dNZ8jXnrVcxHSrMQ@mail.gmail.com","subject":"Re: [PATCH v3] submodules: fix of regression on fetching of non-init subsub-repo","fromName":"Peter Kästle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-12-09T09:58:35Z","receivedAt":"2020-12-09T10:00:19Z","isPatch":true,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"Hi Philippe,\n\nwhen sending the patch yesterday, I had the gut feeling to hold it back \nto double check it a day later. Should have done so, some of your \nfindings were really stupid mistakes.\n\n\nOn 09.12.20 00:25, Philippe Blain wrote:\n> Le mar. 8 déc. 2020, à 10 h 43, Peter Kaestle\n> a écrit :\n>>\n>> -- 8< --\n>>\n>> Furthermore a regression test case is added, which tests for recursive\n>> fetches on a superproject with uninitialized sub repositories. This\n>> issue was leading to an infinite loop when doing a revert of a62387b.\n> \n> I think this paragraph could be removed as it's saying the same thing as\n> the one below.\n\njip.\n\n[...]\n\n>> +fetch_with_recusion_abort () {\n> \n> s/recusion/recursion/\n> \n\nyes.\n\n[...]\n\n>> +test_expect_success 'setup recursive fetch with uninit submodule' '\n>> + # does not depend on any previous test setups\n>> +\n>> + # setup a remote superproject to make git fetch work with an uninit submodule\n>> + git init --bare super_bare &&\n>> + git clone super_bare super &&\n>> + git init sub &&\n>> +\n>> + >sub/file &&\n>> + git -C sub add file &&\n>> + git -C sub commit -m \"add file\" &&\n>> + git -C sub rev-parse HEAD >expect &&\n>> +\n>> + # adding submodule without cloning\n>> + echo \"[submodule \\\"sub\\\"]\" >super/.gitmodules &&\n>> + echo \"path = sub\" >>super/.gitmodules &&\n>> + echo \"url = ../sub\" >>super/.gitmodules &&\n>> + git -C super update-index --add --cacheinfo 160000 $(cat expect) sub &&\n>> + mkdir super/sub &&\n>> +\n>> + git -C super submodule status >out &&\n>> + sed -e \"s/^-//\" -e \"s/ sub.*$//\" out >actual &&\n>> + test_cmp expect actual\n>> +'\n> \n> I think this is overly complicated, what I was hinting at was adding\n> the submodule\n> in the superproject before cloning it, so something along these lines:\n> \n> test_create_repo super &&\n> test_commit -C super initial &&\n> test_create_repo sub &&\n> test_commit -C sub initial &&\n> git -C sub rev-parse HEAD >expect &&\n> \n> git -C super submodule add ../sub &&\n> git -C super commit -m \"add sub\" &&\n> \n> git clone super superclone &&\n> git -C superclone submodule status >out &&\n> sed -e \"s/^-//\" -e \"s/ sub.*$//\" out >actual &&\n> test_cmp expect actual\n> \n> And then running the two tests below in \"superclone\".\n\nIndeed, looks much simpler, I gave it a try run and it resulted in same \nbehavior, will double check the logic of it and then send v4. Thanks a lot.\n\n[...]\n\n\n-- \nkind regards\n--peter;\n\n"},{"id":"411837","messageId":"20201209105844.7019-1-peter.kaestle@nokia.com","threadId":"54746","inReplyTo":"c0971d1b-3bc5-8004-09f8-7ce10fb3df26@nokia.com","subject":"[PATCH v4] submodules: fix of regression on fetching of non-init subsub-repo","fromName":"Peter Kaestle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-12-09T10:58:44Z","receivedAt":"2020-12-09T10:59:57Z","isPatch":true,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"A regression has been introduced by a62387b (submodule.c: fetch in\nsubmodules git directory instead of in worktree, 2018-11-28).\n\nThe scenario in which it triggers is when one has a repository with a\nsubmodule inside a submodule like this:\nsuperproject/middle_repo/inner_repo\n\nPerson A and B have both a clone of it, while Person B is not working\nwith the inner_repo and thus does not have it initialized in his working\ncopy.\n\nNow person A introduces a change to the inner_repo and propagates it\nthrough the middle_repo and the superproject.\n\nOnce person A pushed the changes and person B wants to fetch them using\n\"git fetch\" at the superproject level, B's git call will return with\nerror saying:\n\nCould not access submodule 'inner_repo'\nErrors during submodule fetch:\n middle_repo\n\nExpectation is that in this case the inner submodule will be recognized\nas uninitialized submodule and skipped by the git fetch command.\n\nThis used to work correctly before 'a62387b (submodule.c: fetch in\nsubmodules git directory instead of in worktree, 2018-11-28)'.\n\nStarting with a62387b the code wants to evaluate \"is_empty_dir()\" inside\n.git/modules for a directory only existing in the worktree, delivering\nthen of course wrong return value.\n\nThis patch ensures is_empty_dir() is getting the correct path of the\nuninitialized submodule by concatenation of the actual worktree and the\nname of the uninitialized submodule.\n\nThe first attempt to fix this regression, in 1b7ac4e6d4 (submodules:\nfix of regression on fetching of non-init subsub-repo, 2020-11-12), by\nsimply reverting a62387b, resulted in an infinite loop of submodule\nfetches in the simpler case of a recursive fetch of a superproject with\nuninitialized submodules, and so this commit was reverted in 7091499bc0\n(Revert \"submodules: fix of regression on fetching of non-init\nsubsub-repo\", 2020-12-02).\nTo prevent future breakages, also add a regression test for this\nscenario.\n\nSigned-off-by: Peter Kaestle \nCC: Junio C Hamano \nCC: Philippe Blain \nCC: Ralf Thielow \nCC: Eric Sunshine \n---\n submodule.c | 7 ++-\n t/t5526-fetch-submodules.sh | 117 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 123 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex b3bb59f066..b561445329 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1477,6 +1477,7 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\tstrbuf_release(&submodule_prefix);\n \t\t\treturn 1;\n \t\t} else {\n+\t\t\tstruct strbuf empty_submodule_path = STRBUF_INIT;\n \n \t\t\tfetch_task_release(task);\n \t\t\tfree(task);\n@@ -1485,13 +1486,17 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\t * An empty directory is normal,\n \t\t\t * the submodule is not initialized\n \t\t\t */\n+\t\t\tstrbuf_addf(&empty_submodule_path, \"%s/%s/\",\n+\t\t\t\t\t\t\tspf->r->worktree,\n+\t\t\t\t\t\t\tce->name);\n \t\t\tif (S_ISGITLINK(ce->ce_mode) &&\n-\t\t\t !is_empty_dir(ce->name)) {\n+\t\t\t !is_empty_dir(empty_submodule_path.buf)) {\n \t\t\t\tspf->result = 1;\n \t\t\t\tstrbuf_addf(err,\n \t\t\t\t\t _(\"Could not access submodule '%s'\\n\"),\n \t\t\t\t\t ce->name);\n \t\t\t}\n+\t\t\tstrbuf_release(&empty_submodule_path);\n \t\t}\n \t}\n \ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex dd8e423d25..c42ece1f04 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -719,4 +719,121 @@ test_expect_success 'fetch new submodule commit intermittently referenced by sup\n \t)\n '\n \n+add_commit_push () {\n+\tdir=\"$1\" &&\n+\tmsg=\"$2\" &&\n+\tshift 2 &&\n+\tgit -C \"$dir\" add \"$@\" &&\n+\tgit -C \"$dir\" commit -a -m \"$msg\" &&\n+\tgit -C \"$dir\" push\n+}\n+\n+compare_refs_in_dir () {\n+\tfail= &&\n+\tif test \"x$1\" = 'x!'\n+\tthen\n+\t\tfail='!' &&\n+\t\tshift\n+\tfi &&\n+\tgit -C \"$1\" rev-parse --verify \"$2\" >expect &&\n+\tgit -C \"$3\" rev-parse --verify \"$4\" >actual &&\n+\teval $fail test_cmp expect actual\n+}\n+\n+\n+test_expect_success 'setup nested submodule fetch test' '\n+\t# does not depend on any previous test setups\n+\n+\tfor repo in outer middle inner\n+\tdo\n+\t\tgit init --bare $repo &&\n+\t\tgit clone $repo ${repo}_content &&\n+\t\techo \"$repo\" >\"${repo}_content/file\" &&\n+\t\tadd_commit_push ${repo}_content \"initial\" file ||\n+\t\treturn 1\n+\tdone &&\n+\n+\tgit clone outer A &&\n+\tgit -C A submodule add \"$pwd/middle\" &&\n+\tgit -C A/middle/ submodule add \"$pwd/inner\" &&\n+\tadd_commit_push A/middle/ \"adding inner sub\" .gitmodules inner &&\n+\tadd_commit_push A/ \"adding middle sub\" .gitmodules middle &&\n+\n+\tgit clone outer B &&\n+\tgit -C B/ submodule update --init middle &&\n+\n+\tcompare_refs_in_dir A HEAD B HEAD &&\n+\tcompare_refs_in_dir A/middle HEAD B/middle HEAD &&\n+\ttest_path_is_file B/file &&\n+\ttest_path_is_file B/middle/file &&\n+\ttest_path_is_missing B/middle/inner/file &&\n+\n+\techo \"change on inner repo of A\" >\"A/middle/inner/file\" &&\n+\tadd_commit_push A/middle/inner \"change on inner\" file &&\n+\tadd_commit_push A/middle \"change on inner\" inner &&\n+\tadd_commit_push A \"change on inner\" middle\n+'\n+\n+test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '\n+\t# depends on previous test for setup\n+\n+\tgit -C B/ fetch &&\n+\tcompare_refs_in_dir A origin/HEAD B origin/HEAD\n+'\n+\n+fetch_with_recursion_abort () {\n+\t# In a regression the following git call will run into infinite recursion.\n+\t# To handle that, we connect the sed command to the git call by a pipe\n+\t# so that sed can kill the infinite recursion when detected.\n+\t# The recursion creates git output like:\n+\t# Fetching submodule sub\n+\t# Fetching submodule sub/sub <-- [1]\n+\t# Fetching submodule sub/sub/sub\n+\t# ...\n+\t# [1] sed will stop reading and cause git to eventually stop and die\n+\n+\tgit -C \"$1\" fetch --recurse-submodules 2>&1 |\n+\t\tsed \"/Fetching submodule $2[^$]/q\" >out &&\n+\t! grep \"Fetching submodule $2[^$]\" out\n+}\n+\n+test_expect_success 'setup recursive fetch with uninit submodule' '\n+\t# does not depend on any previous test setups\n+\n+\ttest_create_repo super &&\n+\ttest_commit -C super initial &&\n+\ttest_create_repo sub &&\n+\ttest_commit -C sub initial &&\n+\tgit -C sub rev-parse HEAD >expect &&\n+\n+\tgit -C super submodule add ../sub &&\n+\tgit -C super commit -m \"add sub\" &&\n+\n+\tgit clone super superclone &&\n+\tgit -C superclone submodule status >out &&\n+\tsed -e \"s/^-//\" -e \"s/ sub.*$//\" out >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'recursive fetch with uninit submodule' '\n+\t# depends on previous test for setup\n+\n+\tfetch_with_recursion_abort superclone sub &&\n+\tgit -C superclone submodule status >out &&\n+\tsed -e \"s/^-//\" -e \"s/ sub$//\" out >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'recursive fetch after deinit a submodule' '\n+\t# depends on previous test for setup\n+\n+\tgit -C superclone submodule update --init sub &&\n+\tgit -C superclone submodule deinit -f sub &&\n+\n+\tfetch_with_recursion_abort superclone sub &&\n+\tgit -C superclone submodule status >out &&\n+\tsed -e \"s/^-//\" -e \"s/ sub$//\" out >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.29.2.457.gf179bca\n\n"},{"id":"411852","messageId":"CADtb9Dy4BHckP9SMKLRJA7CRTA9Z_UmCiF4C8t0d3ARELfoszQ@mail.gmail.com","threadId":"54746","inReplyTo":"20201209105844.7019-1-peter.kaestle@nokia.com","subject":"Re: [PATCH v4] submodules: fix of regression on fetching of non-init subsub-repo","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-12-09T14:00:23Z","receivedAt":"2020-12-09T14:02:07Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Le mer. 9 déc. 2020, à 05 h 58, Peter Kaestle\n a écrit :\n>\n> ---8<---\n>\n\nReviewed-by: Philippe Blain \n\nThanks again for working on this.\n\nPhilippe.\nP.S. I wouldn't call anything you wrote in v3 \"stupid mistakes\" :) we\nall have a lot on our minds\n- especially these days!\n"}]}