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

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

Heiko Voigt <hvoigt@hvoigt.net> writes:

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