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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Heiko Voigt <hvoigt@xxxxxxxxxx> writes:

> This allows us to tell the user which submodules have not been pushed.
> Additionally this is helpful when we want to automatically try to push
> submodules that have not been pushed.

Makes sense.

> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]