{"thread":{"id":"16369","subject":"[PATCH 1/3] builtin-branch: use strbuf in delete_branches()","startedAt":"2008-11-17T20:48:34Z","lastAt":"2008-11-19T19:48:48Z","messageCount":7,"participants":["Miklos Vajna","Junio C Hamano","Johannes Schindelin","Lars Hjemli"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"96054","messageId":"cover.1226954771.git.vmiklos@frugalware.org","threadId":"16369","inReplyTo":null,"subject":"[PATCH 0/3] builtin-branch: use strbuf","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-11-17T20:48:34Z","receivedAt":"2008-11-17T20:48:34Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"These 3 patches do what's in the subject: use the strbuf API instead of\nsnprintf() / sprintf().\n\nThe current risk of writing unallocated memory is low, I think, but\nstill, the return value of these functions were not checked at several\nplaces.\n\nMiklos Vajna (3):\n builtin-branch: use strbuf in delete_branches()\n builtin-branch: use strbuf in fill_tracking_info()\n builtin-branch: use strbuf in rename_branch()\n\n builtin-branch.c | 65 +++++++++++++++++++++++++++--------------------------\n 1 files changed, 33 insertions(+), 32 deletions(-)\n"},{"id":"96053","messageId":"b3716161b2a3d508ec4306623c5c8889d9367e1d.1226954771.git.vmiklos@frugalware.org","threadId":"16369","inReplyTo":"cover.1226954771.git.vmiklos@frugalware.org","subject":"[PATCH 1/3] builtin-branch: use strbuf in delete_branches()","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-11-17T20:48:35Z","receivedAt":"2008-11-17T20:48:35Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"In case the length of branch name is greather then PATH_MAX-7, we write\nto unallocated memory otherwise.\n\nSigned-off-by: Miklos Vajna \n---\n builtin-branch.c | 8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-branch.c b/builtin-branch.c\nindex 2b3613f..b9149b7 100644\n--- a/builtin-branch.c\n+++ b/builtin-branch.c\n@@ -97,7 +97,6 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)\n \tunsigned char sha1[20];\n \tchar *name = NULL;\n \tconst char *fmt, *remote;\n-\tchar section[PATH_MAX];\n \tint i;\n \tint ret = 0;\n \n@@ -165,11 +164,12 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)\n \t\t\t argv[i]);\n \t\t\tret = 1;\n \t\t} else {\n+\t\t\tstruct strbuf buf = STRBUF_INIT;\n \t\t\tprintf(\"Deleted %sbranch %s.\\n\", remote, argv[i]);\n-\t\t\tsnprintf(section, sizeof(section), \"branch.%s\",\n-\t\t\t\t argv[i]);\n-\t\t\tif (git_config_rename_section(section, NULL) < 0)\n+\t\t\tstrbuf_addf(&buf, \"branch.%s\", argv[i]);\n+\t\t\tif (git_config_rename_section(buf.buf, NULL) < 0)\n \t\t\t\twarning(\"Update of config-file failed\");\n+\t\t\tstrbuf_release(&buf);\n \t\t}\n \t}\n \n-- \n1.6.0.2\n"},{"id":"96055","messageId":"5dee4e908190e7a4ed4a693fdf0fb3b3af88fc57.1226954771.git.vmiklos@frugalware.org","threadId":"16369","inReplyTo":"b3716161b2a3d508ec4306623c5c8889d9367e1d.1226954771.git.vmiklos@frugalware.org","subject":"[PATCH 2/3] builtin-branch: use strbuf in fill_tracking_info()","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-11-17T20:48:36Z","receivedAt":"2008-11-17T20:48:36Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"This is just about using the API, though in case of ~ 10^100 commits,\nthis would fix the problem of writing to unallocated memory as well. ;-)\n\nSigned-off-by: Miklos Vajna \n---\n builtin-branch.c | 18 ++++++++----------\n 1 files changed, 8 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin-branch.c b/builtin-branch.c\nindex b9149b7..c8a8e2a 100644\n--- a/builtin-branch.c\n+++ b/builtin-branch.c\n@@ -279,7 +279,7 @@ static int ref_cmp(const void *r1, const void *r2)\n \treturn strcmp(c1->name, c2->name);\n }\n \n-static void fill_tracking_info(char *stat, const char *branch_name)\n+static void fill_tracking_info(struct strbuf *stat, const char *branch_name)\n {\n \tint ours, theirs;\n \tstruct branch *branch = branch_get(branch_name);\n@@ -287,11 +287,11 @@ static void fill_tracking_info(char *stat, const char *branch_name)\n \tif (!stat_tracking_info(branch, &ours, &theirs) || (!ours && !theirs))\n \t\treturn;\n \tif (!ours)\n-\t\tsprintf(stat, \"[behind %d] \", theirs);\n+\t\tstrbuf_addf(stat, \"[behind %d] \", theirs);\n \telse if (!theirs)\n-\t\tsprintf(stat, \"[ahead %d] \", ours);\n+\t\tstrbuf_addf(stat, \"[ahead %d] \", ours);\n \telse\n-\t\tsprintf(stat, \"[ahead %d, behind %d] \", ours, theirs);\n+\t\tstrbuf_addf(stat, \"[ahead %d, behind %d] \", ours, theirs);\n }\n \n static int matches_merge_filter(struct commit *commit)\n@@ -334,11 +334,8 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n \t}\n \n \tif (verbose) {\n-\t\tstruct strbuf subject = STRBUF_INIT;\n+\t\tstruct strbuf subject = STRBUF_INIT, stat = STRBUF_INIT;\n \t\tconst char *sub = \" **** invalid ref ****\";\n-\t\tchar stat[128];\n-\n-\t\tstat[0] = '\\0';\n \n \t\tcommit = item->commit;\n \t\tif (commit && !parse_commit(commit)) {\n@@ -348,13 +345,14 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n \t\t}\n \n \t\tif (item->kind == REF_LOCAL_BRANCH)\n-\t\t\tfill_tracking_info(stat, item->name);\n+\t\t\tfill_tracking_info(&stat, item->name);\n \n \t\tprintf(\"%c %s%-*s%s %s %s%s\\n\", c, branch_get_color(color),\n \t\t maxwidth, item->name,\n \t\t branch_get_color(COLOR_BRANCH_RESET),\n \t\t find_unique_abbrev(item->commit->object.sha1, abbrev),\n-\t\t stat, sub);\n+\t\t stat.buf, sub);\n+\t\tstrbuf_release(&stat);\n \t\tstrbuf_release(&subject);\n \t} else {\n \t\tprintf(\"%c %s%s%s\\n\", c, branch_get_color(color), item->name,\n-- \n1.6.0.2\n"},{"id":"96056","messageId":"225d83c1ead50340eed97c64fbb8995017bf1ca8.1226954771.git.vmiklos@frugalware.org","threadId":"16369","inReplyTo":"5dee4e908190e7a4ed4a693fdf0fb3b3af88fc57.1226954771.git.vmiklos@frugalware.org","subject":"[PATCH 3/3] builtin-branch: use strbuf in rename_branch()","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-11-17T20:48:37Z","receivedAt":"2008-11-17T20:48:37Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"In case the length of branch name is greather then PATH_MAX-11, we write\nto unallocated memory otherwise.\n\nSigned-off-by: Miklos Vajna \n---\n builtin-branch.c | 39 +++++++++++++++++++++------------------\n 1 files changed, 21 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin-branch.c b/builtin-branch.c\nindex c8a8e2a..494cbac 100644\n--- a/builtin-branch.c\n+++ b/builtin-branch.c\n@@ -424,42 +424,45 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev, str\n \n static void rename_branch(const char *oldname, const char *newname, int force)\n {\n-\tchar oldref[PATH_MAX], newref[PATH_MAX], logmsg[PATH_MAX*2 + 100];\n+\tstruct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = STRBUF_INIT;\n \tunsigned char sha1[20];\n-\tchar oldsection[PATH_MAX], newsection[PATH_MAX];\n+\tstruct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;\n \n \tif (!oldname)\n \t\tdie(\"cannot rename the current branch while not on any.\");\n \n-\tif (snprintf(oldref, sizeof(oldref), \"refs/heads/%s\", oldname) > sizeof(oldref))\n-\t\tdie(\"Old branchname too long\");\n+\tstrbuf_addf(&oldref, \"refs/heads/%s\", oldname);\n \n-\tif (check_ref_format(oldref))\n-\t\tdie(\"Invalid branch name: %s\", oldref);\n+\tif (check_ref_format(oldref.buf))\n+\t\tdie(\"Invalid branch name: %s\", oldref.buf);\n \n-\tif (snprintf(newref, sizeof(newref), \"refs/heads/%s\", newname) > sizeof(newref))\n-\t\tdie(\"New branchname too long\");\n+\tstrbuf_addf(&newref, \"refs/heads/%s\", newname);\n \n-\tif (check_ref_format(newref))\n-\t\tdie(\"Invalid branch name: %s\", newref);\n+\tif (check_ref_format(newref.buf))\n+\t\tdie(\"Invalid branch name: %s\", newref.buf);\n \n-\tif (resolve_ref(newref, sha1, 1, NULL) && !force)\n+\tif (resolve_ref(newref.buf, sha1, 1, NULL) && !force)\n \t\tdie(\"A branch named '%s' already exists.\", newname);\n \n-\tsnprintf(logmsg, sizeof(logmsg), \"Branch: renamed %s to %s\",\n-\t\t oldref, newref);\n+\tstrbuf_addf(&logmsg, \"Branch: renamed %s to %s\",\n+\t\t oldref.buf, newref.buf);\n \n-\tif (rename_ref(oldref, newref, logmsg))\n+\tif (rename_ref(oldref.buf, newref.buf, logmsg.buf))\n \t\tdie(\"Branch rename failed\");\n+\tstrbuf_release(&logmsg);\n \n \t/* no need to pass logmsg here as HEAD didn't really move */\n-\tif (!strcmp(oldname, head) && create_symref(\"HEAD\", newref, NULL))\n+\tif (!strcmp(oldname, head) && create_symref(\"HEAD\", newref.buf, NULL))\n \t\tdie(\"Branch renamed to %s, but HEAD is not updated!\", newname);\n \n-\tsnprintf(oldsection, sizeof(oldsection), \"branch.%s\", oldref + 11);\n-\tsnprintf(newsection, sizeof(newsection), \"branch.%s\", newref + 11);\n-\tif (git_config_rename_section(oldsection, newsection) < 0)\n+\tstrbuf_addf(&oldsection, \"branch.%s\", oldref.buf + 11);\n+\tstrbuf_release(&oldref);\n+\tstrbuf_addf(&newsection, \"branch.%s\", newref.buf + 11);\n+\tstrbuf_release(&newref);\n+\tif (git_config_rename_section(oldsection.buf, newsection.buf) < 0)\n \t\tdie(\"Branch is renamed, but update of config-file failed\");\n+\tstrbuf_release(&oldsection);\n+\tstrbuf_release(&newsection);\n }\n \n static int opt_parse_with_commit(const struct option *opt, const char *arg, int unset)\n-- \n1.6.0.2\n"},{"id":"96106","messageId":"7vbpwcvb8i.fsf@gitster.siamese.dyndns.org","threadId":"16369","inReplyTo":"225d83c1ead50340eed97c64fbb8995017bf1ca8.1226954771.git.vmiklos@frugalware.org","subject":"Re: [PATCH 3/3] builtin-branch: use strbuf in rename_branch()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-18T23:57:17Z","receivedAt":"2008-11-18T23:57:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miklos Vajna writes:\n\n> In case the length of branch name is greather then PATH_MAX-11, we write\n> to unallocated memory otherwise.\n\nTrue for {old,new}section.\n\nI'll apply three patches from you as-is. Thanks.\n\nHaving said that,\n\n> -\tsnprintf(logmsg, sizeof(logmsg), \"Branch: renamed %s to %s\",\n> -\t\t oldref, newref);\n> +\tstrbuf_addf(&logmsg, \"Branch: renamed %s to %s\",\n> +\t\t oldref.buf, newref.buf);\n\nI am wondering why nobody has complained until now, but shouldn't this be\noldname and newname? \n\n Reflog message: Branch: renamed refs/heads/master to refs/heads/naster\n\ndoes not feel right, even though it is perfectly understandable to people\nwho know the internal (i.e. branches are implemented as a ref in\nrefs/heads hierarchy).\n\nRewording of the above, if it is ever done, has to be a separate commit,\nand it is a behaviour change (if some third-party tool is reading and\nparsing the reflog we will break it) which I do not particularly think is\nworth doing.\n\nI am mentioning this only because I just noticed it (and do not want to do\nthe thinking myself ;-).\n"},{"id":"96109","messageId":"alpine.DEB.1.00.0811190209470.30769@pacific.mpi-cbg.de","threadId":"16369","inReplyTo":"7vbpwcvb8i.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 3/3] builtin-branch: use strbuf in rename_branch()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-11-19T01:11:01Z","receivedAt":"2008-11-19T01:11:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 18 Nov 2008, Junio C Hamano wrote:\n\n> Miklos Vajna writes:\n> \n> > -\tsnprintf(logmsg, sizeof(logmsg), \"Branch: renamed %s to %s\",\n> > -\t\t oldref, newref);\n> > +\tstrbuf_addf(&logmsg, \"Branch: renamed %s to %s\",\n> > +\t\t oldref.buf, newref.buf);\n> \n> I am wondering why nobody has complained until now, but shouldn't this \n> be oldname and newname?\n\nI think that was the intention. Lars?\n\nCiao,\nDscho\n"},{"id":"96184","messageId":"8c5c35580811191148m45cdff68vbe190ef911066da8@mail.gmail.com","threadId":"16369","inReplyTo":"alpine.DEB.1.00.0811190209470.30769@pacific.mpi-cbg.de","subject":"Re: [PATCH 3/3] builtin-branch: use strbuf in rename_branch()","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2008-11-19T19:48:48Z","receivedAt":"2008-11-19T19:48:48Z","isPatch":true,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"On Wed, Nov 19, 2008 at 02:11, Johannes Schindelin\n wrote:\n> Hi,\n>\n> On Tue, 18 Nov 2008, Junio C Hamano wrote:\n>\n>> Miklos Vajna writes:\n>>\n>> > - snprintf(logmsg, sizeof(logmsg), \"Branch: renamed %s to %s\",\n>> > - oldref, newref);\n>> > + strbuf_addf(&logmsg, \"Branch: renamed %s to %s\",\n>> > + oldref.buf, newref.buf);\n>>\n>> I am wondering why nobody has complained until now, but shouldn't this\n>> be oldname and newname?\n>\n> I think that was the intention. Lars?\n\nSome background: the message was first generated internally (in\nc976d415) by refs.c:rename_ref() and thus it made sense to use the\nfull refname. Sometime later (in 678d0f4c), rename_ref() was modified\nto get the message as an argument from\nbuiltin_branch.c:rename_branch() but the format of the message was\nkept (almost) identical.\n\nPersonally, I think it's nice if the reflog contains the full refname.\n\n--\nlarsh\n"}]}