{"thread":{"id":"51641","subject":"Git fetch bug in git 2.21+ \"Could not access submodule '%s'\"","startedAt":"2019-08-14T10:05:00Z","lastAt":"2019-08-14T19:31:12Z","messageCount":8,"participants":["Paolo Pettinato (ppettina)","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"380430","messageId":"951a0ac4-592f-d71c-df6a-53a806249f7b@cisco.com","threadId":"51641","inReplyTo":null,"subject":"Git fetch bug in git 2.21+ \"Could not access submodule '%s'\"","fromName":"Paolo Pettinato (ppettina)","fromEmail":"ppettina@cisco.com","sentAt":"2019-08-14T09:57:50Z","receivedAt":"2019-08-14T10:05:00Z","isPatch":false,"sender":{"key":"ppettina@cisco.com","avatar":null},"body":"Spotted this when our Jenkins executors had git updated, and later \nmanaged to reproduce locally.\nThe issue happens when fetching an updated ref from a remote, and that \nref updates a submodule which is not checked out but whose folder is dirty.\n\nSteps to reproduce (on *nix) with repositories on GitHub:\nScenario: git-fetch-bug repository with 2 branches, the HEADs for these \nbranches point respectively to 2 different commits in a submodule.\n\n$ mkdir git_repo\n$ cd git_repo\n\n# Manually fetch \"master\" branch\n$ git init\n$ git remote add origin https://github.com/paolope/git-fetch-bug.git\n$ git fetch origin master\n\n# Checkout\n$ git checkout master\n\n# Repo now contains a folder named \"sm\" which is bound to contain a \nsubmodule checkout. But the submodule is not checked out yet.\n# Dirty that folder:\n$ touch sm/test\n\n# Fetching another branch will fail\n$ git fetch origin branch_1\n\nOutput:\nremote: Enumerating objects: 7, done.\nremote: Counting objects: 100% (7/7), done.\nremote: Compressing objects: 100% (3/3), done.\nremote: Total 5 (delta 2), reused 5 (delta 2), pack-reused 0\nUnpacking objects: 100% (5/5), done.\n From https://github.com/paolope/git-fetch-bug\n  * branch            branch_1   -> FETCH_HEAD\n  * [new branch]      branch_1   -> origin/branch_1\nCould not access submodule 'sm' # fails, plus no newline here :P!\n\n# Re-issuing the command succeeds\n$ git fetch origin branch_1\n\nOutput:\n From https://github.com/paolope/git-fetch-bug\n  * branch            branch_1   -> FETCH_HEAD\n\nI'd expect the command not to fail, or to fail consistently. This \nbehaviour didn't happen in git 2.18.1, so the regression was introduced \nsomewhere in between.\nIt is possible that this is related to this issue: \nhttps://marc.info/?l=git&m=155246493613592&w=2\nRegards,\nPaolo Pettinato\n**"},{"id":"380440","messageId":"20190814153607.GB12093@sigill.intra.peff.net","threadId":"51641","inReplyTo":"951a0ac4-592f-d71c-df6a-53a806249f7b@cisco.com","subject":"Re: Git fetch bug in git 2.21+ \"Could not access submodule '%s'\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-14T15:36:07Z","receivedAt":"2019-08-14T15:36:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 14, 2019 at 09:57:50AM +0000, Paolo Pettinato (ppettina) wrote:\n\n> The issue happens when fetching an updated ref from a remote, and that \n> ref updates a submodule which is not checked out but whose folder is dirty.\n> \n> Steps to reproduce (on *nix) with repositories on GitHub:\n> [...]\n> # Repo now contains a folder named \"sm\" which is bound to contain a \n> submodule checkout. But the submodule is not checked out yet.\n> # Dirty that folder:\n> $ touch sm/test\n> \n> # Fetching another branch will fail\n> $ git fetch origin branch_1\n\nThanks, I was able to reproduce here. Since this worked in v2.18.1, it\nwas an easy candidate for bisecting.  The resulting commit is 26f80ccfc1\n(submodule: migrate get_next_submodule to use repository structs,\n2018-11-28).\n\nIt looks like your case falls afoul of this logic added by that commit\nin get_next_submodule():\n\n  repo = get_submodule_repo_for(spf->r, submodule);\n  if (repo) {\n          ...\n  } else {\n          /*\n\t   * An empty directory is normal,\n\t   * the submodule is not initialized\n\t   */\n\t  if (S_ISGITLINK(ce->ce_mode) &&\n\t      !is_empty_dir(ce->name)) {\n\t\t  spf->result = 1;\n\t\t  strbuf_addf(err,\n\t\t\t      _(\"Could not access submodule '%s'\"),\n\t\t\t      ce->name);\n\t  }\n  }\n\nBecause you created a file in the uninitialized submodule directory, it\nfools the is_empty_dir() check. It seems like there should be a more\nrobust way to check whether the submodule is initialized. Maybe:\n\ndiff --git a/submodule.c b/submodule.c\nindex 77ace5e784..748ebe5909 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1294,6 +1294,9 @@ static int get_next_submodule(struct child_process *cp,\n \t\tif (!S_ISGITLINK(ce->ce_mode))\n \t\t\tcontinue;\n \n+\t\tif (!is_submodule_active(spf->r, ce->name))\n+\t\t\tcontinue;\n+\n \t\tsubmodule = submodule_from_path(spf->r, &null_oid, ce->name);\n \t\tif (!submodule) {\n \t\t\tconst char *name = default_name_or_path(ce->name);\n\nbut that seems to fail t5526's \"on-demand works without .gitmodules\nentry\" test.\n\nI think is_submodule_populated_gently() more exactly matches what the\ncurrent code is trying to do, like so:\n\ndiff --git a/submodule.c b/submodule.c\nindex 77ace5e784..4b26faee5d 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1347,8 +1347,7 @@ 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\tif (S_ISGITLINK(ce->ce_mode) &&\n-\t\t\t    !is_empty_dir(ce->name)) {\n+\t\t\tif (is_submodule_populated_gently(ce->name, NULL)) {\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\nbut it feels odd to me. Even if the submodule is not currently checked\nout, we'd presumably still want to do the recursive fetch as long as we\nhave a repo under $GIT_DIR/modules? And anyway, it fails another test in\nt5526 (\"fetching submodule into a broken repository\").\n\nMaybe somebody with more submodule expertise can jump in.\n\n> # Re-issuing the command succeeds\n> $ git fetch origin branch_1\n> [...]\n> I'd expect the command not to fail, or to fail consistently.\n\nI think that part is expected. After receiving new objects, `fetch`\ntries to see if it found out about any new submodules that might need to\nbe recursively fetched. In your first command, we actually received the\nnew objects (but then erroneously complained because the submodule was\nnot initialized). In the second, we already had those objects and didn't\nneed to do that check.\n\n-Peff\n"},{"id":"380442","messageId":"20190814155953.GC12093@sigill.intra.peff.net","threadId":"51641","inReplyTo":"951a0ac4-592f-d71c-df6a-53a806249f7b@cisco.com","subject":"[PATCH] get_next_submodule(): format error string as an error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-14T15:59:53Z","receivedAt":"2019-08-14T15:59:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 14, 2019 at 09:57:50AM +0000, Paolo Pettinato (ppettina) wrote:\n\n> Could not access submodule 'sm' # fails, plus no newline here :P!\n\nThis part seems easy enough to fix.\n\n-- >8 --\nSubject: get_next_submodule(): format error string as an error\n\nThe run_processes_parallel() interface passes its callback functions an\n\"err\" strbuf in which they can accumulate errors. However, this differs\nfrom our usual \"err\" strbufs in that the result is not simply passed to\nerror(), like:\n\n  if (frob_repo(&err) < 0)\n\terror(\"frobnication failed: %s\", err.buf);\n\nInstead, we append the error buffer as-is to a buffer collecting the\nsub-process stderr, adding neither a prefix nor a trailing newline. This\ngives callbacks more flexibility (e.g., get_next_submodule() adds its\nown \"Fetching submodule foo\" informational lines), but it means they're\nalso responsible for formatting any errors themselves.\n\nWe forgot to do so in the single error message in get_next_submodule(),\nmeaning that it was output without a trailing newline. While we're\nfixing that, let's also give it the usual \"error:\" prefix and downcase\nthe start of the message. We can't use error() here, because it always\noutputs directly to stderr.\n\nLooking at other users of run_processes_parallel(), there are a few\nsimilar messages in update_clone_task_finished(). But those sites do\ncorrectly add a newline (they don't use an \"error\" prefix, but it\ndoesn't make as much sense there).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n submodule.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 0f199c5137..a5ba57ac36 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1478,7 +1478,7 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\t    !is_empty_dir(ce->name)) {\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+\t\t\t\t\t    _(\"error: could not access submodule '%s'\\n\"),\n \t\t\t\t\t    ce->name);\n \t\t\t}\n \t\t}\n-- \n2.23.0.rc2.479.gbd16c8906f\n\n"},{"id":"380444","messageId":"xmqqtvaj67v3.fsf@gitster-ct.c.googlers.com","threadId":"51641","inReplyTo":"20190814155953.GC12093@sigill.intra.peff.net","subject":"Re: [PATCH] get_next_submodule(): format error string as an error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-14T16:15:44Z","receivedAt":"2019-08-14T16:15:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Aug 14, 2019 at 09:57:50AM +0000, Paolo Pettinato (ppettina) wrote:\n>\n>> Could not access submodule 'sm' # fails, plus no newline here :P!\n>\n> This part seems easy enough to fix.\n\nIndeed, it is ;-)\n\n> diff --git a/submodule.c b/submodule.c\n> index 0f199c5137..a5ba57ac36 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1478,7 +1478,7 @@ static int get_next_submodule(struct child_process *cp,\n>  \t\t\t    !is_empty_dir(ce->name)) {\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> +\t\t\t\t\t    _(\"error: could not access submodule '%s'\\n\"),\n>  \t\t\t\t\t    ce->name);\n>  \t\t\t}\n>  \t\t}\n"},{"id":"380446","messageId":"xmqqpnl766pj.fsf@gitster-ct.c.googlers.com","threadId":"51641","inReplyTo":"20190814153607.GB12093@sigill.intra.peff.net","subject":"Re: Git fetch bug in git 2.21+ \"Could not access submodule '%s'\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-14T16:40:40Z","receivedAt":"2019-08-14T16:40:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Because you created a file in the uninitialized submodule directory, it\n> fools the is_empty_dir() check. It seems like there should be a more\n> robust way to check whether the submodule is initialized. Maybe:\n>\n> diff --git a/submodule.c b/submodule.c\n> index 77ace5e784..748ebe5909 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1294,6 +1294,9 @@ static int get_next_submodule(struct child_process *cp,\n>  \t\tif (!S_ISGITLINK(ce->ce_mode))\n>  \t\t\tcontinue;\n>  \n> +\t\tif (!is_submodule_active(spf->r, ce->name))\n> +\t\t\tcontinue;\n> +\n>  \t\tsubmodule = submodule_from_path(spf->r, &null_oid, ce->name);\n>  \t\tif (!submodule) {\n>  \t\t\tconst char *name = default_name_or_path(ce->name);\n>\n> but that seems to fail t5526's \"on-demand works without .gitmodules\n> entry\" test.\n>\n> I think is_submodule_populated_gently() more exactly matches what the\n> current code is trying to do, like so:\n>\n> diff --git a/submodule.c b/submodule.c\n> index 77ace5e784..4b26faee5d 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1347,8 +1347,7 @@ 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\tif (S_ISGITLINK(ce->ce_mode) &&\n> -\t\t\t    !is_empty_dir(ce->name)) {\n> +\t\t\tif (is_submodule_populated_gently(ce->name, NULL)) {\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> but it feels odd to me. Even if the submodule is not currently checked\n> out, we'd presumably still want to do the recursive fetch as long as we\n> have a repo under $GIT_DIR/modules?\n\n... which means that we are not interested in \"is it populated?\" but\nin \"have we done 'git submodule init' to show interest in it?\".  But\nsince we are walking the in-core index and picking only the gitlink\nentries in it in the early part of this loop, we know ce cannot be\nanything but a submodule at this point, so we will not be in the \"we\nare interesteed in the submodule, but the current HEAD and index is\nat a commit that does not have it, hence $GIT_DIR/modules/ is the\nonly place that knows about it\" situation.  If we are interested in\nit enough to have a repository stashed under $GIT_DIR/modules/, we\nshould have a submodule there, shouldn't we?\n\nWhat I do not quite get is that repo_submodule_init(), which is\ncalled by get_submodule_repo_for(), looks into $GIT_DIR/modules/,\naccording to the in-code comment of that function.  So \"we cannot\nget the repo for it, which is an error condition, but we will\ncomplain only for non-empty directory\" logic feels iffy.\n\nStepping back even a bit more, \"an empty directory is normal\" makes\nsome sense.  If the user or the build system created a non-directory\nat a path where a populated submodule would sit, that would not be\ngood.  If the user or the build system created a random file in the\nunpopulated empty directory, in which the working tree files of the\nsubmodule would be created once the submodule getspopulated, that\nwould be equally bad, wouldn't it?  Why is the user mucking with\nthat directory in the first place, and isn't the flagging of the\nsituation as an error, done with 26f80ccf (\"submodule: migrate\nget_next_submodule to use repository structs\", 2018-11-28), a\nbugfix?  If not, why not?\n\nPuzzled....\n\n"},{"id":"380447","messageId":"5a58b0eb-0690-c445-dbfd-bd4c5b614629@cisco.com","threadId":"51641","inReplyTo":"xmqqpnl766pj.fsf@gitster-ct.c.googlers.com","subject":"Re: Git fetch bug in git 2.21+ \"Could not access submodule '%s'\"","fromName":"Paolo Pettinato (ppettina)","fromEmail":"ppettina@cisco.com","sentAt":"2019-08-14T17:03:44Z","receivedAt":"2019-08-14T17:03:53Z","isPatch":false,"sender":{"key":"ppettina@cisco.com","avatar":null},"body":"Thanks for the reply!\n\nOn 14/08/2019 17:40, Junio C Hamano wrote:\n>   Why is the user mucking with\n> that directory in the first place, and isn't the flagging of the\n> situation as an error, done with 26f80ccf (\"submodule: migrate\n> get_next_submodule to use repository structs\", 2018-11-28), a\n> bugfix?  If not, why not?\n\nNot sure if you're implying here that this is not a bug; I'd say that:\n\n- Mucking about with a folder that's supposed to contain a submodule is \nnot something that a lot of people do (and we worked around the issue), \nand people shouldn't do that, but...\n\n- ... regardless, I believe that \"git fetch\" shouldn't particularly care \nabout the state of the current working directory. I didn't ask it to do \nanything with the submodules, nor have I initialised them. In my \n(limited) knowledge of git, I'd expect git fetch to do its magic \nentirely between the remote and the .git folder.\n\nOur use case was for a submodule containing encrypted secrets; and the \nmucking about was stubbing out those secrets in a test build without \nfetching/decrypting them.\n\nIt's arguable whether this should be fixed or not; less arguable that we \ncould use a better error message and consistency (1st execution fails, \n2nd does not).\n"},{"id":"380450","messageId":"xmqqh86j637g.fsf@gitster-ct.c.googlers.com","threadId":"51641","inReplyTo":"5a58b0eb-0690-c445-dbfd-bd4c5b614629@cisco.com","subject":"Re: Git fetch bug in git 2.21+ \"Could not access submodule '%s'\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-14T17:56:19Z","receivedAt":"2019-08-14T17:56:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Paolo Pettinato (ppettina)\" <ppettina@cisco.com> writes:\n\n> Thanks for the reply!\n>\n> On 14/08/2019 17:40, Junio C Hamano wrote:\n>>   Why is the user mucking with\n>> that directory in the first place, and isn't the flagging of the\n>> situation as an error, done with 26f80ccf (\"submodule: migrate\n>> get_next_submodule to use repository structs\", 2018-11-28), a\n>> bugfix?  If not, why not?\n>\n> Not sure if you're implying here that this is not a bug; I'd say that:\n\nYeah, sorry for a confused comment.\n\nIt does feel strange that the error behaviour depends on what is in\nthe working tree for a \"fetch\", which is between two $GIT_DIR and\ndoes not involve the working tree on the receiving side (that brings\nus back to my earlier comment in the same message).\n"},{"id":"380457","messageId":"20190814193110.GA31218@sigill.intra.peff.net","threadId":"51641","inReplyTo":"xmqqpnl766pj.fsf@gitster-ct.c.googlers.com","subject":"Re: Git fetch bug in git 2.21+ \"Could not access submodule '%s'\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-14T19:31:10Z","receivedAt":"2019-08-14T19:31:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 14, 2019 at 09:40:40AM -0700, Junio C Hamano wrote:\n\n> > but it feels odd to me. Even if the submodule is not currently checked\n> > out, we'd presumably still want to do the recursive fetch as long as we\n> > have a repo under $GIT_DIR/modules?\n> \n> ... which means that we are not interested in \"is it populated?\" but\n> in \"have we done 'git submodule init' to show interest in it?\".  But\n> since we are walking the in-core index and picking only the gitlink\n> entries in it in the early part of this loop, we know ce cannot be\n> anything but a submodule at this point, so we will not be in the \"we\n> are interesteed in the submodule, but the current HEAD and index is\n> at a commit that does not have it, hence $GIT_DIR/modules/ is the\n> only place that knows about it\" situation.  If we are interested in\n> it enough to have a repository stashed under $GIT_DIR/modules/, we\n> should have a submodule there, shouldn't we?\n\nYeah, as I was writing the above I too was wondering whether this was a\ncase that could even happen. So it may be that the two ways of asking\nthe question end up the same in practice.\n\n> What I do not quite get is that repo_submodule_init(), which is\n> called by get_submodule_repo_for(), looks into $GIT_DIR/modules/,\n> according to the in-code comment of that function.  So \"we cannot\n> get the repo for it, which is an error condition, but we will\n> complain only for non-empty directory\" logic feels iffy.\n\nRight. This whole non-empty directory thing _feels_ like a hack that was\nadded to paper over the fact that get_submodule_repo_for() does not\ndistinguish between \"nope, this module is not active\" and \"an error\noccurred\".\n\nGiven that any real errors there would come from\nread_and_verify_repository_format(), which generates its own error\nmessages, I wonder if we should simply quietly ignore any entries for\nwhich get_submodule_repo_for() returns NULL. I suppose that would impact\nour exit code, though (i.e., a real broken submodule would not cause the\nouter fetch to exit with a non-zero code).\n\nProbably that could be dealt with by having get_submodule_repo_for()\nreturn a tristate enum: a working struct, an error, or ENOENT. Actually,\nI guess we could set errno. ;)\n\nIt's not clear to me, though, that the rest of the functions are\ndistinguishing between \"broken repo at submodule path\" and \"no such\nsubmodule\". For instance, get_submodule_repo_for() itself will happily\nhit a fallback path if repo_submodule_init() returns an error. That\nwould really only want to trigger on this ENOENT-equivalent case.\n\n> Stepping back even a bit more, \"an empty directory is normal\" makes\n> some sense.  If the user or the build system created a non-directory\n> at a path where a populated submodule would sit, that would not be\n> good.  If the user or the build system created a random file in the\n> unpopulated empty directory, in which the working tree files of the\n> submodule would be created once the submodule getspopulated, that\n> would be equally bad, wouldn't it?  Why is the user mucking with\n> that directory in the first place, and isn't the flagging of the\n> situation as an error, done with 26f80ccf (\"submodule: migrate\n> get_next_submodule to use repository structs\", 2018-11-28), a\n> bugfix?  If not, why not?\n\nI agree that the user putting things in that directory is kind of weird.\nIt just seems odd that fetch, which doesn't at all care about the\nworking tree, would be the thing to complain about it.\n\n-Peff\n"}]}