On Tue, Mar 11, 2025 at 04:24:13PM +0100, Christian Couder wrote:
> Using strvec_push() to push `NULL` into a 'strvec' results in a
> segfault, because `xstrdup(NULL)` crashes.
>
> So when an URL is missing from the config, let's push an empty string
> instead of `NULL` into the 'strvec' that stores URLs.
Is a configured remote with out a url key really a missing url, though?
In other contexts it defaults to the name of the remote. E.g.:
# make a repo so "foo" is a valid url
git init foo
git -C foo commit --allow-empty bar
# configure a fetch refspec, but no url!
git init
git config remote.foo.fetch '+refs/heads/*:refs/remotes/foo/*'
# now fetching will use the configured refspec with a url of "foo"
git fetch foo
# and git-remote will report it, along with its url
git remote ;# shows "foo"
git remote --get-url foo ;# also shows "foo"
This is obviously a weird thing to be doing, so I admit I don't really
care all that much. But it feels like the most natural thing is just:
diff --git a/promisor-remote.c b/promisor-remote.c
index 6a0a61382f..761eb1dbd5 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -327,7 +327,7 @@ static void promisor_info_vecs(struct repository *repo,
char *url_key = xstrfmt("remote.%s.url", r->name);
strvec_push(names, r->name);
- strvec_push(urls, git_config_get_string(url_key, &url) ? NULL : url);
+ strvec_push(urls, git_config_get_string(url_key, &url) ? r->name : url);
free(url);
free(url_key);
> We could have modified strvec_push() to behave like
> strvec_push_nodup() and accept `NULL`, but it's not clear that it's
> the right thing to do for the strvec API. 'strvec' is a kind of NULL
> terminated array that is designed to be compatible with 'argv'
> variables used on the command line. So we might want to disallow
> pushing any `NULL` in it instead.
>
> It's also not clear if `xstrdup(NULL)` should crash or BUG or just
> return NULL.
We have xstrdup_or_null() for the latter suggestion. There was some
light discussion at the time about having xstrdup(NULL) handle this
automatically:
https://lore.kernel.org/git/20150112231231.GA4023@xxxxxxxx/
but it was mostly negative. I don't think anybody really dug into the
thought experiment beyond a general "it might propagate NULL places you
wouldn't expect" vibe, though.
For the same reason I'd be a little hesitant to bless NULLs inside
strvec structures. I think "nodup" allowing them is mostly an unintended
consequence.
> For all these reasons, let's just focus on fixing the issue in
> "promisor-remote.c" and let's leave improving the strvec API and/or
> xstrdup() for a future effort.
This part I certainly agree with. ;)
> for (r = repo->promisor_remote_config->promisors; r; r = r->next) {
> - char *url;
> + char *url = NULL;
> + const char *url_pushed = "";
> char *url_key = xstrfmt("remote.%s.url", r->name);
>
> + if (!git_config_get_string(url_key, &url) && url)
> + url_pushed = url;
> +
> strvec_push(names, r->name);
> - strvec_push(urls, git_config_get_string(url_key, &url) ? NULL : url);
> + strvec_push(urls, url_pushed);
>
> free(url);
Probably not super important, but while reading this I noticed that
using git_config_get_string_tmp() would make the memory management a
little simpler (since you do not need to free "url", you are free to
point it to at the empty string and do not need a separate url_pushed).
-Peff
