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

[Heiko Voigt <hvoigt@hvoigt.net>]

On Mon, Oct 10, 2016 at 03:56:13PM -0700, Junio C Hamano wrote:
> 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.

First the naming part. How about:

	submodule_has_commits()

?

Second as mentioned a previous answer[1] to this part: I would actually
like to have a die() here instead of blindly proceeding. Since the user
either specified --recurse-submodules=... at the commandline or it was
implicitly enabled because we have submodules in the tree we should be
careful and not push revisions referencing submodules that are not
available at a remote. If we can not properly figure it out I would
suggest to stop and tell the user how to solve the situation. E.g.
either she clones the appropriate submodules or specifies
--no-recurse-submodules on the commandline to tell git that she does not
care.

Returning 0 here means: "No push needed" but the correct answer would
be: "We do not know". Question is what we should do here which I am
planning to address in a separate patch series since that will be
changing behavior.

So how about:


	if (!submodule_has_hashes(path, hashes))
		/* NEEDSWORK: The correct answer here is "We do not
		 * know" instead of "No". We currently proceed pushing
		 * here as if the submodules commits are available on a
		 * remote, which is not always correct. */
		return 0;

What do you think?

Cheers Heiko

[1] http://public-inbox.org/git/20160919195812.GC62429@book.hvoigt.net/