Re: [PATCH 5/9] submodule.c: do not copy around submodule list
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Oct 17, 2018, 21:45 UTC
- Message-ID
- <20181017214534.199890-1-jonathantanmy@google.com>
- In-Reply-To
- <20181016181327.107186-6-sbeller@google.com>
> 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".
Show 7 quoted lines
> @@ -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.
Show 5 quoted lines
> - 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...
Show 7 quoted lines
> - 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.
Show 6 quoted lines
> @@ -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.