| Message ID | 20181016181327.107186-6-sbeller@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Resending sb/submodule-recursive-fetch-gets-the-tip | expand |
> By doing so we'll have access to the util pointer for longer that > contains the commits that we need to fetch, which will be > useful in a later patch. It seems that the main point of this patch is so that the OIDs be stored in changed_submodule_names, because you need them in a later patch. If so, I would have expected a commit title like "submodule: store OIDs in changed_submodule_names". > @@ -1142,8 +1142,7 @@ static void calculate_changed_submodule_paths( > struct submodule_parallel_fetch *spf) > { > struct argv_array argv = ARGV_ARRAY_INIT; > - struct string_list changed_submodules = STRING_LIST_INIT_DUP; > - const struct string_list_item *name; > + struct string_list_item *name; Prior to this patch, changed_submodules is here just so that we know what to add to changed_submodule_names (it will be cleared at the end of the function). So removing it is fine. > - collect_changed_submodules(&changed_submodules, &argv); > + collect_changed_submodules(&spf->changed_submodule_names, &argv); > > - for_each_string_list_item(name, &changed_submodules) { > + for_each_string_list_item(name, &spf->changed_submodule_names) { We add all the changed submodules directly to changed_submodule_names, and iterate over it. So we use changed_submodule_names as a scratchpad... > - if (!submodule_has_commits(path, commits)) > - string_list_append(&spf->changed_submodule_names, > - name->string); > + if (submodule_has_commits(path, commits)) { > + oid_array_clear(commits); > + *name->string = '\0'; > + } ...but this is fine because previously, we appended the name->string to changed_submodule_names (with no util) whereas now, we make the name->string empty in the opposite condition. Before this patch, at the end of the loop, we had all the wanted submodule names with no util in changed_submodule_names. With this patch, at the end of the loop, we have all the wanted submodule names with util pointing to an OID array, and also some blanks with util pointing to a cleared OID array. > - free_submodules_oids(&changed_submodules); > + string_list_remove_empty_items(&spf->changed_submodule_names, 1); The local variable changed_submodules no longer exists, so removing that line is fine. And we remove all the blanks from changed_submodule_names. > @@ -1377,7 +1378,7 @@ int fetch_populated_submodules(struct repository *r, > > argv_array_clear(&spf.args); > out: > - string_list_clear(&spf.changed_submodule_names, 1); > + free_submodules_oids(&spf.changed_submodule_names); And because changed_submodule_names now has a non-trivial util pointer, we need to free it properly. This patch looks good, except that the commit title and message could perhaps be clearer.
Jonathan Tan <jonathantanmy@google.com> writes: >> By doing so we'll have access to the util pointer for longer that >> contains the commits that we need to fetch, which will be >> useful in a later patch. > > It seems that the main point of this patch is so that the OIDs be stored > in changed_submodule_names, because you need them in a later patch. If > so, I would have expected a commit title like "submodule: store OIDs in > changed_submodule_names". > > ... > This patch looks good, except that the commit title and message could > perhaps be clearer. Thanks for a thoughtful review; not just this step, but for your review comments on all the other steps in the series are very helpful.
diff --git a/submodule.c b/submodule.c index 6b4cee82bf..cbefe5f54d 100644 --- a/submodule.c +++ b/submodule.c @@ -1142,8 +1142,7 @@ static void calculate_changed_submodule_paths( struct submodule_parallel_fetch *spf) { struct argv_array argv = ARGV_ARRAY_INIT; - struct string_list changed_submodules = STRING_LIST_INIT_DUP; - const struct string_list_item *name; + struct string_list_item *name; /* No need to check if there are no submodules configured */ if (!submodule_from_path(the_repository, NULL, NULL)) @@ -1160,9 +1159,9 @@ static void calculate_changed_submodule_paths( * Collect all submodules (whether checked out or not) for which new * commits have been recorded upstream in "changed_submodule_names". */ - collect_changed_submodules(&changed_submodules, &argv); + collect_changed_submodules(&spf->changed_submodule_names, &argv); - for_each_string_list_item(name, &changed_submodules) { + for_each_string_list_item(name, &spf->changed_submodule_names) { struct oid_array *commits = name->util; const struct submodule *submodule; const char *path = NULL; @@ -1176,12 +1175,14 @@ static void calculate_changed_submodule_paths( if (!path) continue; - if (!submodule_has_commits(path, commits)) - string_list_append(&spf->changed_submodule_names, - name->string); + if (submodule_has_commits(path, commits)) { + oid_array_clear(commits); + *name->string = '\0'; + } } - free_submodules_oids(&changed_submodules); + string_list_remove_empty_items(&spf->changed_submodule_names, 1); + argv_array_clear(&argv); oid_array_clear(&ref_tips_before_fetch); oid_array_clear(&ref_tips_after_fetch); @@ -1377,7 +1378,7 @@ int fetch_populated_submodules(struct repository *r, argv_array_clear(&spf.args); out: - string_list_clear(&spf.changed_submodule_names, 1); + free_submodules_oids(&spf.changed_submodule_names); return spf.result; }