Re: tracking repository

[Junio C Hamano <gitster@pobox.com>]

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.

> 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)?

>  		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.