Re: [PATCH 2/2] serialize collection of refs that contain submodule changes
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 16, 2016, 17:47 UTC
- Message-ID
- <xmqqd1k3losd.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20160914175130.GB7613@sandbox>
Heiko Voigt <hvoigt@hvoigt.net> writes:
Show 44 quoted lines
> diff --git a/submodule.c b/submodule.c
> index b04c066..a15e346 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -627,24 +627,31 @@ static void free_submodules_sha1s(struct string_list *submodules)
> string_list_clear(submodules, 1);
> }
>
> -int find_unpushed_submodules(unsigned char new_sha1[20],
> +static void append_hash_to_argv(const unsigned char sha1[20],
> + void *data)
> +{
> + struct argv_array *argv = (struct argv_array *) data;
> + argv_array_push(argv, sha1_to_hex(sha1));
> +}
> +
> +int find_unpushed_submodules(struct sha1_array *hashes,
> 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, i;
> - char *sha1_copy;
> + int i;
> struct string_list submodules = STRING_LIST_INIT_DUP;
> + struct argv_array argv = ARGV_ARRAY_INIT;
>
> - struct strbuf remotes_arg = STRBUF_INIT;
> -
> - strbuf_addf(&remotes_arg, "--remotes=%s", remotes_name);
> init_revisions(&rev, NULL);
> - sha1_copy = xstrdup(sha1_to_hex(new_sha1));
> - argv[1] = sha1_copy;
> - argv[3] = remotes_arg.buf;
> - setup_revisions(argc, argv, &rev, NULL);
> +
> + /* argv.argv[0] will be ignored by setup_revisions */
> + argv_array_push(&argv, "find_unpushed_submodules");
> + sha1_array_for_each_unique(hashes, append_hash_to_argv, &argv);
> + argv_array_push(&argv, "--not");
> + argv_array_pushf(&argv, "--remotes=%s", remotes_name);
> +
> + setup_revisions(argv.argc, argv.argv, &rev, NULL);Yes, its about time to for us to lose that fixed-size argv[] and replace it with an argv-array ;-).
> if (prepare_revision_walk(&rev))
> die("revision walk setup failed");So this one used to get a single commit at the tip of what we pushed in the superproject and was asked "Look at the history we just pushed leading to the tip commit, and tell me if any of the ones new to the remote requires submodule commits the remote does not yet have". Now the caller collects all the tip commits and asks us once: "Here are the new tips we just pushed; in the history leading to them, is there a commit that the remote did not have that requires submodule history the remote does not yet have?".
Makes sort-of sense.
I speculated that you would be doing the same kind of optimization to feed all positive commits to rev-list at once in each submodule repository in the review of the previous one, but you didn't do it here. You did the same for superproject in this step. Perhaps 3 or 4 would do so in the submodule repository.
One thing that makes me worried is how the ref cache layer interacts with this. I see you first call push_unpushed_submodules() when ON_DEMAND is set, which would result in pushes in submodule repositories, updating their remote tracking branches. At that point, before you make another call to find_unpushed_submodules(), is our cached ref layer knows that the remote tracking branches are now up to date (otherwise, we would incorrectly judge that these submodules need pushing based on stale information)?
Show 20 quoted lines
> diff --git a/transport.c b/transport.c
> index 94d6dc3..76e1daf 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -903,23 +903,29 @@ int transport_push(struct transport *transport,
>
> if ((flags & TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND) && !is_bare_repository()) {
> struct ref *ref = remote_refs;
> + struct sha1_array hashes = SHA1_ARRAY_INIT;
> +
> for (; ref; ref = ref->next)
> - if (!is_null_oid(&ref->new_oid) &&
> - !push_unpushed_submodules(ref->new_oid.hash,
> - transport->remote->name))
> - die ("Failed to push all needed submodules!");
> + if (!is_null_oid(&ref->new_oid))
> + sha1_array_append(&hashes, ref->new_oid.hash);
> +
> + if (!push_unpushed_submodules(&hashes, transport->remote->name))
> + die ("Failed to push all needed submodules!");Do we leak the contents of hashes here?
Show 19 quoted lines
> }
>
> if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
> TRANSPORT_RECURSE_SUBMODULES_CHECK)) && !is_bare_repository()) {
> struct ref *ref = remote_refs;
> struct string_list needs_pushing = STRING_LIST_INIT_DUP;
> + struct sha1_array hashes = SHA1_ARRAY_INIT;
>
> for (; ref; ref = ref->next)
> - if (!is_null_oid(&ref->new_oid) &&
> - find_unpushed_submodules(ref->new_oid.hash,
> - transport->remote->name, &needs_pushing))
> - die_with_unpushed_submodules(&needs_pushing);
> + if (!is_null_oid(&ref->new_oid))
> + sha1_array_append(&hashes, ref->new_oid.hash);
> +
> + if (find_unpushed_submodules(&hashes, transport->remote->name,
> + &needs_pushing))
> + die_with_unpushed_submodules(&needs_pushing);Do we leak the contents of hashes here? I do not think we need to worry about needs_pushing leaking, as we will always die if it is not empty, but it might be a better code hygiene to clear it as well.
> } > > push_ret = transport->push_refs(transport, remote_refs, flags);
Thanks.