{"thread":{"id":"46369","subject":"[PATCH] submodule: use cheaper check for submodule pushes","startedAt":"2017-07-12T23:45:17Z","lastAt":"2017-08-16T16:36:05Z","messageCount":20,"participants":["Stefan Beller","Jonathan Nieder","Junio C Hamano","Heiko Voigt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"324325","messageId":"20170712234504.15811-1-sbeller@google.com","threadId":"46369","inReplyTo":null,"subject":"[PATCH] submodule: use cheaper check for submodule pushes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-12T23:45:04Z","receivedAt":"2017-07-12T23:45:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In the function push_submodule[1] we use add_submodule_odb[2] to determine\nif a submodule has been populated. However the function does not work with\nthe submodules objects that are added, instead a new child process is used\nto perform the actual push in the submodule.\n\nUse is_submodule_populated[3] that is cheaper to guard from unpopulated\nsubmodules.\n\n[1] 'push_submodule' was added in eb21c732d6 (push: teach\n --recurse-submodules the on-demand option, 2012-03-29)\n[2] 'add_submodule_odb' was introduced in 752c0c2492 (Add the\n --submodule option to the diff option family, 2009-10-19)\n[3] 'is_submodule_populated' was added in 5688c28d81 (submodules:\n add helper to determine if a submodule is populated, 2016-12-16)\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex da2b484879..55afad3e8c 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -976,7 +976,9 @@ static int push_submodule(const char *path,\n \t\t\t const struct string_list *push_options,\n \t\t\t int dry_run)\n {\n-\tif (add_submodule_odb(path))\n+\tint code;\n+\n+\tif (!is_submodule_populated_gently(path, &code))\n \t\treturn 1;\n \n \tif (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {\n-- \n2.13.2.695.g117ddefdb4\n\n"},{"id":"324327","messageId":"20170713000117.GJ93855@aiede.mtv.corp.google.com","threadId":"46369","inReplyTo":"20170712234504.15811-1-sbeller@google.com","subject":"Re: [PATCH] submodule: use cheaper check for submodule pushes","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-07-13T00:01:17Z","receivedAt":"2017-07-13T00:01:33Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n> In the function push_submodule[1] we use add_submodule_odb[2] to determine\n> if a submodule has been populated. However the function does not work with\n> the submodules objects that are added, instead a new child process is used\n> to perform the actual push in the submodule.\n>\n> Use is_submodule_populated[3] that is cheaper to guard from unpopulated\n> submodules.\n>\n> [1] 'push_submodule' was added in eb21c732d6 (push: teach\n> --recurse-submodules the on-demand option, 2012-03-29)\n> [2] 'add_submodule_odb' was introduced in 752c0c2492 (Add the\n> --submodule option to the diff option family, 2009-10-19)\n> [3] 'is_submodule_populated' was added in 5688c28d81 (submodules:\n> add helper to determine if a submodule is populated, 2016-12-16)\n\nThese footnotes don't answer the question that I really have: why did\nthis use add_submodule_odb in the first place?\n\nE.g. did the ref iteration code require access to the object store\npreviously and stop requiring it later?\n\n> Signed-off-by: Stefan Beller \n> ---\n> submodule.c | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n> \n> diff --git a/submodule.c b/submodule.c\n> index da2b484879..55afad3e8c 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -976,7 +976,9 @@ static int push_submodule(const char *path,\n> \t\t\t const struct string_list *push_options,\n> \t\t\t int dry_run)\n> {\n> -\tif (add_submodule_odb(path))\n> +\tint code;\n> +\n> +\tif (!is_submodule_populated_gently(path, &code))\n\nShould this examine the code to distinguish between hard errors\n(e.g. \"Error reading .git\") and a missing repository?\n\nThanks,\nJonathan\n"},{"id":"324328","messageId":"CAGZ79kZMVXR_S7z=uFohOYXAJrpj=hRFn=w--EWiEEvxF3KCkw@mail.gmail.com","threadId":"46369","inReplyTo":"20170713000117.GJ93855@aiede.mtv.corp.google.com","subject":"Re: [PATCH] submodule: use cheaper check for submodule pushes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-13T00:09:06Z","receivedAt":"2017-07-13T00:09:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Jul 12, 2017 at 5:01 PM, Jonathan Nieder wrote:\n\n> These footnotes don't answer the question that I really have: why did\n> this use add_submodule_odb in the first place?\n\noh, I forgot to put that down: presumably add_submodule_odb was\nused because it was available back then? Note the dates!\n\n> E.g. did the ref iteration code require access to the object store\n> previously and stop requiring it later?\n\nNo. See [1], it was overkill since the beginning of time.\n\n>> - if (add_submodule_odb(path))\n>> + int code;\n>> +\n>> + if (!is_submodule_populated_gently(path, &code))\n>\n> Should this examine the code to distinguish between hard errors\n> (e.g. \"Error reading .git\") and a missing repository?\n\nadd_submodule_odb does neither, so I think this is best kept without\nadditional checks.\n\nThanks!\n"},{"id":"324332","messageId":"xmqq60exyx5k.fsf@gitster.mtv.corp.google.com","threadId":"46369","inReplyTo":"20170713000117.GJ93855@aiede.mtv.corp.google.com","subject":"Re: [PATCH] submodule: use cheaper check for submodule pushes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-13T00:53:59Z","receivedAt":"2017-07-13T00:54:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n>> In the function push_submodule[1] we use add_submodule_odb[2] to determine\n>> if a submodule has been populated. However the function does not work with\n>> the submodules objects that are added, instead a new child process is used\n>> to perform the actual push in the submodule.\n>>\n>> Use is_submodule_populated[3] that is cheaper to guard from unpopulated\n>> submodules.\n>>\n>> [1] 'push_submodule' was added in eb21c732d6 (push: teach\n>> --recurse-submodules the on-demand option, 2012-03-29)\n>> [2] 'add_submodule_odb' was introduced in 752c0c2492 (Add the\n>> --submodule option to the diff option family, 2009-10-19)\n>> [3] 'is_submodule_populated' was added in 5688c28d81 (submodules:\n>> add helper to determine if a submodule is populated, 2016-12-16)\n>\n> These footnotes don't answer the question that I really have: why did\n> this use add_submodule_odb in the first place?\n>\n> E.g. did the ref iteration code require access to the object store\n> previously and stop requiring it later?\n\nYes, the most important question is if it is safe to lose the access\nto the object store of the submodule. It is an endgame we should\naim for to get rid of add_submodule_odb(), but does the rest of this\ncodepath not require objects in the submodule at all or do we still\nneed to change something to make it so?\n"},{"id":"324335","messageId":"CAGZ79kaHX-YCMv01T-QE=mYeymjTnwrpg9-bsOrCjg3NWEunDA@mail.gmail.com","threadId":"46369","inReplyTo":"xmqq60exyx5k.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule: use cheaper check for submodule pushes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-13T05:14:34Z","receivedAt":"2017-07-13T05:14:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Jul 12, 2017 at 5:53 PM, Junio C Hamano wrote:\n> Jonathan Nieder writes:\n>\n>>> In the function push_submodule[1] we use add_submodule_odb[2] to determine\n>>> if a submodule has been populated. However the function does not work with\n>>> the submodules objects that are added, instead a new child process is used\n>>> to perform the actual push in the submodule.\n>>>\n>>> Use is_submodule_populated[3] that is cheaper to guard from unpopulated\n>>> submodules.\n>>>\n>>> [1] 'push_submodule' was added in eb21c732d6 (push: teach\n>>> --recurse-submodules the on-demand option, 2012-03-29)\n>>> [2] 'add_submodule_odb' was introduced in 752c0c2492 (Add the\n>>> --submodule option to the diff option family, 2009-10-19)\n>>> [3] 'is_submodule_populated' was added in 5688c28d81 (submodules:\n>>> add helper to determine if a submodule is populated, 2016-12-16)\n>>\n>> These footnotes don't answer the question that I really have: why did\n>> this use add_submodule_odb in the first place?\n>>\n>> E.g. did the ref iteration code require access to the object store\n>> previously and stop requiring it later?\n>\n> Yes, the most important question is if it is safe to lose the access\n> to the object store of the submodule. It is an endgame we should\n> aim for to get rid of add_submodule_odb(), but does the rest of this\n> codepath not require objects in the submodule at all or do we still\n> need to change something to make it so?\n\nYes, as the code in the current form as well as in its first occurrence\nused the result of add_submodule_odb to determine if to spawn a child process.\n"},{"id":"324406","messageId":"xmqqa848xjxr.fsf@gitster.mtv.corp.google.com","threadId":"46369","inReplyTo":"CAGZ79kaHX-YCMv01T-QE=mYeymjTnwrpg9-bsOrCjg3NWEunDA@mail.gmail.com","subject":"Re: [PATCH] submodule: use cheaper check for submodule pushes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-13T18:37:04Z","receivedAt":"2017-07-13T18:37:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Wed, Jul 12, 2017 at 5:53 PM, Junio C Hamano wrote:\n>> Jonathan Nieder writes:\n>>\n>>>> In the function push_submodule[1] we use add_submodule_odb[2] to determine\n>>>> if a submodule has been populated. However the function does not work with\n>>>> the submodules objects that are added, instead a new child process is used\n>>>> to perform the actual push in the submodule.\n>>>>\n>>>> Use is_submodule_populated[3] that is cheaper to guard from unpopulated\n>>>> submodules.\n>>>>\n>>>> [1] 'push_submodule' was added in eb21c732d6 (push: teach\n>>>> --recurse-submodules the on-demand option, 2012-03-29)\n>>>> [2] 'add_submodule_odb' was introduced in 752c0c2492 (Add the\n>>>> --submodule option to the diff option family, 2009-10-19)\n>>>> [3] 'is_submodule_populated' was added in 5688c28d81 (submodules:\n>>>> add helper to determine if a submodule is populated, 2016-12-16)\n>>>\n>>> These footnotes don't answer the question that I really have: why did\n>>> this use add_submodule_odb in the first place?\n>>>\n>>> E.g. did the ref iteration code require access to the object store\n>>> previously and stop requiring it later?\n>>\n>> Yes, the most important question is if it is safe to lose the access\n>> to the object store of the submodule. It is an endgame we should\n>> aim for to get rid of add_submodule_odb(), but does the rest of this\n>> codepath not require objects in the submodule at all or do we still\n>> need to change something to make it so?\n>\n> Yes, as the code in the current form as well as in its first occurrence\n> used the result of add_submodule_odb to determine if to spawn a child process.\n\nThe original added so that the return value of the call can be used\nfor that, and the current code still uses the return value for that\npurpose.\n\nThat much is already known. \n\nI think Jonathan's question (which I concurred) is if we also ended\nup relying on the side effect of calling that function (i.e. being\nable to now find objects that are not in our repository but in the\nsubmodule's object store). By looking at the eb21c732d6, we can\ntell that the original didn't mean to and didn't add any code that\nrelies on the ability to be able to read from the submodule object\nstore. I am not sure if that is still true after 5 years (i.e. is\nthere any new code added in the meantime that made us depend on the\nability to read from submodule object store?).\n\nMy hunch (and hope) is that we are probably safe, but that is a lot\nweaker than \"yes this is a good change we want to apply\".\n\n"},{"id":"324425","messageId":"CAGZ79kYtRbHvfC6d=+eEXCceJam4wwo9XqvY752c_pg6kq90DA@mail.gmail.com","threadId":"46369","inReplyTo":"xmqqa848xjxr.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule: use cheaper check for submodule pushes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-13T19:39:20Z","receivedAt":"2017-07-13T19:39:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jul 13, 2017 at 11:37 AM, Junio C Hamano wrote:\n\n>\n> I think Jonathan's question (which I concurred) is if we also ended\n> up relying on the side effect of calling that function (i.e. being\n> able to now find objects that are not in our repository but in the\n> submodule's object store). By looking at the eb21c732d6, we can\n> tell that the original didn't mean to and didn't add any code that\n> relies on the ability to be able to read from the submodule object\n> store. I am not sure if that is still true after 5 years (i.e. is\n> there any new code added in the meantime that made us depend on the\n> ability to read from submodule object store?).\n\nYes we are safe, because the function itself only spawns a child process\n(not using any of the objects).\n\nIt's only caller push_unpushed_submodules also doesn't rely on objects\nloaded after calling push_submodule.\n\nThe caller of push_unpushed_submodules (transport.c, transport_push)\nalso doesn't need submodule objects loaded.\n\n> My hunch (and hope) is that we are probably safe, but that is a lot\n> weaker than \"yes this is a good change we want to apply\".\n\nGiven the above (I went through the code), all I can do is repeating\n\"yes this is a good change we want to apply\".\n\nThanks,\nStefan\n"},{"id":"324443","messageId":"20170713204854.GA158114@aiede.mtv.corp.google.com","threadId":"46369","inReplyTo":"CAGZ79kYtRbHvfC6d=+eEXCceJam4wwo9XqvY752c_pg6kq90DA@mail.gmail.com","subject":"Re: [PATCH] submodule: use cheaper check for submodule pushes","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-07-13T20:48:54Z","receivedAt":"2017-07-13T20:49:30Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n> Yes we are safe, because the function itself only spawns a child process\n> (not using any of the objects).\n>\n> It's only caller push_unpushed_submodules also doesn't rely on objects\n> loaded after calling push_submodule.\n>\n> The caller of push_unpushed_submodules (transport.c, transport_push)\n> also doesn't need submodule objects loaded.\n\nThanks for looking into it. This is what the commit message should\nsay to help reviewers or people trying to understand it later. The\nfootnotes don't help and are distracting, except that it makes sense\nto point out the original GSoC patch to say the alternate submodule\nodb wasn't needed even then.\n\nE.g.:\n\n Subject: push: do not add submodule odb as an alternate when recursing on demand\n\n \"git push --recurse-submodules=on-demand\" adds each submodule as an\n alternate with add_submodule_odb before checking whether the\n submodule has anything to push and pushing it if so.\n\n However, it never accesses any objects from the submodule. In the\n parent process it uses the submodule's ref database to see if there\n is anything to push. The actual push (which does rely on objects)\n occurs in a child process.\n\n The same was try when this call was originally added in\n v1.7.11-rc0~111^2 (push: teach --recurse-submodules the on-demand\n option, 2012-03-29). Most likely it was added by analogy with\n fetch --recurse-submodules=on-demand, which did use the submodule's\n object database.\n\n Use is_submodule_populated_gently instead, which is simpler and\n cheaper.\n\n[...]\n> On Thu, Jul 13, 2017 at 11:37 AM, Junio C Hamano wrote:\n\n>> My hunch (and hope) is that we are probably safe, but that is a lot\n>> weaker than \"yes this is a good change we want to apply\".\n>\n> Given the above (I went through the code), all I can do is repeating\n> \"yes this is a good change we want to apply\".\n\nWith such a commit message change, this seems like a reasonable change\nin principle (though I haven't looked carefully to verify it).\n\nMy one doubt is the is_submodule_populated_gently. Why are we using\nthat instead of simpler is_submodule_populated? The names and API\ncomments don't explain.\n\nThanks for your patient explanations,\nJonathan\n"},{"id":"324446","messageId":"CAGZ79kZouNBxOKr7X8j6wqebp3Wh3cDqhYR-t_PxaF7AwQ0Wzg@mail.gmail.com","threadId":"46369","inReplyTo":"20170713204854.GA158114@aiede.mtv.corp.google.com","subject":"Re: [PATCH] submodule: use cheaper check for submodule pushes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-13T20:54:04Z","receivedAt":"2017-07-13T20:54:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jul 13, 2017 at 1:48 PM, Jonathan Nieder wrote:\n> Hi,\n>\n> Stefan Beller wrote:\n>\n>> Yes we are safe, because the function itself only spawns a child process\n>> (not using any of the objects).\n>>\n>> It's only caller push_unpushed_submodules also doesn't rely on objects\n>> loaded after calling push_submodule.\n>>\n>> The caller of push_unpushed_submodules (transport.c, transport_push)\n>> also doesn't need submodule objects loaded.\n>\n> Thanks for looking into it. This is what the commit message should\n> say to help reviewers or people trying to understand it later. The\n> footnotes don't help and are distracting, except that it makes sense\n> to point out the original GSoC patch to say the alternate submodule\n> odb wasn't needed even then.\n>\n> E.g.:\n>\n> Subject: push: do not add submodule odb as an alternate when recursing on demand\n>\n> \"git push --recurse-submodules=on-demand\" adds each submodule as an\n> alternate with add_submodule_odb before checking whether the\n> submodule has anything to push and pushing it if so.\n>\n> However, it never accesses any objects from the submodule. In the\n> parent process it uses the submodule's ref database to see if there\n> is anything to push. The actual push (which does rely on objects)\n> occurs in a child process.\n>\n> The same was try when this call was originally added in\n> v1.7.11-rc0~111^2 (push: teach --recurse-submodules the on-demand\n> option, 2012-03-29). Most likely it was added by analogy with\n> fetch --recurse-submodules=on-demand, which did use the submodule's\n> object database.\n>\n> Use is_submodule_populated_gently instead, which is simpler and\n> cheaper.\n\nThanks for giving a good example of commit message that I could use\nin a reroll.\n\n\n> With such a commit message change, this seems like a reasonable change\n> in principle (though I haven't looked carefully to verify it).\n>\n> My one doubt is the is_submodule_populated_gently. Why are we using\n> that instead of simpler is_submodule_populated? The names and API\n> comments don't explain.\n\nOne could posit this is laziness of thinking.\nSee 15cdc64776 (make is_submodule_populated gently, 2017-03-14),\nand discover there is no non-gentle version of is_submodule_populated.\nAnd for each new use, it may be cheaper to just use the gentle version\ninstead of adding a non-gentle version.\n\nThanks,\nStefan\n"},{"id":"326464","messageId":"20170815224332.22730-1-sbeller@google.com","threadId":"46369","inReplyTo":"CAGZ79kZouNBxOKr7X8j6wqebp3Wh3cDqhYR-t_PxaF7AwQ0Wzg@mail.gmail.com","subject":"[PATCH] push: do not add submodule odb as an alternate when recursing on demand","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-15T22:43:32Z","receivedAt":"2017-08-15T22:43:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"\"git push --recurse-submodules=on-demand\" adds each submodule as an\nalternate with add_submodule_odb before checking whether the\nsubmodule has anything to push and pushing it if so.\n\nHowever, it never accesses any objects from the submodule. In the\nparent process it uses the submodule's ref database to see if there\nis anything to push. The actual push (which does rely on objects)\noccurs in a child process.\n\nThe same was true when this call was originally added in\nv1.7.11-rc0~111^2 (push: teach --recurse-submodules the on-demand\noption, 2012-03-29). Most likely it was added by analogy with\nfetch --recurse-submodules=on-demand, which did use the submodule's\nobject database.\n\nUse is_submodule_populated_gently instead, which is simpler and\ncheaper.\n\nSigned-off-by: Stefan Beller \n---\n\n Originally I intended to send this out as part of a larger series,\n but the series is getting too large series, sending all things in smaller\n units!\n \n Thanks,\n Stefan\n\n submodule.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 111a3007fc..e20216bc0f 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -966,7 +966,9 @@ static int push_submodule(const char *path,\n \t\t\t const struct string_list *push_options,\n \t\t\t int dry_run)\n {\n-\tif (add_submodule_odb(path))\n+\tint code;\n+\n+\tif (!is_submodule_populated_gently(path, &code))\n \t\treturn 1;\n \n \tif (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {\n-- \n2.14.0.rc0.3.g6c2e499285\n\n"},{"id":"326468","messageId":"20170815231038.GE13924@aiede.mtv.corp.google.com","threadId":"46369","inReplyTo":"20170815224332.22730-1-sbeller@google.com","subject":"Re: [PATCH] push: do not add submodule odb as an alternate when recursing on demand","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-08-15T23:10:38Z","receivedAt":"2017-08-15T23:10:50Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> \"git push --recurse-submodules=on-demand\" adds each submodule as an\n> alternate with add_submodule_odb before checking whether the\n> submodule has anything to push and pushing it if so.\n>\n> However, it never accesses any objects from the submodule. In the\n> parent process it uses the submodule's ref database to see if there\n> is anything to push. The actual push (which does rely on objects)\n> occurs in a child process.\n>\n> The same was true when this call was originally added in\n> v1.7.11-rc0~111^2 (push: teach --recurse-submodules the on-demand\n> option, 2012-03-29). Most likely it was added by analogy with\n> fetch --recurse-submodules=on-demand, which did use the submodule's\n> object database.\n>\n> Use is_submodule_populated_gently instead, which is simpler and\n> cheaper.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> submodule.c | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n\nReviewed-by: Jonathan Nieder \n\nThanks for picking up this loose end.\n\n> diff --git a/submodule.c b/submodule.c\n> index 111a3007fc..e20216bc0f 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -966,7 +966,9 @@ static int push_submodule(const char *path,\n> \t\t\t const struct string_list *push_options,\n> \t\t\t int dry_run)\n> {\n> -\tif (add_submodule_odb(path))\n> +\tint code;\n> +\n> +\tif (!is_submodule_populated_gently(path, &code))\n> \t\treturn 1;\n> \n> \tif (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {\n"},{"id":"326469","messageId":"20170815231414.GF13924@aiede.mtv.corp.google.com","threadId":"46369","inReplyTo":"20170815224332.22730-1-sbeller@google.com","subject":"Re: [PATCH] push: do not add submodule odb as an alternate when recursing on demand","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-08-15T23:14:14Z","receivedAt":"2017-08-15T23:14:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> Use is_submodule_populated_gently instead, which is simpler and\n> cheaper.\n[...]\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -966,7 +966,9 @@ static int push_submodule(const char *path,\n> \t\t\t const struct string_list *push_options,\n> \t\t\t int dry_run)\n> {\n> -\tif (add_submodule_odb(path))\n> +\tint code;\n> +\n> +\tif (!is_submodule_populated_gently(path, &code))\n> \t\treturn 1;\n\nAh, I forgot about this detail. I don't think it should block this\npatch (so my Reviewed-by still stands), but I wonder why this needs to\nbe gentle. add_submodule_odb is gentle so that is the conservative\nthing to do, but that doesn't mean it is the *right* thing to do.\n\nIf this passed NULL instead of &code as the second argument, would\nanything break?\n\nCould there be a comment explaining what kind of error we are\nexpecting and why it is okay to continue when that error is\nencountered without any error handling?\n\nThanks,\nJonathan\n"},{"id":"326470","messageId":"xmqqh8x8pg8p.fsf@gitster.mtv.corp.google.com","threadId":"46369","inReplyTo":"20170815224332.22730-1-sbeller@google.com","subject":"Re: [PATCH] push: do not add submodule odb as an alternate when recursing on demand","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-15T23:23:50Z","receivedAt":"2017-08-15T23:23:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> \"git push --recurse-submodules=on-demand\" adds each submodule as an\n> alternate with add_submodule_odb before checking whether the\n> submodule has anything to push and pushing it if so.\n>\n> However, it never accesses any objects from the submodule.\n> ...\n> Use is_submodule_populated_gently instead, which is simpler and\n> cheaper.\n>\n> Signed-off-by: Stefan Beller \n> ---\n>\n> Originally I intended to send this out as part of a larger series,\n> but the series is getting too large series, sending all things in smaller\n> units!\n\nThis vaguely reminds me that you sent something imilar perhaps for a\ndifferent codepath.\n\nIs \"is it populated\" a good thing to check here, though? IIRC,\nadd-submodule-odb allows you to add the object database of an\ninactivated submodule, so this seems to change the behaviour. I do\nnot know if the behaviour change is a good thing (i.e. bugfix) or\nnot (i.e. regression) offhand, though.\n\nThanks.\n\n> diff --git a/submodule.c b/submodule.c\n> index 111a3007fc..e20216bc0f 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -966,7 +966,9 @@ static int push_submodule(const char *path,\n> \t\t\t const struct string_list *push_options,\n> \t\t\t int dry_run)\n> {\n> -\tif (add_submodule_odb(path))\n> +\tint code;\n> +\n> +\tif (!is_submodule_populated_gently(path, &code))\n> \t\treturn 1;\n> \n> \tif (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {\n"},{"id":"326471","messageId":"CAGZ79kb2LJ=o0JhL77KY9aYgkQf+M_EuKhOP=UocJERAa4Gfxw@mail.gmail.com","threadId":"46369","inReplyTo":"20170815231414.GF13924@aiede.mtv.corp.google.com","subject":"Re: [PATCH] push: do not add submodule odb as an alternate when recursing on demand","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-15T23:27:20Z","receivedAt":"2017-08-15T23:27:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Aug 15, 2017 at 4:14 PM, Jonathan Nieder wrote:\n> Stefan Beller wrote:\n>\n>> Use is_submodule_populated_gently instead, which is simpler and\n>> cheaper.\n> [...]\n>> --- a/submodule.c\n>> +++ b/submodule.c\n>> @@ -966,7 +966,9 @@ static int push_submodule(const char *path,\n>> const struct string_list *push_options,\n>> int dry_run)\n>> {\n>> - if (add_submodule_odb(path))\n>> + int code;\n>> +\n>> + if (!is_submodule_populated_gently(path, &code))\n>> return 1;\n>\n> Ah, I forgot about this detail. I don't think it should block this\n> patch (so my Reviewed-by still stands), but I wonder why this needs to\n> be gentle. add_submodule_odb is gentle so that is the conservative\n> thing to do, but that doesn't mean it is the *right* thing to do.\n>\n> If this passed NULL instead of &code as the second argument, would\n> anything break?\n\npush_submodule, which is called by push_unpushed_submodules\njust issues warnings on submodule push error, (which happen all\nbefore the main push,) such that any submodule error does not prevent\nthe main push.\n\nWe could tighten that, but I would suggest another patch for that.\n\n> Could there be a comment explaining what kind of error we are\n> expecting and why it is okay to continue when that error is\n> encountered without any error handling?\n\nThe same as in add_submodule_odb, digging through the\nlogs. I up to now we did not care about the submodule succeeding\nas much, it was just an aid for the main repo push.\n\n>\n> Thanks,\n> Jonathan\n"},{"id":"326472","messageId":"CAGZ79kag+U94tzJ14mB4VZjSZ2MtUJ4vu4MXKLKkpkLw=2K_NA@mail.gmail.com","threadId":"46369","inReplyTo":"xmqqh8x8pg8p.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] push: do not add submodule odb as an alternate when recursing on demand","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-15T23:31:05Z","receivedAt":"2017-08-15T23:31:11Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Aug 15, 2017 at 4:23 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> \"git push --recurse-submodules=on-demand\" adds each submodule as an\n>> alternate with add_submodule_odb before checking whether the\n>> submodule has anything to push and pushing it if so.\n>>\n>> However, it never accesses any objects from the submodule.\n>> ...\n>> Use is_submodule_populated_gently instead, which is simpler and\n>> cheaper.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>>\n>> Originally I intended to send this out as part of a larger series,\n>> but the series is getting too large series, sending all things in smaller\n>> units!\n>\n> This vaguely reminds me that you sent something imilar perhaps for a\n> different codepath.\n\nhttps://public-inbox.org/git/xmqqh8xzq6td.fsf@gitster.mtv.corp.google.com/\n\n> Is \"is it populated\" a good thing to check here, though? IIRC,\n> add-submodule-odb allows you to add the object database of an\n> inactivated submodule, so this seems to change the behaviour. I do\n> not know if the behaviour change is a good thing (i.e. bugfix) or\n> not (i.e. regression) offhand, though.\n\nGood point, we should be able to push non-populated, even inactive(?)\nsubmodules. For that we strictly need add_submodule_odb here\n(or the repo object of the submodule, eventually).\n\nSo let's retract this patch for now.\n\nThanks.\n"},{"id":"326473","messageId":"xmqqd17wpe14.fsf@gitster.mtv.corp.google.com","threadId":"46369","inReplyTo":"CAGZ79kag+U94tzJ14mB4VZjSZ2MtUJ4vu4MXKLKkpkLw=2K_NA@mail.gmail.com","subject":"Re: [PATCH] push: do not add submodule odb as an alternate when recursing on demand","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-16T00:11:35Z","receivedAt":"2017-08-16T00:11:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> Is \"is it populated\" a good thing to check here, though? IIRC,\n>> add-submodule-odb allows you to add the object database of an\n>> inactivated submodule, so this seems to change the behaviour. I do\n>> not know if the behaviour change is a good thing (i.e. bugfix) or\n>> not (i.e. regression) offhand, though.\n>\n> Good point, we should be able to push non-populated, even inactive(?)\n> submodules. For that we strictly need add_submodule_odb here\n> (or the repo object of the submodule, eventually).\n>\n> So let's retract this patch for now.\n\nNot so fast. \n\nI am not convinced \"push --recursive\" should touch a submodule that\nwas once cloned from the upstream and then deactivated, so using\nadd-submodule-odb to decide if the push should go through may be a\nbug that we may want to fix, in which case the diff of the patch in\nquestion may be good as-is. We need to sell it as a bugfix to the\nusers, who may complain about behaviour change (if there is one).\n\nOn the other hand, even if it were desirable for such a deactivated\nsubmodule to be pushed, as your log message explained, there is no\nreason to contaminate the in-core object hash by calling the\nadd-submodule-odb helper, when the only thing we care about is \"do\nwe have the refs and object store for this submodule? we do not care\nif it is activated or not\". Perhaps there is a more appropriate\nhelper in submodule.c that answers that question that we should be\nusing instead of add-submodule-odb, and if there is not yet such a\nhelper, perhaps this indicates that we need to add such a helper,\nwhich essentially is the early half of what add-submodule-odb does,\ni.e. ask git_path_submodule() for the object store and check if that\ndirectory exists.\n\nThanks.\n"},{"id":"326478","messageId":"CAGZ79kZAkdiKHUweQK6U4dqAakfzReDMfiHKDmzsKGuAH3BgyA@mail.gmail.com","threadId":"46369","inReplyTo":"xmqqd17wpe14.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] push: do not add submodule odb as an alternate when recursing on demand","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-16T01:05:25Z","receivedAt":"2017-08-16T01:05:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Aug 15, 2017 at 5:11 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>>> Is \"is it populated\" a good thing to check here, though? IIRC,\n>>> add-submodule-odb allows you to add the object database of an\n>>> inactivated submodule, so this seems to change the behaviour. I do\n>>> not know if the behaviour change is a good thing (i.e. bugfix) or\n>>> not (i.e. regression) offhand, though.\n>>\n>> Good point, we should be able to push non-populated, even inactive(?)\n>> submodules. For that we strictly need add_submodule_odb here\n>> (or the repo object of the submodule, eventually).\n>>\n>> So let's retract this patch for now.\n>\n> Not so fast.\n\nOk, I took another look at the code.\n\nWhile we may desire that un-populated submodules can be pushed\n(due to checking out another revision where the submodule\ndoesn't exist, before pushing), this is not supported currently, because\nthe call to run the push in the submodule assumes there is a\n\"/.git\" on which the child process can operate.\nSo for now we HAVE to have the submodule populated.\n\n In the future we may have the a lighter version just checking the\n object store of the submodule. Maybe this use case in the submodule\n can be covered by the .remote/.imported mechanism as well, such\n that we'd know if we have any local objects.\n\n> I am not convinced \"push --recursive\" should touch a submodule that\n> was once cloned from the upstream and then deactivated, so using\n> add-submodule-odb to decide if the push should go through may be a\n> bug that we may want to fix, in which case the diff of the patch in\n> question may be good as-is. We need to sell it as a bugfix to the\n> users, who may complain about behaviour change (if there is one).\n\ncorrect. Coupled with the observation above, we want really need\nboth \"is active and populated\" (your paragraph suggest not-active\nsubmodules don't need pushing, the populated bit comes from the\nobservation above)\n\n> On the other hand, even if it were desirable for such a deactivated\n> submodule to be pushed, as your log message explained, there is no\n> reason to contaminate the in-core object hash by calling the\n> add-submodule-odb helper, when the only thing we care about is \"do\n> we have the refs and object store for this submodule? we do not care\n> if it is activated or not\". Perhaps there is a more appropriate\n> helper in submodule.c that answers that question that we should be\n> using instead of add-submodule-odb, and if there is not yet such a\n> helper, perhaps this indicates that we need to add such a helper,\n> which essentially is the early half of what add-submodule-odb does,\n> i.e. ask git_path_submodule() for the object store and check if that\n> directory exists.\n\nI think due to the constraint of also needing a worktree the is_populated\nhelper is the correct choice here.\n\n\n>\n> Thanks.\n"},{"id":"326484","messageId":"20170816020809.GI13924@aiede.mtv.corp.google.com","threadId":"46369","inReplyTo":"CAGZ79kZAkdiKHUweQK6U4dqAakfzReDMfiHKDmzsKGuAH3BgyA@mail.gmail.com","subject":"Re: [PATCH] push: do not add submodule odb as an alternate when recursing on demand","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-08-16T02:08:09Z","receivedAt":"2017-08-16T02:08:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n> On Tue, Aug 15, 2017 at 5:11 PM, Junio C Hamano wrote:\n>> Stefan Beller writes:\n>>> Junio C Hamano wrote:\n\n>>>> Is \"is it populated\" a good thing to check here, though? IIRC,\n>>>> add-submodule-odb allows you to add the object database of an\n>>>> inactivated submodule, so this seems to change the behaviour. I do\n>>>> not know if the behaviour change is a good thing (i.e. bugfix) or\n>>>> not (i.e. regression) offhand, though.\n>>>\n>>> Good point, we should be able to push non-populated, even inactive(?)\n>>> submodules. For that we strictly need add_submodule_odb here\n>>> (or the repo object of the submodule, eventually).\n>>>\n>>> So let's retract this patch for now.\n>>\n>> Not so fast.\n>\n> Ok, I took another look at the code.\n>\n> While we may desire that un-populated submodules can be pushed\n> (due to checking out another revision where the submodule\n> doesn't exist, before pushing), this is not supported currently, because\n> the call to run the push in the submodule assumes there is a\n> \"/.git\" on which the child process can operate.\n> So for now we HAVE to have the submodule populated.\n\nIt was not immediately obvious to me that this is just \"for now\".\n\nI would be really confused if I had deactivated a submodule and\n\"git push --recurse-submodules\" pushed from it anyway. If the\nsubmodule is active but not populated, then the question becomes \"Why\nwasn't it populated?\"\n\nIf this is a bare repository, then nothing is populated, and pushing\nfrom an active-but-unpopulated submodule sounds like a plausible wish.\nBut in a non-bare repository, I'm having trouble imagining the use\ncase that brings this situation about.\n\nAnd where people have been needing this so far has been non-bare\nrepositories. In that context, the check \"is active and populated\"\ndoes not seem unusual or provisional. Are you hinting that replacing\nthe check with \"is active\" would make it work well in bare\nrepositories? I think I agree, though you'd have to be careful about\nthe case where the submodule is active but hasn't been fetched to\n$GIT_DIR/modules yet.\n\nThanks,\nJonathan\n"},{"id":"326487","messageId":"CAGZ79kaUgPmoPw7XGY6qxc64_5DygfX4xM5noX4E5EW6=Ejm+Q@mail.gmail.com","threadId":"46369","inReplyTo":"20170816020809.GI13924@aiede.mtv.corp.google.com","subject":"Re: [PATCH] push: do not add submodule odb as an alternate when recursing on demand","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-16T05:52:18Z","receivedAt":"2017-08-16T05:52:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Aug 15, 2017 at 7:08 PM, Jonathan Nieder wrote:\n> Hi,\n>\n> Stefan Beller wrote:\n>> On Tue, Aug 15, 2017 at 5:11 PM, Junio C Hamano wrote:\n>>> Stefan Beller writes:\n>>>> Junio C Hamano wrote:\n>\n>>>>> Is \"is it populated\" a good thing to check here, though? IIRC,\n>>>>> add-submodule-odb allows you to add the object database of an\n>>>>> inactivated submodule, so this seems to change the behaviour. I do\n>>>>> not know if the behaviour change is a good thing (i.e. bugfix) or\n>>>>> not (i.e. regression) offhand, though.\n>>>>\n>>>> Good point, we should be able to push non-populated, even inactive(?)\n>>>> submodules. For that we strictly need add_submodule_odb here\n>>>> (or the repo object of the submodule, eventually).\n>>>>\n>>>> So let's retract this patch for now.\n>>>\n>>> Not so fast.\n>>\n>> Ok, I took another look at the code.\n>>\n>> While we may desire that un-populated submodules can be pushed\n>> (due to checking out another revision where the submodule\n>> doesn't exist, before pushing), this is not supported currently, because\n>> the call to run the push in the submodule assumes there is a\n>> \"/.git\" on which the child process can operate.\n>> So for now we HAVE to have the submodule populated.\n>\n> It was not immediately obvious to me that this is just \"for now\".\n>\n> I would be really confused if I had deactivated a submodule and\n> \"git push --recurse-submodules\" pushed from it anyway.\n\nagreed.\n\n> If the\n> submodule is active but not populated, then the question becomes \"Why\n> wasn't it populated?\"\n\nbecause you checked out a different revision where the sub is not\npart of the working tree, for example. As long as you have locally\ninteresting \"stuff\" you want to transfer it. (IMHO even when the sub\nis not active)\n\n> If this is a bare repository, then nothing is populated,\n\nCurrently bare repos do not support having submodules\nAT ALL, i.e. there is no modules/ directory\nwhere the submodules would have any valuable information.\ninstead we *only* have the .gitmodules files at e.g. HEAD.\n\n> and pushing\n> from an active-but-unpopulated submodule sounds like a plausible wish.\n\nit seems we're on the same page here.\n\n> But in a non-bare repository, I'm having trouble imagining the use\n> case that brings this situation about.\n\ngit checkout revision-with-sub\n# hack.. hack.. hack..\n\"git commit --recurse-submodules -m bugfix\"\n# ^ not yet implemented\ngit checkout revision-without-sub\n# Oh! I forgot to push the bugfix\ngit push --recurse-submodule \n\n> And where people have been needing this so far has been non-bare\n> repositories. In that context, the check \"is active and populated\"\n> does not seem unusual or provisional. Are you hinting that replacing\n> the check with \"is active\" would make it work well in bare\n> repositories?\n\nNo I was saying:\n* currently the code only works when \"is populated\"\n* currently \"is active\" seems not to be considered\n\n* in the future the code shall work even when not \"is populated\",\n (but submodule git dir is there at the very least)\n* we may want to discuss if we want to care about \"is active\"\n additionally\n\n> I think I agree, though you'd have to be careful about\n> the case where the submodule is active but hasn't been fetched to\n> $GIT_DIR/modules yet.\n\nWhen not fetched yet, there is no modules/ git dir and\nwe are sure there is no locally interesting information, so we\nwant to omit the push. (see above \"has git dir\" is the minimum\nrequirement, as opposed to current \"is populated\".\n\n> Thanks,\n> Jonathan\n"},{"id":"326511","messageId":"20170816163554.GA4683@book.hvoigt.net","threadId":"46369","inReplyTo":"CAGZ79kZAkdiKHUweQK6U4dqAakfzReDMfiHKDmzsKGuAH3BgyA@mail.gmail.com","subject":"Re: [PATCH] push: do not add submodule odb as an alternate when recursing on demand","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2017-08-16T16:35:54Z","receivedAt":"2017-08-16T16:36:05Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nwas about to write that we are maybe overly cautious here. Because the\ncurrent way a submodule ends up in the list to be pushed is through:\n\n find_unpushed_submodules()\n\nthat itself collects all changed submodules when submodule_needs_pushing() is\ntrue. In there we have this:\n\n if (!submodule_has_commits(path, commits))\n /*\n * NOTE: We do consider it safe to return \"no\" here. The\n * correct answer would be \"We do not know\" instead of\n * \"No push needed\", but it is quite hard to change\n * the submodule pointer without having the submodule\n * around. If a user did however change the submodules\n * without having the submodule around, this indicates\n * an expert who knows what they are doing or a\n * maintainer integrating work from other people. In\n * both cases it should be safe to skip this check.\n */\n return 0;\n\nSo if the check, whether a submodule has commits, fails for any reason it will\nnot end up in the list to be pushed.\n\nAs a side note: inside submodule_has_commits() there is an add_submodule_odb()\nfollowed by a process to really make sure that the commits are in the\nsubmodule.\n\nSo IMO at this point we can be sure that the *database* exists and this extra\ncheck could be dropped if we said that a caller to push_submodule() should make\nsure that the submodule exists. The current ones are doing it already (if I did\nnot miss anything).\n\nOn Tue, Aug 15, 2017 at 06:05:25PM -0700, Stefan Beller wrote:\n> On Tue, Aug 15, 2017 at 5:11 PM, Junio C Hamano wrote:\n> > Stefan Beller writes:\n> >\n> >>> Is \"is it populated\" a good thing to check here, though? IIRC,\n> >>> add-submodule-odb allows you to add the object database of an\n> >>> inactivated submodule, so this seems to change the behaviour. I do\n> >>> not know if the behaviour change is a good thing (i.e. bugfix) or\n> >>> not (i.e. regression) offhand, though.\n> >>\n> >> Good point, we should be able to push non-populated, even inactive(?)\n> >> submodules. For that we strictly need add_submodule_odb here\n> >> (or the repo object of the submodule, eventually).\n> >>\n> >> So let's retract this patch for now.\n> >\n> > Not so fast.\n> \n> Ok, I took another look at the code.\n> \n> While we may desire that un-populated submodules can be pushed\n> (due to checking out another revision where the submodule\n> doesn't exist, before pushing), this is not supported currently, because\n> the call to run the push in the submodule assumes there is a\n> \"/.git\" on which the child process can operate.\n> So for now we HAVE to have the submodule populated.\n\nThat is a good point though. In the current form of push_submodule() we need to\nhave a populated submodule. So IMO to check whether the submodule is actually\n*populated* instead of adding the odb is correct and a possible bug fix.\n\nCheers Heiko\n"}]}