{"thread":{"id":"36957","subject":"[PATCH 0/7] cleaning up determine_author_info","startedAt":"2014-06-18T20:19:44Z","lastAt":"2014-06-23T17:21:37Z","messageCount":16,"participants":["Jeff King","Eric Sunshine","Erik Faye-Lund"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"244584","messageId":"20140618201944.GA23238@sigill.intra.peff.net","threadId":"36957","inReplyTo":null,"subject":"[PATCH 0/7] cleaning up determine_author_info","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T20:19:44Z","receivedAt":"2014-06-18T20:19:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is another function I ran across in today's cleanups. The memory\nleak in it has bugged me for a while (even though it's really not a big\ndeal in practice). So this is mostly minor cleanups, but I did find a\nbug in the commit parser.\n\n  [1/7]: commit: provide a function to find a header in a buffer\n  [2/7]: record_author_info: fix memory leak on malformed commit\n  [3/7]: record_author_info: use find_commit_header\n  [4/7]: ident_split: store begin/end pairs on their own struct\n  [5/7]: use strbufs in date functions\n  [6/7]: determine_author_info: reuse parsing functions\n  [7/7]: determine_author_info: stop leaking name/email\n\nI built it on top of my commit-slab topic, as otherwise you get some\ntextual conflicts in determine_author_info. But I notice that Junio's\njk/commit-buffer-length is based on an older master; applying there\nproduces some other minor textual conflicts. I can build it on whatever\nis convenient and handle the conflicts myself. But if\njk/commit-buffer-length is set to graduate soon (as it is marked in WC),\nI can just hold onto this until then.\n\n-Peff\n"},{"id":"244585","messageId":"20140618202737.GA23896@sigill.intra.peff.net","threadId":"36957","inReplyTo":"20140618201944.GA23238@sigill.intra.peff.net","subject":"[PATCH 1/7] commit: provide a function to find a header in a buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T20:27:37Z","receivedAt":"2014-06-18T20:27:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Usually when we parse a commit, we read it line by line and\nhandle each header in a single pass (e.g., in parse_commit\nand parse_commit_header).  Sometimes, however, we only care\nabout extracting a single header. Code in this situation is\nstuck doing an ad-hoc parse of the commit buffer.\n\nLet's provide a reusable function to locate a header within\nthe commit.  The code is modeled after pretty.c's\nget_header, which is used to extract the encoding.\n\nSince some callers may not have the \"struct commit\" to go\nalong with the buffer, we drop that parameter.  The only\nthing lost is a warning for truncated commits, but that's\nOK.  This shouldn't happen in practice, and even if it does,\nthere's no particular reason that this function needs to\ncomplain about it. It either finds the header it was asked\nfor, or it doesn't (and in the latter case, the caller can\ncomplain).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nAs noted in the comments below, I punted on extracting\nmulti-line headers like mergetag, because this function only\nreturns a pointer. It might make sense to wrap it with a\nfunction to pull out a copy of the header, with continuation\nlines connected to each other (that's almost what the static\nget_header does, but I didn't make it public exactly because\nit doesn't handle continuation lines).\n\nThat might be a more natural interface than\nread_commit_extra_header_lines, which pulls out all headers\nexcept ones that are specifically excluded. I haven't looked\nclosely enough. But in either case, that could easily come\non top of this.\n\n commit.c | 23 +++++++++++++++++++++++\n commit.h | 11 +++++++++++\n pretty.c | 33 ++++++---------------------------\n 3 files changed, 40 insertions(+), 27 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 11106fb..d04b525 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1652,3 +1652,26 @@ void print_commit_list(struct commit_list *list,\n \t\tprintf(format, sha1_to_hex(list->item->object.sha1));\n \t}\n }\n+\n+const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n+{\n+\tint key_len = strlen(key);\n+\tconst char *line = msg;\n+\n+\twhile (line) {\n+\t\tconst char *eol = strchrnul(line, '\\n'), *next;\n+\n+\t\tif (line == eol)\n+\t\t\treturn NULL;\n+\t\tnext = *eol ? eol + 1 : NULL;\n+\n+\t\tif (eol - line > key_len &&\n+\t\t    !strncmp(line, key, key_len) &&\n+\t\t    line[key_len] == ' ') {\n+\t\t\t*out_len = eol - line - key_len - 1;\n+\t\t\treturn line + key_len + 1;\n+\t\t}\n+\t\tline = next;\n+\t}\n+\treturn NULL;\n+}\ndiff --git a/commit.h b/commit.h\nindex 61559a9..7c766e9 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -312,6 +312,17 @@ extern struct commit_extra_header *read_commit_extra_headers(struct commit *, co\n \n extern void free_commit_extra_headers(struct commit_extra_header *extra);\n \n+/*\n+ * Search the commit object contents given by \"msg\" for the header \"key\".\n+ * Returns a pointer to the start of the header contents, or NULL. The length\n+ * of the header, up to the first newline, is returned via out_len.\n+ *\n+ * Note that some headers (like mergetag) may be multi-line. It is the caller's\n+ * responsibility to parse further in this case!\n+ */\n+extern const char *find_commit_header(const char *msg, const char *key,\n+\t\t\t\t      size_t *out_len);\n+\n struct merge_remote_desc {\n \tstruct object *obj; /* the named object, could be a tag */\n \tconst char *name;\ndiff --git a/pretty.c b/pretty.c\nindex cc5b45d..6081750 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -548,31 +548,11 @@ static void add_merge_info(const struct pretty_print_context *pp,\n \tstrbuf_addch(sb, '\\n');\n }\n \n-static char *get_header(const struct commit *commit, const char *msg,\n-\t\t\tconst char *key)\n+static char *get_header(const char *msg, const char *key)\n {\n-\tint key_len = strlen(key);\n-\tconst char *line = msg;\n-\n-\twhile (line) {\n-\t\tconst char *eol = strchrnul(line, '\\n'), *next;\n-\n-\t\tif (line == eol)\n-\t\t\treturn NULL;\n-\t\tif (!*eol) {\n-\t\t\twarning(\"malformed commit (header is missing newline): %s\",\n-\t\t\t\tsha1_to_hex(commit->object.sha1));\n-\t\t\tnext = NULL;\n-\t\t} else\n-\t\t\tnext = eol + 1;\n-\t\tif (eol - line > key_len &&\n-\t\t    !strncmp(line, key, key_len) &&\n-\t\t    line[key_len] == ' ') {\n-\t\t\treturn xmemdupz(line + key_len + 1, eol - line - key_len - 1);\n-\t\t}\n-\t\tline = next;\n-\t}\n-\treturn NULL;\n+\tsize_t len;\n+\tconst char *v = find_commit_header(msg, key, &len);\n+\treturn v ? xmemdupz(v, len) : NULL;\n }\n \n static char *replace_encoding_header(char *buf, const char *encoding)\n@@ -618,11 +598,10 @@ const char *logmsg_reencode(const struct commit *commit,\n \n \tif (!output_encoding || !*output_encoding) {\n \t\tif (commit_encoding)\n-\t\t\t*commit_encoding =\n-\t\t\t\tget_header(commit, msg, \"encoding\");\n+\t\t\t*commit_encoding = get_header(msg, \"encoding\");\n \t\treturn msg;\n \t}\n-\tencoding = get_header(commit, msg, \"encoding\");\n+\tencoding = get_header(msg, \"encoding\");\n \tif (commit_encoding)\n \t\t*commit_encoding = encoding;\n \tuse_encoding = encoding ? encoding : utf8;\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244586","messageId":"20140618202849.GB23896@sigill.intra.peff.net","threadId":"36957","inReplyTo":"20140618201944.GA23238@sigill.intra.peff.net","subject":"[PATCH 2/7] record_author_info: fix memory leak on malformed commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T20:28:49Z","receivedAt":"2014-06-18T20:28:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we hit the end-of-header without finding an \"author\"\nline, we just return from the function. We should jump to\nthe fail_exit path to clean up the buffer that we may have\nallocated.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n commit.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/commit.c b/commit.c\nindex d04b525..0c40cfa 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -617,7 +617,7 @@ static void record_author_date(struct author_date_slab *author_date,\n \t\tident_line = skip_prefix(buf, \"author \");\n \t\tif (!ident_line) {\n \t\t\tif (!line_end[0] || line_end[1] == '\\n')\n-\t\t\t\treturn; /* end of header */\n+\t\t\t\tgoto fail_exit; /* end of header */\n \t\t\tcontinue;\n \t\t}\n \t\tif (split_ident_line(&ident,\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244587","messageId":"20140618202927.GC23896@sigill.intra.peff.net","threadId":"36957","inReplyTo":"20140618201944.GA23238@sigill.intra.peff.net","subject":"[PATCH 3/7] record_author_info: use find_commit_header","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T20:29:27Z","receivedAt":"2014-06-18T20:29:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This saves us some manual parsing and makes the code more\nreadable.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI suspect there are other sites which could use this helper, too; I\ndidn't do an exhaustive search.\n\n commit.c | 23 ++++++++---------------\n 1 file changed, 8 insertions(+), 15 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 0c40cfa..c33431c 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -606,26 +606,19 @@ define_commit_slab(author_date_slab, unsigned long);\n static void record_author_date(struct author_date_slab *author_date,\n \t\t\t       struct commit *commit)\n {\n-\tconst char *buf, *line_end, *ident_line;\n \tconst char *buffer = get_commit_buffer(commit, NULL);\n \tstruct ident_split ident;\n+\tconst char *ident_line;\n+\tsize_t ident_len;\n \tchar *date_end;\n \tunsigned long date;\n \n-\tfor (buf = buffer; buf; buf = line_end + 1) {\n-\t\tline_end = strchrnul(buf, '\\n');\n-\t\tident_line = skip_prefix(buf, \"author \");\n-\t\tif (!ident_line) {\n-\t\t\tif (!line_end[0] || line_end[1] == '\\n')\n-\t\t\t\tgoto fail_exit; /* end of header */\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (split_ident_line(&ident,\n-\t\t\t\t     ident_line, line_end - ident_line) ||\n-\t\t    !ident.date_begin || !ident.date_end)\n-\t\t\tgoto fail_exit; /* malformed \"author\" line */\n-\t\tbreak;\n-\t}\n+\tident_line = find_commit_header(buffer, \"author\", &ident_len);\n+\tif (!ident_line)\n+\t\tgoto fail_exit; /* no author line */\n+\tif (split_ident_line(&ident, ident_line, ident_len) ||\n+\t    !ident.date_begin || !ident.date_end)\n+\t\tgoto fail_exit; /* malformed \"author\" line */\n \n \tdate = strtoul(ident.date_begin, &date_end, 10);\n \tif (date_end != ident.date_end)\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244589","messageId":"20140618203133.GD23896@sigill.intra.peff.net","threadId":"36957","inReplyTo":"20140618201944.GA23238@sigill.intra.peff.net","subject":"[PATCH 4/7] ident_split: store begin/end pairs on their own struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T20:31:33Z","receivedAt":"2014-06-18T20:31:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we parse an ident line, we end up with several fields,\neach with a begin/end pointer into the buffer, like:\n\n  const char *name_begin;\n  const char *name_end;\n\nThere is nothing except the field names to indicate that\nthey are paired. This makes it annoying to write helper\nfunctions for dealing with the sub-fields, as you have to\npass both sides. Instead, let's move them into a single\nstruct \"name\", with fields \"begin\" and \"end\". This will be\nstored identically, but can be passed as a unit.\n\nWe have to do a mechanical update of \"s/_/./\" at each point\nof use, but other than that, the fields should behave\nidentically.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nSuggestions welcome on the name \"pointer_pair\".\n\nWhile writing this series, I also noticed that it would be more\nconvenient to have a pointer/len combination rather than two pointers.\nYou can convert between them, of course, but I found I was always\nconverting the other way.\n\nI left it this way because it makes the mass-update mechanical (and\nbecause now that I can pass the pair as a unit, I don't have to write\nthe same \"ident->name_begin, ident->name_end - ident->name_begin\" pair\nover and over).\n\n builtin/blame.c         | 16 +++++++-------\n builtin/check-mailmap.c |  8 +++----\n builtin/commit.c        | 26 +++++++++++-----------\n builtin/shortlog.c      |  8 +++----\n cache.h                 | 17 ++++++++-------\n commit.c                |  6 +++---\n ident.c                 | 57 +++++++++++++++++++++++--------------------------\n log-tree.c              |  2 +-\n pretty.c                | 36 +++++++++++++++----------------\n revision.c              | 12 +++++------\n 10 files changed, 93 insertions(+), 95 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 53f43ab..6e6dddb 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1599,19 +1599,19 @@ static void get_ac_line(const char *inbuf, const char *what,\n \t\treturn;\n \t}\n \n-\tnamelen = ident.name_end - ident.name_begin;\n-\tnamebuf = ident.name_begin;\n+\tnamelen = ident.name.end - ident.name.begin;\n+\tnamebuf = ident.name.begin;\n \n-\tmaillen = ident.mail_end - ident.mail_begin;\n-\tmailbuf = ident.mail_begin;\n+\tmaillen = ident.mail.end - ident.mail.begin;\n+\tmailbuf = ident.mail.begin;\n \n-\tif (ident.date_begin && ident.date_end)\n-\t\t*time = strtoul(ident.date_begin, NULL, 10);\n+\tif (ident.date.begin && ident.date.end)\n+\t\t*time = strtoul(ident.date.begin, NULL, 10);\n \telse\n \t\t*time = 0;\n \n-\tif (ident.tz_begin && ident.tz_end)\n-\t\tstrbuf_add(tz, ident.tz_begin, ident.tz_end - ident.tz_begin);\n+\tif (ident.tz.begin && ident.tz.end)\n+\t\tstrbuf_add(tz, ident.tz.begin, ident.tz.end - ident.tz.begin);\n \telse\n \t\tstrbuf_addstr(tz, \"(unknown)\");\n \ndiff --git a/builtin/check-mailmap.c b/builtin/check-mailmap.c\nindex 8f4d809..65dcbc6 100644\n--- a/builtin/check-mailmap.c\n+++ b/builtin/check-mailmap.c\n@@ -23,10 +23,10 @@ static void check_mailmap(struct string_list *mailmap, const char *contact)\n \tif (split_ident_line(&ident, contact, strlen(contact)))\n \t\tdie(_(\"unable to parse contact: %s\"), contact);\n \n-\tname = ident.name_begin;\n-\tnamelen = ident.name_end - ident.name_begin;\n-\tmail = ident.mail_begin;\n-\tmaillen = ident.mail_end - ident.mail_begin;\n+\tname = ident.name.begin;\n+\tnamelen = ident.name.end - ident.name.begin;\n+\tmail = ident.mail.begin;\n+\tmaillen = ident.mail.end - ident.mail.begin;\n \n \tmap_user(mailmap, &mail, &maillen, &name, &namelen);\n \ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 84cec9a..047cc76 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -514,14 +514,14 @@ static void export_one(const char *var, const char *s, const char *e, int hack)\n \n static int sane_ident_split(struct ident_split *person)\n {\n-\tif (!person->name_begin || !person->name_end ||\n-\t    person->name_begin == person->name_end)\n+\tif (!person->name.begin || !person->name.end ||\n+\t    person->name.begin == person->name.end)\n \t\treturn 0; /* no human readable name */\n-\tif (!person->mail_begin || !person->mail_end ||\n-\t    person->mail_begin == person->mail_end)\n+\tif (!person->mail.begin || !person->mail.end ||\n+\t    person->mail.begin == person->mail.end)\n \t\treturn 0; /* no usable mail */\n-\tif (!person->date_begin || !person->date_end ||\n-\t    !person->tz_begin || !person->tz_end)\n+\tif (!person->date.begin || !person->date.end ||\n+\t    !person->tz.begin || !person->tz.end)\n \t\treturn 0;\n \treturn 1;\n }\n@@ -602,9 +602,9 @@ static void determine_author_info(struct strbuf *author_ident)\n \tstrbuf_addstr(author_ident, fmt_ident(name, email, date, IDENT_STRICT));\n \tif (!split_ident_line(&author, author_ident->buf, author_ident->len) &&\n \t    sane_ident_split(&author)) {\n-\t\texport_one(\"GIT_AUTHOR_NAME\", author.name_begin, author.name_end, 0);\n-\t\texport_one(\"GIT_AUTHOR_EMAIL\", author.mail_begin, author.mail_end, 0);\n-\t\texport_one(\"GIT_AUTHOR_DATE\", author.date_begin, author.tz_end, '@');\n+\t\texport_one(\"GIT_AUTHOR_NAME\", author.name.begin, author.name.end, 0);\n+\t\texport_one(\"GIT_AUTHOR_EMAIL\", author.mail.begin, author.mail.end, 0);\n+\t\texport_one(\"GIT_AUTHOR_DATE\", author.date.begin, author.tz.end, '@');\n \t}\n }\n \n@@ -861,8 +861,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\t\t_(\"%s\"\n \t\t\t\t\"Author:    %.*s <%.*s>\"),\n \t\t\t\tident_shown++ ? \"\" : \"\\n\",\n-\t\t\t\t(int)(ai.name_end - ai.name_begin), ai.name_begin,\n-\t\t\t\t(int)(ai.mail_end - ai.mail_begin), ai.mail_begin);\n+\t\t\t\t(int)(ai.name.end - ai.name.begin), ai.name.begin,\n+\t\t\t\t(int)(ai.mail.end - ai.mail.begin), ai.mail.begin);\n \n \t\tif (author_date_is_interesting())\n \t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL,\n@@ -876,8 +876,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\t\t_(\"%s\"\n \t\t\t\t\"Committer: %.*s <%.*s>\"),\n \t\t\t\tident_shown++ ? \"\" : \"\\n\",\n-\t\t\t\t(int)(ci.name_end - ci.name_begin), ci.name_begin,\n-\t\t\t\t(int)(ci.mail_end - ci.mail_begin), ci.mail_begin);\n+\t\t\t\t(int)(ci.name.end - ci.name.begin), ci.name.begin,\n+\t\t\t\t(int)(ci.mail.end - ci.mail.begin), ci.mail.begin);\n \n \t\tif (ident_shown)\n \t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, \"%s\", \"\");\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 4b7e536..e4ddfea 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -44,10 +44,10 @@ static void insert_one_record(struct shortlog *log,\n \tif (split_ident_line(&ident, author, strlen(author)))\n \t\treturn;\n \n-\tnamebuf = ident.name_begin;\n-\tmailbuf = ident.mail_begin;\n-\tnamelen = ident.name_end - ident.name_begin;\n-\tmaillen = ident.mail_end - ident.mail_begin;\n+\tnamebuf = ident.name.begin;\n+\tmailbuf = ident.mail.begin;\n+\tnamelen = ident.name.end - ident.name.begin;\n+\tmaillen = ident.mail.end - ident.mail.begin;\n \n \tmap_user(&log->mailmap, &mailbuf, &maillen, &namebuf, &namelen);\n \tstrbuf_add(&namemailbuf, namebuf, namelen);\ndiff --git a/cache.h b/cache.h\nindex cbe1935..5255661 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1045,15 +1045,16 @@ extern const char *git_editor(void);\n extern const char *git_pager(int stdout_is_tty);\n extern int git_ident_config(const char *, const char *, void *);\n \n+struct pointer_pair {\n+\tconst char *begin;\n+\tconst char *end;\n+};\n+\n struct ident_split {\n-\tconst char *name_begin;\n-\tconst char *name_end;\n-\tconst char *mail_begin;\n-\tconst char *mail_end;\n-\tconst char *date_begin;\n-\tconst char *date_end;\n-\tconst char *tz_begin;\n-\tconst char *tz_end;\n+\tstruct pointer_pair name;\n+\tstruct pointer_pair mail;\n+\tstruct pointer_pair date;\n+\tstruct pointer_pair tz;\n };\n /*\n  * Signals an success with 0, but time part of the result may be NULL\ndiff --git a/commit.c b/commit.c\nindex c33431c..dcf8dce 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -617,11 +617,11 @@ static void record_author_date(struct author_date_slab *author_date,\n \tif (!ident_line)\n \t\tgoto fail_exit; /* no author line */\n \tif (split_ident_line(&ident, ident_line, ident_len) ||\n-\t    !ident.date_begin || !ident.date_end)\n+\t    !ident.date.begin || !ident.date.end)\n \t\tgoto fail_exit; /* malformed \"author\" line */\n \n-\tdate = strtoul(ident.date_begin, &date_end, 10);\n-\tif (date_end != ident.date_end)\n+\tdate = strtoul(ident.date.begin, &date_end, 10);\n+\tif (date_end != ident.date.end)\n \t\tgoto fail_exit; /* malformed date */\n \t*(author_date_slab_at(author_date, commit)) = date;\n \ndiff --git a/ident.c b/ident.c\nindex 1d9b6e7..570fda7 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -206,42 +206,42 @@ int split_ident_line(struct ident_split *split, const char *line, int len)\n \n \tmemset(split, 0, sizeof(*split));\n \n-\tsplit->name_begin = line;\n+\tsplit->name.begin = line;\n \tfor (cp = line; *cp && cp < line + len; cp++)\n \t\tif (*cp == '<') {\n-\t\t\tsplit->mail_begin = cp + 1;\n+\t\t\tsplit->mail.begin = cp + 1;\n \t\t\tbreak;\n \t\t}\n-\tif (!split->mail_begin)\n+\tif (!split->mail.begin)\n \t\treturn status;\n \n-\tfor (cp = split->mail_begin - 2; line <= cp; cp--)\n+\tfor (cp = split->mail.begin - 2; line <= cp; cp--)\n \t\tif (!isspace(*cp)) {\n-\t\t\tsplit->name_end = cp + 1;\n+\t\t\tsplit->name.end = cp + 1;\n \t\t\tbreak;\n \t\t}\n-\tif (!split->name_end) {\n+\tif (!split->name.end) {\n \t\t/* no human readable name */\n-\t\tsplit->name_end = split->name_begin;\n+\t\tsplit->name.end = split->name.begin;\n \t}\n \n-\tfor (cp = split->mail_begin; cp < line + len; cp++)\n+\tfor (cp = split->mail.begin; cp < line + len; cp++)\n \t\tif (*cp == '>') {\n-\t\t\tsplit->mail_end = cp;\n+\t\t\tsplit->mail.end = cp;\n \t\t\tbreak;\n \t\t}\n-\tif (!split->mail_end)\n+\tif (!split->mail.end)\n \t\treturn status;\n \n \t/*\n \t * Look from the end-of-line to find the trailing \">\" of the mail\n-\t * address, even though we should already know it as split->mail_end.\n+\t * address, even though we should already know it as split->mail.end.\n \t * This can help in cases of broken idents with an extra \">\" somewhere\n \t * in the email address.  Note that we are assuming the timestamp will\n \t * never have a \">\" in it.\n \t *\n \t * Note that we will always find some \">\" before going off the front of\n-\t * the string, because will always hit the split->mail_end closing\n+\t * the string, because will always hit the split->mail.end closing\n \t * bracket.\n \t */\n \tfor (cp = line + len - 1; *cp != '>'; cp--)\n@@ -251,27 +251,27 @@ int split_ident_line(struct ident_split *split, const char *line, int len)\n \t\t;\n \tif (line + len <= cp)\n \t\tgoto person_only;\n-\tsplit->date_begin = cp;\n+\tsplit->date.begin = cp;\n \tspan = strspn(cp, \"0123456789\");\n \tif (!span)\n \t\tgoto person_only;\n-\tsplit->date_end = split->date_begin + span;\n-\tfor (cp = split->date_end; cp < line + len && isspace(*cp); cp++)\n+\tsplit->date.end = split->date.begin + span;\n+\tfor (cp = split->date.end; cp < line + len && isspace(*cp); cp++)\n \t\t;\n \tif (line + len <= cp || (*cp != '+' && *cp != '-'))\n \t\tgoto person_only;\n-\tsplit->tz_begin = cp;\n+\tsplit->tz.begin = cp;\n \tspan = strspn(cp + 1, \"0123456789\");\n \tif (!span)\n \t\tgoto person_only;\n-\tsplit->tz_end = split->tz_begin + 1 + span;\n+\tsplit->tz.end = split->tz.begin + 1 + span;\n \treturn 0;\n \n person_only:\n-\tsplit->date_begin = NULL;\n-\tsplit->date_end = NULL;\n-\tsplit->tz_begin = NULL;\n-\tsplit->tz_end = NULL;\n+\tsplit->date.begin = NULL;\n+\tsplit->date.end = NULL;\n+\tsplit->tz.begin = NULL;\n+\tsplit->tz.end = NULL;\n \treturn 0;\n }\n \n@@ -417,15 +417,14 @@ int git_ident_config(const char *var, const char *value, void *data)\n \treturn 0;\n }\n \n-static int buf_cmp(const char *a_begin, const char *a_end,\n-\t\t   const char *b_begin, const char *b_end)\n+static int buf_cmp(const struct pointer_pair *a, const struct pointer_pair *b)\n {\n-\tint a_len = a_end - a_begin;\n-\tint b_len = b_end - b_begin;\n+\tint a_len = a->end - a->begin;\n+\tint b_len = b->end - b->begin;\n \tint min = a_len < b_len ? a_len : b_len;\n \tint cmp;\n \n-\tcmp = memcmp(a_begin, b_begin, min);\n+\tcmp = memcmp(a->begin, b->begin, min);\n \tif (cmp)\n \t\treturn cmp;\n \n@@ -437,11 +436,9 @@ int ident_cmp(const struct ident_split *a,\n {\n \tint cmp;\n \n-\tcmp = buf_cmp(a->mail_begin, a->mail_end,\n-\t\t      b->mail_begin, b->mail_end);\n+\tcmp = buf_cmp(&a->mail, &b->mail);\n \tif (cmp)\n \t\treturn cmp;\n \n-\treturn buf_cmp(a->name_begin, a->name_end,\n-\t\t       b->name_begin, b->name_end);\n+\treturn buf_cmp(&a->name, &b->name);\n }\ndiff --git a/log-tree.c b/log-tree.c\nindex 4447021..385f07f 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -620,7 +620,7 @@ void show_log(struct rev_info *opt)\n \tctx.mailmap = opt->mailmap;\n \tctx.color = opt->diffopt.use_color;\n \tctx.output_encoding = get_log_output_encoding();\n-\tif (opt->from_ident.mail_begin && opt->from_ident.name_begin)\n+\tif (opt->from_ident.mail.begin && opt->from_ident.name.begin)\n \t\tctx.from_ident = &opt->from_ident;\n \tpretty_print_commit(&ctx, commit, &msgbuf);\n \ndiff --git a/pretty.c b/pretty.c\nindex 6081750..83ef5b7 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -399,13 +399,13 @@ const char *show_ident_date(const struct ident_split *ident,\n \tunsigned long date = 0;\n \tlong tz = 0;\n \n-\tif (ident->date_begin && ident->date_end)\n-\t\tdate = strtoul(ident->date_begin, NULL, 10);\n+\tif (ident->date.begin && ident->date.end)\n+\t\tdate = strtoul(ident->date.begin, NULL, 10);\n \tif (date_overflows(date))\n \t\tdate = 0;\n \telse {\n-\t\tif (ident->tz_begin && ident->tz_end)\n-\t\t\ttz = strtol(ident->tz_begin, NULL, 10);\n+\t\tif (ident->tz.begin && ident->tz.end)\n+\t\t\ttz = strtol(ident->tz.begin, NULL, 10);\n \t\tif (tz >= INT_MAX || tz <= INT_MIN)\n \t\t\ttz = 0;\n \t}\n@@ -429,10 +429,10 @@ void pp_user_info(struct pretty_print_context *pp,\n \tif (split_ident_line(&ident, line, line_end - line))\n \t\treturn;\n \n-\tmailbuf = ident.mail_begin;\n-\tmaillen = ident.mail_end - ident.mail_begin;\n-\tnamebuf = ident.name_begin;\n-\tnamelen = ident.name_end - ident.name_begin;\n+\tmailbuf = ident.mail.begin;\n+\tmaillen = ident.mail.end - ident.mail.begin;\n+\tnamebuf = ident.name.begin;\n+\tnamelen = ident.name.end - ident.name.begin;\n \n \tif (pp->mailmap)\n \t\tmap_user(pp->mailmap, &mailbuf, &maillen, &namebuf, &namelen);\n@@ -449,10 +449,10 @@ void pp_user_info(struct pretty_print_context *pp,\n \t\t\tstring_list_append(&pp->in_body_headers,\n \t\t\t\t\t   strbuf_detach(&buf, NULL));\n \n-\t\t\tmailbuf = pp->from_ident->mail_begin;\n-\t\t\tmaillen = pp->from_ident->mail_end - mailbuf;\n-\t\t\tnamebuf = pp->from_ident->name_begin;\n-\t\t\tnamelen = pp->from_ident->name_end - namebuf;\n+\t\t\tmailbuf = pp->from_ident->mail.begin;\n+\t\t\tmaillen = pp->from_ident->mail.end - mailbuf;\n+\t\t\tnamebuf = pp->from_ident->name.begin;\n+\t\t\tnamelen = pp->from_ident->name.end - namebuf;\n \t\t}\n \n \t\tstrbuf_addstr(sb, \"From: \");\n@@ -677,10 +677,10 @@ static size_t format_person_part(struct strbuf *sb, char part,\n \tif (split_ident_line(&s, msg, len) < 0)\n \t\tgoto skip;\n \n-\tname = s.name_begin;\n-\tnamelen = s.name_end - s.name_begin;\n-\tmail = s.mail_begin;\n-\tmaillen = s.mail_end - s.mail_begin;\n+\tname = s.name.begin;\n+\tnamelen = s.name.end - s.name.begin;\n+\tmail = s.mail.begin;\n+\tmaillen = s.mail.end - s.mail.begin;\n \n \tif (part == 'N' || part == 'E') /* mailmap lookup */\n \t\tmailmap_name(&mail, &maillen, &name, &namelen);\n@@ -693,11 +693,11 @@ static size_t format_person_part(struct strbuf *sb, char part,\n \t\treturn placeholder_len;\n \t}\n \n-\tif (!s.date_begin)\n+\tif (!s.date.begin)\n \t\tgoto skip;\n \n \tif (part == 't') {\t/* date, UNIX timestamp */\n-\t\tstrbuf_add(sb, s.date_begin, s.date_end - s.date_begin);\n+\t\tstrbuf_add(sb, s.date.begin, s.date.end - s.date.begin);\n \t\treturn placeholder_len;\n \t}\n \ndiff --git a/revision.c b/revision.c\nindex 9d628c6..c5fbaf8 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2762,10 +2762,10 @@ static int commit_rewrite_person(struct strbuf *buf, const char *what, struct st\n \tif (split_ident_line(&ident, person, len))\n \t\treturn 0;\n \n-\tmail = ident.mail_begin;\n-\tmaillen = ident.mail_end - ident.mail_begin;\n-\tname = ident.name_begin;\n-\tnamelen = ident.name_end - ident.name_begin;\n+\tmail = ident.mail.begin;\n+\tmaillen = ident.mail.end - ident.mail.begin;\n+\tname = ident.name.begin;\n+\tnamelen = ident.name.end - ident.name.begin;\n \n \tif (map_user(mailmap, &mail, &maillen, &name, &namelen)) {\n \t\tstruct strbuf namemail = STRBUF_INIT;\n@@ -2773,8 +2773,8 @@ static int commit_rewrite_person(struct strbuf *buf, const char *what, struct st\n \t\tstrbuf_addf(&namemail, \"%.*s <%.*s>\",\n \t\t\t    (int)namelen, name, (int)maillen, mail);\n \n-\t\tstrbuf_splice(buf, ident.name_begin - buf->buf,\n-\t\t\t      ident.mail_end - ident.name_begin + 1,\n+\t\tstrbuf_splice(buf, ident.name.begin - buf->buf,\n+\t\t\t      ident.mail.end - ident.name.begin + 1,\n \t\t\t      namemail.buf, namemail.len);\n \n \t\tstrbuf_release(&namemail);\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244590","messageId":"20140618203241.GE23896@sigill.intra.peff.net","threadId":"36957","inReplyTo":"20140618201944.GA23238@sigill.intra.peff.net","subject":"[PATCH 5/7] use strbufs in date functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T20:32:41Z","receivedAt":"2014-06-18T20:32:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Many of the date functions write into fixed-size buffers.\nThis is a minor pain, as we have to take special\nprecautions, and frequently end up copying the result into a\nstrbuf or heap-allocated buffer anyway (for which we\nsometimes use strcpy!).\n\nLet's instead teach parse_date, datestamp, etc to write to a\nstrbuf. The obvious downside is that we might need to\nperform a heap allocation where we otherwise would not need\nto. However, it turns out that the only two new allocations\nrequired are:\n\n  1. In test-date.c, where we don't care about efficiency.\n\n  2. In determine_author_info, which is not performance\n     critical (and where the use of a strbuf will help later\n     refactoring).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI don't think the strcpys are a problem in practice, because we're\ntypically writing fixed-size output generated by parse_date. But I\ndidn't analyze it too deeply, so you might be able to cause shenanigans\nif you can impact GIT_AUTHOR_DATE or something.\n\n builtin/commit.c | 20 ++++++++++----------\n cache.h          |  4 ++--\n date.c           | 13 +++++++------\n fast-import.c    | 20 +++++++++-----------\n ident.c          | 26 +++++++++++---------------\n test-date.c      | 10 ++++++----\n 6 files changed, 45 insertions(+), 48 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 047cc76..bf770cf 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -526,19 +526,16 @@ static int sane_ident_split(struct ident_split *person)\n \treturn 1;\n }\n \n-static int parse_force_date(const char *in, char *out, int len)\n+static int parse_force_date(const char *in, struct strbuf *out)\n {\n-\tif (len < 1)\n-\t\treturn -1;\n-\t*out++ = '@';\n-\tlen--;\n+\tstrbuf_addch(out, '@');\n \n-\tif (parse_date(in, out, len) < 0) {\n+\tif (parse_date(in, out) < 0) {\n \t\tint errors = 0;\n \t\tunsigned long t = approxidate_careful(in, &errors);\n \t\tif (errors)\n \t\t\treturn -1;\n-\t\tsnprintf(out, len, \"%lu\", t);\n+\t\tstrbuf_addf(out, \"%lu\", t);\n \t}\n \n \treturn 0;\n@@ -548,7 +545,7 @@ static void determine_author_info(struct strbuf *author_ident)\n {\n \tchar *name, *email, *date;\n \tstruct ident_split author;\n-\tchar date_buf[64];\n+\tstruct strbuf date_buf = STRBUF_INIT;\n \n \tname = getenv(\"GIT_AUTHOR_NAME\");\n \temail = getenv(\"GIT_AUTHOR_EMAIL\");\n@@ -594,9 +591,10 @@ static void determine_author_info(struct strbuf *author_ident)\n \t}\n \n \tif (force_date) {\n-\t\tif (parse_force_date(force_date, date_buf, sizeof(date_buf)))\n+\t\tstrbuf_reset(&date_buf);\n+\t\tif (parse_force_date(force_date, &date_buf))\n \t\t\tdie(_(\"invalid date format: %s\"), force_date);\n-\t\tdate = date_buf;\n+\t\tdate = date_buf.buf;\n \t}\n \n \tstrbuf_addstr(author_ident, fmt_ident(name, email, date, IDENT_STRICT));\n@@ -606,6 +604,8 @@ static void determine_author_info(struct strbuf *author_ident)\n \t\texport_one(\"GIT_AUTHOR_EMAIL\", author.mail.begin, author.mail.end, 0);\n \t\texport_one(\"GIT_AUTHOR_DATE\", author.date.begin, author.tz.end, '@');\n \t}\n+\n+\tstrbuf_release(&date_buf);\n }\n \n static void split_ident_or_die(struct ident_split *id, const struct strbuf *buf)\ndiff --git a/cache.h b/cache.h\nindex 5255661..c7740a8 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1023,10 +1023,10 @@ enum date_mode {\n const char *show_date(unsigned long time, int timezone, enum date_mode mode);\n void show_date_relative(unsigned long time, int tz, const struct timeval *now,\n \t\t\tstruct strbuf *timebuf);\n-int parse_date(const char *date, char *buf, int bufsize);\n+int parse_date(const char *date, struct strbuf *out);\n int parse_date_basic(const char *date, unsigned long *timestamp, int *offset);\n int parse_expiry_date(const char *date, unsigned long *timestamp);\n-void datestamp(char *buf, int bufsize);\n+void datestamp(struct strbuf *out);\n #define approxidate(s) approxidate_careful((s), NULL)\n unsigned long approxidate_careful(const char *, int *);\n unsigned long approxidate_relative(const char *date, const struct timeval *now);\ndiff --git a/date.c b/date.c\nindex 782de95..2c33468 100644\n--- a/date.c\n+++ b/date.c\n@@ -605,7 +605,7 @@ static int match_tz(const char *date, int *offp)\n \treturn end - date;\n }\n \n-static int date_string(unsigned long date, int offset, char *buf, int len)\n+static void date_string(unsigned long date, int offset, struct strbuf *buf)\n {\n \tint sign = '+';\n \n@@ -613,7 +613,7 @@ static int date_string(unsigned long date, int offset, char *buf, int len)\n \t\toffset = -offset;\n \t\tsign = '-';\n \t}\n-\treturn snprintf(buf, len, \"%lu %c%02d%02d\", date, sign, offset / 60, offset % 60);\n+\tstrbuf_addf(buf, \"%lu %c%02d%02d\", date, sign, offset / 60, offset % 60);\n }\n \n /*\n@@ -735,13 +735,14 @@ int parse_expiry_date(const char *date, unsigned long *timestamp)\n \treturn errors;\n }\n \n-int parse_date(const char *date, char *result, int maxlen)\n+int parse_date(const char *date, struct strbuf *result)\n {\n \tunsigned long timestamp;\n \tint offset;\n \tif (parse_date_basic(date, &timestamp, &offset))\n \t\treturn -1;\n-\treturn date_string(timestamp, offset, result, maxlen);\n+\tdate_string(timestamp, offset, result);\n+\treturn 0;\n }\n \n enum date_mode parse_date_format(const char *format)\n@@ -766,7 +767,7 @@ enum date_mode parse_date_format(const char *format)\n \t\tdie(\"unknown date format %s\", format);\n }\n \n-void datestamp(char *buf, int bufsize)\n+void datestamp(struct strbuf *out)\n {\n \ttime_t now;\n \tint offset;\n@@ -776,7 +777,7 @@ void datestamp(char *buf, int bufsize)\n \toffset = tm_to_time_t(localtime(&now)) - now;\n \toffset /= 60;\n \n-\tdate_string(now, offset, buf, bufsize);\n+\tdate_string(now, offset, out);\n }\n \n /*\ndiff --git a/fast-import.c b/fast-import.c\nindex 6707a66..122b916 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1967,7 +1967,7 @@ static int parse_data(struct strbuf *sb, uintmax_t limit, uintmax_t *len_res)\n \treturn 1;\n }\n \n-static int validate_raw_date(const char *src, char *result, int maxlen)\n+static int validate_raw_date(const char *src, struct strbuf *result)\n {\n \tconst char *orig_src = src;\n \tchar *endp;\n@@ -1985,11 +1985,10 @@ static int validate_raw_date(const char *src, char *result, int maxlen)\n \t\treturn -1;\n \n \tnum = strtoul(src + 1, &endp, 10);\n-\tif (errno || endp == src + 1 || *endp || (endp - orig_src) >= maxlen ||\n-\t    1400 < num)\n+\tif (errno || endp == src + 1 || *endp || 1400 < num)\n \t\treturn -1;\n \n-\tstrcpy(result, orig_src);\n+\tstrbuf_addstr(result, orig_src);\n \treturn 0;\n }\n \n@@ -1997,7 +1996,7 @@ static char *parse_ident(const char *buf)\n {\n \tconst char *ltgt;\n \tsize_t name_len;\n-\tchar *ident;\n+\tstruct strbuf ident = STRBUF_INIT;\n \n \t/* ensure there is a space delimiter even if there is no name */\n \tif (*buf == '<')\n@@ -2016,26 +2015,25 @@ static char *parse_ident(const char *buf)\n \t\tdie(\"Missing space after > in ident string: %s\", buf);\n \tltgt++;\n \tname_len = ltgt - buf;\n-\tident = xmalloc(name_len + 24);\n-\tstrncpy(ident, buf, name_len);\n+\tstrbuf_add(&ident, buf, name_len);\n \n \tswitch (whenspec) {\n \tcase WHENSPEC_RAW:\n-\t\tif (validate_raw_date(ltgt, ident + name_len, 24) < 0)\n+\t\tif (validate_raw_date(ltgt, &ident) < 0)\n \t\t\tdie(\"Invalid raw date \\\"%s\\\" in ident: %s\", ltgt, buf);\n \t\tbreak;\n \tcase WHENSPEC_RFC2822:\n-\t\tif (parse_date(ltgt, ident + name_len, 24) < 0)\n+\t\tif (parse_date(ltgt, &ident) < 0)\n \t\t\tdie(\"Invalid rfc2822 date \\\"%s\\\" in ident: %s\", ltgt, buf);\n \t\tbreak;\n \tcase WHENSPEC_NOW:\n \t\tif (strcmp(\"now\", ltgt))\n \t\t\tdie(\"Date in ident must be 'now': %s\", buf);\n-\t\tdatestamp(ident + name_len, 24);\n+\t\tdatestamp(&ident);\n \t\tbreak;\n \t}\n \n-\treturn ident;\n+\treturn strbuf_detach(&ident, NULL);\n }\n \n static void parse_and_store_blob(\ndiff --git a/ident.c b/ident.c\nindex 570fda7..f4fac4a 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -9,7 +9,7 @@\n \n static struct strbuf git_default_name = STRBUF_INIT;\n static struct strbuf git_default_email = STRBUF_INIT;\n-static char git_default_date[50];\n+static struct strbuf git_default_date = STRBUF_INIT;\n \n #define IDENT_NAME_GIVEN 01\n #define IDENT_MAIL_GIVEN 02\n@@ -129,9 +129,9 @@ const char *ident_default_email(void)\n \n static const char *ident_default_date(void)\n {\n-\tif (!git_default_date[0])\n-\t\tdatestamp(git_default_date, sizeof(git_default_date));\n-\treturn git_default_date;\n+\tif (!git_default_date.len)\n+\t\tdatestamp(&git_default_date);\n+\treturn git_default_date.buf;\n }\n \n static int crud(unsigned char c)\n@@ -292,7 +292,6 @@ const char *fmt_ident(const char *name, const char *email,\n \t\t      const char *date_str, int flag)\n {\n \tstatic struct strbuf ident = STRBUF_INIT;\n-\tchar date[50];\n \tint strict = (flag & IDENT_STRICT);\n \tint want_date = !(flag & IDENT_NO_DATE);\n \tint want_name = !(flag & IDENT_NO_NAME);\n@@ -320,15 +319,6 @@ const char *fmt_ident(const char *name, const char *email,\n \t\tdie(\"unable to auto-detect email address (got '%s')\", email);\n \t}\n \n-\tif (want_date) {\n-\t\tif (date_str && date_str[0]) {\n-\t\t\tif (parse_date(date_str, date, sizeof(date)) < 0)\n-\t\t\t\tdie(\"invalid date format: %s\", date_str);\n-\t\t}\n-\t\telse\n-\t\t\tstrcpy(date, ident_default_date());\n-\t}\n-\n \tstrbuf_reset(&ident);\n \tif (want_name) {\n \t\tstrbuf_addstr_without_crud(&ident, name);\n@@ -339,8 +329,14 @@ const char *fmt_ident(const char *name, const char *email,\n \t\t\tstrbuf_addch(&ident, '>');\n \tif (want_date) {\n \t\tstrbuf_addch(&ident, ' ');\n-\t\tstrbuf_addstr_without_crud(&ident, date);\n+\t\tif (date_str && date_str[0]) {\n+\t\t\tif (parse_date(date_str, &ident) < 0)\n+\t\t\t\tdie(\"invalid date format: %s\", date_str);\n+\t\t}\n+\t\telse\n+\t\t\tstrbuf_addstr(&ident, ident_default_date());\n \t}\n+\n \treturn ident.buf;\n }\n \ndiff --git a/test-date.c b/test-date.c\nindex 10afaab..94a6997 100644\n--- a/test-date.c\n+++ b/test-date.c\n@@ -19,19 +19,21 @@ static void show_dates(char **argv, struct timeval *now)\n \n static void parse_dates(char **argv, struct timeval *now)\n {\n+\tstruct strbuf result = STRBUF_INIT;\n+\n \tfor (; *argv; argv++) {\n-\t\tchar result[100];\n \t\tunsigned long t;\n \t\tint tz;\n \n-\t\tresult[0] = 0;\n-\t\tparse_date(*argv, result, sizeof(result));\n-\t\tif (sscanf(result, \"%lu %d\", &t, &tz) == 2)\n+\t\tstrbuf_reset(&result);\n+\t\tparse_date(*argv, &result);\n+\t\tif (sscanf(result.buf, \"%lu %d\", &t, &tz) == 2)\n \t\t\tprintf(\"%s -> %s\\n\",\n \t\t\t       *argv, show_date(t, tz, DATE_ISO8601));\n \t\telse\n \t\t\tprintf(\"%s -> bad\\n\", *argv);\n \t}\n+\tstrbuf_release(&result);\n }\n \n static void parse_approxidate(char **argv, struct timeval *now)\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244592","messageId":"20140618203539.GF23896@sigill.intra.peff.net","threadId":"36957","inReplyTo":"20140618201944.GA23238@sigill.intra.peff.net","subject":"[PATCH 6/7] determine_author_info: reuse parsing functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T20:35:39Z","receivedAt":"2014-06-18T20:35:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Rather than parsing the header manually to find the \"author\"\nfield, and then parsing its sub-parts, let's use\nfind_commit_header and split_ident_line. This is shorter and\neasier to read, and should do a more careful parsing job.\n\nFor example, the current parser could find the end-of-email\nright-bracket across a newline (for a malformed commit), and\ncalculate a bogus gigantic length for the date (by using\n\"eol - rb\").\n\nAs a bonus, this also plugs a memory leak when we pull the\ndate field from an existing commit (we still leak the name\nand email buffers, which will be fixed in a later commit).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe large buffer comes from wrapping around the negative side of the\nsize_t space.  In theory you could wrap far enough to get a buffer that\nwe can actually allocate (probably only on a 32-bit system), and then\nwe followup by copying \"len\" random bytes into it. I doubt an attacker\ncould get that data out of the program, though, as we then run it\nthrough fmt_ident, which should complain if it's full of garbage.\n\n builtin/commit.c | 61 +++++++++++++++++++++++++++++---------------------------\n 1 file changed, 32 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex bf770cf..62abee0 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -541,6 +541,16 @@ static int parse_force_date(const char *in, struct strbuf *out)\n \treturn 0;\n }\n \n+static void strbuf_add_pair(struct strbuf *buf, const struct pointer_pair *p)\n+{\n+\tstrbuf_add(buf, p->begin, p->end - p->begin);\n+}\n+\n+static char *xmemdupz_pair(const struct pointer_pair *p)\n+{\n+\treturn xmemdupz(p->begin, p->end - p->begin);\n+}\n+\n static void determine_author_info(struct strbuf *author_ident)\n {\n \tchar *name, *email, *date;\n@@ -552,42 +562,35 @@ static void determine_author_info(struct strbuf *author_ident)\n \tdate = getenv(\"GIT_AUTHOR_DATE\");\n \n \tif (author_message) {\n-\t\tconst char *a, *lb, *rb, *eol;\n-\t\tsize_t len;\n+\t\tstruct ident_split ident;\n+\t\tunsigned long len;\n+\t\tconst char *a;\n \n-\t\ta = strstr(author_message_buffer, \"\\nauthor \");\n+\t\ta = find_commit_header(author_message_buffer, \"author\", &len);\n \t\tif (!a)\n-\t\t\tdie(_(\"invalid commit: %s\"), author_message);\n-\n-\t\tlb = strchrnul(a + strlen(\"\\nauthor \"), '<');\n-\t\trb = strchrnul(lb, '>');\n-\t\teol = strchrnul(rb, '\\n');\n-\t\tif (!*lb || !*rb || !*eol)\n-\t\t\tdie(_(\"invalid commit: %s\"), author_message);\n-\n-\t\tif (lb == a + strlen(\"\\nauthor \"))\n-\t\t\t/* \\nauthor <foo@example.com> */\n-\t\t\tname = xcalloc(1, 1);\n-\t\telse\n-\t\t\tname = xmemdupz(a + strlen(\"\\nauthor \"),\n-\t\t\t\t\t(lb - strlen(\" \") -\n-\t\t\t\t\t (a + strlen(\"\\nauthor \"))));\n-\t\temail = xmemdupz(lb + strlen(\"<\"), rb - (lb + strlen(\"<\")));\n-\t\tlen = eol - (rb + strlen(\"> \"));\n-\t\tdate = xmalloc(len + 2);\n-\t\t*date = '@';\n-\t\tmemcpy(date + 1, rb + strlen(\"> \"), len);\n-\t\tdate[len + 1] = '\\0';\n+\t\t\tdie(_(\"commit '%s' lacks author header\"), author_message);\n+\t\tif (split_ident_line(&ident, a, len) < 0)\n+\t\t\tdie(_(\"commit '%s' has malformed author line\"), author_message);\n+\n+\t\tname = xmemdupz_pair(&ident.name);\n+\t\temail = xmemdupz_pair(&ident.mail);\n+\t\tif (ident.date.begin) {\n+\t\t\tstrbuf_reset(&date_buf);\n+\t\t\tstrbuf_addch(&date_buf, '@');\n+\t\t\tstrbuf_add_pair(&date_buf, &ident.date);\n+\t\t\tstrbuf_addch(&date_buf, ' ');\n+\t\t\tstrbuf_add_pair(&date_buf, &ident.tz);\n+\t\t\tdate = date_buf.buf;\n+\t\t}\n \t}\n \n \tif (force_author) {\n-\t\tconst char *lb = strstr(force_author, \" <\");\n-\t\tconst char *rb = strchr(force_author, '>');\n+\t\tstruct ident_split ident;\n \n-\t\tif (!lb || !rb)\n+\t\tif (split_ident_line(&ident, force_author, strlen(force_author)) < 0)\n \t\t\tdie(_(\"malformed --author parameter\"));\n-\t\tname = xstrndup(force_author, lb - force_author);\n-\t\temail = xstrndup(lb + 2, rb - (lb + 2));\n+\t\tname = xmemdupz_pair(&ident.name);\n+\t\temail = xmemdupz_pair(&ident.mail);\n \t}\n \n \tif (force_date) {\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244593","messageId":"20140618203609.GG23896@sigill.intra.peff.net","threadId":"36957","inReplyTo":"20140618201944.GA23238@sigill.intra.peff.net","subject":"[PATCH 7/7] determine_author_info: stop leaking name/email","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-18T20:36:09Z","receivedAt":"2014-06-18T20:36:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we get the author name and email either from an\nexisting commit or from the \"--author\" option, we create a\ncopy of the strings. We cannot just free() these copies,\nsince the same pointers may also be pointing to getenv()\nstorage which we do not own.\n\nInstead, let's treat these the same way as we do the date\nbuffer: keep a strbuf to be released, and point the bare\npointers at the strbuf.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/commit.c | 20 +++++++++++++-------\n 1 file changed, 13 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 62abee0..72beb7f 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -546,16 +546,20 @@ static void strbuf_add_pair(struct strbuf *buf, const struct pointer_pair *p)\n \tstrbuf_add(buf, p->begin, p->end - p->begin);\n }\n \n-static char *xmemdupz_pair(const struct pointer_pair *p)\n+static char *set_pair(struct strbuf *buf, struct pointer_pair *p)\n {\n-\treturn xmemdupz(p->begin, p->end - p->begin);\n+\tstrbuf_reset(buf);\n+\tstrbuf_add_pair(buf, p);\n+\treturn buf->buf;\n }\n \n static void determine_author_info(struct strbuf *author_ident)\n {\n \tchar *name, *email, *date;\n \tstruct ident_split author;\n-\tstruct strbuf date_buf = STRBUF_INIT;\n+\tstruct strbuf name_buf = STRBUF_INIT,\n+\t\t      mail_buf = STRBUF_INIT,\n+\t\t      date_buf = STRBUF_INIT;\n \n \tname = getenv(\"GIT_AUTHOR_NAME\");\n \temail = getenv(\"GIT_AUTHOR_EMAIL\");\n@@ -572,8 +576,8 @@ static void determine_author_info(struct strbuf *author_ident)\n \t\tif (split_ident_line(&ident, a, len) < 0)\n \t\t\tdie(_(\"commit '%s' has malformed author line\"), author_message);\n \n-\t\tname = xmemdupz_pair(&ident.name);\n-\t\temail = xmemdupz_pair(&ident.mail);\n+\t\tname = set_pair(&name_buf, &ident.name);\n+\t\temail = set_pair(&mail_buf, &ident.mail);\n \t\tif (ident.date.begin) {\n \t\t\tstrbuf_reset(&date_buf);\n \t\t\tstrbuf_addch(&date_buf, '@');\n@@ -589,8 +593,8 @@ static void determine_author_info(struct strbuf *author_ident)\n \n \t\tif (split_ident_line(&ident, force_author, strlen(force_author)) < 0)\n \t\t\tdie(_(\"malformed --author parameter\"));\n-\t\tname = xmemdupz_pair(&ident.name);\n-\t\temail = xmemdupz_pair(&ident.mail);\n+\t\tname = set_pair(&name_buf, &ident.name);\n+\t\temail = set_pair(&mail_buf, &ident.mail);\n \t}\n \n \tif (force_date) {\n@@ -608,6 +612,8 @@ static void determine_author_info(struct strbuf *author_ident)\n \t\texport_one(\"GIT_AUTHOR_DATE\", author.date.begin, author.tz.end, '@');\n \t}\n \n+\tstrbuf_release(&name_buf);\n+\tstrbuf_release(&mail_buf);\n \tstrbuf_release(&date_buf);\n }\n \n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244841","messageId":"CAPig+cQuBg0sjZXfkB7c_L=PMJuaPbWZBz9UV2Jbtr3eqJuPcw@mail.gmail.com","threadId":"36957","inReplyTo":"20140618202737.GA23896@sigill.intra.peff.net","subject":"Re: [PATCH 1/7] commit: provide a function to find a header in a buffer","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-23T01:26:44Z","receivedAt":"2014-06-23T01:26:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wednesday, June 18, 2014, Jeff King <peff@peff.net> wrote:\n> Usually when we parse a commit, we read it line by line and\n> handle each header in a single pass (e.g., in parse_commit\n> and parse_commit_header).  Sometimes, however, we only care\n> about extracting a single header. Code in this situation is\n> stuck doing an ad-hoc parse of the commit buffer.\n>\n> Let's provide a reusable function to locate a header within\n> the commit.  The code is modeled after pretty.c's\n> get_header, which is used to extract the encoding.\n>\n> Since some callers may not have the \"struct commit\" to go\n> along with the buffer, we drop that parameter.  The only\n> thing lost is a warning for truncated commits, but that's\n> OK.  This shouldn't happen in practice, and even if it does,\n> there's no particular reason that this function needs to\n> complain about it. It either finds the header it was asked\n> for, or it doesn't (and in the latter case, the caller can\n> complain).\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> diff --git a/commit.c b/commit.c\n> index 11106fb..d04b525 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -1652,3 +1652,26 @@ void print_commit_list(struct commit_list *list,\n>                 printf(format, sha1_to_hex(list->item->object.sha1));\n>         }\n>  }\n> +\n> +const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n> +{\n> +       int key_len = strlen(key);\n> +       const char *line = msg;\n> +\n> +       while (line) {\n> +               const char *eol = strchrnul(line, '\\n'), *next;\n> +\n> +               if (line == eol)\n> +                       return NULL;\n> +               next = *eol ? eol + 1 : NULL;\n> +\n> +               if (eol - line > key_len &&\n> +                   !strncmp(line, key, key_len) &&\n> +                   line[key_len] == ' ') {\n> +                       *out_len = eol - line - key_len - 1;\n> +                       return line + key_len + 1;\n> +               }\n> +               line = next;\n\nThis is already simplified from the original implementation in\nget_header(), but it can be simplified further by dropping 'next',\nwhich is not otherwise used, and assigning 'line' directly:\n\n    line = *eol ? eol + 1 : NULL;\n\n> +       }\n> +       return NULL;\n> +}\n> diff --git a/commit.h b/commit.h\n> index 61559a9..7c766e9 100644\n> --- a/commit.h\n> +++ b/commit.h\n> @@ -312,6 +312,17 @@ extern struct commit_extra_header *read_commit_extra_headers(struct commit *, co\n>\n>  extern void free_commit_extra_headers(struct commit_extra_header *extra);\n>\n> +/*\n> + * Search the commit object contents given by \"msg\" for the header \"key\".\n> + * Returns a pointer to the start of the header contents, or NULL. The length\n> + * of the header, up to the first newline, is returned via out_len.\n> + *\n> + * Note that some headers (like mergetag) may be multi-line. It is the caller's\n> + * responsibility to parse further in this case!\n> + */\n> +extern const char *find_commit_header(const char *msg, const char *key,\n> +                                     size_t *out_len);\n> +\n>  struct merge_remote_desc {\n>         struct object *obj; /* the named object, could be a tag */\n>         const char *name;\n> diff --git a/pretty.c b/pretty.c\n> index cc5b45d..6081750 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -548,31 +548,11 @@ static void add_merge_info(const struct pretty_print_context *pp,\n>         strbuf_addch(sb, '\\n');\n>  }\n>\n> -static char *get_header(const struct commit *commit, const char *msg,\n> -                       const char *key)\n> +static char *get_header(const char *msg, const char *key)\n>  {\n> -       int key_len = strlen(key);\n> -       const char *line = msg;\n> -\n> -       while (line) {\n> -               const char *eol = strchrnul(line, '\\n'), *next;\n> -\n> -               if (line == eol)\n> -                       return NULL;\n> -               if (!*eol) {\n> -                       warning(\"malformed commit (header is missing newline): %s\",\n> -                               sha1_to_hex(commit->object.sha1));\n> -                       next = NULL;\n> -               } else\n> -                       next = eol + 1;\n> -               if (eol - line > key_len &&\n> -                   !strncmp(line, key, key_len) &&\n> -                   line[key_len] == ' ') {\n> -                       return xmemdupz(line + key_len + 1, eol - line - key_len - 1);\n> -               }\n> -               line = next;\n> -       }\n> -       return NULL;\n> +       size_t len;\n> +       const char *v = find_commit_header(msg, key, &len);\n> +       return v ? xmemdupz(v, len) : NULL;\n>  }\n>\n>  static char *replace_encoding_header(char *buf, const char *encoding)\n> @@ -618,11 +598,10 @@ const char *logmsg_reencode(const struct commit *commit,\n>\n>         if (!output_encoding || !*output_encoding) {\n>                 if (commit_encoding)\n> -                       *commit_encoding =\n> -                               get_header(commit, msg, \"encoding\");\n> +                       *commit_encoding = get_header(msg, \"encoding\");\n>                 return msg;\n>         }\n> -       encoding = get_header(commit, msg, \"encoding\");\n> +       encoding = get_header(msg, \"encoding\");\n>         if (commit_encoding)\n>                 *commit_encoding = encoding;\n>         use_encoding = encoding ? encoding : utf8;\n> --\n> 2.0.0.566.gfe3e6b2\n"},{"id":"244842","messageId":"CAPig+cSZfm4fL0xBUCMKU5h8gWeSb736T8Rv62QhREg1F=xzqA@mail.gmail.com","threadId":"36957","inReplyTo":"20140618203133.GD23896@sigill.intra.peff.net","subject":"Re: [PATCH 4/7] ident_split: store begin/end pairs on their own struct","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-23T01:28:29Z","receivedAt":"2014-06-23T01:28:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wednesday, June 18, 2014, Jeff King <peff@peff.net> wrote:\n> Subject: ident_split: store begin/end pairs on their own struct\n\ns/on/in/\n\n> When we parse an ident line, we end up with several fields,\n> each with a begin/end pointer into the buffer, like:\n>\n>   const char *name_begin;\n>   const char *name_end;\n>\n> There is nothing except the field names to indicate that\n> they are paired. This makes it annoying to write helper\n> functions for dealing with the sub-fields, as you have to\n> pass both sides. Instead, let's move them into a single\n> struct \"name\", with fields \"begin\" and \"end\". This will be\n> stored identically, but can be passed as a unit.\n>\n> We have to do a mechanical update of \"s/_/./\" at each point\n> of use, but other than that, the fields should behave\n> identically.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> Suggestions welcome on the name \"pointer_pair\".\n\nstr_segment  ;-)\n\n> While writing this series, I also noticed that it would be more\n> convenient to have a pointer/len combination rather than two pointers.\n> You can convert between them, of course, but I found I was always\n> converting the other way.\n>\n> I left it this way because it makes the mass-update mechanical (and\n> because now that I can pass the pair as a unit, I don't have to write\n> the same \"ident->name_begin, ident->name_end - ident->name_begin\" pair\n> over and over).\n"},{"id":"244851","messageId":"CAPig+cT7mAGaGXZHNEWvZ31acth2wooexZ5s7wWFrJ40rBviYw@mail.gmail.com","threadId":"36957","inReplyTo":"20140618203609.GG23896@sigill.intra.peff.net","subject":"Re: [PATCH 7/7] determine_author_info: stop leaking name/email","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-23T09:28:14Z","receivedAt":"2014-06-23T09:28:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jun 18, 2014 at 4:36 PM, Jeff King <peff@peff.net> wrote:\n> When we get the author name and email either from an\n> existing commit or from the \"--author\" option, we create a\n> copy of the strings. We cannot just free() these copies,\n> since the same pointers may also be pointing to getenv()\n> storage which we do not own.\n>\n> Instead, let's treat these the same way as we do the date\n> buffer: keep a strbuf to be released, and point the bare\n> pointers at the strbuf.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/commit.c | 20 +++++++++++++-------\n>  1 file changed, 13 insertions(+), 7 deletions(-)\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 62abee0..72beb7f 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -546,16 +546,20 @@ static void strbuf_add_pair(struct strbuf *buf, const struct pointer_pair *p)\n>         strbuf_add(buf, p->begin, p->end - p->begin);\n>  }\n>\n> -static char *xmemdupz_pair(const struct pointer_pair *p)\n> +static char *set_pair(struct strbuf *buf, struct pointer_pair *p)\n>  {\n> -       return xmemdupz(p->begin, p->end - p->begin);\n> +       strbuf_reset(buf);\n> +       strbuf_add_pair(buf, p);\n> +       return buf->buf;\n>  }\n>\n>  static void determine_author_info(struct strbuf *author_ident)\n>  {\n>         char *name, *email, *date;\n>         struct ident_split author;\n> -       struct strbuf date_buf = STRBUF_INIT;\n> +       struct strbuf name_buf = STRBUF_INIT,\n> +                     mail_buf = STRBUF_INIT,\n\nnit: The associated 'char *' variable is named \"email\", so perhaps\ns/mail_buf/email_buf/g\n\n> +                     date_buf = STRBUF_INIT;\n>\n>         name = getenv(\"GIT_AUTHOR_NAME\");\n>         email = getenv(\"GIT_AUTHOR_EMAIL\");\n> @@ -572,8 +576,8 @@ static void determine_author_info(struct strbuf *author_ident)\n>                 if (split_ident_line(&ident, a, len) < 0)\n>                         die(_(\"commit '%s' has malformed author line\"), author_message);\n>\n> -               name = xmemdupz_pair(&ident.name);\n> -               email = xmemdupz_pair(&ident.mail);\n> +               name = set_pair(&name_buf, &ident.name);\n> +               email = set_pair(&mail_buf, &ident.mail);\n>                 if (ident.date.begin) {\n>                         strbuf_reset(&date_buf);\n>                         strbuf_addch(&date_buf, '@');\n> @@ -589,8 +593,8 @@ static void determine_author_info(struct strbuf *author_ident)\n>\n>                 if (split_ident_line(&ident, force_author, strlen(force_author)) < 0)\n>                         die(_(\"malformed --author parameter\"));\n> -               name = xmemdupz_pair(&ident.name);\n> -               email = xmemdupz_pair(&ident.mail);\n> +               name = set_pair(&name_buf, &ident.name);\n> +               email = set_pair(&mail_buf, &ident.mail);\n\nDoes the code become too convoluted with these changes? You're now\nmaintaining three 'char *' variables in parallel with three strbuf\nvariables. Is it possible to drop the 'char *' variables and just pass\nthe .buf member of the strbufs to fmt_ident()?\n\nAlternately, you also could solve the leaks by having an envdup() helper:\n\n    static char *envdup(const char *s)\n    {\n        const char *v = getenv(s);\n        return v ? xstrdup(v) : NULL;\n    }\n\n    ...\n    name = envdup(\"GIT_AUTHOR_NAME\");\n    email = envdup(\"GIT_AUTHOR_EMAIL\");\n    ...\n\nAnd then just free() 'name' and 'email' normally.\n\n>         }\n>\n>         if (force_date) {\n> @@ -608,6 +612,8 @@ static void determine_author_info(struct strbuf *author_ident)\n>                 export_one(\"GIT_AUTHOR_DATE\", author.date.begin, author.tz.end, '@');\n>         }\n>\n> +       strbuf_release(&name_buf);\n> +       strbuf_release(&mail_buf);\n>         strbuf_release(&date_buf);\n>  }\n>\n> --\n> 2.0.0.566.gfe3e6b2\n"},{"id":"244852","messageId":"CABPQNSaqGaCnE7jY6yj_0m8iHpU2Y5rt9wAHW=sM_oH9r1qTzA@mail.gmail.com","threadId":"36957","inReplyTo":"CAPig+cT7mAGaGXZHNEWvZ31acth2wooexZ5s7wWFrJ40rBviYw@mail.gmail.com","subject":"Re: [PATCH 7/7] determine_author_info: stop leaking name/email","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2014-06-23T09:33:56Z","receivedAt":"2014-06-23T09:33:56Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Mon, Jun 23, 2014 at 11:28 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Wed, Jun 18, 2014 at 4:36 PM, Jeff King <peff@peff.net> wrote:\n>> When we get the author name and email either from an\n>> existing commit or from the \"--author\" option, we create a\n>> copy of the strings. We cannot just free() these copies,\n>> since the same pointers may also be pointing to getenv()\n>> storage which we do not own.\n>>\n>> Instead, let's treat these the same way as we do the date\n>> buffer: keep a strbuf to be released, and point the bare\n>> pointers at the strbuf.\n>>\n>> Signed-off-by: Jeff King <peff@peff.net>\n>> ---\n>>  builtin/commit.c | 20 +++++++++++++-------\n>>  1 file changed, 13 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/builtin/commit.c b/builtin/commit.c\n>> index 62abee0..72beb7f 100644\n>> --- a/builtin/commit.c\n>> +++ b/builtin/commit.c\n>> @@ -546,16 +546,20 @@ static void strbuf_add_pair(struct strbuf *buf, const struct pointer_pair *p)\n>>         strbuf_add(buf, p->begin, p->end - p->begin);\n>>  }\n>>\n>> -static char *xmemdupz_pair(const struct pointer_pair *p)\n>> +static char *set_pair(struct strbuf *buf, struct pointer_pair *p)\n>>  {\n>> -       return xmemdupz(p->begin, p->end - p->begin);\n>> +       strbuf_reset(buf);\n>> +       strbuf_add_pair(buf, p);\n>> +       return buf->buf;\n>>  }\n>>\n>>  static void determine_author_info(struct strbuf *author_ident)\n>>  {\n>>         char *name, *email, *date;\n>>         struct ident_split author;\n>> -       struct strbuf date_buf = STRBUF_INIT;\n>> +       struct strbuf name_buf = STRBUF_INIT,\n>> +                     mail_buf = STRBUF_INIT,\n>\n> nit: The associated 'char *' variable is named \"email\", so perhaps\n> s/mail_buf/email_buf/g\n>\n>> +                     date_buf = STRBUF_INIT;\n>>\n>>         name = getenv(\"GIT_AUTHOR_NAME\");\n>>         email = getenv(\"GIT_AUTHOR_EMAIL\");\n>> @@ -572,8 +576,8 @@ static void determine_author_info(struct strbuf *author_ident)\n>>                 if (split_ident_line(&ident, a, len) < 0)\n>>                         die(_(\"commit '%s' has malformed author line\"), author_message);\n>>\n>> -               name = xmemdupz_pair(&ident.name);\n>> -               email = xmemdupz_pair(&ident.mail);\n>> +               name = set_pair(&name_buf, &ident.name);\n>> +               email = set_pair(&mail_buf, &ident.mail);\n>>                 if (ident.date.begin) {\n>>                         strbuf_reset(&date_buf);\n>>                         strbuf_addch(&date_buf, '@');\n>> @@ -589,8 +593,8 @@ static void determine_author_info(struct strbuf *author_ident)\n>>\n>>                 if (split_ident_line(&ident, force_author, strlen(force_author)) < 0)\n>>                         die(_(\"malformed --author parameter\"));\n>> -               name = xmemdupz_pair(&ident.name);\n>> -               email = xmemdupz_pair(&ident.mail);\n>> +               name = set_pair(&name_buf, &ident.name);\n>> +               email = set_pair(&mail_buf, &ident.mail);\n>\n> Does the code become too convoluted with these changes? You're now\n> maintaining three 'char *' variables in parallel with three strbuf\n> variables. Is it possible to drop the 'char *' variables and just pass\n> the .buf member of the strbufs to fmt_ident()?\n>\n> Alternately, you also could solve the leaks by having an envdup() helper:\n>\n>     static char *envdup(const char *s)\n>     {\n>         const char *v = getenv(s);\n>         return v ? xstrdup(v) : NULL;\n>     }\n>\n>     ...\n>     name = envdup(\"GIT_AUTHOR_NAME\");\n>     email = envdup(\"GIT_AUTHOR_EMAIL\");\n>     ...\n>\n> And then just free() 'name' and 'email' normally.\n\nThis approach has the added benefit of fixing the case where getenv\nuses a static buffer, like POSIX allows.\n"},{"id":"244853","messageId":"CAPig+cRfTrUgJPV781zyV+m5gbYM9f2z+t=gfTU7grUDtX0mxA@mail.gmail.com","threadId":"36957","inReplyTo":"CABPQNSaqGaCnE7jY6yj_0m8iHpU2Y5rt9wAHW=sM_oH9r1qTzA@mail.gmail.com","subject":"Re: [PATCH 7/7] determine_author_info: stop leaking name/email","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-23T09:48:45Z","receivedAt":"2014-06-23T09:48:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 23, 2014 at 5:33 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:\n> On Mon, Jun 23, 2014 at 11:28 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> On Wed, Jun 18, 2014 at 4:36 PM, Jeff King <peff@peff.net> wrote:\n>>> When we get the author name and email either from an\n>>> existing commit or from the \"--author\" option, we create a\n>>> copy of the strings. We cannot just free() these copies,\n>>> since the same pointers may also be pointing to getenv()\n>>> storage which we do not own.\n>>>\n>>> Instead, let's treat these the same way as we do the date\n>>> buffer: keep a strbuf to be released, and point the bare\n>>> pointers at the strbuf.\n>>>\n>>> Signed-off-by: Jeff King <peff@peff.net>\n>>> ---\n>>>  builtin/commit.c | 20 +++++++++++++-------\n>>>  1 file changed, 13 insertions(+), 7 deletions(-)\n>>>\n>>> diff --git a/builtin/commit.c b/builtin/commit.c\n>>> index 62abee0..72beb7f 100644\n>>> --- a/builtin/commit.c\n>>> +++ b/builtin/commit.c\n>>> @@ -546,16 +546,20 @@ static void strbuf_add_pair(struct strbuf *buf, const struct pointer_pair *p)\n>>>         strbuf_add(buf, p->begin, p->end - p->begin);\n>>>  }\n>>>\n>>> -static char *xmemdupz_pair(const struct pointer_pair *p)\n>>> +static char *set_pair(struct strbuf *buf, struct pointer_pair *p)\n>>>  {\n>>> -       return xmemdupz(p->begin, p->end - p->begin);\n>>> +       strbuf_reset(buf);\n>>> +       strbuf_add_pair(buf, p);\n>>> +       return buf->buf;\n>>>  }\n>>>\n>>>  static void determine_author_info(struct strbuf *author_ident)\n>>>  {\n>>>         char *name, *email, *date;\n>>>         struct ident_split author;\n>>> -       struct strbuf date_buf = STRBUF_INIT;\n>>> +       struct strbuf name_buf = STRBUF_INIT,\n>>> +                     mail_buf = STRBUF_INIT,\n>>\n>> nit: The associated 'char *' variable is named \"email\", so perhaps\n>> s/mail_buf/email_buf/g\n>>\n>>> +                     date_buf = STRBUF_INIT;\n>>>\n>>>         name = getenv(\"GIT_AUTHOR_NAME\");\n>>>         email = getenv(\"GIT_AUTHOR_EMAIL\");\n>>> @@ -572,8 +576,8 @@ static void determine_author_info(struct strbuf *author_ident)\n>>>                 if (split_ident_line(&ident, a, len) < 0)\n>>>                         die(_(\"commit '%s' has malformed author line\"), author_message);\n>>>\n>>> -               name = xmemdupz_pair(&ident.name);\n>>> -               email = xmemdupz_pair(&ident.mail);\n>>> +               name = set_pair(&name_buf, &ident.name);\n>>> +               email = set_pair(&mail_buf, &ident.mail);\n>>>                 if (ident.date.begin) {\n>>>                         strbuf_reset(&date_buf);\n>>>                         strbuf_addch(&date_buf, '@');\n>>> @@ -589,8 +593,8 @@ static void determine_author_info(struct strbuf *author_ident)\n>>>\n>>>                 if (split_ident_line(&ident, force_author, strlen(force_author)) < 0)\n>>>                         die(_(\"malformed --author parameter\"));\n>>> -               name = xmemdupz_pair(&ident.name);\n>>> -               email = xmemdupz_pair(&ident.mail);\n>>> +               name = set_pair(&name_buf, &ident.name);\n>>> +               email = set_pair(&mail_buf, &ident.mail);\n>>\n>> Does the code become too convoluted with these changes? You're now\n>> maintaining three 'char *' variables in parallel with three strbuf\n>> variables. Is it possible to drop the 'char *' variables and just pass\n>> the .buf member of the strbufs to fmt_ident()?\n>>\n>> Alternately, you also could solve the leaks by having an envdup() helper:\n>>\n>>     static char *envdup(const char *s)\n>>     {\n>>         const char *v = getenv(s);\n>>         return v ? xstrdup(v) : NULL;\n>>     }\n>>\n>>     ...\n>>     name = envdup(\"GIT_AUTHOR_NAME\");\n>>     email = envdup(\"GIT_AUTHOR_EMAIL\");\n>>     ...\n>>\n>> And then just free() 'name' and 'email' normally.\n>\n> This approach has the added benefit of fixing the case where getenv\n> uses a static buffer, like POSIX allows.\n\nThat reminds me that I was going to suggest a suitable variation of\nenvdup if Peff wanted to keep the strbufs:\n\n    static void strbuf_env(struct strbuf *s, const char *e)\n    {\n        const char *v = getenv(e);\n        if (v)\n            strbuf_add(s, v);\n    }\n\n(Or add a generalized strbuf_add_gently() to strbuf.{h,c}, if it seems\nlikely to come up more often, which would allow\nstrbuf_add_gently(&name_buf, getenv(\"GIT_AUTHOR_NAME\")) -- with a\nbetter name than \"gently\", of course.)\n"},{"id":"244876","messageId":"20140623164742.GA4838@sigill.intra.peff.net","threadId":"36957","inReplyTo":"CAPig+cQuBg0sjZXfkB7c_L=PMJuaPbWZBz9UV2Jbtr3eqJuPcw@mail.gmail.com","subject":"Re: [PATCH 1/7] commit: provide a function to find a header in a buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-23T16:47:42Z","receivedAt":"2014-06-23T16:47:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 22, 2014 at 09:26:44PM -0400, Eric Sunshine wrote:\n\n> > +               if (line == eol)\n> > +                       return NULL;\n> > +               next = *eol ? eol + 1 : NULL;\n> > +\n> > +               if (eol - line > key_len &&\n> > +                   !strncmp(line, key, key_len) &&\n> > +                   line[key_len] == ' ') {\n> > +                       *out_len = eol - line - key_len - 1;\n> > +                       return line + key_len + 1;\n> > +               }\n> > +               line = next;\n> \n> This is already simplified from the original implementation in\n> get_header(), but it can be simplified further by dropping 'next',\n> which is not otherwise used, and assigning 'line' directly:\n> \n>     line = *eol ? eol + 1 : NULL;\n\nYeah, you're right. Squashed, thanks.\n\n-Peff\n"},{"id":"244879","messageId":"20140623172025.GB4838@sigill.intra.peff.net","threadId":"36957","inReplyTo":"CAPig+cT7mAGaGXZHNEWvZ31acth2wooexZ5s7wWFrJ40rBviYw@mail.gmail.com","subject":"Re: [PATCH 7/7] determine_author_info: stop leaking name/email","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-23T17:20:25Z","receivedAt":"2014-06-23T17:20:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 23, 2014 at 05:28:14AM -0400, Eric Sunshine wrote:\n\n> >  static void determine_author_info(struct strbuf *author_ident)\n> >  {\n> >         char *name, *email, *date;\n> >         struct ident_split author;\n> > -       struct strbuf date_buf = STRBUF_INIT;\n> > +       struct strbuf name_buf = STRBUF_INIT,\n> > +                     mail_buf = STRBUF_INIT,\n> \n> nit: The associated 'char *' variable is named \"email\", so perhaps\n> s/mail_buf/email_buf/g\n\nYeah, you wouldn't believe the number of times I switched back and forth\nwhile writing the patch. The variable is called \"mail\" in \"struct\nident_split\", so you have a mismatch at some point unless we rename the\nlocal pointer.\n\nI avoided doing that to keep the diff smaller, but perhaps it's worth\ndoing.\n\n> >                 if (split_ident_line(&ident, force_author, strlen(force_author)) < 0)\n> >                         die(_(\"malformed --author parameter\"));\n> > -               name = xmemdupz_pair(&ident.name);\n> > -               email = xmemdupz_pair(&ident.mail);\n> > +               name = set_pair(&name_buf, &ident.name);\n> > +               email = set_pair(&mail_buf, &ident.mail);\n> \n> Does the code become too convoluted with these changes? You're now\n> maintaining three 'char *' variables in parallel with three strbuf\n> variables. Is it possible to drop the 'char *' variables and just pass\n> the .buf member of the strbufs to fmt_ident()?\n\nYeah, I agree it is getting a bit complicated (I tried to introduce\nhelpers like set_pair to at least keep the per-variable work down to a\nsingle line in each instance).\n\nIt unfortunately doesn't work to just pass the \".buf\" of each strbuf. We\ncare about the distinction between the empty string and NULL here (the\nlatter will cause fmt_ident to use the default ident).\n\n> Alternately, you also could solve the leaks by having an envdup() helper:\n> \n>     static char *envdup(const char *s)\n>     {\n>         const char *v = getenv(s);\n>         return v ? xstrdup(v) : NULL;\n>     }\n> \n>     ...\n>     name = envdup(\"GIT_AUTHOR_NAME\");\n>     email = envdup(\"GIT_AUTHOR_EMAIL\");\n>     ...\n> \n> And then just free() 'name' and 'email' normally.\n\nYeah, I also considered that. You end up having to free() before\nre-assigning throughout the function, though that is not much worse than\nhaving to strbuf_reset() before re-adding (the reset is hidden in\nset_pair, but we could similarly abstract it).\n\nHere's what that looks like (this converts date_buf away, too, to avoid\nrelying on getenv()'s static return value):\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 62abee0..35045ca 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -551,15 +551,26 @@ static char *xmemdupz_pair(const struct pointer_pair *p)\n \treturn xmemdupz(p->begin, p->end - p->begin);\n }\n \n+static void set_ident_var(char **buf, char *val)\n+{\n+\tfree(*buf);\n+\t*buf = val;\n+}\n+\n+static char *envdup(const char *var)\n+{\n+\tconst char *val = getenv(var);\n+\treturn val ? xstrdup(val) : NULL;\n+}\n+\n static void determine_author_info(struct strbuf *author_ident)\n {\n \tchar *name, *email, *date;\n \tstruct ident_split author;\n-\tstruct strbuf date_buf = STRBUF_INIT;\n \n-\tname = getenv(\"GIT_AUTHOR_NAME\");\n-\temail = getenv(\"GIT_AUTHOR_EMAIL\");\n-\tdate = getenv(\"GIT_AUTHOR_DATE\");\n+\tname = envdup(\"GIT_AUTHOR_NAME\");\n+\temail = envdup(\"GIT_AUTHOR_EMAIL\");\n+\tdate = envdup(\"GIT_AUTHOR_DATE\");\n \n \tif (author_message) {\n \t\tstruct ident_split ident;\n@@ -572,15 +583,15 @@ static void determine_author_info(struct strbuf *author_ident)\n \t\tif (split_ident_line(&ident, a, len) < 0)\n \t\t\tdie(_(\"commit '%s' has malformed author line\"), author_message);\n \n-\t\tname = xmemdupz_pair(&ident.name);\n-\t\temail = xmemdupz_pair(&ident.mail);\n+\t\tset_ident_var(&name, xmemdupz_pair(&ident.name));\n+\t\tset_ident_var(&email, xmemdupz_pair(&ident.mail));\n \t\tif (ident.date.begin) {\n-\t\t\tstrbuf_reset(&date_buf);\n+\t\t\tstruct strbuf date_buf = STRBUF_INIT;\n \t\t\tstrbuf_addch(&date_buf, '@');\n \t\t\tstrbuf_add_pair(&date_buf, &ident.date);\n \t\t\tstrbuf_addch(&date_buf, ' ');\n \t\t\tstrbuf_add_pair(&date_buf, &ident.tz);\n-\t\t\tdate = date_buf.buf;\n+\t\t\tset_ident_var(&date, strbuf_detach(&date_buf, NULL));\n \t\t}\n \t}\n \n@@ -589,15 +600,15 @@ static void determine_author_info(struct strbuf *author_ident)\n \n \t\tif (split_ident_line(&ident, force_author, strlen(force_author)) < 0)\n \t\t\tdie(_(\"malformed --author parameter\"));\n-\t\tname = xmemdupz_pair(&ident.name);\n-\t\temail = xmemdupz_pair(&ident.mail);\n+\t\tset_ident_var(&name, xmemdupz_pair(&ident.name));\n+\t\tset_ident_var(&email, xmemdupz_pair(&ident.mail));\n \t}\n \n \tif (force_date) {\n-\t\tstrbuf_reset(&date_buf);\n+\t\tstruct strbuf date_buf = STRBUF_INIT;\n \t\tif (parse_force_date(force_date, &date_buf))\n \t\t\tdie(_(\"invalid date format: %s\"), force_date);\n-\t\tdate = date_buf.buf;\n+\t\tset_ident_var(&date, strbuf_detach(&date_buf, NULL));\n \t}\n \n \tstrbuf_addstr(author_ident, fmt_ident(name, email, date, IDENT_STRICT));\n@@ -608,7 +619,9 @@ static void determine_author_info(struct strbuf *author_ident)\n \t\texport_one(\"GIT_AUTHOR_DATE\", author.date.begin, author.tz.end, '@');\n \t}\n \n-\tstrbuf_release(&date_buf);\n+\tfree(name);\n+\tfree(email);\n+\tfree(date);\n }\n \n static void split_ident_or_die(struct ident_split *id, const struct strbuf *buf)\n\n\nI dunno. Maybe the set_ident_var helper is a little too cutesy and\nobfuscating.\n\n-Peff\n"},{"id":"244880","messageId":"20140623172136.GC4838@sigill.intra.peff.net","threadId":"36957","inReplyTo":"CABPQNSaqGaCnE7jY6yj_0m8iHpU2Y5rt9wAHW=sM_oH9r1qTzA@mail.gmail.com","subject":"Re: [PATCH 7/7] determine_author_info: stop leaking name/email","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-23T17:21:37Z","receivedAt":"2014-06-23T17:21:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 23, 2014 at 11:33:56AM +0200, Erik Faye-Lund wrote:\n\n> This approach has the added benefit of fixing the case where getenv\n> uses a static buffer, like POSIX allows.\n\nGood point. I knew we could invalidate the pointer if setenv() was called, but\nI didn't know that another getenv() could (I don't know of any such\nimplementation offhand, but it is not too hard to handle it).\n\n-Peff\n"}]}