Re: [PATCH v5 2/3] Refactor submodule push check to use string list instead of integer
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 14, 2012, 03:28 UTC
- Message-ID
- <7vbop29jqf.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <20120213092900.GC15585@t1405.greatnet.de>
Heiko Voigt <hvoigt@hvoigt.net> 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.
Show 20 quoted lines
> 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.
Show 36 quoted lines
> }
> }
> }
>
> -
> -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.