[PATCH] submodule: use cheaper check for submodule pushes

In the function push_submodule[1] we use add_submodule_odb[2] to determine
if a submodule has been populated. However the function does not work with
the submodules objects that are added, instead a new child process is used
to perform the actual push in the submodule.

Use is_submodule_populated[3] that is cheaper to guard from unpopulated
submodules.

[1] 'push_submodule' was added in eb21c732d6 (push: teach
    --recurse-submodules the on-demand option, 2012-03-29)
[2] 'add_submodule_odb' was introduced in 752c0c2492 (Add the
    --submodule option to the diff option family, 2009-10-19)
[3] 'is_submodule_populated' was added in 5688c28d81 (submodules:
    add helper to determine if a submodule is populated, 2016-12-16)

Signed-off-by: Stefan Beller <redacted>
---
 submodule.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/submodule.c b/submodule.c
index da2b484879..55afad3e8c 100644
--- a/submodule.c
+++ b/submodule.c

@@ -976,7 +976,9 @@ static int push_submodule(const char *path,
 			  const struct string_list *push_options,
 			  int dry_run)
 {
-	if (add_submodule_odb(path))
+	int code;
+
+	if (!is_submodule_populated_gently(path, &code))
 		return 1;
 
 	if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {

-- 
2.13.2.695.g117ddefdb4

Hi,

Stefan Beller wrote:

In the function push_submodule[1] we use add_submodule_odb[2] to determine
if a submodule has been populated. However the function does not work with
the submodules objects that are added, instead a new child process is used
to perform the actual push in the submodule.

Use is_submodule_populated[3] that is cheaper to guard from unpopulated
submodules.

[1] 'push_submodule' was added in eb21c732d6 (push: teach
    --recurse-submodules the on-demand option, 2012-03-29)
[2] 'add_submodule_odb' was introduced in 752c0c2492 (Add the
    --submodule option to the diff option family, 2009-10-19)
[3] 'is_submodule_populated' was added in 5688c28d81 (submodules:
    add helper to determine if a submodule is populated, 2016-12-16)

These footnotes don't answer the question that I really have: why did
this use add_submodule_odb in the first place?

E.g. did the ref iteration code require access to the object store
previously and stop requiring it later?

Should this examine the code to distinguish between hard errors
(e.g. "Error reading .git") and a missing repository?

Thanks,
Jonathan

On Wed, Jul 12, 2017 at 5:01 PM, Jonathan Nieder [off-list ref] wrote:

These footnotes don't answer the question that I really have: why did
this use add_submodule_odb in the first place?

oh, I forgot to put that down: presumably add_submodule_odb was
used because it was available back then? Note the dates!

E.g. did the ref iteration code require access to the object store
previously and stop requiring it later?

No. See [1], it was overkill since the beginning of time.

quoted

-     if (add_submodule_odb(path))
+     int code;
+
+     if (!is_submodule_populated_gently(path, &code))

Should this examine the code to distinguish between hard errors
(e.g. "Error reading .git") and a missing repository?

add_submodule_odb does neither, so I think this is best kept without
additional checks.

Thanks!