[PATCH 0/4] builtin-clone: use strbuf

[PATCH 0/4] builtin-clone: use strbuf

[Miklos Vajna]

Hi,

These patches replace hand-rolled sprintf/strcpy usage and such with
using the strbuf API.

The first patch is unrelated, I just noticed that minor issue during
creating the other patches.

Miklos Vajna (4):
  builtin-clone: fix a memory leak in cmd_clone()
  builtin-clone: use strbuf in guess_dir_name()
  builtin-clone: use strbuf in clone_local() and
    copy_or_link_directory()
  builtin_clone: use strbuf in cmd_clone()

 builtin-clone.c |  110 ++++++++++++++++++++++++++++++-------------------------
 1 files changed, 60 insertions(+), 50 deletions(-)

[PATCH 1/4] builtin-clone: fix a memory leak in cmd_clone()

[Miklos Vajna]

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-clone.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index 8e1a1d3..da21cab 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -516,6 +516,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		refs = transport_get_remote_refs(transport);
 		transport_fetch_refs(transport, refs);
 	}
+	free(dir);
 
 	clear_extra_refs();
 
-- 
1.6.0.4

[PATCH 2/4] builtin-clone: use strbuf in guess_dir_name()

[Miklos Vajna]

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-clone.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index da21cab..52b1242 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -134,9 +134,9 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 	}
 
 	if (is_bare) {
-		char *result = xmalloc(end - start + 5);
-		sprintf(result, "%.*s.git", (int)(end - start), start);
-		return result;
+		struct strbuf result = STRBUF_INIT;
+		strbuf_addf(&result, "%.*s.git", (int)(end - start), start);
+		return strbuf_detach(&result, 0);
 	}
 
 	return xstrndup(start, end - start);
-- 
1.6.0.4

[PATCH 3/4] builtin-clone: use strbuf in clone_local() and copy_or_link_directory()

[Miklos Vajna]

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-clone.c |   58 +++++++++++++++++++++++++++++-------------------------
 1 files changed, 31 insertions(+), 27 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index 52b1242..8182ebd 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -183,36 +183,38 @@ static void setup_reference(const char *repo)
 	free(ref_git_copy);
 }
 
