{"thread":{"id":"16418","subject":"[PATCH 0/4] builtin-clone: use strbuf","startedAt":"2008-11-21T00:44:57Z","lastAt":"2008-11-25T22:40:05Z","messageCount":7,"participants":["Miklos Vajna","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"96298","messageId":"cover.1227227976.git.vmiklos@frugalware.org","threadId":"16418","inReplyTo":null,"subject":"[PATCH 0/4] builtin-clone: use strbuf","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-11-21T00:44:57Z","receivedAt":"2008-11-21T00:44:57Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"Hi,\n\nThese patches replace hand-rolled sprintf/strcpy usage and such with\nusing the strbuf API.\n\nThe first patch is unrelated, I just noticed that minor issue during\ncreating the other patches.\n\nMiklos Vajna (4):\n builtin-clone: fix a memory leak in cmd_clone()\n builtin-clone: use strbuf in guess_dir_name()\n builtin-clone: use strbuf in clone_local() and\n copy_or_link_directory()\n builtin_clone: use strbuf in cmd_clone()\n\n builtin-clone.c | 110 ++++++++++++++++++++++++++++++-------------------------\n 1 files changed, 60 insertions(+), 50 deletions(-)\n"},{"id":"96300","messageId":"6bd31bceb3840f14d747972b4858e1c5b215744d.1227227976.git.vmiklos@frugalware.org","threadId":"16418","inReplyTo":"cover.1227227976.git.vmiklos@frugalware.org","subject":"[PATCH 1/4] builtin-clone: fix a memory leak in cmd_clone()","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-11-21T00:44:58Z","receivedAt":"2008-11-21T00:44:58Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"Signed-off-by: Miklos Vajna \n---\n builtin-clone.c | 1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-clone.c b/builtin-clone.c\nindex 8e1a1d3..da21cab 100644\n--- a/builtin-clone.c\n+++ b/builtin-clone.c\n@@ -516,6 +516,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\trefs = transport_get_remote_refs(transport);\n \t\ttransport_fetch_refs(transport, refs);\n \t}\n+\tfree(dir);\n \n \tclear_extra_refs();\n \n-- \n1.6.0.4\n"},{"id":"96299","messageId":"33c4500c44cb27b1bfb3ec5584c0665b0dbd7c73.1227227976.git.vmiklos@frugalware.org","threadId":"16418","inReplyTo":"6bd31bceb3840f14d747972b4858e1c5b215744d.1227227976.git.vmiklos@frugalware.org","subject":"[PATCH 2/4] builtin-clone: use strbuf in guess_dir_name()","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-11-21T00:44:59Z","receivedAt":"2008-11-21T00:44:59Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"Signed-off-by: Miklos Vajna \n---\n builtin-clone.c | 6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-clone.c b/builtin-clone.c\nindex da21cab..52b1242 100644\n--- a/builtin-clone.c\n+++ b/builtin-clone.c\n@@ -134,9 +134,9 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n \t}\n \n \tif (is_bare) {\n-\t\tchar *result = xmalloc(end - start + 5);\n-\t\tsprintf(result, \"%.*s.git\", (int)(end - start), start);\n-\t\treturn result;\n+\t\tstruct strbuf result = STRBUF_INIT;\n+\t\tstrbuf_addf(&result, \"%.*s.git\", (int)(end - start), start);\n+\t\treturn strbuf_detach(&result, 0);\n \t}\n \n \treturn xstrndup(start, end - start);\n-- \n1.6.0.4\n"},{"id":"96302","messageId":"59af178ebb4da602253e95f46f54473f42177e09.1227227976.git.vmiklos@frugalware.org","threadId":"16418","inReplyTo":"33c4500c44cb27b1bfb3ec5584c0665b0dbd7c73.1227227976.git.vmiklos@frugalware.org","subject":"[PATCH 3/4] builtin-clone: use strbuf in clone_local() and copy_or_link_directory()","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-11-21T00:45:00Z","receivedAt":"2008-11-21T00:45:00Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"Signed-off-by: Miklos Vajna \n---\n builtin-clone.c | 58 +++++++++++++++++++++++++++++-------------------------\n 1 files changed, 31 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin-clone.c b/builtin-clone.c\nindex 52b1242..8182ebd 100644\n--- a/builtin-clone.c\n+++ b/builtin-clone.c\n@@ -183,36 +183,38 @@ static void setup_reference(const char *repo)\n \tfree(ref_git_copy);\n }\n \n-static void copy_or_link_directory(char *src, char *dest)\n+static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest)\n {\n \tstruct dirent *de;\n \tstruct stat buf;\n \tint src_len, dest_len;\n \tDIR *dir;\n \n-\tdir = opendir(src);\n+\tdir = opendir(src->buf);\n \tif (!dir)\n-\t\tdie(\"failed to open %s\\n\", src);\n+\t\tdie(\"failed to open %s\\n\", src->buf);\n \n-\tif (mkdir(dest, 0777)) {\n+\tif (mkdir(dest->buf, 0777)) {\n \t\tif (errno != EEXIST)\n-\t\t\tdie(\"failed to create directory %s\\n\", dest);\n-\t\telse if (stat(dest, &buf))\n-\t\t\tdie(\"failed to stat %s\\n\", dest);\n+\t\t\tdie(\"failed to create directory %s\\n\", dest->buf);\n+\t\telse if (stat(dest->buf, &buf))\n+\t\t\tdie(\"failed to stat %s\\n\", dest->buf);\n \t\telse if (!S_ISDIR(buf.st_mode))\n-\t\t\tdie(\"%s exists and is not a directory\\n\", dest);\n+\t\t\tdie(\"%s exists and is not a directory\\n\", dest->buf);\n \t}\n \n-\tsrc_len = strlen(src);\n-\tsrc[src_len] = '/';\n-\tdest_len = strlen(dest);\n-\tdest[dest_len] = '/';\n+\tstrbuf_addch(src, '/');\n+\tsrc_len = src->len;\n+\tstrbuf_addch(dest, '/');\n+\tdest_len = dest->len;\n \n \twhile ((de = readdir(dir)) != NULL) {\n-\t\tstrcpy(src + src_len + 1, de->d_name);\n-\t\tstrcpy(dest + dest_len + 1, de->d_name);\n-\t\tif (stat(src, &buf)) {\n-\t\t\twarning (\"failed to stat %s\\n\", src);\n+\t\tstrbuf_setlen(src, src_len);\n+\t\tstrbuf_addstr(src, de->d_name);\n+\t\tstrbuf_setlen(dest, dest_len);\n+\t\tstrbuf_addstr(dest, de->d_name);\n+\t\tif (stat(src->buf, &buf)) {\n+\t\t\twarning (\"failed to stat %s\\n\", src->buf);\n \t\t\tcontinue;\n \t\t}\n \t\tif (S_ISDIR(buf.st_mode)) {\n@@ -221,17 +223,17 @@ static void copy_or_link_directory(char *src, char *dest)\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (unlink(dest) && errno != ENOENT)\n-\t\t\tdie(\"failed to unlink %s\\n\", dest);\n+\t\tif (unlink(dest->buf) && errno != ENOENT)\n+\t\t\tdie(\"failed to unlink %s\\n\", dest->buf);\n \t\tif (!option_no_hardlinks) {\n-\t\t\tif (!link(src, dest))\n+\t\t\tif (!link(src->buf, dest->buf))\n \t\t\t\tcontinue;\n \t\t\tif (option_local)\n-\t\t\t\tdie(\"failed to create link %s\\n\", dest);\n+\t\t\t\tdie(\"failed to create link %s\\n\", dest->buf);\n \t\t\toption_no_hardlinks = 1;\n \t\t}\n-\t\tif (copy_file(dest, src, 0666))\n-\t\t\tdie(\"failed to copy file to %s\\n\", dest);\n+\t\tif (copy_file(dest->buf, src->buf, 0666))\n+\t\t\tdie(\"failed to copy file to %s\\n\", dest->buf);\n \t}\n \tclosedir(dir);\n }\n@@ -240,17 +242,19 @@ static const struct ref *clone_local(const char *src_repo,\n \t\t\t\t const char *dest_repo)\n {\n \tconst struct ref *ret;\n-\tchar src[PATH_MAX];\n-\tchar dest[PATH_MAX];\n+\tstruct strbuf src = STRBUF_INIT;\n+\tstruct strbuf dest = STRBUF_INIT;\n \tstruct remote *remote;\n \tstruct transport *transport;\n \n \tif (option_shared)\n \t\tadd_to_alternates_file(src_repo);\n \telse {\n-\t\tsnprintf(src, PATH_MAX, \"%s/objects\", src_repo);\n-\t\tsnprintf(dest, PATH_MAX, \"%s/objects\", dest_repo);\n-\t\tcopy_or_link_directory(src, dest);\n+\t\tstrbuf_addf(&src, \"%s/objects\", src_repo);\n+\t\tstrbuf_addf(&dest, \"%s/objects\", dest_repo);\n+\t\tcopy_or_link_directory(&src, &dest);\n+\t\tstrbuf_release(&src);\n+\t\tstrbuf_release(&dest);\n \t}\n \n \tremote = remote_get(src_repo);\n-- \n1.6.0.4\n"},{"id":"96301","messageId":"f38db18d6741dfd116752ff4797479833fd00bc3.1227227976.git.vmiklos@frugalware.org","threadId":"16418","inReplyTo":"59af178ebb4da602253e95f46f54473f42177e09.1227227976.git.vmiklos@frugalware.org","subject":"[PATCH 4/4] builtin_clone: use strbuf in cmd_clone()","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-11-21T00:45:01Z","receivedAt":"2008-11-21T00:45:01Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"Signed-off-by: Miklos Vajna \n---\n builtin-clone.c | 45 +++++++++++++++++++++++++--------------------\n 1 files changed, 25 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin-clone.c b/builtin-clone.c\nindex 8182ebd..a6ea3d6 100644\n--- a/builtin-clone.c\n+++ b/builtin-clone.c\n@@ -358,8 +358,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tconst char *repo_name, *repo, *work_tree, *git_dir;\n \tchar *path, *dir;\n \tconst struct ref *refs, *head_points_at, *remote_head, *mapped_refs;\n-\tchar branch_top[256], key[256], value[256];\n-\tstruct strbuf reflog_msg = STRBUF_INIT;\n+\tstruct strbuf key = STRBUF_INIT, value = STRBUF_INIT;\n+\tstruct strbuf branch_top = STRBUF_INIT, reflog_msg = STRBUF_INIT;\n \tstruct transport *transport = NULL;\n \tchar *src_ref_prefix = \"refs/heads/\";\n \n@@ -463,35 +463,36 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (option_bare) {\n \t\tif (option_mirror)\n \t\t\tsrc_ref_prefix = \"refs/\";\n-\t\tstrcpy(branch_top, src_ref_prefix);\n+\t\tstrbuf_addstr(&branch_top, src_ref_prefix);\n \n \t\tgit_config_set(\"core.bare\", \"true\");\n \t} else {\n-\t\tsnprintf(branch_top, sizeof(branch_top),\n-\t\t\t \"refs/remotes/%s/\", option_origin);\n+\t\tstrbuf_addf(&branch_top, \"refs/remotes/%s/\", option_origin);\n \t}\n \n \tif (option_mirror || !option_bare) {\n \t\t/* Configure the remote */\n \t\tif (option_mirror) {\n-\t\t\tsnprintf(key, sizeof(key),\n-\t\t\t\t\t\"remote.%s.mirror\", option_origin);\n-\t\t\tgit_config_set(key, \"true\");\n+\t\t\tstrbuf_addf(&key, \"remote.%s.mirror\", option_origin);\n+\t\t\tgit_config_set(key.buf, \"true\");\n+\t\t\tstrbuf_reset(&key);\n \t\t}\n \n-\t\tsnprintf(key, sizeof(key), \"remote.%s.url\", option_origin);\n-\t\tgit_config_set(key, repo);\n+\t\tstrbuf_addf(&key, \"remote.%s.url\", option_origin);\n+\t\tgit_config_set(key.buf, repo);\n+\t\t\tstrbuf_reset(&key);\n \n-\t\tsnprintf(key, sizeof(key), \"remote.%s.fetch\", option_origin);\n-\t\tsnprintf(value, sizeof(value),\n-\t\t\t\t\"+%s*:%s*\", src_ref_prefix, branch_top);\n-\t\tgit_config_set_multivar(key, value, \"^$\", 0);\n+\t\tstrbuf_addf(&key, \"remote.%s.fetch\", option_origin);\n+\t\tstrbuf_addf(&value, \"+%s*:%s*\", src_ref_prefix, branch_top.buf);\n+\t\tgit_config_set_multivar(key.buf, value.buf, \"^$\", 0);\n+\t\tstrbuf_reset(&key);\n+\t\tstrbuf_reset(&value);\n \t}\n \n \trefspec.force = 0;\n \trefspec.pattern = 1;\n \trefspec.src = src_ref_prefix;\n-\trefspec.dst = branch_top;\n+\trefspec.dst = branch_top.buf;\n \n \tif (path && !is_bundle)\n \t\trefs = clone_local(path, git_dir);\n@@ -546,7 +547,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\t\t head_points_at->old_sha1,\n \t\t\t\t NULL, 0, DIE_ON_ERR);\n \n-\t\t\tstrbuf_addstr(&head_ref, branch_top);\n+\t\t\tstrbuf_addstr(&head_ref, branch_top.buf);\n \t\t\tstrbuf_addstr(&head_ref, \"HEAD\");\n \n \t\t\t/* Remote branch link */\n@@ -554,10 +555,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\t\t head_points_at->peer_ref->name,\n \t\t\t\t reflog_msg.buf);\n \n-\t\t\tsnprintf(key, sizeof(key), \"branch.%s.remote\", head);\n-\t\t\tgit_config_set(key, option_origin);\n-\t\t\tsnprintf(key, sizeof(key), \"branch.%s.merge\", head);\n-\t\t\tgit_config_set(key, head_points_at->name);\n+\t\t\tstrbuf_addf(&key, \"branch.%s.remote\", head);\n+\t\t\tgit_config_set(key.buf, option_origin);\n+\t\t\tstrbuf_reset(&key);\n+\t\t\tstrbuf_addf(&key, \"branch.%s.merge\", head);\n+\t\t\tgit_config_set(key.buf, head_points_at->name);\n \t\t}\n \t} else if (remote_head) {\n \t\t/* Source had detached HEAD pointing somewhere. */\n@@ -607,6 +609,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t}\n \n \tstrbuf_release(&reflog_msg);\n+\tstrbuf_release(&branch_top);\n+\tstrbuf_release(&key);\n+\tstrbuf_release(&value);\n \tjunk_pid = 0;\n \treturn 0;\n }\n-- \n1.6.0.4\n"},{"id":"96399","messageId":"7vabbpok6v.fsf@gitster.siamese.dyndns.org","threadId":"16418","inReplyTo":"6bd31bceb3840f14d747972b4858e1c5b215744d.1227227976.git.vmiklos@frugalware.org","subject":"Re: [PATCH 1/4] builtin-clone: fix a memory leak in cmd_clone()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-24T03:51:36Z","receivedAt":"2008-11-24T03:51:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miklos Vajna writes:\n\n> Signed-off-by: Miklos Vajna \n> ---\n> builtin-clone.c | 1 +\n> 1 files changed, 1 insertions(+), 0 deletions(-)\n>\n> diff --git a/builtin-clone.c b/builtin-clone.c\n> index 8e1a1d3..da21cab 100644\n> --- a/builtin-clone.c\n> +++ b/builtin-clone.c\n> @@ -516,6 +516,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \t\trefs = transport_get_remote_refs(transport);\n> \t\ttransport_fetch_refs(transport, refs);\n> \t}\n> +\tfree(dir);\n> \n> \tclear_extra_refs();\n\nCan't this be done much earlier? This variable can potentially be\nassigned to work_tree, but after we set up the atexit handler neither dir\nnor work_tree are not used (it is a bit hard to see as this function\nitself is a bit too big to be maintainable).\n"},{"id":"96501","messageId":"20081125224005.GG4746@genesis.frugalware.org","threadId":"16418","inReplyTo":"7vabbpok6v.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/4] builtin-clone: fix a memory leak in cmd_clone()","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-11-25T22:40:05Z","receivedAt":"2008-11-25T22:40:05Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Sun, Nov 23, 2008 at 07:51:36PM -0800, Junio C Hamano wrote:\n> Can't this be done much earlier? This variable can potentially be\n> assigned to work_tree, but after we set up the atexit handler neither dir\n> nor work_tree are not used (it is a bit hard to see as this function\n> itself is a bit too big to be maintainable).\n\nHm, reading the code again I think you should just drop 1/4.\n\nwork_tree is a pointer to dir, and junk_work_tree is a pointer to\nwork_tree. junk_work_tree is used in the signal handler, so it has to be\nallocated during the whole clone.\n\nI guess it was just an accident that my patch passed the tests.\n\nSorry for the mistake.\n"}]}