On Mon, Oct 10, 2016 at 03:56:13PM -0700, Junio C Hamano wrote:
> Heiko Voigt <hvoigt@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx/