Re: [PATCH v2 3/3] batch check whether submodule needs pushing into one call

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 10, 2016, 22:56 UTC
Message-ID: <xmqqlgxvbype.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <67d4c48dc0129f20041c88d27a49c7a21188c882.1475851621.git.hvoigt@hvoigt.net>

Heiko Voigt <hvoigt@hvoigt.net> writes:

Show 27 quoted lines

> -static int submodule_needs_pushing(const char *path, const unsigned char sha1[20])
> +static int check_has_hash(const unsigned char sha1[20], void *data)
>  {
> -	if (add_submodule_odb(path) || !lookup_commit_reference(sha1))
> +	int *has_hash = (int *) data;
> +
> +	if (!lookup_commit_reference(sha1))
> +		*has_hash = 0;
> +
> +	return 0;
> +}
> +
> +static int submodule_has_hashes(const char *path, struct sha1_array *hashes)
> +{
> +	int has_hash = 1;
> +
> +	if (add_submodule_odb(path))
> +		return 0;
> +
> +	sha1_array_for_each_unique(hashes, check_has_hash, &has_hash);
> +	return has_hash;
> +}
> +
> +static int submodule_needs_pushing(const char *path, struct sha1_array *hashes)
> +{
> +	if (!submodule_has_hashes(path, hashes))
>  		return 0;

Same comment about naming.

What do check-has-hash and submodule-has-hashes exactly mean by "hash" in their names? Because I think what is checked here is "does the local submodule repository have _all_ the commits referenced from the superproject commit we are pushing?", so I'd prefer to see "commit" in their names.

If we do not even have these commits locally, then there is no point attempting to push, so returning 0 (i.e. it is not "needs pushing" situation) is correct but it is a but subtle. It's not "we know they already have them", but it is "even if we tried to push, it won't do us or the other side any good." A single-liner in-code comment may help.

Thanks.