Re: [PATCH 4/6] submodule: change string_list changed_submodule_paths

[Junio C Hamano <gitster@pobox.com>]

Brandon Williams <bmwill@google.com> writes:

> Eliminate a call to 'xstrdup()' by changing the string_list
> 'changed_submodule_paths' to duplicated strings added to it.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  submodule.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 7baa28ae0..3bcf44521 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -20,7 +20,7 @@
>  static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
>  static int config_update_recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
>  static int parallel_jobs = 1;
> -static struct string_list changed_submodule_paths = STRING_LIST_INIT_NODUP;
> +static struct string_list changed_submodule_paths = STRING_LIST_INIT_DUP;
>  static int initialized_fetch_ref_tips;
>  static struct oid_array ref_tips_before_fetch;
>  static struct oid_array ref_tips_after_fetch;
> @@ -939,7 +939,7 @@ static void submodule_collect_changed_cb(struct diff_queue_struct *q,
>  			struct string_list_item *path;
>  			path = unsorted_string_list_lookup(&changed_submodule_paths, p->two->path);
>  			if (!path && !is_submodule_commit_present(p->two->path, p->two->oid.hash))
> -				string_list_append(&changed_submodule_paths, xstrdup(p->two->path));
> +				string_list_append(&changed_submodule_paths, p->two->path);

I notice that "path" is not used at all, and other users of this
string list do not even bother using a variable, i.e.

	if (!unsorted_string_list_lookup(&changed_submodule_paths, ...))

In fact, it might be even better to use a hashmap for this instead?

The call to string_list_clear() onthis list tells it to free the
util field, and I went to see what we are storing in the util field,
but it seems that it is freeing NULLs, which is somewhat misleading
and time-wasting on the code readers.  Using hashmap may also clear
this up.

But all of the above are not within the scope of this topic ;-)