Hi,
On Fri, Sep 16, 2016 at 10:47:46AM -0700, Junio C Hamano wrote:
> 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)?
I am not sure if I understand you correctly here. With the "ref cache layer"
you are referring to add_submodule_odb() which is called indirectly from
submodule_needs_pushing()? Those revs are only used to check whether the hash
we need on the remote side exists in the local submodule. That should not
change due to a push. The actual check whether the commit(s) exist on the
remote side is done using a 'rev-list' in a subprocess later.
> > 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?
Good catch, sorry about that. Will clear the hashes array.
> > }
> >
> > 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.
As above, will fix.
Cheers Heiko