[PATCH 4/5] Keep '*' in pattern refspecs

[PATCH 4/5] Keep '*' in pattern refspecs

[Daniel Barkalow]

In order to do anything more capable with refspecs, the first step is
to keep the entire input. Additionally, validate patterns by checking
for the ref matching the rules for a pattern as given by
check_ref_format(). This requires a slight change to make it require
the '*' to be at the beginning of a path component.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
 refs.c   |    4 +---
 remote.c |   24 +++++++++---------------
 2 files changed, 10 insertions(+), 18 deletions(-)

diff --git a/refs.c b/refs.c
index 6eb5f53..a50ba79 100644
--- a/refs.c
+++ b/refs.c
@@ -718,9 +718,7 @@ int check_ref_format(const char *ref)
 		while ((ch = *cp++) != 0) {
 			bad_type = bad_ref_char(ch);
 			if (bad_type) {
-				return (bad_type == 2 && !*cp)
-					? CHECK_REF_FORMAT_WILDCARD
-					: CHECK_REF_FORMAT_ERROR;
+				return CHECK_REF_FORMAT_ERROR;
 			}
 			if (ch == '/')
 				break;
diff --git a/remote.c b/remote.c
index 93fd03d..d0ce4c6 100644
--- a/remote.c
+++ b/remote.c
@@ -10,8 +10,8 @@ static struct refspec s_tag_refspec = {
 	0,
 	1,
 	0,
-	"refs/tags/",
-	"refs/tags/"
+	"refs/tags/*",
+	"refs/tags/*"
 };
 
 const struct refspec *tag_refspec = &s_tag_refspec;
@@ -451,16 +451,11 @@ static void read_config(void)
  */
 static int verify_refname(char *name, int is_glob)
 {
-	int result, len = -1;
+	int result;
 
-	if (is_glob) {
-		len = strlen(name);
-		assert(name[len - 1] == '/');
-		name[len - 1] = '\0';
-	}
 	result = check_ref_format(name);
-	if (is_glob)
-		name[len - 1] = '/';
+	if (is_glob && result == CHECK_REF_FORMAT_WILDCARD)
+		result = CHECK_REF_FORMAT_OK;
 	return result;
 }
 
@@ -517,7 +512,7 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp
 		if (rhs) {
 			size_t rlen = strlen(++rhs);
 			is_glob = (2 <= rlen && !strcmp(rhs + rlen - 2, "/*"));
-			rs[i].dst = xstrndup(rhs, rlen - is_glob);
+			rs[i].dst = xstrndup(rhs, rlen);
 		}
 
 		llen = (rhs ? (rhs - lhs - 1) : strlen(lhs));
@@ -525,7 +520,6 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp
 			if ((rhs && !is_glob) || (!rhs && fetch))
 				goto invalid;
 			is_glob = 1;
-			llen--;
 		} else if (rhs && is_glob) {
 			goto invalid;
 		}
