{"thread":{"id":"44917","subject":"[RFC/PATCH] Disallow commands from within unpopulated submodules.","startedAt":"2017-01-19T19:30:33Z","lastAt":"2017-01-21T12:55:41Z","messageCount":13,"participants":["Stefan Beller","Jeff King","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"309761","messageId":"20170119193023.26837-1-sbeller@google.com","threadId":"44917","inReplyTo":null,"subject":"[RFC/PATCH] Disallow commands from within unpopulated submodules.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-19T19:30:23Z","receivedAt":"2017-01-19T19:30:33Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Consider you have a submodule at path \"sub\". What should happen in case\nyou run a command such as \"git -C sub add .\" ?\n\nHere is what currently happens:\n1) The submodule is populated, i.e. there is a .git (file/dir) inside\n \"sub\". This is equivalent of running \"git add .\" in the submodule and\n it behaves as you would expect, adding all files to the index.\n2) The submodule is not populated or even not initialized.\n For quite some time we got\n $ git -C sub add .\n git: pathspec.c:317: prefix_pathspec: Assertion `item->nowildcard_len <= item->len && item->prefix <= item->len' failed.\n Aborted (core dumped)\n (This is fixed by another patch in flight to not assert,\n but rather die with a better message instead; but that patch is\n merely a fix of a corner case in the pathspec code.)\n\nWhile 1) is rather uncontroversial, there are multiple things the user\nmay have intended with this command in 2):\n * add the submodule to the superproject\n * add all files inside the sub/ directory to the submodule or\n superproject.\nIt is unclear what the user intended, so rather error out instead.\n\nNow let's ask the same question for \"git -C sub status .\" (which is a\ncommand that is only reading and not writing to the repository)\n\n1) If the submodule is populated, the user clearly intended to know\n more about the submodules status\n2) It is unclear if the user wanted to learn about the submodules state\n (So ideally: \"The submodule 'sub' is not initialized. To init ...\")\n or the status check should be applied to the superproject instead.\n\nAvoid the confusion in 2) as well and just error out for now. Later on\nwe may want to add another flag to git.c to allow commands to be run\ninside unpopulated submodules and each command reacts appropriately.\n\nSigned-off-by: Stefan Beller \n---\n\nThis is the next logical step after sb/pathspec-errors (pathspec:\ngive better message for submodule related pathspec error). If you are in\na path that is clearly a submodules, I would expect that most users would\nexpect the git operation to apply to the submodule. In case of unpopulated\nsubmodules, this is not the case though, but we apply the operation to the\nsuperproject, which may be wrong or confusing. Hence just error out for now.\nLater we may want to add a flag that allows specific commands to run in such\na setup (e.g. git status could give a fancier message than a die(..)).\n\nI marked this as RFC\n* to request for comments if this is a good idea from a UI-perspective\n* because I did not adapt any test for this patch. (A lot of submodule tests\n seem to break with this; From a cursory read of those tests, I'd rather\n blame the tests for being sloppy than this patch damaging user expectations)\n\nThanks,\nStefan\n\n git.c | 3 +++\n submodule.c | 36 ++++++++++++++++++++++++++++++++++++\n submodule.h | 1 +\n 3 files changed, 40 insertions(+)\n\ndiff --git a/git.c b/git.c\nindex bbaa949e9c..ca53b61474 100644\n--- a/git.c\n+++ b/git.c\n@@ -2,6 +2,7 @@\n #include \"exec_cmd.h\"\n #include \"help.h\"\n #include \"run-command.h\"\n+#include \"submodule.h\"\n \n const char git_usage_string[] =\n \t\"git [--version] [--help] [-C ] [-c name=value]\\n\"\n@@ -364,6 +365,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \t\tif (prefix)\n \t\t\tdie(\"can't use --super-prefix from a subdirectory\");\n \t}\n+\tif (prefix)\n+\t\tcheck_prefix_inside_submodule(prefix);\n \n \tif (!help && p->option & NEED_WORK_TREE)\n \t\tsetup_work_tree();\ndiff --git a/submodule.c b/submodule.c\nindex 73521cdbb2..357a22b804 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -495,6 +495,42 @@ void set_config_fetch_recurse_submodules(int value)\n \tconfig_fetch_recurse_submodules = value;\n }\n \n+/* check if the given prefix is inside an uninitialized submodule */\n+void check_prefix_inside_submodule(const char *prefix)\n+{\n+\tconst char *work_tree = get_git_work_tree();\n+\tif (work_tree) {\n+\t\tint pos;\n+\t\tconst struct cache_entry *in_submodule = NULL;\n+\n+\t\tif (read_cache() < 0)\n+\t\t\tdie(\"index file corrupt\");\n+\t\tpos = cache_name_pos(prefix, strlen(prefix));\n+\t\t/*\n+\t\t * gitlinks are recored with no ending '/' in the index,\n+\t\t * but the prefix has an ending '/', so we will never find\n+\t\t * an exact match, but always the position where we'd\n+\t\t * insert the prefix.\n+\t\t */\n+\t\tif (pos < 0) {\n+\t\t\tconst struct cache_entry *ce;\n+\t\t\tint len = strlen(prefix);\n+\t\t\t/* Check the previous position */\n+\t\t\tpos = (-1 - pos) - 1;\n+\t\t\tce = active_cache[pos];\n+\t\t\tif (ce->ce_namelen < len)\n+\t\t\t\tlen = ce->ce_namelen;\n+\t\t\tif (!memcmp(ce->name, prefix, len))\n+\t\t\t\tin_submodule = ce;\n+\t\t} else\n+\t\t\t/* This case cannot happen because */\n+\t\t\tdie(\"BUG: prefixes end with '/', but we do not record ending slashes in the index\");\n+\n+\t\tif (in_submodule)\n+\t\t\tdie(_(\"command from inside unpopulated submodule '%s' not supported.\"), in_submodule->name);\n+\t}\n+}\n+\n static int has_remote(const char *refname, const struct object_id *oid,\n \t\t int flags, void *cb_data)\n {\ndiff --git a/submodule.h b/submodule.h\nindex b7576d6f43..6b30542822 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -30,6 +30,7 @@ struct submodule_update_strategy {\n };\n #define SUBMODULE_UPDATE_STRATEGY_INIT {SM_UPDATE_UNSPECIFIED, NULL}\n \n+extern void check_prefix_inside_submodule(const char *prefix);\n int is_staging_gitmodules_ok(void);\n int update_path_in_gitmodules(const char *oldpath, const char *newpath);\n int remove_path_from_gitmodules(const char *path);\n-- \n2.11.0.299.g762782ba8a\n\n"},{"id":"309849","messageId":"20170120191728.l3ne5tt5pwbmafjh@sigill.intra.peff.net","threadId":"44917","inReplyTo":"20170119193023.26837-1-sbeller@google.com","subject":"Re: [RFC/PATCH] Disallow commands from within unpopulated submodules.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-20T19:17:28Z","receivedAt":"2017-01-20T19:18:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 19, 2017 at 11:30:23AM -0800, Stefan Beller wrote:\n\n> Now let's ask the same question for \"git -C sub status .\" (which is a\n> command that is only reading and not writing to the repository)\n> \n> 1) If the submodule is populated, the user clearly intended to know\n> more about the submodules status\n> 2) It is unclear if the user wanted to learn about the submodules state\n> (So ideally: \"The submodule 'sub' is not initialized. To init ...\")\n> or the status check should be applied to the superproject instead.\n> \n> Avoid the confusion in 2) as well and just error out for now. Later on\n> we may want to add another flag to git.c to allow commands to be run\n> inside unpopulated submodules and each command reacts appropriately.\n\nI like the general idea of catching commands in unpopulated submodules,\nbut I'm somewhat uncomfortable with putting an unconditional check into\ngit.c, for two reasons:\n\n 1. Reading the index can be expensive. You would not want \"git\n rev-parse\" to incur this cost.\n\n 2. How does this interact with commands which do interact with the\n index? Don't they expect to find the_index unpopulated?\n\n (I notice that it's effectively tied to RUN_SETUP, which is good.\n But that also means that many commands, like \"diff\", won't get the\n benefit. Not to mention non-builtins).\n\nI'd rather see it in the commands themselves. Especially given the\n\"ideal\" in your status example, which requires command-specific\nknowledge.\n\n-Peff\n"},{"id":"309851","messageId":"CAGZ79kaJQefSDkV-LKxRCTtSepsNsX7U+AZqy3Z_YCd1xsmTxQ@mail.gmail.com","threadId":"44917","inReplyTo":"20170120191728.l3ne5tt5pwbmafjh@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] Disallow commands from within unpopulated submodules.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-20T19:33:45Z","receivedAt":"2017-01-20T19:34:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jan 20, 2017 at 11:17 AM, Jeff King wrote:\n> On Thu, Jan 19, 2017 at 11:30:23AM -0800, Stefan Beller wrote:\n>\n>> Now let's ask the same question for \"git -C sub status .\" (which is a\n>> command that is only reading and not writing to the repository)\n>>\n>> 1) If the submodule is populated, the user clearly intended to know\n>> more about the submodules status\n>> 2) It is unclear if the user wanted to learn about the submodules state\n>> (So ideally: \"The submodule 'sub' is not initialized. To init ...\")\n>> or the status check should be applied to the superproject instead.\n>>\n>> Avoid the confusion in 2) as well and just error out for now. Later on\n>> we may want to add another flag to git.c to allow commands to be run\n>> inside unpopulated submodules and each command reacts appropriately.\n>\n> I like the general idea of catching commands in unpopulated submodules,\n> but I'm somewhat uncomfortable with putting an unconditional check into\n> git.c, for two reasons:\n>\n> 1. Reading the index can be expensive. You would not want \"git\n> rev-parse\" to incur this cost.\n\nWell, I would want rev-parse to not be run in the wrong repo.\n(intended to rev-parse something in the submodule, but got results for\nthe superproject).\n\nTalking about rev-parse, I was about to propose an extension in reply to\n\"[PATCH] git-prompt.sh: add submodule indicator\" that rev-parse could\nlearn a flag similar to --show-toplevel, named:\n --show-superproject-if-any or\n --indicate-if-in-submodule-possibly\nwhich would help out there.\n\n>\n> 2. How does this interact with commands which do interact with the\n> index? Don't they expect to find the_index unpopulated?\n\nThat is another sloppiness in this RFC patch, as I haven't nailed down\nthe corner cases yet.\n\n>\n> (I notice that it's effectively tied to RUN_SETUP, which is good.\n> But that also means that many commands, like \"diff\", won't get the\n> benefit. Not to mention non-builtins).\n>\n> I'd rather see it in the commands themselves. Especially given the\n> \"ideal\" in your status example, which requires command-specific\n> knowledge.\n\nSo you rather want to go bottom up, i.e. add it to each command individually\nfor which it makes sense, instead of rather first having a catch-it-all like\nthis and then we can have a flag similar to RUN_SETUP, e.g.\nALLOW_IN_UNPOP_SUBMODULE, which allows commands to\ntake over the responsibility to act responsibly in this case?\n\nstatus may be the first command for going that route; I wonder if we'd\nwant to add this feature unconditionally or only in the porcelain case.\n(In plumbing you're supposed to know what you're doing... so there is\nno need as well as our promise to not change it)\n\nThanks,\nStefan\n\n>\n> -Peff\n"},{"id":"309852","messageId":"20170120194224.vikzovupwqx53x2c@sigill.intra.peff.net","threadId":"44917","inReplyTo":"CAGZ79kaJQefSDkV-LKxRCTtSepsNsX7U+AZqy3Z_YCd1xsmTxQ@mail.gmail.com","subject":"Re: [RFC/PATCH] Disallow commands from within unpopulated submodules.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-20T19:42:24Z","receivedAt":"2017-01-20T19:42:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 20, 2017 at 11:33:45AM -0800, Stefan Beller wrote:\n\n> > I'd rather see it in the commands themselves. Especially given the\n> > \"ideal\" in your status example, which requires command-specific\n> > knowledge.\n> \n> So you rather want to go bottom up, i.e. add it to each command individually\n> for which it makes sense, instead of rather first having a catch-it-all like\n> this and then we can have a flag similar to RUN_SETUP, e.g.\n> ALLOW_IN_UNPOP_SUBMODULE, which allows commands to\n> take over the responsibility to act responsibly in this case?\n\nYes. I know it's \"less safe\" in the sense that commands have to make an\neffort to detect the situation, but I feel like only they'll know what\nthe sensible behavior is. And they can also do the check at a time when\nthey would be reading the index anyway.\n\n> status may be the first command for going that route; I wonder if we'd\n> want to add this feature unconditionally or only in the porcelain case.\n> (In plumbing you're supposed to know what you're doing... so there is\n> no need as well as our promise to not change it)\n\nYeah. The reason that it would be so painful to load the index\nfor every rev-parse is not just that it probably doesn't otherwise need\nthe index, but that scripts may make a _ton_ of rev-parse (or other\nplumbing) calls.\n\nOne alternative would be to make the check cheaper. Could we reliably\ntell from the submodule.foo.* block in the config that path \"foo\" is a\nsubmodule? I think that would work after \"submodule init\" but not right\nafter \"git clone\". So the index really is the source of truth there.\n\nI guess there could be an index extension \"these are the gitlinks I\ncontain\" and in theory we could read just that extension. I dunno.\n\n-Peff\n"},{"id":"309853","messageId":"CAGZ79kYKY=hDVjUx7AkeWZ=3V8Fy2hqQMFPZcoxT4NvXTFgG=Q@mail.gmail.com","threadId":"44917","inReplyTo":"20170120194224.vikzovupwqx53x2c@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] Disallow commands from within unpopulated submodules.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-20T19:53:01Z","receivedAt":"2017-01-20T19:53:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jan 20, 2017 at 11:42 AM, Jeff King wrote:\n> On Fri, Jan 20, 2017 at 11:33:45AM -0800, Stefan Beller wrote:\n>\n>> > I'd rather see it in the commands themselves. Especially given the\n>> > \"ideal\" in your status example, which requires command-specific\n>> > knowledge.\n>>\n>> So you rather want to go bottom up, i.e. add it to each command individually\n>> for which it makes sense, instead of rather first having a catch-it-all like\n>> this and then we can have a flag similar to RUN_SETUP, e.g.\n>> ALLOW_IN_UNPOP_SUBMODULE, which allows commands to\n>> take over the responsibility to act responsibly in this case?\n>\n> Yes. I know it's \"less safe\" in the sense that commands have to make an\n> effort to detect the situation, but I feel like only they'll know what\n> the sensible behavior is. And they can also do the check at a time when\n> they would be reading the index anyway.\n>\n>> status may be the first command for going that route; I wonder if we'd\n>> want to add this feature unconditionally or only in the porcelain case.\n>> (In plumbing you're supposed to know what you're doing... so there is\n>> no need as well as our promise to not change it)\n>\n> Yeah. The reason that it would be so painful to load the index\n> for every rev-parse is not just that it probably doesn't otherwise need\n> the index, but that scripts may make a _ton_ of rev-parse (or other\n> plumbing) calls.\n>\n> One alternative would be to make the check cheaper. Could we reliably\n> tell from the submodule.foo.* block in the config that path \"foo\" is a\n> submodule? I think that would work after \"submodule init\" but not right\n> after \"git clone\". So the index really is the source of truth there.\n\nWell we can check if there is a .gitmodules file that has a\nsubmodule.*.path equal to the last part of $CWD, no need to look\nat the git config.\n\nAnd that would also work right after git clone (in an\nunpopulated/uninitialized submodule as I call it).\n\nAnd in my current understanding of submodules the check in\n.gitmodules ought to be enough, too.\n\n>\n> I guess there could be an index extension \"these are the gitlinks I\n> contain\" and in theory we could read just that extension. I dunno.\n>\n> -Peff\n"},{"id":"309854","messageId":"20170120200041.hefg44stddqe344z@sigill.intra.peff.net","threadId":"44917","inReplyTo":"CAGZ79kYKY=hDVjUx7AkeWZ=3V8Fy2hqQMFPZcoxT4NvXTFgG=Q@mail.gmail.com","subject":"Re: [RFC/PATCH] Disallow commands from within unpopulated submodules.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-20T20:00:41Z","receivedAt":"2017-01-20T20:00:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 20, 2017 at 11:53:01AM -0800, Stefan Beller wrote:\n\n> > One alternative would be to make the check cheaper. Could we reliably\n> > tell from the submodule.foo.* block in the config that path \"foo\" is a\n> > submodule? I think that would work after \"submodule init\" but not right\n> > after \"git clone\". So the index really is the source of truth there.\n> \n> Well we can check if there is a .gitmodules file that has a\n> submodule.*.path equal to the last part of $CWD, no need to look\n> at the git config.\n> \n> And that would also work right after git clone (in an\n> unpopulated/uninitialized submodule as I call it).\n> \n> And in my current understanding of submodules the check in\n> .gitmodules ought to be enough, too.\n\nYeah, that probably makes sense. You can have a gitlink without a\n.gitmodules file, but I don't quite know what that would mean in terms\nof submodules (I guess it's not a submodule but \"something else\").\n\n-Peff\n"},{"id":"309855","messageId":"CAGZ79kYSW7i9kyT5Nd2QuiRF+=oWz+6U6VGpAQhfspUHMjWnpw@mail.gmail.com","threadId":"44917","inReplyTo":"20170120200041.hefg44stddqe344z@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] Disallow commands from within unpopulated submodules.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-20T20:07:28Z","receivedAt":"2017-01-20T20:07:49Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jan 20, 2017 at 12:00 PM, Jeff King wrote:\n> On Fri, Jan 20, 2017 at 11:53:01AM -0800, Stefan Beller wrote:\n>\n>> > One alternative would be to make the check cheaper. Could we reliably\n>> > tell from the submodule.foo.* block in the config that path \"foo\" is a\n>> > submodule? I think that would work after \"submodule init\" but not right\n>> > after \"git clone\". So the index really is the source of truth there.\n>>\n>> Well we can check if there is a .gitmodules file that has a\n>> submodule.*.path equal to the last part of $CWD, no need to look\n>> at the git config.\n>>\n>> And that would also work right after git clone (in an\n>> unpopulated/uninitialized submodule as I call it).\n>>\n>> And in my current understanding of submodules the check in\n>> .gitmodules ought to be enough, too.\n>\n> Yeah, that probably makes sense. You can have a gitlink without a\n> .gitmodules file, but I don't quite know what that would mean in terms\n> of submodules (I guess it's not a submodule but \"something else\").\n\nyeah, I agree it could be git series[1] at work, or as you said\n\"something else\", and we have no idea what to do.\n\nI think this could actually be implemented top-down, because the\ncheck is cheap as we're beginning with lstat(.gitmodules), and no further\npursue checking this corner case in case the .gitmodules is not found.\n\nI'll see if I can make a patch that passes the test suite.\n\n[1] https://github.com/git-series/git-series/blob/master/INTERNALS.md\n\n>\n> -Peff\n"},{"id":"309858","messageId":"xmqq37gdxvbx.fsf@gitster.mtv.corp.google.com","threadId":"44917","inReplyTo":"20170120200041.hefg44stddqe344z@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] Disallow commands from within unpopulated submodules.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-20T21:41:54Z","receivedAt":"2017-01-20T21:42:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> And in my current understanding of submodules the check in\n>> .gitmodules ought to be enough, too.\n>\n> Yeah, that probably makes sense. You can have a gitlink without a\n> .gitmodules file, but I don't quite know what that would mean in terms\n> of submodules (I guess it's not a submodule but \"something else\").\n\nThat may be a lot better than reading the index unconditionally, but\nI'd rather not to see \"git rev-parse\" read \".gitmodules\" at all. It\nwould discourage scripted use of Git for no good reason.\n\n\n"},{"id":"309859","messageId":"20170120214812.hhyjj3ldhhbrohlw@sigill.intra.peff.net","threadId":"44917","inReplyTo":"xmqq37gdxvbx.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH] Disallow commands from within unpopulated submodules.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-20T21:48:13Z","receivedAt":"2017-01-20T21:50:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 20, 2017 at 01:41:54PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> >> And in my current understanding of submodules the check in\n> >> .gitmodules ought to be enough, too.\n> >\n> > Yeah, that probably makes sense. You can have a gitlink without a\n> > .gitmodules file, but I don't quite know what that would mean in terms\n> > of submodules (I guess it's not a submodule but \"something else\").\n> \n> That may be a lot better than reading the index unconditionally, but\n> I'd rather not to see \"git rev-parse\" read \".gitmodules\" at all. It\n> would discourage scripted use of Git for no good reason.\n\nWhy is that? Just because it makes rev-parse seem more bloated?\n\nI think Stefan's putting it into git.c is confusing the issue a bit.\nThis is fundamentally about figuring out which git repository we're in,\nand that procedure is the right place to put the check.\n\nIOW, when we call setup_git_repository() we are already walking up the\ntree and looking at .git/HEAD, .git/config, etc to see if we are in a\nvalid git repository. It doesn't seem unreasonable to me to make this\npart of that check. I.e.:\n\n - if we we walked up from the working tree (so we have a non-NULL\n prefix); and\n\n - if there is a .gitmodules file; and\n\n - if the .gitmodules file shows that we were inside what _should_ have\n been a submodule; then\n\n - complain and do not accept the outer repository as a valid repo.\n\nThat adds only an extra failed open() for people who do not use\nsubmodules, and an extra config-file parse for people who do. And then\nonly when they are not in the top-level of the working tree (so scripts,\netc that cd_to_toplevel wouldn't pay per-invocation).\n\n-Peff\n"},{"id":"309860","messageId":"xmqqy3y5wg0l.fsf@gitster.mtv.corp.google.com","threadId":"44917","inReplyTo":"xmqq37gdxvbx.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH] Disallow commands from within unpopulated submodules.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-20T21:58:02Z","receivedAt":"2017-01-20T21:59:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>>> And in my current understanding of submodules the check in\n>>> .gitmodules ought to be enough, too.\n>>\n>> Yeah, that probably makes sense. You can have a gitlink without a\n>> .gitmodules file, but I don't quite know what that would mean in terms\n>> of submodules (I guess it's not a submodule but \"something else\").\n>\n> That may be a lot better than reading the index unconditionally, but\n> I'd rather not to see \"git rev-parse\" read \".gitmodules\" at all. It\n> would discourage scripted use of Git for no good reason.\n\nThinking about this more, I suspect that\n\n\tcd sub && git anything\n\nwhen the index of the top-level thinks \"sub\" must be a submodule and\nthe user is not interested in \"sub\" (hence it hasn't gone through\n\"git submodule init\" or \"update\") should get the same error as you\nwould get if you did\n\n\tcd /var/tmp/ && git anything\n\nwhen none of /, /var, /var/tmp/ is controlled by any Git repository.\nI.e. \"fatal: Not a git repository\".\n\nPerhaps we can update two things and make it cheap.\n\n - checking out the top-level working tree without populating the\n working tree of a submodule learns to do a bit more than just\n creating an empty directory. Instead, it creates the third kind\n of \".git\" (we currently support two kinds of \".git\", one that is\n a repository itself, and another that is points at a repository),\n that tells us that there is not (yet) a repository there.\n\n - the \"discovering the root of the working tree\" logic learns to\n notice the third kind of \".git\" and stop with \"Not a git\n repository\".\n\n\n"},{"id":"309862","messageId":"20170120220114.fkf42kdcsmnqy6zo@sigill.intra.peff.net","threadId":"44917","inReplyTo":"xmqqy3y5wg0l.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH] Disallow commands from within unpopulated submodules.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-20T22:01:14Z","receivedAt":"2017-01-20T22:03:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 20, 2017 at 01:58:02PM -0800, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > Jeff King writes:\n> >\n> >>> And in my current understanding of submodules the check in\n> >>> .gitmodules ought to be enough, too.\n> >>\n> >> Yeah, that probably makes sense. You can have a gitlink without a\n> >> .gitmodules file, but I don't quite know what that would mean in terms\n> >> of submodules (I guess it's not a submodule but \"something else\").\n> >\n> > That may be a lot better than reading the index unconditionally, but\n> > I'd rather not to see \"git rev-parse\" read \".gitmodules\" at all. It\n> > would discourage scripted use of Git for no good reason.\n> \n> Thinking about this more, I suspect that\n> \n> \tcd sub && git anything\n> \n> when the index of the top-level thinks \"sub\" must be a submodule and\n> the user is not interested in \"sub\" (hence it hasn't gone through\n> \"git submodule init\" or \"update\") should get the same error as you\n> would get if you did\n> \n> \tcd /var/tmp/ && git anything\n> \n> when none of /, /var, /var/tmp/ is controlled by any Git repository.\n> I.e. \"fatal: Not a git repository\".\n\nNot sure if our emails just crossed, but yes, I agree completely.\n\n> Perhaps we can update two things and make it cheap.\n> \n> - checking out the top-level working tree without populating the\n> working tree of a submodule learns to do a bit more than just\n> creating an empty directory. Instead, it creates the third kind\n> of \".git\" (we currently support two kinds of \".git\", one that is\n> a repository itself, and another that is points at a repository),\n> that tells us that there is not (yet) a repository there.\n> \n> - the \"discovering the root of the working tree\" logic learns to\n> notice the third kind of \".git\" and stop with \"Not a git\n> repository\".\n\nYeah, I thought about suggesting something like that earlier. It's\nslightly more efficient than the \"find the root and then complain\" thing\nStefan and I were talking about, but I'd worry it comes with weird\ncorner cases. E.g., are there tools (or other parts of git) that care\nabout it literally being an empty directory? E.g., would parts of\n\"checkout\" need to know that it's OK to blow it away?\n"},{"id":"309873","messageId":"CAGZ79kYW1zS3-9AYPaiUfBGTFygyg1ZVd3YyOctp3gihfEpHeg@mail.gmail.com","threadId":"44917","inReplyTo":"xmqqy3y5wg0l.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH] Disallow commands from within unpopulated submodules.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-20T23:22:31Z","receivedAt":"2017-01-20T23:22:38Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jan 20, 2017 at 1:58 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Jeff King writes:\n>>\n>>>> And in my current understanding of submodules the check in\n>>>> .gitmodules ought to be enough, too.\n>>>\n>>> Yeah, that probably makes sense. You can have a gitlink without a\n>>> .gitmodules file, but I don't quite know what that would mean in terms\n>>> of submodules (I guess it's not a submodule but \"something else\").\n>>\n>> That may be a lot better than reading the index unconditionally, but\n>> I'd rather not to see \"git rev-parse\" read \".gitmodules\" at all. It\n>> would discourage scripted use of Git for no good reason.\n>\n> Thinking about this more, I suspect that\n>\n> cd sub && git anything\n>\n> when the index of the top-level thinks \"sub\" must be a submodule and\n> the user is not interested in \"sub\" (hence it hasn't gone through\n> \"git submodule init\" or \"update\") should get the same error as you\n> would get if you did\n>\n> cd /var/tmp/ && git anything\n>\n> when none of /, /var, /var/tmp/ is controlled by any Git repository.\n> I.e. \"fatal: Not a git repository\".\n\nI agree. The idea with a tombstone sounds great from a\nperformance perspective as you do not need to do extra work\nin the superproject at all, because any gitlink is detected early\nin the discovery.\n\nThe big BUT is however the following:\nHow do current users know if a submodule is e.g. populated?\n(From say a third party script). Most likely they use something like\n\n test -e ${sub}/.git\n\nas that just worked. So if we go with the tombstone idea, we\nmay break things. (i.e. the fictional third party script confirms any\nsubmodule to be there, but it is not)\n\nI do really like the idea though, so maybe we also need to provide\nsome submodule plumbing that we opine to be the \"correct\" way\nto see the submodules state[1] to make the transition easier for the\nscript writers?\n\n[1] c.f. submodule states in\nhttps://github.com/gitster/git/commit/e2b51b9df618ceeff7c4ec044e20f5ce9a87241e\n\n>\n> Perhaps we can update two things and make it cheap.\n>\n> - checking out the top-level working tree without populating the\n> working tree of a submodule learns to do a bit more than just\n> creating an empty directory. Instead, it creates the third kind\n> of \".git\" (we currently support two kinds of \".git\", one that is\n> a repository itself, and another that is points at a repository),\n> that tells us that there is not (yet) a repository there.\n>\n> - the \"discovering the root of the working tree\" logic learns to\n> notice the third kind of \".git\" and stop with \"Not a git\n> repository\".\n"},{"id":"309888","messageId":"CACsJy8DJpueeRqLSbGuY6gsMKeYd+Q0dUzFB4p_Sq-795MhG1A@mail.gmail.com","threadId":"44917","inReplyTo":"20170120191728.l3ne5tt5pwbmafjh@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] Disallow commands from within unpopulated submodules.","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-01-21T12:55:05Z","receivedAt":"2017-01-21T12:55:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jan 21, 2017 at 2:17 AM, Jeff King wrote:\n> On Thu, Jan 19, 2017 at 11:30:23AM -0800, Stefan Beller wrote:\n>\n>> Now let's ask the same question for \"git -C sub status .\" (which is a\n>> command that is only reading and not writing to the repository)\n>>\n>> 1) If the submodule is populated, the user clearly intended to know\n>> more about the submodules status\n>> 2) It is unclear if the user wanted to learn about the submodules state\n>> (So ideally: \"The submodule 'sub' is not initialized. To init ...\")\n>> or the status check should be applied to the superproject instead.\n>>\n>> Avoid the confusion in 2) as well and just error out for now. Later on\n>> we may want to add another flag to git.c to allow commands to be run\n>> inside unpopulated submodules and each command reacts appropriately.\n>\n> I like the general idea of catching commands in unpopulated submodules,\n> but I'm somewhat uncomfortable with putting an unconditional check into\n> git.c, for two reasons:\n>\n> 1. Reading the index can be expensive. You would not want \"git\n> rev-parse\" to incur this cost.\n>\n> 2. How does this interact with commands which do interact with the\n> index? Don't they expect to find the_index unpopulated?\n>\n> (I notice that it's effectively tied to RUN_SETUP, which is good.\n> But that also means that many commands, like \"diff\", won't get the\n> benefit. Not to mention non-builtins).\n>\n> I'd rather see it in the commands themselves. Especially given the\n> \"ideal\" in your status example, which requires command-specific\n> knowledge.\n\nI agree. It's already bad enough for pathspec code to peek into the\nindex, adding a hidden dependency between parse_pathspec() and\nread_cache(). And I still think parse_pathspec() is not the right\nplace to check submodule paths. Worktree should be checked as well, in\nthe case that the submodule is not yet registered in the index. The\nright place to do that is per-command, with their consent so to speak,\nbecause they may need to set things up (index, .git/config and stuff)\nproperly before explicitly doing this check.\n-- \nDuy\n"}]}