Christian Couder <christian.couder@xxxxxxxxx> writes:
> Pushing NULL into a 'strvec' results in a segfault, as 'strvec' is a
> kind of NULL terminated array that is designed to be compatible with
> 'argv' variables used on the command line.
It is good that you corrected a caller that tries to make a strvec
into such a state, but shouldn't strvec_push() protect itself with a
BUG or something, I have to wonder.
> So when an URL is missing from the config, let's push an empty string
> instead of NULL into the 'strvec' that stores URLs.
How will these strings in the "names" strvec used? When URLs are
present, I'm sure we are going to use it as URL, but when they are
missing, what happens? The second hunk of this patch seems to
ignore such a broken entry with an empty URL, but that smells like
sweeping a problem under the rug. Shouldn't such a promisor be
either diagnosed as an error, or skipped when you accumulate URLs to
be used into the strvec, so that the later code (i.e. the second
hunk) does not even have to worry about it?
> @@ -356,7 +360,7 @@ char *promisor_remote_info(struct repository *repo)
> strbuf_addch(&sb, ';');
> strbuf_addstr(&sb, "name=");
> strbuf_addstr_urlencode(&sb, names.v[i], allow_unsanitized);
> - if (urls.v[i]) {
> + if (urls.v[i] && *urls.v[i]) {
Why does urls.v[i] need to be checked? Didn't you just make sure
that the strvec would not have NULL in it?
