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