On Thu, Jul 13, 2017 at 1:48 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote: > Hi, > > Stefan Beller wrote: > >> 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. Thanks for giving a good example of commit message that I could use in a reroll. > 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. One could posit this is laziness of thinking. See 15cdc64776 (make is_submodule_populated gently, 2017-03-14), and discover there is no non-gentle version of is_submodule_populated. And for each new use, it may be cheaper to just use the gentle version instead of adding a non-gentle version. Thanks, Stefan
