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

From: Jeff King <peff@peff.net>
Date: May 31, 2017, 04:23 UTC
Message-ID: <20170531042304.unkxkuw6s3tcpzkm@sigill.intra.peff.net>
In-Reply-To: <20170530071244.32257-1-szeder.dev@gmail.com>

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

Show 20 quoted lines

> 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