{"thread":{"id":"8230","subject":"[PATCH 3/3] Use stringbuf to clean up some string handling code.","startedAt":"2007-05-20T02:25:42Z","lastAt":"2007-05-20T11:19:17Z","messageCount":4,"participants":["Timo Sirainen","Alex Riesen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"42667","messageId":"1179627942.32181.1288.camel@hurina","threadId":"8230","inReplyTo":null,"subject":"[PATCH 3/3] Use stringbuf to clean up some string handling code.","fromName":"Timo Sirainen","fromEmail":"tss@iki.fi","sentAt":"2007-05-20T02:25:42Z","receivedAt":"2007-05-20T02:25:42Z","isPatch":true,"sender":{"key":"tss@iki.fi","avatar":null},"body":"---\n commit.c      |   30 +++++++++++++-----------------\n local-fetch.c |   34 ++++++++++++++++------------------\n 2 files changed, 29 insertions(+), 35 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex bee066f..58f1718 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -6,6 +6,7 @@\n #include \"interpolate.h\"\n #include \"diff.h\"\n #include \"revision.h\"\n+#include \"str.h\"\n \n int save_commit_buffer = 1;\n \n@@ -821,7 +822,7 @@ static long format_commit_message(const struct\ncommit *commit,\n \t\tILEFT_RIGHT,\n \t};\n \tstruct commit_list *p;\n-\tchar parents[1024];\n+\tstringbuf(parents, 1024);\n \tint i;\n \tenum { HEADER, SUBJECT, BODY } state;\n \n@@ -853,22 +854,17 @@ static long format_commit_message(const struct\ncommit *commit,\n \t\t\t ? \"<\"\n \t\t\t : \">\");\n \n-\tparents[1] = 0;\n-\tfor (i = 0, p = commit->parents;\n-\t\t\tp && i < sizeof(parents) - 1;\n-\t\t\tp = p->next)\n-\t\ti += snprintf(parents + i, sizeof(parents) - i - 1, \" %s\",\n-\t\t\tsha1_to_hex(p->item->object.sha1));\n-\tinterp_set_entry(table, IPARENTS, parents + 1);\n-\n-\tparents[1] = 0;\n-\tfor (i = 0, p = commit->parents;\n-\t\t\tp && i < sizeof(parents) - 1;\n-\t\t\tp = p->next)\n-\t\ti += snprintf(parents + i, sizeof(parents) - i - 1, \" %s\",\n-\t\t\tfind_unique_abbrev(p->item->object.sha1,\n-\t\t\t\tDEFAULT_ABBREV));\n-\tinterp_set_entry(table, IPARENTS_ABBREV, parents + 1);\n+\tstr_c(parents)[1] = 0;\n+\tfor (p = commit->parents; p; p = p->next)\n+\t\tstr_printfa(parents, \" %s\", sha1_to_hex(p->item->object.sha1));\n+\tinterp_set_entry(table, IPARENTS, str_c(parents) + 1);\n+\n+\tstr_c(parents)[1] = 0;\n+\tfor (p = commit->parents; p; p = p->next)\n+\t\tstr_printfa(parents, \" %s\",\n+\t\t\t    find_unique_abbrev(p->item->object.sha1,\n+\t\t\t\t\t       DEFAULT_ABBREV));\n+\tinterp_set_entry(table, IPARENTS_ABBREV, str_c(parents) + 1);\n \n \tfor (i = 0, state = HEADER; msg[i] && state < BODY; i++) {\n \t\tint eol;\ndiff --git a/local-fetch.c b/local-fetch.c\nindex 4b650ef..6d0599f 100644\n--- a/local-fetch.c\n+++ b/local-fetch.c\n@@ -4,6 +4,7 @@\n #include \"cache.h\"\n #include \"commit.h\"\n #include \"fetch.h\"\n+#include \"str.h\"\n \n static int use_link;\n static int use_symlink;\n@@ -21,12 +22,11 @@ static struct packed_git *packs;\n static void setup_index(unsigned char *sha1)\n {\n \tstruct packed_git *new_pack;\n-\tchar filename[PATH_MAX];\n-\tstrcpy(filename, path);\n-\tstrcat(filename, \"/objects/pack/pack-\");\n-\tstrcat(filename, sha1_to_hex(sha1));\n-\tstrcat(filename, \".idx\");\n-\tnew_pack = parse_pack_index_file(sha1, filename);\n+\tstringbuf(filename, PATH_MAX);\n+\n+\tstr_printfa(filename, \"%s/objects/pack/pack-%s.idx\",\n+\t\t    path, sha1_to_hex(sha1));\n+\tnew_pack = parse_pack_index_file(sha1, str_c(filename));\n \tnew_pack->next = packs;\n \tpacks = new_pack;\n }\n@@ -35,10 +35,11 @@ static int setup_indices(void)\n {\n \tDIR *dir;\n \tstruct dirent *de;\n-\tchar filename[PATH_MAX];\n+\tstringbuf(filename, PATH_MAX);\n \tunsigned char sha1[20];\n-\tsprintf(filename, \"%s/objects/pack/\", path);\n-\tdir = opendir(filename);\n+\n+\tstr_printfa(filename, \"%s/objects/pack/\", path);\n+\tdir = opendir(str_c(filename));\n \tif (!dir)\n \t\treturn -1;\n \twhile ((de = readdir(dir)) != NULL) {\n@@ -137,20 +138,17 @@ static int fetch_pack(const unsigned char *sha1)\n static int fetch_file(const unsigned char *sha1)\n {\n \tstatic int object_name_start = -1;\n-\tstatic char filename[PATH_MAX];\n+\tstatic stringbuf(filename, PATH_MAX);\n \tchar *hex = sha1_to_hex(sha1);\n \tchar *dest_filename = sha1_file_name(sha1);\n \n  \tif (object_name_start < 0) {\n-\t\tstrcpy(filename, path); /* e.g. git.git */\n-\t\tstrcat(filename, \"/objects/\");\n-\t\tobject_name_start = strlen(filename);\n+\t\tstr_printfa(filename, \"%s/objects/\", path); /* e.g. git.git */\n+\t\tobject_name_start = str_len(filename);\n \t}\n-\tfilename[object_name_start+0] = hex[0];\n-\tfilename[object_name_start+1] = hex[1];\n-\tfilename[object_name_start+2] = '/';\n-\tstrcpy(filename + object_name_start + 3, hex + 2);\n-\treturn copy_file(filename, dest_filename, hex, 0);\n+\tstr_truncate(filename, object_name_start);\n+\tstr_printfa(filename, \"%c%c/%s\", hex[0], hex[1], hex + 2);\n+\treturn copy_file(str_c(filename), dest_filename, hex, 0);\n }\n \n int fetch(unsigned char *sha1)\n-- \n1.5.1.4\n\n"},{"id":"42683","messageId":"20070520095623.GA3106@steel.home","threadId":"8230","inReplyTo":"1179627942.32181.1288.camel@hurina","subject":"Re: [PATCH 3/3] Use stringbuf to clean up some string handling code.","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-05-20T09:56:23Z","receivedAt":"2007-05-20T09:56:23Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Timo Sirainen, Sun, May 20, 2007 04:25:42 +0200:\n> ---\n>  commit.c      |   30 +++++++++++++-----------------\n>  local-fetch.c |   34 ++++++++++++++++------------------\n>  2 files changed, 29 insertions(+), 35 deletions(-)\n\nI find it hard to believe that it actually was a cleanup.\n\nIt is a nicer code, but... it is bigger, heavier on stack, and it does\nnot actually fix anything.\n\nIn my experience, such changes are seldom worth the effort. It may be\na nice code (and I actually like str.[hc]), but its use _must_ be\njustified. I.e. it must simplify a complex formatting routine, or fix\na bug, which otherwise would be too hard or ugly to fix. It is\ndefinitely not the case in this patch.\n"},{"id":"42687","messageId":"7v646nq08k.fsf@assigned-by-dhcp.cox.net","threadId":"8230","inReplyTo":"20070520095623.GA3106@steel.home","subject":"Re: [PATCH 3/3] Use stringbuf to clean up some string handling code.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-20T10:04:59Z","receivedAt":"2007-05-20T10:04:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen <raa.lkml@gmail.com> writes:\n\n> Timo Sirainen, Sun, May 20, 2007 04:25:42 +0200:\n>> ---\n>>  commit.c      |   30 +++++++++++++-----------------\n>>  local-fetch.c |   34 ++++++++++++++++------------------\n>>  2 files changed, 29 insertions(+), 35 deletions(-)\n>\n> I find it hard to believe that it actually was a cleanup.\n>\n> It is a nicer code, but... it is bigger, heavier on stack, and it does\n> not actually fix anything.\n>\n> In my experience, such changes are seldom worth the effort. It may be\n> a nice code (and I actually like str.[hc]), but its use _must_ be\n> justified. I.e. it must simplify a complex formatting routine, or fix\n> a bug, which otherwise would be too hard or ugly to fix. It is\n> definitely not the case in this patch.\n\nThanks.  I was kind of waiting for somebody to say that for me\n;-)\n"},{"id":"42700","messageId":"1179659957.32181.1312.camel@hurina","threadId":"8230","inReplyTo":"20070520095623.GA3106@steel.home","subject":"Re: [PATCH 3/3] Use stringbuf to clean up some string handling code.","fromName":"Timo Sirainen","fromEmail":"tss@iki.fi","sentAt":"2007-05-20T11:19:17Z","receivedAt":"2007-05-20T11:19:17Z","isPatch":true,"sender":{"key":"tss@iki.fi","avatar":null},"body":"On Sun, 2007-05-20 at 11:56 +0200, Alex Riesen wrote:\n> Timo Sirainen, Sun, May 20, 2007 04:25:42 +0200:\n> > ---\n> >  commit.c      |   30 +++++++++++++-----------------\n> >  local-fetch.c |   34 ++++++++++++++++------------------\n> >  2 files changed, 29 insertions(+), 35 deletions(-)\n> \n> I find it hard to believe that it actually was a cleanup.\n> \n> It is a nicer code, but... it is bigger, heavier on stack, and it does\n> not actually fix anything.\n> \n> In my experience, such changes are seldom worth the effort. It may be\n> a nice code (and I actually like str.[hc]), but its use _must_ be\n> justified. I.e. it must simplify a complex formatting routine, or fix\n> a bug, which otherwise would be too hard or ugly to fix. It is\n> definitely not the case in this patch.\n\nIn my own projects security is the highest priority and it justifies\npretty much all changes. I've done several large changes that change\nthousands of lines of code just because it makes it a bit easier to\nverify the code's safety/correctness.\n\nI realize that other projects may not want to use all of the tricks that\nI'm using in my C code (type safe dynamic arrays, type safe context\npointer in callback functions, etc.), but I was hoping that at least the\nlibc string handling functions would never be used in a large project\nanymore. Using them makes it extremely time consuming to verify the\ncode's safety, and at least I try to avoid software if I can't do that.\n\n"}]}