Re: [PATCH 18/19] submodule: don't add submodule as odb for push

From: Jonathan Tan <jonathantanmy@google.com>
Date: Oct 11, 2018, 23:00 UTC
Message-ID: <20181011230028.200488-1-jonathantanmy@google.com>
In-Reply-To: <20181011211754.31369-19-sbeller@google.com>

Show 11 quoted lines

> The submodule was added as an alternative in eb21c732d6 (push: teach
> --recurse-submodules the on-demand option, 2012-03-29), but was
> not explained, why.
> 
> In similar code, submodule_has_commits, the submodule is added as an
> alternative to perform a quick check if we need to dive into the submodule.
> 
> However in push_submodule
> (a) for_each_remote_ref_submodule will also provide the quick check and
> (b) after that we don't need to have submodule objects around, as all
>     further code is to spawn a separate process.

After some investigation, I think I understand. I would explain it this way:

  In push_submodule(), because we do not actually need access to objects
  in the submodule, do not invoke add_submodule_odb().
  (for_each_remote_ref_submodule() does not require access to those
  objects, and the actual push is done by spawning another process,
  which handles object access by itself.)

I'm not sure if it's worth mentioning the commit in which the call was introduced, since nothing seems to have changed between then and now (the same bug is present when it was introduced, and now).

I also checked the users of push_submodule() (transport_push()) and indeed it doesn't seem to make use of the additional objects brought in by add_submodule_odb().

Do you know if pushing of submodules is exercised by any test?

Other than that, the code itself looks good.