Re: tracking repository

From: Daniel Barkalow <barkalow@iabervon.org>
Date: Mar 17, 2008, 02:13 UTC
Message-ID: <alpine.LNX.1.00.0803162143230.19665@iabervon.org>
In-Reply-To: <7vlk4ichm4.fsf@gitster.siamese.dyndns.org>

On Sun, 16 Mar 2008, Junio C Hamano wrote:

Show 5 quoted lines

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

I don't think we ever formalized anything; it just makes sure not to actually create anything bad, and doesn't give any feedback on the configuration.

Show 10 quoted lines

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

I think I wasn't sure that aborting on invalid input wouldn't cause worse problems. There were actually a number of tests, IIRC, that required that certain configurations silently did nothing, which I mostly left alone.

Also, it's possible that we're parsing refspecs because we're using "git 
remote" to modify the configuration to replace an invalid refspec, and it 
would be unfortunate to die() because the remote has an invalid refspec.

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

It's that case (there's a *, but not until after the :); I think the history was that the code first just looked for <a>:<b>, and used it for non-pattern matches, and then started using <a>/*:<b>/* as a pattern first, and then I made the C version match that shell version.

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

Nope.

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

That one was just an oversight. I was just glad I didn't have to make it actually support that refspec and have it do the obvious (but annoying to implement) thing.

	-Daniel
*This .sig left intentionally blank*