Daniel Barkalow [off-list ref] 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.
quoted hunk
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.
On Sun, 16 Mar 2008, Junio C Hamano wrote:
Daniel Barkalow [off-list ref] writes:
quoted
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.
quoted
... 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.
quoted
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.
quoted
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*
On Sun, 16 Mar 2008, Junio C Hamano wrote:
Daniel Barkalow [off-list ref] writes:
quoted
We don't currently have any concept of an invalid refspec;
We don't? or just that parse_ref_spec() does not detect one?
quoted
... 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.
Yeah, we're definitely too lenient. t3200-branch has been using the
refspec "=" since July without anybody noticing that it's wrong.
-Daniel
*This .sig left intentionally blank*