Re: [PATCH 4/6] submodule: change string_list changed_submodule_paths
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 1, 2017, 03:28 UTC
- Message-ID
- <xmqqr309gss1.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170428235402.162251-5-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
Show 27 quoted lines
> 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 ;-)