-static void copy_or_link_directory(char *src, char *dest)
+static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest)
 {
 	struct dirent *de;
 	struct stat buf;
 	int src_len, dest_len;
 	DIR *dir;
 
-	dir = opendir(src);
+	dir = opendir(src->buf);
 	if (!dir)
-		die("failed to open %s\n", src);
+		die("failed to open %s\n", src->buf);
 
-	if (mkdir(dest, 0777)) {
+	if (mkdir(dest->buf, 0777)) {
 		if (errno != EEXIST)
-			die("failed to create directory %s\n", dest);
-		else if (stat(dest, &buf))
-			die("failed to stat %s\n", dest);
+			die("failed to create directory %s\n", dest->buf);
+		else if (stat(dest->buf, &buf))
+			die("failed to stat %s\n", dest->buf);
 		else if (!S_ISDIR(buf.st_mode))
-			die("%s exists and is not a directory\n", dest);
+			die("%s exists and is not a directory\n", dest->buf);
 	}
 
-	src_len = strlen(src);
-	src[src_len] = '/';
-	dest_len = strlen(dest);
-	dest[dest_len] = '/';
+	strbuf_addch(src, '/');
+	src_len = src->len;
+	strbuf_addch(dest, '/');
+	dest_len = dest->len;
 
 	while ((de = readdir(dir)) != NULL) {
-		strcpy(src + src_len + 1, de->d_name);
-		strcpy(dest + dest_len + 1, de->d_name);
-		if (stat(src, &buf)) {
-			warning ("failed to stat %s\n", src);
+		strbuf_setlen(src, src_len);
+		strbuf_addstr(src, de->d_name);
+		strbuf_setlen(dest, dest_len);
+		strbuf_addstr(dest, de->d_name);
+		if (stat(src->buf, &buf)) {
+			warning ("failed to stat %s\n", src->buf);
 			continue;
 		}
 		if (S_ISDIR(buf.st_mode)) {
@@ -221,17 +223,17 @@ static void copy_or_link_directory(char *src, char *dest)
 			continue;
 		}
 
-		if (unlink(dest) && errno != ENOENT)
-			die("failed to unlink %s\n", dest);
+		if (unlink(dest->buf) && errno != ENOENT)
+			die("failed to unlink %s\n", dest->buf);
 		if (!option_no_hardlinks) {
-			if (!link(src, dest))
+			if (!link(src->buf, dest->buf))
 				continue;
 			if (option_local)
-				die("failed to create link %s\n", dest);
+				die("failed to create link %s\n", dest->buf);
 			option_no_hardlinks = 1;
 		}
-		if (copy_file(dest, src, 0666))
-			die("failed to copy file to %s\n", dest);
+		if (copy_file(dest->buf, src->buf, 0666))
+			die("failed to copy file to %s\n", dest->buf);
 	}
 	closedir(dir);
 }
@@ -240,17 +242,19 @@ static const struct ref *clone_local(const char *src_repo,
 				     const char *dest_repo)
 {
 	const struct ref *ret;
-	char src[PATH_MAX];
-	char dest[PATH_MAX];
+	struct strbuf src = STRBUF_INIT;
+	struct strbuf dest = STRBUF_INIT;
 	struct remote *remote;
 	struct transport *transport;
 
 	if (option_shared)
 		add_to_alternates_file(src_repo);
 	else {
-		snprintf(src, PATH_MAX, "%s/objects", src_repo);
-		snprintf(dest, PATH_MAX, "%s/objects", dest_repo);
-		copy_or_link_directory(src, dest);
+		strbuf_addf(&src, "%s/objects", src_repo);
+		strbuf_addf(&dest, "%s/objects", dest_repo);
+		copy_or_link_directory(&src, &dest);
+		strbuf_release(&src);
+		strbuf_release(&dest);
 	}
 
 	remote = remote_get(src_repo);
-- 
1.6.0.4

[PATCH 4/4] builtin_clone: use strbuf in cmd_clone()

[Miklos Vajna]

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-clone.c |   45 +++++++++++++++++++++++++--------------------
 1 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index 8182ebd..a6ea3d6 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -358,8 +358,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	const char *repo_name, *repo, *work_tree, *git_dir;
 	char *path, *dir;
 	const struct ref *refs, *head_points_at, *remote_head, *mapped_refs;
-	char branch_top[256], key[256], value[256];
-	struct strbuf reflog_msg = STRBUF_INIT;
+	struct strbuf key = STRBUF_INIT, value = STRBUF_INIT;
+	struct strbuf branch_top = STRBUF_INIT, reflog_msg = STRBUF_INIT;
 	struct transport *transport = NULL;
 	char *src_ref_prefix = "refs/heads/";
 
@@ -463,35 +463,36 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (option_bare) {
 		if (option_mirror)
 			src_ref_prefix = "refs/";
-		strcpy(branch_top, src_ref_prefix);
+		strbuf_addstr(&branch_top, src_ref_prefix);
 
 		git_config_set("core.bare", "true");
 	} else {
-		snprintf(branch_top, sizeof(branch_top),
-			 "refs/remotes/%s/", option_origin);
+		strbuf_addf(&branch_top, "refs/remotes/%s/", option_origin);
 	}
 
 	if (option_mirror || !option_bare) {
 		/* Configure the remote */
 		if (option_mirror) {
-			snprintf(key, sizeof(key),
-					"remote.%s.mirror", option_origin);
-			git_config_set(key, "true");
+			strbuf_addf(&key, "remote.%s.mirror", option_origin);
+			git_config_set(key.buf, "true");
+			strbuf_reset(&key);
 		}
 
-		snprintf(key, sizeof(key), "remote.%s.url", option_origin);
-		git_config_set(key, repo);
+		strbuf_addf(&key, "remote.%s.url", option_origin);
+		git_config_set(key.buf, repo);
+			strbuf_reset(&key);
 
-		snprintf(key, sizeof(key), "remote.%s.fetch", option_origin);
-		snprintf(value, sizeof(value),
-				"+%s*:%s*", src_ref_prefix, branch_top);
-		git_config_set_multivar(key, value, "^$", 0);
+		strbuf_addf(&key, "remote.%s.fetch", option_origin);
+		strbuf_addf(&value, "+%s*:%s*", src_ref_prefix, branch_top.buf);
+		git_config_set_multivar(key.buf, value.buf, "^$", 0);
+		strbuf_reset(&key);
+		strbuf_reset(&value);
 	}
 
 	refspec.force = 0;
 	refspec.pattern = 1;
 	refspec.src = src_ref_prefix;
-	refspec.dst = branch_top;
+	refspec.dst = branch_top.buf;
 
 	if (path && !is_bundle)
 		refs = clone_local(path, git_dir);
@@ -546,7 +547,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 				   head_points_at->old_sha1,
 				   NULL, 0, DIE_ON_ERR);
 
-			strbuf_addstr(&head_ref, branch_top);
+			strbuf_addstr(&head_ref, branch_top.buf);
 			strbuf_addstr(&head_ref, "HEAD");
 
 			/* Remote branch link */
@@ -554,10 +555,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 				      head_points_at->peer_ref->name,
 				      reflog_msg.buf);
 
-			snprintf(key, sizeof(key), "branch.%s.remote", head);
-			git_config_set(key, option_origin);
-			snprintf(key, sizeof(key), "branch.%s.merge", head);
-			git_config_set(key, head_points_at->name);
+			strbuf_addf(&key, "branch.%s.remote", head);
+			git_config_set(key.buf, option_origin);
+			strbuf_reset(&key);
+			strbuf_addf(&key, "branch.%s.merge", head);
+			git_config_set(key.buf, head_points_at->name);
 		}
 	} else if (remote_head) {
 		/* Source had detached HEAD pointing somewhere. */
@@ -607,6 +609,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	}
 
 	strbuf_release(&reflog_msg);
