Re: [PATCH 2/2] serialize collection of refs that contain submodule changes

[Junio C Hamano <gitster@pobox.com>]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> 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)?

> 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?

>  		}
>  
>  		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.