| Message ID | 20181011211754.31369-19-sbeller@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bring more repository handles into our code base | expand |
> 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.
> 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)
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(&cp.args, "push");
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 <sbeller@google.com> --- submodule.c | 3 --- 1 file changed, 3 deletions(-)