threads / patch / 30550

patcharchive: simplify refname handling

Subject: [PATCH] archive: simplify refname handling

## tl;dr

5 messages between May 18, 2012 and May 18, 2012. Diffs are folded; open one to read it.

replies: 4people: 4as markdown or json

René Scharfe· May 18, 2012, 05:15 UTC · lore

There is no need to build a copy of the relevant part of the string just to make sure we have a NUL-terminated string. We can simply pass the length of the interesting part to dwim_ref() instead.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 archive.c |   17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)
Show changes to archive.c +5 −12
diff --git a/archive.c b/archive.c
index cd083ea..ecc1ff0 100644
--- a/archive.c
+++ b/archive.c
@@ -254,18 +254,11 @@ static void parse_treeish_arg(const char **argv,
 	/* Remotes are only allowed to fetch actual refs */
 	if (remote) {
 		char *ref = NULL;
-		const char *refname, *colon = NULL;
-
-		colon = strchr(name, ':');
-		if (colon)
-			refname = xstrndup(name, colon - name);
-		else
-			refname = name;
-
-		if (!dwim_ref(refname, strlen(refname), sha1, &ref))
-			die("no such ref: %s", refname);
-		if (refname != name)
-			free((void *)refname);
+		const char *colon = strchr(name, ':');
+		size_t refnamelen = colon ? colon - name : strlen(name);
+
+		if (!dwim_ref(name, refnamelen, sha1, &ref))
+			die("no such ref: %.*s", refnamelen, name);
 		free(ref);
 	}
 

-- 
1.7.10.2
Jeff King· May 18, 2012, 15:59 UTC · re: René Scharfe · lore

Re: [PATCH] archive: simplify refname handling

[+cc Carlos, as this is his code]
On Fri, May 18, 2012 at 07:15:17AM +0200, René Scharfe wrote:
Show 35 quoted lines
> There is no need to build a copy of the relevant part of the string just
> to make sure we have a NUL-terminated string.  We can simply pass the
> length of the interesting part to dwim_ref() instead.
> 
> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
> ---
>  archive.c |   17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/archive.c b/archive.c
> index cd083ea..ecc1ff0 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -254,18 +254,11 @@ static void parse_treeish_arg(const char **argv,
>  	/* Remotes are only allowed to fetch actual refs */
>  	if (remote) {
>  		char *ref = NULL;
> -		const char *refname, *colon = NULL;
> -
> -		colon = strchr(name, ':');
> -		if (colon)
> -			refname = xstrndup(name, colon - name);
> -		else
> -			refname = name;
> -
> -		if (!dwim_ref(refname, strlen(refname), sha1, &ref))
> -			die("no such ref: %s", refname);
> -		if (refname != name)
> -			free((void *)refname);
> +		const char *colon = strchr(name, ':');
> +		size_t refnamelen = colon ? colon - name : strlen(name);
> +
> +		if (!dwim_ref(name, refnamelen, sha1, &ref))
> +			die("no such ref: %.*s", refnamelen, name);
>  		free(ref);
Looks obviously correct to me.
-Peff
Carlos Martín Nieto· May 18, 2012, 16:44 UTC · re: Jeff King · lore

Re: [PATCH] archive: simplify refname handling

On Fri, 2012-05-18 at 11:59 -0400, Jeff King wrote:
Show 41 quoted lines
> [+cc Carlos, as this is his code]
> 
> On Fri, May 18, 2012 at 07:15:17AM +0200, René Scharfe wrote:
> 
> > There is no need to build a copy of the relevant part of the string just
> > to make sure we have a NUL-terminated string.  We can simply pass the
> > length of the interesting part to dwim_ref() instead.
> > 
> > Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
> > ---
> >  archive.c |   17 +++++------------
> >  1 file changed, 5 insertions(+), 12 deletions(-)
> > 
> > diff --git a/archive.c b/archive.c
> > index cd083ea..ecc1ff0 100644
> > --- a/archive.c
> > +++ b/archive.c
> > @@ -254,18 +254,11 @@ static void parse_treeish_arg(const char **argv,
> >  	/* Remotes are only allowed to fetch actual refs */
> >  	if (remote) {
> >  		char *ref = NULL;
> > -		const char *refname, *colon = NULL;
> > -
> > -		colon = strchr(name, ':');
> > -		if (colon)
> > -			refname = xstrndup(name, colon - name);
> > -		else
> > -			refname = name;
> > -
> > -		if (!dwim_ref(refname, strlen(refname), sha1, &ref))
> > -			die("no such ref: %s", refname);
> > -		if (refname != name)
> > -			free((void *)refname);
> > +		const char *colon = strchr(name, ':');
> > +		size_t refnamelen = colon ? colon - name : strlen(name);
> > +
> > +		if (!dwim_ref(name, refnamelen, sha1, &ref))
> > +			die("no such ref: %.*s", refnamelen, name);
> >  		free(ref);
> 
> Looks obviously correct to me.
Yep, that's clearly a much better way of doing it.
   cmn
Junio C Hamano· May 18, 2012, 18:27 UTC · re: René Scharfe · lore

Re: [PATCH] archive: simplify refname handling

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
Show 7 quoted lines
> +		const char *colon = strchr(name, ':');
> +		size_t refnamelen = colon ? colon - name : strlen(name);
> +
> +		if (!dwim_ref(name, refnamelen, sha1, &ref))
> +			die("no such ref: %.*s", refnamelen, name);
>  		free(ref);
>  	}

Both dwim_ref()'s second parameter the vararg that corresponds to the length part of "%.*s" in die(), expect "int", not "size_t", so I needed this to get it compile.

 archive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to archive.c +1 −1
diff --git a/archive.c b/archive.c
index 081ecb5..1e7156d 100644
--- a/archive.c
+++ b/archive.c
@@ -261,7 +261,7 @@ static void parse_treeish_arg(const char **argv,
 	if (remote) {
 		char *ref = NULL;
 		const char *colon = strchr(name, ':');
-		size_t refnamelen = colon ? colon - name : strlen(name);
+		int refnamelen = colon ? colon - name : strlen(name);
 
 		if (!dwim_ref(name, refnamelen, sha1, &ref))
 			die("no such ref: %.*s", refnamelen, name);

← back to recent threads