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

[Brandon Williams <bmwill@google.com>]

On 04/30, Junio C Hamano wrote:
> 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 ;-)

All good good points which hopefully are resolved in a later patch.  This
patch exists mainly to factor out the change to make the string_list
duplicate the strings added to it as opposed to have that change seem
random and out of place in a later patch.

Most of this logic itself is removed entirely later once it is unified
with the other code path which checked for changed submodules.  This may
be out of context on this particular patch, but eventually an oid_array
is stored in the 'util' field of the string list items.  This oid_array
holds all of the changes to the submodule pointers.  This allows us to
then batch check for the existence of all the submodule commits instead
of checking each change individually (which is what this code path
currently does).

-- 
Brandon Williams