{"thread":{"id":"44245","subject":"[PATCH v2 1/3] serialize collection of changed submodules","startedAt":"2016-10-07T15:07:33Z","lastAt":"2016-10-20T23:00:50Z","messageCount":19,"participants":["Heiko Voigt","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"303569","messageId":"10cd5be93601bc52388100e80b6c6735a7cacfb4.1475851621.git.hvoigt@hvoigt.net","threadId":"44245","inReplyTo":"cover.1475851621.git.hvoigt@hvoigt.net","subject":"[PATCH v2 1/3] serialize collection of changed submodules","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-10-07T15:06:34Z","receivedAt":"2016-10-07T15:07:33Z","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---\n submodule.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----\n 1 file changed, 58 insertions(+), 5 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 2de06a3351..59c9d15905 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -554,19 +554,34 @@ 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+\n+\titem = string_list_insert(submodules, path);\n+\tif (item->util)\n+\t\treturn (struct sha1_array *) item->util;\n+\n+\t/* NEEDSWORK: should we have sha1_array_init()? */\n+\titem->util = xcalloc(1, sizeof(struct sha1_array));\n+\treturn (struct sha1_array *) item->util;\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 +597,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 +645,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.10.1.637.g09b28c5\n\n"},{"id":"303570","messageId":"81bdbf6a1295c17c1b9233c91da6e5eb4583785e.1475851621.git.hvoigt@hvoigt.net","threadId":"44245","inReplyTo":"cover.1475851621.git.hvoigt@hvoigt.net","subject":"[PATCH v2 2/3] serialize collection of refs that contain submodule changes","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-10-07T15:06:35Z","receivedAt":"2016-10-07T15:07:36Z","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 submodule.c | 36 +++++++++++++++++++++---------------\n submodule.h | 5 +++--\n transport.c | 29 +++++++++++++++++++++--------\n 3 files changed, 45 insertions(+), 25 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 59c9d15905..5044afc2f8 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -522,6 +522,13 @@ static int has_remote(const char *refname, const struct object_id *oid,\n \treturn 1;\n }\n \n+static int append_hash_to_argv(const unsigned char sha1[20], void *data)\n+{\n+\tstruct argv_array *argv = (struct argv_array *) data;\n+\targv_array_push(argv, sha1_to_hex(sha1));\n+\treturn 0;\n+}\n+\n static int submodule_needs_pushing(const char *path, const unsigned char sha1[20])\n {\n \tif (add_submodule_odb(path) || !lookup_commit_reference(sha1))\n@@ -623,24 +630,24 @@ 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+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@@ -648,8 +655,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@@ -687,12 +693,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 d9e197a948..065b2f0a2a 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 94d6dc3725..05f2ce83f1 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -903,23 +903,36 @@ 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\tsha1_array_clear(&hashes);\n+\t\t\t\tdie (\"Failed to push all needed submodules!\");\n+\t\t\t}\n+\t\t\tsha1_array_clear(&hashes);\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\tsha1_array_clear(&hashes);\n+\t\t\t\tdie_with_unpushed_submodules(&needs_pushing);\n+\t\t\t}\n+\t\t\tstring_list_clear(&needs_pushing, 0);\n+\t\t\tsha1_array_clear(&hashes);\n \t\t}\n \n \t\tpush_ret = transport->push_refs(transport, remote_refs, flags);\n-- \n2.10.1.637.g09b28c5\n\n"},{"id":"303571","messageId":"cover.1475851621.git.hvoigt@hvoigt.net","threadId":"44245","inReplyTo":null,"subject":"[PATCH v2 0/3] Speedup finding of unpushed submodules","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-10-07T15:06:33Z","receivedAt":"2016-10-07T15:07:37Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"You can find the first iteration of this series as part of this thread:\n\nhttp://public-inbox.org/git/%3C20160914173124.GA7613@sandbox%3E/\n\nAll mentioned issues should be fixed. I dropped the last patch which was\nthe cause of the broken tests.\n\nThis should optimize every part of this test to a nice speed if you are\npushing to a remote. The only case that is still broken/slow as hell is\nwhen calling push with a direct url.\n\nI am thinking whether we should maybe error out with a \"not implemented\"\nmessage or something and mention that --recurse-submoules does not work\nwith direct urls? But we might want to have another look at performance\nwith this patch included. Maybe it is actually useable with the last\npatch included which was not yet on pu.\n\nCheers Heiko\n\nHeiko Voigt (3):\n serialize collection of changed submodules\n serialize collection of refs that contain submodule changes\n batch check whether submodule needs pushing into one call\n\n submodule.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++--------------\n submodule.h | 5 +--\n transport.c | 29 ++++++++++-----\n 3 files changed, 114 insertions(+), 36 deletions(-)\n\n-- \n2.10.1.637.g09b28c5\n\n"},{"id":"303572","messageId":"67d4c48dc0129f20041c88d27a49c7a21188c882.1475851621.git.hvoigt@hvoigt.net","threadId":"44245","inReplyTo":"cover.1475851621.git.hvoigt@hvoigt.net","subject":"[PATCH v2 3/3] batch check whether submodule needs pushing into one call","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-10-07T15:06:36Z","receivedAt":"2016-10-07T15:07:39Z","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---\n submodule.c | 63 +++++++++++++++++++++++++++++++++----------------------------\n 1 file changed, 34 insertions(+), 29 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 5044afc2f8..a05c2a34b1 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -529,27 +529,49 @@ static int append_hash_to_argv(const unsigned char sha1[20], void *data)\n \treturn 0;\n }\n \n-static int submodule_needs_pushing(const char *path, const unsigned char sha1[20])\n+static int check_has_hash(const unsigned char sha1[20], void *data)\n {\n-\tif (add_submodule_odb(path) || !lookup_commit_reference(sha1))\n+\tint *has_hash = (int *) data;\n+\n+\tif (!lookup_commit_reference(sha1))\n+\t\t*has_hash = 0;\n+\n+\treturn 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 \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\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\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@@ -604,21 +626,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@@ -658,13 +665,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.1.637.g09b28c5\n\n"},{"id":"303595","messageId":"CAGZ79kZiY56-84aThH1F02E_HzCTAK3KSYLbyP1D5GUAt892cw@mail.gmail.com","threadId":"44245","inReplyTo":"10cd5be93601bc52388100e80b6c6735a7cacfb4.1475851621.git.hvoigt@hvoigt.net","subject":"Re: [PATCH v2 1/3] serialize collection of changed submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-07T17:59:29Z","receivedAt":"2016-10-07T18:07:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Oct 7, 2016 at 8:06 AM, Heiko Voigt wrote:\n> To check whether a submodule needs to be pushed we need to collect all\n> changed submodules. Lets collect them first and then execute the\n> possibly expensive test whether certain revisions are already pushed\n> only once per submodule.\n>\n> There is further potential for optimization since we can assemble one\n> command and only issued that instead of one call for each remote ref in\n> the submodule.\n>\n> Signed-off-by: Heiko Voigt \n> ---\n> submodule.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----\n> 1 file changed, 58 insertions(+), 5 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index 2de06a3351..59c9d15905 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -554,19 +554,34 @@ static int submodule_needs_pushing(const char *path, const unsigned char sha1[20\n> return 0;\n> }\n>\n> +static struct sha1_array *get_sha1s_from_list(struct string_list *submodules,\n> + const char *path)\n\nSo this will take the stringlist `submodules` and insert the path into it,\nif it wasn't already in there. In case it is newly inserted, add a sha1_array\nas util, so each inserted path has it's own empty array.\n\nSo it is both init of the data structures as well as retrieving them. I was\ninitially confused by the name as I assumed it would give you sha1s out\nof a string list (e.g. transform strings to internal sha1 things).\nMaybe it's just\nme having a hard time to understand that, but I feel like the name could be\nimproved.\n\n lookup_sha1_list_by_path,\n insert_path_and_return_sha1_list ?\n\n> +{\n> + struct string_list_item *item;\n> +\n> + item = string_list_insert(submodules, path);\n> + if (item->util)\n> + return (struct sha1_array *) item->util;\n> +\n> + /* NEEDSWORK: should we have sha1_array_init()? */\n> + item->util = xcalloc(1, sizeof(struct sha1_array));\n> + return (struct sha1_array *) item->util;\n> +}\n> +\n> static void collect_submodules_from_diff(struct diff_queue_struct *q,\n> struct diff_options *options,\n> void *data)\n> {\n> int i;\n> - struct string_list *needs_pushing = data;\n> + struct string_list *submodules = data;\n>\n> for (i = 0; i < q->nr; i++) {\n> struct diff_filepair *p = q->queue[i];\n> + struct sha1_array *hashes;\n> if (!S_ISGITLINK(p->two->mode))\n> continue;\n> - if (submodule_needs_pushing(p->two->path, p->two->oid.hash))\n> - string_list_insert(needs_pushing, p->two->path);\n> + hashes = get_sha1s_from_list(submodules, p->two->path);\n> + sha1_array_append(hashes, p->two->oid.hash);\n> }\n> }\n>\n> @@ -582,14 +597,41 @@ static void find_unpushed_submodule_commits(struct commit *commit,\n> diff_tree_combined_merge(commit, 1, &rev);\n> }\n>\n> +struct collect_submodule_from_sha1s_data {\n> + char *submodule_path;\n> + struct string_list *needs_pushing;\n> +};\n> +\n> +static void collect_submodules_from_sha1s(const unsigned char sha1[20],\n> + void *data)\n> +{\n> + struct collect_submodule_from_sha1s_data *me =\n> + (struct collect_submodule_from_sha1s_data *) data;\n> +\n> + if (submodule_needs_pushing(me->submodule_path, sha1))\n> + string_list_insert(me->needs_pushing, me->submodule_path);\n> +}\n> +\n> +static void free_submodules_sha1s(struct string_list *submodules)\n> +{\n> + int i;\n> + for (i = 0; i < submodules->nr; i++) {\n> + struct string_list_item *item = &submodules->items[i];\n\nYou do not seem to make use of `i` explicitely, so\nfor_each_string_list_item might be more readable here?\n\n\n> + struct sha1_array *hashes = (struct sha1_array *) item->util;\n> + sha1_array_clear(hashes);\n> + }\n> + string_list_clear(submodules, 1);\n> +}\n> +\n> int find_unpushed_submodules(unsigned char new_sha1[20],\n> const char *remotes_name, struct string_list *needs_pushing)\n> {\n> struct rev_info rev;\n> struct commit *commit;\n> const char *argv[] = {NULL, NULL, \"--not\", \"NULL\", NULL};\n> - int argc = ARRAY_SIZE(argv) - 1;\n> + int argc = ARRAY_SIZE(argv) - 1, i;\n> char *sha1_copy;\n> + struct string_list submodules = STRING_LIST_INIT_DUP;\n>\n> struct strbuf remotes_arg = STRBUF_INIT;\n>\n> @@ -603,12 +645,23 @@ int find_unpushed_submodules(unsigned char new_sha1[20],\n> die(\"revision walk setup failed\");\n>\n> while ((commit = get_revision(&rev)) != NULL)\n> - find_unpushed_submodule_commits(commit, needs_pushing);\n> + find_unpushed_submodule_commits(commit, &submodules);\n>\n> reset_revision_walk();\n> free(sha1_copy);\n> strbuf_release(&remotes_arg);\n>\n> + for (i = 0; i < submodules.nr; i++) {\n> + struct string_list_item *item = &submodules.items[i];\n\nYou do not seem to make use of `i` explicitely, so\nfor_each_string_list_item might be more readable here?\n\n\n> + struct collect_submodule_from_sha1s_data data;\n> + data.submodule_path = item->string;\n> + data.needs_pushing = needs_pushing;\n> + sha1_array_for_each_unique((struct sha1_array *) item->util,\n> + collect_submodules_from_sha1s,\n> + &data);\n> + }\n> + free_submodules_sha1s(&submodules);\n> +\n> return needs_pushing->nr;\n> }\n>\n> --\n> 2.10.1.637.g09b28c5\n>\n"},{"id":"303596","messageId":"CAGZ79kZ5CPTuW2fARDs3BUt89b8H_=P3otZv+Vm5nTV70NLWtg@mail.gmail.com","threadId":"44245","inReplyTo":"81bdbf6a1295c17c1b9233c91da6e5eb4583785e.1475851621.git.hvoigt@hvoigt.net","subject":"Re: [PATCH v2 2/3] serialize collection of refs that contain submodule changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-07T18:16:31Z","receivedAt":"2016-10-07T18:17:12Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Oct 7, 2016 at 8:06 AM, Heiko Voigt wrote:\n> We are iterating over each pushed ref and want to check whether it\n> contains changes to submodules. Instead of immediately checking each ref\n> lets first collect them and then do the check for all of them in one\n> revision walk.\n>\n> Signed-off-by: Heiko Voigt \n> ---\n> submodule.c | 36 +++++++++++++++++++++---------------\n> submodule.h | 5 +++--\n> transport.c | 29 +++++++++++++++++++++--------\n> 3 files changed, 45 insertions(+), 25 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index 59c9d15905..5044afc2f8 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -522,6 +522,13 @@ static int has_remote(const char *refname, const struct object_id *oid,\n> return 1;\n> }\n>\n> +static int append_hash_to_argv(const unsigned char sha1[20], void *data)\n> +{\n> + struct argv_array *argv = (struct argv_array *) data;\n> + argv_array_push(argv, sha1_to_hex(sha1));\n\nNit of the day:\nWhen using the struct child-process, we have the oldstyle argv NULL\nterminated array as\nwell as the new style args argv_array. So in that context we'd prefer\n`args` as a name for\nargv_array as that helps to distinguish from the old array type.\nHere however `argv` seems to be a reasonable name, in fact whenever we\ndo not deal with\nchild processes, we seem to not like the `args` name:\n\n $ git grep argv_array |wc -l\n 577\n $ git grep argv_array |grep args |wc -l\n 293\n\nThe rest looks good to me. :)\n"},{"id":"303603","messageId":"CAGZ79kYZbkXFLABdiQwTLLJ42CnV5OPWA5ESe0VfZLvrkQRWfA@mail.gmail.com","threadId":"44245","inReplyTo":"67d4c48dc0129f20041c88d27a49c7a21188c882.1475851621.git.hvoigt@hvoigt.net","subject":"Re: [PATCH v2 3/3] batch check whether submodule needs pushing into one call","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-07T18:30:41Z","receivedAt":"2016-10-07T18:30:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Oct 7, 2016 at 8:06 AM, Heiko Voigt wrote:\n> We run a command for each sha1 change in a submodule. This is\n> unnecessary since we can simply batch all sha1's we want to check into\n> one command. Lets do it so we can speedup the check when many submodule\n> changes are in need of checking.\n>\n> Signed-off-by: Heiko Voigt \n> ---\n> submodule.c | 63 +++++++++++++++++++++++++++++++++----------------------------\n> 1 file changed, 34 insertions(+), 29 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index 5044afc2f8..a05c2a34b1 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -529,27 +529,49 @@ static int append_hash_to_argv(const unsigned char sha1[20], void *data)\n> return 0;\n> }\n>\n> -static int submodule_needs_pushing(const char *path, const unsigned char sha1[20])\n> +static int check_has_hash(const unsigned char sha1[20], void *data)\n> {\n> - if (add_submodule_odb(path) || !lookup_commit_reference(sha1))\n> + int *has_hash = (int *) data;\n> +\n> + if (!lookup_commit_reference(sha1))\n> + *has_hash = 0;\n> +\n> + return 0;\n> +}\n> +\n> +static int submodule_has_hashes(const char *path, struct sha1_array *hashes)\n> +{\n> + int has_hash = 1;\n> +\n> + if (add_submodule_odb(path))\n> + return 0;\n> +\n> + sha1_array_for_each_unique(hashes, check_has_hash, &has_hash);\n> + return has_hash;\n> +}\n> +\n> +static int submodule_needs_pushing(const char *path, struct sha1_array *hashes)\n> +{\n> + if (!submodule_has_hashes(path, hashes))\n\nSo the above is an implicit lookup already, but we did that before,\ntoo, so it's fine.\n\n> @@ -658,13 +665,11 @@ int find_unpushed_submodules(struct sha1_array *hashes,\n> argv_array_clear(&argv);\n>\n> for (i = 0; i < submodules.nr; i++) {\n> - struct string_list_item *item = &submodules.items[i];\n> - struct collect_submodule_from_sha1s_data data;\n> - data.submodule_path = item->string;\n> - data.needs_pushing = needs_pushing;\n> - sha1_array_for_each_unique((struct sha1_array *) item->util,\n> - collect_submodules_from_sha1s,\n> - &data);\n> + struct string_list_item *submodule = &submodules.items[i];\n> + struct sha1_array *hashes = (struct sha1_array *) submodule->util;\n> +\n> + if (submodule_needs_pushing(submodule->string, hashes))\n> + string_list_insert(needs_pushing, submodule->string);\n\nThat makes sense.\n\nThanks!\nStefan\n"},{"id":"303815","messageId":"xmqqvawzbzb2.fsf@gitster.mtv.corp.google.com","threadId":"44245","inReplyTo":"CAGZ79kZiY56-84aThH1F02E_HzCTAK3KSYLbyP1D5GUAt892cw@mail.gmail.com","subject":"Re: [PATCH v2 1/3] serialize collection of changed submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-10T22:43:13Z","receivedAt":"2016-10-10T22:43:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> +static struct sha1_array *get_sha1s_from_list(struct string_list *submodules,\n>> + const char *path)\n>\n> So this will take the stringlist `submodules` and insert the path into it,\n> if it wasn't already in there. In case it is newly inserted, add a sha1_array\n> as util, so each inserted path has it's own empty array.\n>\n> So it is both init of the data structures as well as retrieving them. I was\n> initially confused by the name as I assumed it would give you sha1s out\n> of a string list (e.g. transform strings to internal sha1 things).\n> Maybe it's just\n> me having a hard time to understand that, but I feel like the name could be\n> improved.\n>\n> lookup_sha1_list_by_path,\n> insert_path_and_return_sha1_list ?\n\nI do not think either the name or the \"find if exists otherwise\ninitialize one\" behaviour is particularly confusing, but I do not\nthink \"maintain a set of sha1_arrays keyed with a string\" is a so\nwidely reusable general concept/construct. As can be seen easily in\nthe names of parameters, this function is about maintaining a set of\nsha1_arrays keyed by paths to submodules, and I also assume that the\narray indexed by path is not meant to be a general purpose \"we can\nuse it to store any 40-hex thing\" but to store something specific.\n\nWhat is that specific thing? The names of commit objects in the\nsubmodule repository?\n\nI'd prefer to see that exact thing used to construct the function\nname for a helper function with specific usage in mind, i.e.\nget_commit_object_names_for_submodule_path() or something along that\nline.\n"},{"id":"303816","messageId":"xmqqpon7bz26.fsf@gitster.mtv.corp.google.com","threadId":"44245","inReplyTo":"81bdbf6a1295c17c1b9233c91da6e5eb4583785e.1475851621.git.hvoigt@hvoigt.net","subject":"Re: [PATCH v2 2/3] serialize collection of refs that contain submodule changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-10T22:48:33Z","receivedAt":"2016-10-10T22:48:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt writes:\n\n> +static int append_hash_to_argv(const unsigned char sha1[20], void *data)\n> +{\n> +\tstruct argv_array *argv = (struct argv_array *) data;\n> +\targv_array_push(argv, sha1_to_hex(sha1));\n> +\treturn 0;\n> +}\n\nDo we have struct object_id readily available in the caller?\n\n ... goes and looks ...\n\nNo, this is part of sha1_array API, so this callback is perfectly\nfine.\n"},{"id":"303817","messageId":"xmqqlgxvbype.fsf@gitster.mtv.corp.google.com","threadId":"44245","inReplyTo":"67d4c48dc0129f20041c88d27a49c7a21188c882.1475851621.git.hvoigt@hvoigt.net","subject":"Re: [PATCH v2 3/3] batch check whether submodule needs pushing into one call","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-10T22:56:13Z","receivedAt":"2016-10-10T22:56:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt writes:\n\n> -static int submodule_needs_pushing(const char *path, const unsigned char sha1[20])\n> +static int check_has_hash(const unsigned char sha1[20], void *data)\n> {\n> -\tif (add_submodule_odb(path) || !lookup_commit_reference(sha1))\n> +\tint *has_hash = (int *) data;\n> +\n> +\tif (!lookup_commit_reference(sha1))\n> +\t\t*has_hash = 0;\n> +\n> +\treturn 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\nSame comment about naming. \n\nWhat do check-has-hash and submodule-has-hashes exactly mean by\n\"hash\" in their names? Because I think what is checked here is\n\"does the local submodule repository have _all_ the commits\nreferenced from the superproject commit we are pushing?\", so I'd\nprefer to see \"commit\" in their names.\n\nIf we do not even have these commits locally, then there is no point\nattempting to push, so returning 0 (i.e. it is not \"needs pushing\"\nsituation) is correct but it is a but subtle. It's not \"we know\nthey already have them\", but it is \"even if we tried to push, it\nwon't do us or the other side any good.\" A single-liner in-code\ncomment may help.\n\nThanks.\n"},{"id":"303982","messageId":"20161012130034.GA84247@book.hvoigt.net","threadId":"44245","inReplyTo":"xmqqvawzbzb2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 1/3] serialize collection of changed submodules","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-10-12T13:00:34Z","receivedAt":"2016-10-12T13:01:41Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Mon, Oct 10, 2016 at 03:43:13PM -0700, Junio C Hamano wrote:\n> Stefan Beller writes:\n> \n> >> +static struct sha1_array *get_sha1s_from_list(struct string_list *submodules,\n> >> + const char *path)\n> >\n> > So this will take the stringlist `submodules` and insert the path into it,\n> > if it wasn't already in there. In case it is newly inserted, add a sha1_array\n> > as util, so each inserted path has it's own empty array.\n> >\n> > So it is both init of the data structures as well as retrieving them. I was\n> > initially confused by the name as I assumed it would give you sha1s out\n> > of a string list (e.g. transform strings to internal sha1 things).\n> > Maybe it's just\n> > me having a hard time to understand that, but I feel like the name could be\n> > improved.\n> >\n> > lookup_sha1_list_by_path,\n> > insert_path_and_return_sha1_list ?\n> \n> I do not think either the name or the \"find if exists otherwise\n> initialize one\" behaviour is particularly confusing, but I do not\n> think \"maintain a set of sha1_arrays keyed with a string\" is a so\n> widely reusable general concept/construct. As can be seen easily in\n> the names of parameters, this function is about maintaining a set of\n> sha1_arrays keyed by paths to submodules, and I also assume that the\n> array indexed by path is not meant to be a general purpose \"we can\n> use it to store any 40-hex thing\" but to store something specific.\n> \n> What is that specific thing? The names of commit objects in the\n> submodule repository?\n> \n> I'd prefer to see that exact thing used to construct the function\n> name for a helper function with specific usage in mind, i.e.\n> get_commit_object_names_for_submodule_path() or something along that\n> line.\n\nI did not name this function too precisely to keep it's name short since\neverything specific was quite long, like the suggestion from Junio.\n\nSince this is a static function local to the submodule file I was\nassuming anyone interested would just look up the usage and immediately\nsee the purpose. If I look into submodule-cache.c where I have a similar\nfunctionality we used 'lookup_or_create' for this create on demand\nfunctionality. So a function name would be:\n\n\tlookup_or_create_commit_objects_for_submodule_path(...\n\nWhich seems quite extensively long for a static function so how about\nwe shorten it a bit and add a comment:\n\n\t/* lookup or create commit object list for submodule */\n\tget_commit_objects_for_submodule_path(...\n\n?\n\nCheers Heiko\n"},{"id":"303984","messageId":"20161012131027.GB84247@book.hvoigt.net","threadId":"44245","inReplyTo":"CAGZ79kZ5CPTuW2fARDs3BUt89b8H_=P3otZv+Vm5nTV70NLWtg@mail.gmail.com","subject":"Re: [PATCH v2 2/3] serialize collection of refs that contain submodule changes","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-10-12T13:10:27Z","receivedAt":"2016-10-12T13:11:06Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Fri, Oct 07, 2016 at 11:16:31AM -0700, Stefan Beller wrote:\n> > diff --git a/submodule.c b/submodule.c\n> > index 59c9d15905..5044afc2f8 100644\n> > --- a/submodule.c\n> > +++ b/submodule.c\n> > @@ -522,6 +522,13 @@ static int has_remote(const char *refname, const struct object_id *oid,\n> > return 1;\n> > }\n> >\n> > +static int append_hash_to_argv(const unsigned char sha1[20], void *data)\n> > +{\n> > + struct argv_array *argv = (struct argv_array *) data;\n> > + argv_array_push(argv, sha1_to_hex(sha1));\n> \n> Nit of the day:\n> When using the struct child-process, we have the oldstyle argv NULL\n> terminated array as\n> well as the new style args argv_array. So in that context we'd prefer\n> `args` as a name for\n> argv_array as that helps to distinguish from the old array type.\n> Here however `argv` seems to be a reasonable name, in fact whenever we\n> do not deal with\n> child processes, we seem to not like the `args` name:\n> \n> $ git grep argv_array |wc -l\n> 577\n> $ git grep argv_array |grep args |wc -l\n> 293\n> \n> The rest looks good to me. :)\n\nThanks. So I do not completely get what you are suggesting: args or kept\nit the way it is? Since in the end you are saying it is ok here ;) I\nmainly chose this name because I am substituting the argv variable which\nis already called 'argv' with this array. That might also be the reason\nwhy in so many locations with struct child_processe's we have the 'argv'\nname: Because they initially started with the old-style NULL terminated\narray.\n\nI am fine with it either way. Just tell me what you like :)\n\nCheers Heiko\n"},{"id":"303985","messageId":"20161012131149.GC84247@book.hvoigt.net","threadId":"44245","inReplyTo":"CAGZ79kZiY56-84aThH1F02E_HzCTAK3KSYLbyP1D5GUAt892cw@mail.gmail.com","subject":"Re: [PATCH v2 1/3] serialize collection of changed submodules","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-10-12T13:11:49Z","receivedAt":"2016-10-12T13:15:27Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Fri, Oct 07, 2016 at 10:59:29AM -0700, Stefan Beller wrote:\n> On Fri, Oct 7, 2016 at 8:06 AM, Heiko Voigt wrote:\n> > +static void free_submodules_sha1s(struct string_list *submodules)\n> > +{\n> > + int i;\n> > + for (i = 0; i < submodules->nr; i++) {\n> > + struct string_list_item *item = &submodules->items[i];\n> \n> You do not seem to make use of `i` explicitely, so\n> for_each_string_list_item might be more readable here?\n\nWill change.\n\n> > @@ -603,12 +645,23 @@ int find_unpushed_submodules(unsigned char new_sha1[20],\n> > die(\"revision walk setup failed\");\n> >\n> > while ((commit = get_revision(&rev)) != NULL)\n> > - find_unpushed_submodule_commits(commit, needs_pushing);\n> > + find_unpushed_submodule_commits(commit, &submodules);\n> >\n> > reset_revision_walk();\n> > free(sha1_copy);\n> > strbuf_release(&remotes_arg);\n> >\n> > + for (i = 0; i < submodules.nr; i++) {\n> > + struct string_list_item *item = &submodules.items[i];\n> \n> You do not seem to make use of `i` explicitely, so\n> for_each_string_list_item might be more readable here?\n\nAs above.\n\nCheers Heiko\n"},{"id":"303987","messageId":"20161012133338.GD84247@book.hvoigt.net","threadId":"44245","inReplyTo":"xmqqlgxvbype.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 3/3] batch check whether submodule needs pushing into one call","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-10-12T13:33:38Z","receivedAt":"2016-10-12T13:35:07Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Mon, Oct 10, 2016 at 03:56:13PM -0700, Junio C Hamano wrote:\n> Heiko Voigt writes:\n> \n> > -static int submodule_needs_pushing(const char *path, const unsigned char sha1[20])\n> > +static int check_has_hash(const unsigned char sha1[20], void *data)\n> > {\n> > -\tif (add_submodule_odb(path) || !lookup_commit_reference(sha1))\n> > +\tint *has_hash = (int *) data;\n> > +\n> > +\tif (!lookup_commit_reference(sha1))\n> > +\t\t*has_hash = 0;\n> > +\n> > +\treturn 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> Same comment about naming. \n> \n> What do check-has-hash and submodule-has-hashes exactly mean by\n> \"hash\" in their names? Because I think what is checked here is\n> \"does the local submodule repository have _all_ the commits\n> referenced from the superproject commit we are pushing?\", so I'd\n> prefer to see \"commit\" in their names.\n> \n> If we do not even have these commits locally, then there is no point\n> attempting to push, so returning 0 (i.e. it is not \"needs pushing\"\n> situation) is correct but it is a but subtle. It's not \"we know\n> they already have them\", but it is \"even if we tried to push, it\n> won't do us or the other side any good.\" A single-liner in-code\n> comment may help.\n\nFirst the naming part. How about:\n\n\tsubmodule_has_commits()\n\n?\n\nSecond as mentioned a previous answer[1] to this part: I would actually\nlike to have a die() here instead of blindly proceeding. Since the user\neither specified --recurse-submodules=... at the commandline or it was\nimplicitly enabled because we have submodules in the tree we should be\ncareful and not push revisions referencing submodules that are not\navailable at a remote. If we can not properly figure it out I would\nsuggest to stop and tell the user how to solve the situation. E.g.\neither she clones the appropriate submodules or specifies\n--no-recurse-submodules on the commandline to tell git that she does not\ncare.\n\nReturning 0 here means: \"No push needed\" but the correct answer would\nbe: \"We do not know\". Question is what we should do here which I am\nplanning to address in a separate patch series since that will be\nchanging behavior.\n\nSo how about:\n\n\n\tif (!submodule_has_hashes(path, hashes))\n\t\t/* NEEDSWORK: The correct answer here is \"We do not\n\t\t * know\" instead of \"No\". We currently proceed pushing\n\t\t * here as if the submodules commits are available on a\n\t\t * remote, which is not always correct. */\n\t\treturn 0;\n\nWhat do you think?\n\nCheers Heiko\n\n[1] http://public-inbox.org/git/20160919195812.GC62429@book.hvoigt.net/\n"},{"id":"304014","messageId":"xmqq1szl5vvf.fsf@gitster.mtv.corp.google.com","threadId":"44245","inReplyTo":"20161012130034.GA84247@book.hvoigt.net","subject":"Re: [PATCH v2 1/3] serialize collection of changed submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-12T17:18:28Z","receivedAt":"2016-10-12T17:20:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt writes:\n\n> Which seems quite extensively long for a static function so how about\n> we shorten it a bit and add a comment:\n>\n> \t/* lookup or create commit object list for submodule */\n> \tget_commit_objects_for_submodule_path(...\n\nOr you can even lose \"get_\" and \"path\", I guess. You are not even\n\"getting\" commits but the array that holds them, so the caller can\nuse it to \"get\" one of them or it can even use it to \"put\" a new\none, no? \"get-commit-objects\" is a misnomer in that sense. Either\none of\n\n get_submodule_commits_array()\n submodule_commits()\n\nperhaps? I dunno.\n"},{"id":"304016","messageId":"xmqqwphd4gf6.fsf@gitster.mtv.corp.google.com","threadId":"44245","inReplyTo":"20161012133338.GD84247@book.hvoigt.net","subject":"Re: [PATCH v2 3/3] batch check whether submodule needs pushing into one call","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-12T17:37:33Z","receivedAt":"2016-10-12T17:37:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt writes:\n\n>> If we do not even have these commits locally, then there is no point\n>> attempting to push, so returning 0 (i.e. it is not \"needs pushing\"\n>> situation) is correct but it is a but subtle. It's not \"we know\n>> they already have them\", but it is \"even if we tried to push, it\n>> won't do us or the other side any good.\" A single-liner in-code\n>> comment may help.\n>\n> First the naming part. How about:\n>\n> \tsubmodule_has_commits()\n\nNice.\n\n> Returning 0 here means: \"No push needed\" but the correct answer would\n> be: \"We do not know\". \n\nIs it? Perhaps I am misreading the \"submodule-has-commits\"; I\nthought it was \"the remote may or may not need updating, but we\nourselves don't have what they may need to have commits in their\nsubmodule that are referenced by their superproject, so it would not\nhelp them even if we pushed our submodule to them\". It indeed is\ndifferent from \"No push needed\" (rather, \"our pushing would be\npointless\").\n\n> So how about:\n>\n>\n> \tif (!submodule_has_hashes(path, hashes))\n> \t\t/* NEEDSWORK: The correct answer here is \"We do not\n> \t\t * know\" instead of \"No\". We currently proceed pushing\n> \t\t * here as if the submodules commits are available on a\n> \t\t * remote, which is not always correct. */\n> \t\treturn 0;\n\nI am not sure. \n\nWhat should happen in this scenario?\n\n * We have two remotes, A and B, for our superproject.\n\n * We are not interested in one submodule at path X. Our repository\n is primarily used to work on the superproject and possibly other\n submodules but not the one at path X.\n\n * We pulled from A to update ourselves. They were actively working\n on the submodule we are not interested in, and path X in the\n superproject records a new commit that we do not have.\n\n * We are now trying to push to B.\n\nShould different things happen in these two subcases?\n\n - We are not interested in submodule at path X, so we haven't even\n done \"submodule init\" on it.\n\n - We are not interested in submodule at path X, so even though we\n do have a rather stale clone of it, we do not usually bother\n updating what is checked out at path X and commit our changes\n outside that area.\n\nI tend to think that in these two cases the same thing should\nhappen. I am not sure if that same thing should be rejection\n(i.e. \"you do not know for sure that the commit at path X of the\nsuperproject you are pushing exists in the submodule repository at\nthe receiving end, so I'd refuse to push the superproject\"), as it\nmakes the only remedy for the situation is for you to make a full\nclone of the submodule you are not interested in and you have never\ntouched yourself in either of these two subcases.\n\n\n"},{"id":"304078","messageId":"20161013152745.GA29710@book.hvoigt.net","threadId":"44245","inReplyTo":"xmqq1szl5vvf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 1/3] serialize collection of changed submodules","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-10-13T15:27:45Z","receivedAt":"2016-10-13T15:28:00Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Wed, Oct 12, 2016 at 10:18:28AM -0700, Junio C Hamano wrote:\n> Heiko Voigt writes:\n> \n> > Which seems quite extensively long for a static function so how about\n> > we shorten it a bit and add a comment:\n> >\n> > \t/* lookup or create commit object list for submodule */\n> > \tget_commit_objects_for_submodule_path(...\n> \n> Or you can even lose \"get_\" and \"path\", I guess. You are not even\n> \"getting\" commits but the array that holds them, so the caller can\n> use it to \"get\" one of them or it can even use it to \"put\" a new\n> one, no? \"get-commit-objects\" is a misnomer in that sense. Either\n> one of\n> \n> get_submodule_commits_array()\n> submodule_commits()\n> \n> perhaps? I dunno.\n\nI like the last one. Will use 'submodule_commits()'.\n\nCheers Heiko\n"},{"id":"304084","messageId":"20161013155929.GB29710@book.hvoigt.net","threadId":"44245","inReplyTo":"xmqqwphd4gf6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 3/3] batch check whether submodule needs pushing into one call","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2016-10-13T15:59:29Z","receivedAt":"2016-10-13T16:19:53Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Wed, Oct 12, 2016 at 10:37:33AM -0700, Junio C Hamano wrote:\n> Heiko Voigt writes:\n> \n> >> If we do not even have these commits locally, then there is no point\n> >> attempting to push, so returning 0 (i.e. it is not \"needs pushing\"\n> >> situation) is correct but it is a but subtle. It's not \"we know\n> >> they already have them\", but it is \"even if we tried to push, it\n> >> won't do us or the other side any good.\" A single-liner in-code\n> >> comment may help.\n> >\n> > First the naming part. How about:\n> >\n> > \tsubmodule_has_commits()\n> \n> Nice.\n\nOk will use that. And while I am at it: I will also rename all the\n'hashes' variables to commits because that makes the code way clearer I\nthink.\n\n> > Returning 0 here means: \"No push needed\" but the correct answer would\n> > be: \"We do not know\". \n> \n> Is it? Perhaps I am misreading the \"submodule-has-commits\"; I\n> thought it was \"the remote may or may not need updating, but we\n> ourselves don't have what they may need to have commits in their\n> submodule that are referenced by their superproject, so it would not\n> help them even if we pushed our submodule to them\". It indeed is\n> different from \"No push needed\" (rather, \"our pushing would be\n> pointless\").\n\nYes you could also rephrase/see it that way. But the question is: If we\ndo not have what the remote needs would the user expect us to tell him\nthat fact and stop or does he usually not care?\n\n> > So how about:\n> >\n> >\n> > \tif (!submodule_has_hashes(path, hashes))\n> > \t\t/* NEEDSWORK: The correct answer here is \"We do not\n> > \t\t * know\" instead of \"No\". We currently proceed pushing\n> > \t\t * here as if the submodules commits are available on a\n> > \t\t * remote, which is not always correct. */\n> > \t\treturn 0;\n> \n> I am not sure. \n> \n> What should happen in this scenario?\n> \n> * We have two remotes, A and B, for our superproject.\n> \n> * We are not interested in one submodule at path X. Our repository\n> is primarily used to work on the superproject and possibly other\n> submodules but not the one at path X.\n> \n> * We pulled from A to update ourselves. They were actively working\n> on the submodule we are not interested in, and path X in the\n> superproject records a new commit that we do not have.\n> \n> * We are now trying to push to B.\n\nI am not sure if this is a typical scenario? Well, if you are updating\nyour main branch from someone else and then push it to your own fork\nmaybe. You could specify --no-recurse-submodules for this case though.\nThe proper solution for this case would probably be something along the\nlines of 'submodule..fetchRecurseSubmodules' but for push so we\ncan mark certain submodules as uninteresting by default.\n\nI like to be more protective to the user here. Its usually more\nannoying for possibly many others when you push out things that have\nmissing things compared to one person not being able to push because his\nsubmodule is not up-to-date/initialized.\n\n> Should different things happen in these two subcases?\n> \n> - We are not interested in submodule at path X, so we haven't even\n> done \"submodule init\" on it.\n> \n> - We are not interested in submodule at path X, so even though we\n> do have a rather stale clone of it, we do not usually bother\n> updating what is checked out at path X and commit our changes\n> outside that area.\n> \n> I tend to think that in these two cases the same thing should\n> happen. I am not sure if that same thing should be rejection\n> (i.e. \"you do not know for sure that the commit at path X of the\n> superproject you are pushing exists in the submodule repository at\n> the receiving end, so I'd refuse to push the superproject\"), as it\n> makes the only remedy for the situation is for you to make a full\n> clone of the submodule you are not interested in and you have never\n> touched yourself in either of these two subcases.\n\nI also think in both situations the same thing should happen. A decision\nthat something different should happen should be made explicitly instead\nof implicitly just because some submodule is not initialized. That might\nbe by accident or because a certain submodule is new so here the choice\nshould be made deliberately by the user, IMO.\n\nCheers Heiko\n"},{"id":"304544","messageId":"CAGZ79kZi+7=5auvU90weSHje_DrtPyyMbUadOveKa-5MzjGQFQ@mail.gmail.com","threadId":"44245","inReplyTo":"20161012131027.GB84247@book.hvoigt.net","subject":"Re: [PATCH v2 2/3] serialize collection of refs that contain submodule changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-10-20T23:00:43Z","receivedAt":"2016-10-20T23:00:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> Thanks. So I do not completely get what you are suggesting: args or kept\n> it the way it is? Since in the end you are saying it is ok here ;) I\n> mainly chose this name because I am substituting the argv variable which\n> is already called 'argv' with this array. That might also be the reason\n> why in so many locations with struct child_processe's we have the 'argv'\n> name: Because they initially started with the old-style NULL terminated\n> array.\n>\n> I am fine with it either way. Just tell me what you like :)\n\nI think it's fine as is here; I was just confused when first seeing this code.\n\n>\n> Cheers Heiko\n"}]}