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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jul 13, 2017, 20:48 UTC
Message-ID: <20170713204854.GA158114@aiede.mtv.corp.google.com>
In-Reply-To: <CAGZ79kYtRbHvfC6d=+eEXCceJam4wwo9XqvY752c_pg6kq90DA@mail.gmail.com>

Hi,

Stefan Beller wrote:

Show 8 quoted lines

> 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.

Thanks for looking into it. This is what the commit message should say to help reviewers or people trying to understand it later. The footnotes don't help and are distracting, except that it makes sense to point out the original GSoC patch to say the alternate submodule odb wasn't needed even then.

E.g.:

 Subject: push: do not add submodule odb as an alternate when recursing on demand

 "git push --recurse-submodules=on-demand" adds each submodule as an
 alternate with add_submodule_odb before checking whether the
 submodule has anything to push and pushing it if so.

 However, it never accesses any objects from the submodule.  In the
 parent process it uses the submodule's ref database to see if there
 is anything to push.  The actual push (which does rely on objects)
 occurs in a child process.

 The same was try when this call was originally added in
 v1.7.11-rc0~111^2 (push: teach --recurse-submodules the on-demand
 option, 2012-03-29).  Most likely it was added by analogy with
 fetch --recurse-submodules=on-demand, which did use the submodule's
 object database.

 Use is_submodule_populated_gently instead, which is simpler and
 cheaper.

[...]

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

Show 5 quoted lines

>> 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".

With such a commit message change, this seems like a reasonable change in principle (though I haven't looked carefully to verify it).

My one doubt is the is_submodule_populated_gently. Why are we using that instead of simpler is_submodule_populated? The names and API comments don't explain.

Thanks for your patient explanations, Jonathan