Re: [PATCH v3] builtin/submodule--helper.c: handle missing submodule URLs

From: Jeff King <peff@peff.net>
Date: May 24, 2023, 20:33 UTC
Message-ID: <20230524203321.GD892557@coredump.intra.peff.net>
In-Reply-To: <ae6cf3fa461b85e346f034371dae56a2790dfa20.1684957882.git.me@ttaylorr.com>

On Wed, May 24, 2023 at 03:51:43PM -0400, Taylor Blau wrote:

Show 15 quoted lines

> 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