{"thread":{"id":"36955","subject":"[PATCH 0/2] dropping manual malloc calculations","startedAt":"2014-06-18T20:00:01Z","lastAt":"2014-06-25T19:54:38Z","messageCount":27,"participants":["Jeff King","Junio C Hamano","René Scharfe","Eric Sunshine","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"244579","messageId":"20140618200000.GA22994@sigill.intra.peff.net","threadId":"36955","inReplyTo":null,"subject":"[PATCH 0/2] dropping manual malloc calculations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T20:00:01Z","receivedAt":"2014-06-18T20:00:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"While working on the skip_prefix series, I ended up grepping for:\n\n  + strlen(\"\n\nto find spots in need of skip_prefix. Of course, it turns up many other\nnasty ad-hoc calculations. Here's a short series that addresses a few.\nThere are many more, but hopefully the first patch provides a tool that\ncan help us in the future.\n\n  [1/2]: strbuf: add xstrdup_fmt helper\n  [2/2]: use xstrdup_fmt in favor of manual size calculations\n\n-Peff\n"},{"id":"244580","messageId":"20140618200133.GA23057@sigill.intra.peff.net","threadId":"36955","inReplyTo":"20140618200000.GA22994@sigill.intra.peff.net","subject":"[PATCH 1/2] strbuf: add xstrdup_fmt helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T20:01:34Z","receivedAt":"2014-06-18T20:01:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"You can use a strbuf to build up a string from parts, and\nthen detach it. In the general case, you might use multiple\nstrbuf_add* functions to do the building. However, in many\ncases, a single strbuf_addf is sufficient, and we end up\nwith:\n\n  struct strbuf buf = STRBUF_INIT;\n  ...\n  strbuf_addf(&buf, fmt, some, args);\n  str = strbuf_detach(&buf, NULL);\n\nWe can make this much more readable (and avoid introducing\nan extra variable, which can clutter the code) by\nintroducing a convenience function:\n\n  str = xstrdup_fmt(fmt, some, args);\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI'm open to suggestions on the name. This really is the same thing\nconceptually as the GNU asprintf(), but the interface is different (that\nfunction takes a pointer-to-pointer as an out-parameter, and returns the\nnumber of characters return).\n\n strbuf.c | 19 +++++++++++++++++++\n strbuf.h |  9 +++++++++\n 2 files changed, 28 insertions(+)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex ac62982..6674d74 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -600,3 +600,22 @@ char *xstrdup_tolower(const char *string)\n \tresult[i] = '\\0';\n \treturn result;\n }\n+\n+char *xstrdup_vfmt(const char *fmt, va_list ap)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstrbuf_vaddf(&buf, fmt, ap);\n+\treturn strbuf_detach(&buf, NULL);\n+}\n+\n+char *xstrdup_fmt(const char *fmt, ...)\n+{\n+\tva_list ap;\n+\tchar *ret;\n+\n+\tva_start(ap, fmt);\n+\tret = xstrdup_vfmt(fmt, ap);\n+\tva_end(ap);\n+\n+\treturn ret;\n+}\ndiff --git a/strbuf.h b/strbuf.h\nindex e9ad03e..61818f9 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -187,4 +187,13 @@ extern int fprintf_ln(FILE *fp, const char *fmt, ...);\n \n char *xstrdup_tolower(const char *);\n \n+/*\n+ * Create a newly allocated string using printf format. You can do this easily\n+ * with a strbuf, but this provides a shortcut to save a few lines.\n+ */\n+__attribute__((format (printf, 1, 0)))\n+char *xstrdup_vfmt(const char *fmt, va_list ap);\n+__attribute__((format (printf, 1, 2)))\n+char *xstrdup_fmt(const char *fmt, ...);\n+\n #endif /* STRBUF_H */\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244581","messageId":"20140618200213.GB23057@sigill.intra.peff.net","threadId":"36955","inReplyTo":"20140618200000.GA22994@sigill.intra.peff.net","subject":"[PATCH 2/2] use xstrdup_fmt in favor of manual size calculations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T20:02:13Z","receivedAt":"2014-06-18T20:02:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In many parts of the code, we do an ugly and error-prone\nmalloc like:\n\n  const char *fmt = \"something %s\";\n  buf = xmalloc(strlen(foo) + 10 + 1);\n  sprintf(buf, fmt, foo);\n\nThis makes the code brittle, and if we ever get the\nallocation wrong, is a potential heap overflow. Let's\ninstead favor xstrdup_fmt, which handles the allocation\nautomatically, and makes the code shorter and more readable.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n remote.c       |  6 +-----\n unpack-trees.c | 17 ++++++-----------\n 2 files changed, 7 insertions(+), 16 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 0e9459c..792dcee 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -170,7 +170,6 @@ static struct branch *make_branch(const char *name, int len)\n {\n \tstruct branch *ret;\n \tint i;\n-\tchar *refname;\n \n \tfor (i = 0; i < branches_nr; i++) {\n \t\tif (len ? (!strncmp(name, branches[i]->name, len) &&\n@@ -186,10 +185,7 @@ static struct branch *make_branch(const char *name, int len)\n \t\tret->name = xstrndup(name, len);\n \telse\n \t\tret->name = xstrdup(name);\n-\trefname = xmalloc(strlen(name) + strlen(\"refs/heads/\") + 1);\n-\tstrcpy(refname, \"refs/heads/\");\n-\tstrcpy(refname + strlen(\"refs/heads/\"), ret->name);\n-\tret->refname = refname;\n+\tret->refname = xstrdup_fmt(\"refs/heads/%s\", ret->name);\n \n \treturn ret;\n }\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 97fc995..dd1e06e 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -56,17 +56,15 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n \tint i;\n \tconst char **msgs = opts->msgs;\n \tconst char *msg;\n-\tchar *tmp;\n \tconst char *cmd2 = strcmp(cmd, \"checkout\") ? cmd : \"switch branches\";\n+\n \tif (advice_commit_before_merge)\n \t\tmsg = \"Your local changes to the following files would be overwritten by %s:\\n%%s\"\n \t\t\t\"Please, commit your changes or stash them before you can %s.\";\n \telse\n \t\tmsg = \"Your local changes to the following files would be overwritten by %s:\\n%%s\";\n-\ttmp = xmalloc(strlen(msg) + strlen(cmd) + strlen(cmd2) - 2);\n-\tsprintf(tmp, msg, cmd, cmd2);\n-\tmsgs[ERROR_WOULD_OVERWRITE] = tmp;\n-\tmsgs[ERROR_NOT_UPTODATE_FILE] = tmp;\n+\tmsgs[ERROR_WOULD_OVERWRITE] = msgs[ERROR_NOT_UPTODATE_FILE] =\n+\t\txstrdup_fmt(msg, cmd, cmd2);\n \n \tmsgs[ERROR_NOT_UPTODATE_DIR] =\n \t\t\"Updating the following directories would lose untracked files in it:\\n%s\";\n@@ -76,12 +74,9 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n \t\t\t\"Please move or remove them before you can %s.\";\n \telse\n \t\tmsg = \"The following untracked working tree files would be %s by %s:\\n%%s\";\n-\ttmp = xmalloc(strlen(msg) + strlen(cmd) + strlen(\"removed\") + strlen(cmd2) - 4);\n-\tsprintf(tmp, msg, \"removed\", cmd, cmd2);\n-\tmsgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] = tmp;\n-\ttmp = xmalloc(strlen(msg) + strlen(cmd) + strlen(\"overwritten\") + strlen(cmd2) - 4);\n-\tsprintf(tmp, msg, \"overwritten\", cmd, cmd2);\n-\tmsgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = tmp;\n+\n+\tmsgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] = xstrdup_fmt(msg, \"removed\", cmd, cmd2);\n+\tmsgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = xstrdup_fmt(msg, \"overwritten\", cmd, cmd2);\n \n \t/*\n \t * Special case: ERROR_BIND_OVERLAP refers to a pair of paths, we\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244607","messageId":"xmqq7g4dj1cn.fsf@gitster.dls.corp.google.com","threadId":"36955","inReplyTo":"20140618200133.GA23057@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] strbuf: add xstrdup_fmt helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-18T22:32:08Z","receivedAt":"2014-06-18T22:32:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> You can use a strbuf to build up a string from parts, and\n> then detach it. In the general case, you might use multiple\n> strbuf_add* functions to do the building. However, in many\n> cases, a single strbuf_addf is sufficient, and we end up\n> with:\n>\n>   struct strbuf buf = STRBUF_INIT;\n>   ...\n>   strbuf_addf(&buf, fmt, some, args);\n>   str = strbuf_detach(&buf, NULL);\n>\n> We can make this much more readable (and avoid introducing\n> an extra variable, which can clutter the code) by\n> introducing a convenience function:\n>\n>   str = xstrdup_fmt(fmt, some, args);\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I'm open to suggestions on the name. This really is the same thing\n> conceptually as the GNU asprintf(), but the interface is different (that\n> function takes a pointer-to-pointer as an out-parameter, and returns the\n> number of characters return).\n\nNaming it with anything \"dup\" certainly feels wrong.  The returned\nstring is not a duplicate of anything.\n\nTo me, the function feels like an \"sprintf done right\"; as you said,\nthe best name for \"printf-like format into an allocated piece of\nmemory\" is unfortunately taken as asprintf(3).\n\nI wonder if our callers can instead use asprintf(3) with its\nslightly more quirky API (and then we supply compat/asprintf.c for\nnon-GNU platforms).  Right now we only have three call sites, but if\nwe anticipate that \"printf-like format into an allocated piece of\nmemory\" will prove be generally useful in our code base, following\nan API that other people already have established may give our\ndevelopers one less thing that they have to learn.\n\nAs usual, I would expect we would have xasprintf wrapper around it\nto die instead of returning -1 upon allocation failure.\n\nThe call sites do not look too bad (see below) if we were to go that\nroute instead.\n\n\n remote.c       |  2 +-\n unpack-trees.c | 10 ++++++----\n 2 files changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex b46f467..87fa7ec 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -185,7 +185,7 @@ static struct branch *make_branch(const char *name, int len)\n \t\tret->name = xstrndup(name, len);\n \telse\n \t\tret->name = xstrdup(name);\n-\tret->refname = xstrdup_fmt(\"refs/heads/%s\", ret->name);\n+\tasprintf(&ret->refname, \"refs/heads/%s\", ret->name);\n \n \treturn ret;\n }\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex dd1e06e..d6a07b8 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -63,8 +63,8 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n \t\t\t\"Please, commit your changes or stash them before you can %s.\";\n \telse\n \t\tmsg = \"Your local changes to the following files would be overwritten by %s:\\n%%s\";\n-\tmsgs[ERROR_WOULD_OVERWRITE] = msgs[ERROR_NOT_UPTODATE_FILE] =\n-\t\txstrdup_fmt(msg, cmd, cmd2);\n+\txasprintf(&msgs[ERROR_WOULD_OVERWRITE], msg, cmd, cmd2);\n+\tmsgs[ERROR_NOT_UPTODATE_FILE] =\tmsgs[ERROR_WOULD_OVERWRITE];\n \n \tmsgs[ERROR_NOT_UPTODATE_DIR] =\n \t\t\"Updating the following directories would lose untracked files in it:\\n%s\";\n@@ -75,8 +75,10 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n \telse\n \t\tmsg = \"The following untracked working tree files would be %s by %s:\\n%%s\";\n \n-\tmsgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] = xstrdup_fmt(msg, \"removed\", cmd, cmd2);\n-\tmsgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = xstrdup_fmt(msg, \"overwritten\", cmd, cmd2);\n+\txasprintf(&msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED],\n+\t\t msg, \"removed\", cmd, cmd2);\n+\txasprintf(&msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN],\n+\t\t msg, \"overwritten\", cmd, cmd2);\n \n \t/*\n \t * Special case: ERROR_BIND_OVERLAP refers to a pair of paths, we\n"},{"id":"244633","messageId":"20140619090532.GB1009@sigill.intra.peff.net","threadId":"36955","inReplyTo":"xmqq7g4dj1cn.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] strbuf: add xstrdup_fmt helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-19T09:05:32Z","receivedAt":"2014-06-19T09:05:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 18, 2014 at 03:32:08PM -0700, Junio C Hamano wrote:\n\n> >   str = xstrdup_fmt(fmt, some, args);\n> > ---\n> > I'm open to suggestions on the name. This really is the same thing\n> > conceptually as the GNU asprintf(), but the interface is different (that\n> > function takes a pointer-to-pointer as an out-parameter, and returns the\n> > number of characters return).\n> \n> Naming it with anything \"dup\" certainly feels wrong.  The returned\n> string is not a duplicate of anything.\n\nI was somewhat inspired by \"mkpathdup\" and friends. It is quite simialr\nto mkpathdup, except that it is not limited to paths (and does not do\nany path-specific munging). I agree something with \"printf\" in the name\nis probably more descriptive, though.\n\n> I wonder if our callers can instead use asprintf(3) with its\n> slightly more quirky API (and then we supply compat/asprintf.c for\n> non-GNU platforms).  Right now we only have three call sites, but if\n> we anticipate that \"printf-like format into an allocated piece of\n> memory\" will prove be generally useful in our code base, following\n> an API that other people already have established may give our\n> developers one less thing that they have to learn.\n\nI considered that, but I do find asprintf's interface unnecessarily\nannoying (you can't return a value directly, and you run afoul of const\nissues when passing pointers to pointers). As you noted, it's not _too_\nbad, but we really get nothing from the integer return type. AFAICT, it\nhelps with:\n\n  1. You know how many characters are in the string. If you cared about\n     that here, you would just use a strbuf directly, which is much more\n     flexible.\n\n  2. The error handling is different. But our x-variant would just die()\n     on errors anyway, so we do not care.\n\nSo modeling after asprintf feels like carrying around unnecessary\nbaggage (and I am not convinced asprintf is in wide enough use, nor that\nthe function is complicated enough to care about developer familiarity).\nPart of me is tempted to call it xasprintf anyway, and use our own\ninterface. GNU does not get to squat on that useful name. ;)\n\nThat is probably unnecessarily confusing, though.\n\n> As usual, I would expect we would have xasprintf wrapper around it\n> to die instead of returning -1 upon allocation failure.\n\nWould it be crazy to just call it xprintf? By our current scheme that\nwould be \"a wrapper for printf\", which means it is potentially\nconfusing.\n\nLiterally any other unused letter would be fine. dprintf for detach? I\ndunno.\n\n-Peff\n"},{"id":"244694","messageId":"xmqq38f0j13u.fsf@gitster.dls.corp.google.com","threadId":"36955","inReplyTo":"20140619090532.GB1009@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] strbuf: add xstrdup_fmt helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-19T16:49:41Z","receivedAt":"2014-06-19T16:49:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Jun 18, 2014 at 03:32:08PM -0700, Junio C Hamano wrote:\n>\n>> >   str = xstrdup_fmt(fmt, some, args);\n>> > ---\n>> > I'm open to suggestions on the name. This really is the same thing\n>> > conceptually as the GNU asprintf(), but the interface is different (that\n>> > function takes a pointer-to-pointer as an out-parameter, and returns the\n>> > number of characters return).\n>> \n>> Naming it with anything \"dup\" certainly feels wrong.  The returned\n>> string is not a duplicate of anything.\n>\n> I was somewhat inspired by \"mkpathdup\" and friends.\n\nThat name is from \"mkpath gives its result in a static buffer and\nforces the callers to xstrdup() if they want to keep the result;\nthis is a thin wrapper to do so for the caller\".  As there is no\nstr_fmt() that gives its result in a static, so...\n\n> I considered that, but I do find asprintf's interface unnecessarily\n> annoying...\n\nYes, it is annoying.\n\n> Would it be crazy to just call it xprintf? By our current scheme that\n> would be \"a wrapper for printf\", which means it is potentially\n> confusing.\n>\n> Literally any other unused letter would be fine. dprintf for detach? I\n> dunno.\n\nIf we twist the logic behind the 'mkpathdup' name a little bit,\nperhaps we can call it sprintf_dup or something.  That is, \"sprintf\nwants a fixed preallocated piece of memory to print into, and we\nrelieve the callers of having to do so\", perhaps?\n\ndprintf, mprintf, etc. are also fine by me.\n"},{"id":"244695","messageId":"53A31557.8070901@web.de","threadId":"36955","inReplyTo":"20140619090532.GB1009@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] strbuf: add xstrdup_fmt helper","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-06-19T16:52:39Z","receivedAt":"2014-06-19T16:52:39Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 19.06.2014 11:05, schrieb Jeff King:\n> On Wed, Jun 18, 2014 at 03:32:08PM -0700, Junio C Hamano wrote:\n>\n>>>    str = xstrdup_fmt(fmt, some, args);\n>>> ---\n>>> I'm open to suggestions on the name. This really is the same thing\n>>> conceptually as the GNU asprintf(), but the interface is different (that\n>>> function takes a pointer-to-pointer as an out-parameter, and returns the\n>>> number of characters return).\n>>\n>> Naming it with anything \"dup\" certainly feels wrong.  The returned\n>> string is not a duplicate of anything.\n>\n> I was somewhat inspired by \"mkpathdup\" and friends. It is quite simialr\n> to mkpathdup, except that it is not limited to paths (and does not do\n> any path-specific munging). I agree something with \"printf\" in the name\n> is probably more descriptive, though.\n>\n>> I wonder if our callers can instead use asprintf(3) with its\n>> slightly more quirky API (and then we supply compat/asprintf.c for\n>> non-GNU platforms).  Right now we only have three call sites, but if\n>> we anticipate that \"printf-like format into an allocated piece of\n>> memory\" will prove be generally useful in our code base, following\n>> an API that other people already have established may give our\n>> developers one less thing that they have to learn.\n>\n> I considered that, but I do find asprintf's interface unnecessarily\n> annoying (you can't return a value directly, and you run afoul of const\n> issues when passing pointers to pointers). As you noted, it's not _too_\n> bad, but we really get nothing from the integer return type. AFAICT, it\n> helps with:\n>\n>    1. You know how many characters are in the string. If you cared about\n>       that here, you would just use a strbuf directly, which is much more\n>       flexible.\n>\n>    2. The error handling is different. But our x-variant would just die()\n>       on errors anyway, so we do not care.\n>\n> So modeling after asprintf feels like carrying around unnecessary\n> baggage (and I am not convinced asprintf is in wide enough use, nor that\n> the function is complicated enough to care about developer familiarity).\n> Part of me is tempted to call it xasprintf anyway, and use our own\n> interface. GNU does not get to squat on that useful name. ;)\n>\n> That is probably unnecessarily confusing, though.\n>\n>> As usual, I would expect we would have xasprintf wrapper around it\n>> to die instead of returning -1 upon allocation failure.\n>\n> Would it be crazy to just call it xprintf? By our current scheme that\n> would be \"a wrapper for printf\", which means it is potentially\n> confusing.\n>\n> Literally any other unused letter would be fine. dprintf for detach? I\n> dunno.\n\nI agree that \"dup\" doesn't fit and that potential callers don't need the \nlength of the generated string (or should use strbuf otherwise).\n\nIncluding \"print\" in the name is not necessary, though -- die(), error() \nand friends work fine without them.\n\nWhat about simply removing the \"dup_\" part and using xstrfmt?\n\nRené\n"},{"id":"244712","messageId":"20140619211659.GA32412@sigill.intra.peff.net","threadId":"36955","inReplyTo":"xmqq38f0j13u.fsf@gitster.dls.corp.google.com","subject":"[PATCH v2] dropping manual malloc calculations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-19T21:16:59Z","receivedAt":"2014-06-19T21:16:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 19, 2014 at 09:49:41AM -0700, Junio C Hamano wrote:\n\n> If we twist the logic behind the 'mkpathdup' name a little bit,\n> perhaps we can call it sprintf_dup or something.  That is, \"sprintf\n> wants a fixed preallocated piece of memory to print into, and we\n> relieve the callers of having to do so\", perhaps?\n\nRight, that is the \"dup\" I was thinking of. Anyway, I think René's\n\"xstrfmt\" is my favorite of the suggestions, and you seemed to like it,\ntoo. Here is a re-roll using that name.\n\nThe first two patches are the same except for the name change. The rest\nare more applications of it (and other techniques) found by grepping for\n\"malloc.*strlen\". The diffstat is very encouraging, and I think the\nresults are much more readable (in addition to being more obviously\ncorrect). The last two also fix real \"bugs\", but I doubt either is a\nproblem in practice.\n\n    builtin/apply.c         |  4 +--\n    builtin/fetch.c         |  9 ++----\n    builtin/fmt-merge-msg.c |  7 ++---\n    builtin/name-rev.c      |  5 +---\n    builtin/receive-pack.c  |  5 +---\n    builtin/show-branch.c   | 10 +++----\n    environment.c           | 12 +++-----\n    http-push.c             | 24 +++++-----------\n    http-walker.c           |  6 ++--\n    match-trees.c           |  9 ++----\n    merge-recursive.c       | 53 ++++++++++++++++++++---------------\n    merge.c                 | 42 +++++++++------------------\n    remote.c                |  6 +---\n    sequencer.c             |  9 ++----\n    sha1_name.c             |  5 +---\n    shell.c                 |  6 +---\n    strbuf.c                | 19 +++++++++++++\n    strbuf.h                |  9 ++++++\n    unpack-trees.c          | 17 ++++-------\n    walker.c                | 18 ++++++------\n    20 files changed, 117 insertions(+), 158 deletions(-)\n\n  [01/10]: strbuf: add xstrfmt helper\n  [02/10]: use xstrfmt in favor of manual size calculations\n  [03/10]: use xstrdup instead of xmalloc + strcpy\n  [04/10]: use xstrfmt to replace xmalloc + sprintf\n  [05/10]: use xstrfmt to replace xmalloc + strcpy/strcat\n  [06/10]: setup_git_env: use git_pathdup instead of xmalloc + sprintf\n  [07/10]: sequencer: use argv_array_pushf\n  [08/10]: merge: use argv_array when spawning merge strategy\n  [09/10]: walker_fetch: fix minor memory leak\n  [10/10]: unique_path: fix unlikely heap overflow\n"},{"id":"244713","messageId":"20140619211812.GA28474@sigill.intra.peff.net","threadId":"36955","inReplyTo":"20140619211659.GA32412@sigill.intra.peff.net","subject":"[PATCH v2 01/10] strbuf: add xstrfmt helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-19T21:18:12Z","receivedAt":"2014-06-19T21:18:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"You can use a strbuf to build up a string from parts, and\nthen detach it. In the general case, you might use multiple\nstrbuf_add* functions to do the building. However, in many\ncases, a single strbuf_addf is sufficient, and we end up\nwith:\n\n  struct strbuf buf = STRBUF_INIT;\n  ...\n  strbuf_addf(&buf, fmt, some, args);\n  str = strbuf_detach(&buf, NULL);\n\nWe can make this much more readable (and avoid introducing\nan extra variable, which can clutter the code) by\nintroducing a convenience function:\n\n  str = xstrfmt(fmt, some, args);\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n strbuf.c | 19 +++++++++++++++++++\n strbuf.h |  9 +++++++++\n 2 files changed, 28 insertions(+)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex ac62982..12c7865 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -600,3 +600,22 @@ char *xstrdup_tolower(const char *string)\n \tresult[i] = '\\0';\n \treturn result;\n }\n+\n+char *xstrvfmt(const char *fmt, va_list ap)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstrbuf_vaddf(&buf, fmt, ap);\n+\treturn strbuf_detach(&buf, NULL);\n+}\n+\n+char *xstrfmt(const char *fmt, ...)\n+{\n+\tva_list ap;\n+\tchar *ret;\n+\n+\tva_start(ap, fmt);\n+\tret = xstrvfmt(fmt, ap);\n+\tva_end(ap);\n+\n+\treturn ret;\n+}\ndiff --git a/strbuf.h b/strbuf.h\nindex e9ad03e..a594c24 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -187,4 +187,13 @@ extern int fprintf_ln(FILE *fp, const char *fmt, ...);\n \n char *xstrdup_tolower(const char *);\n \n+/*\n+ * Create a newly allocated string using printf format. You can do this easily\n+ * with a strbuf, but this provides a shortcut to save a few lines.\n+ */\n+__attribute__((format (printf, 1, 0)))\n+char *xstrvfmt(const char *fmt, va_list ap);\n+__attribute__((format (printf, 1, 2)))\n+char *xstrfmt(const char *fmt, ...);\n+\n #endif /* STRBUF_H */\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244714","messageId":"20140619211918.GB28474@sigill.intra.peff.net","threadId":"36955","inReplyTo":"20140619211659.GA32412@sigill.intra.peff.net","subject":"[PATCH v2 02/10] use xstrfmt in favor of manual size calculations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-19T21:19:18Z","receivedAt":"2014-06-19T21:19:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In many parts of the code, we do an ugly and error-prone\nmalloc like:\n\n  const char *fmt = \"something %s\";\n  buf = xmalloc(strlen(foo) + 10 + 1);\n  sprintf(buf, fmt, foo);\n\nThis makes the code brittle, and if we ever get the\nallocation wrong, is a potential heap overflow. Let's\ninstead favor xstrfmt, which handles the allocation\nautomatically, and makes the code shorter and more readable.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThese could actually be squashed into later commits, I suppose, but I\nleft it separate since it had already been reviewed.\n\n remote.c       |  6 +-----\n unpack-trees.c | 17 ++++++-----------\n 2 files changed, 7 insertions(+), 16 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 0e9459c..bf27e44 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -170,7 +170,6 @@ static struct branch *make_branch(const char *name, int len)\n {\n \tstruct branch *ret;\n \tint i;\n-\tchar *refname;\n \n \tfor (i = 0; i < branches_nr; i++) {\n \t\tif (len ? (!strncmp(name, branches[i]->name, len) &&\n@@ -186,10 +185,7 @@ static struct branch *make_branch(const char *name, int len)\n \t\tret->name = xstrndup(name, len);\n \telse\n \t\tret->name = xstrdup(name);\n-\trefname = xmalloc(strlen(name) + strlen(\"refs/heads/\") + 1);\n-\tstrcpy(refname, \"refs/heads/\");\n-\tstrcpy(refname + strlen(\"refs/heads/\"), ret->name);\n-\tret->refname = refname;\n+\tret->refname = xstrfmt(\"refs/heads/%s\", ret->name);\n \n \treturn ret;\n }\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 97fc995..c237370 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -56,17 +56,15 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n \tint i;\n \tconst char **msgs = opts->msgs;\n \tconst char *msg;\n-\tchar *tmp;\n \tconst char *cmd2 = strcmp(cmd, \"checkout\") ? cmd : \"switch branches\";\n+\n \tif (advice_commit_before_merge)\n \t\tmsg = \"Your local changes to the following files would be overwritten by %s:\\n%%s\"\n \t\t\t\"Please, commit your changes or stash them before you can %s.\";\n \telse\n \t\tmsg = \"Your local changes to the following files would be overwritten by %s:\\n%%s\";\n-\ttmp = xmalloc(strlen(msg) + strlen(cmd) + strlen(cmd2) - 2);\n-\tsprintf(tmp, msg, cmd, cmd2);\n-\tmsgs[ERROR_WOULD_OVERWRITE] = tmp;\n-\tmsgs[ERROR_NOT_UPTODATE_FILE] = tmp;\n+\tmsgs[ERROR_WOULD_OVERWRITE] = msgs[ERROR_NOT_UPTODATE_FILE] =\n+\t\txstrfmt(msg, cmd, cmd2);\n \n \tmsgs[ERROR_NOT_UPTODATE_DIR] =\n \t\t\"Updating the following directories would lose untracked files in it:\\n%s\";\n@@ -76,12 +74,9 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n \t\t\t\"Please move or remove them before you can %s.\";\n \telse\n \t\tmsg = \"The following untracked working tree files would be %s by %s:\\n%%s\";\n-\ttmp = xmalloc(strlen(msg) + strlen(cmd) + strlen(\"removed\") + strlen(cmd2) - 4);\n-\tsprintf(tmp, msg, \"removed\", cmd, cmd2);\n-\tmsgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] = tmp;\n-\ttmp = xmalloc(strlen(msg) + strlen(cmd) + strlen(\"overwritten\") + strlen(cmd2) - 4);\n-\tsprintf(tmp, msg, \"overwritten\", cmd, cmd2);\n-\tmsgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = tmp;\n+\n+\tmsgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] = xstrfmt(msg, \"removed\", cmd, cmd2);\n+\tmsgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = xstrfmt(msg, \"overwritten\", cmd, cmd2);\n \n \t/*\n \t * Special case: ERROR_BIND_OVERLAP refers to a pair of paths, we\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244715","messageId":"20140619211942.GC28474@sigill.intra.peff.net","threadId":"36955","inReplyTo":"20140619211659.GA32412@sigill.intra.peff.net","subject":"[PATCH v2 03/10] use xstrdup instead of xmalloc + strcpy","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-19T21:19:43Z","receivedAt":"2014-06-19T21:19:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is one line shorter, and makes sure the length in the\nmalloc and copy steps match.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/receive-pack.c | 5 +----\n http-push.c            | 6 ++----\n http-walker.c          | 3 +--\n 3 files changed, 4 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex c323081..18458e8 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -614,12 +614,9 @@ static void run_update_post_hook(struct command *commands)\n \targv[0] = hook;\n \n \tfor (argc = 1, cmd = commands; cmd; cmd = cmd->next) {\n-\t\tchar *p;\n \t\tif (cmd->error_string || cmd->did_not_exist)\n \t\t\tcontinue;\n-\t\tp = xmalloc(strlen(cmd->ref_name) + 1);\n-\t\tstrcpy(p, cmd->ref_name);\n-\t\targv[argc] = p;\n+\t\targv[argc] = xstrdup(cmd->ref_name);\n \t\targc++;\n \t}\n \targv[argc] = NULL;\ndiff --git a/http-push.c b/http-push.c\nindex de00d16..95650a0 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -767,15 +767,13 @@ static void handle_new_lock_ctx(struct xml_ctx *ctx, int tag_closed)\n \n \tif (tag_closed && ctx->cdata) {\n \t\tif (!strcmp(ctx->name, DAV_ACTIVELOCK_OWNER)) {\n-\t\t\tlock->owner = xmalloc(strlen(ctx->cdata) + 1);\n-\t\t\tstrcpy(lock->owner, ctx->cdata);\n+\t\t\tlock->owner = xstrdup(ctx->cdata);\n \t\t} else if (!strcmp(ctx->name, DAV_ACTIVELOCK_TIMEOUT)) {\n \t\t\tif (starts_with(ctx->cdata, \"Second-\"))\n \t\t\t\tlock->timeout =\n \t\t\t\t\tstrtol(ctx->cdata + 7, NULL, 10);\n \t\t} else if (!strcmp(ctx->name, DAV_ACTIVELOCK_TOKEN)) {\n-\t\t\tlock->token = xmalloc(strlen(ctx->cdata) + 1);\n-\t\t\tstrcpy(lock->token, ctx->cdata);\n+\t\t\tlock->token = xstrdup(ctx->cdata);\n \n \t\t\tgit_SHA1_Init(&sha_ctx);\n \t\t\tgit_SHA1_Update(&sha_ctx, lock->token, strlen(lock->token));\ndiff --git a/http-walker.c b/http-walker.c\nindex 1516c5e..ab6a4fe 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -566,8 +566,7 @@ struct walker *get_http_walker(const char *url)\n \tstruct walker *walker = xmalloc(sizeof(struct walker));\n \n \tdata->alt = xmalloc(sizeof(*data->alt));\n-\tdata->alt->base = xmalloc(strlen(url) + 1);\n-\tstrcpy(data->alt->base, url);\n+\tdata->alt->base = xstrdup(url);\n \tfor (s = data->alt->base + strlen(data->alt->base) - 1; *s == '/'; --s)\n \t\t*s = 0;\n \n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244716","messageId":"20140619212433.GD28474@sigill.intra.peff.net","threadId":"36955","inReplyTo":"20140619211659.GA32412@sigill.intra.peff.net","subject":"[PATCH v2 04/10] use xstrfmt to replace xmalloc + sprintf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-19T21:24:33Z","receivedAt":"2014-06-19T21:24:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is one line shorter, and makes sure the length in the\nmalloc and sprintf steps match.\n\nThese conversions are very straightforward; we can drop the\nmalloc entirely, and replace the sprintf with xstrfmt.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nJust a note on one thing I would look for as a reviewer:\n\n  In theory this could introduce a time-of-use error (either we xstrfmt\n  at the malloc, in which case the arguments to format might not be\n  ready yet, or we xstrfmt where the old sprintf was, in which case the\n  pointer is uninitialized earlier). In practice, this is not an issue.\n  The two are almost always right next to each other. And even when they\n  are not, the xmalloc already runs strlen() on the arguments, so it\n  should be safe to do xstrfmt there, too. I.e., if there is a bug, it\n  was already there. :)\n\n builtin/fmt-merge-msg.c |  7 ++-----\n builtin/show-branch.c   | 10 ++++------\n http-push.c             | 18 +++++-------------\n http-walker.c           |  3 +--\n match-trees.c           |  9 ++-------\n merge-recursive.c       | 12 ++++--------\n 6 files changed, 18 insertions(+), 41 deletions(-)\n\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex 3906eda..c462e19 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -178,11 +178,8 @@ static int handle_line(char *line, struct merge_parents *merge_parents)\n \t\tint len = strlen(origin);\n \t\tif (origin[0] == '\\'' && origin[len - 1] == '\\'')\n \t\t\torigin = xmemdupz(origin + 1, len - 2);\n-\t} else {\n-\t\tchar *new_origin = xmalloc(strlen(origin) + strlen(src) + 5);\n-\t\tsprintf(new_origin, \"%s of %s\", origin, src);\n-\t\torigin = new_origin;\n-\t}\n+\t} else\n+\t\torigin = xstrfmt(\"%s of %s\", origin, src);\n \tif (strcmp(\".\", src))\n \t\torigin_data->is_local_branch = 0;\n \tstring_list_append(&origins, origin)->util = origin_data;\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex d873172..5fd4e4e 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -755,7 +755,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t}\n \n \t\tfor (i = 0; i < reflog; i++) {\n-\t\t\tchar *logmsg, *m;\n+\t\t\tchar *logmsg;\n \t\t\tconst char *msg;\n \t\t\tunsigned long timestamp;\n \t\t\tint tz;\n@@ -770,11 +770,9 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\t\tmsg = \"(none)\";\n \t\t\telse\n \t\t\t\tmsg++;\n-\t\t\tm = xmalloc(strlen(msg) + 200);\n-\t\t\tsprintf(m, \"(%s) %s\",\n-\t\t\t\tshow_date(timestamp, tz, 1),\n-\t\t\t\tmsg);\n-\t\t\treflog_msg[i] = m;\n+\t\t\treflog_msg[i] = xstrfmt(\"(%s) %s\",\n+\t\t\t\t\t\tshow_date(timestamp, tz, 1),\n+\t\t\t\t\t\tmsg);\n \t\t\tfree(logmsg);\n \t\t\tsprintf(nth_desc, \"%s@{%d}\", *av, base+i);\n \t\t\tappend_ref(nth_desc, sha1, 1);\ndiff --git a/http-push.c b/http-push.c\nindex 95650a0..390f74c 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -854,8 +854,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)\n \tstruct xml_ctx ctx;\n \tchar *escaped;\n \n-\turl = xmalloc(strlen(repo->url) + strlen(path) + 1);\n-\tsprintf(url, \"%s%s\", repo->url, path);\n+\turl = xstrfmt(\"%s%s\", repo->url, path);\n \n \t/* Make sure leading directories exist for the remote ref */\n \tep = strchr(url + strlen(repo->url) + 1, '/');\n@@ -1115,7 +1114,7 @@ static void remote_ls(const char *path, int flags,\n \t\t      void (*userFunc)(struct remote_ls_ctx *ls),\n \t\t      void *userData)\n {\n-\tchar *url = xmalloc(strlen(repo->url) + strlen(path) + 1);\n+\tchar *url = xstrfmt(\"%s%s\", repo->url, path);\n \tstruct active_request_slot *slot;\n \tstruct slot_results results;\n \tstruct strbuf in_buffer = STRBUF_INIT;\n@@ -1131,8 +1130,6 @@ static void remote_ls(const char *path, int flags,\n \tls.userData = userData;\n \tls.userFunc = userFunc;\n \n-\tsprintf(url, \"%s%s\", repo->url, path);\n-\n \tstrbuf_addf(&out_buffer.buf, PROPFIND_ALL_REQUEST);\n \n \tdav_headers = curl_slist_append(dav_headers, \"Depth: 1\");\n@@ -1534,10 +1531,9 @@ static void update_remote_info_refs(struct remote_lock *lock)\n \n static int remote_exists(const char *path)\n {\n-\tchar *url = xmalloc(strlen(repo->url) + strlen(path) + 1);\n+\tchar *url = xstrfmt(\"%s%s\", repo->url, path);\n \tint ret;\n \n-\tsprintf(url, \"%s%s\", repo->url, path);\n \n \tswitch (http_get_strbuf(url, NULL, NULL)) {\n \tcase HTTP_OK:\n@@ -1557,12 +1553,9 @@ static int remote_exists(const char *path)\n \n static void fetch_symref(const char *path, char **symref, unsigned char *sha1)\n {\n-\tchar *url;\n+\tchar *url = xstrfmt(\"%s%s\", repo->url, path);\n \tstruct strbuf buffer = STRBUF_INIT;\n \n-\turl = xmalloc(strlen(repo->url) + strlen(path) + 1);\n-\tsprintf(url, \"%s%s\", repo->url, path);\n-\n \tif (http_get_strbuf(url, &buffer, NULL) != HTTP_OK)\n \t\tdie(\"Couldn't get %s for remote symref\\n%s\", url,\n \t\t    curl_errorstr);\n@@ -1671,8 +1664,7 @@ static int delete_remote_branch(const char *pattern, int force)\n \tfprintf(stderr, \"Removing remote branch '%s'\\n\", remote_ref->name);\n \tif (dry_run)\n \t\treturn 0;\n-\turl = xmalloc(strlen(repo->url) + strlen(remote_ref->name) + 1);\n-\tsprintf(url, \"%s%s\", repo->url, remote_ref->name);\n+\turl = xstrfmt(\"%s%s\", repo->url, remote_ref->name);\n \tslot = get_active_slot();\n \tslot->results = &results;\n \tcurl_setup_http_get(slot->curl, url, DAV_DELETE);\ndiff --git a/http-walker.c b/http-walker.c\nindex ab6a4fe..dbddfaa 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -341,8 +341,7 @@ static void fetch_alternates(struct walker *walker, const char *base)\n \tif (walker->get_verbosely)\n \t\tfprintf(stderr, \"Getting alternates list for %s\\n\", base);\n \n-\turl = xmalloc(strlen(base) + 31);\n-\tsprintf(url, \"%s/objects/info/http-alternates\", base);\n+\turl = xstrfmt(\"%s/objects/info/http-alternates\", base);\n \n \t/*\n \t * Use a callback to process the result, since another request\ndiff --git a/match-trees.c b/match-trees.c\nindex e80b4af..1ce0954 100644\n--- a/match-trees.c\n+++ b/match-trees.c\n@@ -140,17 +140,12 @@ static void match_trees(const unsigned char *hash1,\n \t\t\tgoto next;\n \t\tscore = score_trees(elem, hash2);\n \t\tif (*best_score < score) {\n-\t\t\tchar *newpath;\n-\t\t\tnewpath = xmalloc(strlen(base) + strlen(path) + 1);\n-\t\t\tsprintf(newpath, \"%s%s\", base, path);\n \t\t\tfree(*best_match);\n-\t\t\t*best_match = newpath;\n+\t\t\t*best_match = xstrfmt(\"%s%s\", base, path);\n \t\t\t*best_score = score;\n \t\t}\n \t\tif (recurse_limit) {\n-\t\t\tchar *newbase;\n-\t\t\tnewbase = xmalloc(strlen(base) + strlen(path) + 2);\n-\t\t\tsprintf(newbase, \"%s%s/\", base, path);\n+\t\t\tchar *newbase = xstrfmt(\"%s%s/\", base, path);\n \t\t\tmatch_trees(elem, hash2, best_score, best_match,\n \t\t\t\t    newbase, recurse_limit - 1);\n \t\t\tfree(newbase);\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex cab16fa..532a1da 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -969,14 +969,10 @@ merge_file_special_markers(struct merge_options *o,\n \tchar *side2 = NULL;\n \tstruct merge_file_info mfi;\n \n-\tif (filename1) {\n-\t\tside1 = xmalloc(strlen(branch1) + strlen(filename1) + 2);\n-\t\tsprintf(side1, \"%s:%s\", branch1, filename1);\n-\t}\n-\tif (filename2) {\n-\t\tside2 = xmalloc(strlen(branch2) + strlen(filename2) + 2);\n-\t\tsprintf(side2, \"%s:%s\", branch2, filename2);\n-\t}\n+\tif (filename1)\n+\t\tside1 = xstrfmt(\"%s:%s\", branch1, filename1);\n+\tif (filename2)\n+\t\tside2 = xstrfmt(\"%s:%s\", branch2, filename2);\n \n \tmfi = merge_file_1(o, one, a, b,\n \t\t\t   side1 ? side1 : branch1, side2 ? side2 : branch2);\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244717","messageId":"20140619212656.GE28474@sigill.intra.peff.net","threadId":"36955","inReplyTo":"20140619211659.GA32412@sigill.intra.peff.net","subject":"[PATCH v2 05/10] use xstrfmt to replace xmalloc + strcpy/strcat","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-19T21:26:56Z","receivedAt":"2014-06-19T21:26:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's easy to get manual allocation calculations wrong, and\nthe use of strcpy/strcat raise red flags for people looking\nfor buffer overflows (though in this case each site was\nfine).\n\nIt's also shorter to use xstrfmt, and the printf-format\ntends to be easier for a reader to see what the final string\nwill look like.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nBy the way, I think that the tip_name allocation in name_rev leaks\nbadly, but it's a little tricky to fix (we sometimes hand off ownership\nof the variable, and sometimes not). However, this patch does not make\nit any worse, and nobody is complaining, so I left it for now.\n\n builtin/apply.c    | 4 +---\n builtin/fetch.c    | 9 ++-------\n builtin/name-rev.c | 5 +----\n sha1_name.c        | 5 +----\n shell.c            | 6 +-----\n 5 files changed, 6 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 9c5724e..b796910 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -1281,9 +1281,7 @@ static int parse_git_header(const char *line, int len, unsigned int size, struct\n \t */\n \tpatch->def_name = git_header_name(line, len);\n \tif (patch->def_name && root) {\n-\t\tchar *s = xmalloc(root_len + strlen(patch->def_name) + 1);\n-\t\tstrcpy(s, root);\n-\t\tstrcpy(s + root_len, patch->def_name);\n+\t\tchar *s = xstrfmt(\"%s%s\", root, patch->def_name);\n \t\tfree(patch->def_name);\n \t\tpatch->def_name = s;\n \t}\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 55f457c..40d989f 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1053,16 +1053,11 @@ static int fetch_one(struct remote *remote, int argc, const char **argv)\n \t\trefs = xcalloc(argc + 1, sizeof(const char *));\n \t\tfor (i = 0; i < argc; i++) {\n \t\t\tif (!strcmp(argv[i], \"tag\")) {\n-\t\t\t\tchar *ref;\n \t\t\t\ti++;\n \t\t\t\tif (i >= argc)\n \t\t\t\t\tdie(_(\"You need to specify a tag name.\"));\n-\t\t\t\tref = xmalloc(strlen(argv[i]) * 2 + 22);\n-\t\t\t\tstrcpy(ref, \"refs/tags/\");\n-\t\t\t\tstrcat(ref, argv[i]);\n-\t\t\t\tstrcat(ref, \":refs/tags/\");\n-\t\t\t\tstrcat(ref, argv[i]);\n-\t\t\t\trefs[j++] = ref;\n+\t\t\t\trefs[j++] = xstrfmt(\"refs/tags/%s:refs/tags/%s\",\n+\t\t\t\t\t\t    argv[i], argv[i]);\n \t\t\t} else\n \t\t\t\trefs[j++] = argv[i];\n \t\t}\ndiff --git a/builtin/name-rev.c b/builtin/name-rev.c\nindex c824d4e..3c8f319 100644\n--- a/builtin/name-rev.c\n+++ b/builtin/name-rev.c\n@@ -33,10 +33,7 @@ static void name_rev(struct commit *commit,\n \t\treturn;\n \n \tif (deref) {\n-\t\tchar *new_name = xmalloc(strlen(tip_name)+3);\n-\t\tstrcpy(new_name, tip_name);\n-\t\tstrcat(new_name, \"^0\");\n-\t\ttip_name = new_name;\n+\t\ttip_name = xstrfmt(\"%s^0\", tip_name);\n \n \t\tif (generation)\n \t\t\tdie(\"generation: %d, but deref?\", generation);\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 2b6322f..5e95690 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1252,10 +1252,7 @@ static void diagnose_invalid_sha1_path(const char *prefix,\n \t\tdie(\"Path '%s' exists on disk, but not in '%.*s'.\",\n \t\t    filename, object_name_len, object_name);\n \tif (errno == ENOENT || errno == ENOTDIR) {\n-\t\tchar *fullname = xmalloc(strlen(filename)\n-\t\t\t\t\t     + strlen(prefix) + 1);\n-\t\tstrcpy(fullname, prefix);\n-\t\tstrcat(fullname, filename);\n+\t\tchar *fullname = xstrfmt(\"%s%s\", prefix, filename);\n \n \t\tif (!get_tree_entry(tree_sha1, fullname,\n \t\t\t\t    sha1, &mode)) {\ndiff --git a/shell.c b/shell.c\nindex 5c0d47a..ace62e4 100644\n--- a/shell.c\n+++ b/shell.c\n@@ -46,11 +46,7 @@ static int is_valid_cmd_name(const char *cmd)\n \n static char *make_cmd(const char *prog)\n {\n-\tchar *prefix = xmalloc((strlen(prog) + strlen(COMMAND_DIR) + 2));\n-\tstrcpy(prefix, COMMAND_DIR);\n-\tstrcat(prefix, \"/\");\n-\tstrcat(prefix, prog);\n-\treturn prefix;\n+\treturn xstrfmt(\"%s/%s\", COMMAND_DIR, prog);\n }\n \n static void cd_to_homedir(void)\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244718","messageId":"20140619212800.GF28474@sigill.intra.peff.net","threadId":"36955","inReplyTo":"20140619211659.GA32412@sigill.intra.peff.net","subject":"[PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-19T21:28:00Z","receivedAt":"2014-06-19T21:28:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is shorter, harder to get wrong, and more clearly\ncaptures the intent.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI wondered if there was a reason to avoid this (because we are in\nsetup_git_env, which can potentially be called by git_pathdup). But the\ngit_graft_file initialization below already uses it, and I\ndouble-checked that it is safe once git_dir is set.\n\n environment.c | 12 ++++--------\n 1 file changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex 4dac5e9..4de7b81 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -135,15 +135,11 @@ static void setup_git_env(void)\n \tgitfile = read_gitfile(git_dir);\n \tgit_dir = xstrdup(gitfile ? gitfile : git_dir);\n \tgit_object_dir = getenv(DB_ENVIRONMENT);\n-\tif (!git_object_dir) {\n-\t\tgit_object_dir = xmalloc(strlen(git_dir) + 9);\n-\t\tsprintf(git_object_dir, \"%s/objects\", git_dir);\n-\t}\n+\tif (!git_object_dir)\n+\t\tgit_object_dir = git_pathdup(\"objects\");\n \tgit_index_file = getenv(INDEX_ENVIRONMENT);\n-\tif (!git_index_file) {\n-\t\tgit_index_file = xmalloc(strlen(git_dir) + 7);\n-\t\tsprintf(git_index_file, \"%s/index\", git_dir);\n-\t}\n+\tif (!git_index_file)\n+\t\tgit_index_file = git_pathdup(\"index\");\n \tgit_graft_file = getenv(GRAFT_ENVIRONMENT);\n \tif (!git_graft_file)\n \t\tgit_graft_file = git_pathdup(\"info/grafts\");\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244719","messageId":"20140619212820.GG28474@sigill.intra.peff.net","threadId":"36955","inReplyTo":"20140619211659.GA32412@sigill.intra.peff.net","subject":"[PATCH v2 07/10] sequencer: use argv_array_pushf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-19T21:28:20Z","receivedAt":"2014-06-19T21:28:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This avoids a manual allocation calculation, and is shorter\nto boot.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sequencer.c | 9 ++-------\n 1 file changed, 2 insertions(+), 7 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 0a80c58..2fea824 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -396,18 +396,13 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n {\n \tstruct argv_array array;\n \tint rc;\n-\tchar *gpg_sign;\n \n \targv_array_init(&array);\n \targv_array_push(&array, \"commit\");\n \targv_array_push(&array, \"-n\");\n \n-\tif (opts->gpg_sign) {\n-\t\tgpg_sign = xmalloc(3 + strlen(opts->gpg_sign));\n-\t\tsprintf(gpg_sign, \"-S%s\", opts->gpg_sign);\n-\t\targv_array_push(&array, gpg_sign);\n-\t\tfree(gpg_sign);\n-\t}\n+\tif (opts->gpg_sign)\n+\t\targv_array_pushf(&array, \"-S%s\", opts->gpg_sign);\n \tif (opts->signoff)\n \t\targv_array_push(&array, \"-s\");\n \tif (!opts->edit) {\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244720","messageId":"20140619212931.GH28474@sigill.intra.peff.net","threadId":"36955","inReplyTo":"20140619211659.GA32412@sigill.intra.peff.net","subject":"[PATCH v2 08/10] merge: use argv_array when spawning merge strategy","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-19T21:29:31Z","receivedAt":"2014-06-19T21:29:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is shorter, and avoids a rather complicated set of\nallocation and free steps.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n merge.c | 42 +++++++++++++-----------------------------\n 1 file changed, 13 insertions(+), 29 deletions(-)\n\ndiff --git a/merge.c b/merge.c\nindex 70f1000..1fa6e52 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -18,39 +18,23 @@ int try_merge_command(const char *strategy, size_t xopts_nr,\n \t\t      const char **xopts, struct commit_list *common,\n \t\t      const char *head_arg, struct commit_list *remotes)\n {\n-\tconst char **args;\n-\tint i = 0, x = 0, ret;\n+\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\tint i, ret;\n \tstruct commit_list *j;\n-\tstruct strbuf buf = STRBUF_INIT;\n \n-\targs = xmalloc((4 + xopts_nr + commit_list_count(common) +\n-\t\t\tcommit_list_count(remotes)) * sizeof(char *));\n-\tstrbuf_addf(&buf, \"merge-%s\", strategy);\n-\targs[i++] = buf.buf;\n-\tfor (x = 0; x < xopts_nr; x++) {\n-\t\tchar *s = xmalloc(strlen(xopts[x])+2+1);\n-\t\tstrcpy(s, \"--\");\n-\t\tstrcpy(s+2, xopts[x]);\n-\t\targs[i++] = s;\n-\t}\n-\tfor (j = common; j; j = j->next)\n-\t\targs[i++] = xstrdup(merge_argument(j->item));\n-\targs[i++] = \"--\";\n-\targs[i++] = head_arg;\n-\tfor (j = remotes; j; j = j->next)\n-\t\targs[i++] = xstrdup(merge_argument(j->item));\n-\targs[i] = NULL;\n-\tret = run_command_v_opt(args, RUN_GIT_CMD);\n-\tstrbuf_release(&buf);\n-\ti = 1;\n-\tfor (x = 0; x < xopts_nr; x++)\n-\t\tfree((void *)args[i++]);\n+\targv_array_pushf(&args, \"merge-%s\", strategy);\n+\tfor (i = 0; i < xopts_nr; i++)\n+\t\targv_array_pushf(&args, \"--%s\", xopts[i]);\n \tfor (j = common; j; j = j->next)\n-\t\tfree((void *)args[i++]);\n-\ti += 2;\n+\t\targv_array_push(&args, merge_argument(j->item));\n+\targv_array_push(&args, \"--\");\n+\targv_array_push(&args, head_arg);\n \tfor (j = remotes; j; j = j->next)\n-\t\tfree((void *)args[i++]);\n-\tfree(args);\n+\t\targv_array_push(&args, merge_argument(j->item));\n+\n+\tret = run_command_v_opt(args.argv, RUN_GIT_CMD);\n+\targv_array_clear(&args);\n+\n \tdiscard_cache();\n \tif (read_cache() < 0)\n \t\tdie(_(\"failed to read the cache\"));\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244721","messageId":"20140619212948.GI28474@sigill.intra.peff.net","threadId":"36955","inReplyTo":"20140619211659.GA32412@sigill.intra.peff.net","subject":"[PATCH v2 09/10] walker_fetch: fix minor memory leak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-19T21:29:48Z","receivedAt":"2014-06-19T21:29:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We sometimes allocate \"msg\" on the heap, but will fail to\nfree it if we hit the failure code path. We can instead keep\na separate variable that is safe to be freed no matter how\nwe get to the failure code path.\n\nWhile we're here, we can also do two readability\nimprovements:\n\n  1. Use xstrfmt instead of a manual malloc/sprintf\n\n  2. Due to the \"maybe we allocate msg, maybe we don't\"\n     strategy, the logic for deciding which message to show\n     was split into two parts. Since the deallocation is now\n     pushed onto a separate variable, this is no longer a\n     concern, and we can keep all of the logic in the same\n     place.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n walker.c | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/walker.c b/walker.c\nindex 1dd86b8..0148264 100644\n--- a/walker.c\n+++ b/walker.c\n@@ -253,7 +253,8 @@ int walker_fetch(struct walker *walker, int targets, char **target,\n {\n \tstruct ref_lock **lock = xcalloc(targets, sizeof(struct ref_lock *));\n \tunsigned char *sha1 = xmalloc(targets * 20);\n-\tchar *msg;\n+\tconst char *msg;\n+\tchar *to_free = NULL;\n \tint ret;\n \tint i;\n \n@@ -285,21 +286,19 @@ int walker_fetch(struct walker *walker, int targets, char **target,\n \tif (loop(walker))\n \t\tgoto unlock_and_fail;\n \n-\tif (write_ref_log_details) {\n-\t\tmsg = xmalloc(strlen(write_ref_log_details) + 12);\n-\t\tsprintf(msg, \"fetch from %s\", write_ref_log_details);\n-\t} else {\n-\t\tmsg = NULL;\n-\t}\n+\tif (write_ref_log_details)\n+\t\tmsg = to_free = xstrfmt(\"fetch from %s\", write_ref_log_details);\n+\telse\n+\t\tmsg = \"fetch (unknown)\";\n \tfor (i = 0; i < targets; i++) {\n \t\tif (!write_ref || !write_ref[i])\n \t\t\tcontinue;\n-\t\tret = write_ref_sha1(lock[i], &sha1[20 * i], msg ? msg : \"fetch (unknown)\");\n+\t\tret = write_ref_sha1(lock[i], &sha1[20 * i], msg);\n \t\tlock[i] = NULL;\n \t\tif (ret)\n \t\t\tgoto unlock_and_fail;\n \t}\n-\tfree(msg);\n+\tfree(to_free);\n \n \treturn 0;\n \n@@ -307,6 +306,7 @@ unlock_and_fail:\n \tfor (i = 0; i < targets; i++)\n \t\tif (lock[i])\n \t\t\tunlock_ref(lock[i]);\n+\tfree(to_free);\n \n \treturn -1;\n }\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244722","messageId":"20140619213026.GJ28474@sigill.intra.peff.net","threadId":"36955","inReplyTo":"20140619211659.GA32412@sigill.intra.peff.net","subject":"[PATCH v2 10/10] unique_path: fix unlikely heap overflow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-19T21:30:26Z","receivedAt":"2014-06-19T21:30:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When merge-recursive creates a unique filename, it uses a\ntemplate like:\n\n  path~branch_%d\n\nwhere the final \"_%d\" is filled by an incrementing counter\nuntil we find a unique name. We allocate 8 characters for\nthe counter, but there is no logic to limit the size of the\ninteger.\n\nOf course, this is extremely unlikely, as you would need a\nhundred million collisions to trigger the problem.  Even if\nan attacker constructed a specialized repo, it is unlikely\nthat the victim would have the patience to run the merge.\n\nHowever, we can make it trivially correct (and hopefully\nmore readable) by using a strbuf.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n merge-recursive.c | 41 ++++++++++++++++++++++++++---------------\n 1 file changed, 26 insertions(+), 15 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 532a1da..398a734 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -601,25 +601,36 @@ static int remove_file(struct merge_options *o, int clean,\n \treturn 0;\n }\n \n+/* add a string to a strbuf, but converting \"/\" to \"_\" */\n+static void add_flattened_path(struct strbuf *out, const char *s)\n+{\n+\tsize_t i = out->len;\n+\tstrbuf_addstr(out, s);\n+\tfor (; i < out->len; i++)\n+\t\tif (out->buf[i] == '/')\n+\t\t\tout->buf[i] = '_';\n+}\n+\n static char *unique_path(struct merge_options *o, const char *path, const char *branch)\n {\n-\tchar *newpath = xmalloc(strlen(path) + 1 + strlen(branch) + 8 + 1);\n+\tstruct strbuf newpath = STRBUF_INIT;\n \tint suffix = 0;\n \tstruct stat st;\n-\tchar *p = newpath + strlen(path);\n-\tstrcpy(newpath, path);\n-\t*(p++) = '~';\n-\tstrcpy(p, branch);\n-\tfor (; *p; ++p)\n-\t\tif ('/' == *p)\n-\t\t\t*p = '_';\n-\twhile (string_list_has_string(&o->current_file_set, newpath) ||\n-\t       string_list_has_string(&o->current_directory_set, newpath) ||\n-\t       lstat(newpath, &st) == 0)\n-\t\tsprintf(p, \"_%d\", suffix++);\n-\n-\tstring_list_insert(&o->current_file_set, newpath);\n-\treturn newpath;\n+\tsize_t base_len;\n+\n+\tstrbuf_addf(&newpath, \"%s~\", path);\n+\tadd_flattened_path(&newpath, branch);\n+\n+\tbase_len = newpath.len;\n+\twhile (string_list_has_string(&o->current_file_set, newpath.buf) ||\n+\t       string_list_has_string(&o->current_directory_set, newpath.buf) ||\n+\t       lstat(newpath.buf, &st) == 0) {\n+\t\tstrbuf_setlen(&newpath, base_len);\n+\t\tstrbuf_addf(&newpath, \"_%d\", suffix++);\n+\t}\n+\n+\tstring_list_insert(&o->current_file_set, newpath.buf);\n+\treturn strbuf_detach(&newpath, NULL);\n }\n \n static int dir_in_way(const char *path, int check_working_copy)\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244858","messageId":"CAPig+cTyZxH6Ka3JVpYBP7=i=HAWoSYBt9MScwfVaTcKKmi1UA@mail.gmail.com","threadId":"36955","inReplyTo":"20140619212800.GF28474@sigill.intra.peff.net","subject":"Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-23T10:21:26Z","receivedAt":"2014-06-23T10:21:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jun 19, 2014 at 5:28 PM, Jeff King <peff@peff.net> wrote:\n> This is shorter, harder to get wrong, and more clearly\n> captures the intent.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I wondered if there was a reason to avoid this (because we are in\n> setup_git_env, which can potentially be called by git_pathdup). But the\n> git_graft_file initialization below already uses it, and I\n> double-checked that it is safe once git_dir is set.\n\nThis patch will conflict textually with patch 6/28 of Duy's\nnd/multiple-work-trees series [1].\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/242300/focus=243649\n\n>  environment.c | 12 ++++--------\n>  1 file changed, 4 insertions(+), 8 deletions(-)\n>\n> diff --git a/environment.c b/environment.c\n> index 4dac5e9..4de7b81 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -135,15 +135,11 @@ static void setup_git_env(void)\n>         gitfile = read_gitfile(git_dir);\n>         git_dir = xstrdup(gitfile ? gitfile : git_dir);\n>         git_object_dir = getenv(DB_ENVIRONMENT);\n> -       if (!git_object_dir) {\n> -               git_object_dir = xmalloc(strlen(git_dir) + 9);\n> -               sprintf(git_object_dir, \"%s/objects\", git_dir);\n> -       }\n> +       if (!git_object_dir)\n> +               git_object_dir = git_pathdup(\"objects\");\n>         git_index_file = getenv(INDEX_ENVIRONMENT);\n> -       if (!git_index_file) {\n> -               git_index_file = xmalloc(strlen(git_dir) + 7);\n> -               sprintf(git_index_file, \"%s/index\", git_dir);\n> -       }\n> +       if (!git_index_file)\n> +               git_index_file = git_pathdup(\"index\");\n>         git_graft_file = getenv(GRAFT_ENVIRONMENT);\n>         if (!git_graft_file)\n>                 git_graft_file = git_pathdup(\"info/grafts\");\n> --\n> 2.0.0.566.gfe3e6b2\n"},{"id":"244899","messageId":"xmqq61jrb62x.fsf@gitster.dls.corp.google.com","threadId":"36955","inReplyTo":"CAPig+cTyZxH6Ka3JVpYBP7=i=HAWoSYBt9MScwfVaTcKKmi1UA@mail.gmail.com","subject":"Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-23T22:43:02Z","receivedAt":"2014-06-23T22:43:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Thu, Jun 19, 2014 at 5:28 PM, Jeff King <peff@peff.net> wrote:\n>> This is shorter, harder to get wrong, and more clearly\n>> captures the intent.\n>>\n>> Signed-off-by: Jeff King <peff@peff.net>\n>> ---\n>> I wondered if there was a reason to avoid this (because we are in\n>> setup_git_env, which can potentially be called by git_pathdup). But the\n>> git_graft_file initialization below already uses it, and I\n>> double-checked that it is safe once git_dir is set.\n>\n> This patch will conflict textually with patch 6/28 of Duy's\n> nd/multiple-work-trees series [1].\n\nThanks; I noticed that and dropped the other topic tentatively, as\nit is being rerolled anyway.  In addition to that, because this\nseries seems fairly focused and well done, and the owners of two\ntopics known to be competent and active folks, I do not think there\nis not much to be worried about ;-).\n\n>\n> [1]: http://thread.gmane.org/gmane.comp.version-control.git/242300/focus=243649\n>\n>>  environment.c | 12 ++++--------\n>>  1 file changed, 4 insertions(+), 8 deletions(-)\n>>\n>> diff --git a/environment.c b/environment.c\n>> index 4dac5e9..4de7b81 100644\n>> --- a/environment.c\n>> +++ b/environment.c\n>> @@ -135,15 +135,11 @@ static void setup_git_env(void)\n>>         gitfile = read_gitfile(git_dir);\n>>         git_dir = xstrdup(gitfile ? gitfile : git_dir);\n>>         git_object_dir = getenv(DB_ENVIRONMENT);\n>> -       if (!git_object_dir) {\n>> -               git_object_dir = xmalloc(strlen(git_dir) + 9);\n>> -               sprintf(git_object_dir, \"%s/objects\", git_dir);\n>> -       }\n>> +       if (!git_object_dir)\n>> +               git_object_dir = git_pathdup(\"objects\");\n>>         git_index_file = getenv(INDEX_ENVIRONMENT);\n>> -       if (!git_index_file) {\n>> -               git_index_file = xmalloc(strlen(git_dir) + 7);\n>> -               sprintf(git_index_file, \"%s/index\", git_dir);\n>> -       }\n>> +       if (!git_index_file)\n>> +               git_index_file = git_pathdup(\"index\");\n>>         git_graft_file = getenv(GRAFT_ENVIRONMENT);\n>>         if (!git_graft_file)\n>>                 git_graft_file = git_pathdup(\"info/grafts\");\n>> --\n>> 2.0.0.566.gfe3e6b2\n"},{"id":"244930","messageId":"CACsJy8ACXPq+-py+vwDFg9nfB6eaC5jy+nXpHn8y6QEe88FuLg@mail.gmail.com","threadId":"36955","inReplyTo":"CAPig+cTyZxH6Ka3JVpYBP7=i=HAWoSYBt9MScwfVaTcKKmi1UA@mail.gmail.com","subject":"Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-06-24T13:02:21Z","receivedAt":"2014-06-24T13:02:21Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jun 23, 2014 at 5:21 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Thu, Jun 19, 2014 at 5:28 PM, Jeff King <peff@peff.net> wrote:\n>> This is shorter, harder to get wrong, and more clearly\n>> captures the intent.\n>>\n>> Signed-off-by: Jeff King <peff@peff.net>\n>> ---\n>> I wondered if there was a reason to avoid this (because we are in\n>> setup_git_env, which can potentially be called by git_pathdup). But the\n>> git_graft_file initialization below already uses it, and I\n>> double-checked that it is safe once git_dir is set.\n>\n> This patch will conflict textually with patch 6/28 of Duy's\n> nd/multiple-work-trees series [1].\n\nI'll just steal the conflicted bit about git_object_dir and put in the\nre-roll. It's a better way anyway. If git_index_file change still\nresults in conflicts, Junio can resolve it easily (I don't touch it in\nnd/multiple-work-trees).\n-- \nDuy\n"},{"id":"244933","messageId":"CACsJy8B-zQUH++U_RKq16_M+6FF5bmHXA100xM3uO42TUj3kJg@mail.gmail.com","threadId":"36955","inReplyTo":"20140619212800.GF28474@sigill.intra.peff.net","subject":"Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-06-24T13:30:26Z","receivedAt":"2014-06-24T13:30:26Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"While it's about malloc..\n\nOn Fri, Jun 20, 2014 at 4:28 AM, Jeff King <peff@peff.net> wrote:\n> diff --git a/environment.c b/environment.c\n> index 4dac5e9..4de7b81 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -135,15 +135,11 @@ static void setup_git_env(void)\n>         gitfile = read_gitfile(git_dir);\n>         git_dir = xstrdup(gitfile ? gitfile : git_dir);\n>         git_object_dir = getenv(DB_ENVIRONMENT);\n> -       if (!git_object_dir) {\n> -               git_object_dir = xmalloc(strlen(git_dir) + 9);\n> -               sprintf(git_object_dir, \"%s/objects\", git_dir);\n> -       }\n\nIf DB_ENVIRONMENT is set, we should xstrdup(git_object_dir) because\ngetenv's return value is not guaranteed persistent. Since you're touch\nthis area, perhaps do it too (in this, or another patch)?\n-- \nDuy\n"},{"id":"244944","messageId":"20140624205815.GA28724@sigill.intra.peff.net","threadId":"36955","inReplyTo":"CACsJy8B-zQUH++U_RKq16_M+6FF5bmHXA100xM3uO42TUj3kJg@mail.gmail.com","subject":"Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-24T20:58:15Z","receivedAt":"2014-06-24T20:58:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 24, 2014 at 08:30:26PM +0700, Duy Nguyen wrote:\n\n> On Fri, Jun 20, 2014 at 4:28 AM, Jeff King <peff@peff.net> wrote:\n> > diff --git a/environment.c b/environment.c\n> > index 4dac5e9..4de7b81 100644\n> > --- a/environment.c\n> > +++ b/environment.c\n> > @@ -135,15 +135,11 @@ static void setup_git_env(void)\n> >         gitfile = read_gitfile(git_dir);\n> >         git_dir = xstrdup(gitfile ? gitfile : git_dir);\n> >         git_object_dir = getenv(DB_ENVIRONMENT);\n> > -       if (!git_object_dir) {\n> > -               git_object_dir = xmalloc(strlen(git_dir) + 9);\n> > -               sprintf(git_object_dir, \"%s/objects\", git_dir);\n> > -       }\n> \n> If DB_ENVIRONMENT is set, we should xstrdup(git_object_dir) because\n> getenv's return value is not guaranteed persistent. Since you're touch\n> this area, perhaps do it too (in this, or another patch)?\n\nHere's a replacement patch that handles this (and just drops the ugly\nmallocs as a side effect).\n\n-- >8 --\nSubject: [PATCH] setup_git_env: copy getenv return value\n\nThe return value of getenv is not guaranteed to survive\nacross further invocations of setenv or even getenv. When we\nare assigning it to globals that last the lifetime of the\nprogram, we should make our own copy.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n environment.c | 22 +++++++++-------------\n 1 file changed, 9 insertions(+), 13 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex 4dac5e9..efb2fa0 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -124,6 +124,12 @@ static char *expand_namespace(const char *raw_namespace)\n \treturn strbuf_detach(&buf, NULL);\n }\n \n+static char *git_path_from_env(const char *envvar, const char *path)\n+{\n+\tconst char *value = getenv(envvar);\n+\treturn value ? xstrdup(value) : git_pathdup(path);\n+}\n+\n static void setup_git_env(void)\n {\n \tconst char *gitfile;\n@@ -134,19 +140,9 @@ static void setup_git_env(void)\n \t\tgit_dir = DEFAULT_GIT_DIR_ENVIRONMENT;\n \tgitfile = read_gitfile(git_dir);\n \tgit_dir = xstrdup(gitfile ? gitfile : git_dir);\n-\tgit_object_dir = getenv(DB_ENVIRONMENT);\n-\tif (!git_object_dir) {\n-\t\tgit_object_dir = xmalloc(strlen(git_dir) + 9);\n-\t\tsprintf(git_object_dir, \"%s/objects\", git_dir);\n-\t}\n-\tgit_index_file = getenv(INDEX_ENVIRONMENT);\n-\tif (!git_index_file) {\n-\t\tgit_index_file = xmalloc(strlen(git_dir) + 7);\n-\t\tsprintf(git_index_file, \"%s/index\", git_dir);\n-\t}\n-\tgit_graft_file = getenv(GRAFT_ENVIRONMENT);\n-\tif (!git_graft_file)\n-\t\tgit_graft_file = git_pathdup(\"info/grafts\");\n+\tgit_object_dir = git_path_from_env(DB_ENVIRONMENT, \"objects\");\n+\tgit_index_file = git_path_from_env(INDEX_ENVIRONMENT, \"index\");\n+\tgit_graft_file = git_path_from_env(GRAFT_ENVIRONMENT, \"info/grafts\");\n \tif (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))\n \t\tcheck_replace_refs = 0;\n \tnamespace = expand_namespace(getenv(GIT_NAMESPACE_ENVIRONMENT));\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244966","messageId":"CACsJy8BT8eNfKy64Bt3rJ642d449MdkKLbTCi0MYNcDRQ0X=ag@mail.gmail.com","threadId":"36955","inReplyTo":"20140624205815.GA28724@sigill.intra.peff.net","subject":"Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-06-25T12:37:55Z","receivedAt":"2014-06-25T12:37:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jun 25, 2014 at 3:58 AM, Jeff King <peff@peff.net> wrote:\n> Here's a replacement patch that handles this (and just drops the ugly\n> mallocs as a side effect).\n\nShortly after I wrote my email, I thought about getenvdup() and look\nfor similar getenv/xstrdup patterns. But I saw only one in config.c.\nSo let's forget about it. Your patch looks good.\n\n>\n> -- >8 --\n> Subject: [PATCH] setup_git_env: copy getenv return value\n>\n> The return value of getenv is not guaranteed to survive\n> across further invocations of setenv or even getenv. When we\n> are assigning it to globals that last the lifetime of the\n> program, we should make our own copy.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  environment.c | 22 +++++++++-------------\n>  1 file changed, 9 insertions(+), 13 deletions(-)\n>\n> diff --git a/environment.c b/environment.c\n> index 4dac5e9..efb2fa0 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -124,6 +124,12 @@ static char *expand_namespace(const char *raw_namespace)\n>         return strbuf_detach(&buf, NULL);\n>  }\n>\n> +static char *git_path_from_env(const char *envvar, const char *path)\n> +{\n> +       const char *value = getenv(envvar);\n> +       return value ? xstrdup(value) : git_pathdup(path);\n> +}\n> +\n>  static void setup_git_env(void)\n>  {\n>         const char *gitfile;\n> @@ -134,19 +140,9 @@ static void setup_git_env(void)\n>                 git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;\n>         gitfile = read_gitfile(git_dir);\n>         git_dir = xstrdup(gitfile ? gitfile : git_dir);\n> -       git_object_dir = getenv(DB_ENVIRONMENT);\n> -       if (!git_object_dir) {\n> -               git_object_dir = xmalloc(strlen(git_dir) + 9);\n> -               sprintf(git_object_dir, \"%s/objects\", git_dir);\n> -       }\n> -       git_index_file = getenv(INDEX_ENVIRONMENT);\n> -       if (!git_index_file) {\n> -               git_index_file = xmalloc(strlen(git_dir) + 7);\n> -               sprintf(git_index_file, \"%s/index\", git_dir);\n> -       }\n> -       git_graft_file = getenv(GRAFT_ENVIRONMENT);\n> -       if (!git_graft_file)\n> -               git_graft_file = git_pathdup(\"info/grafts\");\n> +       git_object_dir = git_path_from_env(DB_ENVIRONMENT, \"objects\");\n> +       git_index_file = git_path_from_env(INDEX_ENVIRONMENT, \"index\");\n> +       git_graft_file = git_path_from_env(GRAFT_ENVIRONMENT, \"info/grafts\");\n>         if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))\n>                 check_replace_refs = 0;\n>         namespace = expand_namespace(getenv(GIT_NAMESPACE_ENVIRONMENT));\n> --\n> 2.0.0.566.gfe3e6b2\n>\n\n\n\n-- \nDuy\n"},{"id":"244971","messageId":"xmqqmwd0aotu.fsf@gitster.dls.corp.google.com","threadId":"36955","inReplyTo":"20140624205815.GA28724@sigill.intra.peff.net","subject":"Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-25T17:20:13Z","receivedAt":"2014-06-25T17:20:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Here's a replacement patch that handles this (and just drops the ugly\n> mallocs as a side effect).\n>\n> -- >8 --\n> Subject: [PATCH] setup_git_env: copy getenv return value\n>\n> The return value of getenv is not guaranteed to survive\n> across further invocations of setenv or even getenv. When we\n> are assigning it to globals that last the lifetime of the\n> program, we should make our own copy.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n\nSigh. This mail unfortunately crossed with 64f25581 (Merge branch 'jk/xstrfmt'\ninto next, 2014-06-23) with about 20 hours of lag.\n\nI'd make it relative like the attached on top of the series.  Note\nthat I tweaked the args to git_pathdup() to avoid the \"are you sure\nyou want to give a variable format string to git_pathdup() which you\nsaid is like printf(3)?\" warning from the compiler.\n\nThanks.\n\n-- >8 --\nFrom: Jeff King <peff@peff.net>\nDate: Tue, 24 Jun 2014 16:58:15 -0400\nSubject: [PATCH] setup_git_env(): introduce git_path_from_env() helper\n\n\"Check the value of an environment and fall back to a known path\ninside $GIT_DIR\" is repeated a few times to determine the location\nof the data store, the index and the graft file, but the return\nvalue of getenv is not guaranteed to survive across further\ninvocations of setenv or even getenv.\n\nMake sure to xstrdup() the value we receive from getenv(3), and\nencapsulate the pattern into a helper function.\n\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n environment.c | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex 4de7b81..565f652 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -124,6 +124,12 @@ static char *expand_namespace(const char *raw_namespace)\n \treturn strbuf_detach(&buf, NULL);\n }\n \n+static char *git_path_from_env(const char *envvar, const char *path)\n+{\n+\tconst char *value = getenv(envvar);\n+\treturn value ? xstrdup(value) : git_pathdup(\"%s\", path);\n+}\n+\n static void setup_git_env(void)\n {\n \tconst char *gitfile;\n@@ -134,15 +140,9 @@ static void setup_git_env(void)\n \t\tgit_dir = DEFAULT_GIT_DIR_ENVIRONMENT;\n \tgitfile = read_gitfile(git_dir);\n \tgit_dir = xstrdup(gitfile ? gitfile : git_dir);\n-\tgit_object_dir = getenv(DB_ENVIRONMENT);\n-\tif (!git_object_dir)\n-\t\tgit_object_dir = git_pathdup(\"objects\");\n-\tgit_index_file = getenv(INDEX_ENVIRONMENT);\n-\tif (!git_index_file)\n-\t\tgit_index_file = git_pathdup(\"index\");\n-\tgit_graft_file = getenv(GRAFT_ENVIRONMENT);\n-\tif (!git_graft_file)\n-\t\tgit_graft_file = git_pathdup(\"info/grafts\");\n+\tgit_object_dir = git_path_from_env(DB_ENVIRONMENT, \"objects\");\n+\tgit_index_file = git_path_from_env(INDEX_ENVIRONMENT, \"index\");\n+\tgit_graft_file = git_path_from_env(GRAFT_ENVIRONMENT, \"info/grafts\");\n \tif (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))\n \t\tcheck_replace_refs = 0;\n \tnamespace = expand_namespace(getenv(GIT_NAMESPACE_ENVIRONMENT));\n-- \n2.0.0-641-g934bf98\n"},{"id":"244972","messageId":"20140625172237.GA15294@sigill.intra.peff.net","threadId":"36955","inReplyTo":"xmqqmwd0aotu.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-25T17:22:38Z","receivedAt":"2014-06-25T17:22:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 25, 2014 at 10:20:13AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Here's a replacement patch that handles this (and just drops the ugly\n> > mallocs as a side effect).\n> >\n> > -- >8 --\n> > Subject: [PATCH] setup_git_env: copy getenv return value\n> >\n> > The return value of getenv is not guaranteed to survive\n> > across further invocations of setenv or even getenv. When we\n> > are assigning it to globals that last the lifetime of the\n> > program, we should make our own copy.\n> >\n> > Signed-off-by: Jeff King <peff@peff.net>\n> > ---\n> \n> Sigh. This mail unfortunately crossed with 64f25581 (Merge branch 'jk/xstrfmt'\n> into next, 2014-06-23) with about 20 hours of lag.\n\nAh, sorry. I had checked yesterday that jk/xstrfmt hadn't been merged\nyet, but I didn't check when responding to Duy.\n\n> I'd make it relative like the attached on top of the series.  Note\n> that I tweaked the args to git_pathdup() to avoid the \"are you sure\n> you want to give a variable format string to git_pathdup() which you\n> said is like printf(3)?\" warning from the compiler.\n\nBoth changes look good to me. Thanks for taking care of it.\n\n-Peff\n"},{"id":"244977","messageId":"xmqqtx789341.fsf@gitster.dls.corp.google.com","threadId":"36955","inReplyTo":"20140625172237.GA15294@sigill.intra.peff.net","subject":"Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-25T19:54:38Z","receivedAt":"2014-06-25T19:54:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Jun 25, 2014 at 10:20:13AM -0700, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> > Here's a replacement patch that handles this (and just drops the ugly\n>> > mallocs as a side effect).\n>> >\n>> > -- >8 --\n>> > Subject: [PATCH] setup_git_env: copy getenv return value\n>> >\n>> > The return value of getenv is not guaranteed to survive\n>> > across further invocations of setenv or even getenv. When we\n>> > are assigning it to globals that last the lifetime of the\n>> > program, we should make our own copy.\n>> >\n>> > Signed-off-by: Jeff King <peff@peff.net>\n>> > ---\n>> \n>> Sigh. This mail unfortunately crossed with 64f25581 (Merge branch 'jk/xstrfmt'\n>> into next, 2014-06-23) with about 20 hours of lag.\n>\n> Ah, sorry. I had checked yesterday that jk/xstrfmt hadn't been merged\n> yet, but I didn't check when responding to Duy.\n\nSorry to have sighed --- crossing e-mails happen all the time.  No\nneed to feel sorry.\n\n>> I'd make it relative like the attached on top of the series.  Note\n>> that I tweaked the args to git_pathdup() to avoid the \"are you sure\n>> you want to give a variable format string to git_pathdup() which you\n>> said is like printf(3)?\" warning from the compiler.\n>\n> Both changes look good to me. Thanks for taking care of it.\n\nThanks.\n"}]}