Re: tracking repository

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 17, 2008, 00:35 UTC
Message-ID: <7vlk4ichm4.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <alpine.LNX.1.00.0803161716470.19665@iabervon.org>

Daniel Barkalow <barkalow@iabervon.org> writes:

> We don't currently have any concept of an invalid refspec;

We don't? or just that parse_ref_spec() does not detect one?

> ... we just have 
> things that fall back to not being patterns and not being possible to 
> match (due to one or the other side being invalid as a ref name).

I am afraid that is an invitation for more bugs and confusions.

It probably is not too late to fix this; users would rather want to see their misconfigurations clearly flagged as such, rather than the code letting bogosity through silently and doing something that does not exactly match what they configured.

Show 13 quoted lines

> diff --git a/remote.c b/remote.c
> index f3f7375..fffde34 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -404,18 +404,17 @@ struct refspec *parse_ref_spec(int nr_refspec, const char **refspec)
>  			rs[i].force = 1;
>  			sp++;
>  		}
> -		gp = strchr(sp, '*');
> +		gp = strstr(sp, "/*");
>  		ep = strchr(sp, ':');
>  		if (gp && ep && gp > ep)
>  			gp = NULL;

How would this trigger? We find * (or /*) but that is the one on the LHS, which means the spec was like "refs/heads/foobar:refs/remotes/origin/*", and it makes me wonder if we should mark this as an configuration error.

Did erroring out on "gp && ep && gp > ep" here have issues (i.e. reject a valid configuration)?

Show 10 quoted lines

>  		if (ep) {
>  			if (ep[1]) {
> -				const char *glob = strchr(ep + 1, '*');
> +				const char *glob = strstr(ep + 1, "/*");
>  				if (!glob)
>  					gp = NULL;
>  				if (gp)
> -					rs[i].dst = xstrndup(ep + 1,
> -							     glob - ep - 1);
> +					rs[i].dst = xstrndup(ep + 1, glob - ep);

This truncates "refs/heads/*:refs/remotes/origin/*/bar" as if it did not have "/bar" without any error indication. The same questions apply.