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

[Stefan Beller]

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

t5531-deep-submodule-push.sh (all of them)
t5545-push-options.sh (ok 4 - push options and submodules)

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

[Jonathan Tan]

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

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

[Stefan Beller]

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.

Signed-off-by: Stefan Beller 
---
 submodule.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/submodule.c b/submodule.c
index 5e1a6c0b7c..f70d75ef45 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1006,9 +1006,6 @@ static int push_submodule(const char *path,
  const struct string_list *push_options,
  int dry_run)
 {
-   if (add_submodule_odb(path))
-   return 1;
-
if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
struct child_process cp = CHILD_PROCESS_INIT;
argv_array_push(, "push");
-- 
2.19.0