Stefan Beller <sbeller@xxxxxxxxxx> writes:
> 'calculate_changed_submodule_paths' uses a local list to compute the
> changed submodules, and then produces the result by copying appropriate
> items into the result list.
>
> Instead use the result list directly and prune items afterwards
> using string_list_remove_empty_items.
That may describe what the patch does, but does not explain why we
would even want to do so in the first place.
It may be a safe thing to do to munge the list in place as there is
nobody that looks at the list after we are done in the current code,
but the above description does not tell that to readers and fails to
give readers warm and fuzzy feeling that the safety likely will stay
with us in the future.
> As a side effect, we'll have access to the util pointer for longer that
> contains the commits that we need to fetch.
If this patch does not move free-submodules-oids call to out: label
(i.e. instead do so after the call to string-list-remove-empty-items
is made, perhaps), then the list of oids will have the same lifespan
as the original code, no? Then it is not a "side effect" but is
deliberate change of behaviour this patch makes. We can access the
list for longer time than before, which may be a good thing, in
which case we'd want to explain the potential benefit we could reap
with future changes, no?
>
> Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
> ---
> submodule.c | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 582c0263b91..0efe6711a8c 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1128,8 +1128,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))
> @@ -1146,9 +1145,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;
> @@ -1162,12 +1161,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);
> @@ -1362,7 +1363,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;
> }
