{"thread":{"id":"45827","subject":"[PATCH 1/6] submodule: rename add_sha1_to_array","startedAt":"2017-04-28T23:54:33Z","lastAt":"2017-05-02T19:30:52Z","messageCount":33,"participants":["Brandon Williams","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"318219","messageId":"20170428235402.162251-2-bmwill@google.com","threadId":"45827","inReplyTo":"20170428235402.162251-1-bmwill@google.com","subject":"[PATCH 1/6] submodule: rename add_sha1_to_array","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-04-28T23:53:57Z","receivedAt":"2017-04-28T23:54:33Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Rename 'add_sha1_to_array()' to 'append_oid_to_array()' to more\naccuratly describe what the function does since it handles 'struct\nobject_id' and not sha1 character arrays.\n\nSigned-off-by: Brandon Williams \n---\n submodule.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex d3299e29c..be0f5d847 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -951,17 +951,18 @@ static void submodule_collect_changed_cb(struct diff_queue_struct *q,\n \t}\n }\n \n-static int add_sha1_to_array(const char *ref, const struct object_id *oid,\n-\t\t\t int flags, void *data)\n+static int append_oid_to_array(const char *ref, const struct object_id *oid,\n+\t\t\t int flags, void *data)\n {\n-\toid_array_append(data, oid);\n+\tstruct oid_array *array = data;\n+\toid_array_append(array, oid);\n \treturn 0;\n }\n \n void check_for_new_submodule_commits(struct object_id *oid)\n {\n \tif (!initialized_fetch_ref_tips) {\n-\t\tfor_each_ref(add_sha1_to_array, &ref_tips_before_fetch);\n+\t\tfor_each_ref(append_oid_to_array, &ref_tips_before_fetch);\n \t\tinitialized_fetch_ref_tips = 1;\n \t}\n \n-- \n2.13.0.rc0.306.g87b477812d-goog\n\n"},{"id":"318220","messageId":"20170428235402.162251-1-bmwill@google.com","threadId":"45827","inReplyTo":null,"subject":"[PATCH 0/6] changed submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-04-28T23:53:56Z","receivedAt":"2017-04-28T23:54:35Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"A little bit ago I was looking for a function in our code base which would be\nable to give me a list of all the submodules which changed across a number of\ncommits. I stumbled upon some of that logic in both our recurisve fetch and\npull code paths. Problem is both of these code paths were performing almost\nidentical tasks, without sharing any code.\n\nThis series is meant as a cleanup to submodule.c in order to unify these two\ncode paths as well as make it easier for future callers to be able to query for\nsubmodules which have chagned across a number of commits.\n\nBrandon Williams (6):\n submodule: rename add_sha1_to_array\n submodule: rename free_submodules_sha1s\n submodule: remove add_oid_to_argv\n submodule: change string_list changed_submodule_paths\n submodule: improve submodule_has_commits\n submodule: refactor logic to determine changed submodules\n\n submodule.c | 295 ++++++++++++++++++++++++++++--------------------------------\n 1 file changed, 139 insertions(+), 156 deletions(-)\n\n-- \n2.13.0.rc0.306.g87b477812d-goog\n\n"},{"id":"318221","messageId":"20170428235402.162251-5-bmwill@google.com","threadId":"45827","inReplyTo":"20170428235402.162251-1-bmwill@google.com","subject":"[PATCH 4/6] submodule: change string_list changed_submodule_paths","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-04-28T23:54:00Z","receivedAt":"2017-04-28T23:54:36Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Eliminate a call to 'xstrdup()' by changing the string_list\n'changed_submodule_paths' to duplicated strings added to it.\n\nSigned-off-by: Brandon Williams \n---\n submodule.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 7baa28ae0..3bcf44521 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -20,7 +20,7 @@\n static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;\n static int config_update_recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n static int parallel_jobs = 1;\n-static struct string_list changed_submodule_paths = STRING_LIST_INIT_NODUP;\n+static struct string_list changed_submodule_paths = STRING_LIST_INIT_DUP;\n static int initialized_fetch_ref_tips;\n static struct oid_array ref_tips_before_fetch;\n static struct oid_array ref_tips_after_fetch;\n@@ -939,7 +939,7 @@ static void submodule_collect_changed_cb(struct diff_queue_struct *q,\n \t\t\tstruct string_list_item *path;\n \t\t\tpath = unsorted_string_list_lookup(&changed_submodule_paths, p->two->path);\n \t\t\tif (!path && !is_submodule_commit_present(p->two->path, p->two->oid.hash))\n-\t\t\t\tstring_list_append(&changed_submodule_paths, xstrdup(p->two->path));\n+\t\t\t\tstring_list_append(&changed_submodule_paths, p->two->path);\n \t\t} else {\n \t\t\t/* Submodule is new or was moved here */\n \t\t\t/* NEEDSWORK: When the .git directories of submodules\n-- \n2.13.0.rc0.306.g87b477812d-goog\n\n"},{"id":"318222","messageId":"20170428235402.162251-4-bmwill@google.com","threadId":"45827","inReplyTo":"20170428235402.162251-1-bmwill@google.com","subject":"[PATCH 3/6] submodule: remove add_oid_to_argv","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-04-28T23:53:59Z","receivedAt":"2017-04-28T23:54:37Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"The function 'add_oid_to_argv()' provides the same functionality as\n'append_oid_to_argv()'. Remove this duplicate function and instead use\n'append_oid_to_argv()' where 'add_oid_to_argv()' was previously used.\n\nSigned-off-by: Brandon Williams \n---\n submodule.c | 10 ++--------\n 1 file changed, 2 insertions(+), 8 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 46abd52b1..7baa28ae0 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -970,12 +970,6 @@ void check_for_new_submodule_commits(struct object_id *oid)\n \toid_array_append(&ref_tips_after_fetch, oid);\n }\n \n-static int add_oid_to_argv(const struct object_id *oid, void *data)\n-{\n-\targv_array_push(data, oid_to_hex(oid));\n-\treturn 0;\n-}\n-\n static void calculate_changed_submodule_paths(void)\n {\n \tstruct rev_info rev;\n@@ -989,10 +983,10 @@ static void calculate_changed_submodule_paths(void)\n \tinit_revisions(&rev, NULL);\n \targv_array_push(&argv, \"--\"); /* argv[0] program name */\n \toid_array_for_each_unique(&ref_tips_after_fetch,\n-\t\t\t\t add_oid_to_argv, &argv);\n+\t\t\t\t append_oid_to_argv, &argv);\n \targv_array_push(&argv, \"--not\");\n \toid_array_for_each_unique(&ref_tips_before_fetch,\n-\t\t\t\t add_oid_to_argv, &argv);\n+\t\t\t\t append_oid_to_argv, &argv);\n \tsetup_revisions(argv.argc, argv.argv, &rev, NULL);\n \tif (prepare_revision_walk(&rev))\n \t\tdie(\"revision walk setup failed\");\n-- \n2.13.0.rc0.306.g87b477812d-goog\n\n"},{"id":"318223","messageId":"20170428235402.162251-3-bmwill@google.com","threadId":"45827","inReplyTo":"20170428235402.162251-1-bmwill@google.com","subject":"[PATCH 2/6] submodule: rename free_submodules_sha1s","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-04-28T23:53:58Z","receivedAt":"2017-04-28T23:54:38Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Rename 'free_submodules_sha1s()' to 'free_submodules_oids()' since the\nfunction frees a 'struct string_list' which has a 'struct oid_array'\nstored in the 'util' field.\n\nSigned-off-by: Brandon Williams \n---\n submodule.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex be0f5d847..46abd52b1 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -738,7 +738,7 @@ static void find_unpushed_submodule_commits(struct commit *commit,\n \tdiff_tree_combined_merge(commit, 1, &rev);\n }\n \n-static void free_submodules_sha1s(struct string_list *submodules)\n+static void free_submodules_oids(struct string_list *submodules)\n {\n \tstruct string_list_item *item;\n \tfor_each_string_list_item(item, submodules)\n@@ -779,7 +779,8 @@ int find_unpushed_submodules(struct oid_array *commits,\n \t\tif (submodule_needs_pushing(submodule->string, commits))\n \t\t\tstring_list_insert(needs_pushing, submodule->string);\n \t}\n-\tfree_submodules_sha1s(&submodules);\n+\n+\tfree_submodules_oids(&submodules);\n \n \treturn needs_pushing->nr;\n }\n-- \n2.13.0.rc0.306.g87b477812d-goog\n\n"},{"id":"318224","messageId":"20170428235402.162251-6-bmwill@google.com","threadId":"45827","inReplyTo":"20170428235402.162251-1-bmwill@google.com","subject":"[PATCH 5/6] submodule: improve submodule_has_commits","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-04-28T23:54:01Z","receivedAt":"2017-04-28T23:54:40Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Teach 'submodule_has_commits()' to ensure that if a commit exists in a\nsubmodule, that it is also reachable from a ref.\n\nThis is a prepritory step prior to merging the logic which checkes for\nchanged submodules when fetching or pushing.\n\nSigned-off-by: Brandon Williams \n---\n submodule.c | 24 ++++++++++++++++++++++++\n 1 file changed, 24 insertions(+)\n\ndiff --git a/submodule.c b/submodule.c\nindex 3bcf44521..100d31d39 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -648,6 +648,30 @@ static int submodule_has_commits(const char *path, struct oid_array *commits)\n \t\treturn 0;\n \n \toid_array_for_each_unique(commits, check_has_commit, &has_commit);\n+\n+\tif (has_commit) {\n+\t\t/*\n+\t\t * Even if the submodule is checked out and the commit is\n+\t\t * present, make sure it is reachable from a ref.\n+\t\t */\n+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\tstruct strbuf out = STRBUF_INIT;\n+\n+\t\targv_array_pushl(&cp.args, \"rev-list\", \"-n\", \"1\", NULL);\n+\t\toid_array_for_each_unique(commits, append_oid_to_argv, &cp.args);\n+\t\targv_array_pushl(&cp.args, \"--not\", \"--all\", 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.dir = path;\n+\n+\t\tif (capture_command(&cp, &out, 1024) || out.len)\n+\t\t\thas_commit = 0;\n+\n+\t\tstrbuf_release(&out);\n+\t}\n+\n \treturn has_commit;\n }\n \n-- \n2.13.0.rc0.306.g87b477812d-goog\n\n"},{"id":"318225","messageId":"20170428235402.162251-7-bmwill@google.com","threadId":"45827","inReplyTo":"20170428235402.162251-1-bmwill@google.com","subject":"[PATCH 6/6] submodule: refactor logic to determine changed submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-04-28T23:54:02Z","receivedAt":"2017-04-28T23:54:49Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"There are currently two instances (fetch and push) where we want to\ndetermine if submodules have changed given some revision specification.\nThese two instances don't use the same logic to generate a list of\nchanged submodules and as a result there is a fair amount of code\nduplication.\n\nThis patch refactors these two code paths such that they both use the\nsame logic to generate a list of changed submodules. This also makes it\neasier for future callers to be able to reuse this logic as they only\nneed to create an argv_array with the revision specification to be using\nduring the revision walk.\n\nSigned-off-by: Brandon Williams \n---\n submodule.c | 247 ++++++++++++++++++++++++++----------------------------------\n 1 file changed, 105 insertions(+), 142 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 100d31d39..88093779e 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -617,6 +617,94 @@ const struct submodule *submodule_from_ce(const struct cache_entry *ce)\n \treturn submodule_from_path(null_sha1, ce->name);\n }\n \n+static struct oid_array *submodule_commits(struct string_list *submodules,\n+\t\t\t\t\t const 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 oid_array *) item->util;\n+\n+\t/* NEEDSWORK: should we have oid_array_init()? */\n+\titem->util = xcalloc(1, sizeof(struct oid_array));\n+\treturn (struct oid_array *) item->util;\n+}\n+\n+static void collect_changed_submodules_cb(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 *changed = data;\n+\n+\tfor (i = 0; i < q->nr; i++) {\n+\t\tstruct diff_filepair *p = q->queue[i];\n+\t\tstruct oid_array *commits;\n+\t\tif (!S_ISGITLINK(p->two->mode))\n+\t\t\tcontinue;\n+\n+\t\tif (S_ISGITLINK(p->one->mode)) {\n+\t\t\t/*\n+\t\t\t * NEEDSWORK: We should honor the name configured in\n+\t\t\t * the .gitmodules file of the commit we are examining\n+\t\t\t * here to be able to correctly follow submodules\n+\t\t\t * being moved around.\n+\t\t\t */\n+\t\t\tcommits = submodule_commits(changed, p->two->path);\n+\t\t\toid_array_append(commits, &p->two->oid);\n+\t\t} else {\n+\t\t\t/* Submodule is new or was moved here */\n+\t\t\t/*\n+\t\t\t * NEEDSWORK: When the .git directories of submodules\n+\t\t\t * live inside the superprojects .git directory some\n+\t\t\t * day we should fetch new submodules directly into\n+\t\t\t * that location too when config or options request\n+\t\t\t * that so they can be checked out from there.\n+\t\t\t */\n+\t\t\tcontinue;\n+\t\t}\n+\t}\n+}\n+\n+/*\n+ * Collect the paths of submodules in 'changed' which have changed based on\n+ * the revisions as specified in 'argv'. Each entry in 'changed' will also\n+ * have a corresponding 'struct oid_array' (in the 'util' field) which lists\n+ * what the submodule pointers were updated to during the change.\n+ */\n+static void collect_changed_submodules(struct string_list *changed,\n+\t\t\t\t struct argv_array *argv)\n+{\n+\tstruct rev_info rev;\n+\tconst struct commit *commit;\n+\n+\tinit_revisions(&rev, NULL);\n+\tsetup_revisions(argv->argc, argv->argv, &rev, NULL);\n+\tif (prepare_revision_walk(&rev))\n+\t\tdie(\"revision walk setup failed\");\n+\n+\twhile ((commit = get_revision(&rev))) {\n+\t\tstruct rev_info diff_rev;\n+\n+\t\tinit_revisions(&diff_rev, NULL);\n+\t\tdiff_rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;\n+\t\tdiff_rev.diffopt.format_callback = collect_changed_submodules_cb;\n+\t\tdiff_rev.diffopt.format_callback_data = changed;\n+\t\tdiff_tree_combined_merge(commit, 1, &diff_rev);\n+\t}\n+\n+\treset_revision_walk();\n+}\n+\n+static void free_submodules_oids(struct string_list *submodules)\n+{\n+\tstruct string_list_item *item;\n+\tfor_each_string_list_item(item, submodules)\n+\t\toid_array_clear((struct oid_array *) item->util);\n+\tstring_list_clear(submodules, 1);\n+}\n+\n static int has_remote(const char *refname, const struct object_id *oid,\n \t\t int flags, void *cb_data)\n {\n@@ -719,92 +807,31 @@ static int submodule_needs_pushing(const char *path, struct oid_array *commits)\n \treturn 0;\n }\n \n-static struct oid_array *submodule_commits(struct string_list *submodules,\n-\t\t\t\t\t const 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 oid_array *) item->util;\n-\n-\t/* NEEDSWORK: should we have oid_array_init()? */\n-\titem->util = xcalloc(1, sizeof(struct oid_array));\n-\treturn (struct oid_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 *submodules = data;\n-\n-\tfor (i = 0; i < q->nr; i++) {\n-\t\tstruct diff_filepair *p = q->queue[i];\n-\t\tstruct oid_array *commits;\n-\t\tif (!S_ISGITLINK(p->two->mode))\n-\t\t\tcontinue;\n-\t\tcommits = submodule_commits(submodules, p->two->path);\n-\t\toid_array_append(commits, &p->two->oid);\n-\t}\n-}\n-\n-static void find_unpushed_submodule_commits(struct commit *commit,\n-\t\tstruct string_list *needs_pushing)\n-{\n-\tstruct rev_info rev;\n-\n-\tinit_revisions(&rev, NULL);\n-\trev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;\n-\trev.diffopt.format_callback = collect_submodules_from_diff;\n-\trev.diffopt.format_callback_data = needs_pushing;\n-\tdiff_tree_combined_merge(commit, 1, &rev);\n-}\n-\n-static void free_submodules_oids(struct string_list *submodules)\n-{\n-\tstruct string_list_item *item;\n-\tfor_each_string_list_item(item, submodules)\n-\t\toid_array_clear((struct oid_array *) item->util);\n-\tstring_list_clear(submodules, 1);\n-}\n-\n int find_unpushed_submodules(struct oid_array *commits,\n \t\tconst char *remotes_name, struct string_list *needs_pushing)\n {\n-\tstruct rev_info rev;\n-\tstruct commit *commit;\n \tstruct string_list submodules = STRING_LIST_INIT_DUP;\n \tstruct string_list_item *submodule;\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n \n-\tinit_revisions(&rev, NULL);\n-\n \t/* argv.argv[0] will be ignored by setup_revisions */\n \targv_array_push(&argv, \"find_unpushed_submodules\");\n \toid_array_for_each_unique(commits, append_oid_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-\twhile ((commit = get_revision(&rev)) != NULL)\n-\t\tfind_unpushed_submodule_commits(commit, &submodules);\n-\n-\treset_revision_walk();\n-\targv_array_clear(&argv);\n+\tcollect_changed_submodules(&submodules, &argv);\n \n \tfor_each_string_list_item(submodule, &submodules) {\n-\t\tstruct oid_array *commits = (struct oid_array *) submodule->util;\n+\t\tstruct oid_array *commits = submodule->util;\n+\t\tconst char *path = submodule->string;\n \n-\t\tif (submodule_needs_pushing(submodule->string, commits))\n-\t\t\tstring_list_insert(needs_pushing, submodule->string);\n+\t\tif (submodule_needs_pushing(path, commits))\n+\t\t\tstring_list_insert(needs_pushing, path);\n \t}\n \n \tfree_submodules_oids(&submodules);\n+\targv_array_clear(&argv);\n \n \treturn needs_pushing->nr;\n }\n@@ -921,61 +948,6 @@ int push_unpushed_submodules(struct oid_array *commits,\n \treturn ret;\n }\n \n-static int is_submodule_commit_present(const char *path, unsigned char sha1[20])\n-{\n-\tint is_present = 0;\n-\tif (!add_submodule_odb(path) && lookup_commit_reference(sha1)) {\n-\t\t/* Even if the submodule is checked out and the commit is\n-\t\t * present, make sure it is reachable from a ref. */\n-\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\t\tconst char *argv[] = {\"rev-list\", \"-n\", \"1\", NULL, \"--not\", \"--all\", NULL};\n-\t\tstruct strbuf buf = STRBUF_INIT;\n-\n-\t\targv[3] = 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.dir = path;\n-\t\tif (!capture_command(&cp, &buf, 1024) && !buf.len)\n-\t\t\tis_present = 1;\n-\n-\t\tstrbuf_release(&buf);\n-\t}\n-\treturn is_present;\n-}\n-\n-static void submodule_collect_changed_cb(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-\tfor (i = 0; i < q->nr; i++) {\n-\t\tstruct diff_filepair *p = q->queue[i];\n-\t\tif (!S_ISGITLINK(p->two->mode))\n-\t\t\tcontinue;\n-\n-\t\tif (S_ISGITLINK(p->one->mode)) {\n-\t\t\t/* NEEDSWORK: We should honor the name configured in\n-\t\t\t * the .gitmodules file of the commit we are examining\n-\t\t\t * here to be able to correctly follow submodules\n-\t\t\t * being moved around. */\n-\t\t\tstruct string_list_item *path;\n-\t\t\tpath = unsorted_string_list_lookup(&changed_submodule_paths, p->two->path);\n-\t\t\tif (!path && !is_submodule_commit_present(p->two->path, p->two->oid.hash))\n-\t\t\t\tstring_list_append(&changed_submodule_paths, p->two->path);\n-\t\t} else {\n-\t\t\t/* Submodule is new or was moved here */\n-\t\t\t/* NEEDSWORK: When the .git directories of submodules\n-\t\t\t * live inside the superprojects .git directory some\n-\t\t\t * day we should fetch new submodules directly into\n-\t\t\t * that location too when config or options request\n-\t\t\t * that so they can be checked out from there. */\n-\t\t\tcontinue;\n-\t\t}\n-\t}\n-}\n-\n static int append_oid_to_array(const char *ref, const struct object_id *oid,\n \t\t\t int flags, void *data)\n {\n@@ -996,45 +968,36 @@ void check_for_new_submodule_commits(struct object_id *oid)\n \n static void calculate_changed_submodule_paths(void)\n {\n-\tstruct rev_info rev;\n-\tstruct commit *commit;\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n+\tstruct string_list changed_submodules = STRING_LIST_INIT_DUP;\n+\tconst struct string_list_item *item;\n \n \t/* No need to check if there are no submodules configured */\n \tif (!submodule_from_path(NULL, NULL))\n \t\treturn;\n \n-\tinit_revisions(&rev, NULL);\n \targv_array_push(&argv, \"--\"); /* argv[0] program name */\n \toid_array_for_each_unique(&ref_tips_after_fetch,\n \t\t\t\t append_oid_to_argv, &argv);\n \targv_array_push(&argv, \"--not\");\n \toid_array_for_each_unique(&ref_tips_before_fetch,\n \t\t\t\t append_oid_to_argv, &argv);\n-\tsetup_revisions(argv.argc, argv.argv, &rev, NULL);\n-\tif (prepare_revision_walk(&rev))\n-\t\tdie(\"revision walk setup failed\");\n \n \t/*\n \t * Collect all submodules (whether checked out or not) for which new\n \t * commits have been recorded upstream in \"changed_submodule_paths\".\n \t */\n-\twhile ((commit = get_revision(&rev))) {\n-\t\tstruct commit_list *parent = commit->parents;\n-\t\twhile (parent) {\n-\t\t\tstruct diff_options diff_opts;\n-\t\t\tdiff_setup(&diff_opts);\n-\t\t\tDIFF_OPT_SET(&diff_opts, RECURSIVE);\n-\t\t\tdiff_opts.output_format |= DIFF_FORMAT_CALLBACK;\n-\t\t\tdiff_opts.format_callback = submodule_collect_changed_cb;\n-\t\t\tdiff_setup_done(&diff_opts);\n-\t\t\tdiff_tree_sha1(parent->item->object.oid.hash, commit->object.oid.hash, \"\", &diff_opts);\n-\t\t\tdiffcore_std(&diff_opts);\n-\t\t\tdiff_flush(&diff_opts);\n-\t\t\tparent = parent->next;\n-\t\t}\n+\tcollect_changed_submodules(&changed_submodules, &argv);\n+\n+\tfor_each_string_list_item(item, &changed_submodules) {\n+\t\tstruct oid_array *commits = item->util;\n+\t\tconst char *path = item->string;\n+\n+\t\tif (!submodule_has_commits(path, commits))\n+\t\t\tstring_list_append(&changed_submodule_paths, path);\n \t}\n \n+\tfree_submodules_oids(&changed_submodules);\n \targv_array_clear(&argv);\n \toid_array_clear(&ref_tips_before_fetch);\n \toid_array_clear(&ref_tips_after_fetch);\n-- \n2.13.0.rc0.306.g87b477812d-goog\n\n"},{"id":"318227","messageId":"CAGZ79kZzpyw0ijiM12TwMHAx1Ung-xKPEm2rVY9803hvk=+UMQ@mail.gmail.com","threadId":"45827","inReplyTo":"20170428235402.162251-6-bmwill@google.com","subject":"Re: [PATCH 5/6] submodule: improve submodule_has_commits","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-29T00:28:14Z","receivedAt":"2017-04-29T00:28:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Apr 28, 2017 at 4:54 PM, Brandon Williams wrote:\n> Teach 'submodule_has_commits()' to ensure that if a commit exists in a\n> submodule, that it is also reachable from a ref.\n>\n> This is a prepritory step prior to merging the logic which checkes for\n\ns/prepritory/preparatory/\ns/checkes/checks/\n\nThis is the first commit in the series that changes user observable behavior,\nI guess there will be tests in a later patch? Can you elaborate in this commit\nmessage more about why it is useful (or at least harmless for performing\nthis check in the case of fetch/push)?\n\n> diff --git a/submodule.c b/submodule.c\n> index 3bcf44521..100d31d39 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -648,6 +648,30 @@ static int submodule_has_commits(const char *path, struct oid_array *commits)\n> return 0;\n>\n> oid_array_for_each_unique(commits, check_has_commit, &has_commit);\n> +\n> + if (has_commit) {\n> + /*\n> + * Even if the submodule is checked out and the commit is\n> + * present, make sure it is reachable from a ref.\n> + */\n> + struct child_process cp = CHILD_PROCESS_INIT;\n> + struct strbuf out = STRBUF_INIT;\n> +\n> + argv_array_pushl(&cp.args, \"rev-list\", \"-n\", \"1\", NULL);\n> + oid_array_for_each_unique(commits, append_oid_to_argv, &cp.args);\n> + argv_array_pushl(&cp.args, \"--not\", \"--all\", NULL);\n> +\n> + prepare_submodule_repo_env(&cp.env_array);\n> + cp.git_cmd = 1;\n> + cp.no_stdin = 1;\n> + cp.dir = path;\n> +\n> + if (capture_command(&cp, &out, 1024) || out.len)\n> + has_commit = 0;\n\nSo if we fail to launch capture_command, we assume we do not\nhave the commits?\n\ncapture_command can fail for reasons that are hard to track down\nor even spurious (OOM due to excessive output, disk failure,\ncorrupt repo, error in executing the process, getting a signal and\nso on), some of them are ok to ignore, others should never be ignored.\n\nSo I'd rather die on capture_command, and inspect out.len only\nin case of successful capturing.\n\nIn addition to that we're only interested if there is any output,\nsuch that we can optimize further:\nc.f. http://public-inbox.org/git/20170324223848.GH31294@aiede.mtv.corp.google.com/\n\n if (start_command(&cp))\n die(\"cannot start git-rev-list in submodule'%s', sub->path);\n\n /* read only one character, if any */\n if (xread(cp.out, &tmp, 1);\n has_commit = 0;\n\n /*\n * close cp.out, such that the child may get SIGPIPE, upon which\n * it dies (silently, maybe we need to suppress cp.err ?)\n */\n close(cp.out);\n\n /*\n * Though we need to be nitpicky about finish_command IIUC by now:\n * TODO(sbeller): if this turns out to be true, fixup is_submodule_modified\n */\n int code = finish_command(&cp);\n if (!code && code != 128 + SIGPIPE)\n die(\"git rev-list failed in submodule'%s'\", sub->path);\n\nUpon rereading the patch, I notice the '-n 1', which would make the\noptimized code above useless, so just consider it food for thought.\n\nThanks,\nStefan\n"},{"id":"318228","messageId":"CAGZ79kYqiSyxtpux77RSGx56Bzj3YA7Tu180=oFbPb1fMgEMkA@mail.gmail.com","threadId":"45827","inReplyTo":"20170428235402.162251-7-bmwill@google.com","subject":"Re: [PATCH 6/6] submodule: refactor logic to determine changed submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-29T00:53:46Z","receivedAt":"2017-04-29T00:53:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"+ Heiko, who touched the pushing code end of last year.\n\nOn Fri, Apr 28, 2017 at 4:54 PM, Brandon Williams wrote:\n> There are currently two instances (fetch and push) where we want to\n> determine if submodules have changed given some revision specification.\n> These two instances don't use the same logic to generate a list of\n> changed submodules and as a result there is a fair amount of code\n> duplication.\n>\n> This patch refactors these two code paths such that they both use the\n> same logic to generate a list of changed submodules. This also makes it\n> easier for future callers to be able to reuse this logic as they only\n> need to create an argv_array with the revision specification to be using\n> during the revision walk.\n\nThanks for doing some refactoring. :)\n\n> -static struct oid_array *submodule_commits(struct string_list *submodules,\n> - const char *path)\n> ...\n\n> -static void free_submodules_oids(struct string_list *submodules)\n> -{\n> ...\n\nThese are just moved north, no change in code.\nIf you want to be extra nice to reviewers this could have been an extra patch,\nas it makes reviewing easier if you just have to look at the lines of code with\none goal (\"shuffling code around, no change intended\" vs \"make a change\nto improve things with no bad side effects\")\n\n\n\n> +\n> +static void collect_changed_submodules_cb(struct diff_queue_struct *q,\n> + struct diff_options *options,\n> + void *data)\n> +{\n\nThis one combines both collect_submodules_from_diff and\nsubmodule_collect_changed_cb ?\n\n> @@ -921,61 +948,6 @@ int push_unpushed_submodules(struct oid_array *commits,\n> return ret;\n> }\n>\n> -static int is_submodule_commit_present(const char *path, unsigned char sha1[20])\n> -{\n> - int is_present = 0;\n> - if (!add_submodule_odb(path) && lookup_commit_reference(sha1)) {\n> - /* Even if the submodule is checked out and the commit is\n> - * present, make sure it is reachable from a ref. */\n> - struct child_process cp = CHILD_PROCESS_INIT;\n> - const char *argv[] = {\"rev-list\", \"-n\", \"1\", NULL, \"--not\", \"--all\", NULL};\n> - struct strbuf buf = STRBUF_INIT;\n> -\n> - argv[3] = sha1_to_hex(sha1);\n> - cp.argv = argv;\n> - prepare_submodule_repo_env(&cp.env_array);\n> - cp.git_cmd = 1;\n> - cp.no_stdin = 1;\n> - cp.dir = path;\n> - if (!capture_command(&cp, &buf, 1024) && !buf.len)\n> - is_present = 1;\n\nOh, I see where the nit in patch 5/6 is coming from. Another note\non that: The hint is way off. The hint should be on the order of\nGIT_SHA1_HEXSZ\n\n> int find_unpushed_submodules(struct oid_array *commits,\n> const char *remotes_name, struct string_list *needs_pushing)\n> ...\n\n> static void calculate_changed_submodule_paths(void)\n> ...\n\nThese are both nicely clean now.\n\nThanks,\nStefan\n"},{"id":"318271","messageId":"20170430231434.GA70010@google.com","threadId":"45827","inReplyTo":"CAGZ79kZzpyw0ijiM12TwMHAx1Ung-xKPEm2rVY9803hvk=+UMQ@mail.gmail.com","subject":"Re: [PATCH 5/6] submodule: improve submodule_has_commits","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-04-30T23:14:34Z","receivedAt":"2017-04-30T23:15:03Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 04/28, Stefan Beller wrote:\n> On Fri, Apr 28, 2017 at 4:54 PM, Brandon Williams wrote:\n> > Teach 'submodule_has_commits()' to ensure that if a commit exists in a\n> > submodule, that it is also reachable from a ref.\n> >\n> > This is a prepritory step prior to merging the logic which checkes for\n> \n> s/prepritory/preparatory/\n> s/checkes/checks/\n> \n> This is the first commit in the series that changes user observable behavior,\n> I guess there will be tests in a later patch? Can you elaborate in this commit\n> message more about why it is useful (or at least harmless for performing\n> this check in the case of fetch/push)?\n\nThis hunk of logic is essentially a copy and paste from elsewhere in the\nfile. Essentially both code paths were essentially doing the same thing\n(checking if a submodule has a commit) but one of the code paths included\nthis logic to ensure that it was reachable from a ref in the submodule.\nIn order to merge the two code paths, I included that logic here.\n\nMaybe it would make sense to squash this into the next patch as the\nplace where this was copied from is removed?\n\n-- \nBrandon Williams\n"},{"id":"318281","messageId":"xmqq1ss9jqsw.fsf@gitster.mtv.corp.google.com","threadId":"45827","inReplyTo":"20170428235402.162251-1-bmwill@google.com","subject":"Re: [PATCH 0/6] changed submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-01T01:42:39Z","receivedAt":"2017-05-01T01:42:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> A little bit ago I was looking for a function in our code base which would be\n> able to give me a list of all the submodules which changed across a number of\n> commits. I stumbled upon some of that logic in both our recurisve fetch and\n> pull code paths. Problem is both of these code paths were performing almost\n> identical tasks, without sharing any code.\n>\n> This series is meant as a cleanup to submodule.c in order to unify these two\n> code paths as well as make it easier for future callers to be able to query for\n> submodules which have chagned across a number of commits.\n\nYay. Nice to see code reduction.\n\n>\n> Brandon Williams (6):\n> submodule: rename add_sha1_to_array\n> submodule: rename free_submodules_sha1s\n> submodule: remove add_oid_to_argv\n> submodule: change string_list changed_submodule_paths\n> submodule: improve submodule_has_commits\n> submodule: refactor logic to determine changed submodules\n>\n> submodule.c | 295 ++++++++++++++++++++++++++++--------------------------------\n> 1 file changed, 139 insertions(+), 156 deletions(-)\n"},{"id":"318344","messageId":"xmqqvaplgt8r.fsf@gitster.mtv.corp.google.com","threadId":"45827","inReplyTo":"20170428235402.162251-2-bmwill@google.com","subject":"Re: [PATCH 1/6] submodule: rename add_sha1_to_array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-01T03:18:12Z","receivedAt":"2017-05-01T03:18:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> Rename 'add_sha1_to_array()' to 'append_oid_to_array()' to more\n> accuratly describe what the function does since it handles 'struct\n\naccurately. Will fix while queuing.\n\nMakes sense. Thanks.\n\n> object_id' and not sha1 character arrays.\n>\n> Signed-off-by: Brandon Williams \n> ---\n> submodule.c | 9 +++++----\n> 1 file changed, 5 insertions(+), 4 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index d3299e29c..be0f5d847 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -951,17 +951,18 @@ static void submodule_collect_changed_cb(struct diff_queue_struct *q,\n> \t}\n> }\n> \n> -static int add_sha1_to_array(const char *ref, const struct object_id *oid,\n> -\t\t\t int flags, void *data)\n> +static int append_oid_to_array(const char *ref, const struct object_id *oid,\n> +\t\t\t int flags, void *data)\n> {\n> -\toid_array_append(data, oid);\n> +\tstruct oid_array *array = data;\n> +\toid_array_append(array, oid);\n> \treturn 0;\n> }\n> \n> void check_for_new_submodule_commits(struct object_id *oid)\n> {\n> \tif (!initialized_fetch_ref_tips) {\n> -\t\tfor_each_ref(add_sha1_to_array, &ref_tips_before_fetch);\n> +\t\tfor_each_ref(append_oid_to_array, &ref_tips_before_fetch);\n> \t\tinitialized_fetch_ref_tips = 1;\n> \t}\n"},{"id":"318345","messageId":"xmqqr309gss1.fsf@gitster.mtv.corp.google.com","threadId":"45827","inReplyTo":"20170428235402.162251-5-bmwill@google.com","subject":"Re: [PATCH 4/6] submodule: change string_list changed_submodule_paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-01T03:28:14Z","receivedAt":"2017-05-01T03:45:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> Eliminate a call to 'xstrdup()' by changing the string_list\n> 'changed_submodule_paths' to duplicated strings added to it.\n>\n> Signed-off-by: Brandon Williams \n> ---\n> submodule.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index 7baa28ae0..3bcf44521 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -20,7 +20,7 @@\n> static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;\n> static int config_update_recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n> static int parallel_jobs = 1;\n> -static struct string_list changed_submodule_paths = STRING_LIST_INIT_NODUP;\n> +static struct string_list changed_submodule_paths = STRING_LIST_INIT_DUP;\n> static int initialized_fetch_ref_tips;\n> static struct oid_array ref_tips_before_fetch;\n> static struct oid_array ref_tips_after_fetch;\n> @@ -939,7 +939,7 @@ static void submodule_collect_changed_cb(struct diff_queue_struct *q,\n> \t\t\tstruct string_list_item *path;\n> \t\t\tpath = unsorted_string_list_lookup(&changed_submodule_paths, p->two->path);\n> \t\t\tif (!path && !is_submodule_commit_present(p->two->path, p->two->oid.hash))\n> -\t\t\t\tstring_list_append(&changed_submodule_paths, xstrdup(p->two->path));\n> +\t\t\t\tstring_list_append(&changed_submodule_paths, p->two->path);\n\nI notice that \"path\" is not used at all, and other users of this\nstring list do not even bother using a variable, i.e.\n\n\tif (!unsorted_string_list_lookup(&changed_submodule_paths, ...))\n\nIn fact, it might be even better to use a hashmap for this instead?\n\nThe call to string_list_clear() onthis list tells it to free the\nutil field, and I went to see what we are storing in the util field,\nbut it seems that it is freeing NULLs, which is somewhat misleading\nand time-wasting on the code readers. Using hashmap may also clear\nthis up.\n\nBut all of the above are not within the scope of this topic ;-)\n"},{"id":"318346","messageId":"xmqqmvaxgscr.fsf@gitster.mtv.corp.google.com","threadId":"45827","inReplyTo":"20170428235402.162251-6-bmwill@google.com","subject":"Re: [PATCH 5/6] submodule: improve submodule_has_commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-01T03:37:24Z","receivedAt":"2017-05-01T03:45:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> \toid_array_for_each_unique(commits, check_has_commit, &has_commit);\n> +\n> +\tif (has_commit) {\n> +\t\t/*\n> +\t\t * Even if the submodule is checked out and the commit is\n> +\t\t * present, make sure it is reachable from a ref.\n> +\t\t */\n> +\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\t\tstruct strbuf out = STRBUF_INIT;\n> +\n> +\t\targv_array_pushl(&cp.args, \"rev-list\", \"-n\", \"1\", NULL);\n> +\t\toid_array_for_each_unique(commits, append_oid_to_argv, &cp.args);\n> +\t\targv_array_pushl(&cp.args, \"--not\", \"--all\", 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.dir = path;\n> +\n> +\t\tif (capture_command(&cp, &out, 1024) || out.len)\n> +\t\t\thas_commit = 0;\n> +\n> +\t\tstrbuf_release(&out);\n> +\t}\n> +\n> \treturn has_commit;\n> }\n\nThe \"check-has-commit\" we see in the pre-context is \"we contaminated\nour in-core object store by tentatively borrowing from submodule's\nobject store---now do we see these commits in our in-core view?\"\nWhich is a wrong thing to do from two separate point of view. Even\nthough the commit in question may be visible in our contaminated\nview, there is no guarantee that the commit exists in the object\nstore of the correct submodule. And of course the commit may exist\nbut may not be anchored by any ref.\n\nThis patch fixes the latter, and if we remove that check-has-commit\ncall before it, we can fix the former at the same time.\n\nThere is value in leaving the check-has-commit code if we anticipate\nthat we would very often have to say \"no, the submodule does not\nhave these commits\"---a cheap but wrong check it does can be used as\nan optimization. If we do not have the commit object anywhere,\nthere is no chance we have it in the object store of the correct\nsubmodule and have it reachable from a ref, so we can fail without\nspawning rev-list which is expensive.\n\n\n"},{"id":"318375","messageId":"20170501163536.GB39135@google.com","threadId":"45827","inReplyTo":"xmqqr309gss1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/6] submodule: change string_list changed_submodule_paths","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-01T16:35:36Z","receivedAt":"2017-05-01T16:35:45Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 04/30, Junio C Hamano wrote:\n> Brandon Williams writes:\n> \n> > Eliminate a call to 'xstrdup()' by changing the string_list\n> > 'changed_submodule_paths' to duplicated strings added to it.\n> >\n> > Signed-off-by: Brandon Williams \n> > ---\n> > submodule.c | 4 ++--\n> > 1 file changed, 2 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/submodule.c b/submodule.c\n> > index 7baa28ae0..3bcf44521 100644\n> > --- a/submodule.c\n> > +++ b/submodule.c\n> > @@ -20,7 +20,7 @@\n> > static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;\n> > static int config_update_recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n> > static int parallel_jobs = 1;\n> > -static struct string_list changed_submodule_paths = STRING_LIST_INIT_NODUP;\n> > +static struct string_list changed_submodule_paths = STRING_LIST_INIT_DUP;\n> > static int initialized_fetch_ref_tips;\n> > static struct oid_array ref_tips_before_fetch;\n> > static struct oid_array ref_tips_after_fetch;\n> > @@ -939,7 +939,7 @@ static void submodule_collect_changed_cb(struct diff_queue_struct *q,\n> > \t\t\tstruct string_list_item *path;\n> > \t\t\tpath = unsorted_string_list_lookup(&changed_submodule_paths, p->two->path);\n> > \t\t\tif (!path && !is_submodule_commit_present(p->two->path, p->two->oid.hash))\n> > -\t\t\t\tstring_list_append(&changed_submodule_paths, xstrdup(p->two->path));\n> > +\t\t\t\tstring_list_append(&changed_submodule_paths, p->two->path);\n> \n> I notice that \"path\" is not used at all, and other users of this\n> string list do not even bother using a variable, i.e.\n> \n> \tif (!unsorted_string_list_lookup(&changed_submodule_paths, ...))\n> \n> In fact, it might be even better to use a hashmap for this instead?\n> \n> The call to string_list_clear() onthis list tells it to free the\n> util field, and I went to see what we are storing in the util field,\n> but it seems that it is freeing NULLs, which is somewhat misleading\n> and time-wasting on the code readers. Using hashmap may also clear\n> this up.\n> \n> But all of the above are not within the scope of this topic ;-)\n\nAll good good points which hopefully are resolved in a later patch. This\npatch exists mainly to factor out the change to make the string_list\nduplicate the strings added to it as opposed to have that change seem\nrandom and out of place in a later patch.\n\nMost of this logic itself is removed entirely later once it is unified\nwith the other code path which checked for changed submodules. This may\nbe out of context on this particular patch, but eventually an oid_array\nis stored in the 'util' field of the string list items. This oid_array\nholds all of the changes to the submodule pointers. This allows us to\nthen batch check for the existence of all the submodule commits instead\nof checking each change individually (which is what this code path\ncurrently does).\n\n-- \nBrandon Williams\n"},{"id":"318376","messageId":"20170501164615.GC39135@google.com","threadId":"45827","inReplyTo":"xmqqmvaxgscr.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 5/6] submodule: improve submodule_has_commits","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-01T16:46:15Z","receivedAt":"2017-05-01T16:51:01Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 04/30, Junio C Hamano wrote:\n> Brandon Williams writes:\n> \n> > \toid_array_for_each_unique(commits, check_has_commit, &has_commit);\n> > +\n> > +\tif (has_commit) {\n> > +\t\t/*\n> > +\t\t * Even if the submodule is checked out and the commit is\n> > +\t\t * present, make sure it is reachable from a ref.\n> > +\t\t */\n> > +\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> > +\t\tstruct strbuf out = STRBUF_INIT;\n> > +\n> > +\t\targv_array_pushl(&cp.args, \"rev-list\", \"-n\", \"1\", NULL);\n> > +\t\toid_array_for_each_unique(commits, append_oid_to_argv, &cp.args);\n> > +\t\targv_array_pushl(&cp.args, \"--not\", \"--all\", 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.dir = path;\n> > +\n> > +\t\tif (capture_command(&cp, &out, 1024) || out.len)\n> > +\t\t\thas_commit = 0;\n> > +\n> > +\t\tstrbuf_release(&out);\n> > +\t}\n> > +\n> > \treturn has_commit;\n> > }\n> \n> The \"check-has-commit\" we see in the pre-context is \"we contaminated\n> our in-core object store by tentatively borrowing from submodule's\n> object store---now do we see these commits in our in-core view?\"\n> Which is a wrong thing to do from two separate point of view. Even\n> though the commit in question may be visible in our contaminated\n> view, there is no guarantee that the commit exists in the object\n> store of the correct submodule. And of course the commit may exist\n> but may not be anchored by any ref.\n> \n> This patch fixes the latter, and if we remove that check-has-commit\n> call before it, we can fix the former at the same time.\n\nI noticed this when cleaning up this code but was unsure if I should\ndrop the \"check-has-commit\" bit.\n\n> \n> There is value in leaving the check-has-commit code if we anticipate\n> that we would very often have to say \"no, the submodule does not\n> have these commits\"---a cheap but wrong check it does can be used as\n> an optimization. If we do not have the commit object anywhere,\n> there is no chance we have it in the object store of the correct\n> submodule and have it reachable from a ref, so we can fail without\n> spawning rev-list which is expensive.\n\nMostly because it gave the code a way to fail quickly, of course I'm\nmaking the assumption that polluting the object store than then checking\nit is quicker than launching a child process (though I guess most things\nare cheaper than launching a process ;)\n\n-- \nBrandon Williams\n"},{"id":"318377","messageId":"20170501164951.GD39135@google.com","threadId":"45827","inReplyTo":"CAGZ79kYqiSyxtpux77RSGx56Bzj3YA7Tu180=oFbPb1fMgEMkA@mail.gmail.com","subject":"Re: [PATCH 6/6] submodule: refactor logic to determine changed submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-01T16:49:51Z","receivedAt":"2017-05-01T16:51:27Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 04/28, Stefan Beller wrote:\n> + Heiko, who touched the pushing code end of last year.\n> \n> On Fri, Apr 28, 2017 at 4:54 PM, Brandon Williams wrote:\n> > There are currently two instances (fetch and push) where we want to\n> > determine if submodules have changed given some revision specification.\n> > These two instances don't use the same logic to generate a list of\n> > changed submodules and as a result there is a fair amount of code\n> > duplication.\n> >\n> > This patch refactors these two code paths such that they both use the\n> > same logic to generate a list of changed submodules. This also makes it\n> > easier for future callers to be able to reuse this logic as they only\n> > need to create an argv_array with the revision specification to be using\n> > during the revision walk.\n> \n> Thanks for doing some refactoring. :)\n> \n> > -static struct oid_array *submodule_commits(struct string_list *submodules,\n> > - const char *path)\n> > ...\n> \n> > -static void free_submodules_oids(struct string_list *submodules)\n> > -{\n> > ...\n> \n> These are just moved north, no change in code.\n> If you want to be extra nice to reviewers this could have been an extra patch,\n> as it makes reviewing easier if you just have to look at the lines of code with\n> one goal (\"shuffling code around, no change intended\" vs \"make a change\n> to improve things with no bad side effects\")\n\nYeah I suppose, I was having a difficult time breaking this largest\npatch into multiple.\n\n> \n> > +\n> > +static void collect_changed_submodules_cb(struct diff_queue_struct *q,\n> > + struct diff_options *options,\n> > + void *data)\n> > +{\n> \n> This one combines both collect_submodules_from_diff and\n> submodule_collect_changed_cb ?\n\nYep.\n\n> \n> > @@ -921,61 +948,6 @@ int push_unpushed_submodules(struct oid_array *commits,\n> > return ret;\n> > }\n> >\n> > -static int is_submodule_commit_present(const char *path, unsigned char sha1[20])\n> > -{\n> > - int is_present = 0;\n> > - if (!add_submodule_odb(path) && lookup_commit_reference(sha1)) {\n> > - /* Even if the submodule is checked out and the commit is\n> > - * present, make sure it is reachable from a ref. */\n> > - struct child_process cp = CHILD_PROCESS_INIT;\n> > - const char *argv[] = {\"rev-list\", \"-n\", \"1\", NULL, \"--not\", \"--all\", NULL};\n> > - struct strbuf buf = STRBUF_INIT;\n> > -\n> > - argv[3] = sha1_to_hex(sha1);\n> > - cp.argv = argv;\n> > - prepare_submodule_repo_env(&cp.env_array);\n> > - cp.git_cmd = 1;\n> > - cp.no_stdin = 1;\n> > - cp.dir = path;\n> > - if (!capture_command(&cp, &buf, 1024) && !buf.len)\n> > - is_present = 1;\n> \n> Oh, I see where the nit in patch 5/6 is coming from. Another note\n> on that: The hint is way off. The hint should be on the order of\n> GIT_SHA1_HEXSZ\n\nYeah agreed. I make this change in the earlier patch.\n\n> \n> > int find_unpushed_submodules(struct oid_array *commits,\n> > const char *remotes_name, struct string_list *needs_pushing)\n> > ...\n> \n> > static void calculate_changed_submodule_paths(void)\n> > ...\n> \n> These are both nicely clean now.\n> \n> Thanks,\n> Stefan\n\n-- \nBrandon Williams\n"},{"id":"318378","messageId":"CAGZ79ka=gALFrzj1OpL9tifiS7eSOSVAMfxY3gD3T6o-teJuaA@mail.gmail.com","threadId":"45827","inReplyTo":"20170430231434.GA70010@google.com","subject":"Re: [PATCH 5/6] submodule: improve submodule_has_commits","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-01T16:52:20Z","receivedAt":"2017-05-01T16:52:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, Apr 30, 2017 at 4:14 PM, Brandon Williams wrote:\n\n> This hunk of logic is essentially a copy and paste from elsewhere in the\n> file. Essentially both code paths were essentially doing the same thing\n> (checking if a submodule has a commit) but one of the code paths included\n> this logic to ensure that it was reachable from a ref in the submodule.\n> In order to merge the two code paths, I included that logic here.\n\nyes, I get that. The question is whether the other code path omitted the check\nintentionally or just missed it (Is it a bug or feature to have this\ncheck in that code\npath now)?\n"},{"id":"318379","messageId":"20170501165515.GE39135@google.com","threadId":"45827","inReplyTo":"CAGZ79ka=gALFrzj1OpL9tifiS7eSOSVAMfxY3gD3T6o-teJuaA@mail.gmail.com","subject":"Re: [PATCH 5/6] submodule: improve submodule_has_commits","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-01T16:55:15Z","receivedAt":"2017-05-01T16:55:29Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/01, Stefan Beller wrote:\n> On Sun, Apr 30, 2017 at 4:14 PM, Brandon Williams wrote:\n> \n> > This hunk of logic is essentially a copy and paste from elsewhere in the\n> > file. Essentially both code paths were essentially doing the same thing\n> > (checking if a submodule has a commit) but one of the code paths included\n> > this logic to ensure that it was reachable from a ref in the submodule.\n> > In order to merge the two code paths, I included that logic here.\n> \n> yes, I get that. The question is whether the other code path omitted the check\n> intentionally or just missed it (Is it a bug or feature to have this\n> check in that code\n> path now)?\n\nI'd take a look at Junio's response. This check more than likely needs\nto be included.\n\n-- \nBrandon Williams\n"},{"id":"318437","messageId":"20170502010239.179369-1-bmwill@google.com","threadId":"45827","inReplyTo":"20170428235402.162251-1-bmwill@google.com","subject":"[PATCH v2 0/6] changed submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-02T01:02:33Z","receivedAt":"2017-05-02T01:02:49Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Changes in v2:\n- few tweaks in the commit messages of a couple of the commits\n- use 'GIT_MAX_HEXSZ + 1' as the hint size in [5/6]\n- added a comment in [5/6] better explaining the rational for having a quick,\n incorrect check for the existence of a commit in a submodule.\n\nBrandon Williams (6):\n submodule: rename add_sha1_to_array\n submodule: rename free_submodules_sha1s\n submodule: remove add_oid_to_argv\n submodule: change string_list changed_submodule_paths\n submodule: improve submodule_has_commits\n submodule: refactor logic to determine changed submodules\n\n submodule.c | 305 +++++++++++++++++++++++++++++-------------------------------\n 1 file changed, 149 insertions(+), 156 deletions(-)\n\n-- \n2.13.0.rc1.294.g07d810a77f-goog\n\n"},{"id":"318438","messageId":"20170502010239.179369-2-bmwill@google.com","threadId":"45827","inReplyTo":"20170502010239.179369-1-bmwill@google.com","subject":"[PATCH v2 1/6] submodule: rename add_sha1_to_array","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-02T01:02:34Z","receivedAt":"2017-05-02T01:02:51Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Rename 'add_sha1_to_array()' to 'append_oid_to_array()' to more\naccurately describe what the function does since it handles 'struct\nobject_id' and not sha1 character arrays.\n\nChange-Id: Ia6d15f34cee4d0dc32f7a475c69f4cb3aa8ce5bf\nSigned-off-by: Brandon Williams \n---\n submodule.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex d3299e29c..be0f5d847 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -951,17 +951,18 @@ static void submodule_collect_changed_cb(struct diff_queue_struct *q,\n \t}\n }\n \n-static int add_sha1_to_array(const char *ref, const struct object_id *oid,\n-\t\t\t int flags, void *data)\n+static int append_oid_to_array(const char *ref, const struct object_id *oid,\n+\t\t\t int flags, void *data)\n {\n-\toid_array_append(data, oid);\n+\tstruct oid_array *array = data;\n+\toid_array_append(array, oid);\n \treturn 0;\n }\n \n void check_for_new_submodule_commits(struct object_id *oid)\n {\n \tif (!initialized_fetch_ref_tips) {\n-\t\tfor_each_ref(add_sha1_to_array, &ref_tips_before_fetch);\n+\t\tfor_each_ref(append_oid_to_array, &ref_tips_before_fetch);\n \t\tinitialized_fetch_ref_tips = 1;\n \t}\n \n-- \n2.13.0.rc1.294.g07d810a77f-goog\n\n"},{"id":"318439","messageId":"20170502010239.179369-3-bmwill@google.com","threadId":"45827","inReplyTo":"20170502010239.179369-1-bmwill@google.com","subject":"[PATCH v2 2/6] submodule: rename free_submodules_sha1s","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-02T01:02:35Z","receivedAt":"2017-05-02T01:02:54Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Rename 'free_submodules_sha1s()' to 'free_submodules_oids()' since the\nfunction frees a 'struct string_list' which has a 'struct oid_array'\nstored in the 'util' field.\n\nChange-Id: I0c52fa3af1b1492b196bcf52f8b8cc8f5daf085d\nSigned-off-by: Brandon Williams \n---\n submodule.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex be0f5d847..46abd52b1 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -738,7 +738,7 @@ static void find_unpushed_submodule_commits(struct commit *commit,\n \tdiff_tree_combined_merge(commit, 1, &rev);\n }\n \n-static void free_submodules_sha1s(struct string_list *submodules)\n+static void free_submodules_oids(struct string_list *submodules)\n {\n \tstruct string_list_item *item;\n \tfor_each_string_list_item(item, submodules)\n@@ -779,7 +779,8 @@ int find_unpushed_submodules(struct oid_array *commits,\n \t\tif (submodule_needs_pushing(submodule->string, commits))\n \t\t\tstring_list_insert(needs_pushing, submodule->string);\n \t}\n-\tfree_submodules_sha1s(&submodules);\n+\n+\tfree_submodules_oids(&submodules);\n \n \treturn needs_pushing->nr;\n }\n-- \n2.13.0.rc1.294.g07d810a77f-goog\n\n"},{"id":"318440","messageId":"20170502010239.179369-4-bmwill@google.com","threadId":"45827","inReplyTo":"20170502010239.179369-1-bmwill@google.com","subject":"[PATCH v2 3/6] submodule: remove add_oid_to_argv","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-02T01:02:36Z","receivedAt":"2017-05-02T01:02:57Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"The function 'add_oid_to_argv()' provides the same functionality as\n'append_oid_to_argv()'. Remove this duplicate function and instead use\n'append_oid_to_argv()' where 'add_oid_to_argv()' was previously used.\n\nChange-Id: Id0abea012707460cb7000df761e6557ba5cd88d9\nSigned-off-by: Brandon Williams \n---\n submodule.c | 10 ++--------\n 1 file changed, 2 insertions(+), 8 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 46abd52b1..7baa28ae0 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -970,12 +970,6 @@ void check_for_new_submodule_commits(struct object_id *oid)\n \toid_array_append(&ref_tips_after_fetch, oid);\n }\n \n-static int add_oid_to_argv(const struct object_id *oid, void *data)\n-{\n-\targv_array_push(data, oid_to_hex(oid));\n-\treturn 0;\n-}\n-\n static void calculate_changed_submodule_paths(void)\n {\n \tstruct rev_info rev;\n@@ -989,10 +983,10 @@ static void calculate_changed_submodule_paths(void)\n \tinit_revisions(&rev, NULL);\n \targv_array_push(&argv, \"--\"); /* argv[0] program name */\n \toid_array_for_each_unique(&ref_tips_after_fetch,\n-\t\t\t\t add_oid_to_argv, &argv);\n+\t\t\t\t append_oid_to_argv, &argv);\n \targv_array_push(&argv, \"--not\");\n \toid_array_for_each_unique(&ref_tips_before_fetch,\n-\t\t\t\t add_oid_to_argv, &argv);\n+\t\t\t\t append_oid_to_argv, &argv);\n \tsetup_revisions(argv.argc, argv.argv, &rev, NULL);\n \tif (prepare_revision_walk(&rev))\n \t\tdie(\"revision walk setup failed\");\n-- \n2.13.0.rc1.294.g07d810a77f-goog\n\n"},{"id":"318441","messageId":"20170502010239.179369-6-bmwill@google.com","threadId":"45827","inReplyTo":"20170502010239.179369-1-bmwill@google.com","subject":"[PATCH v2 5/6] submodule: improve submodule_has_commits","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-02T01:02:38Z","receivedAt":"2017-05-02T01:03:00Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Teach 'submodule_has_commits()' to ensure that if a commit exists in a\nsubmodule, that it is also reachable from a ref.\n\nThis is a preparatory step prior to merging the logic which checks for\nchanged submodules when fetching or pushing.\n\nChange-Id: I4fed2acfa7e69a5fbbca534df165671e77a90f22\nSigned-off-by: Brandon Williams \n---\n submodule.c | 34 ++++++++++++++++++++++++++++++++++\n 1 file changed, 34 insertions(+)\n\ndiff --git a/submodule.c b/submodule.c\nindex 3bcf44521..057695e64 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -644,10 +644,44 @@ static int submodule_has_commits(const char *path, struct oid_array *commits)\n {\n \tint has_commit = 1;\n \n+\t/*\n+\t * Perform a cheap, but incorrect check for the existance of 'commits'.\n+\t * This is done by adding the submodule's object store to the in-core\n+\t * object store, and then querying for each commit's existance. If we\n+\t * do not have the commit object anywhere, there is no chance we have\n+\t * it in the object store of the correct submodule and have it\n+\t * reachable from a ref, so we can fail early without spawning rev-list\n+\t * which is expensive.\n+\t */\n \tif (add_submodule_odb(path))\n \t\treturn 0;\n \n \toid_array_for_each_unique(commits, check_has_commit, &has_commit);\n+\n+\tif (has_commit) {\n+\t\t/*\n+\t\t * Even if the submodule is checked out and the commit is\n+\t\t * present, make sure it exists in the submodule's object store\n+\t\t * and that it is reachable from a ref.\n+\t\t */\n+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\tstruct strbuf out = STRBUF_INIT;\n+\n+\t\targv_array_pushl(&cp.args, \"rev-list\", \"-n\", \"1\", NULL);\n+\t\toid_array_for_each_unique(commits, append_oid_to_argv, &cp.args);\n+\t\targv_array_pushl(&cp.args, \"--not\", \"--all\", 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.dir = path;\n+\n+\t\tif (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || out.len)\n+\t\t\thas_commit = 0;\n+\n+\t\tstrbuf_release(&out);\n+\t}\n+\n \treturn has_commit;\n }\n \n-- \n2.13.0.rc1.294.g07d810a77f-goog\n\n"},{"id":"318442","messageId":"20170502010239.179369-7-bmwill@google.com","threadId":"45827","inReplyTo":"20170502010239.179369-1-bmwill@google.com","subject":"[PATCH v2 6/6] submodule: refactor logic to determine changed submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-02T01:02:39Z","receivedAt":"2017-05-02T01:03:04Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"There are currently two instances (fetch and push) where we want to\ndetermine if submodules have changed given some revision specification.\nThese two instances don't use the same logic to generate a list of\nchanged submodules and as a result there is a fair amount of code\nduplication.\n\nThis patch refactors these two code paths such that they both use the\nsame logic to generate a list of changed submodules. This also makes it\neasier for future callers to be able to reuse this logic as they only\nneed to create an argv_array with the revision specification to be using\nduring the revision walk.\n\nChange-Id: Ie2381bd3eaecf5cec62e127df470f27a44ea47da\nSigned-off-by: Brandon Williams \n---\n submodule.c | 247 ++++++++++++++++++++++++++----------------------------------\n 1 file changed, 105 insertions(+), 142 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 057695e64..7eaa3d384 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -617,6 +617,94 @@ const struct submodule *submodule_from_ce(const struct cache_entry *ce)\n \treturn submodule_from_path(null_sha1, ce->name);\n }\n \n+static struct oid_array *submodule_commits(struct string_list *submodules,\n+\t\t\t\t\t const 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 oid_array *) item->util;\n+\n+\t/* NEEDSWORK: should we have oid_array_init()? */\n+\titem->util = xcalloc(1, sizeof(struct oid_array));\n+\treturn (struct oid_array *) item->util;\n+}\n+\n+static void collect_changed_submodules_cb(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 *changed = data;\n+\n+\tfor (i = 0; i < q->nr; i++) {\n+\t\tstruct diff_filepair *p = q->queue[i];\n+\t\tstruct oid_array *commits;\n+\t\tif (!S_ISGITLINK(p->two->mode))\n+\t\t\tcontinue;\n+\n+\t\tif (S_ISGITLINK(p->one->mode)) {\n+\t\t\t/*\n+\t\t\t * NEEDSWORK: We should honor the name configured in\n+\t\t\t * the .gitmodules file of the commit we are examining\n+\t\t\t * here to be able to correctly follow submodules\n+\t\t\t * being moved around.\n+\t\t\t */\n+\t\t\tcommits = submodule_commits(changed, p->two->path);\n+\t\t\toid_array_append(commits, &p->two->oid);\n+\t\t} else {\n+\t\t\t/* Submodule is new or was moved here */\n+\t\t\t/*\n+\t\t\t * NEEDSWORK: When the .git directories of submodules\n+\t\t\t * live inside the superprojects .git directory some\n+\t\t\t * day we should fetch new submodules directly into\n+\t\t\t * that location too when config or options request\n+\t\t\t * that so they can be checked out from there.\n+\t\t\t */\n+\t\t\tcontinue;\n+\t\t}\n+\t}\n+}\n+\n+/*\n+ * Collect the paths of submodules in 'changed' which have changed based on\n+ * the revisions as specified in 'argv'. Each entry in 'changed' will also\n+ * have a corresponding 'struct oid_array' (in the 'util' field) which lists\n+ * what the submodule pointers were updated to during the change.\n+ */\n+static void collect_changed_submodules(struct string_list *changed,\n+\t\t\t\t struct argv_array *argv)\n+{\n+\tstruct rev_info rev;\n+\tconst struct commit *commit;\n+\n+\tinit_revisions(&rev, NULL);\n+\tsetup_revisions(argv->argc, argv->argv, &rev, NULL);\n+\tif (prepare_revision_walk(&rev))\n+\t\tdie(\"revision walk setup failed\");\n+\n+\twhile ((commit = get_revision(&rev))) {\n+\t\tstruct rev_info diff_rev;\n+\n+\t\tinit_revisions(&diff_rev, NULL);\n+\t\tdiff_rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;\n+\t\tdiff_rev.diffopt.format_callback = collect_changed_submodules_cb;\n+\t\tdiff_rev.diffopt.format_callback_data = changed;\n+\t\tdiff_tree_combined_merge(commit, 1, &diff_rev);\n+\t}\n+\n+\treset_revision_walk();\n+}\n+\n+static void free_submodules_oids(struct string_list *submodules)\n+{\n+\tstruct string_list_item *item;\n+\tfor_each_string_list_item(item, submodules)\n+\t\toid_array_clear((struct oid_array *) item->util);\n+\tstring_list_clear(submodules, 1);\n+}\n+\n static int has_remote(const char *refname, const struct object_id *oid,\n \t\t int flags, void *cb_data)\n {\n@@ -729,92 +817,31 @@ static int submodule_needs_pushing(const char *path, struct oid_array *commits)\n \treturn 0;\n }\n \n-static struct oid_array *submodule_commits(struct string_list *submodules,\n-\t\t\t\t\t const 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 oid_array *) item->util;\n-\n-\t/* NEEDSWORK: should we have oid_array_init()? */\n-\titem->util = xcalloc(1, sizeof(struct oid_array));\n-\treturn (struct oid_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 *submodules = data;\n-\n-\tfor (i = 0; i < q->nr; i++) {\n-\t\tstruct diff_filepair *p = q->queue[i];\n-\t\tstruct oid_array *commits;\n-\t\tif (!S_ISGITLINK(p->two->mode))\n-\t\t\tcontinue;\n-\t\tcommits = submodule_commits(submodules, p->two->path);\n-\t\toid_array_append(commits, &p->two->oid);\n-\t}\n-}\n-\n-static void find_unpushed_submodule_commits(struct commit *commit,\n-\t\tstruct string_list *needs_pushing)\n-{\n-\tstruct rev_info rev;\n-\n-\tinit_revisions(&rev, NULL);\n-\trev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;\n-\trev.diffopt.format_callback = collect_submodules_from_diff;\n-\trev.diffopt.format_callback_data = needs_pushing;\n-\tdiff_tree_combined_merge(commit, 1, &rev);\n-}\n-\n-static void free_submodules_oids(struct string_list *submodules)\n-{\n-\tstruct string_list_item *item;\n-\tfor_each_string_list_item(item, submodules)\n-\t\toid_array_clear((struct oid_array *) item->util);\n-\tstring_list_clear(submodules, 1);\n-}\n-\n int find_unpushed_submodules(struct oid_array *commits,\n \t\tconst char *remotes_name, struct string_list *needs_pushing)\n {\n-\tstruct rev_info rev;\n-\tstruct commit *commit;\n \tstruct string_list submodules = STRING_LIST_INIT_DUP;\n \tstruct string_list_item *submodule;\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n \n-\tinit_revisions(&rev, NULL);\n-\n \t/* argv.argv[0] will be ignored by setup_revisions */\n \targv_array_push(&argv, \"find_unpushed_submodules\");\n \toid_array_for_each_unique(commits, append_oid_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-\twhile ((commit = get_revision(&rev)) != NULL)\n-\t\tfind_unpushed_submodule_commits(commit, &submodules);\n-\n-\treset_revision_walk();\n-\targv_array_clear(&argv);\n+\tcollect_changed_submodules(&submodules, &argv);\n \n \tfor_each_string_list_item(submodule, &submodules) {\n-\t\tstruct oid_array *commits = (struct oid_array *) submodule->util;\n+\t\tstruct oid_array *commits = submodule->util;\n+\t\tconst char *path = submodule->string;\n \n-\t\tif (submodule_needs_pushing(submodule->string, commits))\n-\t\t\tstring_list_insert(needs_pushing, submodule->string);\n+\t\tif (submodule_needs_pushing(path, commits))\n+\t\t\tstring_list_insert(needs_pushing, path);\n \t}\n \n \tfree_submodules_oids(&submodules);\n+\targv_array_clear(&argv);\n \n \treturn needs_pushing->nr;\n }\n@@ -931,61 +958,6 @@ int push_unpushed_submodules(struct oid_array *commits,\n \treturn ret;\n }\n \n-static int is_submodule_commit_present(const char *path, unsigned char sha1[20])\n-{\n-\tint is_present = 0;\n-\tif (!add_submodule_odb(path) && lookup_commit_reference(sha1)) {\n-\t\t/* Even if the submodule is checked out and the commit is\n-\t\t * present, make sure it is reachable from a ref. */\n-\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\t\tconst char *argv[] = {\"rev-list\", \"-n\", \"1\", NULL, \"--not\", \"--all\", NULL};\n-\t\tstruct strbuf buf = STRBUF_INIT;\n-\n-\t\targv[3] = 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.dir = path;\n-\t\tif (!capture_command(&cp, &buf, 1024) && !buf.len)\n-\t\t\tis_present = 1;\n-\n-\t\tstrbuf_release(&buf);\n-\t}\n-\treturn is_present;\n-}\n-\n-static void submodule_collect_changed_cb(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-\tfor (i = 0; i < q->nr; i++) {\n-\t\tstruct diff_filepair *p = q->queue[i];\n-\t\tif (!S_ISGITLINK(p->two->mode))\n-\t\t\tcontinue;\n-\n-\t\tif (S_ISGITLINK(p->one->mode)) {\n-\t\t\t/* NEEDSWORK: We should honor the name configured in\n-\t\t\t * the .gitmodules file of the commit we are examining\n-\t\t\t * here to be able to correctly follow submodules\n-\t\t\t * being moved around. */\n-\t\t\tstruct string_list_item *path;\n-\t\t\tpath = unsorted_string_list_lookup(&changed_submodule_paths, p->two->path);\n-\t\t\tif (!path && !is_submodule_commit_present(p->two->path, p->two->oid.hash))\n-\t\t\t\tstring_list_append(&changed_submodule_paths, p->two->path);\n-\t\t} else {\n-\t\t\t/* Submodule is new or was moved here */\n-\t\t\t/* NEEDSWORK: When the .git directories of submodules\n-\t\t\t * live inside the superprojects .git directory some\n-\t\t\t * day we should fetch new submodules directly into\n-\t\t\t * that location too when config or options request\n-\t\t\t * that so they can be checked out from there. */\n-\t\t\tcontinue;\n-\t\t}\n-\t}\n-}\n-\n static int append_oid_to_array(const char *ref, const struct object_id *oid,\n \t\t\t int flags, void *data)\n {\n@@ -1006,45 +978,36 @@ void check_for_new_submodule_commits(struct object_id *oid)\n \n static void calculate_changed_submodule_paths(void)\n {\n-\tstruct rev_info rev;\n-\tstruct commit *commit;\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n+\tstruct string_list changed_submodules = STRING_LIST_INIT_DUP;\n+\tconst struct string_list_item *item;\n \n \t/* No need to check if there are no submodules configured */\n \tif (!submodule_from_path(NULL, NULL))\n \t\treturn;\n \n-\tinit_revisions(&rev, NULL);\n \targv_array_push(&argv, \"--\"); /* argv[0] program name */\n \toid_array_for_each_unique(&ref_tips_after_fetch,\n \t\t\t\t append_oid_to_argv, &argv);\n \targv_array_push(&argv, \"--not\");\n \toid_array_for_each_unique(&ref_tips_before_fetch,\n \t\t\t\t append_oid_to_argv, &argv);\n-\tsetup_revisions(argv.argc, argv.argv, &rev, NULL);\n-\tif (prepare_revision_walk(&rev))\n-\t\tdie(\"revision walk setup failed\");\n \n \t/*\n \t * Collect all submodules (whether checked out or not) for which new\n \t * commits have been recorded upstream in \"changed_submodule_paths\".\n \t */\n-\twhile ((commit = get_revision(&rev))) {\n-\t\tstruct commit_list *parent = commit->parents;\n-\t\twhile (parent) {\n-\t\t\tstruct diff_options diff_opts;\n-\t\t\tdiff_setup(&diff_opts);\n-\t\t\tDIFF_OPT_SET(&diff_opts, RECURSIVE);\n-\t\t\tdiff_opts.output_format |= DIFF_FORMAT_CALLBACK;\n-\t\t\tdiff_opts.format_callback = submodule_collect_changed_cb;\n-\t\t\tdiff_setup_done(&diff_opts);\n-\t\t\tdiff_tree_sha1(parent->item->object.oid.hash, commit->object.oid.hash, \"\", &diff_opts);\n-\t\t\tdiffcore_std(&diff_opts);\n-\t\t\tdiff_flush(&diff_opts);\n-\t\t\tparent = parent->next;\n-\t\t}\n+\tcollect_changed_submodules(&changed_submodules, &argv);\n+\n+\tfor_each_string_list_item(item, &changed_submodules) {\n+\t\tstruct oid_array *commits = item->util;\n+\t\tconst char *path = item->string;\n+\n+\t\tif (!submodule_has_commits(path, commits))\n+\t\t\tstring_list_append(&changed_submodule_paths, path);\n \t}\n \n+\tfree_submodules_oids(&changed_submodules);\n \targv_array_clear(&argv);\n \toid_array_clear(&ref_tips_before_fetch);\n \toid_array_clear(&ref_tips_after_fetch);\n-- \n2.13.0.rc1.294.g07d810a77f-goog\n\n"},{"id":"318443","messageId":"20170502010239.179369-5-bmwill@google.com","threadId":"45827","inReplyTo":"20170502010239.179369-1-bmwill@google.com","subject":"[PATCH v2 4/6] submodule: change string_list changed_submodule_paths","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-02T01:02:37Z","receivedAt":"2017-05-02T01:03:06Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Eliminate a call to 'xstrdup()' by changing the string_list\n'changed_submodule_paths' to duplicated strings added to it.\n\nChange-Id: Id4b53837a6e209c0c0837c9f5ba06c70df2ffe06\nSigned-off-by: Brandon Williams \n---\n submodule.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 7baa28ae0..3bcf44521 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -20,7 +20,7 @@\n static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;\n static int config_update_recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n static int parallel_jobs = 1;\n-static struct string_list changed_submodule_paths = STRING_LIST_INIT_NODUP;\n+static struct string_list changed_submodule_paths = STRING_LIST_INIT_DUP;\n static int initialized_fetch_ref_tips;\n static struct oid_array ref_tips_before_fetch;\n static struct oid_array ref_tips_after_fetch;\n@@ -939,7 +939,7 @@ static void submodule_collect_changed_cb(struct diff_queue_struct *q,\n \t\t\tstruct string_list_item *path;\n \t\t\tpath = unsorted_string_list_lookup(&changed_submodule_paths, p->two->path);\n \t\t\tif (!path && !is_submodule_commit_present(p->two->path, p->two->oid.hash))\n-\t\t\t\tstring_list_append(&changed_submodule_paths, xstrdup(p->two->path));\n+\t\t\t\tstring_list_append(&changed_submodule_paths, p->two->path);\n \t\t} else {\n \t\t\t/* Submodule is new or was moved here */\n \t\t\t/* NEEDSWORK: When the .git directories of submodules\n-- \n2.13.0.rc1.294.g07d810a77f-goog\n\n"},{"id":"318444","messageId":"CAGZ79kbq_WSH01yE+oT9aagmyPWF55eCmU_2Dv_k1soB9-bwnA@mail.gmail.com","threadId":"45827","inReplyTo":"20170502010239.179369-2-bmwill@google.com","subject":"Re: [PATCH v2 1/6] submodule: rename add_sha1_to_array","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-02T01:05:30Z","receivedAt":"2017-05-02T01:05:36Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 1, 2017 at 6:02 PM, Brandon Williams wrote:\n\n> Change-Id: Ia6d15f34cee4d0dc32f7a475c69f4cb3aa8ce5bf\n\nUh?\n\nMaybe another side project for the long todo list: get git-trailers into shape,\nsuch that it can be configured to yell at you upon formatting the patch or\nsending email when a given trailer occurs.\n\nThanks,\nStefan\n"},{"id":"318445","messageId":"20170502010909.GC154031@google.com","threadId":"45827","inReplyTo":"CAGZ79kbq_WSH01yE+oT9aagmyPWF55eCmU_2Dv_k1soB9-bwnA@mail.gmail.com","subject":"Re: [PATCH v2 1/6] submodule: rename add_sha1_to_array","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-02T01:09:09Z","receivedAt":"2017-05-02T01:09:16Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/01, Stefan Beller wrote:\n> On Mon, May 1, 2017 at 6:02 PM, Brandon Williams wrote:\n> \n> > Change-Id: Ia6d15f34cee4d0dc32f7a475c69f4cb3aa8ce5bf\n> \n> Uh?\n\nOops! I forgot to run it through my scrubbing script...\n\n> \n> Maybe another side project for the long todo list: get git-trailers into shape,\n> such that it can be configured to yell at you upon formatting the patch or\n> sending email when a given trailer occurs.\n> \n> Thanks,\n> Stefan\n\n-- \nBrandon Williams\n"},{"id":"318446","messageId":"CAGZ79kbbz3AAjbg_dV9RVS8kgLs-zWZxt5tsFbQczCm78LcTVw@mail.gmail.com","threadId":"45827","inReplyTo":"20170502010239.179369-6-bmwill@google.com","subject":"Re: [PATCH v2 5/6] submodule: improve submodule_has_commits","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-02T01:34:35Z","receivedAt":"2017-05-02T01:34:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 1, 2017 at 6:02 PM, Brandon Williams wrote:\n> Teach 'submodule_has_commits()' to ensure that if a commit exists in a\n> submodule, that it is also reachable from a ref.\n>\n> This is a preparatory step prior to merging the logic which checks for\n> changed submodules when fetching or pushing.\n>\n> Change-Id: I4fed2acfa7e69a5fbbca534df165671e77a90f22\n> Signed-off-by: Brandon Williams \n> ---\n> submodule.c | 34 ++++++++++++++++++++++++++++++++++\n> 1 file changed, 34 insertions(+)\n>\n> diff --git a/submodule.c b/submodule.c\n> index 3bcf44521..057695e64 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -644,10 +644,44 @@ static int submodule_has_commits(const char *path, struct oid_array *commits)\n> {\n> int has_commit = 1;\n>\n> + /*\n> + * Perform a cheap, but incorrect check for the existance of 'commits'.\n> + * This is done by adding the submodule's object store to the in-core\n> + * object store, and then querying for each commit's existance. If we\n> + * do not have the commit object anywhere, there is no chance we have\n> + * it in the object store of the correct submodule and have it\n> + * reachable from a ref, so we can fail early without spawning rev-list\n> + * which is expensive.\n> + */\n> if (add_submodule_odb(path))\n> return 0;\n\nThanks for the comment!\n\n>\n> oid_array_for_each_unique(commits, check_has_commit, &has_commit);\n> +\n> + if (has_commit) {\n> + /*\n> + * Even if the submodule is checked out and the commit is\n> + * present, make sure it exists in the submodule's object store\n> + * and that it is reachable from a ref.\n> + */\n> + struct child_process cp = CHILD_PROCESS_INIT;\n> + struct strbuf out = STRBUF_INIT;\n> +\n> + argv_array_pushl(&cp.args, \"rev-list\", \"-n\", \"1\", NULL);\n> + oid_array_for_each_unique(commits, append_oid_to_argv, &cp.args);\n> + argv_array_pushl(&cp.args, \"--not\", \"--all\", NULL);\n> +\n> + prepare_submodule_repo_env(&cp.env_array);\n> + cp.git_cmd = 1;\n> + cp.no_stdin = 1;\n> + cp.dir = path;\n> +\n> + if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || out.len)\n\neh, I gave too much and self-contradicting feedback here earlier,\nideally I'd like to review this to be similar as:\n\n if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1)\n die(\"cannot capture git-rev-list in submodule '%s', sub->path);\n\n if (out.len)\n has_commit = 0;\n\ninstead as that does not have a silent error. (though it errs\non the safe side, so maybe it is not to bad.)\n\nI could understand if the callers do not want to have\n`submodule_has_commits` die()-ing on them, so maybe\n\n if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) {\n warning(\"cannot capture git-rev-list in submodule '%s', sub->path);\n has_commit = -1;\n /* this would require auditing all callers and handling -1 though */\n }\n\n if (out.len)\n has_commit = 0;\n\nAs the comment eludes, we'd then have\n 0 -> has no commits\n 1 -> has commits\n-1 -> error\n\nSo to group (error || has_no_commits), we could write\n\n if (submodule_has_commits(..) <= 0)\n\nwhich is awkward. So maybe we can rename the function\nto misses_submodule_commits instead, as then we could\nflip the return value as well and have\n\n 0 -> has commits\n 1 -> has no commits\n-1 -> error\n\nand the lazy invoker could just go with\n\n if (!misses_submodule_commits(..))\n proceed();\n else\n die(\"missing submodule commits or errors; I don't care\");\n\nwhereas the careful invoker could go with\n\n switch (misses_submodule_commits(..)) {\n case 0:\n proceed(); break;\n case 1:\n pull_magic_trick(); break;\n case -1:\n make_errors_go_away_and_retry(); break;\n }\n\n\n\n---\nOn the longer term plan:\nAs you wrote about costs. Maybe instead of invoking rev-list,\nwe could try to have this in-core as a first try-out for\n\"classified-repos\", looking at refs.h there is e.g.\n\n int for_each_ref_submodule(const char *submodule_path,\n each_ref_fn fn, void *cb_data);\n\nwhich we could use to obtain all submodule refs and then\nuse the revision walking machinery to find out ourselves if\nwe have or do not have the commits. (As we loaded the\nodb of the submodule, this would *just work*, building one\nkludgy hack upon the next.)\n\nThanks,\nStefan\n"},{"id":"318545","messageId":"20170502172555.GA181268@google.com","threadId":"45827","inReplyTo":"CAGZ79kbbz3AAjbg_dV9RVS8kgLs-zWZxt5tsFbQczCm78LcTVw@mail.gmail.com","subject":"Re: [PATCH v2 5/6] submodule: improve submodule_has_commits","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-02T17:25:55Z","receivedAt":"2017-05-02T17:26:04Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/01, Stefan Beller wrote:\n> On Mon, May 1, 2017 at 6:02 PM, Brandon Williams wrote:\n> > +\n> > + if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || out.len)\n> \n> eh, I gave too much and self-contradicting feedback here earlier,\n> ideally I'd like to review this to be similar as:\n> \n> if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1)\n> die(\"cannot capture git-rev-list in submodule '%s', sub->path);\n\nThis wouldn't really work because if you provide a SHA1 to rev-list\nwhich it isn't able to find then it returns a non-zero exit code which\nwould cause this to die, which isn't the desired behavior.\n\n> \n> if (out.len)\n> has_commit = 0;\n> \n> instead as that does not have a silent error. (though it errs\n> on the safe side, so maybe it is not to bad.)\n> \n> I could understand if the callers do not want to have\n> `submodule_has_commits` die()-ing on them, so maybe\n> \n> if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) {\n> warning(\"cannot capture git-rev-list in submodule '%s', sub->path);\n> has_commit = -1;\n> /* this would require auditing all callers and handling -1 though */\n> }\n> \n> if (out.len)\n> has_commit = 0;\n> \n> As the comment eludes, we'd then have\n> 0 -> has no commits\n> 1 -> has commits\n> -1 -> error\n> \n> So to group (error || has_no_commits), we could write\n> \n> if (submodule_has_commits(..) <= 0)\n> \n> which is awkward. So maybe we can rename the function\n> to misses_submodule_commits instead, as then we could\n> flip the return value as well and have\n> \n> 0 -> has commits\n> 1 -> has no commits\n> -1 -> error\n> \n> and the lazy invoker could just go with\n> \n> if (!misses_submodule_commits(..))\n> proceed();\n> else\n> die(\"missing submodule commits or errors; I don't care\");\n> \n> whereas the careful invoker could go with\n> \n> switch (misses_submodule_commits(..)) {\n> case 0:\n> proceed(); break;\n> case 1:\n> pull_magic_trick(); break;\n> case -1:\n> make_errors_go_away_and_retry(); break;\n> }\n\nI feel like you're making this a little too complicated, as all I'm\ndoing is shuffling around already existing logic. I understand the want\nto make things more robust but this seems unnecessarily complex.\n\n> ---\n> On the longer term plan:\n> As you wrote about costs. Maybe instead of invoking rev-list,\n> we could try to have this in-core as a first try-out for\n> \"classified-repos\", looking at refs.h there is e.g.\n> \n> int for_each_ref_submodule(const char *submodule_path,\n> each_ref_fn fn, void *cb_data);\n> \n> which we could use to obtain all submodule refs and then\n> use the revision walking machinery to find out ourselves if\n> we have or do not have the commits. (As we loaded the\n> odb of the submodule, this would *just work*, building one\n> kludgy hack upon the next.)\n> \n> Thanks,\n> Stefan\n\n-- \nBrandon Williams\n"},{"id":"318548","messageId":"CAGZ79kZxYCr+H3PLdnxJp7_aG=JmeMTGry51ppCe3=R2xLrP1w@mail.gmail.com","threadId":"45827","inReplyTo":"20170502172555.GA181268@google.com","subject":"Re: [PATCH v2 5/6] submodule: improve submodule_has_commits","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-02T17:55:35Z","receivedAt":"2017-05-02T17:55:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, May 2, 2017 at 10:25 AM, Brandon Williams wrote:\n> On 05/01, Stefan Beller wrote:\n>> On Mon, May 1, 2017 at 6:02 PM, Brandon Williams wrote:\n>> > +\n>> > + if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || out.len)\n>>\n>> eh, I gave too much and self-contradicting feedback here earlier,\n>> ideally I'd like to review this to be similar as:\n>>\n>> if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1)\n>> die(\"cannot capture git-rev-list in submodule '%s', sub->path);\n>\n> This wouldn't really work because if you provide a SHA1 to rev-list\n> which it isn't able to find then it returns a non-zero exit code which\n> would cause this to die, which isn't the desired behavior.\n\nOh. In that case, why do we even check for its stdout?\n(from rev-lists man page)\n --quiet\n Don’t print anything to standard output. This form is primarily\n meant to allow the caller to test the exit status to see if a range\n of objects is fully connected (or not). It is faster than\n redirecting stdout to /dev/null as the output does not have to be\n formatted.\n\n>\n> I feel like you're making this a little too complicated, as all I'm\n> doing is shuffling around already existing logic. I understand the want\n> to make things more robust but this seems unnecessarily complex.\n\nok. I was just giving my thoughts on how I would approach it.\n\nThanks,\nStefan\n"},{"id":"318557","messageId":"20170502191407.GF181268@google.com","threadId":"45827","inReplyTo":"CAGZ79kZxYCr+H3PLdnxJp7_aG=JmeMTGry51ppCe3=R2xLrP1w@mail.gmail.com","subject":"Re: [PATCH v2 5/6] submodule: improve submodule_has_commits","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-02T19:14:07Z","receivedAt":"2017-05-02T19:14:30Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/02, Stefan Beller wrote:\n> On Tue, May 2, 2017 at 10:25 AM, Brandon Williams wrote:\n> > On 05/01, Stefan Beller wrote:\n> >> On Mon, May 1, 2017 at 6:02 PM, Brandon Williams wrote:\n> >> > +\n> >> > + if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || out.len)\n> >>\n> >> eh, I gave too much and self-contradicting feedback here earlier,\n> >> ideally I'd like to review this to be similar as:\n> >>\n> >> if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1)\n> >> die(\"cannot capture git-rev-list in submodule '%s', sub->path);\n> >\n> > This wouldn't really work because if you provide a SHA1 to rev-list\n> > which it isn't able to find then it returns a non-zero exit code which\n> > would cause this to die, which isn't the desired behavior.\n> \n> Oh. In that case, why do we even check for its stdout?\n> (from rev-lists man page)\n> --quiet\n> Don’t print anything to standard output. This form is primarily\n> meant to allow the caller to test the exit status to see if a range\n> of objects is fully connected (or not). It is faster than\n> redirecting stdout to /dev/null as the output does not have to be\n> formatted.\n> \n\nSounds good, Let me take a look at using --quiet\n\n> >\n> > I feel like you're making this a little too complicated, as all I'm\n> > doing is shuffling around already existing logic. I understand the want\n> > to make things more robust but this seems unnecessarily complex.\n> \n> ok. I was just giving my thoughts on how I would approach it.\n> \n> Thanks,\n> Stefan\n\n-- \nBrandon Williams\n"},{"id":"318558","messageId":"20170502193043.GG181268@google.com","threadId":"45827","inReplyTo":"20170502191407.GF181268@google.com","subject":"Re: [PATCH v2 5/6] submodule: improve submodule_has_commits","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-02T19:30:43Z","receivedAt":"2017-05-02T19:30:52Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/02, Brandon Williams wrote:\n> On 05/02, Stefan Beller wrote:\n> > On Tue, May 2, 2017 at 10:25 AM, Brandon Williams wrote:\n> > > On 05/01, Stefan Beller wrote:\n> > >> On Mon, May 1, 2017 at 6:02 PM, Brandon Williams wrote:\n> > >> > +\n> > >> > + if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || out.len)\n> > >>\n> > >> eh, I gave too much and self-contradicting feedback here earlier,\n> > >> ideally I'd like to review this to be similar as:\n> > >>\n> > >> if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1)\n> > >> die(\"cannot capture git-rev-list in submodule '%s', sub->path);\n> > >\n> > > This wouldn't really work because if you provide a SHA1 to rev-list\n> > > which it isn't able to find then it returns a non-zero exit code which\n> > > would cause this to die, which isn't the desired behavior.\n> > \n> > Oh. In that case, why do we even check for its stdout?\n> > (from rev-lists man page)\n> > --quiet\n> > Don’t print anything to standard output. This form is primarily\n> > meant to allow the caller to test the exit status to see if a range\n> > of objects is fully connected (or not). It is faster than\n> > redirecting stdout to /dev/null as the output does not have to be\n> > formatted.\n> > \n> \n> Sounds good, Let me take a look at using --quiet\n\nFrom our offline discussion --quiet doesn't do what we want here. We\nare checking (1) if the commit exists and (2) if it is reachable. Using\n--quiet would only satisfy (1)\n\n> \n> > >\n> > > I feel like you're making this a little too complicated, as all I'm\n> > > doing is shuffling around already existing logic. I understand the want\n> > > to make things more robust but this seems unnecessarily complex.\n> > \n> > ok. I was just giving my thoughts on how I would approach it.\n> > \n> > Thanks,\n> > Stefan\n> \n> -- \n> Brandon Williams\n\n-- \nBrandon Williams\n"}]}