{"thread":{"id":"43925","subject":"[PATCHv2] push: change submodule default to check","startedAt":"2016-08-24T17:57:48Z","lastAt":"2016-09-19T20:11:57Z","messageCount":25,"participants":["Stefan Beller","Junio C Hamano","Jeff King","Heiko Voigt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"300091","messageId":"20160824173017.24782-1-sbeller@google.com","threadId":"43925","inReplyTo":null,"subject":"[PATCHv2] push: change submodule default to check","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-24T17:30:17Z","receivedAt":"2016-08-24T17:57:48Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When working with submodules, it is easy to forget to push the submodules.\nThe setting 'check', which checks if any existing submodule is present on\nat least one remote of the submodule remotes, is designed to prevent this\nmistake.\n\nFlipping the default to check for submodules is safer than the current\ndefault of ignoring submodules while pushing.\n\nSigned-off-by: Stefan Beller \n---\n\nSlightly reworded commit message than in v1,\n(https://public-inbox.org/git/20160817204848.8983-1-sbeller@google.com/)\nThe patch itself is however the same.\n\nI just push it out now with a new commit message, such that we can easier\npick it up later for Git 3.0, when changes that change default make more sense.\n\nAs said in an earlier message, you could however also argue that this is\nfixing a bug in your workflow, so it might be worth fixing before 3.0\nas well. I dunno.\n\nThanks,\nStefan\n\n builtin/push.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 3bb9d6b..479150a 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -22,7 +22,7 @@ static int deleterefs;\n static const char *receivepack;\n static int verbosity;\n static int progress = -1;\n-static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n+static int recurse_submodules = RECURSE_SUBMODULES_CHECK;\n static enum transport_family family;\n \n static struct push_cas_option cas;\n-- \n2.10.0.rc1.1.g1ceb01a\n\n"},{"id":"300102","messageId":"xmqqshtuovvx.fsf@gitster.mtv.corp.google.com","threadId":"43925","inReplyTo":"20160824173017.24782-1-sbeller@google.com","subject":"Re: [PATCHv2] push: change submodule default to check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-24T18:38:10Z","receivedAt":"2016-08-24T18:45:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> When working with submodules, it is easy to forget to push the submodules.\n> The setting 'check', which checks if any existing submodule is present on\n> at least one remote of the submodule remotes, is designed to prevent this\n> mistake.\n>\n> Flipping the default to check for submodules is safer than the current\n> default of ignoring submodules while pushing.\n>\n> Signed-off-by: Stefan Beller \n> ---\n>\n> Slightly reworded commit message than in v1,\n> (https://public-inbox.org/git/20160817204848.8983-1-sbeller@google.com/)\n> The patch itself is however the same.\n>\n> I just push it out now with a new commit message, such that we can easier\n> pick it up later for Git 3.0, when changes that change default make more sense.\n>\n> As said in an earlier message, you could however also argue that this is\n> fixing a bug in your workflow, so it might be worth fixing before 3.0\n> as well. I dunno.\n\nWith this change suddenly landing on the version of Git a user just\nupdated, the only possible negative effect would be that somebody\nwho did not configure push.recurseSubmodules suddenly starts getting\nan error. What would the error message say?\n\nWhat I want you to think about and explain in the justification is\nif it gives the user enough information to decide the best course of\naction to adjust to the new world order, when the user's expectation\nhas been that the superproject gets pushed independent from its\nsubmodules. If the existing error message gives enough information,\nexplains why Git suddenly started refusing the push is generally a\ngood thing for the user, tells some minority users (in whose\nworkflow it is perfectly safe to push out the toplevel independently\nfrom the submodule) how to turn it back to the old behaviour clearly\nenough, then this single-liner may be good enough. Otherwise we may\nneed a new logic to allow us to see if recurse_submodules that is\nset to RECURSE_SUBMODULES_CHECK is because the user explicitly\nconfigured, or because the user did not have any configuration, and\nissue a different message in the latter case.\n\n\n\n> builtin/push.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/push.c b/builtin/push.c\n> index 3bb9d6b..479150a 100644\n> --- a/builtin/push.c\n> +++ b/builtin/push.c\n> @@ -22,7 +22,7 @@ static int deleterefs;\n> static const char *receivepack;\n> static int verbosity;\n> static int progress = -1;\n> -static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n> +static int recurse_submodules = RECURSE_SUBMODULES_CHECK;\n> static enum transport_family family;\n> \n> static struct push_cas_option cas;\n"},{"id":"300109","messageId":"xmqqh9aaot49.fsf@gitster.mtv.corp.google.com","threadId":"43925","inReplyTo":"20160824183112.ceekegpzavnbybxp@sigill.intra.peff.net","subject":"Re: [PATCHv2] push: change submodule default to check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-24T19:37:58Z","receivedAt":"2016-08-24T19:39:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\nThis seems to be dropped from the list, probably due to no \"To:\"\nheader in the original, which led to \"no\", \"To-header\" \"on\" and\n\"input <\" on YOUR recipient list, so I am quoting it in full without\ntrimming.\n\n> On Wed, Aug 24, 2016 at 10:30:17AM -0700, Stefan Beller wrote:\n>\n>> When working with submodules, it is easy to forget to push the submodules.\n>> The setting 'check', which checks if any existing submodule is present on\n>> at least one remote of the submodule remotes, is designed to prevent this\n>> mistake.\n>> \n>> Flipping the default to check for submodules is safer than the current\n>> default of ignoring submodules while pushing.\n>\n> It is safer, and that's good. But it's also slower, because it requires\n> an extra traversal of all of the pushed commits. And now people will\n> have to pay the price even if they are not using submodules at all.\n>\n> For instance, try this from a checkout of linux.git:\n>\n> for i in no check; do\n> \trm -rf dst.git\n> \tgit init --bare dst.git\n> \techo \"==> Pushing with submodules=$i\"\n> \ttime git push --recurse-submodules=$i dst.git HEAD\n> done\n>\n> The second case takes 30-40 seconds longer. This is a full push of\n> history, so it's an extreme case[1], but it's still rather unfortunate.\n>\n> Can we tie this default to some sign that submodules are actually in\n> use? I don't think the presence of .gitmodules is perfect (because you\n> might be in a bare repo, for example, and have just fetched some other\n> history you are relaying), but it may be a good compromise. I'm\n> envisioning something like \"--recurse-submodules=auto-check\" which\n> auto-detects common situations (e.g., presence of .gitmodules or\n> .git/modules checkouts) and enables \"check\", and then setting the\n> default to that in the long run.\n>\n> -Peff\n>\n> [1] Actually, there is another much worse case lurking there. Try:\n>\n> git push --recurse-submodules=check --mirror dst.git\n>\n> from the kernel. I didn't let it finish, but I'd estimate it would\n> take on the order of 5 hours. The problem is that push feeds each\n> updated ref tip to find_unpushed_submodules(), so we end up walking\n> over the same history over and over.\n>\n> I think it should feed all of the \"before\" and \"after\" ref tips it\n> proposes to update to a _single_ revision traversal.\n\nThat sounds massively ... broken. So before even thinking about\nflipping it to default, this needs to be fixed first.\n\n> I also notice that it uses \"--remote=...\", which is weird, because\n> push knows exactly what it proposes to update, which may be ahead of\n> where our refs/remotes/* cache is. Not to mention that we may be\n> pushing to a remote for which we do not keep tracking refs at all!\n>\n> So I'd actually suspect that with your patch, a bare URL like:\n>\n> git push https://github.com/peff/linux.git\n>\n> would do the full 40-second walk, even if I was only pushing up one\n> or two objects.\n"},{"id":"300118","messageId":"xmqqtwe9oo30.fsf@gitster.mtv.corp.google.com","threadId":"43925","inReplyTo":"xmqqh9aaot49.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv2] push: change submodule default to check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-24T21:26:43Z","receivedAt":"2016-08-24T21:26:51Z","isPatch":false,"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>> For instance, try this from a checkout of linux.git:\n>>\n>> for i in no check; do\n>> \trm -rf dst.git\n>> \tgit init --bare dst.git\n>> \techo \"==> Pushing with submodules=$i\"\n>> \ttime git push --recurse-submodules=$i dst.git HEAD\n>> done\n>>\n>> The second case takes 30-40 seconds longer. This is a full push of\n>> history, so it's an extreme case[1], but it's still rather unfortunate.\n\nThis actually bit me just now. github.com/gitster/git.git is\nmirror pushed, and it would seem to take forever, so I killed it,\nand flipped the configuration variable off. Which means the feature\nwon't ever get any testing from me in real life.\n\nPeople, git.git is not a large project in any sense of the word.\n\nWhy wasn't it discovered that \"push.recursesubmodules = check\" is\nUNUSABLE since it was introduced is simply beyond me.\n\nI am mad (which is unusual for me, isn't it? -- I've seen somebody\nelse saying \"I am mad\", but I do not think I ever said it here).\n"},{"id":"300124","messageId":"CAGZ79kYOBqQ0FF4J-+KbefSD8HRrUeMqpO27m_jprhm93aB+LA@mail.gmail.com","threadId":"43925","inReplyTo":"xmqqh9aaot49.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv2] push: change submodule default to check","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-24T22:37:29Z","receivedAt":"2016-08-24T22:37:37Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Aug 24, 2016 at 12:37 PM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n> This seems to be dropped from the list, probably due to no \"To:\"\n> header in the original, which led to \"no\", \"To-header\" \"on\" and\n> \"input <\" on YOUR recipient list, so I am quoting it in full without\n> trimming.\n>\n>> On Wed, Aug 24, 2016 at 10:30:17AM -0700, Stefan Beller wrote:\n>>\n>>> When working with submodules, it is easy to forget to push the submodules.\n>>> The setting 'check', which checks if any existing submodule is present on\n>>> at least one remote of the submodule remotes, is designed to prevent this\n>>> mistake.\n>>>\n>>> Flipping the default to check for submodules is safer than the current\n>>> default of ignoring submodules while pushing.\n>>\n>> It is safer, and that's good. But it's also slower, because it requires\n>> an extra traversal of all of the pushed commits. And now people will\n>> have to pay the price even if they are not using submodules at all.\n>>\n>> For instance, try this from a checkout of linux.git:\n>>\n>> for i in no check; do\n>> rm -rf dst.git\n>> git init --bare dst.git\n>> echo \"==> Pushing with submodules=$i\"\n>> time git push --recurse-submodules=$i dst.git HEAD\n>> done\n>>\n>> The second case takes 30-40 seconds longer. This is a full push of\n>> history, so it's an extreme case[1], but it's still rather unfortunate.\n>>\n>> Can we tie this default to some sign that submodules are actually in\n>> use? I don't think the presence of .gitmodules is perfect (because you\n>> might be in a bare repo, for example, and have just fetched some other\n>> history you are relaying), but it may be a good compromise. I'm\n>> envisioning something like \"--recurse-submodules=auto-check\" which\n>> auto-detects common situations (e.g., presence of .gitmodules or\n>> .git/modules checkouts) and enables \"check\", and then setting the\n>> default to that in the long run.\n>>\n>> -Peff\n>>\n>> [1] Actually, there is another much worse case lurking there. Try:\n>>\n>> git push --recurse-submodules=check --mirror dst.git\n>>\n>> from the kernel. I didn't let it finish, but I'd estimate it would\n>> take on the order of 5 hours. The problem is that push feeds each\n>> updated ref tip to find_unpushed_submodules(), so we end up walking\n>> over the same history over and over.\n>>\n>> I think it should feed all of the \"before\" and \"after\" ref tips it\n>> proposes to update to a _single_ revision traversal.\n>\n> That sounds massively ... broken. So before even thinking about\n> flipping it to default, this needs to be fixed first.\n>\n\nI agree. That sounds bad.\n\nHowever having the --auto-check feels like papering over the\nactual problem which to me sounds like a design problem.\nHowever this may be a viable short term solution.\n\nAbout the design issue:\nWe need to answer the question: Which submodule commits\nare referenced by a given set of superproject commits.\n\nThis question is advancing a very similar question that we'd\nhave to ask in git-gc. In gc we would end up without having to\nworry about a specific set, but rather the all reachable commits\nof the superproject are in the given set.\n\nSo we could solve two issues at the same time if we had a quick\nway to answer this question quickly.\n\nAnd for that I considered introducing a ref in the submodule\n(e.g.) refs/superproject/gc_protector, which records both the\nsuperprojects commit as well as the submodule commit\nin case the superproject changed the submodule pointer.\n\nI have some rough patches for this, but I did not consider sending\nthat to the list as the patches are still rough and not quite\nfully thought out on the design level.\n\n--\nActually screw this. After an office discussion with Jonathan (cc'd):\n\n1) We need to have an \"alternate refs namespace\", which contains\n secondary data (as this can be derived from the primary data with\n lots of compute). So maybe we need a file similar to the packed refs\n file, \"superproject-refs\" that behaves as if it is storing refs,\nbut that file\n is safe to delete as we know all its contents can be recreated.\n\n2) In this new alternate refs space, we can have\n refs/superproject/ in the submodule with the sha1 of the\n superproject that points to a commit which is a dirty merge of all\n submodule commits, that have no other parents.\n\ne.g.\nIn the superproject we might have a history of:\n\n A <- B <- C\n\nwith A being origin/master and C our local master.\n\nIn the submodule we have:\n\n D <- E <- F\n \\\n G\n\nF is the master branch in the submodule, G is a dangling commit.\n\nNow we could have the following git links:\n\n A -> D\n B -> G\n C -> F\n\nThen the refs/superproject/\nwould be a merge of F and G.\n\nWhen pushing in the superproject (A..C) we would need to make sure\nthe submodule is updated as well, i.e. we'd look at\nrefs/superproject/ to know we need to advance the remote\nsubmodule history to contain at least G and F.\n\nThen we can do a standard rev walk starting at G and F downwards until we\nhit a commit that is contained in remote/refs/*.\n\n> Why wasn't it discovered that \"push.recursesubmodules = check\" is\n> UNUSABLE since it was introduced is simply beyond me.\n\nMaybe it is not compatible with your workflow as you push only once a day?\nIn a centralized server model you rarely exceed a few commits on push time,\nbut\n $ git rev-list a829490...c08db5a |wc -l\n 23\n $ git rev-list f5df0f2...f3c0fa5 |wc -l\n 61\n $ git rev-list 8f73021...de36215 |wc -l\n 95\n\nand some new branches, so you pushed around 200 commits.\nI think this misbehaves strongly for typical maintainer work flows.\n\n> I am mad (which is unusual for me, isn't it? -- I've seen somebody\n> else saying \"I am mad\", but I do not think I ever said it here).\n\nPlease direct your anger at the design of submodules.\nThe assumption of the submodule being sort of 'independant' doesn't allow\nfor efficient data structures I would think. So maybe we need to teach the\nsubmodules about the existence and state of the superproject?\n\n/me ducks\n"},{"id":"300128","messageId":"20160824230115.jhmcr4r7wobj5ejb@sigill.intra.peff.net","threadId":"43925","inReplyTo":"CAGZ79kYOBqQ0FF4J-+KbefSD8HRrUeMqpO27m_jprhm93aB+LA@mail.gmail.com","subject":"Re: [PATCHv2] push: change submodule default to check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-24T23:01:16Z","receivedAt":"2016-08-24T23:02:16Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 24, 2016 at 03:37:29PM -0700, Stefan Beller wrote:\n\n> > That sounds massively ... broken. So before even thinking about\n> > flipping it to default, this needs to be fixed first.\n> \n> I agree. That sounds bad.\n> \n> However having the --auto-check feels like papering over the\n> actual problem which to me sounds like a design problem.\n> However this may be a viable short term solution.\n\nSort of...\n\nI may not have been clear, but there are really a few things going on.\n\nOne is that the design of find_unpushed_submodules() is just brain-dead.\nIt does one traversal per updated ref, which means a from-scratch mirror\nis O(nr_of_refs * nr_of_commits). That's just silly, and can easily be\nfixed behind the scenes to be O(nr_of_commits).\n\nAnd I _suspect_ it is what made Junio's earlier push so awful; he\nprobably pushed up the same commits as part of many different branches,\nso he did the same diffs over and over.\n\nSo clearing that up seems like an obvious first step, and dulls the pain\nto \"if submodule recursion is on, the worst case is that you walk all\nthe new objects you are sending\". That's still _a_ traversal, but it's\none we have to do anyway in pack-objects, so it's the same order of\nmagnitude as the rest of the push[1].\n\nThen you've got two cases: the repo is using submodules at all, or they\nare not. The former is an easy case, if we can identify it; we can avoid\nthe traversal at all, and people who do not use submodules are not\nregressed at all.\n\nThat leaves people who _are_ using submodules with paying the extra\ntraversal cost. Not great, but only really a pain when you have a really\nbig chunk of history to push. It may be lost in the noise for such a\npush in more normal circumstances (where bandwidth to push up the\nobjects dominates, though it is unfortunate that we do not even start\nutilizing the bandwidth, the critical resource, until we are done with\nthe submodule check).\n\n[1] Of course with reachability bitmaps the pack-objects traversal goes\n away, but the same cannot be accomplished here (because they do not\n store the gitlink sha1s at all, because they do not imply\n reachability).\n\n> We need to answer the question: Which submodule commits\n> are referenced by a given set of superproject commits.\n> \n> This question is advancing a very similar question that we'd\n> have to ask in git-gc. In gc we would end up without having to\n> worry about a specific set, but rather the all reachable commits\n> of the superproject are in the given set.\n> \n> So we could solve two issues at the same time if we had a quick\n> way to answer this question quickly.\n> [...]\n\nI snipped here because your solutions sound complicated (which isn't to\nsay they're wrong, but that I am not willing to give them a lot of\nthought at this time in the evening ;) ).\n\nOne opposite approach which appeals to me is not to remove the need for\nthe traversal, but to make it much faster. E.g., by storing commits in a\nform that can be traversed more quickly, and possibly keeping a bitmap\ncache of which paths are touched in each commit (I have posted patches\nto the list for the former, but have only been considering experimenting\nwith the latter).\n\nThat's _also_ complicated, but it applies to way more things. Including\nnormal log traversals, path-limiting for diffs, the \"counting\" traversal\ndone by pack-object, etc.\n\nAnd while it is complicated in some ways, it's conceptually simple at\nthe git data model layer. It's returning the same old answers about\ncommits and trees, just faster.\n\nAnyway, food for thought.\n\n-Peff\n"},{"id":"301914","messageId":"20160914173124.GA7613@sandbox","threadId":"43925","inReplyTo":"20160824230115.jhmcr4r7wobj5ejb@sigill.intra.peff.net","subject":"[PATCH 1/2] serialize collection of changed submodules","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-09-14T17:31:24Z","receivedAt":"2016-09-14T17:31:37Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"To check whether a submodule needs to be pushed we need to collect all\nchanged submodules. Lets collect them first and then execute the\npossibly expensive test whether certain revisions are already pushed\nonly once per submodule.\n\nThere is further potential for optimization since we can assemble one\ncommand and only issued that instead of one call for each remote ref in\nthe submodule.\n\nSigned-off-by: Heiko Voigt \n---\nSorry about the late reply. I was not able to process emails until now.\nHere are two patches that should help to improve the situation and batch\nup some processing. This one is for repositories with submodules, so\nthat they do not iterate over the same submodule twice with the same\nhash.\n\nThe second one will be the one people without submodules are interested\nin.\n\nCheers Heiko\n\n submodule.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----\n 1 file changed, 62 insertions(+), 5 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 0ef2ff4..b04c066 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -554,19 +554,38 @@ static int submodule_needs_pushing(const char *path, const unsigned char sha1[20\n \treturn 0;\n }\n \n+static struct sha1_array *get_sha1s_from_list(struct string_list *submodules,\n+\t\tconst char *path)\n+{\n+\tstruct string_list_item *item;\n+\tstruct sha1_array *hashes;\n+\n+\titem = string_list_insert(submodules, path);\n+\tif (item->util)\n+\t\treturn (struct sha1_array *) item->util;\n+\n+\thashes = (struct sha1_array *) xmalloc(sizeof(struct sha1_array));\n+\t/* NEEDSWORK: should we add an initializer function for\n+\t * sha1_array ? */\n+\tmemset(hashes, 0, sizeof(struct sha1_array));\n+\titem->util = hashes;\n+\treturn hashes;\n+}\n+\n static void collect_submodules_from_diff(struct diff_queue_struct *q,\n \t\t\t\t\t struct diff_options *options,\n \t\t\t\t\t void *data)\n {\n \tint i;\n-\tstruct string_list *needs_pushing = data;\n+\tstruct string_list *submodules = data;\n \n \tfor (i = 0; i < q->nr; i++) {\n \t\tstruct diff_filepair *p = q->queue[i];\n+\t\tstruct sha1_array *hashes;\n \t\tif (!S_ISGITLINK(p->two->mode))\n \t\t\tcontinue;\n-\t\tif (submodule_needs_pushing(p->two->path, p->two->oid.hash))\n-\t\t\tstring_list_insert(needs_pushing, p->two->path);\n+\t\thashes = get_sha1s_from_list(submodules, p->two->path);\n+\t\tsha1_array_append(hashes, p->two->oid.hash);\n \t}\n }\n \n@@ -582,14 +601,41 @@ static void find_unpushed_submodule_commits(struct commit *commit,\n \tdiff_tree_combined_merge(commit, 1, &rev);\n }\n \n+struct collect_submodule_from_sha1s_data {\n+\tchar *submodule_path;\n+\tstruct string_list *needs_pushing;\n+};\n+\n+static void collect_submodules_from_sha1s(const unsigned char sha1[20],\n+\t\tvoid *data)\n+{\n+\tstruct collect_submodule_from_sha1s_data *me =\n+\t\t(struct collect_submodule_from_sha1s_data *) data;\n+\n+\tif (submodule_needs_pushing(me->submodule_path, sha1))\n+\t\tstring_list_insert(me->needs_pushing, me->submodule_path);\n+}\n+\n+static void free_submodules_sha1s(struct string_list *submodules)\n+{\n+\tint i;\n+\tfor (i = 0; i < submodules->nr; i++) {\n+\t\tstruct string_list_item *item = &submodules->items[i];\n+\t\tstruct sha1_array *hashes = (struct sha1_array *) item->util;\n+\t\tsha1_array_clear(hashes);\n+\t}\n+\tstring_list_clear(submodules, 1);\n+}\n+\n int find_unpushed_submodules(unsigned char new_sha1[20],\n \t\tconst char *remotes_name, struct string_list *needs_pushing)\n {\n \tstruct rev_info rev;\n \tstruct commit *commit;\n \tconst char *argv[] = {NULL, NULL, \"--not\", \"NULL\", NULL};\n-\tint argc = ARRAY_SIZE(argv) - 1;\n+\tint argc = ARRAY_SIZE(argv) - 1, i;\n \tchar *sha1_copy;\n+\tstruct string_list submodules = STRING_LIST_INIT_DUP;\n \n \tstruct strbuf remotes_arg = STRBUF_INIT;\n \n@@ -603,12 +649,23 @@ int find_unpushed_submodules(unsigned char new_sha1[20],\n \t\tdie(\"revision walk setup failed\");\n \n \twhile ((commit = get_revision(&rev)) != NULL)\n-\t\tfind_unpushed_submodule_commits(commit, needs_pushing);\n+\t\tfind_unpushed_submodule_commits(commit, &submodules);\n \n \treset_revision_walk();\n \tfree(sha1_copy);\n \tstrbuf_release(&remotes_arg);\n \n+\tfor (i = 0; i < submodules.nr; i++) {\n+\t\tstruct string_list_item *item = &submodules.items[i];\n+\t\tstruct collect_submodule_from_sha1s_data data;\n+\t\tdata.submodule_path = item->string;\n+\t\tdata.needs_pushing = needs_pushing;\n+\t\tsha1_array_for_each_unique((struct sha1_array *) item->util,\n+\t\t\t\tcollect_submodules_from_sha1s,\n+\t\t\t\t&data);\n+\t}\n+\tfree_submodules_sha1s(&submodules);\n+\n \treturn needs_pushing->nr;\n }\n \n-- \n2.0.2.832.g083c931\n\n"},{"id":"301916","messageId":"20160914175130.GB7613@sandbox","threadId":"43925","inReplyTo":"20160824230115.jhmcr4r7wobj5ejb@sigill.intra.peff.net","subject":"[PATCH 2/2] serialize collection of refs that contain submodule changes","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-09-14T17:51:30Z","receivedAt":"2016-09-14T17:52:08Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"We are iterating over each pushed ref and want to check whether it\ncontains changes to submodules. Instead of immediately checking each ref\nlets first collect them and then do the check for all of them in one\nrevision walk.\n\nSigned-off-by: Heiko Voigt \n---\n\nSorry this was not catched earlier. This was implemented as part of\nsummer of code and it seems we never tested with --mirror.\n\nThis is the one which does only one revision walk instead of one for\neach ref. Here are some numbers (using the my development clone of git\nitself) from my local machine:\n\nrm -rf && mkdir &&\n(cd && git init) &&\ntime git push --mirror \n\n real\t0m16.056s\n user\t0m24.424s\n sys\t0m1.380s\n\n real\t0m15.885s\n user\t0m24.204s\n sys\t0m1.296s\n\n real\t0m16.731s\n user\t0m24.176s\n sys\t0m1.244s\n\nrm -rf && mkdir &&\n(cd && git init) &&\ntime git push --mirror --recurse-submodules=check \n\n real\t0m21.441s\n user\t0m29.560s\n sys\t0m1.480s\n\n real\t0m21.319s\n user\t0m29.484s\n sys\t0m1.464s\n\n real\t0m21.261s\n user\t0m29.252s\n sys\t0m1.592s\n\nWithout my patches and --recurse-submodules=check the numbers are\nbasically the same. I stopped the test with --recurse-submodules=check\nafter ~ 9 minutes.\n\nCheers Heiko\n\n submodule.c | 36 +++++++++++++++++++++---------------\n submodule.h | 5 +++--\n transport.c | 22 ++++++++++++++--------\n 3 files changed, 38 insertions(+), 25 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex b04c066..a15e346 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -627,24 +627,31 @@ static void free_submodules_sha1s(struct string_list *submodules)\n \tstring_list_clear(submodules, 1);\n }\n \n-int find_unpushed_submodules(unsigned char new_sha1[20],\n+static void append_hash_to_argv(const unsigned char sha1[20],\n+\t\tvoid *data)\n+{\n+\tstruct argv_array *argv = (struct argv_array *) data;\n+\targv_array_push(argv, sha1_to_hex(sha1));\n+}\n+\n+int find_unpushed_submodules(struct sha1_array *hashes,\n \t\tconst char *remotes_name, struct string_list *needs_pushing)\n {\n \tstruct rev_info rev;\n \tstruct commit *commit;\n-\tconst char *argv[] = {NULL, NULL, \"--not\", \"NULL\", NULL};\n-\tint argc = ARRAY_SIZE(argv) - 1, i;\n-\tchar *sha1_copy;\n+\tint i;\n \tstruct string_list submodules = STRING_LIST_INIT_DUP;\n+\tstruct argv_array argv = ARGV_ARRAY_INIT;\n \n-\tstruct strbuf remotes_arg = STRBUF_INIT;\n-\n-\tstrbuf_addf(&remotes_arg, \"--remotes=%s\", remotes_name);\n \tinit_revisions(&rev, NULL);\n-\tsha1_copy = xstrdup(sha1_to_hex(new_sha1));\n-\targv[1] = sha1_copy;\n-\targv[3] = remotes_arg.buf;\n-\tsetup_revisions(argc, argv, &rev, NULL);\n+\n+\t/* argv.argv[0] will be ignored by setup_revisions */\n+\targv_array_push(&argv, \"find_unpushed_submodules\");\n+\tsha1_array_for_each_unique(hashes, append_hash_to_argv, &argv);\n+\targv_array_push(&argv, \"--not\");\n+\targv_array_pushf(&argv, \"--remotes=%s\", remotes_name);\n+\n+\tsetup_revisions(argv.argc, argv.argv, &rev, NULL);\n \tif (prepare_revision_walk(&rev))\n \t\tdie(\"revision walk setup failed\");\n \n@@ -652,8 +659,7 @@ int find_unpushed_submodules(unsigned char new_sha1[20],\n \t\tfind_unpushed_submodule_commits(commit, &submodules);\n \n \treset_revision_walk();\n-\tfree(sha1_copy);\n-\tstrbuf_release(&remotes_arg);\n+\targv_array_clear(&argv);\n \n \tfor (i = 0; i < submodules.nr; i++) {\n \t\tstruct string_list_item *item = &submodules.items[i];\n@@ -691,12 +697,12 @@ static int push_submodule(const char *path)\n \treturn 1;\n }\n \n-int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name)\n+int push_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name)\n {\n \tint i, ret = 1;\n \tstruct string_list needs_pushing = STRING_LIST_INIT_DUP;\n \n-\tif (!find_unpushed_submodules(new_sha1, remotes_name, &needs_pushing))\n+\tif (!find_unpushed_submodules(hashes, remotes_name, &needs_pushing))\n \t\treturn 1;\n \n \tfor (i = 0; i < needs_pushing.nr; i++) {\ndiff --git a/submodule.h b/submodule.h\nindex d9e197a..065b2f0 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -3,6 +3,7 @@\n \n struct diff_options;\n struct argv_array;\n+struct sha1_array;\n \n enum {\n \tRECURSE_SUBMODULES_CHECK = -4,\n@@ -62,9 +63,9 @@ int submodule_uses_gitfile(const char *path);\n int ok_to_remove_submodule(const char *path);\n int merge_submodule(unsigned char result[20], const char *path, const unsigned char base[20],\n \t\t const unsigned char a[20], const unsigned char b[20], int search);\n-int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name,\n+int find_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name,\n \t\tstruct string_list *needs_pushing);\n-int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);\n+int push_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name);\n void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);\n int parallel_submodules(void);\n \ndiff --git a/transport.c b/transport.c\nindex 94d6dc3..76e1daf 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -903,23 +903,29 @@ int transport_push(struct transport *transport,\n \n \t\tif ((flags & TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND) && !is_bare_repository()) {\n \t\t\tstruct ref *ref = remote_refs;\n+\t\t\tstruct sha1_array hashes = SHA1_ARRAY_INIT;\n+\n \t\t\tfor (; ref; ref = ref->next)\n-\t\t\t\tif (!is_null_oid(&ref->new_oid) &&\n-\t\t\t\t !push_unpushed_submodules(ref->new_oid.hash,\n-\t\t\t\t\t transport->remote->name))\n-\t\t\t\t die (\"Failed to push all needed submodules!\");\n+\t\t\t\tif (!is_null_oid(&ref->new_oid))\n+\t\t\t\t\tsha1_array_append(&hashes, ref->new_oid.hash);\n+\n+\t\t\tif (!push_unpushed_submodules(&hashes, transport->remote->name))\n+\t\t\t\tdie (\"Failed to push all needed submodules!\");\n \t\t}\n \n \t\tif ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n \t\t\t TRANSPORT_RECURSE_SUBMODULES_CHECK)) && !is_bare_repository()) {\n \t\t\tstruct ref *ref = remote_refs;\n \t\t\tstruct string_list needs_pushing = STRING_LIST_INIT_DUP;\n+\t\t\tstruct sha1_array hashes = SHA1_ARRAY_INIT;\n \n \t\t\tfor (; ref; ref = ref->next)\n-\t\t\t\tif (!is_null_oid(&ref->new_oid) &&\n-\t\t\t\t find_unpushed_submodules(ref->new_oid.hash,\n-\t\t\t\t\t transport->remote->name, &needs_pushing))\n-\t\t\t\t\tdie_with_unpushed_submodules(&needs_pushing);\n+\t\t\t\tif (!is_null_oid(&ref->new_oid))\n+\t\t\t\t\tsha1_array_append(&hashes, ref->new_oid.hash);\n+\n+\t\t\tif (find_unpushed_submodules(&hashes, transport->remote->name,\n+\t\t\t\t\t\t&needs_pushing))\n+\t\t\t\tdie_with_unpushed_submodules(&needs_pushing);\n \t\t}\n \n \t\tpush_ret = transport->push_refs(transport, remote_refs, flags);\n-- \n2.0.2.832.g083c931\n\n"},{"id":"301924","messageId":"20160914194643.GC7613@sandbox","threadId":"43925","inReplyTo":"20160914175130.GB7613@sandbox","subject":"Re: [PATCH 2/2] serialize collection of refs that contain submodule changes","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-09-14T19:46:43Z","receivedAt":"2016-09-14T19:56:22Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Wed, Sep 14, 2016 at 07:51:30PM +0200, Heiko Voigt wrote:\n> Here are some numbers (using the my development clone of git\n> itself) from my local machine:\n> \n> rm -rf && mkdir &&\n> (cd && git init) &&\n> time git push --mirror \n> \n> real\t0m16.056s\n> user\t0m24.424s\n> sys\t0m1.380s\n> \n> real\t0m15.885s\n> user\t0m24.204s\n> sys\t0m1.296s\n> \n> real\t0m16.731s\n> user\t0m24.176s\n> sys\t0m1.244s\n> \n> rm -rf && mkdir &&\n> (cd && git init) &&\n> time git push --mirror --recurse-submodules=check \n> \n> real\t0m21.441s\n> user\t0m29.560s\n> sys\t0m1.480s\n> \n> real\t0m21.319s\n> user\t0m29.484s\n> sys\t0m1.464s\n> \n> real\t0m21.261s\n> user\t0m29.252s\n> sys\t0m1.592s\n> \n> Without my patches and --recurse-submodules=check the numbers are\n> basically the same. I stopped the test with --recurse-submodules=check\n> after ~ 9 minutes.\n\nFun fact, I let the push without my patch and with\n--recurse-submodules=check finish:\n\nreal\t27m7.962s\nuser\t27m15.568s\nsys\t0m2.016s\n\nThats quite some time...\n\nCheers Heiko\n"},{"id":"301925","messageId":"CAGZ79kbFFwHLhCC=XA=g_hKAgwPrsrwCrmXvW7cRK38w4k48Vw@mail.gmail.com","threadId":"43925","inReplyTo":"20160914194643.GC7613@sandbox","subject":"Re: [PATCH 2/2] serialize collection of refs that contain submodule changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-14T20:04:25Z","receivedAt":"2016-09-14T20:04:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Sep 14, 2016 at 12:46 PM, Heiko Voigt wrote:\n> On Wed, Sep 14, 2016 at 07:51:30PM +0200, Heiko Voigt wrote:\n>> Here are some numbers (using the my development clone of git\n>> itself) from my local machine:\n>>\n>> rm -rf && mkdir &&\n>> (cd && git init) &&\n>> time git push --mirror \n>>\n>> real 0m16.056s\n>> user 0m24.424s\n>> sys 0m1.380s\n>>\n>> real 0m15.885s\n>> user 0m24.204s\n>> sys 0m1.296s\n>>\n>> real 0m16.731s\n>> user 0m24.176s\n>> sys 0m1.244s\n>>\n>> rm -rf && mkdir &&\n>> (cd && git init) &&\n>> time git push --mirror --recurse-submodules=check \n>>\n>> real 0m21.441s\n>> user 0m29.560s\n>> sys 0m1.480s\n>>\n>> real 0m21.319s\n>> user 0m29.484s\n>> sys 0m1.464s\n>>\n>> real 0m21.261s\n>> user 0m29.252s\n>> sys 0m1.592s\n>>\n>> Without my patches and --recurse-submodules=check the numbers are\n>> basically the same. I stopped the test with --recurse-submodules=check\n>> after ~ 9 minutes.\n>\n> Fun fact, I let the push without my patch and with\n> --recurse-submodules=check finish:\n\nThanks for the numbers, one of the major push backs for\norigin/sb/push-make-submodule-check-the-default\nwas that it introduced slowness; this patch might help a bit there.\n"},{"id":"301936","messageId":"xmqqwpiep10i.fsf@gitster.mtv.corp.google.com","threadId":"43925","inReplyTo":"20160914173124.GA7613@sandbox","subject":"Re: [PATCH 1/2] serialize collection of changed submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-14T22:30:53Z","receivedAt":"2016-09-14T22:31:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt writes:\n\n> Sorry about the late reply. I was not able to process emails until now.\n> Here are two patches that should help to improve the situation and batch\n> up some processing. This one is for repositories with submodules, so\n> that they do not iterate over the same submodule twice with the same\n> hash.\n>\n> The second one will be the one people without submodules are interested\n> in.\n\nThanks. Will take a look at later as I'm already deep in today's\nintegration cycle. Very much appreciated.\n"},{"id":"301959","messageId":"20160915121044.GA96648@book.hvoigt.net","threadId":"43925","inReplyTo":"xmqqwpiep10i.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 3/2] batch check whether submodule needs pushing into one call","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-09-15T12:10:44Z","receivedAt":"2016-09-15T12:10:59Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"We run a command for each sha1 change in a submodule. This is\nunnecessary since we can simply batch all sha1's we want to check into\none command. Lets do it so we can speedup the check when many submodule\nchanges are in need of checking.\n\nSigned-off-by: Heiko Voigt \n---\nOn Wed, Sep 14, 2016 at 03:30:53PM -0700, Junio C Hamano wrote:\n> Heiko Voigt writes:\n> \n> > Sorry about the late reply. I was not able to process emails until now.\n> > Here are two patches that should help to improve the situation and batch\n> > up some processing. This one is for repositories with submodules, so\n> > that they do not iterate over the same submodule twice with the same\n> > hash.\n> >\n> > The second one will be the one people without submodules are interested\n> > in.\n> \n> Thanks. Will take a look at later as I'm already deep in today's\n> integration cycle. Very much appreciated.\n\nNo problem. While I am at it: Here are actually another two patches that\nshould make life of submodule users easier (push times of big pushes).\n\nIn Numbers with the qt5[1] superproject and all submodules initialized.\nThe same --mirror test as before with the git repository:\n\n# Without patch:\n\nbook:qt5 hvoigt (5.6)$\nrm -rf ~/Downloads/git-test && mkdir ~/Downloads/git-test &&\n (cd ~/Downloads/git-test && git init) &&\n time git push --mirror --recurse-submodules=check ~/Downloads/git-test\n\nreal\t4m0.881s\nuser\t3m30.139s\nsys\t0m22.329s\n\nWithout --recurse-submodules=check\n\nreal\t0m0.251s\nuser\t0m0.218s\nsys\t0m0.082s\n\n\n# With patch:\n\nreal\t0m1.167s\nuser\t0m0.846s\nsys\t0m0.262s\n\nreal\t0m1.110s\nuser\t0m0.815s\nsys\t0m0.247s\n\nreal\t0m1.111s\nuser\t0m0.818s\nsys\t0m0.251s\n\nWithout --recurse-submodules=check\n\nreal\t0m0.294s\nuser\t0m0.221s\nsys\t0m0.104s\n\nreal\t0m0.248s\nuser\t0m0.216s\nsys\t0m0.080s\n\nreal\t0m0.247s\nuser\t0m0.212s\nsys\t0m0.082s\n\n[1] git://code.qt.io/qt/qt5.git\n\n submodule.c | 75 ++++++++++++++++++++++++++++++++-----------------------------\n 1 file changed, 39 insertions(+), 36 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex a15e346..28bb74e 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -522,27 +522,54 @@ static int has_remote(const char *refname, const struct object_id *oid,\n \treturn 1;\n }\n \n-static int submodule_needs_pushing(const char *path, const unsigned char sha1[20])\n+static void append_hash_to_argv(const unsigned char sha1[20], void *data)\n {\n-\tif (add_submodule_odb(path) || !lookup_commit_reference(sha1))\n+\tstruct argv_array *argv = (struct argv_array *) data;\n+\targv_array_push(argv, sha1_to_hex(sha1));\n+}\n+\n+static void check_has_hash(const unsigned char sha1[20], void *data)\n+{\n+\tint *has_hash = (int *) data;\n+\n+\tif (!lookup_commit_reference(sha1))\n+\t\t*has_hash = 0;\n+}\n+\n+static int submodule_has_hashes(const char *path, struct sha1_array *hashes)\n+{\n+\tint has_hash = 1;\n+\n+\tif (add_submodule_odb(path))\n+\t\treturn 0;\n+\n+\tsha1_array_for_each_unique(hashes, check_has_hash, &has_hash);\n+\treturn has_hash;\n+}\n+\n+static int submodule_needs_pushing(const char *path, struct sha1_array *hashes)\n+{\n+\tif (!submodule_has_hashes(path, hashes))\n \t\treturn 0;\n \n \tif (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\t\tconst char *argv[] = {\"rev-list\", NULL, \"--not\", \"--remotes\", \"-n\", \"1\" , NULL};\n+\n+\t\targv_array_push(&cp.args, \"rev-list\");\n+\t\tsha1_array_for_each_unique(hashes, append_hash_to_argv, &cp.args);\n+\t\targv_array_pushl(&cp.args, \"--not\", \"--remotes\", \"-n\", \"1\" , NULL);\n+\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tint needs_pushing = 0;\n \n-\t\targv[1] = sha1_to_hex(sha1);\n-\t\tcp.argv = argv;\n \t\tprepare_submodule_repo_env(&cp.env_array);\n \t\tcp.git_cmd = 1;\n \t\tcp.no_stdin = 1;\n \t\tcp.out = -1;\n \t\tcp.dir = path;\n \t\tif (start_command(&cp))\n-\t\t\tdie(\"Could not run 'git rev-list %s --not --remotes -n 1' command in submodule %s\",\n-\t\t\t\tsha1_to_hex(sha1), path);\n+\t\t\tdie(\"Could not run 'git rev-list --not --remotes -n 1' command in submodule %s\",\n+\t\t\t\t\tpath);\n \t\tif (strbuf_read(&buf, cp.out, 41))\n \t\t\tneeds_pushing = 1;\n \t\tfinish_command(&cp);\n@@ -601,21 +628,6 @@ static void find_unpushed_submodule_commits(struct commit *commit,\n \tdiff_tree_combined_merge(commit, 1, &rev);\n }\n \n-struct collect_submodule_from_sha1s_data {\n-\tchar *submodule_path;\n-\tstruct string_list *needs_pushing;\n-};\n-\n-static void collect_submodules_from_sha1s(const unsigned char sha1[20],\n-\t\tvoid *data)\n-{\n-\tstruct collect_submodule_from_sha1s_data *me =\n-\t\t(struct collect_submodule_from_sha1s_data *) data;\n-\n-\tif (submodule_needs_pushing(me->submodule_path, sha1))\n-\t\tstring_list_insert(me->needs_pushing, me->submodule_path);\n-}\n-\n static void free_submodules_sha1s(struct string_list *submodules)\n {\n \tint i;\n@@ -627,13 +639,6 @@ static void free_submodules_sha1s(struct string_list *submodules)\n \tstring_list_clear(submodules, 1);\n }\n \n-static void append_hash_to_argv(const unsigned char sha1[20],\n-\t\tvoid *data)\n-{\n-\tstruct argv_array *argv = (struct argv_array *) data;\n-\targv_array_push(argv, sha1_to_hex(sha1));\n-}\n-\n int find_unpushed_submodules(struct sha1_array *hashes,\n \t\tconst char *remotes_name, struct string_list *needs_pushing)\n {\n@@ -662,13 +667,11 @@ int find_unpushed_submodules(struct sha1_array *hashes,\n \targv_array_clear(&argv);\n \n \tfor (i = 0; i < submodules.nr; i++) {\n-\t\tstruct string_list_item *item = &submodules.items[i];\n-\t\tstruct collect_submodule_from_sha1s_data data;\n-\t\tdata.submodule_path = item->string;\n-\t\tdata.needs_pushing = needs_pushing;\n-\t\tsha1_array_for_each_unique((struct sha1_array *) item->util,\n-\t\t\t\tcollect_submodules_from_sha1s,\n-\t\t\t\t&data);\n+\t\tstruct string_list_item *submodule = &submodules.items[i];\n+\t\tstruct sha1_array *hashes = (struct sha1_array *) submodule->util;\n+\n+\t\tif (submodule_needs_pushing(submodule->string, hashes))\n+\t\t\tstring_list_insert(needs_pushing, submodule->string);\n \t}\n \tfree_submodules_sha1s(&submodules);\n \n-- \n2.10.0.133.g13017a3\n\n"},{"id":"301960","messageId":"20160915121858.GB96648@book.hvoigt.net","threadId":"43925","inReplyTo":"xmqqwpiep10i.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 4/2] use actual start hashes for submodule push check instead of local refs","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-09-15T12:18:58Z","receivedAt":"2016-09-15T12:19:11Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Push knows the actual revision range it is actually pushing to a remote.\nLet's use the start revisions to reduce the amount of checked revisions\ninstead of the locally cached remote refs which might be out of date.\n\nThis actually changes behavior as it now can also properly handle pushes\nwith URLs. That is also the reason why we change some tests. When\npassing the remote as URL the push is made unconditionally in on-demand.\nThis is wrong since on-demand should only push the submodule if its SHA1\nreference got changed in the superproject history that is getting pushed.\nThe reason behind that is that the exclusion list for revisions was\nreduced by using the parameters \"--not --remotes=\" to\nreduce the amount of revisions for the revision walk. In case of an URL\nthis does not match anything and thus we would always do a full revision\nwalk until the root commit.\n\nSigned-off-by: Heiko Voigt \n---\nAnd here is another one which makes the check more correct. For push\n--recurse-submodules=check|on-demand with URLs this is also a\nperformance improvement since at the moment we iterate over all\nrevisions unconditionally as explained above.\n\nThis patch changes the behavior (now its how its supposed to be) so\nthere may be fallout from users complaining that their submodules do not\nget pushed with on-demand anymore. This is only for users using URL for\npushing though.\n\nCheers Heiko\n\nBTW: Peff, thanks for the good diagnosis of all these problems.\n\n submodule.c | 12 ++++++------\n submodule.h | 6 +++---\n t/t5531-deep-submodule-push.sh | 24 ++++++++++++++++++------\n transport.c | 40 ++++++++++++++++++++++++++++++----------\n 4 files changed, 57 insertions(+), 25 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 28bb74e..1f82974 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -639,8 +639,8 @@ static void free_submodules_sha1s(struct string_list *submodules)\n \tstring_list_clear(submodules, 1);\n }\n \n-int find_unpushed_submodules(struct sha1_array *hashes,\n-\t\tconst char *remotes_name, struct string_list *needs_pushing)\n+int find_unpushed_submodules(struct sha1_array *old_hashes,\n+\t\tstruct sha1_array *new_hashes, struct string_list *needs_pushing)\n {\n \tstruct rev_info rev;\n \tstruct commit *commit;\n@@ -652,9 +652,9 @@ int find_unpushed_submodules(struct sha1_array *hashes,\n \n \t/* argv.argv[0] will be ignored by setup_revisions */\n \targv_array_push(&argv, \"find_unpushed_submodules\");\n-\tsha1_array_for_each_unique(hashes, append_hash_to_argv, &argv);\n+\tsha1_array_for_each_unique(new_hashes, append_hash_to_argv, &argv);\n \targv_array_push(&argv, \"--not\");\n-\targv_array_pushf(&argv, \"--remotes=%s\", remotes_name);\n+\tsha1_array_for_each_unique(old_hashes, append_hash_to_argv, &argv);\n \n \tsetup_revisions(argv.argc, argv.argv, &rev, NULL);\n \tif (prepare_revision_walk(&rev))\n@@ -700,12 +700,12 @@ static int push_submodule(const char *path)\n \treturn 1;\n }\n \n-int push_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name)\n+int push_unpushed_submodules(struct sha1_array *old_hashes, struct sha1_array *new_hashes)\n {\n \tint i, ret = 1;\n \tstruct string_list needs_pushing = STRING_LIST_INIT_DUP;\n \n-\tif (!find_unpushed_submodules(hashes, remotes_name, &needs_pushing))\n+\tif (!find_unpushed_submodules(old_hashes, new_hashes, &needs_pushing))\n \t\treturn 1;\n \n \tfor (i = 0; i < needs_pushing.nr; i++) {\ndiff --git a/submodule.h b/submodule.h\nindex 065b2f0..55c6c91 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -63,9 +63,9 @@ int submodule_uses_gitfile(const char *path);\n int ok_to_remove_submodule(const char *path);\n int merge_submodule(unsigned char result[20], const char *path, const unsigned char base[20],\n \t\t const unsigned char a[20], const unsigned char b[20], int search);\n-int find_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name,\n-\t\tstruct string_list *needs_pushing);\n-int push_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name);\n+int find_unpushed_submodules(struct sha1_array *old_hashes,\n+\t\tstruct sha1_array *new_hashes, struct string_list *needs_pushing);\n+int push_unpushed_submodules(struct sha1_array *old_hashes, struct sha1_array *new_hashes);\n void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);\n int parallel_submodules(void);\n \ndiff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh\nindex 198ce84..a5b8ef6 100755\n--- a/t/t5531-deep-submodule-push.sh\n+++ b/t/t5531-deep-submodule-push.sh\n@@ -154,6 +154,8 @@ test_expect_success 'push recurse-submodules on command line overrides config' '\n \t\tgit fetch ../pub.git &&\n \t\tgit diff --quiet FETCH_HEAD master &&\n \t\t(cd gar/bage && git diff --quiet origin/master master^) &&\n+\t\t# Since this push was executed reset to previous state\n+\t\t(git push -f ../pub.git master^:master) &&\n \n \t\t# Ensure that we can override check in the config to\n \t\t# disable submodule recursion entirely (alternative form)\n@@ -161,6 +163,8 @@ test_expect_success 'push recurse-submodules on command line overrides config' '\n \t\tgit fetch ../pub.git &&\n \t\tgit diff --quiet FETCH_HEAD master &&\n \t\t(cd gar/bage && git diff --quiet origin/master master^) &&\n+\t\t# Since this push was executed reset to previous state\n+\t\t(git push -f ../pub.git master^:master) &&\n \n \t\t# Ensure that we can override check in the config to\n \t\t# push the submodule too\n@@ -198,11 +202,15 @@ test_expect_success 'push recurse-submodules last one wins on command line' '\n \t\tgit diff --quiet FETCH_HEAD master &&\n \t\t# Check that the submodule commit did not get there\n \t\t(cd gar/bage && git diff --quiet origin/master master^) &&\n+\t\t# Since this push was executed reset to previous state\n+\t\t(git push -f ../pub.git master^:master) &&\n \n \t\t# should result in \"no\"\n \t\tgit push --recurse-submodules=on-demand --no-recurse-submodules ../pub.git master &&\n \t\t# Check that the submodule commit did not get there\n \t\t(cd gar/bage && git diff --quiet origin/master master^) &&\n+\t\t# Since this push was executed reset to previous state\n+\t\t(git push -f ../pub.git master^:master) &&\n \n \t\t# But the options in the other order should push the submodule\n \t\tgit push --recurse-submodules=check --recurse-submodules=on-demand ../pub.git master &&\n@@ -249,9 +257,11 @@ test_expect_success 'push succeeds if submodule commit disabling recursion from\n \t\tgit fetch ../pub.git &&\n \t\tgit diff --quiet FETCH_HEAD master &&\n \t\t# But that the submodule commit did not\n-\t\t( cd gar/bage && git diff --quiet origin/master master^ ) &&\n-\t\t# Now push it to avoid confusing future tests\n-\t\tgit push --recurse-submodules=on-demand ../pub.git master\n+\t\t(cd gar/bage &&\n+\t\t\tgit diff --quiet origin/master master^ &&\n+\t\t\t# Now push it to avoid confusing future tests\n+\t\t\tgit push\n+\t\t)\n \t)\n '\n \n@@ -271,9 +281,11 @@ test_expect_success 'push succeeds if submodule commit disabling recursion from\n \t\tgit fetch ../pub.git &&\n \t\tgit diff --quiet FETCH_HEAD master &&\n \t\t# But that the submodule commit did not\n-\t\t( cd gar/bage && git diff --quiet origin/master master^ ) &&\n-\t\t# Now push it to avoid confusing future tests\n-\t\tgit push --recurse-submodules=on-demand ../pub.git master\n+\t\t(cd gar/bage &&\n+\t\t\tgit diff --quiet origin/master master^ &&\n+\t\t\t# Now push it to avoid confusing future tests\n+\t\t\tgit push\n+\t\t)\n \t)\n '\n \ndiff --git a/transport.c b/transport.c\nindex 76e1daf..475faaf 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -845,6 +845,28 @@ static int run_pre_push_hook(struct transport *transport,\n \treturn ret;\n }\n \n+static void collect_new_old_hashes(struct ref *ref, struct sha1_array *new_hashes,\n+\t\tstruct sha1_array *old_hashes)\n+{\n+\tfor (; ref; ref = ref->next) {\n+\t\tif (!is_null_oid(&ref->new_oid)) {\n+\t\t\t/* Just need to handle non-null oid's. If there\n+\t\t\t * is no new we are handling a deletion and do\n+\t\t\t * not need to check\n+\t\t\t */\n+\t\t\tsha1_array_append(new_hashes, ref->new_oid.hash);\n+\n+\t\t\t/* if the old id is null we are handling an new\n+\t\t\t * ref and can simply skip appending the oid\n+\t\t\t * since they are used to reduce the amount of\n+\t\t\t * checked revisions.\n+\t\t\t */\n+\t\t\tif (!is_null_oid(&ref->old_oid))\n+\t\t\t\tsha1_array_append(old_hashes, ref->old_oid.hash);\n+\t\t}\n+\t}\n+}\n+\n int transport_push(struct transport *transport,\n \t\t int refspec_nr, const char **refspec, int flags,\n \t\t unsigned int *reject_reasons)\n@@ -903,13 +925,12 @@ int transport_push(struct transport *transport,\n \n \t\tif ((flags & TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND) && !is_bare_repository()) {\n \t\t\tstruct ref *ref = remote_refs;\n-\t\t\tstruct sha1_array hashes = SHA1_ARRAY_INIT;\n+\t\t\tstruct sha1_array new_hashes = SHA1_ARRAY_INIT;\n+\t\t\tstruct sha1_array old_hashes = SHA1_ARRAY_INIT;\n \n-\t\t\tfor (; ref; ref = ref->next)\n-\t\t\t\tif (!is_null_oid(&ref->new_oid))\n-\t\t\t\t\tsha1_array_append(&hashes, ref->new_oid.hash);\n+\t\t\tcollect_new_old_hashes(ref, &new_hashes, &old_hashes);\n \n-\t\t\tif (!push_unpushed_submodules(&hashes, transport->remote->name))\n+\t\t\tif (!push_unpushed_submodules(&old_hashes, &new_hashes))\n \t\t\t\tdie (\"Failed to push all needed submodules!\");\n \t\t}\n \n@@ -917,13 +938,12 @@ int transport_push(struct transport *transport,\n \t\t\t TRANSPORT_RECURSE_SUBMODULES_CHECK)) && !is_bare_repository()) {\n \t\t\tstruct ref *ref = remote_refs;\n \t\t\tstruct string_list needs_pushing = STRING_LIST_INIT_DUP;\n-\t\t\tstruct sha1_array hashes = SHA1_ARRAY_INIT;\n+\t\t\tstruct sha1_array new_hashes = SHA1_ARRAY_INIT;\n+\t\t\tstruct sha1_array old_hashes = SHA1_ARRAY_INIT;\n \n-\t\t\tfor (; ref; ref = ref->next)\n-\t\t\t\tif (!is_null_oid(&ref->new_oid))\n-\t\t\t\t\tsha1_array_append(&hashes, ref->new_oid.hash);\n+\t\t\tcollect_new_old_hashes(ref, &new_hashes, &old_hashes);\n \n-\t\t\tif (find_unpushed_submodules(&hashes, transport->remote->name,\n+\t\t\tif (find_unpushed_submodules(&old_hashes, &new_hashes,\n \t\t\t\t\t\t&needs_pushing))\n \t\t\t\tdie_with_unpushed_submodules(&needs_pushing);\n \t\t}\n-- \n2.10.0.133.g13017a3\n\n"},{"id":"302023","messageId":"xmqq1t0kna51.fsf@gitster.mtv.corp.google.com","threadId":"43925","inReplyTo":"20160915121044.GA96648@book.hvoigt.net","subject":"Re: [PATCH 3/2] batch check whether submodule needs pushing into one call","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-15T21:08:58Z","receivedAt":"2016-09-15T21:09:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt writes:\n\n> \tif (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {\n> \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> -\t\tconst char *argv[] = {\"rev-list\", NULL, \"--not\", \"--remotes\", \"-n\", \"1\" , NULL};\n> +\n> +\t\targv_array_push(&cp.args, \"rev-list\");\n> +\t\tsha1_array_for_each_unique(hashes, append_hash_to_argv, &cp.args);\n> +\t\targv_array_pushl(&cp.args, \"--not\", \"--remotes\", \"-n\", \"1\" , NULL);\n> +\n> \t\tstruct strbuf buf = STRBUF_INIT;\n> \t\tint needs_pushing = 0;\n\nThese two become decl-after-stmt; move your new lines a bit lower,\nperhaps?\n\n> -\t\targv[1] = sha1_to_hex(sha1);\n> -\t\tcp.argv = argv;\n> \t\tprepare_submodule_repo_env(&cp.env_array);\n\nBy the way, with the two new patches, 'pu' seems to start failing\nsome tests, e.g. 5533 5404 5405.\n\n"},{"id":"302050","messageId":"20160916094019.GB1488@book.hvoigt.net","threadId":"43925","inReplyTo":"xmqq1t0kna51.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/2] batch check whether submodule needs pushing into one call","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-09-16T09:40:19Z","receivedAt":"2016-09-16T09:40:29Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Thu, Sep 15, 2016 at 02:08:58PM -0700, Junio C Hamano wrote:\n> Heiko Voigt writes:\n> \n> > \tif (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {\n> > \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> > -\t\tconst char *argv[] = {\"rev-list\", NULL, \"--not\", \"--remotes\", \"-n\", \"1\" , NULL};\n> > +\n> > +\t\targv_array_push(&cp.args, \"rev-list\");\n> > +\t\tsha1_array_for_each_unique(hashes, append_hash_to_argv, &cp.args);\n> > +\t\targv_array_pushl(&cp.args, \"--not\", \"--remotes\", \"-n\", \"1\" , NULL);\n> > +\n> > \t\tstruct strbuf buf = STRBUF_INIT;\n> > \t\tint needs_pushing = 0;\n> \n> These two become decl-after-stmt; move your new lines a bit lower,\n> perhaps?\n\nThanks, missed those. Will do.\n\n> > -\t\targv[1] = sha1_to_hex(sha1);\n> > -\t\tcp.argv = argv;\n> > \t\tprepare_submodule_repo_env(&cp.env_array);\n> \n> By the way, with the two new patches, 'pu' seems to start failing\n> some tests, e.g. 5533 5404 5405.\n\nAh ok I did only test on master, will look into those.\n\nCheers Heiko\n"},{"id":"302051","messageId":"20160916123155.GA40725@book.hvoigt.net","threadId":"43925","inReplyTo":"20160916094019.GB1488@book.hvoigt.net","subject":"Re: [PATCH 3/2] batch check whether submodule needs pushing into one call","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-09-16T12:31:55Z","receivedAt":"2016-09-16T12:32:09Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Fri, Sep 16, 2016 at 11:40:19AM +0200, Heiko Voigt wrote:\n> > By the way, with the two new patches, 'pu' seems to start failing\n> > some tests, e.g. 5533 5404 5405.\n> \n> Ah ok I did only test on master, will look into those.\n\nOk I had a look into these and the reason t5533 fails is because on pu\n--recurse-submodules is enabled by default and I missed the case when\noverwriting a ref. In that case we get the sha1 from the remote side as\nold. So we could catch that and fall back to all revisions there, but...\n\n... tl;dr: The solution to use the old revisions from the remote side\nwas too simple and does not make matters better but actually worse for\nsome typical usecases. Its only in the last patch.\n\n... that lead me to further thinking about the previous solution (using\nthe locally cached remote refs) which might actually be a good default\nfor the non-fastforward cases like creating new ref or overwriting a\nref.\n\nMy current patch would handle the --mirror case nicer, since it gets a\nlot of old revs to reduce the revisions to check. For the typical one\nbranch push it would most likely be worse. Except when the user is\nupdating (fast-forwarding) the remote ref we would scan all revs of a\nref until the root because we do not get enough valid revs that already\nexist on the remote.\n\nThe most exact solution would be to use all actual remote refs available\n(not sure if we have them at this point in the process?) another\nsolution would be to still append the --remotes= option as a\nfallback to reduce the revisions.\n\nWhat do others think? Will leave this for a little bit further thinking.\nIts just the last patch (\"use actual start hashes for submodule push\ncheck instead of local refs\") that needs to go back to the drawing\nboard.\n\nCheers Heiko\n"},{"id":"302055","messageId":"xmqqintvlpqv.fsf@gitster.mtv.corp.google.com","threadId":"43925","inReplyTo":"20160914173124.GA7613@sandbox","subject":"Re: [PATCH 1/2] serialize collection of changed submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-16T17:27:04Z","receivedAt":"2016-09-16T17:27:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt writes:\n\n> +static struct sha1_array *get_sha1s_from_list(struct string_list *submodules,\n> +\t\tconst char *path)\n> +{\n> +\tstruct string_list_item *item;\n> +\tstruct sha1_array *hashes;\n> +\n> +\titem = string_list_insert(submodules, path);\n> +\tif (item->util)\n> +\t\treturn (struct sha1_array *) item->util;\n> +\n> +\thashes = (struct sha1_array *) xmalloc(sizeof(struct sha1_array));\n> +\t/* NEEDSWORK: should we add an initializer function for\n> +\t * sha1_array ? */\n> +\tmemset(hashes, 0, sizeof(struct sha1_array));\n> +\titem->util = hashes;\n\n\n\t/* NEEDSWORK: should we have SHA1_ARRAY_INIT etc.? */\n\titem->util = xcalloc(1, sizeof(struct sha1_array));\n\n> static void collect_submodules_from_diff(struct diff_queue_struct *q,\n> \t\t\t\t\t struct diff_options *options,\n> \t\t\t\t\t void *data)\n> {\n> \tint i;\n> -\tstruct string_list *needs_pushing = data;\n> +\tstruct string_list *submodules = data;\n> \n> \tfor (i = 0; i < q->nr; i++) {\n> \t\tstruct diff_filepair *p = q->queue[i];\n> +\t\tstruct sha1_array *hashes;\n> \t\tif (!S_ISGITLINK(p->two->mode))\n> \t\t\tcontinue;\n> -\t\tif (submodule_needs_pushing(p->two->path, p->two->oid.hash))\n> -\t\t\tstring_list_insert(needs_pushing, p->two->path);\n> +\t\thashes = get_sha1s_from_list(submodules, p->two->path);\n> +\t\tsha1_array_append(hashes, p->two->oid.hash);\n> \t}\n> }\n\nSo the idea at this step is still let each commit in the top-level\nhistory inspected for any submodule change, but the result is\ncollected in a mapping (submodule -> [ list of submodule commits ]).\nAs we do not expect too many \"oops, the old commit was better, so\nlet's revert and rebind the old one from the submodule\" in the\nhistory of the top-level, appending and then running for-each-unique\nis an efficient way, instead of first checking if we already have\nit and then inserting new ones to maintain the uniqueness.\n\nMakes sense.\n\n> @@ -582,14 +601,41 @@ static void find_unpushed_submodule_commits(struct commit *commit,\n> \tdiff_tree_combined_merge(commit, 1, &rev);\n> }\n> \n> +struct collect_submodule_from_sha1s_data {\n> +\tchar *submodule_path;\n> +\tstruct string_list *needs_pushing;\n> +};\n> +\n> +static void collect_submodules_from_sha1s(const unsigned char sha1[20],\n> +\t\tvoid *data)\n> +{\n> +\tstruct collect_submodule_from_sha1s_data *me =\n> +\t\t(struct collect_submodule_from_sha1s_data *) data;\n> +\n> +\tif (submodule_needs_pushing(me->submodule_path, sha1))\n> +\t\tstring_list_insert(me->needs_pushing, me->submodule_path);\n> +}\n\nThis is called from sha1_array_for_each_unique() that iterates over\nthe submodule commit object names for one submodule and then ends up\ncalling submodule_needs_pushing() number of times, which smells less\nefficient than it could be. You can ask\n\n rev-list --not --remotes\n\njust once in the submodule repository. I imagine that is what you'll\ndo in the next patch.\n\nAn obvious but much less efficient way to optimize this part would\nbe to see if me->needs_pushing already has me->submodule_path and\nskip the check for submodule_needs_pushing(), but if you drop the\ncall by find_unpushed_submodule to sha1_array_for_each_unique() to\nwalk new submodule commits one by one, that would become irrelevant.\n\n> +static void free_submodules_sha1s(struct string_list *submodules)\n> +{\n> +\tint i;\n> +\tfor (i = 0; i < submodules->nr; i++) {\n> +\t\tstruct string_list_item *item = &submodules->items[i];\n> +\t\tstruct sha1_array *hashes = (struct sha1_array *) item->util;\n> +\t\tsha1_array_clear(hashes);\n> +\t}\n> +\tstring_list_clear(submodules, 1);\n> +}\n> +\n> int find_unpushed_submodules(unsigned char new_sha1[20],\n> \t\tconst char *remotes_name, struct string_list *needs_pushing)\n> {\n> \tstruct rev_info rev;\n> \tstruct commit *commit;\n> \tconst char *argv[] = {NULL, NULL, \"--not\", \"NULL\", NULL};\n> -\tint argc = ARRAY_SIZE(argv) - 1;\n> +\tint argc = ARRAY_SIZE(argv) - 1, i;\n> \tchar *sha1_copy;\n> +\tstruct string_list submodules = STRING_LIST_INIT_DUP;\n> \n> \tstruct strbuf remotes_arg = STRBUF_INIT;\n> \n> @@ -603,12 +649,23 @@ int find_unpushed_submodules(unsigned char new_sha1[20],\n> \t\tdie(\"revision walk setup failed\");\n> \n> \twhile ((commit = get_revision(&rev)) != NULL)\n> -\t\tfind_unpushed_submodule_commits(commit, needs_pushing);\n> +\t\tfind_unpushed_submodule_commits(commit, &submodules);\n> \n> \treset_revision_walk();\n> \tfree(sha1_copy);\n> \tstrbuf_release(&remotes_arg);\n> \n> +\tfor (i = 0; i < submodules.nr; i++) {\n> +\t\tstruct string_list_item *item = &submodules.items[i];\n> +\t\tstruct collect_submodule_from_sha1s_data data;\n> +\t\tdata.submodule_path = item->string;\n> +\t\tdata.needs_pushing = needs_pushing;\n> +\t\tsha1_array_for_each_unique((struct sha1_array *) item->util,\n> +\t\t\t\tcollect_submodules_from_sha1s,\n> +\t\t\t\t&data);\n> +\t}\n> +\tfree_submodules_sha1s(&submodules);\n> +\n> \treturn needs_pushing->nr;\n> }\n"},{"id":"302063","messageId":"xmqqd1k3losd.fsf@gitster.mtv.corp.google.com","threadId":"43925","inReplyTo":"20160914175130.GB7613@sandbox","subject":"Re: [PATCH 2/2] serialize collection of refs that contain submodule changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-16T17:47:46Z","receivedAt":"2016-09-16T17:47:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt writes:\n\n> diff --git a/submodule.c b/submodule.c\n> index b04c066..a15e346 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -627,24 +627,31 @@ static void free_submodules_sha1s(struct string_list *submodules)\n> \tstring_list_clear(submodules, 1);\n> }\n> \n> -int find_unpushed_submodules(unsigned char new_sha1[20],\n> +static void append_hash_to_argv(const unsigned char sha1[20],\n> +\t\tvoid *data)\n> +{\n> +\tstruct argv_array *argv = (struct argv_array *) data;\n> +\targv_array_push(argv, sha1_to_hex(sha1));\n> +}\n> +\n> +int find_unpushed_submodules(struct sha1_array *hashes,\n> \t\tconst char *remotes_name, struct string_list *needs_pushing)\n> {\n> \tstruct rev_info rev;\n> \tstruct commit *commit;\n> -\tconst char *argv[] = {NULL, NULL, \"--not\", \"NULL\", NULL};\n> -\tint argc = ARRAY_SIZE(argv) - 1, i;\n> -\tchar *sha1_copy;\n> +\tint i;\n> \tstruct string_list submodules = STRING_LIST_INIT_DUP;\n> +\tstruct argv_array argv = ARGV_ARRAY_INIT;\n> \n> -\tstruct strbuf remotes_arg = STRBUF_INIT;\n> -\n> -\tstrbuf_addf(&remotes_arg, \"--remotes=%s\", remotes_name);\n> \tinit_revisions(&rev, NULL);\n> -\tsha1_copy = xstrdup(sha1_to_hex(new_sha1));\n> -\targv[1] = sha1_copy;\n> -\targv[3] = remotes_arg.buf;\n> -\tsetup_revisions(argc, argv, &rev, NULL);\n> +\n> +\t/* argv.argv[0] will be ignored by setup_revisions */\n> +\targv_array_push(&argv, \"find_unpushed_submodules\");\n> +\tsha1_array_for_each_unique(hashes, append_hash_to_argv, &argv);\n> +\targv_array_push(&argv, \"--not\");\n> +\targv_array_pushf(&argv, \"--remotes=%s\", remotes_name);\n> +\n> +\tsetup_revisions(argv.argc, argv.argv, &rev, NULL);\n\nYes, its about time to for us to lose that fixed-size argv[] and\nreplace it with an argv-array ;-).\n\n> \tif (prepare_revision_walk(&rev))\n> \t\tdie(\"revision walk setup failed\");\n\nSo this one used to get a single commit at the tip of what we pushed\nin the superproject and was asked \"Look at the history we just\npushed leading to the tip commit, and tell me if any of the ones new\nto the remote requires submodule commits the remote does not yet\nhave\". Now the caller collects all the tip commits and asks us\nonce: \"Here are the new tips we just pushed; in the history leading\nto them, is there a commit that the remote did not have that requires\nsubmodule history the remote does not yet have?\".\n\nMakes sort-of sense.\n\nI speculated that you would be doing the same kind of optimization\nto feed all positive commits to rev-list at once in each submodule\nrepository in the review of the previous one, but you didn't do it\nhere. You did the same for superproject in this step. Perhaps 3 or\n4 would do so in the submodule repository.\n\nOne thing that makes me worried is how the ref cache layer interacts\nwith this. I see you first call push_unpushed_submodules() when\nON_DEMAND is set, which would result in pushes in submodule\nrepositories, updating their remote tracking branches. At that\npoint, before you make another call to find_unpushed_submodules(),\nis our cached ref layer knows that the remote tracking branches\nare now up to date (otherwise, we would incorrectly judge that these\nsubmodules need pushing based on stale information)?\n\n> diff --git a/transport.c b/transport.c\n> index 94d6dc3..76e1daf 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -903,23 +903,29 @@ int transport_push(struct transport *transport,\n> \n> \t\tif ((flags & TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND) && !is_bare_repository()) {\n> \t\t\tstruct ref *ref = remote_refs;\n> +\t\t\tstruct sha1_array hashes = SHA1_ARRAY_INIT;\n> +\n> \t\t\tfor (; ref; ref = ref->next)\n> -\t\t\t\tif (!is_null_oid(&ref->new_oid) &&\n> -\t\t\t\t !push_unpushed_submodules(ref->new_oid.hash,\n> -\t\t\t\t\t transport->remote->name))\n> -\t\t\t\t die (\"Failed to push all needed submodules!\");\n> +\t\t\t\tif (!is_null_oid(&ref->new_oid))\n> +\t\t\t\t\tsha1_array_append(&hashes, ref->new_oid.hash);\n> +\n> +\t\t\tif (!push_unpushed_submodules(&hashes, transport->remote->name))\n> +\t\t\t\tdie (\"Failed to push all needed submodules!\");\n\nDo we leak the contents of hashes here?\n\n> \t\t}\n> \n> \t\tif ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n> \t\t\t TRANSPORT_RECURSE_SUBMODULES_CHECK)) && !is_bare_repository()) {\n> \t\t\tstruct ref *ref = remote_refs;\n> \t\t\tstruct string_list needs_pushing = STRING_LIST_INIT_DUP;\n> +\t\t\tstruct sha1_array hashes = SHA1_ARRAY_INIT;\n> \n> \t\t\tfor (; ref; ref = ref->next)\n> -\t\t\t\tif (!is_null_oid(&ref->new_oid) &&\n> -\t\t\t\t find_unpushed_submodules(ref->new_oid.hash,\n> -\t\t\t\t\t transport->remote->name, &needs_pushing))\n> -\t\t\t\t\tdie_with_unpushed_submodules(&needs_pushing);\n> +\t\t\t\tif (!is_null_oid(&ref->new_oid))\n> +\t\t\t\t\tsha1_array_append(&hashes, ref->new_oid.hash);\n> +\n> +\t\t\tif (find_unpushed_submodules(&hashes, transport->remote->name,\n> +\t\t\t\t\t\t&needs_pushing))\n> +\t\t\t\tdie_with_unpushed_submodules(&needs_pushing);\n\nDo we leak the contents of hashes here? I do not think we need to\nworry about needs_pushing leaking, as we will always die if it is\nnot empty, but it might be a better code hygiene to clear it as\nwell.\n\n> \t\t}\n> \n> \t\tpush_ret = transport->push_refs(transport, remote_refs, flags);\n\nThanks.\n"},{"id":"302064","messageId":"xmqq8turlo8m.fsf@gitster.mtv.corp.google.com","threadId":"43925","inReplyTo":"20160915121044.GA96648@book.hvoigt.net","subject":"Re: [PATCH 3/2] batch check whether submodule needs pushing into one call","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-16T17:59:37Z","receivedAt":"2016-09-16T17:59:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt writes:\n\n> +static void append_hash_to_argv(const unsigned char sha1[20], void *data)\n> {\n> -\tif (add_submodule_odb(path) || !lookup_commit_reference(sha1))\n> +\tstruct argv_array *argv = (struct argv_array *) data;\n> +\targv_array_push(argv, sha1_to_hex(sha1));\n> +}\n\nHmph, why do I think I've seen this before in the previous patch?\n\n ... scans through this patch and finds that a similar one is\n removed ;-)\n\nOK. This makes sense.\n\n> +static void check_has_hash(const unsigned char sha1[20], void *data)\n> +{\n> +\tint *has_hash = (int *) data;\n> +\n> +\tif (!lookup_commit_reference(sha1))\n> +\t\t*has_hash = 0;\n> +}\n> +\n> +static int submodule_has_hashes(const char *path, struct sha1_array *hashes)\n> +{\n> +\tint has_hash = 1;\n> +\n> +\tif (add_submodule_odb(path))\n> +\t\treturn 0;\n> +\n> +\tsha1_array_for_each_unique(hashes, check_has_hash, &has_hash);\n> +\treturn has_hash;\n> +}\n> +\n> +static int submodule_needs_pushing(const char *path, struct sha1_array *hashes)\n> +{\n> +\tif (!submodule_has_hashes(path, hashes))\n> \t\treturn 0;\n\nI think you meant well, but this optimization is wrong. A mere\npresence of an object does not mean that the current tip can reach\nthat object. Imagine you pushed commit A earlier to them at the\ntip, then pushed commit A~ to them at the tip, which is the current\nstate of the remote of the submodule, and since them they may have\nGC'ed. They no longer have the commit A.\n\nFor that matter, because you are doing this check by pretending as\nif all the submodule objects are in the object store of the current\nsuperproject you are working in, and saying \"it exists there in the\nsubmodule repository\" when the only thing you know is it exists in\nan object store of either the submodule repository, the superproject\nrepository, or any of the other submodule repositories, you really\ncannot tell much from a mere presence of an object. Not just the\nremote of the submodule repository you are interested in, but the\nsubmodule repository you are interested in itself, may not have that\nobject.\n\nDrop the previous two helper functions and this short-cut.\n\n> \tif (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {\n> \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> -\t\tconst char *argv[] = {\"rev-list\", NULL, \"--not\", \"--remotes\", \"-n\", \"1\" , NULL};\n> +\n> +\t\targv_array_push(&cp.args, \"rev-list\");\n> +\t\tsha1_array_for_each_unique(hashes, append_hash_to_argv, &cp.args);\n> +\t\targv_array_pushl(&cp.args, \"--not\", \"--remotes\", \"-n\", \"1\" , NULL);\n> +\n> \t\tstruct strbuf buf = STRBUF_INIT;\n> \t\tint needs_pushing = 0;\n> \n> -\t\targv[1] = sha1_to_hex(sha1);\n> -\t\tcp.argv = argv;\n> \t\tprepare_submodule_repo_env(&cp.env_array);\n> \t\tcp.git_cmd = 1;\n> \t\tcp.no_stdin = 1;\n> \t\tcp.out = -1;\n> \t\tcp.dir = path;\n> \t\tif (start_command(&cp))\n> -\t\t\tdie(\"Could not run 'git rev-list %s --not --remotes -n 1' command in submodule %s\",\n> -\t\t\t\tsha1_to_hex(sha1), path);\n> +\t\t\tdie(\"Could not run 'git rev-list --not --remotes -n 1' command in submodule %s\",\n> +\t\t\t\t\tpath);\n> \t\tif (strbuf_read(&buf, cp.out, 41))\n> \t\t\tneeds_pushing = 1;\n> \t\tfinish_command(&cp);\n> @@ -601,21 +628,6 @@ static void find_unpushed_submodule_commits(struct commit *commit,\n> \tdiff_tree_combined_merge(commit, 1, &rev);\n> }\n\nGood. This is the optimization I alluded to in the review of the\nfirst one in the series.\n"},{"id":"302065","messageId":"xmqq1t0jlnm2.fsf@gitster.mtv.corp.google.com","threadId":"43925","inReplyTo":"20160916123155.GA40725@book.hvoigt.net","subject":"Re: [PATCH 3/2] batch check whether submodule needs pushing into one call","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-16T18:13:09Z","receivedAt":"2016-09-16T18:13:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt writes:\n\n> On Fri, Sep 16, 2016 at 11:40:19AM +0200, Heiko Voigt wrote:\n>> > By the way, with the two new patches, 'pu' seems to start failing\n>> > some tests, e.g. 5533 5404 5405.\n>> \n>> Ah ok I did only test on master, will look into those.\n>\n> Ok I had a look into these and the reason t5533 fails is because on pu\n> --recurse-submodules is enabled by default and I missed the case when\n> overwriting a ref. In that case we get the sha1 from the remote side as\n> old. So we could catch that and fall back to all revisions there, but...\n>\n> ... tl;dr: The solution to use the old revisions from the remote side\n> was too simple and does not make matters better but actually worse for\n> some typical usecases. Its only in the last patch.\n\nYou may not even have the old one in your copy of the remote\nrepository if you haven't fetched from them and you are forcing your\npush. \"rev-list --not \" may fail in such a case,\nnot producing the list of new commits. You'd need to exclude old ones\nyou learned over the wire that you do not yet have locally.\n\n> The most exact solution would be to use all actual remote refs available\n> (not sure if we have them at this point in the process?) another\n> solution would be to still append the --remotes= option as a\n> fallback to reduce the revisions.\n\nI'd say --remotes= is the least problematic thing to do.\n\n"},{"id":"302172","messageId":"20160919194412.GA62429@book.hvoigt.net","threadId":"43925","inReplyTo":"xmqqintvlpqv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] serialize collection of changed submodules","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-09-19T19:44:29Z","receivedAt":"2016-09-19T19:44:40Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Fri, Sep 16, 2016 at 10:27:04AM -0700, Junio C Hamano wrote:\n> Heiko Voigt writes:\n> \n> > +static struct sha1_array *get_sha1s_from_list(struct string_list *submodules,\n> > +\t\tconst char *path)\n> > +{\n> > +\tstruct string_list_item *item;\n> > +\tstruct sha1_array *hashes;\n> > +\n> > +\titem = string_list_insert(submodules, path);\n> > +\tif (item->util)\n> > +\t\treturn (struct sha1_array *) item->util;\n> > +\n> > +\thashes = (struct sha1_array *) xmalloc(sizeof(struct sha1_array));\n> > +\t/* NEEDSWORK: should we add an initializer function for\n> > +\t * sha1_array ? */\n> > +\tmemset(hashes, 0, sizeof(struct sha1_array));\n> > +\titem->util = hashes;\n> \n> \n> \t/* NEEDSWORK: should we have SHA1_ARRAY_INIT etc.? */\n> \titem->util = xcalloc(1, sizeof(struct sha1_array));\n\nOk will do.\n"},{"id":"302174","messageId":"20160919195118.GB62429@book.hvoigt.net","threadId":"43925","inReplyTo":"xmqqd1k3losd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] serialize collection of refs that contain submodule changes","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-09-19T19:51:18Z","receivedAt":"2016-09-19T19:51:28Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nOn Fri, Sep 16, 2016 at 10:47:46AM -0700, Junio C Hamano wrote:\n> One thing that makes me worried is how the ref cache layer interacts\n> with this. I see you first call push_unpushed_submodules() when\n> ON_DEMAND is set, which would result in pushes in submodule\n> repositories, updating their remote tracking branches. At that\n> point, before you make another call to find_unpushed_submodules(),\n> is our cached ref layer knows that the remote tracking branches\n> are now up to date (otherwise, we would incorrectly judge that these\n> submodules need pushing based on stale information)?\n\nI am not sure if I understand you correctly here. With the \"ref cache layer\"\nyou are referring to add_submodule_odb() which is called indirectly from\nsubmodule_needs_pushing()? Those revs are only used to check whether the hash\nwe need on the remote side exists in the local submodule. That should not\nchange due to a push. The actual check whether the commit(s) exist on the\nremote side is done using a 'rev-list' in a subprocess later.\n\n\n> > diff --git a/transport.c b/transport.c\n> > index 94d6dc3..76e1daf 100644\n> > --- a/transport.c\n> > +++ b/transport.c\n> > @@ -903,23 +903,29 @@ int transport_push(struct transport *transport,\n> > \n> > \t\tif ((flags & TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND) && !is_bare_repository()) {\n> > \t\t\tstruct ref *ref = remote_refs;\n> > +\t\t\tstruct sha1_array hashes = SHA1_ARRAY_INIT;\n> > +\n> > \t\t\tfor (; ref; ref = ref->next)\n> > -\t\t\t\tif (!is_null_oid(&ref->new_oid) &&\n> > -\t\t\t\t !push_unpushed_submodules(ref->new_oid.hash,\n> > -\t\t\t\t\t transport->remote->name))\n> > -\t\t\t\t die (\"Failed to push all needed submodules!\");\n> > +\t\t\t\tif (!is_null_oid(&ref->new_oid))\n> > +\t\t\t\t\tsha1_array_append(&hashes, ref->new_oid.hash);\n> > +\n> > +\t\t\tif (!push_unpushed_submodules(&hashes, transport->remote->name))\n> > +\t\t\t\tdie (\"Failed to push all needed submodules!\");\n> \n> Do we leak the contents of hashes here?\n\nGood catch, sorry about that. Will clear the hashes array.\n\n\n> > \t\t}\n> > \n> > \t\tif ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |\n> > \t\t\t TRANSPORT_RECURSE_SUBMODULES_CHECK)) && !is_bare_repository()) {\n> > \t\t\tstruct ref *ref = remote_refs;\n> > \t\t\tstruct string_list needs_pushing = STRING_LIST_INIT_DUP;\n> > +\t\t\tstruct sha1_array hashes = SHA1_ARRAY_INIT;\n> > \n> > \t\t\tfor (; ref; ref = ref->next)\n> > -\t\t\t\tif (!is_null_oid(&ref->new_oid) &&\n> > -\t\t\t\t find_unpushed_submodules(ref->new_oid.hash,\n> > -\t\t\t\t\t transport->remote->name, &needs_pushing))\n> > -\t\t\t\t\tdie_with_unpushed_submodules(&needs_pushing);\n> > +\t\t\t\tif (!is_null_oid(&ref->new_oid))\n> > +\t\t\t\t\tsha1_array_append(&hashes, ref->new_oid.hash);\n> > +\n> > +\t\t\tif (find_unpushed_submodules(&hashes, transport->remote->name,\n> > +\t\t\t\t\t\t&needs_pushing))\n> > +\t\t\t\tdie_with_unpushed_submodules(&needs_pushing);\n> \n> Do we leak the contents of hashes here? I do not think we need to\n> worry about needs_pushing leaking, as we will always die if it is\n> not empty, but it might be a better code hygiene to clear it as\n> well.\n\nAs above, will fix.\n\nCheers Heiko\n"},{"id":"302175","messageId":"20160919195812.GC62429@book.hvoigt.net","threadId":"43925","inReplyTo":"xmqq8turlo8m.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/2] batch check whether submodule needs pushing into one call","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-09-19T19:58:12Z","receivedAt":"2016-09-19T19:58:28Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Fri, Sep 16, 2016 at 10:59:37AM -0700, Junio C Hamano wrote:\n> Heiko Voigt writes:\n> > +static void check_has_hash(const unsigned char sha1[20], void *data)\n> > +{\n> > +\tint *has_hash = (int *) data;\n> > +\n> > +\tif (!lookup_commit_reference(sha1))\n> > +\t\t*has_hash = 0;\n> > +}\n> > +\n> > +static int submodule_has_hashes(const char *path, struct sha1_array *hashes)\n> > +{\n> > +\tint has_hash = 1;\n> > +\n> > +\tif (add_submodule_odb(path))\n> > +\t\treturn 0;\n> > +\n> > +\tsha1_array_for_each_unique(hashes, check_has_hash, &has_hash);\n> > +\treturn has_hash;\n> > +}\n> > +\n> > +static int submodule_needs_pushing(const char *path, struct sha1_array *hashes)\n> > +{\n> > +\tif (!submodule_has_hashes(path, hashes))\n> > \t\treturn 0;\n> \n> I think you meant well, but this optimization is wrong. A mere\n> presence of an object does not mean that the current tip can reach\n> that object. Imagine you pushed commit A earlier to them at the\n> tip, then pushed commit A~ to them at the tip, which is the current\n> state of the remote of the submodule, and since them they may have\n> GC'ed. They no longer have the commit A.\n> \n> For that matter, because you are doing this check by pretending as\n> if all the submodule objects are in the object store of the current\n> superproject you are working in, and saying \"it exists there in the\n> submodule repository\" when the only thing you know is it exists in\n> an object store of either the submodule repository, the superproject\n> repository, or any of the other submodule repositories, you really\n> cannot tell much from a mere presence of an object. Not just the\n> remote of the submodule repository you are interested in, but the\n> submodule repository you are interested in itself, may not have that\n> object.\n> \n> Drop the previous two helper functions and this short-cut.\n\nThis is nothing I added. This came from the original code which I simply\nextended to check for all sha1's. The diff is a little bit misleading. This\nwould be the local diff:\n\n- if (add_submodule_odb(path) || !lookup_commit_reference(sha1))\n+ if (!submodule_has_hashes(path, hashes))\n return 0;\n\nI think that this is more a shortcut for: If we can not find the sha1, we do\nnot need to bother spawning a process for the real check. We default to the\nanswer no in that case.\n\nIt looks like the reason for the sha1 check is to avoid error output from the\nspawned 'rev-list' process. The error output might be confusing to the user in\ncase the sha1 does not exist in the submodule. We should probably die here\nsince we were not able to check a submodule that has a diff in the revisions we\nwould push.\n\nAfter thinking about this further: I think it is actually bad to proceed here,\nas the current revisions (just in the superproject) would still be pushed and\nthe user possibly gets an inconsistent state on the remote side which he tried\nto avoid with check/on-demand-push enabled.\n\nSo in short this deserves some further love. Will have a look into adding\nanother patch for this if we agree on my suggestion to die above.\n\nCheers Heiko\n"},{"id":"302177","messageId":"xmqqeg4fejne.fsf@gitster.mtv.corp.google.com","threadId":"43925","inReplyTo":"20160919195118.GB62429@book.hvoigt.net","subject":"Re: [PATCH 2/2] serialize collection of refs that contain submodule changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-19T20:09:41Z","receivedAt":"2016-09-19T20:09:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt writes:\n\n> I am not sure if I understand you correctly here. With the \"ref cache layer\"\n> you are referring to add_submodule_odb() which is called indirectly from\n> submodule_needs_pushing()? Those revs are only used to check whether the hash\n> we need on the remote side exists in the local submodule. That should not\n> change due to a push. The actual check whether the commit(s) exist on the\n> remote side is done using a 'rev-list' in a subprocess later.\n\nI was wondering what would happen in this scenario:\n\n * You have ON_DEMAND set, which causes \"git -C sub push origin\" to\n push out what are new, updating the remote tracking branches in\n the submodule, sub/.git/refs/remotes/origin/*.\n\n * Then you check again. If you used for-each-ref-in-submodule, the\n updated refs/remotes/origin/* may not have been re-read.\n\nBut you check by spawning \"rev-list ... --not --remotes\" as a\nseparate process in submodule_needs_pushing(), and that will force\nthe new process to read the updated state, so it turns out that I\nwas overly worried without good reason ;-)\n\nIt may matter once somebody tries to internalize the external\nrev-list call done via start_command() interface to an internal\ncall, though. But we are not there yet.\n"},{"id":"302178","messageId":"20160919200835.GD62429@book.hvoigt.net","threadId":"43925","inReplyTo":"xmqq1t0jlnm2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/2] batch check whether submodule needs pushing into one call","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-09-19T20:08:35Z","receivedAt":"2016-09-19T20:11:57Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Fri, Sep 16, 2016 at 11:13:09AM -0700, Junio C Hamano wrote:\n> Heiko Voigt writes:\n> > The most exact solution would be to use all actual remote refs available\n> > (not sure if we have them at this point in the process?) another\n> > solution would be to still append the --remotes= option as a\n> > fallback to reduce the revisions.\n> \n> I'd say --remotes= is the least problematic thing to do.\n\nOk then lets drop my last patch and keep it the way it was. Because if\nthe remote sha1 differs we probably do not have it locally anyway. The\nonly case this does not catch is when the user specifies a remote URL.\nBut that just means we will iterate over all revisions instead of a\nreduced set, which makes the check slower but still correct. As one can\nsee from my measurements that should not be that bad anymore.\n\nCheers Heiko\n"}]}