{"thread":{"id":"56919","subject":"[PATCH] submodule: separate out not-found and not-empty errors","startedAt":"2021-11-16T23:06:28Z","lastAt":"2021-11-18T04:06:35Z","messageCount":3,"participants":["Ian Wienand","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"441373","messageId":"YZQ5Zk0ItWvfr8sF@fedora19.localdomain","threadId":"56919","inReplyTo":null,"subject":"[PATCH] submodule: separate out not-found and not-empty errors","fromName":"Ian Wienand","fromEmail":"iwienand@redhat.com","sentAt":"2021-11-16T23:06:14Z","receivedAt":"2021-11-16T23:06:28Z","isPatch":true,"sender":{"key":"iwienand@redhat.com","avatar":"https://avatars.githubusercontent.com/u/108448?v=4"},"body":"After upgrading past 505a2765963 a long-working script to cache git\nrepos started failing with\n\n Could not access submodule '...'\n\nfor every updated submodule on each fetch [1].\n\nUltimately this turned out to be using \"--git-dir=\" from outside the\nrepo; i.e. we really wanted \"-C\" in this script (the man page does\nwarn about this -- but it was working for a long time).\n\nAlthough obvious in hindsight, this was very difficult to diagnose\nfrom the error message. It required me adding debugging to these\nfunctions to determine why it was falling into this path when\neverything looked right on disk.\n\nThis proposes separate messages for the directory missing v. being\npresent but having unexpected contents. Both messages are modified to\ngive the path that is being examined.\n\n[1] https://review.opendev.org/c/openstack/diskimage-builder/+/818053\n\nSigned-off-by: Ian Wienand \n---\n submodule.c | 13 ++++++++++---\n 1 file changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex c689070524..910ee6ba7d 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1521,9 +1521,16 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\tif (S_ISGITLINK(ce->ce_mode) &&\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\t/* is_empty_dir also catches missing dirtectories, but report separately */\n+\t\t\t\tif (!is_directory(empty_submodule_path.buf)) {\n+\t\t\t\t strbuf_addf(err,\n+\t\t\t\t\t _(\"Submodule directory '%s' not found (incorrect --git-dir?)\\n\"),\n+\t\t\t\t\t empty_submodule_path.buf);\n+\t\t\t\t} else {\n+\t\t\t\t strbuf_addf(err,\n+\t\t\t\t\t _(\"Submodule directory '%s' is not empty\\n\"),\n+\t\t\t\t\t empty_submodule_path.buf);\n+\t\t\t\t}\n \t\t\t}\n \t\t\tstrbuf_release(&empty_submodule_path);\n \t\t}\n-- \n2.33.1\n\n"},{"id":"441440","messageId":"xmqqfsrv2kar.fsf@gitster.g","threadId":"56919","inReplyTo":"YZQ5Zk0ItWvfr8sF@fedora19.localdomain","subject":"Re: [PATCH] submodule: separate out not-found and not-empty errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-17T09:39:08Z","receivedAt":"2021-11-17T09:40:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ian Wienand writes:\n\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\t/* is_empty_dir also catches missing dirtectories, but report separately */\n> +\t\t\t\tif (!is_directory(empty_submodule_path.buf)) {\n\nI was hoping that inspecting errno after is_empty_dir() returned\nmight be sufficient (of course, we need to clear errno before\ncalling is_empty_dir() if we go that route), but because this is an\nerror codepath that we do not need to optimize, a call to\nis_directory() that incurs another system call would be fine.\n\n> +\t\t\t\t strbuf_addf(err,\n> +\t\t\t\t\t _(\"Submodule directory '%s' not found (incorrect --git-dir?)\\n\"),\n\n\"not found\" is something the code definitely knows (eh, not quite,\nbut let's read on). \n\nBut let's not make an uninformed guess. This code didn't even check\nif the user gave a --git-dir option.\n\nIf the user is advanced enough to have given \"--git-dir\", \"not found\"\nshould be sufficient to hint that the way the user specified the\nrepository location incorrectly, and a wrong \"--git-dir\" might be\none of the many things the user might suspect on their own.\n\nAnother problem with the message is !is_directory() can mean \"there\nis no filesystem entity at the path\" (i.e. \"submodule directory '%s'\ndoes not exist\") and it can also mean \"there is a filesystem entity\nat the path, but that is not a directory). \"not found\" is not exactly\na good message to give in the latter case.\n\nWe are giving two messages here in this codepath. For example, the\noriginal one would have said something like:\n\n\tCould not access submodule 'foo'\n\tSubmodule directory 'foo' is not empty\n\nSo I suspect that a more appropriate phrasing for the other one (the\nnew one you added) would be something like\n\n\tCould not access submodule 'foo'\n\tPath to the submodule 'foo' is not a directory\n\nperhaps?\n\nThanks.\n\n\n> +\t\t\t\t\t empty_submodule_path.buf);\n> +\t\t\t\t} else {\n> +\t\t\t\t strbuf_addf(err,\n> +\t\t\t\t\t _(\"Submodule directory '%s' is not empty\\n\"),\n> +\t\t\t\t\t empty_submodule_path.buf);\n> +\t\t\t\t}\n> \t\t\t}\n> \t\t\tstrbuf_release(&empty_submodule_path);\n> \t\t}\n"},{"id":"441561","messageId":"YZXRPylIwkN4L30R@fedora19.localdomain","threadId":"56919","inReplyTo":"xmqqfsrv2kar.fsf@gitster.g","subject":"Re: [PATCH] submodule: separate out not-found and not-empty errors","fromName":"Ian Wienand","fromEmail":"iwienand@redhat.com","sentAt":"2021-11-18T04:06:23Z","receivedAt":"2021-11-18T04:06:35Z","isPatch":true,"sender":{"key":"iwienand@redhat.com","avatar":"https://avatars.githubusercontent.com/u/108448?v=4"},"body":"Thanks for the review!\n\nOn Wed, Nov 17, 2021 at 01:39:08AM -0800, Junio C Hamano wrote:\n> Ian Wienand writes:\n> \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\t/* is_empty_dir also catches missing dirtectories, but report separately */\n> > +\t\t\t\tif (!is_directory(empty_submodule_path.buf)) {\n> \n> I was hoping that inspecting errno after is_empty_dir() returned\n> might be sufficient (of course, we need to clear errno before\n> calling is_empty_dir() if we go that route), but because this is an\n> error codepath that we do not need to optimize, a call to\n> is_directory() that incurs another system call would be fine.\n\nThat was my thinking too, I do admit it's a bit of an obscure case to\nend up in, but the regular path doesn't need any more checking.\n\n> > +\t\t\t\t strbuf_addf(err,\n> > +\t\t\t\t\t _(\"Submodule directory '%s' not found (incorrect --git-dir?)\\n\"),\n> \n> \"not found\" is something the code definitely knows (eh, not quite,\n> but let's read on). \n> \n> But let's not make an uninformed guess. This code didn't even check\n> if the user gave a --git-dir option.\n> \n> If the user is advanced enough to have given \"--git-dir\", \"not found\"\n> should be sufficient to hint that the way the user specified the\n> repository location incorrectly, and a wrong \"--git-dir\" might be\n> one of the many things the user might suspect on their own.\n\nOk, I can drop that bit, as you say it's a guess.\n\n> Another problem with the message is !is_directory() can mean \"there\n> is no filesystem entity at the path\" (i.e. \"submodule directory '%s'\n> does not exist\") and it can also mean \"there is a filesystem entity\n> at the path, but that is not a directory). \"not found\" is not exactly\n> a good message to give in the latter case.\n> \n> We are giving two messages here in this codepath. For example, the\n> original one would have said something like:\n> \n> \tCould not access submodule 'foo'\n> \tSubmodule directory 'foo' is not empty\n\nActually in the original patch I dropped the \"Could not access...\"\nbit, but I agree it makes more sense with the more specific details\nadded. I've put that back and append the secondary message below.\n\n> So I suspect that a more appropriate phrasing for the other one (the\n> new one you added) would be something like\n> \n> \tCould not access submodule 'foo'\n> \tPath to the submodule 'foo' is not a directory\n> \n> perhaps?\n\nDone\n\n-i\n\nFrom 6ea39c3e43eccf93e4567329cfb1b9c642154283 Mon Sep 17 00:00:00 2001\nFrom: Ian Wienand \nDate: Wed, 17 Nov 2021 09:39:40 +1100\nSubject: [PATCH] submodule: separate out not-found and not-empty errors\n\nAfter upgrading past 505a2765963 a long-working script to cache git\nrepos started failing with\n\n Could not access submodule '...'\n\nfor every updated submodule on each fetch [1].\n\nUltimately this turned out to be using \"--git-dir=\" from outside the\nrepo; i.e. we really wanted \"-C\" in this script (the man page does\nwarn about this -- but it was working for a long time).\n\nAlthough obvious in hindsight, this was very difficult to diagnose\nfrom the error message. It required me adding debugging to these\nfunctions to determine why it was falling into this path when\neverything looked right on disk.\n\nThis proposes separate messages for the directory missing v. being\npresent but having unexpected contents. Both messages are modified to\ngive the path that is being examined.\n\n[1] https://review.opendev.org/c/openstack/diskimage-builder/+/818053\n\nSigned-off-by: Ian Wienand \n---\n submodule.c | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/submodule.c b/submodule.c\nindex c689070524..352ee50f2e 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1524,6 +1524,16 @@ static int get_next_submodule(struct child_process *cp,\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\t/* is_empty_dir also catches missing directories, but report separately */\n+\t\t\t\tif (!is_directory(empty_submodule_path.buf)) {\n+\t\t\t\t strbuf_addf(err,\n+\t\t\t\t\t _(\"Submodule path '%s' is not a directory\\n\"),\n+\t\t\t\t\t empty_submodule_path.buf);\n+\t\t\t\t} else {\n+\t\t\t\t strbuf_addf(err,\n+\t\t\t\t\t _(\"Submodule directory '%s' is not empty\\n\"),\n+\t\t\t\t\t empty_submodule_path.buf);\n+\t\t\t\t}\n \t\t\t}\n \t\t\tstrbuf_release(&empty_submodule_path);\n \t\t}\n-- \n2.33.1\n\n"}]}