| Message ID | ae6cf3fa461b85e346f034371dae56a2790dfa20.1684957882.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | fbc806acd106ee1c05fd0a0a83b7c59aa79629d8 |
| Headers | show |
| Series | [v3] builtin/submodule--helper.c: handle missing submodule URLs | expand |
Am 24.05.23 um 21:51 schrieb Taylor Blau: > There is no need to check whether `sub` > itself is NULL, since we already perform this check earlier in > `prepare_to_clone_next_submodule()`. Right, and if "sub" is NULL then next_submodule_warn_missing() is called and prepare_to_clone_next_submodule() is exited early. > By adding a NULL-ness check on `sub->url`, we'll fall into the 'else' > branch, setting `url` to `sub->url` (which is NULL). Before attempting > to invoke `git submodule--helper clone`, check whether `url` is NULL, > and die() if it is. Why die() here instead of just warn and skip as well? René
On Wed, May 24, 2023 at 03:51:43PM -0400, Taylor Blau wrote: > In e0a862fdaf (submodule helper: convert relative URL to absolute URL if > needed, 2018-10-16), `prepare_to_clone_next_submodule()` lost the > ability to handle URL-less submodules, due to a change from: > > if (repo_get_config_string_const(the_repostiory, sb.buf, &url)) > url = sub->url; > > to > > if (repo_get_config_string_const(the_repostiory, sb.buf, &url)) { > if (starts_with_dot_slash(sub->url) || > starts_with_dot_dot_slash(sub->url)) { > /* ... */ > } > } This patch looks pretty good to me. I read your v1 and the word "gross" also crossed my mind at the "--url" handling. This one is much better. I did have a few questions, though (below). If I understand correctly, this is not at all new in the -rc releases, but just something that happened to get unearthed? I.e., it can wait until post-release. > , which will segfault when `sub->url` is NULL, since both > `starts_with_dot_slash()` does not guard its arguments as non-NULL. Funny gramm-o, presumably from editing: "both" is plural, but "does" and "its" are singular. I think the gist of it is communicated, though. > Guard the checks to both of the above functions by first checking > whether `sub->url` is non-NULL. There is no need to check whether `sub` > itself is NULL, since we already perform this check earlier in > `prepare_to_clone_next_submodule()`. Good, thanks for checking (and communicating) that possible gotha. > By adding a NULL-ness check on `sub->url`, we'll fall into the 'else' > branch, setting `url` to `sub->url` (which is NULL). Before attempting > to invoke `git submodule--helper clone`, check whether `url` is NULL, > and die() if it is. If I hadn't read v1, I might wonder whether this die() is consistent with the existing behavior. But the point is that submodule--helper would have barfed in such a case anyway, so we are just trading one error for another. One side effect, though, is that this die() will take down the whole superproject process. Whereas I think the intent of the submodule code is to keep going, handling other submodules, even if one fails. This isn't a failure exactly (more of a misconfiguration, if I understand it). But should we be somehow returning an error instead? I say "somehow" because it's not clear how to work that in with the needs_cloning return value (obviously we can say "0", but that is the same as the "skipped" code path; we presumably want to tell the caller there was a failure, so it affects the ultimate return code). > +test_expect_success 'update submodules without url set in .gitconfig' ' Should this be .gitmodules in the title? -Peff
On Wed, May 24, 2023 at 10:29:39PM +0200, René Scharfe wrote: > Am 24.05.23 um 21:51 schrieb Taylor Blau: > > > By adding a NULL-ness check on `sub->url`, we'll fall into the 'else' > > branch, setting `url` to `sub->url` (which is NULL). Before attempting > > to invoke `git submodule--helper clone`, check whether `url` is NULL, > > and die() if it is. > > Why die() here instead of just warn and skip as well? That's a good point. When I read prepare_to_clone_next_submodule(), I thought that it was already too late to skip that submodule. But my understanding was incorrect, we could easily issue a warning() and return '0', which would indicate to skip it. But I concur with Junio earlier in the thread that we don't have to rush things to get this into -rc2 or even 2.41, since this bug has been with us since v2.20.0. Thanks, Taylor
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 6bf8d666ce..6a16208e8a 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2024,14 +2024,17 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, strbuf_reset(&sb); strbuf_addf(&sb, "submodule.%s.url", sub->name); if (repo_config_get_string_tmp(the_repository, sb.buf, &url)) { - if (starts_with_dot_slash(sub->url) || - starts_with_dot_dot_slash(sub->url)) { + if (sub->url && (starts_with_dot_slash(sub->url) || + starts_with_dot_dot_slash(sub->url))) { url = resolve_relative_url(sub->url, NULL, 0); need_free_url = 1; } else url = sub->url; } + if (!url) + die(_("cannot clone submodule '%s' without a URL"), sub->name); + strbuf_reset(&sb); strbuf_addf(&sb, "%s/.git", ce->name); needs_cloning = !file_exists(sb.buf); diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index eae6a46ef3..d9fbabb2b9 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -1351,6 +1351,22 @@ test_expect_success 'clone active submodule without submodule url set' ' ) ' +test_expect_success 'update submodules without url set in .gitconfig' ' + test_when_finished "rm -rf multisuper_clone" && + git clone file://"$pwd"/multisuper multisuper_clone && + + git -C multisuper_clone submodule init && + for s in sub0 sub1 sub2 sub3 + do + key=submodule.$s.url && + git -C multisuper_clone config --local --unset $key && + git -C multisuper_clone config --file .gitmodules --unset $key || return 1 + done && + + test_must_fail git -C multisuper_clone submodule update 2>err && + grep "cannot clone submodule .sub[0-3]. without a URL" err +' + test_expect_success 'clone --recurse-submodules with a pathspec works' ' test_when_finished "rm -rf multisuper_clone" && cat >expected <<-\EOF &&
In e0a862fdaf (submodule helper: convert relative URL to absolute URL if needed, 2018-10-16), `prepare_to_clone_next_submodule()` lost the ability to handle URL-less submodules, due to a change from: if (repo_get_config_string_const(the_repostiory, sb.buf, &url)) url = sub->url; to if (repo_get_config_string_const(the_repostiory, sb.buf, &url)) { if (starts_with_dot_slash(sub->url) || starts_with_dot_dot_slash(sub->url)) { /* ... */ } } , which will segfault when `sub->url` is NULL, since both `starts_with_dot_slash()` does not guard its arguments as non-NULL. Guard the checks to both of the above functions by first checking whether `sub->url` is non-NULL. There is no need to check whether `sub` itself is NULL, since we already perform this check earlier in `prepare_to_clone_next_submodule()`. By adding a NULL-ness check on `sub->url`, we'll fall into the 'else' branch, setting `url` to `sub->url` (which is NULL). Before attempting to invoke `git submodule--helper clone`, check whether `url` is NULL, and die() if it is. Reported-by: Tribo Dar <3bodar@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Range-diff against v2: 1: 38202878b5 ! 1: ae6cf3fa46 builtin/submodule--helper.c: handle missing submodule URLs @@ Commit message , which will segfault when `sub->url` is NULL, since both `starts_with_dot_slash()` does not guard its arguments as non-NULL. - Guard the checks to both of the above functions by first whether - `sub->url` is non-NULL. There is no need to check whether `sub` itself - is NULL, since we already perform this check earlier in + Guard the checks to both of the above functions by first checking + whether `sub->url` is non-NULL. There is no need to check whether `sub` + itself is NULL, since we already perform this check earlier in `prepare_to_clone_next_submodule()`. By adding a NULL-ness check on `sub->url`, we'll fall into the 'else' @@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st strbuf_reset(&sb); strbuf_addf(&sb, "%s/.git", ce->name); needs_cloning = !file_exists(sb.buf); -@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce, - if (suc->update_data->filter_options && suc->update_data->filter_options->choice) - strvec_pushf(&child->args, "--filter=%s", - expand_list_objects_filter_spec(suc->update_data->filter_options)); -+ strvec_pushl(&child->args, "--url", url, NULL); - if (suc->update_data->require_init) - strvec_push(&child->args, "--require-init"); - strvec_pushl(&child->args, "--path", sub->path, NULL); - strvec_pushl(&child->args, "--name", sub->name, NULL); -- strvec_pushl(&child->args, "--url", url, NULL); - if (suc->update_data->references.nr) { - struct string_list_item *item; - ## t/t7400-submodule-basic.sh ## @@ t/t7400-submodule-basic.sh: test_expect_success 'clone active submodule without submodule url set' ' builtin/submodule--helper.c | 7 +++++-- t/t7400-submodule-basic.sh | 16 ++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-)