@@ -722,10 +716,10 @@ int remote_has_url(struct remote *remote, const char *url)
 static int name_fits_pattern(const char *key, const char *name,
 			     const char *value, char **result)
 {
-	size_t klen = strlen(key);
-	int ret = !strncmp(key, name, klen);
+	size_t klen = strchr(key, '*') - key;
+	int ret = !strncmp(name, key, klen);
 	if (ret && value) {
-		size_t vlen = strlen(value);
+		size_t vlen = strchr(value, '*') - value;
 		*result = xmalloc(vlen +
 				  strlen(name) -
 				  klen + 1);
-- 
1.6.1.286.gd33a4.dirty

Re: [PATCH 4/5] Keep '*' in pattern refspecs

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> In order to do anything more capable with refspecs, the first step is
> to keep the entire input. Additionally, validate patterns by checking
> for the ref matching the rules for a pattern as given by
> check_ref_format(). This requires a slight change to make it require
> the '*' to be at the beginning of a path component.

I had a brief "Huh?" moment wondering about this "slight change", but at
this stage it does not change the rule at all ("/*" still must happen at
the end of the string), so there actually is no change.

> diff --git a/remote.c b/remote.c
> index 93fd03d..d0ce4c6 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -722,10 +716,10 @@ int remote_has_url(struct remote *remote, const char *url)
>  static int name_fits_pattern(const char *key, const char *name,
>  			     const char *value, char **result)
>  {
> -	size_t klen = strlen(key);
> -	int ret = !strncmp(key, name, klen);
> +	size_t klen = strchr(key, '*') - key;
> +	int ret = !strncmp(name, key, klen);

Any particular reason why the first parameters to strncmp() were swapped?

>  	if (ret && value) {
> -		size_t vlen = strlen(value);
> +		size_t vlen = strchr(value, '*') - value;

We would want protection from programming error here, to catch keys and
values without any asterisk.  This comment also applies to [5/5].

>  		*result = xmalloc(vlen +
>  				  strlen(name) -
>  				  klen + 1);

Re: [PATCH 4/5] Keep '*' in pattern refspecs

[Daniel Barkalow]

On Fri, 6 Mar 2009, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > In order to do anything more capable with refspecs, the first step is
> > to keep the entire input. Additionally, validate patterns by checking
> > for the ref matching the rules for a pattern as given by
> > check_ref_format(). This requires a slight change to make it require
> > the '*' to be at the beginning of a path component.
> 
> I had a brief "Huh?" moment wondering about this "slight change", but at
> this stage it does not change the rule at all ("/*" still must happen at
> the end of the string), so there actually is no change.

Ah, yes. A slight change to the code in refs.c to enforce the rule that 
was enforced previously in remote.c.

> > diff --git a/remote.c b/remote.c
> > index 93fd03d..d0ce4c6 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -722,10 +716,10 @@ int remote_has_url(struct remote *remote, const char *url)
> >  static int name_fits_pattern(const char *key, const char *name,
> >  			     const char *value, char **result)
> >  {
> > -	size_t klen = strlen(key);
> > -	int ret = !strncmp(key, name, klen);
> > +	size_t klen = strchr(key, '*') - key;
> > +	int ret = !strncmp(name, key, klen);
> 
> Any particular reason why the first parameters to strncmp() were swapped?

An artifact of how I cleaned up the series that I didn't notice; I 
originally wrote it from scratch with strncmp, with an arbitrary argument 
order. Then I split out 2/5 using prefixcmp, and 3/5 keeping prefixcmp's 
order, and didn't notice I didn't need the line of diff. So not really.

> >  	if (ret && value) {
> > -		size_t vlen = strlen(value);
> > +		size_t vlen = strchr(value, '*') - value;
> 
> We would want protection from programming error here, to catch keys and
> values without any asterisk.  This comment also applies to [5/5].

Good idea.

> >  		*result = xmalloc(vlen +
> >  				  strlen(name) -
> >  				  klen + 1);
> 

	-Daniel
*This .sig left intentionally blank*

[PATCH v2 4/5] Keep '*' in pattern refspecs

[Daniel Barkalow]

In order to do anything more capable with refspecs, the first step is
to keep the entire input. Additionally, validate patterns by checking
for the ref matching the rules for a pattern as given by
check_ref_format(). This requires a slight change to
check_ref_format() to make it enforce the requirement that the '*'
immediately follow a '/'.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
Don't arbitrarily swap argument order; check for missing '*' in pattern 
sides.

 refs.c   |    4 +---
 remote.c |   35 +++++++++++++++++++----------------
 2 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/refs.c b/refs.c
index 6eb5f53..a50ba79 100644
--- a/refs.c
+++ b/refs.c
@@ -718,9 +718,7 @@ int check_ref_format(const char *ref)
 		while ((ch = *cp++) != 0) {
 			bad_type = bad_ref_char(ch);
 			if (bad_type) {
-				return (bad_type == 2 && !*cp)
-					? CHECK_REF_FORMAT_WILDCARD
-					: CHECK_REF_FORMAT_ERROR;
+				return CHECK_REF_FORMAT_ERROR;
 			}
 			if (ch == '/')
 				break;
diff --git a/remote.c b/remote.c
index 5638766..d596a48 100644
--- a/remote.c
+++ b/remote.c
@@ -10,8 +10,8 @@ static struct refspec s_tag_refspec = {
 	0,
 	1,
 	0,
-	"refs/tags/",
-	"refs/tags/"
+	"refs/tags/*",
+	"refs/tags/*"
 };
 
 const struct refspec *tag_refspec = &s_tag_refspec;
@@ -451,16 +451,11 @@ static void read_config(void)
  */
 static int verify_refname(char *name, int is_glob)
 {
-	int result, len = -1;
+	int result;
 
-	if (is_glob) {
-		len = strlen(name);
-		assert(name[len - 1] == '/');
-		name[len - 1] = '\0';
-	}
 	result = check_ref_format(name);
-	if (is_glob)
-		name[len - 1] = '/';
+	if (is_glob && result == CHECK_REF_FORMAT_WILDCARD)
+		result = CHECK_REF_FORMAT_OK;
 	return result;
 }
 
@@ -517,7 +512,7 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp
 		if (rhs) {
 			size_t rlen = strlen(++rhs);
 			is_glob = (2 <= rlen && !strcmp(rhs + rlen - 2, "/*"));
-			rs[i].dst = xstrndup(rhs, rlen - is_glob);
+			rs[i].dst = xstrndup(rhs, rlen);
 		}
 
 		llen = (rhs ? (rhs - lhs - 1) : strlen(lhs));
@@ -525,7 +520,6 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp
 			if ((rhs && !is_glob) || (!rhs && fetch))
 				goto invalid;
 			is_glob = 1;
-			llen--;
 		} else if (rhs && is_glob) {
 			goto invalid;
 		}
@@ -720,12 +714,21 @@ int remote_has_url(struct remote *remote, const char *url)
 }
 
 static int match_name_with_pattern(const char *key, const char *name,
 				   const char *value, char **result)
 {
-	size_t klen = strlen(key);
-	int ret = !strncmp(key, name, klen);
+	const char *kstar = strchr(key, '*');
+	size_t klen;
+	int ret;
+	if (!kstar)
+		die("Key '%s' of pattern had no '*'", key);
+	klen = kstar - key;
+	ret = !strncmp(key, name, klen);
 	if (ret && value) {
-		size_t vlen = strlen(value);
+		const char *vstar = strchr(value, '*');
+		size_t vlen;
+		if (!vstar)
+			die("Value '%s' of pattern has no '*'", value);
+		vlen = vstar - value;
 		*result = xmalloc(vlen +
 				  strlen(name) -
 				  klen + 1);
-- 
1.6.1.286.gd33a4.dirty