| Message ID | 20220303005727.69270-6-chooglen@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | submodule: convert parts of 'update' to C | expand |
Glen Choo <chooglen@google.com> writes: > Move the logic of "git submodule--helper ensure-core-worktree" into > run-update-procedure. Since the ensure-core-worktree command is I take it as "... command is now obsolete", or "... has become obsolete"? > obsolete, remove it. > > Signed-off-by: Glen Choo <chooglen@google.com> > --- > builtin/submodule--helper.c | 12 ++---------- > git-submodule.sh | 2 -- > 2 files changed, 2 insertions(+), 12 deletions(-) On the script side, the removed call to ensure-core-worktree used to precede these invocations of the helper submodule--helper relative-path submodule--helper remote-branch submodule--helper print-default-remote before we triggered run-update-procedure, so these helper calls were done only after we made sure we have a submodule there at the path and its configuration file has core.worktree set correctly. If we failed to do so, we wouldn't have made these calls. Now we call them unprotected. It is not immediately obvious if that is a safe/sensible thing to do. I would imagine that we would lose more and more code from the script in the "while" loop before run-update-procedure is triggered, and presumably the equivalent code will be added _after_ the call to ensure_core_worktree() this patch adds to the beginning of update_submodule2(), so in the end, the above will presumably become a non-issue, but the series structure still feels iffy because of it.
Junio C Hamano <gitster@pobox.com> writes: > Glen Choo <chooglen@google.com> writes: > >> Move the logic of "git submodule--helper ensure-core-worktree" into >> run-update-procedure. Since the ensure-core-worktree command is > > I take it as "... command is now obsolete", or "... has become > obsolete"? Yes, that's a better phrasing. > On the script side, the removed call to ensure-core-worktree used > to precede these invocations of the helper > > submodule--helper relative-path > submodule--helper remote-branch > submodule--helper print-default-remote > > before we triggered run-update-procedure, so these helper calls were > done only after we made sure we have a submodule there at the path > and its configuration file has core.worktree set correctly. If we > failed to do so, we wouldn't have made these calls. > > Now we call them unprotected. It is not immediately obvious if that > is a safe/sensible thing to do. > > I would imagine that we would lose more and more code from the > script in the "while" loop before run-update-procedure is triggered, > and presumably the equivalent code will be added _after_ the call to > ensure_core_worktree() this patch adds to the beginning of > update_submodule2(), so in the end, the above will presumably become > a non-issue, but the series structure still feels iffy because of it. I could restructure this series so that this patch is as late as possible, so we don't have to worry about safety for 'remote-branch' and 'print-default-remote', but we still have to consider 'relative-path' because that's still around by the end of this series. Fortunately, relative_path is just a wrapper around dir.h's relative_path(), which AFAICT is just general purpose string manipulation and has nothing to do with Git. (I'm also certain that the other two commands don't interact with core.worktree either, but I'll just restructure the series so that the point is moot.)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 77ca4270f4..6b473fc0d2 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2771,17 +2771,11 @@ static int push_check(int argc, const char **argv, const char *prefix) return 0; } -static int ensure_core_worktree(int argc, const char **argv, const char *prefix) +static void ensure_core_worktree(const char *path) { - const char *path; const char *cw; struct repository subrepo; - if (argc != 2) - BUG("submodule--helper ensure-core-worktree <path>"); - - path = argv[1]; - if (repo_submodule_init(&subrepo, the_repository, path, null_oid())) die(_("could not get a repository handle for submodule '%s'"), path); @@ -2801,8 +2795,6 @@ static int ensure_core_worktree(int argc, const char **argv, const char *prefix) free(abs_path); strbuf_release(&sb); } - - return 0; } static int absorb_git_dirs(int argc, const char **argv, const char *prefix) @@ -3029,6 +3021,7 @@ static int module_create_branch(int argc, const char **argv, const char *prefix) /* NEEDSWORK: this is a temporary name until we delete update_submodule() */ static int update_submodule2(struct update_data *update_data) { + ensure_core_worktree(update_data->sm_path); if (update_data->just_cloned) oidcpy(&update_data->suboid, null_oid()); else if (resolve_gitlink_ref(update_data->sm_path, "HEAD", &update_data->suboid)) @@ -3428,7 +3421,6 @@ static struct cmd_struct commands[] = { {"add", module_add, SUPPORT_SUPER_PREFIX}, {"update-clone", update_clone, 0}, {"run-update-procedure", run_update_procedure, 0}, - {"ensure-core-worktree", ensure_core_worktree, 0}, {"relative-path", resolve_relative_path, 0}, {"resolve-relative-url-test", resolve_relative_url_test, 0}, {"foreach", module_foreach, SUPPORT_SUPER_PREFIX}, diff --git a/git-submodule.sh b/git-submodule.sh index 32a09302ab..458ce73ac6 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -402,8 +402,6 @@ cmd_update() do die_if_unmatched "$quickabort" "$sha1" - git submodule--helper ensure-core-worktree "$sm_path" || exit 1 - displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix") if test $just_cloned -eq 0
Move the logic of "git submodule--helper ensure-core-worktree" into run-update-procedure. Since the ensure-core-worktree command is obsolete, remove it. Signed-off-by: Glen Choo <chooglen@google.com> --- builtin/submodule--helper.c | 12 ++---------- git-submodule.sh | 2 -- 2 files changed, 2 insertions(+), 12 deletions(-)