Re: [PATCH 2/2] serialize collection of refs that contain submodule changes
- From
Heiko Voigt <hvoigt@hvoigt.net>
- Date
- Sep 19, 2016, 19:51 UTC
- Message-ID
- <20160919195118.GB62429@book.hvoigt.net>
- In-Reply-To
- <xmqqd1k3losd.fsf@gitster.mtv.corp.google.com>
Hi,
On Fri, Sep 16, 2016 at 10:47:46AM -0700, Junio C Hamano wrote:
Show 8 quoted lines
> 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.
Show 22 quoted lines
> > 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.
Show 24 quoted lines
> > }
> >
> > 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