Re: [PATCH] submodule: use cheaper check for submodule pushes

From: Stefan Beller <sbeller@google.com>
Date: Jul 13, 2017, 19:39 UTC
Message-ID: <CAGZ79kYtRbHvfC6d=+eEXCceJam4wwo9XqvY752c_pg6kq90DA@mail.gmail.com>
In-Reply-To: <xmqqa848xjxr.fsf@gitster.mtv.corp.google.com>

On Thu, Jul 13, 2017 at 11:37 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 10 quoted lines

>
> I think Jonathan's question (which I concurred) is if we also ended
> up relying on the side effect of calling that function (i.e. being
> able to now find objects that are not in our repository but in the
> submodule's object store).  By looking at the eb21c732d6, we can
> tell that the original didn't mean to and didn't add any code that
> relies on the ability to be able to read from the submodule object
> store.  I am not sure if that is still true after 5 years (i.e. is
> there any new code added in the meantime that made us depend on the
> ability to read from submodule object store?).

Yes we are safe, because the function itself only spawns a child process (not using any of the objects).

It's only caller push_unpushed_submodules also doesn't rely on objects loaded after calling push_submodule.

The caller of push_unpushed_submodules (transport.c, transport_push) also doesn't need submodule objects loaded.

> My hunch (and hope) is that we are probably safe, but that is a lot
> weaker than "yes this is a good change we want to apply".

Given the above (I went through the code), all I can do is repeating "yes this is a good change we want to apply".

Thanks, Stefan