{"thread":{"id":"45529","subject":"[PATCH 0/18] snprintf cleanups","startedAt":"2017-03-28T19:43:13Z","lastAt":"2017-04-18T04:55:10Z","messageCount":30,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":18},"messages":[{"id":"315595","messageId":"20170328194255.vf7nfzzmmzxsbn36@sigill.intra.peff.net","threadId":"45529","inReplyTo":null,"subject":"[PATCH 0/18] snprintf cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-28T19:42:56Z","receivedAt":"2017-03-28T19:43:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our code base calls snprintf() into a fixed-size buffer in a bunch of\nplaces. Sometimes we check the result, and sometimes we accept a silent\ntruncation. In some cases an overflow is easy given long input. In some\ncases it's impossible. And in some cases it depends on how big PATH_MAX\nis on your filesystem, and whether it's actually enforced. :)\n\nThis series attempts to give more predictable and consistent results by\nremoving arbitrary buffer limitations. It also tries to make further\naudits of snprintf() easier by converting to xsnprintf() where\nappropriate.\n\nThere are still some snprintf() calls left after this. A few are in code\nthat's in flux, or is being cleaned up in nearby series (several of my\nrecent cleanup series were split off from this). A few should probably\nremain (e.g., git-daemon will refuse to consider a repo name larger than\nPATH_MAX, which may be a reasonable defense against weird memory tricks.\nI wouldn't be sad to see this turned into a strbuf with an explicit\nlength policy enforced separately, though). And there were a few that I\njust didn't get around to converting (the dumb-http walker, for example,\nbut I think it may need a pretty involved audit overall).\n\nIt's a lot of patches, but hopefully they're all pretty straightforward\nto read.\n\n  [01/18]: do not check odb_mkstemp return value for errors\n  [02/18]: odb_mkstemp: write filename into strbuf\n  [03/18]: odb_mkstemp: use git_path_buf\n  [04/18]: diff: avoid fixed-size buffer for patch-ids\n  [05/18]: tag: use strbuf to format tag header\n  [06/18]: fetch: use heap buffer to format reflog\n  [07/18]: avoid using fixed PATH_MAX buffers for refs\n  [08/18]: avoid using mksnpath for refs\n  [09/18]: create_branch: move msg setup closer to point of use\n  [10/18]: create_branch: use xstrfmt for reflog message\n  [11/18]: name-rev: replace static buffer with strbuf\n  [12/18]: receive-pack: print --pack-header directly into argv array\n  [13/18]: replace unchecked snprintf calls with heap buffers\n  [14/18]: combine-diff: replace malloc/snprintf with xstrfmt\n  [15/18]: convert unchecked snprintf into xsnprintf\n  [16/18]: transport-helper: replace checked snprintf with xsnprintf\n  [17/18]: gc: replace local buffer with git_path\n  [18/18]: daemon: use an argv_array to exec children\n\n bisect.c               |  8 +++++---\n branch.c               | 16 ++++++++--------\n builtin/checkout.c     |  5 ++---\n builtin/fetch.c        |  6 ++++--\n builtin/gc.c           |  8 +-------\n builtin/index-pack.c   | 22 ++++++++++++----------\n builtin/ls-remote.c    | 10 ++++++----\n builtin/name-rev.c     | 21 ++++++++++++---------\n builtin/notes.c        |  9 ++++-----\n builtin/receive-pack.c | 17 ++++++++++-------\n builtin/replace.c      | 50 +++++++++++++++++++++++++++-----------------------\n builtin/rev-parse.c    |  5 +++--\n builtin/tag.c          | 42 ++++++++++++++++++------------------------\n cache.h                |  7 +++++--\n combine-diff.c         |  7 ++++---\n daemon.c               | 38 +++++++++++++++++---------------------\n diff.c                 | 20 +++++++++++++-------\n environment.c          | 14 ++++++--------\n fast-import.c          |  9 +++++----\n grep.c                 |  4 ++--\n http.c                 | 10 +++++-----\n imap-send.c            |  2 +-\n pack-bitmap-write.c    | 14 +++++++-------\n pack-write.c           | 16 ++++++++--------\n refs.c                 | 44 ++++++++++++++++++++++++++------------------\n sha1_file.c            |  4 ++--\n submodule.c            |  2 +-\n transport-helper.c     |  5 +----\n 28 files changed, 215 insertions(+), 200 deletions(-)\n"},{"id":"315596","messageId":"20170328194524.wmj6tg756fze3ab7@sigill.intra.peff.net","threadId":"45529","inReplyTo":"20170328194255.vf7nfzzmmzxsbn36@sigill.intra.peff.net","subject":"[PATCH 01/18] do not check odb_mkstemp return value for errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-28T19:45:25Z","receivedAt":"2017-03-28T19:45:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The odb_mkstemp function does not return an error; it dies\non failure instead. But many of its callers compare the\nresulting descriptor against -1 and die themselves.\n\nMostly this is just pointless, but it does raise a question\nwhen looking at the callers: if they show the results of the\n\"template\" buffer after a failure, what's in it? The answer\nis: it doesn't matter, because it cannot happen.\n\nSo let's make that clear by removing the bogus error checks.\nIn bitmap_writer_finish(), we can drop the error-handling\ncode entirely. In the other two cases, it's shared with the\nopen() in another code path; we can just move the\nerror-check next to that open() call.\n\nAnd while we're at it, let's flesh out the function's\ndocstring a bit to make the error behavior clear.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/index-pack.c | 7 ++++---\n cache.h              | 5 ++++-\n pack-bitmap-write.c  | 2 --\n pack-write.c         | 4 ++--\n 4 files changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 88d205f85..49e7175d9 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -311,10 +311,11 @@ static const char *open_pack_file(const char *pack_name)\n \t\t\toutput_fd = odb_mkstemp(tmp_file, sizeof(tmp_file),\n \t\t\t\t\t\t\"pack/tmp_pack_XXXXXX\");\n \t\t\tpack_name = xstrdup(tmp_file);\n-\t\t} else\n+\t\t} else {\n \t\t\toutput_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR, 0600);\n-\t\tif (output_fd < 0)\n-\t\t\tdie_errno(_(\"unable to create '%s'\"), pack_name);\n+\t\t\tif (output_fd < 0)\n+\t\t\t\tdie_errno(_(\"unable to create '%s'\"), pack_name);\n+\t\t}\n \t\tnothread_data.pack_fd = output_fd;\n \t} else {\n \t\tinput_fd = open(pack_name, O_RDONLY);\ndiff --git a/cache.h b/cache.h\nindex db4120c23..acad7078d 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1673,7 +1673,10 @@ extern struct packed_git *find_sha1_pack(const unsigned char *sha1,\n extern void pack_report(void);\n \n /*\n- * Create a temporary file rooted in the object database directory.\n+ * Create a temporary file rooted in the object database directory, or\n+ * die on failure. The filename is taken from \"pattern\", which should have the\n+ * usual \"XXXXXX\" trailer, and the resulting filename is written into the\n+ * \"template\" buffer. Returns the open descriptor.\n  */\n extern int odb_mkstemp(char *template, size_t limit, const char *pattern);\n \ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex 970559601..44492c346 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -517,8 +517,6 @@ void bitmap_writer_finish(struct pack_idx_entry **index,\n \n \tint fd = odb_mkstemp(tmp_file, sizeof(tmp_file), \"pack/tmp_bitmap_XXXXXX\");\n \n-\tif (fd < 0)\n-\t\tdie_errno(\"unable to create '%s'\", tmp_file);\n \tf = sha1fd(fd, tmp_file);\n \n \tmemcpy(header.magic, BITMAP_IDX_SIGNATURE, sizeof(BITMAP_IDX_SIGNATURE));\ndiff --git a/pack-write.c b/pack-write.c\nindex 88bc7f9f7..19cb514ea 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -77,9 +77,9 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec\n \t\t} else {\n \t\t\tunlink(index_name);\n \t\t\tfd = open(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);\n+\t\t\tif (fd < 0)\n+\t\t\t\tdie_errno(\"unable to create '%s'\", index_name);\n \t\t}\n-\t\tif (fd < 0)\n-\t\t\tdie_errno(\"unable to create '%s'\", index_name);\n \t\tf = sha1fd(fd, index_name);\n \t}\n \n-- \n2.12.2.845.g55fcf8b10\n\n"},{"id":"315597","messageId":"20170328194543.6e7dhx35ttujm4vv@sigill.intra.peff.net","threadId":"45529","inReplyTo":"20170328194255.vf7nfzzmmzxsbn36@sigill.intra.peff.net","subject":"[PATCH 02/18] odb_mkstemp: write filename into strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-28T19:45:43Z","receivedAt":"2017-03-28T19:45:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The odb_mkstemp() function expects the caller to provide a\nfixed buffer to write the resulting tempfile name into. But\nit creates the template using snprintf without checking the\nreturn value. This means we could silently truncate the\nfilename.\n\nIn practice, it's unlikely that the truncation would end in\nthe template-pattern that mkstemp needs to open the file. So\nwe'd probably end up failing either way, unless the path was\nspecially crafted.\n\nThe simplest fix would be to notice the truncation and die.\nHowever, we can observe that most callers immediately\nxstrdup() the result anyway. So instead, let's switch to\nusing a strbuf, which is easier for them (and isn't a big\ndeal for the other 2 callers, who can just strbuf_release\nwhen they're done with it).\n\nNote that many of the callers used static buffers, but this\nwas purely to avoid putting a large buffer on the stack. We\nnever passed the static buffers out of the function, so\nthere's no complicated memory handling we need to change.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/index-pack.c |  6 +++---\n cache.h              |  2 +-\n environment.c        | 16 ++++++++--------\n fast-import.c        |  9 +++++----\n pack-bitmap-write.c  | 12 +++++++-----\n pack-write.c         | 12 ++++++------\n 6 files changed, 30 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 49e7175d9..f4af2ab37 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -307,10 +307,10 @@ static const char *open_pack_file(const char *pack_name)\n \tif (from_stdin) {\n \t\tinput_fd = 0;\n \t\tif (!pack_name) {\n-\t\t\tstatic char tmp_file[PATH_MAX];\n-\t\t\toutput_fd = odb_mkstemp(tmp_file, sizeof(tmp_file),\n+\t\t\tstruct strbuf tmp_file = STRBUF_INIT;\n+\t\t\toutput_fd = odb_mkstemp(&tmp_file,\n \t\t\t\t\t\t\"pack/tmp_pack_XXXXXX\");\n-\t\t\tpack_name = xstrdup(tmp_file);\n+\t\t\tpack_name = strbuf_detach(&tmp_file, NULL);\n \t\t} else {\n \t\t\toutput_fd = open(pack_name, O_CREAT|O_EXCL|O_RDWR, 0600);\n \t\t\tif (output_fd < 0)\ndiff --git a/cache.h b/cache.h\nindex acad7078d..1c1889428 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1678,7 +1678,7 @@ extern void pack_report(void);\n  * usual \"XXXXXX\" trailer, and the resulting filename is written into the\n  * \"template\" buffer. Returns the open descriptor.\n  */\n-extern int odb_mkstemp(char *template, size_t limit, const char *pattern);\n+extern int odb_mkstemp(struct strbuf *template, const char *pattern);\n \n /*\n  * Generate the filename to be used for a pack file with checksum \"sha1\" and\ndiff --git a/environment.c b/environment.c\nindex 2fdba7622..88276790d 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -274,7 +274,7 @@ char *get_object_directory(void)\n \treturn git_object_dir;\n }\n \n-int odb_mkstemp(char *template, size_t limit, const char *pattern)\n+int odb_mkstemp(struct strbuf *template, const char *pattern)\n {\n \tint fd;\n \t/*\n@@ -282,18 +282,18 @@ int odb_mkstemp(char *template, size_t limit, const char *pattern)\n \t * restrictive except to remove write permission.\n \t */\n \tint mode = 0444;\n-\tsnprintf(template, limit, \"%s/%s\",\n-\t\t get_object_directory(), pattern);\n-\tfd = git_mkstemp_mode(template, mode);\n+\tstrbuf_reset(template);\n+\tstrbuf_addf(template, \"%s/%s\", get_object_directory(), pattern);\n+\tfd = git_mkstemp_mode(template->buf, mode);\n \tif (0 <= fd)\n \t\treturn fd;\n \n \t/* slow path */\n \t/* some mkstemp implementations erase template on failure */\n-\tsnprintf(template, limit, \"%s/%s\",\n-\t\t get_object_directory(), pattern);\n-\tsafe_create_leading_directories(template);\n-\treturn xmkstemp_mode(template, mode);\n+\tstrbuf_reset(template);\n+\tstrbuf_addf(template, \"%s/%s\", get_object_directory(), pattern);\n+\tsafe_create_leading_directories(template->buf);\n+\treturn xmkstemp_mode(template->buf, mode);\n }\n \n int odb_pack_keep(const char *name)\ndiff --git a/fast-import.c b/fast-import.c\nindex 41a539f97..900b9626f 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -890,14 +890,15 @@ static struct tree_content *dup_tree_content(struct tree_content *s)\n \n static void start_packfile(void)\n {\n-\tstatic char tmp_file[PATH_MAX];\n+\tstruct strbuf tmp_file = STRBUF_INIT;\n \tstruct packed_git *p;\n \tstruct pack_header hdr;\n \tint pack_fd;\n \n-\tpack_fd = odb_mkstemp(tmp_file, sizeof(tmp_file),\n-\t\t\t      \"pack/tmp_pack_XXXXXX\");\n-\tFLEX_ALLOC_STR(p, pack_name, tmp_file);\n+\tpack_fd = odb_mkstemp(&tmp_file, \"pack/tmp_pack_XXXXXX\");\n+\tFLEX_ALLOC_STR(p, pack_name, tmp_file.buf);\n+\tstrbuf_release(&tmp_file);\n+\n \tp->pack_fd = pack_fd;\n \tp->do_not_close = 1;\n \tpack_file = sha1fd(pack_fd, p->pack_name);\ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex 44492c346..e313f4f2b 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -508,16 +508,16 @@ void bitmap_writer_finish(struct pack_idx_entry **index,\n \t\t\t  const char *filename,\n \t\t\t  uint16_t options)\n {\n-\tstatic char tmp_file[PATH_MAX];\n \tstatic uint16_t default_version = 1;\n \tstatic uint16_t flags = BITMAP_OPT_FULL_DAG;\n+\tstruct strbuf tmp_file = STRBUF_INIT;\n \tstruct sha1file *f;\n \n \tstruct bitmap_disk_header header;\n \n-\tint fd = odb_mkstemp(tmp_file, sizeof(tmp_file), \"pack/tmp_bitmap_XXXXXX\");\n+\tint fd = odb_mkstemp(&tmp_file, \"pack/tmp_bitmap_XXXXXX\");\n \n-\tf = sha1fd(fd, tmp_file);\n+\tf = sha1fd(fd, tmp_file.buf);\n \n \tmemcpy(header.magic, BITMAP_IDX_SIGNATURE, sizeof(BITMAP_IDX_SIGNATURE));\n \theader.version = htons(default_version);\n@@ -537,9 +537,11 @@ void bitmap_writer_finish(struct pack_idx_entry **index,\n \n \tsha1close(f, NULL, CSUM_FSYNC);\n \n-\tif (adjust_shared_perm(tmp_file))\n+\tif (adjust_shared_perm(tmp_file.buf))\n \t\tdie_errno(\"unable to make temporary bitmap file readable\");\n \n-\tif (rename(tmp_file, filename))\n+\tif (rename(tmp_file.buf, filename))\n \t\tdie_errno(\"unable to rename temporary bitmap file to '%s'\", filename);\n+\n+\tstrbuf_release(&tmp_file);\n }\ndiff --git a/pack-write.c b/pack-write.c\nindex 19cb514ea..0c56ce98d 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -71,9 +71,9 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec\n \t\tf = sha1fd_check(index_name);\n \t} else {\n \t\tif (!index_name) {\n-\t\t\tstatic char tmp_file[PATH_MAX];\n-\t\t\tfd = odb_mkstemp(tmp_file, sizeof(tmp_file), \"pack/tmp_idx_XXXXXX\");\n-\t\t\tindex_name = xstrdup(tmp_file);\n+\t\t\tstruct strbuf tmp_file = STRBUF_INIT;\n+\t\t\tfd = odb_mkstemp(&tmp_file, \"pack/tmp_idx_XXXXXX\");\n+\t\t\tindex_name = strbuf_detach(&tmp_file, NULL);\n \t\t} else {\n \t\t\tunlink(index_name);\n \t\t\tfd = open(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);\n@@ -326,11 +326,11 @@ int encode_in_pack_object_header(enum object_type type, uintmax_t size, unsigned\n \n struct sha1file *create_tmp_packfile(char **pack_tmp_name)\n {\n-\tchar tmpname[PATH_MAX];\n+\tstruct strbuf tmpname = STRBUF_INIT;\n \tint fd;\n \n-\tfd = odb_mkstemp(tmpname, sizeof(tmpname), \"pack/tmp_pack_XXXXXX\");\n-\t*pack_tmp_name = xstrdup(tmpname);\n+\tfd = odb_mkstemp(&tmpname, \"pack/tmp_pack_XXXXXX\");\n+\t*pack_tmp_name = strbuf_detach(&tmpname, NULL);\n \treturn sha1fd(fd, *pack_tmp_name);\n }\n \n-- \n2.12.2.845.g55fcf8b10\n\n"},{"id":"315598","messageId":"20170328194551.w2y34j4kc2qd45vy@sigill.intra.peff.net","threadId":"45529","inReplyTo":"20170328194255.vf7nfzzmmzxsbn36@sigill.intra.peff.net","subject":"[PATCH 03/18] odb_mkstemp: use git_path_buf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-28T19:45:52Z","receivedAt":"2017-03-28T19:46:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since git_path_buf() is smart enough to replace \"objects/\"\nwith the correct object path, we can use it instead of\nmanually assembling the path. That's slightly shorter, and\nwill clean up any non-canonical bits in the path.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n environment.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex 88276790d..a9bf5658a 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -282,16 +282,14 @@ int odb_mkstemp(struct strbuf *template, const char *pattern)\n \t * restrictive except to remove write permission.\n \t */\n \tint mode = 0444;\n-\tstrbuf_reset(template);\n-\tstrbuf_addf(template, \"%s/%s\", get_object_directory(), pattern);\n+\tgit_path_buf(template, \"objects/%s\", pattern);\n \tfd = git_mkstemp_mode(template->buf, mode);\n \tif (0 <= fd)\n \t\treturn fd;\n \n \t/* slow path */\n \t/* some mkstemp implementations erase template on failure */\n-\tstrbuf_reset(template);\n-\tstrbuf_addf(template, \"%s/%s\", get_object_directory(), pattern);\n+\tgit_path_buf(template, \"objects/%s\", pattern);\n \tsafe_create_leading_directories(template->buf);\n \treturn xmkstemp_mode(template->buf, mode);\n }\n-- \n2.12.2.845.g55fcf8b10\n\n"},{"id":"315599","messageId":"20170328194623.sv2sih2s7q533zgf@sigill.intra.peff.net","threadId":"45529","inReplyTo":"20170328194255.vf7nfzzmmzxsbn36@sigill.intra.peff.net","subject":"[PATCH 05/18] tag: use strbuf to format tag header","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-28T19:46:23Z","receivedAt":"2017-03-28T19:47:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We format the tag header into a fixed 1024-byte buffer. But\nsince the tag-name and tagger ident can be arbitrarily\nlarge, we may unceremoniously die with \"tag header too big\".\nLet's just use a strbuf instead.\n\nNote that it looks at first glance like we can just format\nthis directly into the \"buf\" strbuf where it will ultimately\ngo. But that buffer may already contain the tag message, and\nwe have no easy way to prepend formatted data to a strbuf\n(we can only splice in an already-generated buffer). This\nisn't a performance-critical path, so going through an extra\nbuffer isn't a big deal.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/tag.c | 27 ++++++++++++---------------\n 1 file changed, 12 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex ad29be692..045e7ad23 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -231,26 +231,22 @@ static void create_tag(const unsigned char *object, const char *tag,\n \t\t       unsigned char *prev, unsigned char *result)\n {\n \tenum object_type type;\n-\tchar header_buf[1024];\n-\tint header_len;\n+\tstruct strbuf header = STRBUF_INIT;\n \tchar *path = NULL;\n \n \ttype = sha1_object_info(object, NULL);\n \tif (type <= OBJ_NONE)\n \t    die(_(\"bad object type.\"));\n \n-\theader_len = snprintf(header_buf, sizeof(header_buf),\n-\t\t\t  \"object %s\\n\"\n-\t\t\t  \"type %s\\n\"\n-\t\t\t  \"tag %s\\n\"\n-\t\t\t  \"tagger %s\\n\\n\",\n-\t\t\t  sha1_to_hex(object),\n-\t\t\t  typename(type),\n-\t\t\t  tag,\n-\t\t\t  git_committer_info(IDENT_STRICT));\n-\n-\tif (header_len > sizeof(header_buf) - 1)\n-\t\tdie(_(\"tag header too big.\"));\n+\tstrbuf_addf(&header,\n+\t\t    \"object %s\\n\"\n+\t\t    \"type %s\\n\"\n+\t\t    \"tag %s\\n\"\n+\t\t    \"tagger %s\\n\\n\",\n+\t\t    sha1_to_hex(object),\n+\t\t    typename(type),\n+\t\t    tag,\n+\t\t    git_committer_info(IDENT_STRICT));\n \n \tif (!opt->message_given) {\n \t\tint fd;\n@@ -288,7 +284,8 @@ static void create_tag(const unsigned char *object, const char *tag,\n \tif (!opt->message_given && !buf->len)\n \t\tdie(_(\"no tag message?\"));\n \n-\tstrbuf_insert(buf, 0, header_buf, header_len);\n+\tstrbuf_insert(buf, 0, header.buf, header.len);\n+\tstrbuf_release(&header);\n \n \tif (build_tag_object(buf, opt->sign, result) < 0) {\n \t\tif (path)\n-- \n2.12.2.845.g55fcf8b10\n\n"},{"id":"315600","messageId":"20170328194636.fak7p7cvpk3zscgz@sigill.intra.peff.net","threadId":"45529","inReplyTo":"20170328194255.vf7nfzzmmzxsbn36@sigill.intra.peff.net","subject":"[PATCH 09/18] create_branch: move msg setup closer to point of use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-28T19:46:36Z","receivedAt":"2017-03-28T19:47:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In create_branch() we write the reflog msg into a buffer in\nthe main function, but then use it only inside a\nconditional. If you carefully follow the logic, you can\nconfirm that we never use the buffer uninitialized nor write\nwhen it would not be used. But we can make this a lot more\nobvious by simply moving the write step inside the\nconditional.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n branch.c | 17 +++++++++--------\n 1 file changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 5c12036b0..6d0ca94cc 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -234,7 +234,7 @@ void create_branch(const char *name, const char *start_name,\n {\n \tstruct commit *commit;\n \tunsigned char sha1[20];\n-\tchar *real_ref, msg[PATH_MAX + 20];\n+\tchar *real_ref;\n \tstruct strbuf ref = STRBUF_INIT;\n \tint forcing = 0;\n \tint dont_change_ref = 0;\n@@ -290,19 +290,20 @@ void create_branch(const char *name, const char *start_name,\n \t\tdie(_(\"Not a valid branch point: '%s'.\"), start_name);\n \thashcpy(sha1, commit->object.oid.hash);\n \n-\tif (forcing)\n-\t\tsnprintf(msg, sizeof msg, \"branch: Reset to %s\",\n-\t\t\t start_name);\n-\telse if (!dont_change_ref)\n-\t\tsnprintf(msg, sizeof msg, \"branch: Created from %s\",\n-\t\t\t start_name);\n-\n \tif (reflog)\n \t\tlog_all_ref_updates = LOG_REFS_NORMAL;\n \n \tif (!dont_change_ref) {\n \t\tstruct ref_transaction *transaction;\n \t\tstruct strbuf err = STRBUF_INIT;\n+\t\tchar msg[PATH_MAX + 20];\n+\n+\t\tif (forcing)\n+\t\t\tsnprintf(msg, sizeof msg, \"branch: Reset to %s\",\n+\t\t\t\t start_name);\n+\t\telse\n+\t\t\tsnprintf(msg, sizeof msg, \"branch: Created from %s\",\n+\t\t\t\t start_name);\n \n \t\ttransaction = ref_transaction_begin(&err);\n \t\tif (!transaction ||\n-- \n2.12.2.845.g55fcf8b10\n\n"},{"id":"315601","messageId":"20170328194633.ok3tamreao5u7gjp@sigill.intra.peff.net","threadId":"45529","inReplyTo":"20170328194255.vf7nfzzmmzxsbn36@sigill.intra.peff.net","subject":"[PATCH 08/18] avoid using mksnpath for refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-28T19:46:33Z","receivedAt":"2017-03-28T19:47:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Like the previous commit, we'd like to avoid the assumption\nthat refs fit into PATH_MAX-sized buffers. These callsites\nhave an extra twist, though: they write the refnames using\nmksnpath. This does two things beyond a regular snprintf:\n\n  1. It quietly writes \"/bad-path/\" when truncation occurs.\n     This saves the caller having to check the error code,\n     but if you aren't actually feeding the result to a\n     system call (and we aren't here), it's questionable.\n\n  2. It calls cleanup_path(), which removes leading\n     instances of \"./\".  That's questionable when dealing\n     with refnames, as we could silently canonicalize a\n     syntactically bogus refname into a valid one.\n\nLet's convert each case to use a strbuf. This is preferable\nto xstrfmt() because we can reuse the same buffer as we\nloop.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs.c | 44 ++++++++++++++++++++++++++------------------\n 1 file changed, 26 insertions(+), 18 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex e7606716d..9de6979ac 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -429,29 +429,31 @@ int expand_ref(const char *str, int len, unsigned char *sha1, char **ref)\n {\n \tconst char **p, *r;\n \tint refs_found = 0;\n+\tstruct strbuf fullref = STRBUF_INIT;\n \n \t*ref = NULL;\n \tfor (p = ref_rev_parse_rules; *p; p++) {\n-\t\tchar fullref[PATH_MAX];\n \t\tunsigned char sha1_from_ref[20];\n \t\tunsigned char *this_result;\n \t\tint flag;\n \n \t\tthis_result = refs_found ? sha1_from_ref : sha1;\n-\t\tmksnpath(fullref, sizeof(fullref), *p, len, str);\n-\t\tr = resolve_ref_unsafe(fullref, RESOLVE_REF_READING,\n+\t\tstrbuf_reset(&fullref);\n+\t\tstrbuf_addf(&fullref, *p, len, str);\n+\t\tr = resolve_ref_unsafe(fullref.buf, RESOLVE_REF_READING,\n \t\t\t\t       this_result, &flag);\n \t\tif (r) {\n \t\t\tif (!refs_found++)\n \t\t\t\t*ref = xstrdup(r);\n \t\t\tif (!warn_ambiguous_refs)\n \t\t\t\tbreak;\n-\t\t} else if ((flag & REF_ISSYMREF) && strcmp(fullref, \"HEAD\")) {\n-\t\t\twarning(\"ignoring dangling symref %s.\", fullref);\n-\t\t} else if ((flag & REF_ISBROKEN) && strchr(fullref, '/')) {\n-\t\t\twarning(\"ignoring broken ref %s.\", fullref);\n+\t\t} else if ((flag & REF_ISSYMREF) && strcmp(fullref.buf, \"HEAD\")) {\n+\t\t\twarning(\"ignoring dangling symref %s.\", fullref.buf);\n+\t\t} else if ((flag & REF_ISBROKEN) && strchr(fullref.buf, '/')) {\n+\t\t\twarning(\"ignoring broken ref %s.\", fullref.buf);\n \t\t}\n \t}\n+\tstrbuf_release(&fullref);\n \treturn refs_found;\n }\n \n@@ -460,21 +462,22 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n \tchar *last_branch = substitute_branch_name(&str, &len);\n \tconst char **p;\n \tint logs_found = 0;\n+\tstruct strbuf path = STRBUF_INIT;\n \n \t*log = NULL;\n \tfor (p = ref_rev_parse_rules; *p; p++) {\n \t\tunsigned char hash[20];\n-\t\tchar path[PATH_MAX];\n \t\tconst char *ref, *it;\n \n-\t\tmksnpath(path, sizeof(path), *p, len, str);\n-\t\tref = resolve_ref_unsafe(path, RESOLVE_REF_READING,\n+\t\tstrbuf_reset(&path);\n+\t\tstrbuf_addf(&path, *p, len, str);\n+\t\tref = resolve_ref_unsafe(path.buf, RESOLVE_REF_READING,\n \t\t\t\t\t hash, NULL);\n \t\tif (!ref)\n \t\t\tcontinue;\n-\t\tif (reflog_exists(path))\n-\t\t\tit = path;\n-\t\telse if (strcmp(ref, path) && reflog_exists(ref))\n+\t\tif (reflog_exists(path.buf))\n+\t\t\tit = path.buf;\n+\t\telse if (strcmp(ref, path.buf) && reflog_exists(ref))\n \t\t\tit = ref;\n \t\telse\n \t\t\tcontinue;\n@@ -485,6 +488,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n \t\tif (!warn_ambiguous_refs)\n \t\t\tbreak;\n \t}\n+\tstrbuf_release(&path);\n \tfree(last_branch);\n \treturn logs_found;\n }\n@@ -944,6 +948,7 @@ char *shorten_unambiguous_ref(const char *refname, int strict)\n \tstatic char **scanf_fmts;\n \tstatic int nr_rules;\n \tchar *short_name;\n+\tstruct strbuf resolved_buf = STRBUF_INIT;\n \n \tif (!nr_rules) {\n \t\t/*\n@@ -1002,7 +1007,6 @@ char *shorten_unambiguous_ref(const char *refname, int strict)\n \t\t */\n \t\tfor (j = 0; j < rules_to_fail; j++) {\n \t\t\tconst char *rule = ref_rev_parse_rules[j];\n-\t\t\tchar refname[PATH_MAX];\n \n \t\t\t/* skip matched rule */\n \t\t\tif (i == j)\n@@ -1013,9 +1017,10 @@ char *shorten_unambiguous_ref(const char *refname, int strict)\n \t\t\t * (with this previous rule) to a valid ref\n \t\t\t * read_ref() returns 0 on success\n \t\t\t */\n-\t\t\tmksnpath(refname, sizeof(refname),\n-\t\t\t\t rule, short_name_len, short_name);\n-\t\t\tif (ref_exists(refname))\n+\t\t\tstrbuf_reset(&resolved_buf);\n+\t\t\tstrbuf_addf(&resolved_buf, rule,\n+\t\t\t\t    short_name_len, short_name);\n+\t\t\tif (ref_exists(resolved_buf.buf))\n \t\t\t\tbreak;\n \t\t}\n \n@@ -1023,10 +1028,13 @@ char *shorten_unambiguous_ref(const char *refname, int strict)\n \t\t * short name is non-ambiguous if all previous rules\n \t\t * haven't resolved to a valid ref\n \t\t */\n-\t\tif (j == rules_to_fail)\n+\t\tif (j == rules_to_fail) {\n+\t\t\tstrbuf_release(&resolved_buf);\n \t\t\treturn short_name;\n+\t\t}\n \t}\n \n+\tstrbuf_release(&resolved_buf);\n \tfree(short_name);\n \treturn xstrdup(refname);\n }\n-- \n2.12.2.845.g55fcf8b10\n\n"},{"id":"315602","messageId":"20170328194626.umvxdr6s6z3anzrj@sigill.intra.peff.net","threadId":"45529","inReplyTo":"20170328194255.vf7nfzzmmzxsbn36@sigill.intra.peff.net","subject":"[PATCH 06/18] fetch: use heap buffer to format reflog","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-28T19:46:26Z","receivedAt":"2017-03-28T19:47:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Part of the reflog content comes from the environment, which\ncan be much larger than our fixed buffer. Let's use a heap\nbuffer so we avoid truncating it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/fetch.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b5ad09d04..4ef7a08af 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -421,7 +421,7 @@ static int s_update_ref(const char *action,\n \t\t\tstruct ref *ref,\n \t\t\tint check_old)\n {\n-\tchar msg[1024];\n+\tchar *msg;\n \tchar *rla = getenv(\"GIT_REFLOG_ACTION\");\n \tstruct ref_transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n@@ -431,7 +431,7 @@ static int s_update_ref(const char *action,\n \t\treturn 0;\n \tif (!rla)\n \t\trla = default_rla.buf;\n-\tsnprintf(msg, sizeof(msg), \"%s: %s\", rla, action);\n+\tmsg = xstrfmt(\"%s: %s\", rla, action);\n \n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n@@ -449,11 +449,13 @@ static int s_update_ref(const char *action,\n \n \tref_transaction_free(transaction);\n \tstrbuf_release(&err);\n+\tfree(msg);\n \treturn 0;\n fail:\n \tref_transaction_free(transaction);\n \terror(\"%s\", err.buf);\n \tstrbuf_release(&err);\n+\tfree(msg);\n \treturn df_conflict ? STORE_REF_ERROR_DF_CONFLICT\n \t\t\t   : STORE_REF_ERROR_OTHER;\n }\n-- \n2.12.2.845.g55fcf8b10\n\n"},{"id":"315603","messageId":"20170328194618.injneju2d2fkghmk@sigill.intra.peff.net","threadId":"45529","inReplyTo":"20170328194255.vf7nfzzmmzxsbn36@sigill.intra.peff.net","subject":"[PATCH 04/18] diff: avoid fixed-size buffer for patch-ids","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-28T19:46:18Z","receivedAt":"2017-03-28T19:47:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"To generate a patch id, we format the diff header into a\nfixed-size buffer, and then feed the result to our sha1\ncomputation. The fixed buffer has size '4*PATH_MAX + 20',\nwhich in theory accomodates the four filenames plus some\nextra data. Except:\n\n  1. The filenames may not be constrained to PATH_MAX. The\n     static value may not be a real limit on the current\n     filesystem. Moreover, we may compute patch-ids for\n     names stored only in git, without touching the current\n     filesystem at all.\n\n  2. The 20 bytes is not nearly enough to cover the\n     extra content we put in the buffer.\n\nAs a result, the data we feed to the sha1 computation may be\ntruncated, and it's possible that a commit with a very long\nfilename could erroneously collide in the patch-id space\nwith another commit. For instance, if one commit modified\n\"really-long-filename/foo\" and another modified \"bar\" in the\nsame directory.\n\nIn practice this is unlikely. Because the filenames are\nrepeated, and because there's a single cutoff at the end of\nthe buffer, the offending filename would have to be on the\norder of four times larger than PATH_MAX.\n\nBut it's easy to fix by moving to a strbuf.\n\nTechnically this may change the output of patch-id for very\nlong filenames, but it's not worth making an exception for\nthis in the --stable output. It was a bug, and one that only\naffected an unlikely set of paths.  And anyway, the exact\nvalue would have varied from platform to platform depending\non the value of PATH_MAX, so there is no \"stable\" value.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c | 20 +++++++++++++-------\n 1 file changed, 13 insertions(+), 7 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 58cb72d7e..89b5dc890 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4577,7 +4577,7 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1,\n \tint i;\n \tgit_SHA_CTX ctx;\n \tstruct patch_id_t data;\n-\tchar buffer[PATH_MAX * 4 + 20];\n+\tstruct strbuf buffer = STRBUF_INIT;\n \n \tgit_SHA1_Init(&ctx);\n \tmemset(&data, 0, sizeof(struct patch_id_t));\n@@ -4607,10 +4607,11 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1,\n \t\tdiff_fill_sha1_info(p->one);\n \t\tdiff_fill_sha1_info(p->two);\n \n+\t\tstrbuf_reset(&buffer);\n \t\tlen1 = remove_space(p->one->path, strlen(p->one->path));\n \t\tlen2 = remove_space(p->two->path, strlen(p->two->path));\n \t\tif (p->one->mode == 0)\n-\t\t\tlen1 = snprintf(buffer, sizeof(buffer),\n+\t\t\tstrbuf_addf(&buffer,\n \t\t\t\t\t\"diff--gita/%.*sb/%.*s\"\n \t\t\t\t\t\"newfilemode%06o\"\n \t\t\t\t\t\"---/dev/null\"\n@@ -4620,7 +4621,7 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1,\n \t\t\t\t\tp->two->mode,\n \t\t\t\t\tlen2, p->two->path);\n \t\telse if (p->two->mode == 0)\n-\t\t\tlen1 = snprintf(buffer, sizeof(buffer),\n+\t\t\tstrbuf_addf(&buffer,\n \t\t\t\t\t\"diff--gita/%.*sb/%.*s\"\n \t\t\t\t\t\"deletedfilemode%06o\"\n \t\t\t\t\t\"---a/%.*s\"\n@@ -4630,7 +4631,7 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1,\n \t\t\t\t\tp->one->mode,\n \t\t\t\t\tlen1, p->one->path);\n \t\telse\n-\t\t\tlen1 = snprintf(buffer, sizeof(buffer),\n+\t\t\tstrbuf_addf(&buffer,\n \t\t\t\t\t\"diff--gita/%.*sb/%.*s\"\n \t\t\t\t\t\"---a/%.*s\"\n \t\t\t\t\t\"+++b/%.*s\",\n@@ -4638,14 +4639,16 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1,\n \t\t\t\t\tlen2, p->two->path,\n \t\t\t\t\tlen1, p->one->path,\n \t\t\t\t\tlen2, p->two->path);\n-\t\tgit_SHA1_Update(&ctx, buffer, len1);\n+\t\tgit_SHA1_Update(&ctx, buffer.buf, buffer.len);\n \n \t\tif (diff_header_only)\n \t\t\tcontinue;\n \n \t\tif (fill_mmfile(&mf1, p->one) < 0 ||\n-\t\t    fill_mmfile(&mf2, p->two) < 0)\n+\t\t    fill_mmfile(&mf2, p->two) < 0) {\n+\t\t\tstrbuf_release(&buffer);\n \t\t\treturn error(\"unable to read files to diff\");\n+\t\t}\n \n \t\tif (diff_filespec_is_binary(p->one) ||\n \t\t    diff_filespec_is_binary(p->two)) {\n@@ -4660,11 +4663,14 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1,\n \t\txecfg.ctxlen = 3;\n \t\txecfg.flags = 0;\n \t\tif (xdi_diff_outf(&mf1, &mf2, patch_id_consume, &data,\n-\t\t\t\t  &xpp, &xecfg))\n+\t\t\t\t  &xpp, &xecfg)) {\n+\t\t\tstrbuf_release(&buffer);\n \t\t\treturn error(\"unable to generate patch-id diff for %s\",\n \t\t\t\t     p->one->path);\n+\t\t}\n \t}\n \n+\tstrbuf_release(&buffer);\n \tgit_SHA1_Final(sha1, &ctx);\n \treturn 0;\n }\n-- \n2.12.2.845.g55fcf8b10\n\n"},{"id":"315604","messageId":"20170328194650.zr3ee3yjqpnxxzlg@sigill.intra.peff.net","threadId":"45529","inReplyTo":"20170328194255.vf7nfzzmmzxsbn36@sigill.intra.peff.net","subject":"[PATCH 13/18] replace unchecked snprintf calls with heap buffers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-28T19:46:50Z","receivedAt":"2017-03-28T19:47:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We'd prefer to avoid unchecked snprintf calls because\ntruncation can lead to unexpected results.\n\nThese are all cases where truncation shouldn't ever happen,\nbecause the input to snprintf is fixed in size. That makes\nthem candidates for xsnprintf(), but it's simpler still to\njust use the heap, and then nobody has to wonder if \"100\" is\nbig enough.\n\nWe'll use xstrfmt() where possible, and a strbuf when we need\nthe resulting size or to reuse the same buffer in a loop.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n bisect.c             | 8 +++++---\n builtin/index-pack.c | 9 +++++----\n builtin/notes.c      | 9 ++++-----\n builtin/rev-parse.c  | 5 +++--\n 4 files changed, 17 insertions(+), 14 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 30808cadf..d12583eaa 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -200,6 +200,7 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n\n {\n \tstruct commit_list *p;\n \tstruct commit_dist *array = xcalloc(nr, sizeof(*array));\n+\tstruct strbuf buf = STRBUF_INIT;\n \tint cnt, i;\n \n \tfor (p = list, cnt = 0; p; p = p->next) {\n@@ -217,17 +218,18 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n\n \t}\n \tQSORT(array, cnt, compare_commit_dist);\n \tfor (p = list, i = 0; i < cnt; i++) {\n-\t\tchar buf[100]; /* enough for dist=%d */\n \t\tstruct object *obj = &(array[i].commit->object);\n \n-\t\tsnprintf(buf, sizeof(buf), \"dist=%d\", array[i].distance);\n-\t\tadd_name_decoration(DECORATION_NONE, buf, obj);\n+\t\tstrbuf_reset(&buf);\n+\t\tstrbuf_addf(&buf, \"dist=%d\", array[i].distance);\n+\t\tadd_name_decoration(DECORATION_NONE, buf.buf, obj);\n \n \t\tp->item = array[i].commit;\n \t\tp = p->next;\n \t}\n \tif (p)\n \t\tp->next = NULL;\n+\tstrbuf_release(&buf);\n \tfree(array);\n \treturn list;\n }\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex f4af2ab37..197c51912 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1443,10 +1443,11 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \tif (!from_stdin) {\n \t\tprintf(\"%s\\n\", sha1_to_hex(sha1));\n \t} else {\n-\t\tchar buf[48];\n-\t\tint len = snprintf(buf, sizeof(buf), \"%s\\t%s\\n\",\n-\t\t\t\t   report, sha1_to_hex(sha1));\n-\t\twrite_or_die(1, buf, len);\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\n+\t\tstrbuf_addf(&buf, \"%s\\t%s\\n\", report, sha1_to_hex(sha1));\n+\t\twrite_or_die(1, buf.buf, buf.len);\n+\t\tstrbuf_release(&buf);\n \n \t\t/*\n \t\t * Let's just mimic git-unpack-objects here and write\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 0513f7455..7b891471c 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -554,7 +554,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \tstruct notes_tree *t;\n \tunsigned char object[20], new_note[20];\n \tconst unsigned char *note;\n-\tchar logmsg[100];\n+\tchar *logmsg;\n \tconst char * const *usage;\n \tstruct note_data d = { 0, 0, NULL, STRBUF_INIT };\n \tstruct option options[] = {\n@@ -618,17 +618,16 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\twrite_note_data(&d, new_note);\n \t\tif (add_note(t, object, new_note, combine_notes_overwrite))\n \t\t\tdie(\"BUG: combine_notes_overwrite failed\");\n-\t\tsnprintf(logmsg, sizeof(logmsg), \"Notes added by 'git notes %s'\",\n-\t\t\targv[0]);\n+\t\tlogmsg = xstrfmt(\"Notes added by 'git notes %s'\", argv[0]);\n \t} else {\n \t\tfprintf(stderr, _(\"Removing note for object %s\\n\"),\n \t\t\tsha1_to_hex(object));\n \t\tremove_note(t, object);\n-\t\tsnprintf(logmsg, sizeof(logmsg), \"Notes removed by 'git notes %s'\",\n-\t\t\targv[0]);\n+\t\tlogmsg = xstrfmt(\"Notes removed by 'git notes %s'\", argv[0]);\n \t}\n \tcommit_notes(t, logmsg);\n \n+\tfree(logmsg);\n \tfree_note_data(&d);\n \tfree_notes(t);\n \treturn 0;\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 9e53a1a7c..f54d7b502 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -213,13 +213,14 @@ static int show_abbrev(const unsigned char *sha1, void *cb_data)\n \n static void show_datestring(const char *flag, const char *datestr)\n {\n-\tstatic char buffer[100];\n+\tchar *buffer;\n \n \t/* date handling requires both flags and revs */\n \tif ((filter & (DO_FLAGS | DO_REVS)) != (DO_FLAGS | DO_REVS))\n \t\treturn;\n-\tsnprintf(buffer, sizeof(buffer), \"%s%lu\", flag, approxidate(datestr));\n+\tbuffer = xstrfmt(\"%s%lu\", flag, approxidate(datestr));\n \tshow(buffer);\n+\tfree(buffer);\n }\n \n static int show_file(const char *arg, int output_prefix)\n-- \n2.12.2.845.g55fcf8b10\n\n"},{"id":"315605","messageId":"20170328194647.ac4rj3aw7jbteqjh@sigill.intra.peff.net","threadId":"45529","inReplyTo":"20170328194255.vf7nfzzmmzxsbn36@sigill.intra.peff.net","subject":"[PATCH 12/18] receive-pack: print --pack-header directly into argv array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-28T19:46:47Z","receivedAt":"2017-03-28T19:47:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After receive-pack reads the pack header from the client, it\nfeeds the already-read part to index-pack and unpack-objects\nvia their --pack-header command-line options.  To do so, we\nformat it into a fixed buffer, then duplicate it into the\nchild's argv_array.\n\nOur buffer is long enough to handle any possible input, so\nthis isn't wrong. But it's more complicated than it needs to\nbe; we can just argv_array_pushf() the final value and avoid\nthe intermediate copy. This drops the magic number and is\nmore efficient, too.\n\nNote that we need to push to the argv_array in order, which\nmeans we can't do the push until we are in the \"unpack-objects\nversus index-pack\" conditional.  Rather than duplicate the\nslightly complicated format specifier, I pushed it into a\nhelper function.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/receive-pack.c | 17 ++++++++++-------\n 1 file changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex fb2a090a0..2ca93adef 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1634,12 +1634,17 @@ static const char *parse_pack_header(struct pack_header *hdr)\n \n static const char *pack_lockfile;\n \n+static void push_header_arg(struct argv_array *args, struct pack_header *hdr)\n+{\n+\targv_array_pushf(args, \"--pack_header=%\"PRIu32\",%\"PRIu32,\n+\t\t\tntohl(hdr->hdr_version), ntohl(hdr->hdr_entries));\n+}\n+\n static const char *unpack(int err_fd, struct shallow_info *si)\n {\n \tstruct pack_header hdr;\n \tconst char *hdr_err;\n \tint status;\n-\tchar hdr_arg[38];\n \tstruct child_process child = CHILD_PROCESS_INIT;\n \tint fsck_objects = (receive_fsck_objects >= 0\n \t\t\t    ? receive_fsck_objects\n@@ -1653,9 +1658,6 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \t\t\tclose(err_fd);\n \t\treturn hdr_err;\n \t}\n-\tsnprintf(hdr_arg, sizeof(hdr_arg),\n-\t\t\t\"--pack_header=%\"PRIu32\",%\"PRIu32,\n-\t\t\tntohl(hdr.hdr_version), ntohl(hdr.hdr_entries));\n \n \tif (si->nr_ours || si->nr_theirs) {\n \t\talt_shallow_file = setup_temporary_shallow(si->shallow);\n@@ -1679,7 +1681,8 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \ttmp_objdir_add_as_alternate(tmp_objdir);\n \n \tif (ntohl(hdr.hdr_entries) < unpack_limit) {\n-\t\targv_array_pushl(&child.args, \"unpack-objects\", hdr_arg, NULL);\n+\t\targv_array_push(&child.args, \"unpack-objects\");\n+\t\tpush_header_arg(&child.args, &hdr);\n \t\tif (quiet)\n \t\t\targv_array_push(&child.args, \"-q\");\n \t\tif (fsck_objects)\n@@ -1697,8 +1700,8 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \t} else {\n \t\tchar hostname[256];\n \n-\t\targv_array_pushl(&child.args, \"index-pack\",\n-\t\t\t\t \"--stdin\", hdr_arg, NULL);\n+\t\targv_array_pushl(&child.args, \"index-pack\", \"--stdin\", NULL);\n+\t\tpush_header_arg(&child.args, &hdr);\n \n \t\tif (gethostname(hostname, sizeof(hostname)))\n \t\t\txsnprintf(hostname, sizeof(hostname), \"localhost\");\n-- \n2.12.2.845.g55fcf8b10\n\n"},{"id":"315606","messageId":"20170328194629.5ddehsxizajitn3q@sigill.intra.peff.net","threadId":"45529","inReplyTo":"20170328194255.vf7nfzzmmzxsbn36@sigill.intra.peff.net","subject":"[PATCH 07/18] avoid using fixed PATH_MAX buffers for refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-28T19:46:30Z","receivedAt":"2017-03-28T19:47:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Many functions which handle refs use a PATH_MAX-sized buffer\nto do so. This is mostly reasonable as we have to write\nloose refs into the filesystem, and at least on Linux the 4K\nPATH_MAX is big enough that nobody would care. But:\n\n  1. The static PATH_MAX is not always the filesystem limit.\n\n  2. On other platforms, PATH_MAX may be much smaller.\n\n  3. As we move to alternate ref storage, we won't be bound\n     by filesystem limits.\n\nLet's convert these to heap buffers so we don't have to\nworry about truncation or size limits.\n\nWe may want to eventually constrain ref lengths for sanity\nand to prevent malicious names, but we should do so\nconsistently across all platforms, and in a central place\n(like the ref code).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/checkout.c  |  5 ++---\n builtin/ls-remote.c | 10 ++++++----\n builtin/replace.c   | 50 +++++++++++++++++++++++++++-----------------------\n builtin/tag.c       | 15 ++++++---------\n 4 files changed, 41 insertions(+), 39 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 81f07c3ef..93e8dfc82 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -889,11 +889,10 @@ static int check_tracking_name(struct remote *remote, void *cb_data)\n static const char *unique_tracking_name(const char *name, struct object_id *oid)\n {\n \tstruct tracking_name_data cb_data = { NULL, NULL, NULL, 1 };\n-\tchar src_ref[PATH_MAX];\n-\tsnprintf(src_ref, PATH_MAX, \"refs/heads/%s\", name);\n-\tcb_data.src_ref = src_ref;\n+\tcb_data.src_ref = xstrfmt(\"refs/heads/%s\", name);\n \tcb_data.dst_oid = oid;\n \tfor_each_remote(check_tracking_name, &cb_data);\n+\tfree(cb_data.src_ref);\n \tif (cb_data.unique)\n \t\treturn cb_data.dst_ref;\n \tfree(cb_data.dst_ref);\ndiff --git a/builtin/ls-remote.c b/builtin/ls-remote.c\nindex 66cdd45cc..b2d7d5ce6 100644\n--- a/builtin/ls-remote.c\n+++ b/builtin/ls-remote.c\n@@ -17,17 +17,19 @@ static const char * const ls_remote_usage[] = {\n static int tail_match(const char **pattern, const char *path)\n {\n \tconst char *p;\n-\tchar pathbuf[PATH_MAX];\n+\tchar *pathbuf;\n \n \tif (!pattern)\n \t\treturn 1; /* no restriction */\n \n-\tif (snprintf(pathbuf, sizeof(pathbuf), \"/%s\", path) > sizeof(pathbuf))\n-\t\treturn error(\"insanely long ref %.*s...\", 20, path);\n+\tpathbuf = xstrfmt(\"/%s\", path);\n \twhile ((p = *(pattern++)) != NULL) {\n-\t\tif (!wildmatch(p, pathbuf, 0, NULL))\n+\t\tif (!wildmatch(p, pathbuf, 0, NULL)) {\n+\t\t\tfree(pathbuf);\n \t\t\treturn 1;\n+\t\t}\n \t}\n+\tfree(pathbuf);\n \treturn 0;\n }\n \ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex f83e7b8fc..065515bab 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -93,26 +93,31 @@ typedef int (*each_replace_name_fn)(const char *name, const char *ref,\n static int for_each_replace_name(const char **argv, each_replace_name_fn fn)\n {\n \tconst char **p, *full_hex;\n-\tchar ref[PATH_MAX];\n+\tstruct strbuf ref = STRBUF_INIT;\n+\tsize_t base_len;\n \tint had_error = 0;\n \tstruct object_id oid;\n \n+\tstrbuf_addstr(&ref, git_replace_ref_base);\n+\tbase_len = ref.len;\n+\n \tfor (p = argv; *p; p++) {\n \t\tif (get_oid(*p, &oid)) {\n \t\t\terror(\"Failed to resolve '%s' as a valid ref.\", *p);\n \t\t\thad_error = 1;\n \t\t\tcontinue;\n \t\t}\n-\t\tfull_hex = oid_to_hex(&oid);\n-\t\tsnprintf(ref, sizeof(ref), \"%s%s\", git_replace_ref_base, full_hex);\n-\t\t/* read_ref() may reuse the buffer */\n-\t\tfull_hex = ref + strlen(git_replace_ref_base);\n-\t\tif (read_ref(ref, oid.hash)) {\n+\n+\t\tstrbuf_setlen(&ref, base_len);\n+\t\tstrbuf_addstr(&ref, oid_to_hex(&oid));\n+\t\tfull_hex = ref.buf + base_len;\n+\n+\t\tif (read_ref(ref.buf, oid.hash)) {\n \t\t\terror(\"replace ref '%s' not found.\", full_hex);\n \t\t\thad_error = 1;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (fn(full_hex, ref, &oid))\n+\t\tif (fn(full_hex, ref.buf, &oid))\n \t\t\thad_error = 1;\n \t}\n \treturn had_error;\n@@ -129,21 +134,18 @@ static int delete_replace_ref(const char *name, const char *ref,\n \n static void check_ref_valid(struct object_id *object,\n \t\t\t    struct object_id *prev,\n-\t\t\t    char *ref,\n-\t\t\t    int ref_size,\n+\t\t\t    struct strbuf *ref,\n \t\t\t    int force)\n {\n-\tif (snprintf(ref, ref_size,\n-\t\t     \"%s%s\", git_replace_ref_base,\n-\t\t     oid_to_hex(object)) > ref_size - 1)\n-\t\tdie(\"replace ref name too long: %.*s...\", 50, ref);\n-\tif (check_refname_format(ref, 0))\n-\t\tdie(\"'%s' is not a valid ref name.\", ref);\n-\n-\tif (read_ref(ref, prev->hash))\n+\tstrbuf_reset(ref);\n+\tstrbuf_addf(ref, \"%s%s\", git_replace_ref_base, oid_to_hex(object));\n+\tif (check_refname_format(ref->buf, 0))\n+\t\tdie(\"'%s' is not a valid ref name.\", ref->buf);\n+\n+\tif (read_ref(ref->buf, prev->hash))\n \t\toidclr(prev);\n \telse if (!force)\n-\t\tdie(\"replace ref '%s' already exists\", ref);\n+\t\tdie(\"replace ref '%s' already exists\", ref->buf);\n }\n \n static int replace_object_oid(const char *object_ref,\n@@ -154,7 +156,7 @@ static int replace_object_oid(const char *object_ref,\n {\n \tstruct object_id prev;\n \tenum object_type obj_type, repl_type;\n-\tchar ref[PATH_MAX];\n+\tstruct strbuf ref = STRBUF_INIT;\n \tstruct ref_transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n \n@@ -167,16 +169,17 @@ static int replace_object_oid(const char *object_ref,\n \t\t    object_ref, typename(obj_type),\n \t\t    replace_ref, typename(repl_type));\n \n-\tcheck_ref_valid(object, &prev, ref, sizeof(ref), force);\n+\tcheck_ref_valid(object, &prev, &ref, force);\n \n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n-\t    ref_transaction_update(transaction, ref, repl->hash, prev.hash,\n+\t    ref_transaction_update(transaction, ref.buf, repl->hash, prev.hash,\n \t\t\t\t   0, NULL, &err) ||\n \t    ref_transaction_commit(transaction, &err))\n \t\tdie(\"%s\", err.buf);\n \n \tref_transaction_free(transaction);\n+\tstrbuf_release(&ref);\n \treturn 0;\n }\n \n@@ -280,7 +283,7 @@ static int edit_and_replace(const char *object_ref, int force, int raw)\n \tchar *tmpfile = git_pathdup(\"REPLACE_EDITOBJ\");\n \tenum object_type type;\n \tstruct object_id old, new, prev;\n-\tchar ref[PATH_MAX];\n+\tstruct strbuf ref = STRBUF_INIT;\n \n \tif (get_oid(object_ref, &old) < 0)\n \t\tdie(\"Not a valid object name: '%s'\", object_ref);\n@@ -289,7 +292,8 @@ static int edit_and_replace(const char *object_ref, int force, int raw)\n \tif (type < 0)\n \t\tdie(\"unable to get object type for %s\", oid_to_hex(&old));\n \n-\tcheck_ref_valid(&old, &prev, ref, sizeof(ref), force);\n+\tcheck_ref_valid(&old, &prev, &ref, force);\n+\tstrbuf_release(&ref);\n \n \texport_object(&old, type, raw, tmpfile);\n \tif (launch_editor(tmpfile, NULL, NULL) < 0)\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 045e7ad23..0eb4e25c6 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -72,25 +72,22 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn,\n \t\t\t     const void *cb_data)\n {\n \tconst char **p;\n-\tchar ref[PATH_MAX];\n+\tstruct strbuf ref = STRBUF_INIT;\n \tint had_error = 0;\n \tunsigned char sha1[20];\n \n \tfor (p = argv; *p; p++) {\n-\t\tif (snprintf(ref, sizeof(ref), \"refs/tags/%s\", *p)\n-\t\t\t\t\t>= sizeof(ref)) {\n-\t\t\terror(_(\"tag name too long: %.*s...\"), 50, *p);\n-\t\t\thad_error = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (read_ref(ref, sha1)) {\n+\t\tstrbuf_reset(&ref);\n+\t\tstrbuf_addf(&ref, \"refs/tags/%s\", *p);\n+\t\tif (read_ref(ref.buf, sha1)) {\n \t\t\terror(_(\"tag '%s' not found.\"), *p);\n \t\t\thad_error = 1;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (fn(*p, ref, sha1, cb_data))\n+\t\tif (fn(*p, ref.buf, sha1, cb_data))\n \t\t\thad_error = 1;\n \t}\n+\tstrbuf_release(&ref);\n \treturn had_error;\n }\n \n-- \n2.12.2.845.g55fcf8b10\n\n"},{"id":"315607","messageId":"20170328194644.jbgf3do2dp6m2a3w@sigill.intra.peff.net","threadId":"45529","inReplyTo":"20170328194255.vf7nfzzmmzxsbn36@sigill.intra.peff.net","subject":"[PATCH 11/18] name-rev: replace static buffer with strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-28T19:46:44Z","receivedAt":"2017-03-28T19:47:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When name-rev needs to format an actual name, we do so into\na fixed-size buffer. That includes the actual ref tip, as\nwell as any traversal information. Since refs can exceed\n1024 bytes, this means you can get a bogus result. E.g.,\ndoing:\n\n   git tag $(perl -e 'print join(\"/\", 1..1024)')\n   git describe --contains HEAD^\n\nresults in \".../282/283\", when it should be\n\".../1023/1024~1\".\n\nWe can solve this by using a heap buffer. We'll use a\nstrbuf, which lets us write into the same buffer from our\nloop without having to reallocate.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/name-rev.c | 21 ++++++++++++---------\n 1 file changed, 12 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/name-rev.c b/builtin/name-rev.c\nindex 8bdc3eaa6..92a5d8a5d 100644\n--- a/builtin/name-rev.c\n+++ b/builtin/name-rev.c\n@@ -238,10 +238,9 @@ static const char *get_exact_ref_match(const struct object *o)\n \treturn NULL;\n }\n \n-/* returns a static buffer */\n-static const char *get_rev_name(const struct object *o)\n+/* may return a constant string or use \"buf\" as scratch space */\n+static const char *get_rev_name(const struct object *o, struct strbuf *buf)\n {\n-\tstatic char buffer[1024];\n \tstruct rev_name *n;\n \tstruct commit *c;\n \n@@ -258,10 +257,9 @@ static const char *get_rev_name(const struct object *o)\n \t\tint len = strlen(n->tip_name);\n \t\tif (len > 2 && !strcmp(n->tip_name + len - 2, \"^0\"))\n \t\t\tlen -= 2;\n-\t\tsnprintf(buffer, sizeof(buffer), \"%.*s~%d\", len, n->tip_name,\n-\t\t\t\tn->generation);\n-\n-\t\treturn buffer;\n+\t\tstrbuf_reset(buf);\n+\t\tstrbuf_addf(buf, \"%.*s~%d\", len, n->tip_name, n->generation);\n+\t\treturn buf->buf;\n \t}\n }\n \n@@ -271,10 +269,11 @@ static void show_name(const struct object *obj,\n {\n \tconst char *name;\n \tconst struct object_id *oid = &obj->oid;\n+\tstruct strbuf buf = STRBUF_INIT;\n \n \tif (!name_only)\n \t\tprintf(\"%s \", caller_name ? caller_name : oid_to_hex(oid));\n-\tname = get_rev_name(obj);\n+\tname = get_rev_name(obj, &buf);\n \tif (name)\n \t\tprintf(\"%s\\n\", name);\n \telse if (allow_undefined)\n@@ -283,6 +282,7 @@ static void show_name(const struct object *obj,\n \t\tprintf(\"%s\\n\", find_unique_abbrev(oid->hash, DEFAULT_ABBREV));\n \telse\n \t\tdie(\"cannot describe '%s'\", oid_to_hex(oid));\n+\tstrbuf_release(&buf);\n }\n \n static char const * const name_rev_usage[] = {\n@@ -294,6 +294,7 @@ static char const * const name_rev_usage[] = {\n \n static void name_rev_line(char *p, struct name_ref_data *data)\n {\n+\tstruct strbuf buf = STRBUF_INIT;\n \tint forty = 0;\n \tchar *p_start;\n \tfor (p_start = p; *p; p++) {\n@@ -314,7 +315,7 @@ static void name_rev_line(char *p, struct name_ref_data *data)\n \t\t\t\tstruct object *o =\n \t\t\t\t\tlookup_object(sha1);\n \t\t\t\tif (o)\n-\t\t\t\t\tname = get_rev_name(o);\n+\t\t\t\t\tname = get_rev_name(o, &buf);\n \t\t\t}\n \t\t\t*(p+1) = c;\n \n@@ -332,6 +333,8 @@ static void name_rev_line(char *p, struct name_ref_data *data)\n \t/* flush */\n \tif (p_start != p)\n \t\tfwrite(p_start, p - p_start, 1, stdout);\n+\n+\tstrbuf_release(&buf);\n }\n \n int cmd_name_rev(int argc, const char **argv, const char *prefix)\n-- \n2.12.2.845.g55fcf8b10\n\n"},{"id":"315608","messageId":"20170328194656.d4sgj67psmpnnmua@sigill.intra.peff.net","threadId":"45529","inReplyTo":"20170328194255.vf7nfzzmmzxsbn36@sigill.intra.peff.net","subject":"[PATCH 15/18] convert unchecked snprintf into xsnprintf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-28T19:46:56Z","receivedAt":"2017-03-28T19:47:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These calls to snprintf should always succeed, because their\ninput is small and fixed. Let's use xsnprintf to make sure\nthis is the case (and to make auditing for actual truncation\neasier).\n\nThese could be candidates for turning into heap buffers, but\nthey fall into a few broad categories that make it not worth\ndoing:\n\n  - formatting single numbers is simple enough that we can\n    see the result should fit\n\n  - the size of a sha1 is likewise well-known, and I didn't\n    want to cause unnecessary conflicts with the ongoing\n    process to convert these constants to GIT_MAX_HEXSZ\n\n  - the interface for curl_errorstr is dictated by curl\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n grep.c      |  4 ++--\n http.c      | 10 +++++-----\n imap-send.c |  2 +-\n sha1_file.c |  4 ++--\n submodule.c |  2 +-\n 5 files changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 0dbdc1d00..39b4b60d2 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1164,7 +1164,7 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,\n \t}\n \tif (opt->linenum) {\n \t\tchar buf[32];\n-\t\tsnprintf(buf, sizeof(buf), \"%d\", lno);\n+\t\txsnprintf(buf, sizeof(buf), \"%d\", lno);\n \t\toutput_color(opt, buf, strlen(buf), opt->color_lineno);\n \t\toutput_sep(opt, sign);\n \t}\n@@ -1651,7 +1651,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \t\t\t\t     opt->color_filename);\n \t\t\toutput_sep(opt, ':');\n \t\t}\n-\t\tsnprintf(buf, sizeof(buf), \"%u\\n\", count);\n+\t\txsnprintf(buf, sizeof(buf), \"%u\\n\", count);\n \t\topt->output(opt, buf, strlen(buf));\n \t\treturn 1;\n \t}\ndiff --git a/http.c b/http.c\nindex 96d84bbed..8d94e2c63 100644\n--- a/http.c\n+++ b/http.c\n@@ -1366,9 +1366,9 @@ static int handle_curl_result(struct slot_results *results)\n \t\t * FAILONERROR it is lost, so we can give only the numeric\n \t\t * status code.\n \t\t */\n-\t\tsnprintf(curl_errorstr, sizeof(curl_errorstr),\n-\t\t\t \"The requested URL returned error: %ld\",\n-\t\t\t results->http_code);\n+\t\txsnprintf(curl_errorstr, sizeof(curl_errorstr),\n+\t\t\t  \"The requested URL returned error: %ld\",\n+\t\t\t  results->http_code);\n \t}\n \n \tif (results->curl_result == CURLE_OK) {\n@@ -1410,8 +1410,8 @@ int run_one_slot(struct active_request_slot *slot,\n {\n \tslot->results = results;\n \tif (!start_active_slot(slot)) {\n-\t\tsnprintf(curl_errorstr, sizeof(curl_errorstr),\n-\t\t\t \"failed to start HTTP request\");\n+\t\txsnprintf(curl_errorstr, sizeof(curl_errorstr),\n+\t\t\t  \"failed to start HTTP request\");\n \t\treturn HTTP_START_FAILED;\n \t}\n \ndiff --git a/imap-send.c b/imap-send.c\nindex 5c7e27a89..857591660 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -964,7 +964,7 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char *f\n \t\tint gai;\n \t\tchar portstr[6];\n \n-\t\tsnprintf(portstr, sizeof(portstr), \"%d\", srvc->port);\n+\t\txsnprintf(portstr, sizeof(portstr), \"%d\", srvc->port);\n \n \t\tmemset(&hints, 0, sizeof(hints));\n \t\thints.ai_socktype = SOCK_STREAM;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 71063890f..43990dec7 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3762,8 +3762,8 @@ static int for_each_file_in_obj_subdir(int subdir_nr,\n \t\t\tchar hex[GIT_SHA1_HEXSZ+1];\n \t\t\tstruct object_id oid;\n \n-\t\t\tsnprintf(hex, sizeof(hex), \"%02x%s\",\n-\t\t\t\t subdir_nr, de->d_name);\n+\t\t\txsnprintf(hex, sizeof(hex), \"%02x%s\",\n+\t\t\t\t  subdir_nr, de->d_name);\n \t\t\tif (!get_oid_hex(hex, &oid)) {\n \t\t\t\tif (obj_cb) {\n \t\t\t\t\tr = obj_cb(&oid, path->buf, data);\ndiff --git a/submodule.c b/submodule.c\nindex 3200b7bb2..e11ea7d0a 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1221,7 +1221,7 @@ static int find_first_merges(struct object_array *result, const char *path,\n \tmemset(&rev_opts, 0, sizeof(rev_opts));\n \n \t/* get all revisions that merge commit a */\n-\tsnprintf(merged_revision, sizeof(merged_revision), \"^%s\",\n+\txsnprintf(merged_revision, sizeof(merged_revision), \"^%s\",\n \t\t\toid_to_hex(&a->object.oid));\n \tinit_revisions(&revs, NULL);\n \trev_opts.submodule = path;\n-- \n2.12.2.845.g55fcf8b10\n\n"},{"id":"315609","messageId":"20170328194640.i5tp5sa374i56cuv@sigill.intra.peff.net","threadId":"45529","inReplyTo":"20170328194255.vf7nfzzmmzxsbn36@sigill.intra.peff.net","subject":"[PATCH 10/18] create_branch: use xstrfmt for reflog message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-28T19:46:40Z","receivedAt":"2017-03-28T19:47:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We generate a reflog message that contains some fixed text\nplus a branch name, and use a buffer of size PATH_MAX + 20.\nThis mostly works if you assume that refnames are shorter\nthan PATH_MAX, but:\n\n  1. That's not necessarily true. PATH_MAX is not always the\n     filesystem's limit.\n\n  2. The \"20\" is not sufficiently large for the fixed text\n     anyway.\n\nLet's just switch to a heap buffer so we don't have to even\ncare.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n branch.c | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 6d0ca94cc..ad5a2299b 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -296,14 +296,12 @@ void create_branch(const char *name, const char *start_name,\n \tif (!dont_change_ref) {\n \t\tstruct ref_transaction *transaction;\n \t\tstruct strbuf err = STRBUF_INIT;\n-\t\tchar msg[PATH_MAX + 20];\n+\t\tchar *msg;\n \n \t\tif (forcing)\n-\t\t\tsnprintf(msg, sizeof msg, \"branch: Reset to %s\",\n-\t\t\t\t start_name);\n+\t\t\tmsg = xstrfmt(\"branch: Reset to %s\", start_name);\n \t\telse\n-\t\t\tsnprintf(msg, sizeof msg, \"branch: Created from %s\",\n-\t\t\t\t start_name);\n+\t\t\tmsg = xstrfmt(\"branch: Created from %s\", start_name);\n \n \t\ttransaction = ref_transaction_begin(&err);\n \t\tif (!transaction ||\n@@ -314,6 +312,7 @@ void create_branch(const char *name, const char *start_name,\n \t\t\tdie(\"%s\", err.buf);\n \t\tref_transaction_free(transaction);\n \t\tstrbuf_release(&err);\n+\t\tfree(msg);\n \t}\n \n \tif (real_ref && track)\n-- \n2.12.2.845.g55fcf8b10\n\n"},{"id":"315610","messageId":"20170328194653.o2qknaynp45ha7yf@sigill.intra.peff.net","threadId":"45529","inReplyTo":"20170328194255.vf7nfzzmmzxsbn36@sigill.intra.peff.net","subject":"[PATCH 14/18] combine-diff: replace malloc/snprintf with xstrfmt","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-28T19:46:53Z","receivedAt":"2017-03-28T19:47:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's no need to use the magic \"100\" when a strbuf can do\nit for us.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n combine-diff.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 59501db99..d3560573a 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -292,9 +292,10 @@ static char *grab_blob(const struct object_id *oid, unsigned int mode,\n \tenum object_type type;\n \n \tif (S_ISGITLINK(mode)) {\n-\t\tblob = xmalloc(100);\n-\t\t*size = snprintf(blob, 100,\n-\t\t\t\t \"Subproject commit %s\\n\", oid_to_hex(oid));\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tstrbuf_addf(&buf, \"Subproject commit %s\\n\", oid_to_hex(oid));\n+\t\t*size = buf.len;\n+\t\tblob = strbuf_detach(&buf, NULL);\n \t} else if (is_null_oid(oid)) {\n \t\t/* deleted blob */\n \t\t*size = 0;\n-- \n2.12.2.845.g55fcf8b10\n\n"},{"id":"315611","messageId":"20170328194703.jsvwdykdshzzas5y@sigill.intra.peff.net","threadId":"45529","inReplyTo":"20170328194255.vf7nfzzmmzxsbn36@sigill.intra.peff.net","subject":"[PATCH 17/18] gc: replace local buffer with git_path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-28T19:47:03Z","receivedAt":"2017-03-28T19:47:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We probe the \"17/\" loose object directory for auto-gc, and\nuse a local buffer to format the path. We can just use\ngit_path() for this. It handles paths of any length\n(reducing our error handling). And because we feed the\nresult straight to a system call, we can just use the static\nvariant.\n\nNote that git_path also knows the string \"objects/\" is\nspecial, and will replace it with git_object_directory()\nwhen necessary.\n\nAnother alternative would be to use sha1_file_name() for the\npretend object \"170000...\", but that ends up being more\nhassle for no gain, as we have to truncate the final path\ncomponent.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/gc.c | 8 +-------\n 1 file changed, 1 insertion(+), 7 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex c2c61a57b..2daede782 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -135,8 +135,6 @@ static int too_many_loose_objects(void)\n \t * distributed, we can check only one and get a reasonable\n \t * estimate.\n \t */\n-\tchar path[PATH_MAX];\n-\tconst char *objdir = get_object_directory();\n \tDIR *dir;\n \tstruct dirent *ent;\n \tint auto_threshold;\n@@ -146,11 +144,7 @@ static int too_many_loose_objects(void)\n \tif (gc_auto_threshold <= 0)\n \t\treturn 0;\n \n-\tif (sizeof(path) <= snprintf(path, sizeof(path), \"%s/17\", objdir)) {\n-\t\twarning(_(\"insanely long object directory %.*s\"), 50, objdir);\n-\t\treturn 0;\n-\t}\n-\tdir = opendir(path);\n+\tdir = opendir(git_path(\"objects/17\"));\n \tif (!dir)\n \t\treturn 0;\n \n-- \n2.12.2.845.g55fcf8b10\n\n"},{"id":"315612","messageId":"20170328194700.yowtm7sg55loyhdg@sigill.intra.peff.net","threadId":"45529","inReplyTo":"20170328194255.vf7nfzzmmzxsbn36@sigill.intra.peff.net","subject":"[PATCH 16/18] transport-helper: replace checked snprintf with xsnprintf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-28T19:47:00Z","receivedAt":"2017-03-28T19:48:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We can use xsnprintf to do our truncation check with less\ncode. The error message isn't as specific, but the point is\nthat this isn't supposed to trigger in the first place\n(because our buffer is big enough to handle any int).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n transport-helper.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex dc90a1fb7..36408046e 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -347,14 +347,11 @@ static int set_helper_option(struct transport *transport,\n static void standard_options(struct transport *t)\n {\n \tchar buf[16];\n-\tint n;\n \tint v = t->verbose;\n \n \tset_helper_option(t, \"progress\", t->progress ? \"true\" : \"false\");\n \n-\tn = snprintf(buf, sizeof(buf), \"%d\", v + 1);\n-\tif (n >= sizeof(buf))\n-\t\tdie(\"impossibly large verbosity value\");\n+\txsnprintf(buf, sizeof(buf), \"%d\", v + 1);\n \tset_helper_option(t, \"verbosity\", buf);\n \n \tswitch (t->family) {\n-- \n2.12.2.845.g55fcf8b10\n\n"},{"id":"315613","messageId":"20170328194810.upseqefljp2v3a6n@sigill.intra.peff.net","threadId":"45529","inReplyTo":"20170328194255.vf7nfzzmmzxsbn36@sigill.intra.peff.net","subject":"[PATCH 18/18] daemon: use an argv_array to exec children","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-28T19:48:10Z","receivedAt":"2017-03-28T19:48:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our struct child_process already has its own argv_array.\nLet's use that to avoid having to format options into\nseparate buffers.\n\nNote that we'll need to declare the child process outside of\nthe run_service_command() helper to do this. But that opens\nup a further simplification, which is that the helper can\nappend to our argument list, saving each caller from\nspecifying \".\" manually.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nOf all the patches in this series, this is the one where I was most\nundecided on whether the result was more readable or not.\n\nMy ulterior motive, of course, was to get rid of the unchecked\nsnprintf() into timebuf. But it could also just become an xsnprintf()\nwith no further changes.\n\n daemon.c | 38 +++++++++++++++++---------------------\n 1 file changed, 17 insertions(+), 21 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 473e6b6b6..f70d27b82 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -449,46 +449,42 @@ static void copy_to_log(int fd)\n \tfclose(fp);\n }\n \n-static int run_service_command(const char **argv)\n+static int run_service_command(struct child_process *cld)\n {\n-\tstruct child_process cld = CHILD_PROCESS_INIT;\n-\n-\tcld.argv = argv;\n-\tcld.git_cmd = 1;\n-\tcld.err = -1;\n-\tif (start_command(&cld))\n+\targv_array_push(&cld->args, \".\");\n+\tcld->git_cmd = 1;\n+\tcld->err = -1;\n+\tif (start_command(cld))\n \t\treturn -1;\n \n \tclose(0);\n \tclose(1);\n \n-\tcopy_to_log(cld.err);\n+\tcopy_to_log(cld->err);\n \n-\treturn finish_command(&cld);\n+\treturn finish_command(cld);\n }\n \n static int upload_pack(void)\n {\n-\t/* Timeout as string */\n-\tchar timeout_buf[64];\n-\tconst char *argv[] = { \"upload-pack\", \"--strict\", NULL, \".\", NULL };\n-\n-\targv[2] = timeout_buf;\n-\n-\tsnprintf(timeout_buf, sizeof timeout_buf, \"--timeout=%u\", timeout);\n-\treturn run_service_command(argv);\n+\tstruct child_process cld = CHILD_PROCESS_INIT;\n+\targv_array_pushl(&cld.args, \"upload-pack\", \"--strict\", NULL);\n+\targv_array_pushf(&cld.args, \"--timeout=%u\", timeout);\n+\treturn run_service_command(&cld);\n }\n \n static int upload_archive(void)\n {\n-\tstatic const char *argv[] = { \"upload-archive\", \".\", NULL };\n-\treturn run_service_command(argv);\n+\tstruct child_process cld = CHILD_PROCESS_INIT;\n+\targv_array_push(&cld.args, \"upload-archive\");\n+\treturn run_service_command(&cld);\n }\n \n static int receive_pack(void)\n {\n-\tstatic const char *argv[] = { \"receive-pack\", \".\", NULL };\n-\treturn run_service_command(argv);\n+\tstruct child_process cld = CHILD_PROCESS_INIT;\n+\targv_array_push(&cld.args, \"receive-pack\");\n+\treturn run_service_command(&cld);\n }\n \n static struct daemon_service daemon_service[] = {\n-- \n2.12.2.845.g55fcf8b10\n"},{"id":"315614","messageId":"20170328195035.ffeewnchhucwi5nj@sigill.intra.peff.net","threadId":"45529","inReplyTo":"20170328194618.injneju2d2fkghmk@sigill.intra.peff.net","subject":"Re: [PATCH 04/18] diff: avoid fixed-size buffer for patch-ids","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-28T19:50:36Z","receivedAt":"2017-03-28T19:51:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 28, 2017 at 03:46:18PM -0400, Jeff King wrote:\n\n> As a result, the data we feed to the sha1 computation may be\n> truncated, and it's possible that a commit with a very long\n> filename could erroneously collide in the patch-id space\n> with another commit. For instance, if one commit modified\n> \"really-long-filename/foo\" and another modified \"bar\" in the\n> same directory.\n> \n> In practice this is unlikely. Because the filenames are\n> repeated, and because there's a single cutoff at the end of\n> the buffer, the offending filename would have to be on the\n> order of four times larger than PATH_MAX.\n> \n> But it's easy to fix by moving to a strbuf.\n\nThe other obvious solution is to avoid formatting the string entirely,\nand just feed the pieces to the sha1 computation. Unfortunately that\nstill involves formatting the modes (into a fixed-size buffer!) since we\nneed the sha1 of their string representations.\n\nThat patch is below for reference. I'm not sure if it's more readable or\nnot.\n\n---\ndiff --git a/diff.c b/diff.c\nindex a628ac3a9..435c734f4 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4570,6 +4570,19 @@ static void patch_id_consume(void *priv, char *line, unsigned long len)\n \tdata->patchlen += new_len;\n }\n \n+static void patch_id_add_string(git_SHA_CTX *ctx, const char *str)\n+{\n+\tgit_SHA1_Update(ctx, str, strlen(str));\n+}\n+\n+static void patch_id_add_mode(git_SHA_CTX *ctx, unsigned mode)\n+{\n+\t/* large enough for 2^32 in octal */\n+\tchar buf[12];\n+\tint len = xsnprintf(buf, sizeof(buf), \"%06o\", mode);\n+\tgit_SHA1_Update(ctx, buf, len);\n+}\n+\n /* returns 0 upon success, and writes result into sha1 */\n static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1, int diff_header_only)\n {\n@@ -4577,7 +4590,6 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1,\n \tint i;\n \tgit_SHA_CTX ctx;\n \tstruct patch_id_t data;\n-\tchar buffer[PATH_MAX * 4 + 20];\n \n \tgit_SHA1_Init(&ctx);\n \tmemset(&data, 0, sizeof(struct patch_id_t));\n@@ -4609,36 +4621,30 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1,\n \n \t\tlen1 = remove_space(p->one->path, strlen(p->one->path));\n \t\tlen2 = remove_space(p->two->path, strlen(p->two->path));\n-\t\tif (p->one->mode == 0)\n-\t\t\tlen1 = snprintf(buffer, sizeof(buffer),\n-\t\t\t\t\t\"diff--gita/%.*sb/%.*s\"\n-\t\t\t\t\t\"newfilemode%06o\"\n-\t\t\t\t\t\"---/dev/null\"\n-\t\t\t\t\t\"+++b/%.*s\",\n-\t\t\t\t\tlen1, p->one->path,\n-\t\t\t\t\tlen2, p->two->path,\n-\t\t\t\t\tp->two->mode,\n-\t\t\t\t\tlen2, p->two->path);\n-\t\telse if (p->two->mode == 0)\n-\t\t\tlen1 = snprintf(buffer, sizeof(buffer),\n-\t\t\t\t\t\"diff--gita/%.*sb/%.*s\"\n-\t\t\t\t\t\"deletedfilemode%06o\"\n-\t\t\t\t\t\"---a/%.*s\"\n-\t\t\t\t\t\"+++/dev/null\",\n-\t\t\t\t\tlen1, p->one->path,\n-\t\t\t\t\tlen2, p->two->path,\n-\t\t\t\t\tp->one->mode,\n-\t\t\t\t\tlen1, p->one->path);\n-\t\telse\n-\t\t\tlen1 = snprintf(buffer, sizeof(buffer),\n-\t\t\t\t\t\"diff--gita/%.*sb/%.*s\"\n-\t\t\t\t\t\"---a/%.*s\"\n-\t\t\t\t\t\"+++b/%.*s\",\n-\t\t\t\t\tlen1, p->one->path,\n-\t\t\t\t\tlen2, p->two->path,\n-\t\t\t\t\tlen1, p->one->path,\n-\t\t\t\t\tlen2, p->two->path);\n-\t\tgit_SHA1_Update(&ctx, buffer, len1);\n+\t\tpatch_id_add_string(&ctx, \"diff--git\");\n+\t\tpatch_id_add_string(&ctx, \"a/\");\n+\t\tgit_SHA1_Update(&ctx, p->one->path, len1);\n+\t\tpatch_id_add_string(&ctx, \"b/\");\n+\t\tgit_SHA1_Update(&ctx, p->two->path, len2);\n+\n+\t\tif (p->one->mode == 0) {\n+\t\t\tpatch_id_add_string(&ctx, \"newfilemode\");\n+\t\t\tpatch_id_add_mode(&ctx, p->two->mode);\n+\t\t\tpatch_id_add_string(&ctx, \"---/dev/null\");\n+\t\t\tpatch_id_add_string(&ctx, \"+++b/\");\n+\t\t\tgit_SHA1_Update(&ctx, p->two->path, len2);\n+\t\t} else if (p->two->mode == 0) {\n+\t\t\tpatch_id_add_string(&ctx, \"deletedfilemode\");\n+\t\t\tpatch_id_add_mode(&ctx, p->one->mode);\n+\t\t\tpatch_id_add_string(&ctx, \"---a/\");\n+\t\t\tgit_SHA1_Update(&ctx, p->one->path, len1);\n+\t\t\tpatch_id_add_string(&ctx, \"+++/dev/null\");\n+\t\t} else {\n+\t\t\tpatch_id_add_string(&ctx, \"---a/\");\n+\t\t\tgit_SHA1_Update(&ctx, p->one->path, len1);\n+\t\t\tpatch_id_add_string(&ctx, \"+++b/\");\n+\t\t\tgit_SHA1_Update(&ctx, p->two->path, len2);\n+\t\t}\n \n \t\tif (diff_header_only)\n \t\t\tcontinue;\n"},{"id":"315627","messageId":"xmqq60itc9pv.fsf@gitster.mtv.corp.google.com","threadId":"45529","inReplyTo":"20170328194255.vf7nfzzmmzxsbn36@sigill.intra.peff.net","subject":"Re: [PATCH 0/18] snprintf cleanups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-28T22:33:48Z","receivedAt":"2017-03-28T22:34:02Z","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> It's a lot of patches, but hopefully they're all pretty straightforward\n> to read.\n\nYes, quite a lot of changes.  I didn't see anything questionable in\nthere.\n\nAs to the \"patch-id\" thing, I find the alternate one slightly easier\nto read.  Also, exactly because this is not a performance critical\ncodepath, it may be better if patch_id_add_string() filtered out\nwhitespaces; that would allow the source to express things in more\nnatural way, e.g.\n\n\t\tpatch_id_addf(&ctx, \"new file mode\");\n\t\tpatch_id_addf(&ctx, \"%06o\", p->two->mode);\n\t\tpatch_id_addf(&ctx, \"--- /dev/null\");\n\t\tpatch_id_addf(&ctx, \"+++ b/%.*s\", len2, p->two->path);\n\nOr I may be going overboard by bringing \"addf\" into the mix X-<.\n"},{"id":"315648","messageId":"20170329034105.bfgh4tutgrmjp2lc@sigill.intra.peff.net","threadId":"45529","inReplyTo":"xmqq60itc9pv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/18] snprintf cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-29T03:41:05Z","receivedAt":"2017-03-29T03:41:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 28, 2017 at 03:33:48PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > It's a lot of patches, but hopefully they're all pretty straightforward\n> > to read.\n> \n> Yes, quite a lot of changes.  I didn't see anything questionable in\n> there.\n> \n> As to the \"patch-id\" thing, I find the alternate one slightly easier\n> to read.  Also, exactly because this is not a performance critical\n> codepath, it may be better if patch_id_add_string() filtered out\n> whitespaces; that would allow the source to express things in more\n> natural way, e.g.\n> \n> \t\tpatch_id_addf(&ctx, \"new file mode\");\n> \t\tpatch_id_addf(&ctx, \"%06o\", p->two->mode);\n> \t\tpatch_id_addf(&ctx, \"--- /dev/null\");\n> \t\tpatch_id_addf(&ctx, \"+++ b/%.*s\", len2, p->two->path);\n> \n> Or I may be going overboard by bringing \"addf\" into the mix X-<.\n\nI think there are two things going on in your example.\n\nOne is that obviously patch_id_addf() removes the spaces from the\nresult. But we could do that now by keeping the big strbuf_addf(), and\nthen just walking the result and feeding non-spaces.\n\nThe second is that your addf means we are back to formatting everything\ninto a buffer again. And it has to be dynamic to handle the final line\nthere, because \"len2\" isn't bounded. At which point we may as well go\nback to sticking it all in one big strbuf (your example also breaks it\ndown line by line, but we could do that with separte strbuf_addf calls,\ntoo).\n\nOr you have to reimplement the printf format-parsing yourself, and write\ninto the sha1 instead of into the buffers. But that's probably insane.\n\nI think the \"no extra buffer with whitespace\" combo is more like:\n\n  void patch_id_add_buf(git_SHA1_CTX *ctx, const char *buf, size_t len)\n  {\n\tfor (; len > 0; buf++, len--) {\n\t\tif (!isspace(*buf))\n\t\t\tgit_SHA1_Update(ctx, buf, 1);\n\t}\n  }\n\n  void patch_id_add_str(git_SHA1_CTX *ctx, const char *str)\n  {\n\tpatch_id_add_buf(ctx, strlen(str));\n  }\n\n  void patch_id_add_mode(git_SHA1_CTX *ctx, unsigned mode)\n  {\n\tchar buf[16]; /* big enough... */\n\tint len = xsnprintf(buf, \"%06o\", mode);\n\tpatch_id_add_buf(ctx, buf, len);\n  }\n\n  patch_id_add_str(&ctx, \"new file mode\");\n  patch_id_add_mode(&ctx, p->two->mode);\n  patch_id_add_str(&ctx, \"--- /dev/null\");\n  patch_id_add_str(&ctx, \"+++ b/\");\n  patch_id_add_buf(&ctx, p->two->path, len2);\n\nI dunno. I wondered if feeding single bytes to the sha1 update might\nactually be noticeably slower, because I would assume that internally it\ngenerally copies data in larger chunks. I didn't measure it, though.\n\n-Peff\n"},{"id":"315652","messageId":"20170329071055.bp7d2gwqdhspz4aq@sigill.intra.peff.net","threadId":"45529","inReplyTo":"20170328194255.vf7nfzzmmzxsbn36@sigill.intra.peff.net","subject":"[PATCH] Makefile: detect errors in running spatch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-29T07:10:55Z","receivedAt":"2017-03-29T07:11:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"make coccicheck\" target runs spatch against each source\nfile. But it does so in a for loop, so \"make\" never sees the\nexit code of spatch. Worse, it redirects stderr to a log\nfile, so the user has no indication of any failure. And then\nto top it all off, because we touched the patch file's\nmtime, make will refuse to repeat the command because it\nthink the target is up-to-date.\n\nSo for example:\n\n  $ make coccicheck SPATCH=does-not-exist\n      SPATCH contrib/coccinelle/free.cocci\n      SPATCH contrib/coccinelle/qsort.cocci\n      SPATCH contrib/coccinelle/xstrdup_or_null.cocci\n      SPATCH contrib/coccinelle/swap.cocci\n      SPATCH contrib/coccinelle/strbuf.cocci\n      SPATCH contrib/coccinelle/object_id.cocci\n      SPATCH contrib/coccinelle/array.cocci\n  $ make coccicheck SPATCH=does-not-exist\n  make: Nothing to be done for 'coccicheck'.\n\nWith this patch, you get:\n\n  $ make coccicheck SPATCH=does-not-exist\n       SPATCH contrib/coccinelle/free.cocci\n  /bin/sh: 4: does-not-exist: not found\n  Makefile:2338: recipe for target 'contrib/coccinelle/free.cocci.patch' failed\n  make: *** [contrib/coccinelle/free.cocci.patch] Error 1\n\nIt also dumps the log on failure, so any errors from spatch\nitself (like syntax errors in our .cocci files) will be seen\nby the user.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is a verbatim repost of:\n\n  http://public-inbox.org/git/20170310083117.cbflqx7zbe4s7cqv@sigill.intra.peff.net/\n\nI think this is a strict improvement over the status quo. The\nconversation in that thread turned to possible refactorings, but since\nthose didn't materialize, I think we should apply this in the meantime.\n\n Makefile | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 9f8b35ad4..9b36068ac 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2348,9 +2348,17 @@ check: common-cmds.h\n C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ))\n %.cocci.patch: %.cocci $(C_SOURCES)\n \t@echo '    ' SPATCH $<; \\\n+\tret=0; \\\n \tfor f in $(C_SOURCES); do \\\n-\t\t$(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS); \\\n-\tdone >$@ 2>$@.log; \\\n+\t\t$(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \\\n+\t\t\t{ ret=$$?; break; }; \\\n+\tdone >$@+ 2>$@.log; \\\n+\tif test $$ret != 0; \\\n+\tthen \\\n+\t\tcat $@.log; \\\n+\t\texit 1; \\\n+\tfi; \\\n+\tmv $@+ $@; \\\n \tif test -s $@; \\\n \tthen \\\n \t\techo '    ' SPATCH result: $@; \\\n-- \n2.12.2.920.gc31091ce4\n"},{"id":"315684","messageId":"xmqqr31gax0y.fsf@gitster.mtv.corp.google.com","threadId":"45529","inReplyTo":"20170329034105.bfgh4tutgrmjp2lc@sigill.intra.peff.net","subject":"Re: [PATCH 0/18] snprintf cleanups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-29T16:05:33Z","receivedAt":"2017-03-29T16:05:42Z","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 Tue, Mar 28, 2017 at 03:33:48PM -0700, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> > It's a lot of patches, but hopefully they're all pretty straightforward\n>> > to read.\n>> \n>> Yes, quite a lot of changes.  I didn't see anything questionable in\n>> there.\n>> \n>> As to the \"patch-id\" thing, I find the alternate one slightly easier\n>> to read.  Also, exactly because this is not a performance critical\n>> codepath, it may be better if patch_id_add_string() filtered out\n>> whitespaces; that would allow the source to express things in more\n>> natural way, e.g.\n>> \n>> \t\tpatch_id_addf(&ctx, \"new file mode\");\n>> \t\tpatch_id_addf(&ctx, \"%06o\", p->two->mode);\n>> \t\tpatch_id_addf(&ctx, \"--- /dev/null\");\n>> \t\tpatch_id_addf(&ctx, \"+++ b/%.*s\", len2, p->two->path);\n>> \n>> Or I may be going overboard by bringing \"addf\" into the mix X-<.\n>\n> I think there are two things going on in your example.\n>\n> One is that obviously patch_id_addf() removes the spaces from the\n> result. But we could do that now by keeping the big strbuf_addf(), and\n> then just walking the result and feeding non-spaces.\n>\n> The second is that your addf means we are back to formatting everything\n> into a buffer again....\n\nYou are right to point out that I was blinded by the ugliness of\nwords stuck together without spaces in between, which was inherited\nfrom the original code, and failed to see the sole point of this\nseries, which is to remove truncation without adding unnecessary\nallocation and freeing.\n\nThanks for straighten my thinking out.  I think the seeming\nugliness, if it ever becomes a real problem, should be handled\noutside this series after the dust settles.\n\n\n\n"},{"id":"315752","messageId":"20170330062730.ycsok7skrjy5c6en@sigill.intra.peff.net","threadId":"45529","inReplyTo":"xmqqr31gax0y.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/18] snprintf cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-30T06:27:30Z","receivedAt":"2017-03-30T06:27:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 29, 2017 at 09:05:33AM -0700, Junio C Hamano wrote:\n\n> > I think there are two things going on in your example.\n> >\n> > One is that obviously patch_id_addf() removes the spaces from the\n> > result. But we could do that now by keeping the big strbuf_addf(), and\n> > then just walking the result and feeding non-spaces.\n> >\n> > The second is that your addf means we are back to formatting everything\n> > into a buffer again....\n> \n> You are right to point out that I was blinded by the ugliness of\n> words stuck together without spaces in between, which was inherited\n> from the original code, and failed to see the sole point of this\n> series, which is to remove truncation without adding unnecessary\n> allocation and freeing.\n> \n> Thanks for straighten my thinking out.  I think the seeming\n> ugliness, if it ever becomes a real problem, should be handled\n> outside this series after the dust settles.\n\nYeah, the no-spaces thing should almost certainly wait.\n\nThere is still the minor question of whether skipping the strbuf\nentirely is nicer, even if you still have to feed it strings without\nspaces (i.e., what I posted in my initial reply).\n\nI'm OK either with the series I posted, or wrapping up the alternative\nin a commit message.\n\n-Peff\n"},{"id":"315794","messageId":"xmqqr31e65kb.fsf@gitster.mtv.corp.google.com","threadId":"45529","inReplyTo":"20170330062730.ycsok7skrjy5c6en@sigill.intra.peff.net","subject":"Re: [PATCH 0/18] snprintf cleanups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-30T17:24:36Z","receivedAt":"2017-03-30T17:24:43Z","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, Mar 29, 2017 at 09:05:33AM -0700, Junio C Hamano wrote:\n>\n>> > I think there are two things going on in your example.\n>> >\n>> > One is that obviously patch_id_addf() removes the spaces from the\n>> > result. But we could do that now by keeping the big strbuf_addf(), and\n>> > then just walking the result and feeding non-spaces.\n>> >\n>> > The second is that your addf means we are back to formatting everything\n>> > into a buffer again....\n>> \n>> You are right to point out that I was blinded by the ugliness of\n>> words stuck together without spaces in between, which was inherited\n>> from the original code, and failed to see the sole point of this\n>> series, which is to remove truncation without adding unnecessary\n>> allocation and freeing.\n>> \n>> Thanks for straighten my thinking out.  I think the seeming\n>> ugliness, if it ever becomes a real problem, should be handled\n>> outside this series after the dust settles.\n>\n> Yeah, the no-spaces thing should almost certainly wait.\n>\n> There is still the minor question of whether skipping the strbuf\n> entirely is nicer, even if you still have to feed it strings without\n> spaces (i.e., what I posted in my initial reply).\n>\n> I'm OK either with the series I posted, or wrapping up the alternative\n> in a commit message.\n\nI do find the updated one easier to follow (if anything it is more\ncompact); I do not think it is worth a reroll, but it is easy enough\nto replace the patch part of the original with the updated patch and\ntweak \"it's easy to fix by moving to a strbuf\" in its log message to\nsomething like \"But it's easy to eliminate the allocation with a few\nhelper functions, and it makes the result easier to follow\", so I am\ntempted to go that route myself...\n"},{"id":"315804","messageId":"20170330182605.ymsr7bjhfdh4ao6a@sigill.intra.peff.net","threadId":"45529","inReplyTo":"xmqqr31e65kb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/18] snprintf cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-30T18:26:05Z","receivedAt":"2017-03-30T18:26:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 30, 2017 at 10:24:36AM -0700, Junio C Hamano wrote:\n\n> > I'm OK either with the series I posted, or wrapping up the alternative\n> > in a commit message.\n> \n> I do find the updated one easier to follow (if anything it is more\n> compact); I do not think it is worth a reroll, but it is easy enough\n> to replace the patch part of the original with the updated patch and\n> tweak \"it's easy to fix by moving to a strbuf\" in its log message to\n> something like \"But it's easy to eliminate the allocation with a few\n> helper functions, and it makes the result easier to follow\", so I am\n> tempted to go that route myself...\n\nYeah, I think that's all that would be needed. Here it is wrapped up as\na patch:\n\n-- >8 --\nSubject: [PATCH] diff: avoid fixed-size buffer for patch-ids\n\nTo generate a patch id, we format the diff header into a\nfixed-size buffer, and then feed the result to our sha1\ncomputation. The fixed buffer has size '4*PATH_MAX + 20',\nwhich in theory accommodates the four filenames plus some\nextra data. Except:\n\n  1. The filenames may not be constrained to PATH_MAX. The\n     static value may not be a real limit on the current\n     filesystem. Moreover, we may compute patch-ids for\n     names stored only in git, without touching the current\n     filesystem at all.\n\n  2. The 20 bytes is not nearly enough to cover the\n     extra content we put in the buffer.\n\nAs a result, the data we feed to the sha1 computation may be\ntruncated, and it's possible that a commit with a very long\nfilename could erroneously collide in the patch-id space\nwith another commit. For instance, if one commit modified\n\"really-long-filename/foo\" and another modified \"bar\" in the\nsame directory.\n\nIn practice this is unlikely. Because the filenames are\nrepeated, and because there's a single cutoff at the end of\nthe buffer, the offending filename would have to be on the\norder of four times larger than PATH_MAX.\n\nWe could fix this by moving to a strbuf. However, we can\nobserve that the purpose of formatting this in the first\nplace is to feed it to git_SHA1_Update(). So instead, let's\njust feed each part of the formatted string directly. This\nactually ends up more readable, and we can even factor out\nsome duplicated bits from the various conditional branches.\n\nTechnically this may change the output of patch-id for very\nlong filenames, but it's not worth making an exception for\nthis in the --stable output. It was a bug, and one that only\naffected an unlikely set of paths.  And anyway, the exact\nvalue would have varied from platform to platform depending\non the value of PATH_MAX, so there is no \"stable\" value.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c | 68 ++++++++++++++++++++++++++++++++++++------------------------------\n 1 file changed, 37 insertions(+), 31 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 58cb72d7e..92d78e459 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4570,6 +4570,19 @@ static void patch_id_consume(void *priv, char *line, unsigned long len)\n \tdata->patchlen += new_len;\n }\n \n+static void patch_id_add_string(git_SHA_CTX *ctx, const char *str)\n+{\n+\tgit_SHA1_Update(ctx, str, strlen(str));\n+}\n+\n+static void patch_id_add_mode(git_SHA_CTX *ctx, unsigned mode)\n+{\n+\t/* large enough for 2^32 in octal */\n+\tchar buf[12];\n+\tint len = xsnprintf(buf, sizeof(buf), \"%06o\", mode);\n+\tgit_SHA1_Update(ctx, buf, len);\n+}\n+\n /* returns 0 upon success, and writes result into sha1 */\n static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1, int diff_header_only)\n {\n@@ -4577,7 +4590,6 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1,\n \tint i;\n \tgit_SHA_CTX ctx;\n \tstruct patch_id_t data;\n-\tchar buffer[PATH_MAX * 4 + 20];\n \n \tgit_SHA1_Init(&ctx);\n \tmemset(&data, 0, sizeof(struct patch_id_t));\n@@ -4609,36 +4621,30 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1,\n \n \t\tlen1 = remove_space(p->one->path, strlen(p->one->path));\n \t\tlen2 = remove_space(p->two->path, strlen(p->two->path));\n-\t\tif (p->one->mode == 0)\n-\t\t\tlen1 = snprintf(buffer, sizeof(buffer),\n-\t\t\t\t\t\"diff--gita/%.*sb/%.*s\"\n-\t\t\t\t\t\"newfilemode%06o\"\n-\t\t\t\t\t\"---/dev/null\"\n-\t\t\t\t\t\"+++b/%.*s\",\n-\t\t\t\t\tlen1, p->one->path,\n-\t\t\t\t\tlen2, p->two->path,\n-\t\t\t\t\tp->two->mode,\n-\t\t\t\t\tlen2, p->two->path);\n-\t\telse if (p->two->mode == 0)\n-\t\t\tlen1 = snprintf(buffer, sizeof(buffer),\n-\t\t\t\t\t\"diff--gita/%.*sb/%.*s\"\n-\t\t\t\t\t\"deletedfilemode%06o\"\n-\t\t\t\t\t\"---a/%.*s\"\n-\t\t\t\t\t\"+++/dev/null\",\n-\t\t\t\t\tlen1, p->one->path,\n-\t\t\t\t\tlen2, p->two->path,\n-\t\t\t\t\tp->one->mode,\n-\t\t\t\t\tlen1, p->one->path);\n-\t\telse\n-\t\t\tlen1 = snprintf(buffer, sizeof(buffer),\n-\t\t\t\t\t\"diff--gita/%.*sb/%.*s\"\n-\t\t\t\t\t\"---a/%.*s\"\n-\t\t\t\t\t\"+++b/%.*s\",\n-\t\t\t\t\tlen1, p->one->path,\n-\t\t\t\t\tlen2, p->two->path,\n-\t\t\t\t\tlen1, p->one->path,\n-\t\t\t\t\tlen2, p->two->path);\n-\t\tgit_SHA1_Update(&ctx, buffer, len1);\n+\t\tpatch_id_add_string(&ctx, \"diff--git\");\n+\t\tpatch_id_add_string(&ctx, \"a/\");\n+\t\tgit_SHA1_Update(&ctx, p->one->path, len1);\n+\t\tpatch_id_add_string(&ctx, \"b/\");\n+\t\tgit_SHA1_Update(&ctx, p->two->path, len2);\n+\n+\t\tif (p->one->mode == 0) {\n+\t\t\tpatch_id_add_string(&ctx, \"newfilemode\");\n+\t\t\tpatch_id_add_mode(&ctx, p->two->mode);\n+\t\t\tpatch_id_add_string(&ctx, \"---/dev/null\");\n+\t\t\tpatch_id_add_string(&ctx, \"+++b/\");\n+\t\t\tgit_SHA1_Update(&ctx, p->two->path, len2);\n+\t\t} else if (p->two->mode == 0) {\n+\t\t\tpatch_id_add_string(&ctx, \"deletedfilemode\");\n+\t\t\tpatch_id_add_mode(&ctx, p->one->mode);\n+\t\t\tpatch_id_add_string(&ctx, \"---a/\");\n+\t\t\tgit_SHA1_Update(&ctx, p->one->path, len1);\n+\t\t\tpatch_id_add_string(&ctx, \"+++/dev/null\");\n+\t\t} else {\n+\t\t\tpatch_id_add_string(&ctx, \"---a/\");\n+\t\t\tgit_SHA1_Update(&ctx, p->one->path, len1);\n+\t\t\tpatch_id_add_string(&ctx, \"+++b/\");\n+\t\t\tgit_SHA1_Update(&ctx, p->two->path, len2);\n+\t\t}\n \n \t\tif (diff_header_only)\n \t\t\tcontinue;\n-- \n2.12.2.922.g77065305a\n\n"},{"id":"316987","messageId":"xmqq1ssrk1za.fsf@gitster.mtv.corp.google.com","threadId":"45529","inReplyTo":"20170328194629.5ddehsxizajitn3q@sigill.intra.peff.net","subject":"Re: [PATCH 07/18] avoid using fixed PATH_MAX buffers for refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-17T06:00:25Z","receivedAt":"2017-04-17T06:00:34Z","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> diff --git a/builtin/replace.c b/builtin/replace.c\n> index f83e7b8fc..065515bab 100644\n> --- a/builtin/replace.c\n> +++ b/builtin/replace.c\n> @@ -93,26 +93,31 @@ typedef int (*each_replace_name_fn)(const char *name, const char *ref,\n>  static int for_each_replace_name(const char **argv, each_replace_name_fn fn)\n>  {\n>  \tconst char **p, *full_hex;\n> -\tchar ref[PATH_MAX];\n> +\tstruct strbuf ref = STRBUF_INIT;\n> +\tsize_t base_len;\n>  \tint had_error = 0;\n>  \tstruct object_id oid;\n>  \n> +\tstrbuf_addstr(&ref, git_replace_ref_base);\n> +\tbase_len = ref.len;\n> +\n>  \tfor (p = argv; *p; p++) {\n>  \t\tif (get_oid(*p, &oid)) {\n>  \t\t\terror(\"Failed to resolve '%s' as a valid ref.\", *p);\n>  \t\t\thad_error = 1;\n>  \t\t\tcontinue;\n>  \t\t}\n> -\t\tfull_hex = oid_to_hex(&oid);\n> -\t\tsnprintf(ref, sizeof(ref), \"%s%s\", git_replace_ref_base, full_hex);\n> -\t\t/* read_ref() may reuse the buffer */\n> -\t\tfull_hex = ref + strlen(git_replace_ref_base);\n> -\t\tif (read_ref(ref, oid.hash)) {\n> +\n> +\t\tstrbuf_setlen(&ref, base_len);\n> +\t\tstrbuf_addstr(&ref, oid_to_hex(&oid));\n> +\t\tfull_hex = ref.buf + base_len;\n> +\n> +\t\tif (read_ref(ref.buf, oid.hash)) {\n>  \t\t\terror(\"replace ref '%s' not found.\", full_hex);\n>  \t\t\thad_error = 1;\n>  \t\t\tcontinue;\n>  \t\t}\n> -\t\tif (fn(full_hex, ref, &oid))\n> +\t\tif (fn(full_hex, ref.buf, &oid))\n>  \t\t\thad_error = 1;\n>  \t}\n>  \treturn had_error;\n\nDon't we need to strbuf_release(&ref) before leaving this function?\n"},{"id":"317067","messageId":"20170418031812.lsi4v5tmxdfuglgo@sigill.intra.peff.net","threadId":"45529","inReplyTo":"xmqq1ssrk1za.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 07/18] avoid using fixed PATH_MAX buffers for refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-18T03:18:13Z","receivedAt":"2017-04-18T03:18:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 16, 2017 at 11:00:25PM -0700, Junio C Hamano wrote:\n\n> > diff --git a/builtin/replace.c b/builtin/replace.c\n> > index f83e7b8fc..065515bab 100644\n> > --- a/builtin/replace.c\n> > +++ b/builtin/replace.c\n> > @@ -93,26 +93,31 @@ typedef int (*each_replace_name_fn)(const char *name, const char *ref,\n> >  static int for_each_replace_name(const char **argv, each_replace_name_fn fn)\n> [...]\n> \n> Don't we need to strbuf_release(&ref) before leaving this function?\n\nYes, good catch. Squashable patch is below.\n\nI'm not sure how I missed that one. I double-checked the other hunks in\nthe patch and they are all fine.\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex 065515bab..ab17668f4 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -120,6 +120,7 @@ static int for_each_replace_name(const char **argv, each_replace_name_fn fn)\n \t\tif (fn(full_hex, ref.buf, &oid))\n \t\t\thad_error = 1;\n \t}\n+\tstrbuf_release(&ref);\n \treturn had_error;\n }\n \n"},{"id":"317082","messageId":"xmqqmvbee2mx.fsf@gitster.mtv.corp.google.com","threadId":"45529","inReplyTo":"20170418031812.lsi4v5tmxdfuglgo@sigill.intra.peff.net","subject":"Re: [PATCH 07/18] avoid using fixed PATH_MAX buffers for refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-18T04:55:02Z","receivedAt":"2017-04-18T04:55:10Z","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 Sun, Apr 16, 2017 at 11:00:25PM -0700, Junio C Hamano wrote:\n>\n>> > diff --git a/builtin/replace.c b/builtin/replace.c\n>> > index f83e7b8fc..065515bab 100644\n>> > --- a/builtin/replace.c\n>> > +++ b/builtin/replace.c\n>> > @@ -93,26 +93,31 @@ typedef int (*each_replace_name_fn)(const char *name, const char *ref,\n>> >  static int for_each_replace_name(const char **argv, each_replace_name_fn fn)\n>> [...]\n>> \n>> Don't we need to strbuf_release(&ref) before leaving this function?\n>\n> Yes, good catch. Squashable patch is below.\n>\n> I'm not sure how I missed that one. I double-checked the other hunks in\n> the patch and they are all fine.\n\nHeh, I do not know how I missed that one while queuing, and while\nadvancing the topic to 'next'.  The third time I read over before\nmerging to 'master' was when I noticed it.\n\n>\n> diff --git a/builtin/replace.c b/builtin/replace.c\n> index 065515bab..ab17668f4 100644\n> --- a/builtin/replace.c\n> +++ b/builtin/replace.c\n> @@ -120,6 +120,7 @@ static int for_each_replace_name(const char **argv, each_replace_name_fn fn)\n>  \t\tif (fn(full_hex, ref.buf, &oid))\n>  \t\t\thad_error = 1;\n>  \t}\n> +\tstrbuf_release(&ref);\n>  \treturn had_error;\n>  }\n>  \n"}]}