[PATCH 1/3] add alloc_ref_with_prefix()

[PATCH 1/3] add alloc_ref_with_prefix()

[René Scharfe]

In three cases in remote.c, a "raw" ref is allocated using alloc_ref()
and then its is constructed using sprintf().  Clean it up by adding a
helper function, alloc_ref_with_prefix(), which creates a composite
name.  Use it in alloc_ref_from_str(), too, as it simplifies the code.

Open code alloc_ref() in alloc_ref_with_prefix(), as the former is
going to be removed in the patch after the next.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 remote.c |   32 ++++++++++++++++----------------
 1 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/remote.c b/remote.c
index 8a04066..98cbcf9 100644
--- a/remote.c
+++ b/remote.c
@@ -749,6 +749,16 @@ int remote_find_tracking(struct remote *remote, struct refspec *refspec)
 	return -1;
 }
 
+static struct ref *alloc_ref_with_prefix(const char *prefix, size_t prefixlen,
+		const char *name)
+{
+	size_t len = strlen(name);
+	struct ref *ref = xcalloc(1, sizeof(struct ref) + prefixlen + len + 1);
+	memcpy(ref->name, prefix, prefixlen);
+	memcpy(ref->name + prefixlen, name, len);
+	return ref;
+}
+
 struct ref *alloc_ref(unsigned namelen)
 {
 	struct ref *ret = xcalloc(1, sizeof(struct ref) + namelen);
@@ -757,9 +767,7 @@ struct ref *alloc_ref(unsigned namelen)
 
 struct ref *alloc_ref_from_str(const char* str)
 {
-	struct ref *ret = alloc_ref(strlen(str) + 1);
-	strcpy(ret->name, str);
-	return ret;
+	return alloc_ref_with_prefix("", 0, str);
 }
 
 static struct ref *copy_ref(const struct ref *ref)
@@ -1152,10 +1160,8 @@ static struct ref *get_expanded_map(const struct ref *remote_refs,
 			struct ref *cpy = copy_ref(ref);
 			match = ref->name + remote_prefix_len;
 
-			cpy->peer_ref = alloc_ref(local_prefix_len +
-						  strlen(match) + 1);
-			sprintf(cpy->peer_ref->name, "%s%s",
-				refspec->dst, match);
+			cpy->peer_ref = alloc_ref_with_prefix(refspec->dst,
+					local_prefix_len, match);
 			if (refspec->force)
 				cpy->peer_ref->force = 1;
 			*tail = cpy;
@@ -1188,7 +1194,6 @@ struct ref *get_remote_ref(const struct ref *remote_refs, const char *name)
 
 static struct ref *get_local_ref(const char *name)
 {
-	struct ref *ret;
 	if (!name)
 		return NULL;
 
@@ -1198,15 +1203,10 @@ static struct ref *get_local_ref(const char *name)
 
 	if (!prefixcmp(name, "heads/") ||
 	    !prefixcmp(name, "tags/") ||
-	    !prefixcmp(name, "remotes/")) {
-		ret = alloc_ref(strlen(name) + 6);
-		sprintf(ret->name, "refs/%s", name);
-		return ret;
-	}
+	    !prefixcmp(name, "remotes/"))
+		return alloc_ref_with_prefix("refs/", 5, name);
 
-	ret = alloc_ref(strlen(name) + 12);
-	sprintf(ret->name, "refs/heads/%s", name);
-	return ret;
+	return alloc_ref_with_prefix("refs/heads/", 11, name);
 }
 
 int get_fetch_map(const struct ref *remote_refs,
-- 
1.6.0.2.542.gde55

[PATCH 2/3] use alloc_ref_from_str() everywhere

[René Scharfe]

Replace pairs of alloc_ref() and strcpy() with alloc_ref_from_str(),
simplifying the code.

In connect.c, also a pair of alloc_ref() and memcpy() is replaced --
the additional cost of a strlen() call should not have too much of an
impact.  Consistency and simplicity are more important.

In remote.c, the code was allocating 11 bytes more than needed for
the name part, but I couldn't see them being used for anything.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 connect.c   |    3 +--
 remote.c    |    3 +--
 transport.c |    6 ++----
 3 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/connect.c b/connect.c
index 67d2cd8..b69060b 100644
--- a/connect.c
+++ b/connect.c
@@ -90,9 +90,8 @@ struct ref **get_remote_heads(int in, struct ref **list,
 			continue;
 		if (nr_match && !path_match(name, nr_match, match))
 			continue;
-		ref = alloc_ref(name_len + 1);
+		ref = alloc_ref_from_str(buffer + 41);
 		hashcpy(ref->old_sha1, old_sha1);
-		memcpy(ref->name, buffer + 41, name_len + 1);
 		*list = ref;
 		list = &ref->next;
 	}
diff --git a/remote.c b/remote.c
index 98cbcf9..44d681d 100644
--- a/remote.c
+++ b/remote.c
@@ -878,8 +878,7 @@ static struct ref *try_explicit_object_name(const char *name)
 	struct ref *ref;
 
 	if (!*name) {
-		ref = alloc_ref(20);
-		strcpy(ref->name, "(delete)");
+		ref = alloc_ref_from_str("(delete)");
 		hashclr(ref->new_sha1);
 		return ref;
 	}
diff --git a/transport.c b/transport.c
index 5110c56..3d03475 100644
--- a/transport.c
+++ b/transport.c
@@ -75,15 +75,14 @@ static int read_loose_refs(struct strbuf *path, int name_offset,
 
 			if (fd < 0)
 				continue;
-			next = alloc_ref(path->len - name_offset + 1);
+			next = alloc_ref_from_str(path->buf + name_offset);
 			if (read_in_full(fd, buffer, 40) != 40 ||
 					get_sha1_hex(buffer, next->old_sha1)) {
 				close(fd);
 				free(next);
 				continue;
 			}
 			close(fd);
-			strcpy(next->name, path->buf + name_offset);
 			(*tail)->next = next;
 			*tail = next;
 		}
@@ -127,14 +126,13 @@ static void insert_packed_refs(const char *packed_refs, struct ref **list)
 				      (*list)->next->name)) > 0)
 			list = &(*list)->next;
 		if (!(*list)->next || cmp < 0) {
-			struct ref *next = alloc_ref(len - 40);
+			struct ref *next = alloc_ref_from_str(buffer + 41);
 			buffer[40] = '\0';
 			if (get_sha1_hex(buffer, next->old_sha1)) {
 				warning ("invalid SHA-1: %s", buffer);
 				free(next);
 				continue;
 			}
-			strcpy(next->name, buffer + 41);
 			next->next = (*list)->next;
 			(*list)->next = next;
 			list = &(*list)->next;
-- 
1.6.0.2.542.gde55

[PATCH 3/3] make alloc_ref_from_str() the new alloc_ref()

[René Scharfe]

With all calls to alloc_ref() gone, we can remove it and then we're free
to give alloc_ref_from_str() the shorter name.  It's a much nicer
interface, as the callers always need to have a name string when they
allocate a ref anyway and don't need to calculate and pass its length+1
any more.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 builtin-fetch.c |    4 ++--
 connect.c       |    2 +-
 http-push.c     |    4 ++--
 remote.c        |   21 +++++++--------------
 remote.h        |    4 +---
 transport.c     |    8 ++++----
 walker.c        |    2 +-
 7 files changed, 18 insertions(+), 27 deletions(-)

diff --git a/builtin-fetch.c b/builtin-fetch.c
index ee93d3a..e008ee9 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -521,8 +521,8 @@ static void find_non_local_tags(struct transport *transport,
 		     will_fetch(head, ref->old_sha1))) {
 			string_list_insert(ref_name, &new_refs);
 
-			rm = alloc_ref_from_str(ref_name);
-			rm->peer_ref = alloc_ref_from_str(ref_name);
+			rm = alloc_ref(ref_name);
+			rm->peer_ref = alloc_ref(ref_name);
 			hashcpy(rm->old_sha1, ref_sha1);
 
 			**tail = rm;
diff --git a/connect.c b/connect.c
index b69060b..0c50d0a 100644
--- a/connect.c
+++ b/connect.c
@@ -90,7 +90,7 @@ struct ref **get_remote_heads(int in, struct ref **list,
 			continue;
 		if (nr_match && !path_match(name, nr_match, match))
 			continue;
-		ref = alloc_ref_from_str(buffer + 41);
+		ref = alloc_ref(buffer + 41);
 		hashcpy(ref->old_sha1, old_sha1);
 		*list = ref;
 		list = &ref->next;
diff --git a/http-push.c b/http-push.c
index 42f4d78..5cecef4 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1780,7 +1780,7 @@ static void one_remote_ref(char *refname)
 	struct ref *ref;
 	struct object *obj;
 
-	ref = alloc_ref_from_str(refname);
+	ref = alloc_ref(refname);
 
 	if (http_fetch_ref(remote->url, ref) != 0) {
 		fprintf(stderr,
@@ -1887,7 +1887,7 @@ static void add_remote_info_ref(struct remote_ls_ctx *ls)
 	char *ref_info;
 	struct ref *ref;
 
-	ref = alloc_ref_from_str(ls->dentry_name);
+	ref = alloc_ref(ls->dentry_name);
 
 	if (http_fetch_ref(remote->url, ref) != 0) {
 		fprintf(stderr,
diff --git a/remote.c b/remote.c
index 44d681d..e530a21 100644
--- a/remote.c
+++ b/remote.c
@@ -759,15 +759,9 @@ static struct ref *alloc_ref_with_prefix(const char *prefix, size_t prefixlen,
 	return ref;
 }
 
-struct ref *alloc_ref(unsigned namelen)
+struct ref *alloc_ref(const char *name)
 {
-	struct ref *ret = xcalloc(1, sizeof(struct ref) + namelen);
-	return ret;
-}
-
-struct ref *alloc_ref_from_str(const char* str)
-{
-	return alloc_ref_with_prefix("", 0, str);
+	return alloc_ref_with_prefix("", 0, name);
 }
 
 static struct ref *copy_ref(const struct ref *ref)
@@ -878,20 +872,20 @@ static struct ref *try_explicit_object_name(const char *name)
 	struct ref *ref;
 
 	if (!*name) {
-		ref = alloc_ref_from_str("(delete)");
+		ref = alloc_ref("(delete)");
 		hashclr(ref->new_sha1);
 		return ref;
 	}
 	if (get_sha1(name, sha1))
 		return NULL;
-	ref = alloc_ref_from_str(name);
+	ref = alloc_ref(name);
 	hashcpy(ref->new_sha1, sha1);
 	return ref;
 }
 
 static struct ref *make_linked_ref(const char *name, struct ref ***tail)
 {
-	struct ref *ret = alloc_ref_from_str(name);
+	struct ref *ret = alloc_ref(name);
 	tail_link_ref(ret, tail);
 	return ret;
 }
@@ -1196,9 +1190,8 @@ static struct ref *get_local_ref(const char *name)
 	if (!name)
 		return NULL;
 
-	if (!prefixcmp(name, "refs/")) {
-		return alloc_ref_from_str(name);
-	}
+	if (!prefixcmp(name, "refs/"))
+		return alloc_ref(name);
 
 	if (!prefixcmp(name, "heads/") ||
 	    !prefixcmp(name, "tags/") ||
diff --git a/remote.h b/remote.h
index c6163ff..d2e170c 100644
--- a/remote.h
+++ b/remote.h
@@ -55,9 +55,7 @@ struct refspec {
 
 extern const struct refspec *tag_refspec;
 
-struct ref *alloc_ref(unsigned namelen);
-
-struct ref *alloc_ref_from_str(const char* str);
+struct ref *alloc_ref(const char *name);
 
 struct ref *copy_ref_list(const struct ref *ref);
 
diff --git a/transport.c b/transport.c
index 3d03475..cfb7350 100644
--- a/transport.c
+++ b/transport.c
@@ -75,7 +75,7 @@ static int read_loose_refs(struct strbuf *path, int name_offset,
 
 			if (fd < 0)
 				continue;
-			next = alloc_ref_from_str(path->buf + name_offset);
+			next = alloc_ref(path->buf + name_offset);
 			if (read_in_full(fd, buffer, 40) != 40 ||
 					get_sha1_hex(buffer, next->old_sha1)) {
 				close(fd);
@@ -126,7 +126,7 @@ static void insert_packed_refs(const char *packed_refs, struct ref **list)
 				      (*list)->next->name)) > 0)
 			list = &(*list)->next;
 		if (!(*list)->next || cmp < 0) {
-			struct ref *next = alloc_ref_from_str(buffer + 41);
+			struct ref *next = alloc_ref(buffer + 41);
 			buffer[40] = '\0';
 			if (get_sha1_hex(buffer, next->old_sha1)) {
 				warning ("invalid SHA-1: %s", buffer);
@@ -499,7 +499,7 @@ static struct ref *get_refs_via_curl(struct transport *transport)
 
 	strbuf_release(&buffer);
 
-	ref = alloc_ref_from_str("HEAD");
+	ref = alloc_ref("HEAD");
 	if (!walker->fetch_ref(walker, ref) &&
 	    !resolve_remote_symref(ref, refs)) {
 		ref->next = refs;
@@ -540,7 +540,7 @@ static struct ref *get_refs_from_bundle(struct transport *transport)
 		die ("Could not read bundle '%s'.", transport->url);
 	for (i = 0; i < data->header.references.nr; i++) {
 		struct ref_list_entry *e = data->header.references.list + i;
-		struct ref *ref = alloc_ref_from_str(e->name);
+		struct ref *ref = alloc_ref(e->name);
 		hashcpy(ref->old_sha1, e->sha1);
 		ref->next = result;
 		result = ref;
diff --git a/walker.c b/walker.c
index 6b4cf70..679adab 100644
--- a/walker.c
+++ b/walker.c
@@ -191,7 +191,7 @@ static int interpret_target(struct walker *walker, char *target, unsigned char *
 	if (!get_sha1_hex(target, sha1))
 		return 0;
 	if (!check_ref_format(target)) {
-		struct ref *ref = alloc_ref_from_str(target);
+		struct ref *ref = alloc_ref(target);
 		if (!walker->fetch_ref(walker, ref)) {
 			hashcpy(sha1, ref->old_sha1);
 			free(ref);
-- 
1.6.0.2.542.gde55

Re: [PATCH 1/3] add alloc_ref_with_prefix()

[Bert Wesarg]

On Sat, Oct 18, 2008 at 10:37, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:
> In three cases in remote.c, a "raw" ref is allocated using alloc_ref()
> and then its is constructed using sprintf().  Clean it up by adding a
> helper function, alloc_ref_with_prefix(), which creates a composite
> name.  Use it in alloc_ref_from_str(), too, as it simplifies the code.
>
> Open code alloc_ref() in alloc_ref_with_prefix(), as the former is
> going to be removed in the patch after the next.
>
> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
> ---
>  remote.c |   32 ++++++++++++++++----------------
>  1 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/remote.c b/remote.c
> index 8a04066..98cbcf9 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -749,6 +749,16 @@ int remote_find_tracking(struct remote *remote, struct refspec *refspec)
>        return -1;
>  }
>
> +static struct ref *alloc_ref_with_prefix(const char *prefix, size_t prefixlen,
> +               const char *name)
> +{
> +       size_t len = strlen(name);
> +       struct ref *ref = xcalloc(1, sizeof(struct ref) + prefixlen + len + 1);
> +       memcpy(ref->name, prefix, prefixlen);
> +       memcpy(ref->name + prefixlen, name, len);
Where does you \0-terminate the string?

Regards
Bert

Re: [PATCH 1/3] add alloc_ref_with_prefix()

[René Scharfe]

>> +static struct ref *alloc_ref_with_prefix(const char *prefix, size_t prefixlen,
>> +               const char *name)
>> +{
>> +       size_t len = strlen(name);
>> +       struct ref *ref = xcalloc(1, sizeof(struct ref) + prefixlen + len + 1);
>> +       memcpy(ref->name, prefix, prefixlen);
>> +       memcpy(ref->name + prefixlen, name, len);
> Where does you \0-terminate the string?

xcalloc() calls calloc(), which zeroes the memory.

René

Re: [PATCH 1/3] add alloc_ref_with_prefix()

[Bert Wesarg]

On Sat, Oct 18, 2008 at 11:39, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:
>>> +static struct ref *alloc_ref_with_prefix(const char *prefix, size_t prefixlen,
>>> +               const char *name)
>>> +{
>>> +       size_t len = strlen(name);
>>> +       struct ref *ref = xcalloc(1, sizeof(struct ref) + prefixlen + len + 1);
>>> +       memcpy(ref->name, prefix, prefixlen);
>>> +       memcpy(ref->name + prefixlen, name, len);
>> Where does you \0-terminate the string?
>
> xcalloc() calls calloc(), which zeroes the memory.
So, you write the memory range twice, just for the last \0?

Bert
>
> René
>

Re: [PATCH 1/3] add alloc_ref_with_prefix()

[René Scharfe]

Bert Wesarg schrieb:
> On Sat, Oct 18, 2008 at 11:39, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:
>>>> +static struct ref *alloc_ref_with_prefix(const char *prefix, size_t prefixlen,
>>>> +               const char *name)
>>>> +{
>>>> +       size_t len = strlen(name);
>>>> +       struct ref *ref = xcalloc(1, sizeof(struct ref) + prefixlen + len + 1);
>>>> +       memcpy(ref->name, prefix, prefixlen);
>>>> +       memcpy(ref->name + prefixlen, name, len);
>>> Where does you \0-terminate the string?
>> xcalloc() calls calloc(), which zeroes the memory.
> So, you write the memory range twice, just for the last \0?

Well, one could exclude the name part from zeroing, that's true.  It's usually
safer to zero a whole block of memory right at allocation time, lest one
forgets, though.  I simply kept it they way it was done before.

That said, here's a patch (4/3):

diff --git a/remote.c b/remote.c
index e530a21..184115d 100644
--- a/remote.c
+++ b/remote.c
@@ -753,9 +753,10 @@ static struct ref *alloc_ref_with_prefix(const char *prefix, size_t prefixlen,
 		const char *name)
 {
 	size_t len = strlen(name);
-	struct ref *ref = xcalloc(1, sizeof(struct ref) + prefixlen + len + 1);
+	struct ref *ref = xmalloc(sizeof(struct ref) + prefixlen + len + 1);
+	memset(ref, sizeof(struct ref));
 	memcpy(ref->name, prefix, prefixlen);
-	memcpy(ref->name + prefixlen, name, len);
+	memcpy(ref->name + prefixlen, name, len + 1);
 	return ref;
 }
 

Re: [PATCH 1/3] add alloc_ref_with_prefix()

[Andreas Ericsson]

Bert Wesarg wrote:
> On Sat, Oct 18, 2008 at 11:39, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:
>>>> +static struct ref *alloc_ref_with_prefix(const char *prefix, size_t prefixlen,
>>>> +               const char *name)
>>>> +{
>>>> +       size_t len = strlen(name);
>>>> +       struct ref *ref = xcalloc(1, sizeof(struct ref) + prefixlen + len + 1);
>>>> +       memcpy(ref->name, prefix, prefixlen);
>>>> +       memcpy(ref->name + prefixlen, name, len);
>>> Where does you \0-terminate the string?
>> xcalloc() calls calloc(), which zeroes the memory.
> So, you write the memory range twice, just for the last \0?
> 

calloc() doesn't do the zeroing pass if it gets memory from the kernel,
which guarantees that the memory is already zeroed out. In reality,
calloc() usually saves on both readability and performance over
explicitly nul-terminating strings.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231