Re: [PATCH v5 2/3] Refactor submodule push check to use string list instead of integer

> Heiko Voigt <hvoigt@hvoigt.net> writes:
> > diff --git a/submodule.c b/submodule.c
> > index 645ff5d..3c714c2 100644
> > --- a/submodule.c
> > +++ b/submodule.c
> > @@ -357,21 +357,20 @@ static void collect_submodules_from_diff(struct diff_queue_struct *q,
> >  					 void *data)
> >  {
> >  	int i;
> > -	int *needs_pushing = data;
> > +	struct string_list *needs_pushing = data;
> >  
> >  	for (i = 0; i < q->nr; i++) {
> >  		struct diff_filepair *p = q->queue[i];
> >  		if (!S_ISGITLINK(p->two->mode))
> >  			continue;
> >  		if (submodule_needs_pushing(p->two->path, p->two->sha1)) {
> > -			*needs_pushing = 1;
> > -			break;
> > +			if (!string_list_has_string(needs_pushing, p->two->path))
> > +				string_list_insert(needs_pushing, p->two->path);
> 
> Does string_list API have "look for this and insert if it doesn't exist
> but otherwise don't do anything"?  Running get_entry_index() to answer
> has_string() once and then calling it again to find where to insert to
> respond to insert() looks a bit wasteful.
> 
> Just wondering.

> > -
> > -static void commit_need_pushing(struct commit *commit, int *needs_pushing)
> > +static void commit_need_pushing(struct commit *commit, struct string_list *needs_pushing)
> >  {
> >  	struct rev_info rev;
> >  
> > @@ -382,14 +381,15 @@ static void commit_need_pushing(struct commit *commit, int *needs_pushing)
> >  	diff_tree_combined_merge(commit, 1, &rev);
> >  }
> >  
> > -int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name)
> > +int check_submodule_needs_pushing(unsigned char new_sha1[20],
> > +		const char *remotes_name, struct string_list *needs_pushing)
> >  {
> >  	struct rev_info rev;
> >  	struct commit *commit;
> >  	const char *argv[] = {NULL, NULL, "--not", "NULL", NULL};
> >  	int argc = ARRAY_SIZE(argv) - 1;
> >  	char *sha1_copy;
> > -	int needs_pushing = 0;
> > +
> >  	struct strbuf remotes_arg = STRBUF_INIT;
> >  
> >  	strbuf_addf(&remotes_arg, "--remotes=%s", remotes_name);
> > @@ -401,14 +401,14 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remote
> >  	if (prepare_revision_walk(&rev))
> >  		die("revision walk setup failed");
> >  
> > -	while ((commit = get_revision(&rev)) && !needs_pushing)
> > -		commit_need_pushing(commit, &needs_pushing);
> > +	while ((commit = get_revision(&rev)) != NULL)
> > +		commit_need_pushing(commit, needs_pushing);
> 
> Now the helper function to find list of submodules that need pushing given
> one commit starting to look more and more misnamed.  It used to be "learn
> if something needs pushing", but now it is "find what needs pushing".
> 
> Can somebody think of a good adjective to describe a submodule (or a set
> of submodules) in this state, so that we can name this helper function
> find_blue_submodules(), if the adjective were "blue"?
> 
> "Unpushed" submodule is the word used in the later part of the patch.