Re: [PATCHv4 1/2] clone: respect additional configured fetch refspecs during initial fetch

[Jeff King <peff@peff.net>]

On Tue, May 30, 2017 at 09:12:43AM +0200, SZEDER Gábor wrote:

> diff --git a/remote.c b/remote.c
> index ad6c5424e..b8fd09dc9 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -626,6 +626,19 @@ struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec)
>  	return parse_refspec_internal(nr_refspec, refspec, 1, 0);
>  }
>  
> +void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec)
> +{
> +	struct refspec *rs;
> +
> +	add_fetch_refspec(remote, refspec);
> +	rs = parse_fetch_refspec(1, &refspec);
> +	REALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr);
> +	remote->fetch[remote->fetch_refspec_nr - 1] = *rs;
> +
> +	/* Not free_refspecs(), as we copied its pointers above */
> +	free(rs);
> +}

What happens here if remote->fetch isn't already initialized? I think
we'd end up with a bunch of garbage values. That's what I was trying to
protect against in my original suggestion.

I'm not sure if that's possible or not. We seem to initialize it in both
remote_get() and for_each_remote(), and I don't think there are any
other ways to get a remote. (In fact, I kind of wondered why we do this
parsing lazily at all, but I think it's so that misconfigured remotes
don't cause us to die() if we aren't accessing them at all).

If we assume that the contract that remote.c provides is that the
fetch fields are always filled in before a "struct remote" is returned
to a caller, and that only such callers would use this function, it's
OK. It just seems more dangerous than it needs to be.

-Peff