* [PATCH v2] builtin/submodule--helper.c: handle missing submodule URLs
2023-05-24 16:02 ` [PATCH] builtin/submodule--helper.c: handle missing submodule URLs Taylor Blau
@ 2023-05-24 16:25 ` Taylor Blau
2023-05-24 18:48 ` Eric Sunshine
2023-05-24 19:51 ` [PATCH v3] " Taylor Blau
2023-05-24 22:58 ` [PATCH] " Jeff King
2 siblings, 1 reply; 12+ messages in thread
From: Taylor Blau @ 2023-05-24 16:25 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Tribo Dar, Stefan Beller
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 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>
---
Junio: feel free to queue either, but after thinking on v1 a little more
I think that it is pretty gross and that this version is much cleaner.
Range-diff against v1:
1: f7a8de14fe ! 1: bc204f450a builtin/submodule--helper.c: handle missing submodule URLs
@@ Commit message
`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). When we then try
- and invoke `git submodule-helper` below, we'll get the expected error
- message:
-
- error: option `url' requires a value
-
- Note that this means that `--url` and its value must come last when
- preparing the argument list, to avoid confusing the next argument as the
- value for `--url` if `url` itself is NULL.
+ 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>
@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st
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);
@@ 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);
@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const st
if (suc->update_data->references.nr) {
struct string_list_item *item;
-@@ builtin/submodule--helper.c: static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
- strvec_push(&child->args, suc->update_data->single_branch ?
- "--single-branch" :
- "--no-single-branch");
-+ /*
-+ * `url` might be NULL, so ensure that this argument is passed
-+ * last to avoid confusing the next argument as the value for
-+ * `--url`.
-+ */
-+ strvec_pushl(&child->args, "--url", url, NULL);
-
- cleanup:
- free(displaypath);
## t/t7400-submodule-basic.sh ##
@@ t/t7400-submodule-basic.sh: test_expect_success 'clone active submodule without submodule url set' '
@@ t/t7400-submodule-basic.sh: test_expect_success 'clone active submodule without
+ done &&
+
+ test_must_fail git -C multisuper_clone submodule update 2>err &&
-+ grep "option .url. requires a value" err
++ grep "cannot clone submodule .sub[0-3]. without a URL" err
+'
+
test_expect_success 'clone --recurse-submodules with a pathspec works' '
builtin/submodule--helper.c | 9 ++++++---
t/t7400-submodule-basic.sh | 16 ++++++++++++++++
2 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6bf8d666ce..efd0761cd0 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);
@@ -2065,11 +2068,11 @@ 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;
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 &&
--
2.41.0.rc1.11.gf7a8de14fe.dirty
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2] builtin/submodule--helper.c: handle missing submodule URLs
2023-05-24 16:25 ` [PATCH v2] " Taylor Blau
@ 2023-05-24 18:48 ` Eric Sunshine
2023-05-24 19:50 ` Taylor Blau
0 siblings, 1 reply; 12+ messages in thread
From: Eric Sunshine @ 2023-05-24 18:48 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Tribo Dar, Stefan Beller
On Wed, May 24, 2023 at 12:40 PM Taylor Blau <me@ttaylorr.com> 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)) {
> /* ... */
> }
> }
>
> , 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
s/first/first checking/
> `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>
> ---
> diff --git 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,
> - 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);
Good. The first version of this patch was more difficult to reason
about due to its "error-at-a-distance" approach. This version is much
cleaner and obvious.
> @@ -2065,11 +2068,11 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
> 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);
This change is unnecessary now, isn't it? Or is there something
nonobvious going on here?
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2] builtin/submodule--helper.c: handle missing submodule URLs
2023-05-24 18:48 ` Eric Sunshine
@ 2023-05-24 19:50 ` Taylor Blau
0 siblings, 0 replies; 12+ messages in thread
From: Taylor Blau @ 2023-05-24 19:50 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git, Junio C Hamano, Tribo Dar, Stefan Beller
On Wed, May 24, 2023 at 02:48:40PM -0400, Eric Sunshine wrote:
> On Wed, May 24, 2023 at 12:40 PM Taylor Blau <me@ttaylorr.com> wrote:
> > Guard the checks to both of the above functions by first whether
>
> s/first/first checking/
Oops. Good eyes, thanks for spotting.
> > @@ -2065,11 +2068,11 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
> > 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);
>
> This change is unnecessary now, isn't it? Or is there something
> nonobvious going on here?
Yeah, this is a stray diff. I'll send a cleaned up version as soon as
'make test' finishes ;-).
Thanks,
Taylor
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3] builtin/submodule--helper.c: handle missing submodule URLs
2023-05-24 16:02 ` [PATCH] builtin/submodule--helper.c: handle missing submodule URLs Taylor Blau
2023-05-24 16:25 ` [PATCH v2] " Taylor Blau
@ 2023-05-24 19:51 ` Taylor Blau
2023-05-24 20:29 ` René Scharfe
2023-05-24 20:33 ` Jeff King
2023-05-24 22:58 ` [PATCH] " Jeff King
2 siblings, 2 replies; 12+ messages in thread
From: Taylor Blau @ 2023-05-24 19:51 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Tribo Dar, Stefan Beller, Eric Sunshine
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(-)
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 &&
--
2.41.0.rc1.11.gae6cf3fa46
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v3] builtin/submodule--helper.c: handle missing submodule URLs
2023-05-24 19:51 ` [PATCH v3] " Taylor Blau
@ 2023-05-24 20:29 ` René Scharfe
2023-05-24 20:36 ` Taylor Blau
2023-05-24 20:33 ` Jeff King
1 sibling, 1 reply; 12+ messages in thread
From: René Scharfe @ 2023-05-24 20:29 UTC (permalink / raw)
To: Taylor Blau, git; +Cc: Junio C Hamano, Tribo Dar, Stefan Beller, Eric Sunshine
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é
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] builtin/submodule--helper.c: handle missing submodule URLs
2023-05-24 20:29 ` René Scharfe
@ 2023-05-24 20:36 ` Taylor Blau
0 siblings, 0 replies; 12+ messages in thread
From: Taylor Blau @ 2023-05-24 20:36 UTC (permalink / raw)
To: René Scharfe
Cc: git, Junio C Hamano, Tribo Dar, Stefan Beller, Eric Sunshine
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] builtin/submodule--helper.c: handle missing submodule URLs
2023-05-24 19:51 ` [PATCH v3] " Taylor Blau
2023-05-24 20:29 ` René Scharfe
@ 2023-05-24 20:33 ` Jeff King
1 sibling, 0 replies; 12+ messages in thread
From: Jeff King @ 2023-05-24 20:33 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Tribo Dar, Stefan Beller, Eric Sunshine
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] builtin/submodule--helper.c: handle missing submodule URLs
2023-05-24 16:02 ` [PATCH] builtin/submodule--helper.c: handle missing submodule URLs Taylor Blau
2023-05-24 16:25 ` [PATCH v2] " Taylor Blau
2023-05-24 19:51 ` [PATCH v3] " Taylor Blau
@ 2023-05-24 22:58 ` Jeff King
2 siblings, 0 replies; 12+ messages in thread
From: Jeff King @ 2023-05-24 22:58 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Tribo Dar, Stefan Beller
On Wed, May 24, 2023 at 12:02:26PM -0400, Taylor Blau wrote:
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 6bf8d666ce..a7b7ea374f 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2024,8 +2024,8 @@ 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
Oh, btw, this need_free_url made me look at the memory handling for the
other side of the else. I think it is all good, but it would be a little
simpler using an extra pointer instead of a boolean:
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6a16208e8a..6b7849b828 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1984,7 +1984,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
ud->super_prefix);
struct strbuf sb = STRBUF_INIT;
int needs_cloning = 0;
- int need_free_url = 0;
+ char *to_free_url = NULL;
if (ce_stage(ce)) {
strbuf_addf(out, _("Skipping unmerged submodule %s"), displaypath);
@@ -2025,10 +2025,9 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
strbuf_addf(&sb, "submodule.%s.url", sub->name);
if (repo_config_get_string_tmp(the_repository, sb.buf, &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
+ starts_with_dot_dot_slash(sub->url)))
+ url = to_free_url = resolve_relative_url(sub->url, NULL, 0);
+ else
url = sub->url;
}
@@ -2089,8 +2088,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
cleanup:
free(displaypath);
strbuf_release(&sb);
- if (need_free_url)
- free((void*)url);
+ free(to_free_url);
return needs_cloning;
}
Probably at this point that is just churn, but I thought I'd throw it
out there.
-Peff
^ permalink raw reply related [flat|nested] 12+ messages in thread