+	strbuf_release(&branch_top);
+	strbuf_release(&key);
+	strbuf_release(&value);
 	junk_pid = 0;
 	return 0;
 }
-- 
1.6.0.4

Re: [PATCH 1/4] builtin-clone: fix a memory leak in cmd_clone()

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
> ---
>  builtin-clone.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/builtin-clone.c b/builtin-clone.c
> index 8e1a1d3..da21cab 100644
> --- a/builtin-clone.c
> +++ b/builtin-clone.c
> @@ -516,6 +516,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  		refs = transport_get_remote_refs(transport);
>  		transport_fetch_refs(transport, refs);
>  	}
> +	free(dir);
>  
>  	clear_extra_refs();

Can't this be done much earlier?  This variable can potentially be
assigned to work_tree, but after we set up the atexit handler neither dir
nor work_tree are not used (it is a bit hard to see as this function
itself is a bit too big to be maintainable).

Re: [PATCH 1/4] builtin-clone: fix a memory leak in cmd_clone()

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 672 bytes --]

On Sun, Nov 23, 2008 at 07:51:36PM -0800, Junio C Hamano <gitster@pobox.com> wrote:
> Can't this be done much earlier?  This variable can potentially be
> assigned to work_tree, but after we set up the atexit handler neither dir
> nor work_tree are not used (it is a bit hard to see as this function
> itself is a bit too big to be maintainable).

Hm, reading the code again I think you should just drop 1/4.

work_tree is a pointer to dir, and junk_work_tree is a pointer to
work_tree. junk_work_tree is used in the signal handler, so it has to be
allocated during the whole clone.

I guess it was just an accident that my patch passed the tests.

Sorry for the mistake.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]