Re: [PATCH 5/9] submodule.c: do not copy around submodule list

[Jonathan Tan <jonathantanmy@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".

> @@ -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.