On Wed, Mar 12, 2025 at 12:06 AM Jeff King <peff@xxxxxxxx> wrote:
>
> 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);
Yeah, right I am using this in the next version. I have added warnings
to help debug this in the case a remote is rejected because urls are
different, as I think it could confuse users.
> > 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.
Yeah, I forgot about it. I think it makes sense to replace xstrdup()
with xstrdup_or_null() in strvec_push().
If we ever want a mode (possibly the default one) that forbids NULL in
strvec, we could add that on top. But right now as strvec_push_nodup()
accepts NULL, I think it makes sense for strvec_push() to accept NULL
too.
Anyway this is something we can work on after the release.
> 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.
I don't mind having both xstrdup() and xstrdup_or_null(). At least it
gives a hint to readers about NULL being expected or not.
> 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.
Yeah, but then if we ever need a strvec like struct that can contain
NULL, it would be kind of sad to have a separate struct with its own
files mostly duplicating the strvec code. I think we would then be
better with strvec having two modes, one accepting NULL and one
rejecting it.
> > 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).
Yeah, I will use this in the next version.
Thanks for the review.
