{"thread":{"id":"36874","subject":"[PATCH 0/15] store length of commit->buffer","startedAt":"2014-06-09T18:02:36Z","lastAt":"2014-06-13T06:32:11Z","messageCount":60,"participants":["Jeff King","Junio C Hamano","Eric Sunshine","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":15},"messages":[{"id":"243629","messageId":"20140609180236.GA24644@sigill.intra.peff.net","threadId":"36874","inReplyTo":null,"subject":"[PATCH 0/15] store length of commit->buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-09T18:02:36Z","receivedAt":"2014-06-09T18:02:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here's my series to drop \"buffer\" from \"struct commit\" in favor of a\nslab, and then add in a length field. It's a lot of commits, but I tried\nto break it down into readable chunks.\n\n  [01/15]: alloc: include any-object allocations in alloc_report\n  [02/15]: commit: push commit_index update into alloc_commit_node\n  [03/15]: do not create \"struct commit\" with xcalloc\n  [04/15]: logmsg_reencode: return const buffer\n  [05/15]: sequencer: use logmsg_reencode in get_message\n  [06/15]: provide a helper to free commit buffer\n  [07/15]: provide a helper to set the commit buffer\n  [08/15]: provide helpers to access the commit buffer\n  [09/15]: use get_cached_commit_buffer where appropriate\n  [10/15]: use get_commit_buffer to avoid duplicate code\n  [11/15]: convert logmsg_reencode to get_commit_buffer\n  [12/15]: use get_commit_buffer everywhere\n  [13/15]: commit-slab: provide a static initializer\n  [14/15]: commit: convert commit->buffer to a slab\n  [15/15]: commit: record buffer length in cache\n\n-Peff\n"},{"id":"243630","messageId":"20140609180917.GA20315@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140609180236.GA24644@sigill.intra.peff.net","subject":"[PATCH 01/15] alloc: include any-object allocations in alloc_report","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-09T18:09:18Z","receivedAt":"2014-06-09T18:09:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When 2c1cbec (Use proper object allocators for unknown\nobject nodes too, 2007-04-16), added a special \"any_object\"\nallocator, it never taught alloc_report to report on it. To\ndo so we need to add an extra type argument to the REPORT\nmacro, as that commit did for DEFINE_ALLOCATOR.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nMy ulterior motive is that I'm going to add more changes that would\ndisrupt this.  I'd also be happy to just drop alloc_report, as nobody\ncalls it (it was purely for debugging, and I suspect hasn't been used\nsince the initial work in 2007).\n\n alloc.c | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/alloc.c b/alloc.c\nindex f3ee745..38ff7e7 100644\n--- a/alloc.c\n+++ b/alloc.c\n@@ -57,13 +57,14 @@ static void report(const char *name, unsigned int count, size_t size)\n \t\t\tname, count, (uintmax_t) size);\n }\n \n-#define REPORT(name)\t\\\n-    report(#name, name##_allocs, name##_allocs * sizeof(struct name) >> 10)\n+#define REPORT(name, type)\t\\\n+    report(#name, name##_allocs, name##_allocs * sizeof(type) >> 10)\n \n void alloc_report(void)\n {\n-\tREPORT(blob);\n-\tREPORT(tree);\n-\tREPORT(commit);\n-\tREPORT(tag);\n+\tREPORT(blob, struct blob);\n+\tREPORT(tree, struct tree);\n+\tREPORT(commit, struct commit);\n+\tREPORT(tag, struct tag);\n+\tREPORT(object, union any_object);\n }\n-- \n2.0.0.729.g520999f\n"},{"id":"243631","messageId":"20140609180959.GB20315@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140609180236.GA24644@sigill.intra.peff.net","subject":"[PATCH 02/15] commit: push commit_index update into alloc_commit_node","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-09T18:09:59Z","receivedAt":"2014-06-09T18:09:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Whenever we create a commit object via lookup_commit, we\ngive it a unique index to be used with the commit-slab API.\nThe theory is that any \"struct commit\" we create would\nfollow this code path, so any such struct would get an\nindex. However, callers could use alloc_commit_node()\ndirectly (and get multiple commits with index 0).\n\nLet's push the indexing into alloc_commit_node so that it's\nhard for callers to get it wrong.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis will make the alloc_report output a little uglier (it will say\nraw_commit), but I don't think anyone cares. And I wanted to make sure\nthere wasn't an easy way to accidentally call the wrong alloc function\nthat doesn't handle the index.\n\n alloc.c  | 12 ++++++++++--\n commit.c |  2 --\n 2 files changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/alloc.c b/alloc.c\nindex 38ff7e7..eb22a45 100644\n--- a/alloc.c\n+++ b/alloc.c\n@@ -47,10 +47,18 @@ union any_object {\n \n DEFINE_ALLOCATOR(blob, struct blob)\n DEFINE_ALLOCATOR(tree, struct tree)\n-DEFINE_ALLOCATOR(commit, struct commit)\n+DEFINE_ALLOCATOR(raw_commit, struct commit)\n DEFINE_ALLOCATOR(tag, struct tag)\n DEFINE_ALLOCATOR(object, union any_object)\n \n+void *alloc_commit_node(void)\n+{\n+\tstatic int commit_count;\n+\tstruct commit *c = alloc_raw_commit_node();\n+\tc->index = commit_count++;\n+\treturn c;\n+}\n+\n static void report(const char *name, unsigned int count, size_t size)\n {\n \tfprintf(stderr, \"%10s: %8u (%\"PRIuMAX\" kB)\\n\",\n@@ -64,7 +72,7 @@ void alloc_report(void)\n {\n \tREPORT(blob, struct blob);\n \tREPORT(tree, struct tree);\n-\tREPORT(commit, struct commit);\n+\tREPORT(raw_commit, struct commit);\n \tREPORT(tag, struct tag);\n \tREPORT(object, union any_object);\n }\ndiff --git a/commit.c b/commit.c\nindex f479331..21957ee 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -17,7 +17,6 @@ static struct commit_extra_header *read_commit_extra_header_lines(const char *bu\n int save_commit_buffer = 1;\n \n const char *commit_type = \"commit\";\n-static int commit_count;\n \n static struct commit *check_commit(struct object *obj,\n \t\t\t\t   const unsigned char *sha1,\n@@ -64,7 +63,6 @@ struct commit *lookup_commit(const unsigned char *sha1)\n \tstruct object *obj = lookup_object(sha1);\n \tif (!obj) {\n \t\tstruct commit *c = alloc_commit_node();\n-\t\tc->index = commit_count++;\n \t\treturn create_object(sha1, OBJ_COMMIT, c);\n \t}\n \tif (!obj->type)\n-- \n2.0.0.729.g520999f\n"},{"id":"243632","messageId":"20140609181008.GC20315@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140609180236.GA24644@sigill.intra.peff.net","subject":"[PATCH 03/15] do not create \"struct commit\" with xcalloc","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-09T18:10:08Z","receivedAt":"2014-06-09T18:10:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In both blame and merge-recursive, we sometimes create a\n\"fake\" commit struct for convenience (e.g., to represent the\nHEAD state as if we would commit it). By allocating\nourselves rather than using alloc_commit_node, we do not\nproperly set the \"index\" field of the commit. This can\nproduce subtle bugs if we then use commit-slab on the\nresulting commit, as we will share the \"0\" index with\nanother commit.\n\nWe can fix this by using alloc_commit_node() to allocate.\nNote that we cannot free the result, as it is part of our\ncommit allocator. However, both cases were already leaking\nthe allocated commit anyway, so there's nothing to fix up.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/blame.c   | 2 +-\n merge-recursive.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex a52a279..d6056a5 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2286,7 +2286,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \tstruct strbuf msg = STRBUF_INIT;\n \n \ttime(&now);\n-\tcommit = xcalloc(1, sizeof(*commit));\n+\tcommit = alloc_commit_node();\n \tcommit->object.parsed = 1;\n \tcommit->date = now;\n \tcommit->object.type = OBJ_COMMIT;\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex cab16fa..2b37d42 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -40,7 +40,7 @@ static struct tree *shift_tree_object(struct tree *one, struct tree *two,\n \n static struct commit *make_virtual_commit(struct tree *tree, const char *comment)\n {\n-\tstruct commit *commit = xcalloc(1, sizeof(struct commit));\n+\tstruct commit *commit = alloc_commit_node();\n \tstruct merge_remote_desc *desc = xmalloc(sizeof(*desc));\n \n \tdesc->name = comment;\n-- \n2.0.0.729.g520999f\n"},{"id":"243633","messageId":"20140609181017.GD20315@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140609180236.GA24644@sigill.intra.peff.net","subject":"[PATCH 04/15] logmsg_reencode: return const buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-09T18:10:18Z","receivedAt":"2014-06-09T18:10:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The return value from logmsg_reencode may be either a newly\nallocated buffer or a pointer to the existing commit->buffer.\nWe would not want the caller to accidentally free() or\nmodify the latter, so let's mark it as const.  We can cast\naway the constness in logmsg_free, but only once we have\ndetermined that it is a free-able buffer.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/blame.c |  2 +-\n builtin/reset.c |  2 +-\n commit.h        |  8 ++++----\n pretty.c        | 14 +++++++-------\n revision.c      | 12 +++++++++---\n 5 files changed, 22 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex d6056a5..6ce3c6d 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1655,7 +1655,7 @@ static void get_commit_info(struct commit *commit,\n {\n \tint len;\n \tconst char *subject, *encoding;\n-\tchar *message;\n+\tconst char *message;\n \n \tcommit_info_init(ret);\n \ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex f368266..7ebee07 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -93,7 +93,7 @@ static int reset_index(const unsigned char *sha1, int reset_type, int quiet)\n static void print_new_head_line(struct commit *commit)\n {\n \tconst char *hex, *body;\n-\tchar *msg;\n+\tconst char *msg;\n \n \thex = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);\n \tprintf(_(\"HEAD is now at %s\"), hex);\ndiff --git a/commit.h b/commit.h\nindex a9f177b..de57df9 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -115,10 +115,10 @@ struct userformat_want {\n \n extern int has_non_ascii(const char *text);\n struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */\n-extern char *logmsg_reencode(const struct commit *commit,\n-\t\t\t     char **commit_encoding,\n-\t\t\t     const char *output_encoding);\n-extern void logmsg_free(char *msg, const struct commit *commit);\n+extern const char *logmsg_reencode(const struct commit *commit,\n+\t\t\t\t   char **commit_encoding,\n+\t\t\t\t   const char *output_encoding);\n+extern void logmsg_free(const char *msg, const struct commit *commit);\n extern void get_commit_format(const char *arg, struct rev_info *);\n extern const char *format_subject(struct strbuf *sb, const char *msg,\n \t\t\t\t  const char *line_separator);\ndiff --git a/pretty.c b/pretty.c\nindex e1e2cad..d152de2 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -606,9 +606,9 @@ static char *replace_encoding_header(char *buf, const char *encoding)\n \treturn strbuf_detach(&tmp, NULL);\n }\n \n-char *logmsg_reencode(const struct commit *commit,\n-\t\t      char **commit_encoding,\n-\t\t      const char *output_encoding)\n+const char *logmsg_reencode(const struct commit *commit,\n+\t\t\t    char **commit_encoding,\n+\t\t\t    const char *output_encoding)\n {\n \tstatic const char *utf8 = \"UTF-8\";\n \tconst char *use_encoding;\n@@ -687,10 +687,10 @@ char *logmsg_reencode(const struct commit *commit,\n \treturn out ? out : msg;\n }\n \n-void logmsg_free(char *msg, const struct commit *commit)\n+void logmsg_free(const char *msg, const struct commit *commit)\n {\n \tif (msg != commit->buffer)\n-\t\tfree(msg);\n+\t\tfree((void *)msg);\n }\n \n static int mailmap_name(const char **email, size_t *email_len,\n@@ -796,7 +796,7 @@ struct format_commit_context {\n \tstruct signature_check signature_check;\n \tenum flush_type flush_type;\n \tenum trunc_type truncate;\n-\tchar *message;\n+\tconst char *message;\n \tchar *commit_encoding;\n \tsize_t width, indent1, indent2;\n \tint auto_color;\n@@ -1700,7 +1700,7 @@ void pretty_print_commit(struct pretty_print_context *pp,\n \tunsigned long beginning_of_body;\n \tint indent = 4;\n \tconst char *msg;\n-\tchar *reencoded;\n+\tconst char *reencoded;\n \tconst char *encoding;\n \tint need_8bit_cte = pp->need_8bit_cte;\n \ndiff --git a/revision.c b/revision.c\nindex 71e2337..47e5b7a 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2788,7 +2788,7 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n {\n \tint retval;\n \tconst char *encoding;\n-\tchar *message;\n+\tconst char *message;\n \tstruct strbuf buf = STRBUF_INIT;\n \n \tif (!opt->grep_filter.pattern_list && !opt->grep_filter.header_list)\n@@ -2830,12 +2830,18 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n \t\tformat_display_notes(commit->object.sha1, &buf, encoding, 1);\n \t}\n \n-\t/* Find either in the original commit message, or in the temporary */\n+\t/* Find either in the original commit message, or in the temporary.\n+\t * Note that we cast away the constness of \"message\" here. It is\n+\t * const because it may come from the cached commit buffer. That's OK,\n+\t * because we know that it is modifiable heap memory, and that while\n+\t * grep_buffer may modify it for speed, it will restore any\n+\t * changes before returning.\n+\t */\n \tif (buf.len)\n \t\tretval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);\n \telse\n \t\tretval = grep_buffer(&opt->grep_filter,\n-\t\t\t\t     message, strlen(message));\n+\t\t\t\t     (char *)message, strlen(message));\n \tstrbuf_release(&buf);\n \tlogmsg_free(message, commit);\n \treturn retval;\n-- \n2.0.0.729.g520999f\n"},{"id":"243634","messageId":"20140609181039.GE20315@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140609180236.GA24644@sigill.intra.peff.net","subject":"[PATCH 05/15] sequencer: use logmsg_reencode in get_message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-09T18:10:39Z","receivedAt":"2014-06-09T18:10:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This simplifies the code, as logmsg_reencode handles the\nreencoding for us in a single call. It also means we learn\nlogmsg_reencode's trick of pulling the buffer from disk when\ncommit->buffer is NULL (we currently just silently return!).\nIt is doubtful this matters in practice, though, as\nsequencer operations would not generally turn off\nsave_commit_buffer.\n\nNote that we may be fixing a bug here. The existing code\ndoes:\n\n  if (same_encoding(to, from))\n\t  reencode_string(buf, to, from);\n\nThat probably should have been \"!same_encoding\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI didn't actually test for the bug, so it's possible that I'm missing\nsomething clever...\n\n sequencer.c | 45 +++++----------------------------------------\n 1 file changed, 5 insertions(+), 40 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 0a80c58..3fcab4d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -116,39 +116,23 @@ static const char *action_name(const struct replay_opts *opts)\n \treturn opts->action == REPLAY_REVERT ? \"revert\" : \"cherry-pick\";\n }\n \n-static char *get_encoding(const char *message);\n-\n struct commit_message {\n \tchar *parent_label;\n \tconst char *label;\n \tconst char *subject;\n-\tchar *reencoded_message;\n \tconst char *message;\n };\n \n static int get_message(struct commit *commit, struct commit_message *out)\n {\n-\tconst char *encoding;\n \tconst char *abbrev, *subject;\n \tint abbrev_len, subject_len;\n \tchar *q;\n \n-\tif (!commit->buffer)\n-\t\treturn -1;\n-\tencoding = get_encoding(commit->buffer);\n-\tif (!encoding)\n-\t\tencoding = \"UTF-8\";\n \tif (!git_commit_encoding)\n \t\tgit_commit_encoding = \"UTF-8\";\n \n-\tout->reencoded_message = NULL;\n-\tout->message = commit->buffer;\n-\tif (same_encoding(encoding, git_commit_encoding))\n-\t\tout->reencoded_message = reencode_string(commit->buffer,\n-\t\t\t\t\tgit_commit_encoding, encoding);\n-\tif (out->reencoded_message)\n-\t\tout->message = out->reencoded_message;\n-\n+\tout->message = logmsg_reencode(commit, NULL, git_commit_encoding);\n \tabbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);\n \tabbrev_len = strlen(abbrev);\n \n@@ -167,29 +151,10 @@ static int get_message(struct commit *commit, struct commit_message *out)\n \treturn 0;\n }\n \n-static void free_message(struct commit_message *msg)\n+static void free_message(struct commit *commit, struct commit_message *msg)\n {\n \tfree(msg->parent_label);\n-\tfree(msg->reencoded_message);\n-}\n-\n-static char *get_encoding(const char *message)\n-{\n-\tconst char *p = message, *eol;\n-\n-\twhile (*p && *p != '\\n') {\n-\t\tfor (eol = p + 1; *eol && *eol != '\\n'; eol++)\n-\t\t\t; /* do nothing */\n-\t\tif (starts_with(p, \"encoding \")) {\n-\t\t\tchar *result = xmalloc(eol - 8 - p);\n-\t\t\tstrlcpy(result, p + 9, eol - 8 - p);\n-\t\t\treturn result;\n-\t\t}\n-\t\tp = eol;\n-\t\tif (*p == '\\n')\n-\t\t\tp++;\n-\t}\n-\treturn NULL;\n+\tlogmsg_free(msg->message, commit);\n }\n \n static void write_cherry_pick_head(struct commit *commit, const char *pseudoref)\n@@ -489,7 +454,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \tunsigned char head[20];\n \tstruct commit *base, *next, *parent;\n \tconst char *base_label, *next_label;\n-\tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n+\tstruct commit_message msg = { NULL, NULL, NULL, NULL };\n \tchar *defmsg = NULL;\n \tstruct strbuf msgbuf = STRBUF_INIT;\n \tint res, unborn = 0, allow;\n@@ -654,7 +619,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\tres = run_git_commit(defmsg, opts, allow);\n \n leave:\n-\tfree_message(&msg);\n+\tfree_message(commit, &msg);\n \tfree(defmsg);\n \n \treturn res;\n-- \n2.0.0.729.g520999f\n"},{"id":"243635","messageId":"20140609181109.GF20315@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140609180236.GA24644@sigill.intra.peff.net","subject":"[PATCH 06/15] provide a helper to free commit buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-09T18:11:09Z","receivedAt":"2014-06-09T18:11:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This converts two lines into one at each caller. But more\nimportantly, it abstracts the concept of freeing the buffer,\nwhich will make it easier to change later.\n\nNote that we also need to provide a \"detach\" mechanism for\nthe weird case in fsck which passes the buffer back to be\nfreed.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/fsck.c       |  3 +--\n builtin/index-pack.c |  2 +-\n builtin/log.c        |  6 ++----\n builtin/rev-list.c   |  3 +--\n commit.c             | 13 +++++++++++++\n commit.h             | 11 +++++++++++\n 6 files changed, 29 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex fc150c8..8aadca1 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -310,8 +310,7 @@ static int fsck_obj(struct object *obj)\n \tif (obj->type == OBJ_COMMIT) {\n \t\tstruct commit *commit = (struct commit *) obj;\n \n-\t\tfree(commit->buffer);\n-\t\tcommit->buffer = NULL;\n+\t\tfree_commit_buffer(commit);\n \n \t\tif (!commit->parents && show_root)\n \t\t\tprintf(\"root %s\\n\", sha1_to_hex(commit->object.sha1));\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 18f57de..995df39 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -786,7 +786,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\t}\n \t\t\tif (obj->type == OBJ_COMMIT) {\n \t\t\t\tstruct commit *commit = (struct commit *) obj;\n-\t\t\t\tcommit->buffer = NULL;\n+\t\t\t\tdetach_commit_buffer(commit);\n \t\t\t}\n \t\t\tobj->flags |= FLAG_CHECKED;\n \t\t}\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 39e8836..226f8f2 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -349,8 +349,7 @@ static int cmd_log_walk(struct rev_info *rev)\n \t\t\trev->max_count++;\n \t\tif (!rev->reflog_info) {\n \t\t\t/* we allow cycles in reflog ancestry */\n-\t\t\tfree(commit->buffer);\n-\t\t\tcommit->buffer = NULL;\n+\t\t\tfree_commit_buffer(commit);\n \t\t}\n \t\tfree_commit_list(commit->parents);\n \t\tcommit->parents = NULL;\n@@ -1508,8 +1507,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t    reopen_stdout(rev.numbered_files ? NULL : commit, NULL, &rev, quiet))\n \t\t\tdie(_(\"Failed to create output files\"));\n \t\tshown = log_tree_commit(&rev, commit);\n-\t\tfree(commit->buffer);\n-\t\tcommit->buffer = NULL;\n+\t\tfree_commit_buffer(commit);\n \n \t\t/* We put one extra blank line between formatted\n \t\t * patches and this flag is used by log-tree code\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 9f92905..e012ebe 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -173,8 +173,7 @@ static void finish_commit(struct commit *commit, void *data)\n \t\tfree_commit_list(commit->parents);\n \t\tcommit->parents = NULL;\n \t}\n-\tfree(commit->buffer);\n-\tcommit->buffer = NULL;\n+\tfree_commit_buffer(commit);\n }\n \n static void finish_object(struct object *obj,\ndiff --git a/commit.c b/commit.c\nindex 21957ee..d7b6836 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -245,6 +245,19 @@ int unregister_shallow(const unsigned char *sha1)\n \treturn 0;\n }\n \n+void free_commit_buffer(struct commit *commit)\n+{\n+\tfree(commit->buffer);\n+\tcommit->buffer = NULL;\n+}\n+\n+const void *detach_commit_buffer(struct commit *commit)\n+{\n+\tvoid *ret = commit->buffer;\n+\tcommit->buffer = NULL;\n+\treturn ret;\n+}\n+\n int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long size)\n {\n \tconst char *tail = buffer;\ndiff --git a/commit.h b/commit.h\nindex de57df9..daccf46 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -51,6 +51,17 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s\n int parse_commit(struct commit *item);\n void parse_commit_or_die(struct commit *item);\n \n+/*\n+ * Free any cached object buffer associated with the commit.\n+ */\n+void free_commit_buffer(struct commit *);\n+\n+/*\n+ * Disassociate any cached object buffer from the commit, but do not free it.\n+ * The buffer (or NULL, if none) is returned.\n+ */\n+const void *detach_commit_buffer(struct commit *);\n+\n /* Find beginning and length of commit subject. */\n int find_commit_subject(const char *commit_buffer, const char **subject);\n \n-- \n2.0.0.729.g520999f\n"},{"id":"243636","messageId":"20140609181117.GG20315@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140609180236.GA24644@sigill.intra.peff.net","subject":"[PATCH 07/15] provide a helper to set the commit buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-09T18:11:17Z","receivedAt":"2014-06-09T18:11:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Right now this is just a one-liner, but abstracting it will\nmake it easier to change later.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/blame.c | 2 +-\n commit.c        | 7 ++++++-\n commit.h        | 6 ++++++\n object.c        | 2 +-\n 4 files changed, 14 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 6ce3c6d..0af3a18 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2313,7 +2313,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \t\t    ident, ident, path,\n \t\t    (!contents_from ? path :\n \t\t     (!strcmp(contents_from, \"-\") ? \"standard input\" : contents_from)));\n-\tcommit->buffer = strbuf_detach(&msg, NULL);\n+\tset_commit_buffer(commit, strbuf_detach(&msg, NULL));\n \n \tif (!contents_from || strcmp(\"-\", contents_from)) {\n \t\tstruct stat st;\ndiff --git a/commit.c b/commit.c\nindex d7b6836..5cc52e0 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -245,6 +245,11 @@ int unregister_shallow(const unsigned char *sha1)\n \treturn 0;\n }\n \n+void set_commit_buffer(struct commit *commit, void *buffer)\n+{\n+\tcommit->buffer = buffer;\n+}\n+\n void free_commit_buffer(struct commit *commit)\n {\n \tfree(commit->buffer);\n@@ -335,7 +340,7 @@ int parse_commit(struct commit *item)\n \t}\n \tret = parse_commit_buffer(item, buffer, size);\n \tif (save_commit_buffer && !ret) {\n-\t\titem->buffer = buffer;\n+\t\tset_commit_buffer(item, buffer);\n \t\treturn 0;\n \t}\n \tfree(buffer);\ndiff --git a/commit.h b/commit.h\nindex daccf46..5cc0bf3 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -52,6 +52,12 @@ int parse_commit(struct commit *item);\n void parse_commit_or_die(struct commit *item);\n \n /*\n+ * Associate an object buffer with the commit. The ownership of the\n+ * memory is handed over to the commit, and must be free()-able.\n+ */\n+void set_commit_buffer(struct commit *, void *buffer);\n+\n+/*\n  * Free any cached object buffer associated with the commit.\n  */\n void free_commit_buffer(struct commit *);\ndiff --git a/object.c b/object.c\nindex 57a0890..44ca657 100644\n--- a/object.c\n+++ b/object.c\n@@ -198,7 +198,7 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t\n \t\t\tif (parse_commit_buffer(commit, buffer, size))\n \t\t\t\treturn NULL;\n \t\t\tif (!commit->buffer) {\n-\t\t\t\tcommit->buffer = buffer;\n+\t\t\t\tset_commit_buffer(commit, buffer);\n \t\t\t\t*eaten_p = 1;\n \t\t\t}\n \t\t\tobj = &commit->object;\n-- \n2.0.0.729.g520999f\n"},{"id":"243637","messageId":"20140609181158.GH20315@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140609180236.GA24644@sigill.intra.peff.net","subject":"[PATCH 08/15] provide helpers to access the commit buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-09T18:11:59Z","receivedAt":"2014-06-09T18:11:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Many sites look at commit->buffer to get more detailed\ninformation than what is in the parsed commit struct.\nHowever, we sometimes drop commit->buffer to save memory,\nin which case the caller would need to read the object\nafresh. Some callers do this (leading to duplicated code),\nand others do not (which opens the possibility of a segfault\nif somebody else frees the buffer).\n\nLet's provide a pair of helpers, \"get\" and \"unuse\", that let\ncallers easily get the buffer. They will use the cached\nbuffer when possible, and otherwise load from disk using\nread_sha1_file.\n\nNote that we also need to add a \"get_cached\" variant which\nreturns NULL when we do not have a cached buffer. At first\nglance this seems to defeat the purpose of \"get\", which is\nto always provide a return value. However, some log code\npaths actually use the NULL-ness of commit->buffer as a\nboolean flag to decide whether to try to printing the\ncommit. At least for now, we want to continue supporting\nthat use.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n commit.c | 28 ++++++++++++++++++++++++++++\n commit.h | 21 +++++++++++++++++++++\n 2 files changed, 49 insertions(+)\n\ndiff --git a/commit.c b/commit.c\nindex 5cc52e0..b9ecae0 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -250,6 +250,34 @@ void set_commit_buffer(struct commit *commit, void *buffer)\n \tcommit->buffer = buffer;\n }\n \n+const void *get_cached_commit_buffer(const struct commit *commit)\n+{\n+\treturn commit->buffer;\n+}\n+\n+const void *get_commit_buffer(const struct commit *commit)\n+{\n+\tconst void *ret = get_cached_commit_buffer(commit);\n+\tif (!ret) {\n+\t\tenum object_type type;\n+\t\tunsigned long size;\n+\t\tret = read_sha1_file(commit->object.sha1, &type, &size);\n+\t\tif (!ret)\n+\t\t\tdie(\"cannot read commit object %s\",\n+\t\t\t    sha1_to_hex(commit->object.sha1));\n+\t\tif (type != OBJ_COMMIT)\n+\t\t\tdie(\"expected commit for %s, got %s\",\n+\t\t\t    sha1_to_hex(commit->object.sha1), typename(type));\n+\t}\n+\treturn ret;\n+}\n+\n+void unuse_commit_buffer(const struct commit *commit, const void *buffer)\n+{\n+\tif (commit->buffer != buffer)\n+\t\tfree((void *)buffer);\n+}\n+\n void free_commit_buffer(struct commit *commit)\n {\n \tfree(commit->buffer);\ndiff --git a/commit.h b/commit.h\nindex 5cc0bf3..67caf92 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -58,6 +58,27 @@ void parse_commit_or_die(struct commit *item);\n void set_commit_buffer(struct commit *, void *buffer);\n \n /*\n+ * Get any cached object buffer associated with the commit. Returns NULL\n+ * if none. The resulting memory should not be freed.\n+ */\n+const void *get_cached_commit_buffer(const struct commit *);\n+\n+/*\n+ * Get the commit's object contents, either from cache or by reading the object\n+ * from disk. The resulting memory should not be modified, and must be given\n+ * to unuse_commit_buffer when the caller is done.\n+ */\n+const void *get_commit_buffer(const struct commit *);\n+\n+/*\n+ * Tell the commit subsytem that we are done with a particular commit buffer.\n+ * The commit and buffer should be the input and return value, respectively,\n+ * from an earlier call to get_commit_buffer.  The buffer may or may not be\n+ * freed by this call; callers should not access the memory afterwards.\n+ */\n+void unuse_commit_buffer(const struct commit *, const void *buffer);\n+\n+/*\n  * Free any cached object buffer associated with the commit.\n  */\n void free_commit_buffer(struct commit *);\n-- \n2.0.0.729.g520999f\n"},{"id":"243638","messageId":"20140609181233.GI20315@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140609180236.GA24644@sigill.intra.peff.net","subject":"[PATCH 09/15] use get_cached_commit_buffer where appropriate","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-09T18:12:33Z","receivedAt":"2014-06-09T18:12:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Some call sites check commit->buffer to see whether we have\na cached buffer, and if so, do some work with it. In the\nlong run we may want to switch these code paths to make\ntheir decision on a different boolean flag (because checking\nthe cache may get a little more expensive in the future).\nBut for now, we can easily support them by converting the\ncalls to use get_cached_commit_buffer.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/rev-list.c | 2 +-\n log-tree.c         | 2 +-\n object.c           | 2 +-\n 3 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex e012ebe..3fcbf21 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -106,7 +106,7 @@ static void show_commit(struct commit *commit, void *data)\n \telse\n \t\tputchar('\\n');\n \n-\tif (revs->verbose_header && commit->buffer) {\n+\tif (revs->verbose_header && get_cached_commit_buffer(commit)) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tstruct pretty_print_context ctx = {0};\n \t\tctx.abbrev = revs->abbrev;\ndiff --git a/log-tree.c b/log-tree.c\nindex cf2f86c..e9ef8ab 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -588,7 +588,7 @@ void show_log(struct rev_info *opt)\n \t\tshow_mergetag(opt, commit);\n \t}\n \n-\tif (!commit->buffer)\n+\tif (!get_cached_commit_buffer(commit))\n \t\treturn;\n \n \tif (opt->show_notes) {\ndiff --git a/object.c b/object.c\nindex 44ca657..67b6e35 100644\n--- a/object.c\n+++ b/object.c\n@@ -197,7 +197,7 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t\n \t\tif (commit) {\n \t\t\tif (parse_commit_buffer(commit, buffer, size))\n \t\t\t\treturn NULL;\n-\t\t\tif (!commit->buffer) {\n+\t\t\tif (!get_cached_commit_buffer(commit)) {\n \t\t\t\tset_commit_buffer(commit, buffer);\n \t\t\t\t*eaten_p = 1;\n \t\t\t}\n-- \n2.0.0.729.g520999f\n"},{"id":"243639","messageId":"20140609181247.GJ20315@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140609180236.GA24644@sigill.intra.peff.net","subject":"[PATCH 10/15] use get_commit_buffer to avoid duplicate code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-09T18:12:47Z","receivedAt":"2014-06-09T18:12:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"For both of these sites, we already do the \"fallback to\nread_sha1_file\" trick. But we can shorten the code by just\nusing get_commit_buffer.\n\nNote that the error cases are slightly different when\nread_sha1_file fails. get_commit_buffer will die() if the\nobject cannot be loaded, or is a non-commit.\n\nFor get_sha1_oneline, this will almost certainly never\nhappen, as we will have just called parse_object (and if it\ndoes, it's probably worth complaining about).\n\nFor record_author_date, the new behavior is probably better;\nwe notify the user of the error instead of silently ignoring\nit. And because it's used only for sorting by author-date,\nsomebody examining a corrupt can fallback to the regular\ntraversal order.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n commit.c    | 16 +++-------------\n sha1_name.c | 18 ++++--------------\n 2 files changed, 7 insertions(+), 27 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex b9ecae0..d00b818 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -583,22 +583,12 @@ 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-\tchar *buffer = NULL;\n+\tconst char *buffer = get_commit_buffer(commit);\n \tstruct ident_split ident;\n \tchar *date_end;\n \tunsigned long date;\n \n-\tif (!commit->buffer) {\n-\t\tunsigned long size;\n-\t\tenum object_type type;\n-\t\tbuffer = read_sha1_file(commit->object.sha1, &type, &size);\n-\t\tif (!buffer)\n-\t\t\treturn;\n-\t}\n-\n-\tfor (buf = commit->buffer ? commit->buffer : buffer;\n-\t     buf;\n-\t     buf = line_end + 1) {\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@@ -619,7 +609,7 @@ static void record_author_date(struct author_date_slab *author_date,\n \t*(author_date_slab_at(author_date, commit)) = date;\n \n fail_exit:\n-\tfree(buffer);\n+\tunuse_commit_buffer(commit, buffer);\n }\n \n static int compare_commits_by_author_date(const void *a_, const void *b_,\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 2b6322f..0a65d23 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -862,27 +862,17 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1,\n \t\tcommit_list_insert(l->item, &backup);\n \t}\n \twhile (list) {\n-\t\tchar *p, *to_free = NULL;\n+\t\tconst char *p, *buf;\n \t\tstruct commit *commit;\n-\t\tenum object_type type;\n-\t\tunsigned long size;\n \t\tint matches;\n \n \t\tcommit = pop_most_recent_commit(&list, ONELINE_SEEN);\n \t\tif (!parse_object(commit->object.sha1))\n \t\t\tcontinue;\n-\t\tif (commit->buffer)\n-\t\t\tp = commit->buffer;\n-\t\telse {\n-\t\t\tp = read_sha1_file(commit->object.sha1, &type, &size);\n-\t\t\tif (!p)\n-\t\t\t\tcontinue;\n-\t\t\tto_free = p;\n-\t\t}\n-\n-\t\tp = strstr(p, \"\\n\\n\");\n+\t\tbuf = get_commit_buffer(commit);\n+\t\tp = strstr(buf, \"\\n\\n\");\n \t\tmatches = p && !regexec(&regex, p + 2, 0, NULL, 0);\n-\t\tfree(to_free);\n+\t\tunuse_commit_buffer(commit, buf);\n \n \t\tif (matches) {\n \t\t\thashcpy(sha1, commit->object.sha1);\n-- \n2.0.0.729.g520999f\n"},{"id":"243640","messageId":"20140609181312.GK20315@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140609180236.GA24644@sigill.intra.peff.net","subject":"[PATCH 11/15] convert logmsg_reencode to get_commit_buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-09T18:13:13Z","receivedAt":"2014-06-09T18:13:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Like the callsites in the previous commit, logmsg_reencode\nalready falls back to read_sha1_file when necessary.\nHowever, I split its conversion out into its own commit\nbecause it's a bit more complex.\n\nWe return either:\n\n  1. The original commit->buffer\n\n  2. A newly allocated buffer from read_sha1_file\n\n  3. A reencoded buffer (based on either 1 or 2 above).\n\nwhile trying to do as few extra reads/allocations as\npossible. Callers currently free the result with\nlogmsg_free, but we can simplify this by pointing them\nstraight to unuse_commit_buffer. This is a slight layering\nviolation, in that we may be passing a buffer from (3).\nHowever, since the end result is to free() anything except\n(1), a behavior which is unlikely to change, and because\nthis makes the interface much simpler, it's a reasonable\nbending of the rules.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/blame.c |  4 ++--\n builtin/reset.c |  2 +-\n commit.h        |  1 -\n pretty.c        | 40 +++++++++++-----------------------------\n revision.c      |  2 +-\n sequencer.c     |  2 +-\n 6 files changed, 16 insertions(+), 35 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 0af3a18..cde19eb 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1666,7 +1666,7 @@ static void get_commit_info(struct commit *commit,\n \t\t    &ret->author_time, &ret->author_tz);\n \n \tif (!detailed) {\n-\t\tlogmsg_free(message, commit);\n+\t\tunuse_commit_buffer(commit, message);\n \t\treturn;\n \t}\n \n@@ -1680,7 +1680,7 @@ static void get_commit_info(struct commit *commit,\n \telse\n \t\tstrbuf_addf(&ret->summary, \"(%s)\", sha1_to_hex(commit->object.sha1));\n \n-\tlogmsg_free(message, commit);\n+\tunuse_commit_buffer(commit, message);\n }\n \n /*\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 7ebee07..850d532 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -109,7 +109,7 @@ static void print_new_head_line(struct commit *commit)\n \t}\n \telse\n \t\tprintf(\"\\n\");\n-\tlogmsg_free(msg, commit);\n+\tunuse_commit_buffer(commit, msg);\n }\n \n static void update_index_from_diff(struct diff_queue_struct *q,\ndiff --git a/commit.h b/commit.h\nindex 67caf92..27e4fd7 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -156,7 +156,6 @@ struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */\n extern const char *logmsg_reencode(const struct commit *commit,\n \t\t\t\t   char **commit_encoding,\n \t\t\t\t   const char *output_encoding);\n-extern void logmsg_free(const char *msg, const struct commit *commit);\n extern void get_commit_format(const char *arg, struct rev_info *);\n extern const char *format_subject(struct strbuf *sb, const char *msg,\n \t\t\t\t  const char *line_separator);\ndiff --git a/pretty.c b/pretty.c\nindex d152de2..8fd60cd 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -613,22 +613,9 @@ const char *logmsg_reencode(const struct commit *commit,\n \tstatic const char *utf8 = \"UTF-8\";\n \tconst char *use_encoding;\n \tchar *encoding;\n-\tchar *msg = commit->buffer;\n+\tconst char *msg = get_commit_buffer(commit);\n \tchar *out;\n \n-\tif (!msg) {\n-\t\tenum object_type type;\n-\t\tunsigned long size;\n-\n-\t\tmsg = read_sha1_file(commit->object.sha1, &type, &size);\n-\t\tif (!msg)\n-\t\t\tdie(\"Cannot read commit object %s\",\n-\t\t\t    sha1_to_hex(commit->object.sha1));\n-\t\tif (type != OBJ_COMMIT)\n-\t\t\tdie(\"Expected commit for '%s', got %s\",\n-\t\t\t    sha1_to_hex(commit->object.sha1), typename(type));\n-\t}\n-\n \tif (!output_encoding || !*output_encoding) {\n \t\tif (commit_encoding)\n \t\t\t*commit_encoding =\n@@ -652,12 +639,13 @@ const char *logmsg_reencode(const struct commit *commit,\n \t\t * Otherwise, we still want to munge the encoding header in the\n \t\t * result, which will be done by modifying the buffer. If we\n \t\t * are using a fresh copy, we can reuse it. But if we are using\n-\t\t * the cached copy from commit->buffer, we need to duplicate it\n-\t\t * to avoid munging commit->buffer.\n+\t\t * the cached copy from get_commit_buffer, we need to duplicate it\n+\t\t * to avoid munging the cached copy.\n \t\t */\n-\t\tout = msg;\n-\t\tif (out == commit->buffer)\n-\t\t\tout = xstrdup(out);\n+\t\tif (msg == get_cached_commit_buffer(commit))\n+\t\t\tout = xstrdup(msg);\n+\t\telse\n+\t\t\tout = (char *)msg;\n \t}\n \telse {\n \t\t/*\n@@ -667,8 +655,8 @@ const char *logmsg_reencode(const struct commit *commit,\n \t\t * copy, we can free it.\n \t\t */\n \t\tout = reencode_string(msg, output_encoding, use_encoding);\n-\t\tif (out && msg != commit->buffer)\n-\t\t\tfree(msg);\n+\t\tif (out)\n+\t\t\tunuse_commit_buffer(commit, msg);\n \t}\n \n \t/*\n@@ -687,12 +675,6 @@ const char *logmsg_reencode(const struct commit *commit,\n \treturn out ? out : msg;\n }\n \n-void logmsg_free(const char *msg, const struct commit *commit)\n-{\n-\tif (msg != commit->buffer)\n-\t\tfree((void *)msg);\n-}\n-\n static int mailmap_name(const char **email, size_t *email_len,\n \t\t\tconst char **name, size_t *name_len)\n {\n@@ -1531,7 +1513,7 @@ void format_commit_message(const struct commit *commit,\n \t}\n \n \tfree(context.commit_encoding);\n-\tlogmsg_free(context.message, commit);\n+\tunuse_commit_buffer(commit, context.message);\n \tfree(context.signature_check.gpg_output);\n \tfree(context.signature_check.signer);\n }\n@@ -1767,7 +1749,7 @@ void pretty_print_commit(struct pretty_print_context *pp,\n \tif (pp->fmt == CMIT_FMT_EMAIL && sb->len <= beginning_of_body)\n \t\tstrbuf_addch(sb, '\\n');\n \n-\tlogmsg_free(reencoded, commit);\n+\tunuse_commit_buffer(commit, reencoded);\n }\n \n void pp_commit_easy(enum cmit_fmt fmt, const struct commit *commit,\ndiff --git a/revision.c b/revision.c\nindex 47e5b7a..ad5fdf4 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2843,7 +2843,7 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n \t\tretval = grep_buffer(&opt->grep_filter,\n \t\t\t\t     (char *)message, strlen(message));\n \tstrbuf_release(&buf);\n-\tlogmsg_free(message, commit);\n+\tunuse_commit_buffer(commit, message);\n \treturn retval;\n }\n \ndiff --git a/sequencer.c b/sequencer.c\nindex 3fcab4d..4632f7d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -154,7 +154,7 @@ static int get_message(struct commit *commit, struct commit_message *out)\n static void free_message(struct commit *commit, struct commit_message *msg)\n {\n \tfree(msg->parent_label);\n-\tlogmsg_free(msg->message, commit);\n+\tunuse_commit_buffer(commit, msg->message);\n }\n \n static void write_cherry_pick_head(struct commit *commit, const char *pseudoref)\n-- \n2.0.0.729.g520999f\n"},{"id":"243641","messageId":"20140609181323.GL20315@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140609180236.GA24644@sigill.intra.peff.net","subject":"[PATCH 12/15] use get_commit_buffer everywhere","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-09T18:13:23Z","receivedAt":"2014-06-09T18:13:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Each of these sites assumes that commit->buffer is valid.\nSince they would segfault if this was not the case, they are\nlikely to be correct in practice. However, we can\nfuture-proof them by using get_commit_buffer.\n\nAnd as a side effect, we abstract away the final bare uses\nof commit->buffer.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/fast-export.c   |  5 ++++-\n builtin/fmt-merge-msg.c |  5 ++++-\n builtin/log.c           |  7 +++++--\n fsck.c                  | 13 +++++++++++--\n merge-recursive.c       |  4 +++-\n notes-merge.c           |  4 +++-\n sequencer.c             |  4 +++-\n 7 files changed, 33 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex b8d8a3a..7ee5e08 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -279,6 +279,7 @@ static const char *find_encoding(const char *begin, const char *end)\n static void handle_commit(struct commit *commit, struct rev_info *rev)\n {\n \tint saved_output_format = rev->diffopt.output_format;\n+\tconst char *commit_buffer;\n \tconst char *author, *author_end, *committer, *committer_end;\n \tconst char *encoding, *message;\n \tchar *reencoded = NULL;\n@@ -288,7 +289,8 @@ static void handle_commit(struct commit *commit, struct rev_info *rev)\n \trev->diffopt.output_format = DIFF_FORMAT_CALLBACK;\n \n \tparse_commit_or_die(commit);\n-\tauthor = strstr(commit->buffer, \"\\nauthor \");\n+\tcommit_buffer = get_commit_buffer(commit);\n+\tauthor = strstr(commit_buffer, \"\\nauthor \");\n \tif (!author)\n \t\tdie (\"Could not find author in commit %s\",\n \t\t     sha1_to_hex(commit->object.sha1));\n@@ -335,6 +337,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev)\n \t\t\t  ? strlen(message) : 0),\n \t       reencoded ? reencoded : message ? message : \"\");\n \tfree(reencoded);\n+\tunuse_commit_buffer(commit, commit_buffer);\n \n \tfor (i = 0, p = commit->parents; p; p = p->next) {\n \t\tint mark = get_object_mark(&p->item->object);\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex 3906eda..01f6d59 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -230,12 +230,14 @@ static void add_branch_desc(struct strbuf *out, const char *name)\n static void record_person(int which, struct string_list *people,\n \t\t\t  struct commit *commit)\n {\n+\tconst char *buffer;\n \tchar *name_buf, *name, *name_end;\n \tstruct string_list_item *elem;\n \tconst char *field;\n \n \tfield = (which == 'a') ? \"\\nauthor \" : \"\\ncommitter \";\n-\tname = strstr(commit->buffer, field);\n+\tbuffer = get_commit_buffer(commit);\n+\tname = strstr(buffer, field);\n \tif (!name)\n \t\treturn;\n \tname += strlen(field);\n@@ -247,6 +249,7 @@ static void record_person(int which, struct string_list *people,\n \tif (name_end < name)\n \t\treturn;\n \tname_buf = xmemdupz(name, name_end - name + 1);\n+\tunuse_commit_buffer(commit, buffer);\n \n \telem = string_list_lookup(people, name_buf);\n \tif (!elem) {\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 226f8f2..2c74260 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -918,9 +918,12 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \tlog_write_email_headers(rev, head, &pp.subject, &pp.after_subject,\n \t\t\t\t&need_8bit_cte);\n \n-\tfor (i = 0; !need_8bit_cte && i < nr; i++)\n-\t\tif (has_non_ascii(list[i]->buffer))\n+\tfor (i = 0; !need_8bit_cte && i < nr; i++) {\n+\t\tconst char *buf = get_commit_buffer(list[i]);\n+\t\tif (has_non_ascii(buf))\n \t\t\tneed_8bit_cte = 1;\n+\t\tunuse_commit_buffer(list[i], buf);\n+\t}\n \n \tif (!branch_name)\n \t\tbranch_name = find_branch_name(rev);\ndiff --git a/fsck.c b/fsck.c\nindex abed62b..8223780 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -276,9 +276,10 @@ static int fsck_ident(const char **ident, struct object *obj, fsck_error error_f\n \treturn 0;\n }\n \n-static int fsck_commit(struct commit *commit, fsck_error error_func)\n+static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n+\t\t\t      fsck_error error_func)\n {\n-\tconst char *buffer = commit->buffer, *tmp;\n+\tconst char *tmp;\n \tunsigned char tree_sha1[20], sha1[20];\n \tstruct commit_graft *graft;\n \tint parents = 0;\n@@ -336,6 +337,14 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)\n \treturn 0;\n }\n \n+static int fsck_commit(struct commit *commit, fsck_error error_func)\n+{\n+\tconst char *buffer = get_commit_buffer(commit);\n+\tint ret = fsck_commit_buffer(commit, buffer, error_func);\n+\tunuse_commit_buffer(commit, buffer);\n+\treturn ret;\n+}\n+\n static int fsck_tag(struct tag *tag, fsck_error error_func)\n {\n \tstruct object *tagged = tag->tagged;\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 2b37d42..0dd6039 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -190,9 +190,11 @@ static void output_commit_title(struct merge_options *o, struct commit *commit)\n \t\t\tprintf(_(\"(bad commit)\\n\"));\n \t\telse {\n \t\t\tconst char *title;\n-\t\t\tint len = find_commit_subject(commit->buffer, &title);\n+\t\t\tconst char *msg = get_commit_buffer(commit);\n+\t\t\tint len = find_commit_subject(msg, &title);\n \t\t\tif (len)\n \t\t\t\tprintf(\"%.*s\\n\", len, title);\n+\t\t\tunuse_commit_buffer(commit, msg);\n \t\t}\n \t}\n }\ndiff --git a/notes-merge.c b/notes-merge.c\nindex 94a1a8a..7885ab2 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -671,7 +671,8 @@ int notes_merge_commit(struct notes_merge_options *o,\n \tDIR *dir;\n \tstruct dirent *e;\n \tstruct strbuf path = STRBUF_INIT;\n-\tchar *msg = strstr(partial_commit->buffer, \"\\n\\n\");\n+\tconst char *buffer = get_commit_buffer(partial_commit);\n+\tconst char *msg = strstr(buffer, \"\\n\\n\");\n \tstruct strbuf sb_msg = STRBUF_INIT;\n \tint baselen;\n \n@@ -720,6 +721,7 @@ int notes_merge_commit(struct notes_merge_options *o,\n \t}\n \n \tstrbuf_attach(&sb_msg, msg, strlen(msg), strlen(msg) + 1);\n+\tunuse_commit_buffer(partial_commit, buffer);\n \tcreate_notes_commit(partial_tree, partial_commit->parents, &sb_msg,\n \t\t\t    result_sha1);\n \tif (o->verbosity >= 4)\ndiff --git a/sequencer.c b/sequencer.c\nindex 4632f7d..5609ab3 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -666,10 +666,12 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,\n \tint subject_len;\n \n \tfor (cur = todo_list; cur; cur = cur->next) {\n+\t\tconst char *commit_buffer = get_commit_buffer(cur->item);\n \t\tsha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);\n-\t\tsubject_len = find_commit_subject(cur->item->buffer, &subject);\n+\t\tsubject_len = find_commit_subject(commit_buffer, &subject);\n \t\tstrbuf_addf(buf, \"%s %s %.*s\\n\", action_str, sha1_abbrev,\n \t\t\tsubject_len, subject);\n+\t\tunuse_commit_buffer(cur->item, commit_buffer);\n \t}\n \treturn 0;\n }\n-- \n2.0.0.729.g520999f\n"},{"id":"243642","messageId":"20140609181512.GM20315@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140609180236.GA24644@sigill.intra.peff.net","subject":"[PATCH 13/15] commit-slab: provide a static initializer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-09T18:15:12Z","receivedAt":"2014-06-09T18:15:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Callers currently must use init_foo_slab() at runtime before\naccessing a slab. For global slabs, it's much nicer if we\ncan initialize them in BSS, so that each user does not have\nto add code to check-and-initialize.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe calling convention is kind of weird. It goes:\n\n  struct foo_slab foo = COMMIT_SLAB_INIT(1, foo);\n\nWe need to know the size of the slab's element-type in the initializer\n(and we grab it from sizeof(**foo.slab).  Another option would be:\n\n  struct foo_slab foo = COMMIT_SLAB_INIT(1, void *);\n\nwhich is simpler, but requires the user to repeat the type of the slab\n(and if they get it wrong, bad things happen).\n\nYet another option would be to simply let a zero-initialized slab be\nacceptable, and have slab_at check whether \"stride\" is initialized (and\nif not, init to 1).\n\n commit-slab.h | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/commit-slab.h b/commit-slab.h\nindex cc114b5..375c9c7 100644\n--- a/commit-slab.h\n+++ b/commit-slab.h\n@@ -117,4 +117,16 @@ static int stat_ ##slabname## realloc\n  * catch because GCC silently parses it by default.\n  */\n \n+/*\n+ * Statically initialize a commit slab named \"var\". Note that this\n+ * evaluates \"stride\" multiple times! Example:\n+ *\n+ *   struct indegree indegrees = COMMIT_SLAB_INIT(1, indegrees);\n+ *\n+ */\n+#define COMMIT_SLAB_INIT(stride, var) { \\\n+\tCOMMIT_SLAB_SIZE / sizeof(**((var).slab)) / (stride), \\\n+\t(stride), 0, NULL \\\n+}\n+\n #endif /* COMMIT_SLAB_H */\n-- \n2.0.0.729.g520999f\n"},{"id":"243643","messageId":"20140609181524.GN20315@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140609180236.GA24644@sigill.intra.peff.net","subject":"[PATCH 14/15] commit: convert commit->buffer to a slab","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-09T18:15:24Z","receivedAt":"2014-06-09T18:15:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This will make it easier to manage the buffer cache\nindependently of the \"struct commit\" objects. It also\nshrinks \"struct commit\" by one pointer, which may be\nhelpful.\n\nUnfortunately it does not reduce the max memory size of\nsomething like \"rev-list\", because rev-list uses\nget_cached_commit_buffer() to decide not to show each\ncommit's output (and due to the design of slab_at, accessing\nthe slab requires us to extend it, allocating exactly the\nsame number of buffer pointers we dropped from the commit\nstructs).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n commit.c | 20 +++++++++++++-------\n commit.h |  1 -\n 2 files changed, 13 insertions(+), 8 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex d00b818..1b344bd 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -245,14 +245,17 @@ int unregister_shallow(const unsigned char *sha1)\n \treturn 0;\n }\n \n+define_commit_slab(buffer_slab, void *);\n+static struct buffer_slab buffer_slab = COMMIT_SLAB_INIT(1, buffer_slab);\n+\n void set_commit_buffer(struct commit *commit, void *buffer)\n {\n-\tcommit->buffer = buffer;\n+\t*buffer_slab_at(&buffer_slab, commit) = buffer;\n }\n \n const void *get_cached_commit_buffer(const struct commit *commit)\n {\n-\treturn commit->buffer;\n+\treturn *buffer_slab_at(&buffer_slab, commit);\n }\n \n const void *get_commit_buffer(const struct commit *commit)\n@@ -274,20 +277,23 @@ const void *get_commit_buffer(const struct commit *commit)\n \n void unuse_commit_buffer(const struct commit *commit, const void *buffer)\n {\n-\tif (commit->buffer != buffer)\n+\tvoid *cached = *buffer_slab_at(&buffer_slab, commit);\n+\tif (cached != buffer)\n \t\tfree((void *)buffer);\n }\n \n void free_commit_buffer(struct commit *commit)\n {\n-\tfree(commit->buffer);\n-\tcommit->buffer = NULL;\n+\tvoid **b = buffer_slab_at(&buffer_slab, commit);\n+\tfree(*b);\n+\t*b = NULL;\n }\n \n const void *detach_commit_buffer(struct commit *commit)\n {\n-\tvoid *ret = commit->buffer;\n-\tcommit->buffer = NULL;\n+\tvoid **b = buffer_slab_at(&buffer_slab, commit);\n+\tvoid *ret = *b;\n+\t*b = NULL;\n \treturn ret;\n }\n \ndiff --git a/commit.h b/commit.h\nindex 27e4fd7..02f894b 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -20,7 +20,6 @@ struct commit {\n \tunsigned long date;\n \tstruct commit_list *parents;\n \tstruct tree *tree;\n-\tchar *buffer;\n };\n \n extern int save_commit_buffer;\n-- \n2.0.0.729.g520999f\n"},{"id":"243644","messageId":"20140609181538.GO20315@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140609180236.GA24644@sigill.intra.peff.net","subject":"[PATCH 15/15] commit: record buffer length in cache","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-09T18:15:38Z","receivedAt":"2014-06-09T18:15:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Most callsites which use the commit buffer try to use the\ncached version attached to the commit, rather than\nre-reading from disk. Unfortunately, that interface provides\nonly a pointer to the NUL-terminated buffer, with no\nindication of the original length.\n\nFor the most part, this doesn't matter. People do not put\nNULs in their commit messages, and the log code is happy to\ntreat it all as a NUL-terminated string. However, some code\npaths do care. For example, when checking signatures, we\nwant to be very careful that we verify all the bytes to\navoid malicious trickery.\n\nThis patch just adds an optional \"size\" out-pointer to\nget_commit_buffer and friends. The existing callers all pass\nNULL (there did not seem to be any obvious sites where we\ncould avoid an immediate strlen() call, though perhaps with\nsome further refactoring we could).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/blame.c         |  2 +-\n builtin/fast-export.c   |  2 +-\n builtin/fmt-merge-msg.c |  2 +-\n builtin/index-pack.c    |  2 +-\n builtin/log.c           |  2 +-\n builtin/rev-list.c      |  2 +-\n commit.c                | 54 ++++++++++++++++++++++++++++++++-----------------\n commit.h                |  8 ++++----\n fsck.c                  |  2 +-\n log-tree.c              |  2 +-\n merge-recursive.c       |  2 +-\n notes-merge.c           |  2 +-\n object.c                |  4 ++--\n pretty.c                |  4 ++--\n sequencer.c             |  2 +-\n sha1_name.c             |  2 +-\n 16 files changed, 56 insertions(+), 38 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex cde19eb..58a2c54 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2313,7 +2313,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \t\t    ident, ident, path,\n \t\t    (!contents_from ? path :\n \t\t     (!strcmp(contents_from, \"-\") ? \"standard input\" : contents_from)));\n-\tset_commit_buffer(commit, strbuf_detach(&msg, NULL));\n+\tset_commit_buffer(commit, msg.buf, msg.len);\n \n \tif (!contents_from || strcmp(\"-\", contents_from)) {\n \t\tstruct stat st;\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 7ee5e08..05d161f 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -289,7 +289,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev)\n \trev->diffopt.output_format = DIFF_FORMAT_CALLBACK;\n \n \tparse_commit_or_die(commit);\n-\tcommit_buffer = get_commit_buffer(commit);\n+\tcommit_buffer = get_commit_buffer(commit, NULL);\n \tauthor = strstr(commit_buffer, \"\\nauthor \");\n \tif (!author)\n \t\tdie (\"Could not find author in commit %s\",\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex 01f6d59..ef8b254 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -236,7 +236,7 @@ static void record_person(int which, struct string_list *people,\n \tconst char *field;\n \n \tfield = (which == 'a') ? \"\\nauthor \" : \"\\ncommitter \";\n-\tbuffer = get_commit_buffer(commit);\n+\tbuffer = get_commit_buffer(commit, NULL);\n \tname = strstr(buffer, field);\n \tif (!name)\n \t\treturn;\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 995df39..25e3e9b 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -786,7 +786,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\t}\n \t\t\tif (obj->type == OBJ_COMMIT) {\n \t\t\t\tstruct commit *commit = (struct commit *) obj;\n-\t\t\t\tdetach_commit_buffer(commit);\n+\t\t\t\tdetach_commit_buffer(commit, NULL);\n \t\t\t}\n \t\t\tobj->flags |= FLAG_CHECKED;\n \t\t}\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 2c74260..c599eac 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -919,7 +919,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \t\t\t\t&need_8bit_cte);\n \n \tfor (i = 0; !need_8bit_cte && i < nr; i++) {\n-\t\tconst char *buf = get_commit_buffer(list[i]);\n+\t\tconst char *buf = get_commit_buffer(list[i], NULL);\n \t\tif (has_non_ascii(buf))\n \t\t\tneed_8bit_cte = 1;\n \t\tunuse_commit_buffer(list[i], buf);\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 3fcbf21..ff84a82 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -106,7 +106,7 @@ static void show_commit(struct commit *commit, void *data)\n \telse\n \t\tputchar('\\n');\n \n-\tif (revs->verbose_header && get_cached_commit_buffer(commit)) {\n+\tif (revs->verbose_header && get_cached_commit_buffer(commit, NULL)) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tstruct pretty_print_context ctx = {0};\n \t\tctx.abbrev = revs->abbrev;\ndiff --git a/commit.c b/commit.c\nindex 1b344bd..b833b9f 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -245,22 +245,31 @@ int unregister_shallow(const unsigned char *sha1)\n \treturn 0;\n }\n \n-define_commit_slab(buffer_slab, void *);\n+struct commit_buffer {\n+\tvoid *buffer;\n+\tunsigned long size;\n+};\n+define_commit_slab(buffer_slab, struct commit_buffer);\n static struct buffer_slab buffer_slab = COMMIT_SLAB_INIT(1, buffer_slab);\n \n-void set_commit_buffer(struct commit *commit, void *buffer)\n+void set_commit_buffer(struct commit *commit, void *buffer, unsigned long size)\n {\n-\t*buffer_slab_at(&buffer_slab, commit) = buffer;\n+\tstruct commit_buffer *v = buffer_slab_at(&buffer_slab, commit);\n+\tv->buffer = buffer;\n+\tv->size = size;\n }\n \n-const void *get_cached_commit_buffer(const struct commit *commit)\n+const void *get_cached_commit_buffer(const struct commit *commit, unsigned long *sizep)\n {\n-\treturn *buffer_slab_at(&buffer_slab, commit);\n+\tstruct commit_buffer *v = buffer_slab_at(&buffer_slab, commit);\n+\tif (sizep)\n+\t\t*sizep = v->size;\n+\treturn v->buffer;\n }\n \n-const void *get_commit_buffer(const struct commit *commit)\n+const void *get_commit_buffer(const struct commit *commit, unsigned long *sizep)\n {\n-\tconst void *ret = get_cached_commit_buffer(commit);\n+\tconst void *ret = get_cached_commit_buffer(commit, sizep);\n \tif (!ret) {\n \t\tenum object_type type;\n \t\tunsigned long size;\n@@ -271,29 +280,38 @@ const void *get_commit_buffer(const struct commit *commit)\n \t\tif (type != OBJ_COMMIT)\n \t\t\tdie(\"expected commit for %s, got %s\",\n \t\t\t    sha1_to_hex(commit->object.sha1), typename(type));\n+\t\tif (sizep)\n+\t\t\t*sizep = size;\n \t}\n \treturn ret;\n }\n \n void unuse_commit_buffer(const struct commit *commit, const void *buffer)\n {\n-\tvoid *cached = *buffer_slab_at(&buffer_slab, commit);\n-\tif (cached != buffer)\n+\tstruct commit_buffer *v = buffer_slab_at(&buffer_slab, commit);\n+\tif (v->buffer != buffer)\n \t\tfree((void *)buffer);\n }\n \n void free_commit_buffer(struct commit *commit)\n {\n-\tvoid **b = buffer_slab_at(&buffer_slab, commit);\n-\tfree(*b);\n-\t*b = NULL;\n+\tstruct commit_buffer *v = buffer_slab_at(&buffer_slab, commit);\n+\tfree(v->buffer);\n+\tv->buffer = NULL;\n+\tv->size = 0;\n }\n \n-const void *detach_commit_buffer(struct commit *commit)\n+const void *detach_commit_buffer(struct commit *commit, unsigned long *sizep)\n {\n-\tvoid **b = buffer_slab_at(&buffer_slab, commit);\n-\tvoid *ret = *b;\n-\t*b = NULL;\n+\tstruct commit_buffer *v = buffer_slab_at(&buffer_slab, commit);\n+\tvoid *ret;\n+\n+\tret = v->buffer;\n+\tif (sizep)\n+\t\t*sizep = v->size;\n+\n+\tv->buffer = NULL;\n+\tv->size = 0;\n \treturn ret;\n }\n \n@@ -374,7 +392,7 @@ int parse_commit(struct commit *item)\n \t}\n \tret = parse_commit_buffer(item, buffer, size);\n \tif (save_commit_buffer && !ret) {\n-\t\tset_commit_buffer(item, buffer);\n+\t\tset_commit_buffer(item, buffer, size);\n \t\treturn 0;\n \t}\n \tfree(buffer);\n@@ -589,7 +607,7 @@ 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);\n+\tconst char *buffer = get_commit_buffer(commit, NULL);\n \tstruct ident_split ident;\n \tchar *date_end;\n \tunsigned long date;\ndiff --git a/commit.h b/commit.h\nindex 02f894b..9ac7954 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -54,20 +54,20 @@ void parse_commit_or_die(struct commit *item);\n  * Associate an object buffer with the commit. The ownership of the\n  * memory is handed over to the commit, and must be free()-able.\n  */\n-void set_commit_buffer(struct commit *, void *buffer);\n+void set_commit_buffer(struct commit *, void *buffer, unsigned long size);\n \n /*\n  * Get any cached object buffer associated with the commit. Returns NULL\n  * if none. The resulting memory should not be freed.\n  */\n-const void *get_cached_commit_buffer(const struct commit *);\n+const void *get_cached_commit_buffer(const struct commit *, unsigned long *size);\n \n /*\n  * Get the commit's object contents, either from cache or by reading the object\n  * from disk. The resulting memory should not be modified, and must be given\n  * to unuse_commit_buffer when the caller is done.\n  */\n-const void *get_commit_buffer(const struct commit *);\n+const void *get_commit_buffer(const struct commit *, unsigned long *size);\n \n /*\n  * Tell the commit subsytem that we are done with a particular commit buffer.\n@@ -86,7 +86,7 @@ void free_commit_buffer(struct commit *);\n  * Disassociate any cached object buffer from the commit, but do not free it.\n  * The buffer (or NULL, if none) is returned.\n  */\n-const void *detach_commit_buffer(struct commit *);\n+const void *detach_commit_buffer(struct commit *, unsigned long *sizep);\n \n /* Find beginning and length of commit subject. */\n int find_commit_subject(const char *commit_buffer, const char **subject);\ndiff --git a/fsck.c b/fsck.c\nindex 8223780..a7233c8 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -339,7 +339,7 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n \n static int fsck_commit(struct commit *commit, fsck_error error_func)\n {\n-\tconst char *buffer = get_commit_buffer(commit);\n+\tconst char *buffer = get_commit_buffer(commit, NULL);\n \tint ret = fsck_commit_buffer(commit, buffer, error_func);\n \tunuse_commit_buffer(commit, buffer);\n \treturn ret;\ndiff --git a/log-tree.c b/log-tree.c\nindex e9ef8ab..4447021 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -588,7 +588,7 @@ void show_log(struct rev_info *opt)\n \t\tshow_mergetag(opt, commit);\n \t}\n \n-\tif (!get_cached_commit_buffer(commit))\n+\tif (!get_cached_commit_buffer(commit, NULL))\n \t\treturn;\n \n \tif (opt->show_notes) {\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 0dd6039..d38a3b2 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -190,7 +190,7 @@ static void output_commit_title(struct merge_options *o, struct commit *commit)\n \t\t\tprintf(_(\"(bad commit)\\n\"));\n \t\telse {\n \t\t\tconst char *title;\n-\t\t\tconst char *msg = get_commit_buffer(commit);\n+\t\t\tconst char *msg = get_commit_buffer(commit, NULL);\n \t\t\tint len = find_commit_subject(msg, &title);\n \t\t\tif (len)\n \t\t\t\tprintf(\"%.*s\\n\", len, title);\ndiff --git a/notes-merge.c b/notes-merge.c\nindex 7885ab2..26360e2 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -671,7 +671,7 @@ int notes_merge_commit(struct notes_merge_options *o,\n \tDIR *dir;\n \tstruct dirent *e;\n \tstruct strbuf path = STRBUF_INIT;\n-\tconst char *buffer = get_commit_buffer(partial_commit);\n+\tconst char *buffer = get_commit_buffer(partial_commit, NULL);\n \tconst char *msg = strstr(buffer, \"\\n\\n\");\n \tstruct strbuf sb_msg = STRBUF_INIT;\n \tint baselen;\ndiff --git a/object.c b/object.c\nindex 67b6e35..9c31e9a 100644\n--- a/object.c\n+++ b/object.c\n@@ -197,8 +197,8 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t\n \t\tif (commit) {\n \t\t\tif (parse_commit_buffer(commit, buffer, size))\n \t\t\t\treturn NULL;\n-\t\t\tif (!get_cached_commit_buffer(commit)) {\n-\t\t\t\tset_commit_buffer(commit, buffer);\n+\t\t\tif (!get_cached_commit_buffer(commit, NULL)) {\n+\t\t\t\tset_commit_buffer(commit, buffer, size);\n \t\t\t\t*eaten_p = 1;\n \t\t\t}\n \t\t\tobj = &commit->object;\ndiff --git a/pretty.c b/pretty.c\nindex 8fd60cd..280d7d0 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -613,7 +613,7 @@ const char *logmsg_reencode(const struct commit *commit,\n \tstatic const char *utf8 = \"UTF-8\";\n \tconst char *use_encoding;\n \tchar *encoding;\n-\tconst char *msg = get_commit_buffer(commit);\n+\tconst char *msg = get_commit_buffer(commit, NULL);\n \tchar *out;\n \n \tif (!output_encoding || !*output_encoding) {\n@@ -642,7 +642,7 @@ const char *logmsg_reencode(const struct commit *commit,\n \t\t * the cached copy from get_commit_buffer, we need to duplicate it\n \t\t * to avoid munging the cached copy.\n \t\t */\n-\t\tif (msg == get_cached_commit_buffer(commit))\n+\t\tif (msg == get_cached_commit_buffer(commit, NULL))\n \t\t\tout = xstrdup(msg);\n \t\telse\n \t\t\tout = (char *)msg;\ndiff --git a/sequencer.c b/sequencer.c\nindex 5609ab3..d83f810 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -666,7 +666,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,\n \tint subject_len;\n \n \tfor (cur = todo_list; cur; cur = cur->next) {\n-\t\tconst char *commit_buffer = get_commit_buffer(cur->item);\n+\t\tconst char *commit_buffer = get_commit_buffer(cur->item, NULL);\n \t\tsha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);\n \t\tsubject_len = find_commit_subject(commit_buffer, &subject);\n \t\tstrbuf_addf(buf, \"%s %s %.*s\\n\", action_str, sha1_abbrev,\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 0a65d23..c2c938c 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -869,7 +869,7 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1,\n \t\tcommit = pop_most_recent_commit(&list, ONELINE_SEEN);\n \t\tif (!parse_object(commit->object.sha1))\n \t\t\tcontinue;\n-\t\tbuf = get_commit_buffer(commit);\n+\t\tbuf = get_commit_buffer(commit, NULL);\n \t\tp = strstr(buf, \"\\n\\n\");\n \t\tmatches = p && !regexec(&regex, p + 2, 0, NULL, 0);\n \t\tunuse_commit_buffer(commit, buf);\n-- \n2.0.0.729.g520999f\n"},{"id":"243672","messageId":"xmqqbnu1emfa.fsf@gitster.dls.corp.google.com","threadId":"36874","inReplyTo":"20140609181323.GL20315@sigill.intra.peff.net","subject":"Re: [PATCH 12/15] use get_commit_buffer everywhere","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-09T22:40:57Z","receivedAt":"2014-06-09T22:40:57Z","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/notes-merge.c b/notes-merge.c\n> index 94a1a8a..7885ab2 100644\n> --- a/notes-merge.c\n> +++ b/notes-merge.c\n> @@ -671,7 +671,8 @@ int notes_merge_commit(struct notes_merge_options *o,\n>  \tDIR *dir;\n>  \tstruct dirent *e;\n>  \tstruct strbuf path = STRBUF_INIT;\n> -\tchar *msg = strstr(partial_commit->buffer, \"\\n\\n\");\n> +\tconst char *buffer = get_commit_buffer(partial_commit);\n> +\tconst char *msg = strstr(buffer, \"\\n\\n\");\n\nThis tightening causes...\n\n>  \tstruct strbuf sb_msg = STRBUF_INIT;\n>  \tint baselen;\n>  \n> @@ -720,6 +721,7 @@ int notes_merge_commit(struct notes_merge_options *o,\n>  \t}\n>  \n>  \tstrbuf_attach(&sb_msg, msg, strlen(msg), strlen(msg) + 1);\n\n...a new error here:\n\nnotes-merge.c:723:2: error: passing argument 2 of 'strbuf_attach'\ndiscards 'const' qualifier from pointer target type [-Werror]\nstrbuf.h:19:13: note: expected 'void *' but argument is of type\n'const char *'\n"},{"id":"243689","messageId":"20140610000223.GA20644@sigill.intra.peff.net","threadId":"36874","inReplyTo":"xmqqbnu1emfa.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 12/15] use get_commit_buffer everywhere","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T00:02:24Z","receivedAt":"2014-06-10T00:02:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 09, 2014 at 03:40:57PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > diff --git a/notes-merge.c b/notes-merge.c\n> > index 94a1a8a..7885ab2 100644\n> > --- a/notes-merge.c\n> > +++ b/notes-merge.c\n> > @@ -671,7 +671,8 @@ int notes_merge_commit(struct notes_merge_options *o,\n> >  \tDIR *dir;\n> >  \tstruct dirent *e;\n> >  \tstruct strbuf path = STRBUF_INIT;\n> > -\tchar *msg = strstr(partial_commit->buffer, \"\\n\\n\");\n> > +\tconst char *buffer = get_commit_buffer(partial_commit);\n> > +\tconst char *msg = strstr(buffer, \"\\n\\n\");\n> \n> This tightening causes...\n> \n> >  \tstruct strbuf sb_msg = STRBUF_INIT;\n> >  \tint baselen;\n> >  \n> > @@ -720,6 +721,7 @@ int notes_merge_commit(struct notes_merge_options *o,\n> >  \t}\n> >  \n> >  \tstrbuf_attach(&sb_msg, msg, strlen(msg), strlen(msg) + 1);\n> \n> ...a new error here:\n> \n> notes-merge.c:723:2: error: passing argument 2 of 'strbuf_attach'\n> discards 'const' qualifier from pointer target type [-Werror]\n> strbuf.h:19:13: note: expected 'void *' but argument is of type\n> 'const char *'\n\nThat's weird. I compile with -Wall -Werror, and my gcc doesn't complain.\nHmph.\n\nI agree it's not right, though. I think the original is questionable,\ntoo. It takes a pointer into the middle of partial_commit->buffer and\nattaches it to a strbuf. That's wrong because:\n\n  1. It's pointing into the middle of an allocated buffer, not the\n     beginning.\n\n  2. We do not own partial_commit->buffer in the first place.\n\nSo any call to strbuf_detach on the result would be disastrous. The\ncompiler doesn't notice because of the const leak in strstr, and it\ndoesn't cause a bug in practice because the only use of the strbuf is to\npass it as a const to create_notes_commit.\n\nI feel like the most elegant solution is for create_notes_commit to take\na buf/len pair rather than a strbuf, but it unfortunately is just\nfeeding that to commit_tree. Adjusting that code path would affect quite\na few other spots.\n\nThe other obvious option is actually populating the strbuf, but it feels\nugly to have to make a copy just to satisfy the function interface.\n\nMaybe a cast and a warning comment are the least evil thing, as below? I\ndunno, it feels pretty wrong.\n\ndiff --git a/notes-merge.c b/notes-merge.c\nindex 94a1a8a..1f3b309 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -671,7 +671,7 @@ int notes_merge_commit(struct notes_merge_options *o,\n \tDIR *dir;\n \tstruct dirent *e;\n \tstruct strbuf path = STRBUF_INIT;\n-\tchar *msg = strstr(partial_commit->buffer, \"\\n\\n\");\n+\tconst char *msg = strstr(partial_commit->buffer, \"\\n\\n\");\n \tstruct strbuf sb_msg = STRBUF_INIT;\n \tint baselen;\n \n@@ -719,7 +719,15 @@ int notes_merge_commit(struct notes_merge_options *o,\n \t\tstrbuf_setlen(&path, baselen);\n \t}\n \n-\tstrbuf_attach(&sb_msg, msg, strlen(msg), strlen(msg) + 1);\n+\t/*\n+\t * This is a bit tricky. We should not be attaching msg, which\n+\t * is not owned by us and is not even the start of a heap buffer, to a\n+\t * strbuf. But the create_notes_commit interface really wants\n+\t * a strbuf, even though it will only ever use it as a buf/len pair and\n+\t * never modify it. So this is tentatively safe as long as nobody ever\n+\t * modifies, detaches, or releases the strbuf.\n+\t */\n+\tstrbuf_attach(&sb_msg, (char *)msg, strlen(msg), strlen(msg) + 1);\n \tcreate_notes_commit(partial_tree, partial_commit->parents, &sb_msg,\n \t\t\t    result_sha1);\n \tif (o->verbosity >= 4)\n\nI'm still confused and disturbed that my gcc is not noticing this\nobvious const violation. Hmm, shutting off ccache seems to make it work.\nDoubly disturbing.\n\n-Peff\n"},{"id":"243693","messageId":"20140610002202.GB20644@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140610000223.GA20644@sigill.intra.peff.net","subject":"Re: [PATCH 12/15] use get_commit_buffer everywhere","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T00:22:03Z","receivedAt":"2014-06-10T00:22:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 09, 2014 at 08:02:24PM -0400, Jeff King wrote:\n\n> I'm still confused and disturbed that my gcc is not noticing this\n> obvious const violation. Hmm, shutting off ccache seems to make it work.\n> Doubly disturbing.\n\nAh, mystery solved. It's a gcc bug:\n\n  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60014\n\nI get:\n\n  $ gcc -c -Wall -Werror -DSHA1_HEADER='\"block-sha1/sha1.h\"' notes-merge.c\n  notes-merge.c: In function ‘notes_merge_commit’:\n  notes-merge.c:723:2: error: passing argument 2 of ‘strbuf_attach’\n    discards ‘const’ qualifier from pointer target type [-Werror]\n  ...etc...\n\n  $ gcc -E -Wall -Werror -DSHA1_HEADER='\"block-sha1/sha1.h\"' notes-merge.c >foo.c\n  $ gcc -c -Wall -Werror -DSHA1_HEADER='\"block-sha1/sha1.h\"' foo.c\n  [no warnings from either]\n\nccache uses the latter technique.\n\n-Peff\n"},{"id":"243695","messageId":"CAPig+cQzNnnTST-SVgxRsgLtHFO8NFpnJ=uFzHStgMeJDhgMfQ@mail.gmail.com","threadId":"36874","inReplyTo":"20140609181017.GD20315@sigill.intra.peff.net","subject":"Re: [PATCH 04/15] logmsg_reencode: return const buffer","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-10T01:36:43Z","receivedAt":"2014-06-10T01:36:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 9, 2014 at 2:10 PM, Jeff King <peff@peff.net> wrote:\n> The return value from logmsg_reencode may be either a newly\n> allocated buffer or a pointer to the existing commit->buffer.\n> We would not want the caller to accidentally free() or\n> modify the latter, so let's mark it as const.  We can cast\n> away the constness in logmsg_free, but only once we have\n> determined that it is a free-able buffer.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> index 71e2337..47e5b7a 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2788,7 +2788,7 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n>  {\n>         int retval;\n>         const char *encoding;\n> -       char *message;\n> +       const char *message;\n>         struct strbuf buf = STRBUF_INIT;\n>\n>         if (!opt->grep_filter.pattern_list && !opt->grep_filter.header_list)\n> @@ -2830,12 +2830,18 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n>                 format_display_notes(commit->object.sha1, &buf, encoding, 1);\n>         }\n>\n> -       /* Find either in the original commit message, or in the temporary */\n> +       /* Find either in the original commit message, or in the temporary.\n\nStyle:\n\n    /*\n     * Find either...\n     */\n\n> +        * Note that we cast away the constness of \"message\" here. It is\n> +        * const because it may come from the cached commit buffer. That's OK,\n> +        * because we know that it is modifiable heap memory, and that while\n> +        * grep_buffer may modify it for speed, it will restore any\n> +        * changes before returning.\n> +        */\n>         if (buf.len)\n>                 retval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);\n>         else\n>                 retval = grep_buffer(&opt->grep_filter,\n> -                                    message, strlen(message));\n> +                                    (char *)message, strlen(message));\n>         strbuf_release(&buf);\n>         logmsg_free(message, commit);\n>         return retval;\n> --\n> 2.0.0.729.g520999f\n>\n"},{"id":"243700","messageId":"20140610.071237.852310668484562387.chriscool@tuxfamily.org","threadId":"36874","inReplyTo":"20140609181538.GO20315@sigill.intra.peff.net","subject":"Re: [PATCH 15/15] commit: record buffer length in cache","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-06-10T05:12:37Z","receivedAt":"2014-06-10T05:12:37Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Jeff King <peff@peff.net>\n>\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -2313,7 +2313,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n>  \t\t    ident, ident, path,\n>  \t\t    (!contents_from ? path :\n>  \t\t     (!strcmp(contents_from, \"-\") ? \"standard input\" : contents_from)));\n> -\tset_commit_buffer(commit, strbuf_detach(&msg, NULL));\n> +\tset_commit_buffer(commit, msg.buf, msg.len);\n\nI find the above strange. I would have done something like:\n\n-\tset_commit_buffer(commit, strbuf_detach(&msg, NULL));\n+\tsize_t size;\n+\tchar *buf = strbuf_detach(&msg, &size);\n+\tset_commit_buffer(commit, buf, size);\n\nThanks,\nChristian.\n"},{"id":"243701","messageId":"20140610052713.GA2978@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140610.071237.852310668484562387.chriscool@tuxfamily.org","subject":"Re: [PATCH 15/15] commit: record buffer length in cache","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T05:27:13Z","receivedAt":"2014-06-10T05:27:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 10, 2014 at 07:12:37AM +0200, Christian Couder wrote:\n\n> From: Jeff King <peff@peff.net>\n> >\n> > --- a/builtin/blame.c\n> > +++ b/builtin/blame.c\n> > @@ -2313,7 +2313,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n> >  \t\t    ident, ident, path,\n> >  \t\t    (!contents_from ? path :\n> >  \t\t     (!strcmp(contents_from, \"-\") ? \"standard input\" : contents_from)));\n> > -\tset_commit_buffer(commit, strbuf_detach(&msg, NULL));\n> > +\tset_commit_buffer(commit, msg.buf, msg.len);\n> \n> I find the above strange. I would have done something like:\n> \n> -\tset_commit_buffer(commit, strbuf_detach(&msg, NULL));\n> +\tsize_t size;\n> +\tchar *buf = strbuf_detach(&msg, &size);\n> +\tset_commit_buffer(commit, buf, size);\n\nIt is a little strange. You can't do:\n\n  set_commit_buffer(commit, strbuf_detach(&msg, NULL), msg.len);\n\nbecause the second argument resets msg.len as a side effect. Doing it\nyour way is longer, but perhaps is a bit more canonical. In general, one\nshould call strbuf_detach to ensure that the buffer is allocated (and\ndoes not point to slopbuf). That's guaranteed here, because we just put\ncontents into the buffer, but it's probably more hygienic to use the\nmore verbose form.\n\n-Peff\n"},{"id":"243713","messageId":"CAPig+cTkwFVDEj7L6qriNP9-Qsu=B-wJsv0z1b++u57dir8XOw@mail.gmail.com","threadId":"36874","inReplyTo":"20140609181158.GH20315@sigill.intra.peff.net","subject":"Re: [PATCH 08/15] provide helpers to access the commit buffer","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-10T08:00:34Z","receivedAt":"2014-06-10T08:00:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Monday, June 9, 2014, Jeff King <peff@peff.net> wrote:\n> Many sites look at commit->buffer to get more detailed\n> information than what is in the parsed commit struct.\n> However, we sometimes drop commit->buffer to save memory,\n> in which case the caller would need to read the object\n> afresh. Some callers do this (leading to duplicated code),\n> and others do not (which opens the possibility of a segfault\n> if somebody else frees the buffer).\n>\n> Let's provide a pair of helpers, \"get\" and \"unuse\", that let\n> callers easily get the buffer. They will use the cached\n> buffer when possible, and otherwise load from disk using\n> read_sha1_file.\n>\n> Note that we also need to add a \"get_cached\" variant which\n> returns NULL when we do not have a cached buffer. At first\n> glance this seems to defeat the purpose of \"get\", which is\n> to always provide a return value. However, some log code\n> paths actually use the NULL-ness of commit->buffer as a\n> boolean flag to decide whether to try to printing the\n\ns/printing/print/ ... or some other variation of the phrase.\n\n> commit. At least for now, we want to continue supporting\n> that use.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n"},{"id":"243714","messageId":"CAPig+cTztiY3va-oztwe+FRsEgtFh0r6jsYY1Vn5dtvg3fHfZA@mail.gmail.com","threadId":"36874","inReplyTo":"20140609181247.GJ20315@sigill.intra.peff.net","subject":"Re: [PATCH 10/15] use get_commit_buffer to avoid duplicate code","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-10T08:01:29Z","receivedAt":"2014-06-10T08:01:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Monday, June 9, 2014, Jeff King <peff@peff.net> wrote:\n> For both of these sites, we already do the \"fallback to\n> read_sha1_file\" trick. But we can shorten the code by just\n> using get_commit_buffer.\n>\n> Note that the error cases are slightly different when\n> read_sha1_file fails. get_commit_buffer will die() if the\n> object cannot be loaded, or is a non-commit.\n>\n> For get_sha1_oneline, this will almost certainly never\n> happen, as we will have just called parse_object (and if it\n> does, it's probably worth complaining about).\n>\n> For record_author_date, the new behavior is probably better;\n> we notify the user of the error instead of silently ignoring\n> it. And because it's used only for sorting by author-date,\n> somebody examining a corrupt can fallback to the regular\n\nMissing word here? \"examining a corrupt [...] can\"\n\n> traversal order.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n"},{"id":"243743","messageId":"xmqqioo8agvo.fsf@gitster.dls.corp.google.com","threadId":"36874","inReplyTo":"20140610000223.GA20644@sigill.intra.peff.net","subject":"Re: [PATCH 12/15] use get_commit_buffer everywhere","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-10T16:06:35Z","receivedAt":"2014-06-10T16:06:35Z","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> I agree it's not right, though. I think the original is questionable,\n> too. It takes a pointer into the middle of partial_commit->buffer and\n> attaches it to a strbuf. That's wrong because:\n>\n>   1. It's pointing into the middle of an allocated buffer, not the\n>      beginning.\n>\n>   2. We do not own partial_commit->buffer in the first place.\n>\n> So any call to strbuf_detach on the result would be disastrous.\n\nYou are right.  Where did this original crap come from X-<...\n\n> ... and it\n> doesn't cause a bug in practice because the only use of the strbuf is to\n> pass it as a const to create_notes_commit.\n>\n> I feel like the most elegant solution is for create_notes_commit to take\n> a buf/len pair rather than a strbuf, but it unfortunately is just\n> feeding that to commit_tree. Adjusting that code path would affect quite\n> a few other spots.\n>\n> The other obvious option is actually populating the strbuf, but it feels\n> ugly to have to make a copy just to satisfy the function interface.\n>\n> Maybe a cast and a warning comment are the least evil thing, as below? I\n> dunno, it feels pretty wrong.\n\nYeah, it does feel wrong wrong wrong.  Perhaps this big comment\nwould serve as a marker for a low-hanging fruit for somebody else to\nfix it, e.g. by using strbuf-add to make a copy, which would be the\neasiest and safest workaround?\n\n> diff --git a/notes-merge.c b/notes-merge.c\n> index 94a1a8a..1f3b309 100644\n> --- a/notes-merge.c\n> +++ b/notes-merge.c\n> @@ -671,7 +671,7 @@ int notes_merge_commit(struct notes_merge_options *o,\n>  \tDIR *dir;\n>  \tstruct dirent *e;\n>  \tstruct strbuf path = STRBUF_INIT;\n> -\tchar *msg = strstr(partial_commit->buffer, \"\\n\\n\");\n> +\tconst char *msg = strstr(partial_commit->buffer, \"\\n\\n\");\n>  \tstruct strbuf sb_msg = STRBUF_INIT;\n>  \tint baselen;\n>  \n> @@ -719,7 +719,15 @@ int notes_merge_commit(struct notes_merge_options *o,\n>  \t\tstrbuf_setlen(&path, baselen);\n>  \t}\n>  \n> -\tstrbuf_attach(&sb_msg, msg, strlen(msg), strlen(msg) + 1);\n> +\t/*\n> +\t * This is a bit tricky. We should not be attaching msg, which\n> +\t * is not owned by us and is not even the start of a heap buffer, to a\n> +\t * strbuf. But the create_notes_commit interface really wants\n> +\t * a strbuf, even though it will only ever use it as a buf/len pair and\n> +\t * never modify it. So this is tentatively safe as long as nobody ever\n> +\t * modifies, detaches, or releases the strbuf.\n> +\t */\n> +\tstrbuf_attach(&sb_msg, (char *)msg, strlen(msg), strlen(msg) + 1);\n>  \tcreate_notes_commit(partial_tree, partial_commit->parents, &sb_msg,\n>  \t\t\t    result_sha1);\n>  \tif (o->verbosity >= 4)\n>\n> I'm still confused and disturbed that my gcc is not noticing this\n> obvious const violation. Hmm, shutting off ccache seems to make it work.\n> Doubly disturbing.\n>\n> -Peff\n"},{"id":"243774","messageId":"20140610203349.GG14974@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140610052713.GA2978@sigill.intra.peff.net","subject":"Re: [PATCH 15/15] commit: record buffer length in cache","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T20:33:49Z","receivedAt":"2014-06-10T20:33:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 10, 2014 at 01:27:13AM -0400, Jeff King wrote:\n\n> > I find the above strange. I would have done something like:\n> > \n> > -\tset_commit_buffer(commit, strbuf_detach(&msg, NULL));\n> > +\tsize_t size;\n> > +\tchar *buf = strbuf_detach(&msg, &size);\n> > +\tset_commit_buffer(commit, buf, size);\n> \n> It is a little strange. You can't do:\n> \n>   set_commit_buffer(commit, strbuf_detach(&msg, NULL), msg.len);\n> \n> because the second argument resets msg.len as a side effect. Doing it\n> your way is longer, but perhaps is a bit more canonical. In general, one\n> should call strbuf_detach to ensure that the buffer is allocated (and\n> does not point to slopbuf). That's guaranteed here, because we just put\n> contents into the buffer, but it's probably more hygienic to use the\n> more verbose form.\n\nI was trying to avoid cluttering up the function with the extra variable\ndefinitions. I ended up with the change below, which I think is clear,\ncorrect, and pushes the complexity outside of the function.\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex cde19eb..53f43ab 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2266,6 +2266,18 @@ static void append_merge_parents(struct commit_list **tail)\n }\n \n /*\n+ * This isn't as simple as passing sb->buf and sb->len, because we\n+ * want to transfer ownership of the buffer to the commit (so we\n+ * must use detach).\n+ */\n+static void set_commit_buffer_from_strbuf(struct commit *c, struct strbuf *sb)\n+{\n+\tsize_t len;\n+\tvoid *buf = strbuf_detach(sb, &len);\n+\tset_commit_buffer(c, buf, len);\n+}\n+\n+/*\n  * Prepare a dummy commit that represents the work tree (or staged) item.\n  * Note that annotating work tree item never works in the reverse.\n  */\n@@ -2313,7 +2325,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \t\t    ident, ident, path,\n \t\t    (!contents_from ? path :\n \t\t     (!strcmp(contents_from, \"-\") ? \"standard input\" : contents_from)));\n-\tset_commit_buffer(commit, strbuf_detach(&msg, NULL));\n+\tset_commit_buffer_from_strbuf(commit, &msg);\n \n \tif (!contents_from || strcmp(\"-\", contents_from)) {\n \t\tstruct stat st;\n"},{"id":"243775","messageId":"20140610203437.GH14974@sigill.intra.peff.net","threadId":"36874","inReplyTo":"CAPig+cTztiY3va-oztwe+FRsEgtFh0r6jsYY1Vn5dtvg3fHfZA@mail.gmail.com","subject":"Re: [PATCH 10/15] use get_commit_buffer to avoid duplicate code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T20:34:37Z","receivedAt":"2014-06-10T20:34:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 10, 2014 at 04:01:29AM -0400, Eric Sunshine wrote:\n\n> > For record_author_date, the new behavior is probably better;\n> > we notify the user of the error instead of silently ignoring\n> > it. And because it's used only for sorting by author-date,\n> > somebody examining a corrupt can fallback to the regular\n> \n> Missing word here? \"examining a corrupt [...] can\"\n\nIt was \"repo\". Thanks, fixed this and your other comments for my\nre-roll (I'm still looking into the best solution for the strbuf_attach\nissue Junio mentioned).\n\n-Peff\n"},{"id":"243778","messageId":"20140610212753.GA15332@sigill.intra.peff.net","threadId":"36874","inReplyTo":"xmqqioo8agvo.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 12/15] use get_commit_buffer everywhere","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T21:27:53Z","receivedAt":"2014-06-10T21:27:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 10, 2014 at 09:06:35AM -0700, Junio C Hamano wrote:\n\n> > So any call to strbuf_detach on the result would be disastrous.\n> \n> You are right.  Where did this original crap come from X-<...\n\nI do not know if that face means you actually looked at the history, but\nin case you did not...\n\nIt was added by Duy's 13f8b72 (Convert commit_tree() to take strbuf as\nmessage, 2011-12-15). However that was v2 of his patch. If you read the\noriginal thread, you can see that v1 passed a separate pointer/length\npair, and was only changed after a reviewer-who-shall-not-be-named asked\nhim to change it. ;)\n\nOf course there were many people participating in the review, and none\nof us noticed it. I think it is simply a subtle bug.\n\n> > I feel like the most elegant solution is for create_notes_commit to take\n> > a buf/len pair rather than a strbuf, but it unfortunately is just\n> > feeding that to commit_tree. Adjusting that code path would affect quite\n> > a few other spots.\n> >\n> > The other obvious option is actually populating the strbuf, but it feels\n> > ugly to have to make a copy just to satisfy the function interface.\n> >\n> > Maybe a cast and a warning comment are the least evil thing, as below? I\n> > dunno, it feels pretty wrong.\n> \n> Yeah, it does feel wrong wrong wrong.  Perhaps this big comment\n> would serve as a marker for a low-hanging fruit for somebody else to\n> fix it, e.g. by using strbuf-add to make a copy, which would be the\n> easiest and safest workaround?\n\nI really think commit_tree is the culprit here. It doesn't actually want\na strbuf at all, but takes one to make passing the pointer/len pair\nsimpler. Fixing it turned out to be not _too_ disruptive, and it showed\nthat there is another dubious use of strbuf_attach from a different\ncaller.\n\nI'll post my re-rolled series with those fixes in a moment.\n\n-Peff\n"},{"id":"243781","messageId":"20140610.233037.83802811806074490.chriscool@tuxfamily.org","threadId":"36874","inReplyTo":"20140610203349.GG14974@sigill.intra.peff.net","subject":"Re: [PATCH 15/15] commit: record buffer length in cache","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-06-10T21:30:37Z","receivedAt":"2014-06-10T21:30:37Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Jeff King <peff@peff.net>\nSubject: Re: [PATCH 15/15] commit: record buffer length in cache\nDate: Tue, 10 Jun 2014 16:33:49 -0400\n\n> On Tue, Jun 10, 2014 at 01:27:13AM -0400, Jeff King wrote:\n> \n>> > I find the above strange. I would have done something like:\n>> > \n>> > -\tset_commit_buffer(commit, strbuf_detach(&msg, NULL));\n>> > +\tsize_t size;\n>> > +\tchar *buf = strbuf_detach(&msg, &size);\n>> > +\tset_commit_buffer(commit, buf, size);\n>> \n>> It is a little strange. You can't do:\n>> \n>>   set_commit_buffer(commit, strbuf_detach(&msg, NULL), msg.len);\n>> \n>> because the second argument resets msg.len as a side effect. Doing it\n>> your way is longer, but perhaps is a bit more canonical. In general, one\n>> should call strbuf_detach to ensure that the buffer is allocated (and\n>> does not point to slopbuf). That's guaranteed here, because we just put\n>> contents into the buffer, but it's probably more hygienic to use the\n>> more verbose form.\n> \n> I was trying to avoid cluttering up the function with the extra variable\n> definitions. I ended up with the change below, which I think is clear,\n> correct, and pushes the complexity outside of the function.\n\nYeah, great!\n \n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index cde19eb..53f43ab 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -2266,6 +2266,18 @@ static void append_merge_parents(struct commit_list **tail)\n>  }\n>  \n>  /*\n> + * This isn't as simple as passing sb->buf and sb->len, because we\n> + * want to transfer ownership of the buffer to the commit (so we\n> + * must use detach).\n> + */\n> +static void set_commit_buffer_from_strbuf(struct commit *c, struct strbuf *sb)\n> +{\n> +\tsize_t len;\n> +\tvoid *buf = strbuf_detach(sb, &len);\n> +\tset_commit_buffer(c, buf, len);\n> +}\n> +\n> +/*\n>   * Prepare a dummy commit that represents the work tree (or staged) item.\n>   * Note that annotating work tree item never works in the reverse.\n>   */\n> @@ -2313,7 +2325,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n>  \t\t    ident, ident, path,\n>  \t\t    (!contents_from ? path :\n>  \t\t     (!strcmp(contents_from, \"-\") ? \"standard input\" : contents_from)));\n> -\tset_commit_buffer(commit, strbuf_detach(&msg, NULL));\n> +\tset_commit_buffer_from_strbuf(commit, &msg);\n>  \n>  \tif (!contents_from || strcmp(\"-\", contents_from)) {\n>  \t\tstruct stat st;\n\nThanks,\nChristian. \n"},{"id":"243782","messageId":"xmqqfvjc8n8x.fsf@gitster.dls.corp.google.com","threadId":"36874","inReplyTo":"20140609180959.GB20315@sigill.intra.peff.net","subject":"Re: [PATCH 02/15] commit: push commit_index update into alloc_commit_node","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-10T21:31:58Z","receivedAt":"2014-06-10T21:31:58Z","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> This will make the alloc_report output a little uglier (it will say\n> raw_commit), but I don't think anyone cares. And I wanted to make sure\n> there wasn't an easy way to accidentally call the wrong alloc function\n> that doesn't handle the index.\n\nThanks; I like this change.\n\n>\n>  alloc.c  | 12 ++++++++++--\n>  commit.c |  2 --\n>  2 files changed, 10 insertions(+), 4 deletions(-)\n>\n> diff --git a/alloc.c b/alloc.c\n> index 38ff7e7..eb22a45 100644\n> --- a/alloc.c\n> +++ b/alloc.c\n> @@ -47,10 +47,18 @@ union any_object {\n>  \n>  DEFINE_ALLOCATOR(blob, struct blob)\n>  DEFINE_ALLOCATOR(tree, struct tree)\n> -DEFINE_ALLOCATOR(commit, struct commit)\n> +DEFINE_ALLOCATOR(raw_commit, struct commit)\n>  DEFINE_ALLOCATOR(tag, struct tag)\n>  DEFINE_ALLOCATOR(object, union any_object)\n>  \n> +void *alloc_commit_node(void)\n> +{\n> +\tstatic int commit_count;\n> +\tstruct commit *c = alloc_raw_commit_node();\n> +\tc->index = commit_count++;\n> +\treturn c;\n> +}\n> +\n>  static void report(const char *name, unsigned int count, size_t size)\n>  {\n>  \tfprintf(stderr, \"%10s: %8u (%\"PRIuMAX\" kB)\\n\",\n> @@ -64,7 +72,7 @@ void alloc_report(void)\n>  {\n>  \tREPORT(blob, struct blob);\n>  \tREPORT(tree, struct tree);\n> -\tREPORT(commit, struct commit);\n> +\tREPORT(raw_commit, struct commit);\n>  \tREPORT(tag, struct tag);\n>  \tREPORT(object, union any_object);\n>  }\n> diff --git a/commit.c b/commit.c\n> index f479331..21957ee 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -17,7 +17,6 @@ static struct commit_extra_header *read_commit_extra_header_lines(const char *bu\n>  int save_commit_buffer = 1;\n>  \n>  const char *commit_type = \"commit\";\n> -static int commit_count;\n>  \n>  static struct commit *check_commit(struct object *obj,\n>  \t\t\t\t   const unsigned char *sha1,\n> @@ -64,7 +63,6 @@ struct commit *lookup_commit(const unsigned char *sha1)\n>  \tstruct object *obj = lookup_object(sha1);\n>  \tif (!obj) {\n>  \t\tstruct commit *c = alloc_commit_node();\n> -\t\tc->index = commit_count++;\n>  \t\treturn create_object(sha1, OBJ_COMMIT, c);\n>  \t}\n>  \tif (!obj->type)\n"},{"id":"243783","messageId":"20140610213509.GA26979@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140609180236.GA24644@sigill.intra.peff.net","subject":"[PATCH v2 0/17] store length of commit->buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T21:35:09Z","receivedAt":"2014-06-10T21:35:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here's a re-roll of the commit-slab series. It fixes the issues pointed\nout by Eric and Christian (thanks, both).\n\nIt adds two new patches at the beginning to clean up the dangerous\nstrbufs that we discussed elsewhere. And as a result, silences the\nwarning from the old 12/15. I even compiled with a working copy of gcc\nto confirm. :)\n\n  [01/17]: commit_tree: take a pointer/len pair rather than a const strbuf\n  [02/17]: replace dangerous uses of strbuf_attach\n  [03/17]: alloc: include any-object allocations in alloc_report\n  [04/17]: commit: push commit_index update into alloc_commit_node\n  [05/17]: do not create \"struct commit\" with xcalloc\n  [06/17]: logmsg_reencode: return const buffer\n  [07/17]: sequencer: use logmsg_reencode in get_message\n  [08/17]: provide a helper to free commit buffer\n  [09/17]: provide a helper to set the commit buffer\n  [10/17]: provide helpers to access the commit buffer\n  [11/17]: use get_cached_commit_buffer where appropriate\n  [12/17]: use get_commit_buffer to avoid duplicate code\n  [13/17]: convert logmsg_reencode to get_commit_buffer\n  [14/17]: use get_commit_buffer everywhere\n  [15/17]: commit-slab: provide a static initializer\n  [16/17]: commit: convert commit->buffer to a slab\n  [17/17]: commit: record buffer length in cache\n\n-Peff\n"},{"id":"243784","messageId":"xmqqbnu08n2j.fsf@gitster.dls.corp.google.com","threadId":"36874","inReplyTo":"20140609181008.GC20315@sigill.intra.peff.net","subject":"Re: [PATCH 03/15] do not create \"struct commit\" with xcalloc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-10T21:35:48Z","receivedAt":"2014-06-10T21:35:48Z","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> In both blame and merge-recursive, we sometimes create a\n> \"fake\" commit struct for convenience (e.g., to represent the\n> HEAD state as if we would commit it). By allocating\n> ourselves rather than using alloc_commit_node, we do not\n> properly set the \"index\" field of the commit. This can\n> produce subtle bugs if we then use commit-slab on the\n> resulting commit, as we will share the \"0\" index with\n> another commit.\n\nThe change I see here is all good, but I wonder if it is too late to\nstart the real index from 1 and catch anything that has 0 index with\na BUG(\"do not hand-allocate a commit\").\n\n> We can fix this by using alloc_commit_node() to allocate.\n> Note that we cannot free the result, as it is part of our\n> commit allocator. However, both cases were already leaking\n> the allocated commit anyway, so there's nothing to fix up.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/blame.c   | 2 +-\n>  merge-recursive.c | 2 +-\n>  2 files changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index a52a279..d6056a5 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -2286,7 +2286,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n>  \tstruct strbuf msg = STRBUF_INIT;\n>  \n>  \ttime(&now);\n> -\tcommit = xcalloc(1, sizeof(*commit));\n> +\tcommit = alloc_commit_node();\n>  \tcommit->object.parsed = 1;\n>  \tcommit->date = now;\n>  \tcommit->object.type = OBJ_COMMIT;\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index cab16fa..2b37d42 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -40,7 +40,7 @@ static struct tree *shift_tree_object(struct tree *one, struct tree *two,\n>  \n>  static struct commit *make_virtual_commit(struct tree *tree, const char *comment)\n>  {\n> -\tstruct commit *commit = xcalloc(1, sizeof(struct commit));\n> +\tstruct commit *commit = alloc_commit_node();\n>  \tstruct merge_remote_desc *desc = xmalloc(sizeof(*desc));\n>  \n>  \tdesc->name = comment;\n"},{"id":"243785","messageId":"20140610213652.GA19147@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140610213509.GA26979@sigill.intra.peff.net","subject":"[PATCH 01/17] commit_tree: take a pointer/len pair rather than a const strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T21:36:52Z","receivedAt":"2014-06-10T21:36:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"While strbufs are pretty common throughout our code, it is\nmore flexible for functions to take a pointer/len pair than\na strbuf. It's easy to turn a strbuf into such a pair (by\ndereferencing its members), but less easy to go the other\nway (you can strbuf_attach, but that has implications about\nmemory ownership).\n\nThis patch teaches commit_tree (and its associated callers\nand sub-functions) to take such a pair for the commit\nmessage rather than a strbuf.  This makes passing the buffer\naround slightly more verbose, but means we can get rid of\nsome dangerous strbuf_attach calls in the next patch.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nSadly, the diffstat is not +x/-x. Rewrapping the longer lines\ngave us a few extra lines.\n\n builtin/commit-tree.c |  4 ++--\n builtin/commit.c      |  4 ++--\n builtin/merge.c       |  8 ++++----\n commit.c              | 12 +++++++-----\n commit.h              |  6 ++++--\n notes-cache.c         |  2 +-\n notes-merge.c         |  6 ++++--\n notes-utils.c         |  7 ++++---\n notes-utils.h         |  2 +-\n 9 files changed, 29 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/commit-tree.c b/builtin/commit-tree.c\nindex 987a4c3..8a66c74 100644\n--- a/builtin/commit-tree.c\n+++ b/builtin/commit-tree.c\n@@ -123,8 +123,8 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)\n \t\t\tdie_errno(\"git commit-tree: failed to read\");\n \t}\n \n-\tif (commit_tree(&buffer, tree_sha1, parents, commit_sha1,\n-\t\t\tNULL, sign_commit)) {\n+\tif (commit_tree(buffer.buf, buffer.len, tree_sha1, parents,\n+\t\t\tcommit_sha1, NULL, sign_commit)) {\n \t\tstrbuf_release(&buffer);\n \t\treturn 1;\n \t}\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 99c2044..ef300f1 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1731,8 +1731,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tappend_merge_tag_headers(parents, &tail);\n \t}\n \n-\tif (commit_tree_extended(&sb, active_cache_tree->sha1, parents, sha1,\n-\t\t\t\t author_ident.buf, sign_commit, extra)) {\n+\tif (commit_tree_extended(sb.buf, sb.len, active_cache_tree->sha1,\n+\t\t\t parents, sha1, author_ident.buf, sign_commit, extra)) {\n \t\trollback_index_files();\n \t\tdie(_(\"failed to write commit object\"));\n \t}\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 428ca24..b49c310 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -852,8 +852,8 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)\n \tparent->next->item = remoteheads->item;\n \tparent->next->next = NULL;\n \tprepare_to_commit(remoteheads);\n-\tif (commit_tree(&merge_msg, result_tree, parent, result_commit, NULL,\n-\t\t\tsign_commit))\n+\tif (commit_tree(merge_msg.buf, merge_msg.len, result_tree, parent,\n+\t\t\tresult_commit, NULL, sign_commit))\n \t\tdie(_(\"failed to write commit object\"));\n \tfinish(head, remoteheads, result_commit, \"In-index merge\");\n \tdrop_save();\n@@ -877,8 +877,8 @@ static int finish_automerge(struct commit *head,\n \t\tcommit_list_insert(head, &parents);\n \tstrbuf_addch(&merge_msg, '\\n');\n \tprepare_to_commit(remoteheads);\n-\tif (commit_tree(&merge_msg, result_tree, parents, result_commit,\n-\t\t\tNULL, sign_commit))\n+\tif (commit_tree(merge_msg.buf, merge_msg.len, result_tree, parents,\n+\t\t\tresult_commit, NULL, sign_commit))\n \t\tdie(_(\"failed to write commit object\"));\n \tstrbuf_addf(&buf, \"Merge made by the '%s' strategy.\", wt_strategy);\n \tfinish(head, remoteheads, result_commit, buf.buf);\ndiff --git a/commit.c b/commit.c\nindex f479331..bd3d5af 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1344,7 +1344,8 @@ void free_commit_extra_headers(struct commit_extra_header *extra)\n \t}\n }\n \n-int commit_tree(const struct strbuf *msg, const unsigned char *tree,\n+int commit_tree(const char *msg, size_t msg_len,\n+\t\tconst unsigned char *tree,\n \t\tstruct commit_list *parents, unsigned char *ret,\n \t\tconst char *author, const char *sign_commit)\n {\n@@ -1352,7 +1353,7 @@ int commit_tree(const struct strbuf *msg, const unsigned char *tree,\n \tint result;\n \n \tappend_merge_tag_headers(parents, &tail);\n-\tresult = commit_tree_extended(msg, tree, parents, ret,\n+\tresult = commit_tree_extended(msg, msg_len, tree, parents, ret,\n \t\t\t\t      author, sign_commit, extra);\n \tfree_commit_extra_headers(extra);\n \treturn result;\n@@ -1473,7 +1474,8 @@ static const char commit_utf8_warn[] =\n \"You may want to amend it after fixing the message, or set the config\\n\"\n \"variable i18n.commitencoding to the encoding your project uses.\\n\";\n \n-int commit_tree_extended(const struct strbuf *msg, const unsigned char *tree,\n+int commit_tree_extended(const char *msg, size_t msg_len,\n+\t\t\t const unsigned char *tree,\n \t\t\t struct commit_list *parents, unsigned char *ret,\n \t\t\t const char *author, const char *sign_commit,\n \t\t\t struct commit_extra_header *extra)\n@@ -1484,7 +1486,7 @@ int commit_tree_extended(const struct strbuf *msg, const unsigned char *tree,\n \n \tassert_sha1_type(tree, OBJ_TREE);\n \n-\tif (memchr(msg->buf, '\\0', msg->len))\n+\tif (memchr(msg, '\\0', msg_len))\n \t\treturn error(\"a NUL byte in commit log message not allowed.\");\n \n \t/* Not having i18n.commitencoding is the same as having utf-8 */\n@@ -1523,7 +1525,7 @@ int commit_tree_extended(const struct strbuf *msg, const unsigned char *tree,\n \tstrbuf_addch(&buffer, '\\n');\n \n \t/* And add the comment */\n-\tstrbuf_addbuf(&buffer, msg);\n+\tstrbuf_add(&buffer, msg, msg_len);\n \n \t/* And check the encoding */\n \tif (encoding_is_utf8 && !verify_utf8(&buffer))\ndiff --git a/commit.h b/commit.h\nindex a9f177b..1cb55ae 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -261,11 +261,13 @@ struct commit_extra_header {\n extern void append_merge_tag_headers(struct commit_list *parents,\n \t\t\t\t     struct commit_extra_header ***tail);\n \n-extern int commit_tree(const struct strbuf *msg, const unsigned char *tree,\n+extern int commit_tree(const char *msg, size_t msg_len,\n+\t\t       const unsigned char *tree,\n \t\t       struct commit_list *parents, unsigned char *ret,\n \t\t       const char *author, const char *sign_commit);\n \n-extern int commit_tree_extended(const struct strbuf *msg, const unsigned char *tree,\n+extern int commit_tree_extended(const char *msg, size_t msg_len,\n+\t\t\t\tconst unsigned char *tree,\n \t\t\t\tstruct commit_list *parents, unsigned char *ret,\n \t\t\t\tconst char *author, const char *sign_commit,\n \t\t\t\tstruct commit_extra_header *);\ndiff --git a/notes-cache.c b/notes-cache.c\nindex 97dfd63..4ad0799 100644\n--- a/notes-cache.c\n+++ b/notes-cache.c\n@@ -59,7 +59,7 @@ int notes_cache_write(struct notes_cache *c)\n \t\treturn -1;\n \tstrbuf_attach(&msg, c->validity,\n \t\t      strlen(c->validity), strlen(c->validity) + 1);\n-\tif (commit_tree(&msg, tree_sha1, NULL, commit_sha1, NULL, NULL) < 0)\n+\tif (commit_tree(msg.buf, msg.len, tree_sha1, NULL, commit_sha1, NULL, NULL) < 0)\n \t\treturn -1;\n \tif (update_ref(\"update notes cache\", c->tree.ref, commit_sha1, NULL,\n \t\t       0, UPDATE_REFS_QUIET_ON_ERR) < 0)\ndiff --git a/notes-merge.c b/notes-merge.c\nindex 94a1a8a..9d94210 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -644,7 +644,8 @@ int notes_merge(struct notes_merge_options *o,\n \t\tstruct commit_list *parents = NULL;\n \t\tcommit_list_insert(remote, &parents); /* LIFO order */\n \t\tcommit_list_insert(local, &parents);\n-\t\tcreate_notes_commit(local_tree, parents, &o->commit_msg,\n+\t\tcreate_notes_commit(local_tree, parents,\n+\t\t\t\t    o->commit_msg.buf, o->commit_msg.len,\n \t\t\t\t    result_sha1);\n \t}\n \n@@ -720,7 +721,8 @@ int notes_merge_commit(struct notes_merge_options *o,\n \t}\n \n \tstrbuf_attach(&sb_msg, msg, strlen(msg), strlen(msg) + 1);\n-\tcreate_notes_commit(partial_tree, partial_commit->parents, &sb_msg,\n+\tcreate_notes_commit(partial_tree, partial_commit->parents,\n+\t\t\t    sb_msg.buf, sb_msg.len,\n \t\t\t    result_sha1);\n \tif (o->verbosity >= 4)\n \t\tprintf(\"Finalized notes merge commit: %s\\n\",\ndiff --git a/notes-utils.c b/notes-utils.c\nindex a0b1d7b..b64dc1b 100644\n--- a/notes-utils.c\n+++ b/notes-utils.c\n@@ -4,7 +4,8 @@\n #include \"notes-utils.h\"\n \n void create_notes_commit(struct notes_tree *t, struct commit_list *parents,\n-\t\t\t const struct strbuf *msg, unsigned char *result_sha1)\n+\t\t\t const char *msg, size_t msg_len,\n+\t\t\t unsigned char *result_sha1)\n {\n \tunsigned char tree_sha1[20];\n \n@@ -25,7 +26,7 @@ void create_notes_commit(struct notes_tree *t, struct commit_list *parents,\n \t\t/* else: t->ref points to nothing, assume root/orphan commit */\n \t}\n \n-\tif (commit_tree(msg, tree_sha1, parents, result_sha1, NULL, NULL))\n+\tif (commit_tree(msg, msg_len, tree_sha1, parents, result_sha1, NULL, NULL))\n \t\tdie(\"Failed to commit notes tree to database\");\n }\n \n@@ -46,7 +47,7 @@ void commit_notes(struct notes_tree *t, const char *msg)\n \tif (buf.buf[buf.len - 1] != '\\n')\n \t\tstrbuf_addch(&buf, '\\n'); /* Make sure msg ends with newline */\n \n-\tcreate_notes_commit(t, NULL, &buf, commit_sha1);\n+\tcreate_notes_commit(t, NULL, buf.buf, buf.len, commit_sha1);\n \tstrbuf_insert(&buf, 0, \"notes: \", 7); /* commit message starts at index 7 */\n \tupdate_ref(buf.buf, t->ref, commit_sha1, NULL, 0,\n \t\t   UPDATE_REFS_DIE_ON_ERR);\ndiff --git a/notes-utils.h b/notes-utils.h\nindex 564e30c..890ddb3 100644\n--- a/notes-utils.h\n+++ b/notes-utils.h\n@@ -15,7 +15,7 @@\n  * The resulting commit SHA1 is stored in result_sha1.\n  */\n void create_notes_commit(struct notes_tree *t, struct commit_list *parents,\n-\t\t\t const struct strbuf *msg, unsigned char *result_sha1);\n+\t\t\t const char *msg, size_t msg_len, unsigned char *result_sha1);\n \n void commit_notes(struct notes_tree *t, const char *msg);\n \n-- \n2.0.0.729.g520999f\n"},{"id":"243786","messageId":"20140610213838.GB19147@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140610213509.GA26979@sigill.intra.peff.net","subject":"[PATCH 02/17] replace dangerous uses of strbuf_attach","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T21:38:38Z","receivedAt":"2014-06-10T21:38:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It is not a good idea to strbuf_attach an arbitrary pointer\njust because a function you are calling wants a strbuf.\nAttaching implies a transfer of memory ownership; if anyone\nwere to modify or release the resulting strbuf, we would\nfree() the pointer, leading to possible problems:\n\n  1. Other users of the original pointer might access freed\n     memory.\n\n  2. The pointer might not be the start of a malloc'd\n     area, so calling free() on it in the first place would\n     be wrong.\n\nIn the two cases modified here, we are fortunate that nobody\ntouches the strbuf once it is attached, but it is an\naccident waiting to happen.  Since the previous commit,\ncommit_tree and friends take a pointer/buf pair, so we can\njust do away with the strbufs entirely.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI also manually checked the other dozen or so calls to strbuf_attach,\nand they all look good.\n\n notes-cache.c | 6 ++----\n notes-merge.c | 5 +----\n 2 files changed, 3 insertions(+), 8 deletions(-)\n\ndiff --git a/notes-cache.c b/notes-cache.c\nindex 4ad0799..c4e9bb7 100644\n--- a/notes-cache.c\n+++ b/notes-cache.c\n@@ -48,7 +48,6 @@ int notes_cache_write(struct notes_cache *c)\n {\n \tunsigned char tree_sha1[20];\n \tunsigned char commit_sha1[20];\n-\tstruct strbuf msg = STRBUF_INIT;\n \n \tif (!c || !c->tree.initialized || !c->tree.ref || !*c->tree.ref)\n \t\treturn -1;\n@@ -57,9 +56,8 @@ int notes_cache_write(struct notes_cache *c)\n \n \tif (write_notes_tree(&c->tree, tree_sha1))\n \t\treturn -1;\n-\tstrbuf_attach(&msg, c->validity,\n-\t\t      strlen(c->validity), strlen(c->validity) + 1);\n-\tif (commit_tree(msg.buf, msg.len, tree_sha1, NULL, commit_sha1, NULL, NULL) < 0)\n+\tif (commit_tree(c->validity, strlen(c->validity), tree_sha1, NULL,\n+\t\t\tcommit_sha1, NULL, NULL) < 0)\n \t\treturn -1;\n \tif (update_ref(\"update notes cache\", c->tree.ref, commit_sha1, NULL,\n \t\t       0, UPDATE_REFS_QUIET_ON_ERR) < 0)\ndiff --git a/notes-merge.c b/notes-merge.c\nindex 9d94210..697cec3 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -673,7 +673,6 @@ int notes_merge_commit(struct notes_merge_options *o,\n \tstruct dirent *e;\n \tstruct strbuf path = STRBUF_INIT;\n \tchar *msg = strstr(partial_commit->buffer, \"\\n\\n\");\n-\tstruct strbuf sb_msg = STRBUF_INIT;\n \tint baselen;\n \n \tstrbuf_addstr(&path, git_path(NOTES_MERGE_WORKTREE));\n@@ -720,10 +719,8 @@ int notes_merge_commit(struct notes_merge_options *o,\n \t\tstrbuf_setlen(&path, baselen);\n \t}\n \n-\tstrbuf_attach(&sb_msg, msg, strlen(msg), strlen(msg) + 1);\n \tcreate_notes_commit(partial_tree, partial_commit->parents,\n-\t\t\t    sb_msg.buf, sb_msg.len,\n-\t\t\t    result_sha1);\n+\t\t\t    msg, strlen(msg), result_sha1);\n \tif (o->verbosity >= 4)\n \t\tprintf(\"Finalized notes merge commit: %s\\n\",\n \t\t\tsha1_to_hex(result_sha1));\n-- \n2.0.0.729.g520999f\n"},{"id":"243787","messageId":"20140610213847.GC19147@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140610213509.GA26979@sigill.intra.peff.net","subject":"[PATCH 03/17] alloc: include any-object allocations in alloc_report","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T21:38:47Z","receivedAt":"2014-06-10T21:38:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When 2c1cbec (Use proper object allocators for unknown\nobject nodes too, 2007-04-16), added a special \"any_object\"\nallocator, it never taught alloc_report to report on it. To\ndo so we need to add an extra type argument to the REPORT\nmacro, as that commit did for DEFINE_ALLOCATOR.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n alloc.c | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/alloc.c b/alloc.c\nindex f3ee745..38ff7e7 100644\n--- a/alloc.c\n+++ b/alloc.c\n@@ -57,13 +57,14 @@ static void report(const char *name, unsigned int count, size_t size)\n \t\t\tname, count, (uintmax_t) size);\n }\n \n-#define REPORT(name)\t\\\n-    report(#name, name##_allocs, name##_allocs * sizeof(struct name) >> 10)\n+#define REPORT(name, type)\t\\\n+    report(#name, name##_allocs, name##_allocs * sizeof(type) >> 10)\n \n void alloc_report(void)\n {\n-\tREPORT(blob);\n-\tREPORT(tree);\n-\tREPORT(commit);\n-\tREPORT(tag);\n+\tREPORT(blob, struct blob);\n+\tREPORT(tree, struct tree);\n+\tREPORT(commit, struct commit);\n+\tREPORT(tag, struct tag);\n+\tREPORT(object, union any_object);\n }\n-- \n2.0.0.729.g520999f\n"},{"id":"243788","messageId":"20140610213904.GD19147@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140610213509.GA26979@sigill.intra.peff.net","subject":"[PATCH 04/17] commit: push commit_index update into alloc_commit_node","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T21:39:04Z","receivedAt":"2014-06-10T21:39:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Whenever we create a commit object via lookup_commit, we\ngive it a unique index to be used with the commit-slab API.\nThe theory is that any \"struct commit\" we create would\nfollow this code path, so any such struct would get an\nindex. However, callers could use alloc_commit_node()\ndirectly (and get multiple commits with index 0).\n\nLet's push the indexing into alloc_commit_node so that it's\nhard for callers to get it wrong.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n alloc.c  | 12 ++++++++++--\n commit.c |  2 --\n 2 files changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/alloc.c b/alloc.c\nindex 38ff7e7..eb22a45 100644\n--- a/alloc.c\n+++ b/alloc.c\n@@ -47,10 +47,18 @@ union any_object {\n \n DEFINE_ALLOCATOR(blob, struct blob)\n DEFINE_ALLOCATOR(tree, struct tree)\n-DEFINE_ALLOCATOR(commit, struct commit)\n+DEFINE_ALLOCATOR(raw_commit, struct commit)\n DEFINE_ALLOCATOR(tag, struct tag)\n DEFINE_ALLOCATOR(object, union any_object)\n \n+void *alloc_commit_node(void)\n+{\n+\tstatic int commit_count;\n+\tstruct commit *c = alloc_raw_commit_node();\n+\tc->index = commit_count++;\n+\treturn c;\n+}\n+\n static void report(const char *name, unsigned int count, size_t size)\n {\n \tfprintf(stderr, \"%10s: %8u (%\"PRIuMAX\" kB)\\n\",\n@@ -64,7 +72,7 @@ void alloc_report(void)\n {\n \tREPORT(blob, struct blob);\n \tREPORT(tree, struct tree);\n-\tREPORT(commit, struct commit);\n+\tREPORT(raw_commit, struct commit);\n \tREPORT(tag, struct tag);\n \tREPORT(object, union any_object);\n }\ndiff --git a/commit.c b/commit.c\nindex bd3d5af..fbdc480 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -17,7 +17,6 @@ static struct commit_extra_header *read_commit_extra_header_lines(const char *bu\n int save_commit_buffer = 1;\n \n const char *commit_type = \"commit\";\n-static int commit_count;\n \n static struct commit *check_commit(struct object *obj,\n \t\t\t\t   const unsigned char *sha1,\n@@ -64,7 +63,6 @@ struct commit *lookup_commit(const unsigned char *sha1)\n \tstruct object *obj = lookup_object(sha1);\n \tif (!obj) {\n \t\tstruct commit *c = alloc_commit_node();\n-\t\tc->index = commit_count++;\n \t\treturn create_object(sha1, OBJ_COMMIT, c);\n \t}\n \tif (!obj->type)\n-- \n2.0.0.729.g520999f\n"},{"id":"243789","messageId":"20140610213911.GE19147@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140610213509.GA26979@sigill.intra.peff.net","subject":"[PATCH 05/17] do not create \"struct commit\" with xcalloc","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T21:39:11Z","receivedAt":"2014-06-10T21:39:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In both blame and merge-recursive, we sometimes create a\n\"fake\" commit struct for convenience (e.g., to represent the\nHEAD state as if we would commit it). By allocating\nourselves rather than using alloc_commit_node, we do not\nproperly set the \"index\" field of the commit. This can\nproduce subtle bugs if we then use commit-slab on the\nresulting commit, as we will share the \"0\" index with\nanother commit.\n\nWe can fix this by using alloc_commit_node() to allocate.\nNote that we cannot free the result, as it is part of our\ncommit allocator. However, both cases were already leaking\nthe allocated commit anyway, so there's nothing to fix up.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/blame.c   | 2 +-\n merge-recursive.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex a52a279..d6056a5 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2286,7 +2286,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \tstruct strbuf msg = STRBUF_INIT;\n \n \ttime(&now);\n-\tcommit = xcalloc(1, sizeof(*commit));\n+\tcommit = alloc_commit_node();\n \tcommit->object.parsed = 1;\n \tcommit->date = now;\n \tcommit->object.type = OBJ_COMMIT;\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex cab16fa..2b37d42 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -40,7 +40,7 @@ static struct tree *shift_tree_object(struct tree *one, struct tree *two,\n \n static struct commit *make_virtual_commit(struct tree *tree, const char *comment)\n {\n-\tstruct commit *commit = xcalloc(1, sizeof(struct commit));\n+\tstruct commit *commit = alloc_commit_node();\n \tstruct merge_remote_desc *desc = xmalloc(sizeof(*desc));\n \n \tdesc->name = comment;\n-- \n2.0.0.729.g520999f\n"},{"id":"243790","messageId":"20140610213930.GF19147@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140610213509.GA26979@sigill.intra.peff.net","subject":"[PATCH 06/17] logmsg_reencode: return const buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T21:39:30Z","receivedAt":"2014-06-10T21:39:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The return value from logmsg_reencode may be either a newly\nallocated buffer or a pointer to the existing commit->buffer.\nWe would not want the caller to accidentally free() or\nmodify the latter, so let's mark it as const.  We can cast\naway the constness in logmsg_free, but only once we have\ndetermined that it is a free-able buffer.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nFixed comment style issue from v1.\n\n builtin/blame.c |  2 +-\n builtin/reset.c |  2 +-\n commit.h        |  8 ++++----\n pretty.c        | 14 +++++++-------\n revision.c      | 13 ++++++++++---\n 5 files changed, 23 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex d6056a5..6ce3c6d 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1655,7 +1655,7 @@ static void get_commit_info(struct commit *commit,\n {\n \tint len;\n \tconst char *subject, *encoding;\n-\tchar *message;\n+\tconst char *message;\n \n \tcommit_info_init(ret);\n \ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex f368266..7ebee07 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -93,7 +93,7 @@ static int reset_index(const unsigned char *sha1, int reset_type, int quiet)\n static void print_new_head_line(struct commit *commit)\n {\n \tconst char *hex, *body;\n-\tchar *msg;\n+\tconst char *msg;\n \n \thex = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);\n \tprintf(_(\"HEAD is now at %s\"), hex);\ndiff --git a/commit.h b/commit.h\nindex 1cb55ae..4df48cb 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -115,10 +115,10 @@ struct userformat_want {\n \n extern int has_non_ascii(const char *text);\n struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */\n-extern char *logmsg_reencode(const struct commit *commit,\n-\t\t\t     char **commit_encoding,\n-\t\t\t     const char *output_encoding);\n-extern void logmsg_free(char *msg, const struct commit *commit);\n+extern const char *logmsg_reencode(const struct commit *commit,\n+\t\t\t\t   char **commit_encoding,\n+\t\t\t\t   const char *output_encoding);\n+extern void logmsg_free(const char *msg, const struct commit *commit);\n extern void get_commit_format(const char *arg, struct rev_info *);\n extern const char *format_subject(struct strbuf *sb, const char *msg,\n \t\t\t\t  const char *line_separator);\ndiff --git a/pretty.c b/pretty.c\nindex e1e2cad..d152de2 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -606,9 +606,9 @@ static char *replace_encoding_header(char *buf, const char *encoding)\n \treturn strbuf_detach(&tmp, NULL);\n }\n \n-char *logmsg_reencode(const struct commit *commit,\n-\t\t      char **commit_encoding,\n-\t\t      const char *output_encoding)\n+const char *logmsg_reencode(const struct commit *commit,\n+\t\t\t    char **commit_encoding,\n+\t\t\t    const char *output_encoding)\n {\n \tstatic const char *utf8 = \"UTF-8\";\n \tconst char *use_encoding;\n@@ -687,10 +687,10 @@ char *logmsg_reencode(const struct commit *commit,\n \treturn out ? out : msg;\n }\n \n-void logmsg_free(char *msg, const struct commit *commit)\n+void logmsg_free(const char *msg, const struct commit *commit)\n {\n \tif (msg != commit->buffer)\n-\t\tfree(msg);\n+\t\tfree((void *)msg);\n }\n \n static int mailmap_name(const char **email, size_t *email_len,\n@@ -796,7 +796,7 @@ struct format_commit_context {\n \tstruct signature_check signature_check;\n \tenum flush_type flush_type;\n \tenum trunc_type truncate;\n-\tchar *message;\n+\tconst char *message;\n \tchar *commit_encoding;\n \tsize_t width, indent1, indent2;\n \tint auto_color;\n@@ -1700,7 +1700,7 @@ void pretty_print_commit(struct pretty_print_context *pp,\n \tunsigned long beginning_of_body;\n \tint indent = 4;\n \tconst char *msg;\n-\tchar *reencoded;\n+\tconst char *reencoded;\n \tconst char *encoding;\n \tint need_8bit_cte = pp->need_8bit_cte;\n \ndiff --git a/revision.c b/revision.c\nindex 71e2337..be151ef 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2788,7 +2788,7 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n {\n \tint retval;\n \tconst char *encoding;\n-\tchar *message;\n+\tconst char *message;\n \tstruct strbuf buf = STRBUF_INIT;\n \n \tif (!opt->grep_filter.pattern_list && !opt->grep_filter.header_list)\n@@ -2830,12 +2830,19 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n \t\tformat_display_notes(commit->object.sha1, &buf, encoding, 1);\n \t}\n \n-\t/* Find either in the original commit message, or in the temporary */\n+\t/*\n+\t * Find either in the original commit message, or in the temporary.\n+\t * Note that we cast away the constness of \"message\" here. It is\n+\t * const because it may come from the cached commit buffer. That's OK,\n+\t * because we know that it is modifiable heap memory, and that while\n+\t * grep_buffer may modify it for speed, it will restore any\n+\t * changes before returning.\n+\t */\n \tif (buf.len)\n \t\tretval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);\n \telse\n \t\tretval = grep_buffer(&opt->grep_filter,\n-\t\t\t\t     message, strlen(message));\n+\t\t\t\t     (char *)message, strlen(message));\n \tstrbuf_release(&buf);\n \tlogmsg_free(message, commit);\n \treturn retval;\n-- \n2.0.0.729.g520999f\n"},{"id":"243791","messageId":"20140610213935.GG19147@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140610213509.GA26979@sigill.intra.peff.net","subject":"[PATCH 07/17] sequencer: use logmsg_reencode in get_message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T21:39:35Z","receivedAt":"2014-06-10T21:39:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This simplifies the code, as logmsg_reencode handles the\nreencoding for us in a single call. It also means we learn\nlogmsg_reencode's trick of pulling the buffer from disk when\ncommit->buffer is NULL (we currently just silently return!).\nIt is doubtful this matters in practice, though, as\nsequencer operations would not generally turn off\nsave_commit_buffer.\n\nNote that we may be fixing a bug here. The existing code\ndoes:\n\n  if (same_encoding(to, from))\n\t  reencode_string(buf, to, from);\n\nThat probably should have been \"!same_encoding\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sequencer.c | 45 +++++----------------------------------------\n 1 file changed, 5 insertions(+), 40 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 0a80c58..3fcab4d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -116,39 +116,23 @@ static const char *action_name(const struct replay_opts *opts)\n \treturn opts->action == REPLAY_REVERT ? \"revert\" : \"cherry-pick\";\n }\n \n-static char *get_encoding(const char *message);\n-\n struct commit_message {\n \tchar *parent_label;\n \tconst char *label;\n \tconst char *subject;\n-\tchar *reencoded_message;\n \tconst char *message;\n };\n \n static int get_message(struct commit *commit, struct commit_message *out)\n {\n-\tconst char *encoding;\n \tconst char *abbrev, *subject;\n \tint abbrev_len, subject_len;\n \tchar *q;\n \n-\tif (!commit->buffer)\n-\t\treturn -1;\n-\tencoding = get_encoding(commit->buffer);\n-\tif (!encoding)\n-\t\tencoding = \"UTF-8\";\n \tif (!git_commit_encoding)\n \t\tgit_commit_encoding = \"UTF-8\";\n \n-\tout->reencoded_message = NULL;\n-\tout->message = commit->buffer;\n-\tif (same_encoding(encoding, git_commit_encoding))\n-\t\tout->reencoded_message = reencode_string(commit->buffer,\n-\t\t\t\t\tgit_commit_encoding, encoding);\n-\tif (out->reencoded_message)\n-\t\tout->message = out->reencoded_message;\n-\n+\tout->message = logmsg_reencode(commit, NULL, git_commit_encoding);\n \tabbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);\n \tabbrev_len = strlen(abbrev);\n \n@@ -167,29 +151,10 @@ static int get_message(struct commit *commit, struct commit_message *out)\n \treturn 0;\n }\n \n-static void free_message(struct commit_message *msg)\n+static void free_message(struct commit *commit, struct commit_message *msg)\n {\n \tfree(msg->parent_label);\n-\tfree(msg->reencoded_message);\n-}\n-\n-static char *get_encoding(const char *message)\n-{\n-\tconst char *p = message, *eol;\n-\n-\twhile (*p && *p != '\\n') {\n-\t\tfor (eol = p + 1; *eol && *eol != '\\n'; eol++)\n-\t\t\t; /* do nothing */\n-\t\tif (starts_with(p, \"encoding \")) {\n-\t\t\tchar *result = xmalloc(eol - 8 - p);\n-\t\t\tstrlcpy(result, p + 9, eol - 8 - p);\n-\t\t\treturn result;\n-\t\t}\n-\t\tp = eol;\n-\t\tif (*p == '\\n')\n-\t\t\tp++;\n-\t}\n-\treturn NULL;\n+\tlogmsg_free(msg->message, commit);\n }\n \n static void write_cherry_pick_head(struct commit *commit, const char *pseudoref)\n@@ -489,7 +454,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \tunsigned char head[20];\n \tstruct commit *base, *next, *parent;\n \tconst char *base_label, *next_label;\n-\tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n+\tstruct commit_message msg = { NULL, NULL, NULL, NULL };\n \tchar *defmsg = NULL;\n \tstruct strbuf msgbuf = STRBUF_INIT;\n \tint res, unborn = 0, allow;\n@@ -654,7 +619,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\tres = run_git_commit(defmsg, opts, allow);\n \n leave:\n-\tfree_message(&msg);\n+\tfree_message(commit, &msg);\n \tfree(defmsg);\n \n \treturn res;\n-- \n2.0.0.729.g520999f\n"},{"id":"243792","messageId":"20140610214005.GH19147@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140610213509.GA26979@sigill.intra.peff.net","subject":"[PATCH 08/17] provide a helper to free commit buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T21:40:06Z","receivedAt":"2014-06-10T21:40:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This converts two lines into one at each caller. But more\nimportantly, it abstracts the concept of freeing the buffer,\nwhich will make it easier to change later.\n\nNote that we also need to provide a \"detach\" mechanism for\nthe weird case in fsck which passes the buffer back to be\nfreed.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/fsck.c       |  3 +--\n builtin/index-pack.c |  2 +-\n builtin/log.c        |  6 ++----\n builtin/rev-list.c   |  3 +--\n commit.c             | 13 +++++++++++++\n commit.h             | 11 +++++++++++\n 6 files changed, 29 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex fc150c8..8aadca1 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -310,8 +310,7 @@ static int fsck_obj(struct object *obj)\n \tif (obj->type == OBJ_COMMIT) {\n \t\tstruct commit *commit = (struct commit *) obj;\n \n-\t\tfree(commit->buffer);\n-\t\tcommit->buffer = NULL;\n+\t\tfree_commit_buffer(commit);\n \n \t\tif (!commit->parents && show_root)\n \t\t\tprintf(\"root %s\\n\", sha1_to_hex(commit->object.sha1));\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 18f57de..995df39 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -786,7 +786,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\t}\n \t\t\tif (obj->type == OBJ_COMMIT) {\n \t\t\t\tstruct commit *commit = (struct commit *) obj;\n-\t\t\t\tcommit->buffer = NULL;\n+\t\t\t\tdetach_commit_buffer(commit);\n \t\t\t}\n \t\t\tobj->flags |= FLAG_CHECKED;\n \t\t}\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 39e8836..226f8f2 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -349,8 +349,7 @@ static int cmd_log_walk(struct rev_info *rev)\n \t\t\trev->max_count++;\n \t\tif (!rev->reflog_info) {\n \t\t\t/* we allow cycles in reflog ancestry */\n-\t\t\tfree(commit->buffer);\n-\t\t\tcommit->buffer = NULL;\n+\t\t\tfree_commit_buffer(commit);\n \t\t}\n \t\tfree_commit_list(commit->parents);\n \t\tcommit->parents = NULL;\n@@ -1508,8 +1507,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t    reopen_stdout(rev.numbered_files ? NULL : commit, NULL, &rev, quiet))\n \t\t\tdie(_(\"Failed to create output files\"));\n \t\tshown = log_tree_commit(&rev, commit);\n-\t\tfree(commit->buffer);\n-\t\tcommit->buffer = NULL;\n+\t\tfree_commit_buffer(commit);\n \n \t\t/* We put one extra blank line between formatted\n \t\t * patches and this flag is used by log-tree code\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 9f92905..e012ebe 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -173,8 +173,7 @@ static void finish_commit(struct commit *commit, void *data)\n \t\tfree_commit_list(commit->parents);\n \t\tcommit->parents = NULL;\n \t}\n-\tfree(commit->buffer);\n-\tcommit->buffer = NULL;\n+\tfree_commit_buffer(commit);\n }\n \n static void finish_object(struct object *obj,\ndiff --git a/commit.c b/commit.c\nindex fbdc480..11a05c1 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -245,6 +245,19 @@ int unregister_shallow(const unsigned char *sha1)\n \treturn 0;\n }\n \n+void free_commit_buffer(struct commit *commit)\n+{\n+\tfree(commit->buffer);\n+\tcommit->buffer = NULL;\n+}\n+\n+const void *detach_commit_buffer(struct commit *commit)\n+{\n+\tvoid *ret = commit->buffer;\n+\tcommit->buffer = NULL;\n+\treturn ret;\n+}\n+\n int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long size)\n {\n \tconst char *tail = buffer;\ndiff --git a/commit.h b/commit.h\nindex 4df48cb..d72ed43 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -51,6 +51,17 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s\n int parse_commit(struct commit *item);\n void parse_commit_or_die(struct commit *item);\n \n+/*\n+ * Free any cached object buffer associated with the commit.\n+ */\n+void free_commit_buffer(struct commit *);\n+\n+/*\n+ * Disassociate any cached object buffer from the commit, but do not free it.\n+ * The buffer (or NULL, if none) is returned.\n+ */\n+const void *detach_commit_buffer(struct commit *);\n+\n /* Find beginning and length of commit subject. */\n int find_commit_subject(const char *commit_buffer, const char **subject);\n \n-- \n2.0.0.729.g520999f\n"},{"id":"243793","messageId":"20140610214014.GI19147@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140610213509.GA26979@sigill.intra.peff.net","subject":"[PATCH 09/17] provide a helper to set the commit buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T21:40:14Z","receivedAt":"2014-06-10T21:40:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Right now this is just a one-liner, but abstracting it will\nmake it easier to change later.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/blame.c | 2 +-\n commit.c        | 7 ++++++-\n commit.h        | 6 ++++++\n object.c        | 2 +-\n 4 files changed, 14 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 6ce3c6d..0af3a18 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2313,7 +2313,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \t\t    ident, ident, path,\n \t\t    (!contents_from ? path :\n \t\t     (!strcmp(contents_from, \"-\") ? \"standard input\" : contents_from)));\n-\tcommit->buffer = strbuf_detach(&msg, NULL);\n+\tset_commit_buffer(commit, strbuf_detach(&msg, NULL));\n \n \tif (!contents_from || strcmp(\"-\", contents_from)) {\n \t\tstruct stat st;\ndiff --git a/commit.c b/commit.c\nindex 11a05c1..fc8b4e2 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -245,6 +245,11 @@ int unregister_shallow(const unsigned char *sha1)\n \treturn 0;\n }\n \n+void set_commit_buffer(struct commit *commit, void *buffer)\n+{\n+\tcommit->buffer = buffer;\n+}\n+\n void free_commit_buffer(struct commit *commit)\n {\n \tfree(commit->buffer);\n@@ -335,7 +340,7 @@ int parse_commit(struct commit *item)\n \t}\n \tret = parse_commit_buffer(item, buffer, size);\n \tif (save_commit_buffer && !ret) {\n-\t\titem->buffer = buffer;\n+\t\tset_commit_buffer(item, buffer);\n \t\treturn 0;\n \t}\n \tfree(buffer);\ndiff --git a/commit.h b/commit.h\nindex d72ed43..cc89128 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -52,6 +52,12 @@ int parse_commit(struct commit *item);\n void parse_commit_or_die(struct commit *item);\n \n /*\n+ * Associate an object buffer with the commit. The ownership of the\n+ * memory is handed over to the commit, and must be free()-able.\n+ */\n+void set_commit_buffer(struct commit *, void *buffer);\n+\n+/*\n  * Free any cached object buffer associated with the commit.\n  */\n void free_commit_buffer(struct commit *);\ndiff --git a/object.c b/object.c\nindex 57a0890..44ca657 100644\n--- a/object.c\n+++ b/object.c\n@@ -198,7 +198,7 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t\n \t\t\tif (parse_commit_buffer(commit, buffer, size))\n \t\t\t\treturn NULL;\n \t\t\tif (!commit->buffer) {\n-\t\t\t\tcommit->buffer = buffer;\n+\t\t\t\tset_commit_buffer(commit, buffer);\n \t\t\t\t*eaten_p = 1;\n \t\t\t}\n \t\t\tobj = &commit->object;\n-- \n2.0.0.729.g520999f\n"},{"id":"243794","messageId":"xmqq7g4o8mup.fsf@gitster.dls.corp.google.com","threadId":"36874","inReplyTo":"20140609181039.GE20315@sigill.intra.peff.net","subject":"Re: [PATCH 05/15] sequencer: use logmsg_reencode in get_message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-10T21:40:30Z","receivedAt":"2014-06-10T21:40:30Z","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> Note that we may be fixing a bug here. The existing code\n> does:\n>\n>   if (same_encoding(to, from))\n> \t  reencode_string(buf, to, from);\n>\n> That probably should have been \"!same_encoding\".\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I didn't actually test for the bug, so it's possible that I'm missing\n> something clever...\n\nThanks for spotting.  There is nothing clever going on.\n\n0e18bcd5 (reencode_string(): introduce and use same_encoding(),\n2012-10-18) has this misconversion.\n\ndiff --git a/sequencer.c b/sequencer.c\nindex bd62680..f2f5b13 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -58,7 +58,7 @@ static int get_message(struct commit *commit, struct commit_message *out)\n \n \tout->reencoded_message = NULL;\n \tout->message = commit->buffer;\n-\tif (strcmp(encoding, git_commit_encoding))\n+\tif (same_encoding(encoding, git_commit_encoding))\n \t\tout->reencoded_message = reencode_string(commit->buffer,\n \t\t\t\t\tgit_commit_encoding, encoding);\n \tif (out->reencoded_message)\n"},{"id":"243796","messageId":"20140610214039.GJ19147@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140610213509.GA26979@sigill.intra.peff.net","subject":"[PATCH 10/17] provide helpers to access the commit buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T21:40:39Z","receivedAt":"2014-06-10T21:40:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Many sites look at commit->buffer to get more detailed\ninformation than what is in the parsed commit struct.\nHowever, we sometimes drop commit->buffer to save memory,\nin which case the caller would need to read the object\nafresh. Some callers do this (leading to duplicated code),\nand others do not (which opens the possibility of a segfault\nif somebody else frees the buffer).\n\nLet's provide a pair of helpers, \"get\" and \"unuse\", that let\ncallers easily get the buffer. They will use the cached\nbuffer when possible, and otherwise load from disk using\nread_sha1_file.\n\nNote that we also need to add a \"get_cached\" variant which\nreturns NULL when we do not have a cached buffer. At first\nglance this seems to defeat the purpose of \"get\", which is\nto always provide a return value. However, some log code\npaths actually use the NULL-ness of commit->buffer as a\nboolean flag to decide whether to try printing the\ncommit. At least for now, we want to continue supporting\nthat use.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nTypofix in commit message since v1.\n\n commit.c | 28 ++++++++++++++++++++++++++++\n commit.h | 21 +++++++++++++++++++++\n 2 files changed, 49 insertions(+)\n\ndiff --git a/commit.c b/commit.c\nindex fc8b4e2..b6b0e0d 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -250,6 +250,34 @@ void set_commit_buffer(struct commit *commit, void *buffer)\n \tcommit->buffer = buffer;\n }\n \n+const void *get_cached_commit_buffer(const struct commit *commit)\n+{\n+\treturn commit->buffer;\n+}\n+\n+const void *get_commit_buffer(const struct commit *commit)\n+{\n+\tconst void *ret = get_cached_commit_buffer(commit);\n+\tif (!ret) {\n+\t\tenum object_type type;\n+\t\tunsigned long size;\n+\t\tret = read_sha1_file(commit->object.sha1, &type, &size);\n+\t\tif (!ret)\n+\t\t\tdie(\"cannot read commit object %s\",\n+\t\t\t    sha1_to_hex(commit->object.sha1));\n+\t\tif (type != OBJ_COMMIT)\n+\t\t\tdie(\"expected commit for %s, got %s\",\n+\t\t\t    sha1_to_hex(commit->object.sha1), typename(type));\n+\t}\n+\treturn ret;\n+}\n+\n+void unuse_commit_buffer(const struct commit *commit, const void *buffer)\n+{\n+\tif (commit->buffer != buffer)\n+\t\tfree((void *)buffer);\n+}\n+\n void free_commit_buffer(struct commit *commit)\n {\n \tfree(commit->buffer);\ndiff --git a/commit.h b/commit.h\nindex cc89128..259c0ae 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -58,6 +58,27 @@ void parse_commit_or_die(struct commit *item);\n void set_commit_buffer(struct commit *, void *buffer);\n \n /*\n+ * Get any cached object buffer associated with the commit. Returns NULL\n+ * if none. The resulting memory should not be freed.\n+ */\n+const void *get_cached_commit_buffer(const struct commit *);\n+\n+/*\n+ * Get the commit's object contents, either from cache or by reading the object\n+ * from disk. The resulting memory should not be modified, and must be given\n+ * to unuse_commit_buffer when the caller is done.\n+ */\n+const void *get_commit_buffer(const struct commit *);\n+\n+/*\n+ * Tell the commit subsytem that we are done with a particular commit buffer.\n+ * The commit and buffer should be the input and return value, respectively,\n+ * from an earlier call to get_commit_buffer.  The buffer may or may not be\n+ * freed by this call; callers should not access the memory afterwards.\n+ */\n+void unuse_commit_buffer(const struct commit *, const void *buffer);\n+\n+/*\n  * Free any cached object buffer associated with the commit.\n  */\n void free_commit_buffer(struct commit *);\n-- \n2.0.0.729.g520999f\n"},{"id":"243795","messageId":"20140610214046.GK19147@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140610213509.GA26979@sigill.intra.peff.net","subject":"[PATCH 11/17] use get_cached_commit_buffer where appropriate","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T21:40:46Z","receivedAt":"2014-06-10T21:40:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Some call sites check commit->buffer to see whether we have\na cached buffer, and if so, do some work with it. In the\nlong run we may want to switch these code paths to make\ntheir decision on a different boolean flag (because checking\nthe cache may get a little more expensive in the future).\nBut for now, we can easily support them by converting the\ncalls to use get_cached_commit_buffer.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/rev-list.c | 2 +-\n log-tree.c         | 2 +-\n object.c           | 2 +-\n 3 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex e012ebe..3fcbf21 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -106,7 +106,7 @@ static void show_commit(struct commit *commit, void *data)\n \telse\n \t\tputchar('\\n');\n \n-\tif (revs->verbose_header && commit->buffer) {\n+\tif (revs->verbose_header && get_cached_commit_buffer(commit)) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tstruct pretty_print_context ctx = {0};\n \t\tctx.abbrev = revs->abbrev;\ndiff --git a/log-tree.c b/log-tree.c\nindex cf2f86c..e9ef8ab 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -588,7 +588,7 @@ void show_log(struct rev_info *opt)\n \t\tshow_mergetag(opt, commit);\n \t}\n \n-\tif (!commit->buffer)\n+\tif (!get_cached_commit_buffer(commit))\n \t\treturn;\n \n \tif (opt->show_notes) {\ndiff --git a/object.c b/object.c\nindex 44ca657..67b6e35 100644\n--- a/object.c\n+++ b/object.c\n@@ -197,7 +197,7 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t\n \t\tif (commit) {\n \t\t\tif (parse_commit_buffer(commit, buffer, size))\n \t\t\t\treturn NULL;\n-\t\t\tif (!commit->buffer) {\n+\t\t\tif (!get_cached_commit_buffer(commit)) {\n \t\t\t\tset_commit_buffer(commit, buffer);\n \t\t\t\t*eaten_p = 1;\n \t\t\t}\n-- \n2.0.0.729.g520999f\n"},{"id":"243797","messageId":"20140610214102.GL19147@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140610213509.GA26979@sigill.intra.peff.net","subject":"[PATCH 12/17] use get_commit_buffer to avoid duplicate code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T21:41:02Z","receivedAt":"2014-06-10T21:41:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"For both of these sites, we already do the \"fallback to\nread_sha1_file\" trick. But we can shorten the code by just\nusing get_commit_buffer.\n\nNote that the error cases are slightly different when\nread_sha1_file fails. get_commit_buffer will die() if the\nobject cannot be loaded, or is a non-commit.\n\nFor get_sha1_oneline, this will almost certainly never\nhappen, as we will have just called parse_object (and if it\ndoes, it's probably worth complaining about).\n\nFor record_author_date, the new behavior is probably better;\nwe notify the user of the error instead of silently ignoring\nit. And because it's used only for sorting by author-date,\nsomebody examining a corrupt repo can fallback to the\nregular traversal order.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nTypofix in commit message since v1.\n\n commit.c    | 16 +++-------------\n sha1_name.c | 18 ++++--------------\n 2 files changed, 7 insertions(+), 27 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex b6b0e0d..1903dde 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -583,22 +583,12 @@ 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-\tchar *buffer = NULL;\n+\tconst char *buffer = get_commit_buffer(commit);\n \tstruct ident_split ident;\n \tchar *date_end;\n \tunsigned long date;\n \n-\tif (!commit->buffer) {\n-\t\tunsigned long size;\n-\t\tenum object_type type;\n-\t\tbuffer = read_sha1_file(commit->object.sha1, &type, &size);\n-\t\tif (!buffer)\n-\t\t\treturn;\n-\t}\n-\n-\tfor (buf = commit->buffer ? commit->buffer : buffer;\n-\t     buf;\n-\t     buf = line_end + 1) {\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@@ -619,7 +609,7 @@ static void record_author_date(struct author_date_slab *author_date,\n \t*(author_date_slab_at(author_date, commit)) = date;\n \n fail_exit:\n-\tfree(buffer);\n+\tunuse_commit_buffer(commit, buffer);\n }\n \n static int compare_commits_by_author_date(const void *a_, const void *b_,\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 2b6322f..0a65d23 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -862,27 +862,17 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1,\n \t\tcommit_list_insert(l->item, &backup);\n \t}\n \twhile (list) {\n-\t\tchar *p, *to_free = NULL;\n+\t\tconst char *p, *buf;\n \t\tstruct commit *commit;\n-\t\tenum object_type type;\n-\t\tunsigned long size;\n \t\tint matches;\n \n \t\tcommit = pop_most_recent_commit(&list, ONELINE_SEEN);\n \t\tif (!parse_object(commit->object.sha1))\n \t\t\tcontinue;\n-\t\tif (commit->buffer)\n-\t\t\tp = commit->buffer;\n-\t\telse {\n-\t\t\tp = read_sha1_file(commit->object.sha1, &type, &size);\n-\t\t\tif (!p)\n-\t\t\t\tcontinue;\n-\t\t\tto_free = p;\n-\t\t}\n-\n-\t\tp = strstr(p, \"\\n\\n\");\n+\t\tbuf = get_commit_buffer(commit);\n+\t\tp = strstr(buf, \"\\n\\n\");\n \t\tmatches = p && !regexec(&regex, p + 2, 0, NULL, 0);\n-\t\tfree(to_free);\n+\t\tunuse_commit_buffer(commit, buf);\n \n \t\tif (matches) {\n \t\t\thashcpy(sha1, commit->object.sha1);\n-- \n2.0.0.729.g520999f\n"},{"id":"243798","messageId":"20140610214139.GM19147@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140610213509.GA26979@sigill.intra.peff.net","subject":"[PATCH 13/17] convert logmsg_reencode to get_commit_buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T21:41:39Z","receivedAt":"2014-06-10T21:41:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Like the callsites in the previous commit, logmsg_reencode\nalready falls back to read_sha1_file when necessary.\nHowever, I split its conversion out into its own commit\nbecause it's a bit more complex.\n\nWe return either:\n\n  1. The original commit->buffer\n\n  2. A newly allocated buffer from read_sha1_file\n\n  3. A reencoded buffer (based on either 1 or 2 above).\n\nwhile trying to do as few extra reads/allocations as\npossible. Callers currently free the result with\nlogmsg_free, but we can simplify this by pointing them\nstraight to unuse_commit_buffer. This is a slight layering\nviolation, in that we may be passing a buffer from (3).\nHowever, since the end result is to free() anything except\n(1), which is unlikely to change, and because this makes the\ninterface much simpler, it's a reasonable bending of the\nrules.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/blame.c |  4 ++--\n builtin/reset.c |  2 +-\n commit.h        |  1 -\n pretty.c        | 40 +++++++++++-----------------------------\n revision.c      |  2 +-\n sequencer.c     |  2 +-\n 6 files changed, 16 insertions(+), 35 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 0af3a18..cde19eb 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1666,7 +1666,7 @@ static void get_commit_info(struct commit *commit,\n \t\t    &ret->author_time, &ret->author_tz);\n \n \tif (!detailed) {\n-\t\tlogmsg_free(message, commit);\n+\t\tunuse_commit_buffer(commit, message);\n \t\treturn;\n \t}\n \n@@ -1680,7 +1680,7 @@ static void get_commit_info(struct commit *commit,\n \telse\n \t\tstrbuf_addf(&ret->summary, \"(%s)\", sha1_to_hex(commit->object.sha1));\n \n-\tlogmsg_free(message, commit);\n+\tunuse_commit_buffer(commit, message);\n }\n \n /*\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 7ebee07..850d532 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -109,7 +109,7 @@ static void print_new_head_line(struct commit *commit)\n \t}\n \telse\n \t\tprintf(\"\\n\");\n-\tlogmsg_free(msg, commit);\n+\tunuse_commit_buffer(commit, msg);\n }\n \n static void update_index_from_diff(struct diff_queue_struct *q,\ndiff --git a/commit.h b/commit.h\nindex 259c0ae..5ce5ce7 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -156,7 +156,6 @@ struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */\n extern const char *logmsg_reencode(const struct commit *commit,\n \t\t\t\t   char **commit_encoding,\n \t\t\t\t   const char *output_encoding);\n-extern void logmsg_free(const char *msg, const struct commit *commit);\n extern void get_commit_format(const char *arg, struct rev_info *);\n extern const char *format_subject(struct strbuf *sb, const char *msg,\n \t\t\t\t  const char *line_separator);\ndiff --git a/pretty.c b/pretty.c\nindex d152de2..8fd60cd 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -613,22 +613,9 @@ const char *logmsg_reencode(const struct commit *commit,\n \tstatic const char *utf8 = \"UTF-8\";\n \tconst char *use_encoding;\n \tchar *encoding;\n-\tchar *msg = commit->buffer;\n+\tconst char *msg = get_commit_buffer(commit);\n \tchar *out;\n \n-\tif (!msg) {\n-\t\tenum object_type type;\n-\t\tunsigned long size;\n-\n-\t\tmsg = read_sha1_file(commit->object.sha1, &type, &size);\n-\t\tif (!msg)\n-\t\t\tdie(\"Cannot read commit object %s\",\n-\t\t\t    sha1_to_hex(commit->object.sha1));\n-\t\tif (type != OBJ_COMMIT)\n-\t\t\tdie(\"Expected commit for '%s', got %s\",\n-\t\t\t    sha1_to_hex(commit->object.sha1), typename(type));\n-\t}\n-\n \tif (!output_encoding || !*output_encoding) {\n \t\tif (commit_encoding)\n \t\t\t*commit_encoding =\n@@ -652,12 +639,13 @@ const char *logmsg_reencode(const struct commit *commit,\n \t\t * Otherwise, we still want to munge the encoding header in the\n \t\t * result, which will be done by modifying the buffer. If we\n \t\t * are using a fresh copy, we can reuse it. But if we are using\n-\t\t * the cached copy from commit->buffer, we need to duplicate it\n-\t\t * to avoid munging commit->buffer.\n+\t\t * the cached copy from get_commit_buffer, we need to duplicate it\n+\t\t * to avoid munging the cached copy.\n \t\t */\n-\t\tout = msg;\n-\t\tif (out == commit->buffer)\n-\t\t\tout = xstrdup(out);\n+\t\tif (msg == get_cached_commit_buffer(commit))\n+\t\t\tout = xstrdup(msg);\n+\t\telse\n+\t\t\tout = (char *)msg;\n \t}\n \telse {\n \t\t/*\n@@ -667,8 +655,8 @@ const char *logmsg_reencode(const struct commit *commit,\n \t\t * copy, we can free it.\n \t\t */\n \t\tout = reencode_string(msg, output_encoding, use_encoding);\n-\t\tif (out && msg != commit->buffer)\n-\t\t\tfree(msg);\n+\t\tif (out)\n+\t\t\tunuse_commit_buffer(commit, msg);\n \t}\n \n \t/*\n@@ -687,12 +675,6 @@ const char *logmsg_reencode(const struct commit *commit,\n \treturn out ? out : msg;\n }\n \n-void logmsg_free(const char *msg, const struct commit *commit)\n-{\n-\tif (msg != commit->buffer)\n-\t\tfree((void *)msg);\n-}\n-\n static int mailmap_name(const char **email, size_t *email_len,\n \t\t\tconst char **name, size_t *name_len)\n {\n@@ -1531,7 +1513,7 @@ void format_commit_message(const struct commit *commit,\n \t}\n \n \tfree(context.commit_encoding);\n-\tlogmsg_free(context.message, commit);\n+\tunuse_commit_buffer(commit, context.message);\n \tfree(context.signature_check.gpg_output);\n \tfree(context.signature_check.signer);\n }\n@@ -1767,7 +1749,7 @@ void pretty_print_commit(struct pretty_print_context *pp,\n \tif (pp->fmt == CMIT_FMT_EMAIL && sb->len <= beginning_of_body)\n \t\tstrbuf_addch(sb, '\\n');\n \n-\tlogmsg_free(reencoded, commit);\n+\tunuse_commit_buffer(commit, reencoded);\n }\n \n void pp_commit_easy(enum cmit_fmt fmt, const struct commit *commit,\ndiff --git a/revision.c b/revision.c\nindex be151ef..1cc91e5 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2844,7 +2844,7 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n \t\tretval = grep_buffer(&opt->grep_filter,\n \t\t\t\t     (char *)message, strlen(message));\n \tstrbuf_release(&buf);\n-\tlogmsg_free(message, commit);\n+\tunuse_commit_buffer(commit, message);\n \treturn retval;\n }\n \ndiff --git a/sequencer.c b/sequencer.c\nindex 3fcab4d..4632f7d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -154,7 +154,7 @@ static int get_message(struct commit *commit, struct commit_message *out)\n static void free_message(struct commit *commit, struct commit_message *msg)\n {\n \tfree(msg->parent_label);\n-\tlogmsg_free(msg->message, commit);\n+\tunuse_commit_buffer(commit, msg->message);\n }\n \n static void write_cherry_pick_head(struct commit *commit, const char *pseudoref)\n-- \n2.0.0.729.g520999f\n"},{"id":"243799","messageId":"20140610214151.GN19147@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140610213509.GA26979@sigill.intra.peff.net","subject":"[PATCH 14/17] use get_commit_buffer everywhere","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T21:41:51Z","receivedAt":"2014-06-10T21:41:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Each of these sites assumes that commit->buffer is valid.\nSince they would segfault if this was not the case, they are\nlikely to be correct in practice. However, we can\nfuture-proof them by using get_commit_buffer.\n\nAnd as a side effect, we abstract away the final bare uses\nof commit->buffer.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/fast-export.c   |  5 ++++-\n builtin/fmt-merge-msg.c |  5 ++++-\n builtin/log.c           |  7 +++++--\n fsck.c                  | 13 +++++++++++--\n merge-recursive.c       |  4 +++-\n notes-merge.c           |  4 +++-\n sequencer.c             |  4 +++-\n 7 files changed, 33 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex b8d8a3a..7ee5e08 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -279,6 +279,7 @@ static const char *find_encoding(const char *begin, const char *end)\n static void handle_commit(struct commit *commit, struct rev_info *rev)\n {\n \tint saved_output_format = rev->diffopt.output_format;\n+\tconst char *commit_buffer;\n \tconst char *author, *author_end, *committer, *committer_end;\n \tconst char *encoding, *message;\n \tchar *reencoded = NULL;\n@@ -288,7 +289,8 @@ static void handle_commit(struct commit *commit, struct rev_info *rev)\n \trev->diffopt.output_format = DIFF_FORMAT_CALLBACK;\n \n \tparse_commit_or_die(commit);\n-\tauthor = strstr(commit->buffer, \"\\nauthor \");\n+\tcommit_buffer = get_commit_buffer(commit);\n+\tauthor = strstr(commit_buffer, \"\\nauthor \");\n \tif (!author)\n \t\tdie (\"Could not find author in commit %s\",\n \t\t     sha1_to_hex(commit->object.sha1));\n@@ -335,6 +337,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev)\n \t\t\t  ? strlen(message) : 0),\n \t       reencoded ? reencoded : message ? message : \"\");\n \tfree(reencoded);\n+\tunuse_commit_buffer(commit, commit_buffer);\n \n \tfor (i = 0, p = commit->parents; p; p = p->next) {\n \t\tint mark = get_object_mark(&p->item->object);\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex 3906eda..01f6d59 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -230,12 +230,14 @@ static void add_branch_desc(struct strbuf *out, const char *name)\n static void record_person(int which, struct string_list *people,\n \t\t\t  struct commit *commit)\n {\n+\tconst char *buffer;\n \tchar *name_buf, *name, *name_end;\n \tstruct string_list_item *elem;\n \tconst char *field;\n \n \tfield = (which == 'a') ? \"\\nauthor \" : \"\\ncommitter \";\n-\tname = strstr(commit->buffer, field);\n+\tbuffer = get_commit_buffer(commit);\n+\tname = strstr(buffer, field);\n \tif (!name)\n \t\treturn;\n \tname += strlen(field);\n@@ -247,6 +249,7 @@ static void record_person(int which, struct string_list *people,\n \tif (name_end < name)\n \t\treturn;\n \tname_buf = xmemdupz(name, name_end - name + 1);\n+\tunuse_commit_buffer(commit, buffer);\n \n \telem = string_list_lookup(people, name_buf);\n \tif (!elem) {\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 226f8f2..2c74260 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -918,9 +918,12 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \tlog_write_email_headers(rev, head, &pp.subject, &pp.after_subject,\n \t\t\t\t&need_8bit_cte);\n \n-\tfor (i = 0; !need_8bit_cte && i < nr; i++)\n-\t\tif (has_non_ascii(list[i]->buffer))\n+\tfor (i = 0; !need_8bit_cte && i < nr; i++) {\n+\t\tconst char *buf = get_commit_buffer(list[i]);\n+\t\tif (has_non_ascii(buf))\n \t\t\tneed_8bit_cte = 1;\n+\t\tunuse_commit_buffer(list[i], buf);\n+\t}\n \n \tif (!branch_name)\n \t\tbranch_name = find_branch_name(rev);\ndiff --git a/fsck.c b/fsck.c\nindex abed62b..8223780 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -276,9 +276,10 @@ static int fsck_ident(const char **ident, struct object *obj, fsck_error error_f\n \treturn 0;\n }\n \n-static int fsck_commit(struct commit *commit, fsck_error error_func)\n+static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n+\t\t\t      fsck_error error_func)\n {\n-\tconst char *buffer = commit->buffer, *tmp;\n+\tconst char *tmp;\n \tunsigned char tree_sha1[20], sha1[20];\n \tstruct commit_graft *graft;\n \tint parents = 0;\n@@ -336,6 +337,14 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)\n \treturn 0;\n }\n \n+static int fsck_commit(struct commit *commit, fsck_error error_func)\n+{\n+\tconst char *buffer = get_commit_buffer(commit);\n+\tint ret = fsck_commit_buffer(commit, buffer, error_func);\n+\tunuse_commit_buffer(commit, buffer);\n+\treturn ret;\n+}\n+\n static int fsck_tag(struct tag *tag, fsck_error error_func)\n {\n \tstruct object *tagged = tag->tagged;\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 2b37d42..0dd6039 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -190,9 +190,11 @@ static void output_commit_title(struct merge_options *o, struct commit *commit)\n \t\t\tprintf(_(\"(bad commit)\\n\"));\n \t\telse {\n \t\t\tconst char *title;\n-\t\t\tint len = find_commit_subject(commit->buffer, &title);\n+\t\t\tconst char *msg = get_commit_buffer(commit);\n+\t\t\tint len = find_commit_subject(msg, &title);\n \t\t\tif (len)\n \t\t\t\tprintf(\"%.*s\\n\", len, title);\n+\t\t\tunuse_commit_buffer(commit, msg);\n \t\t}\n \t}\n }\ndiff --git a/notes-merge.c b/notes-merge.c\nindex 697cec3..e804db2 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -672,7 +672,8 @@ int notes_merge_commit(struct notes_merge_options *o,\n \tDIR *dir;\n \tstruct dirent *e;\n \tstruct strbuf path = STRBUF_INIT;\n-\tchar *msg = strstr(partial_commit->buffer, \"\\n\\n\");\n+\tconst char *buffer = get_commit_buffer(partial_commit);\n+\tconst char *msg = strstr(buffer, \"\\n\\n\");\n \tint baselen;\n \n \tstrbuf_addstr(&path, git_path(NOTES_MERGE_WORKTREE));\n@@ -721,6 +722,7 @@ int notes_merge_commit(struct notes_merge_options *o,\n \n \tcreate_notes_commit(partial_tree, partial_commit->parents,\n \t\t\t    msg, strlen(msg), result_sha1);\n+\tunuse_commit_buffer(partial_commit, buffer);\n \tif (o->verbosity >= 4)\n \t\tprintf(\"Finalized notes merge commit: %s\\n\",\n \t\t\tsha1_to_hex(result_sha1));\ndiff --git a/sequencer.c b/sequencer.c\nindex 4632f7d..5609ab3 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -666,10 +666,12 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,\n \tint subject_len;\n \n \tfor (cur = todo_list; cur; cur = cur->next) {\n+\t\tconst char *commit_buffer = get_commit_buffer(cur->item);\n \t\tsha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);\n-\t\tsubject_len = find_commit_subject(cur->item->buffer, &subject);\n+\t\tsubject_len = find_commit_subject(commit_buffer, &subject);\n \t\tstrbuf_addf(buf, \"%s %s %.*s\\n\", action_str, sha1_abbrev,\n \t\t\tsubject_len, subject);\n+\t\tunuse_commit_buffer(cur->item, commit_buffer);\n \t}\n \treturn 0;\n }\n-- \n2.0.0.729.g520999f\n"},{"id":"243800","messageId":"20140610214251.GO19147@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140610213509.GA26979@sigill.intra.peff.net","subject":"[PATCH 15/17] commit-slab: provide a static initializer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T21:42:51Z","receivedAt":"2014-06-10T21:42:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Callers currently must use init_foo_slab() at runtime before\naccessing a slab. For global slabs, it's much nicer if we\ncan initialize them in BSS, so that each user does not have\nto add code to check-and-initialize.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThere was no comment on this one in v1. I'd be curious if anyone has\ncomments on what I wrote in:\n\n  http://article.gmane.org/gmane.comp.version-control.git/251099\n\n commit-slab.h | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/commit-slab.h b/commit-slab.h\nindex cc114b5..375c9c7 100644\n--- a/commit-slab.h\n+++ b/commit-slab.h\n@@ -117,4 +117,16 @@ static int stat_ ##slabname## realloc\n  * catch because GCC silently parses it by default.\n  */\n \n+/*\n+ * Statically initialize a commit slab named \"var\". Note that this\n+ * evaluates \"stride\" multiple times! Example:\n+ *\n+ *   struct indegree indegrees = COMMIT_SLAB_INIT(1, indegrees);\n+ *\n+ */\n+#define COMMIT_SLAB_INIT(stride, var) { \\\n+\tCOMMIT_SLAB_SIZE / sizeof(**((var).slab)) / (stride), \\\n+\t(stride), 0, NULL \\\n+}\n+\n #endif /* COMMIT_SLAB_H */\n-- \n2.0.0.729.g520999f\n"},{"id":"243801","messageId":"20140610214302.GP19147@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140610213509.GA26979@sigill.intra.peff.net","subject":"[PATCH 16/17] commit: convert commit->buffer to a slab","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T21:43:02Z","receivedAt":"2014-06-10T21:43:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This will make it easier to manage the buffer cache\nindependently of the \"struct commit\" objects. It also\nshrinks \"struct commit\" by one pointer, which may be\nhelpful.\n\nUnfortunately it does not reduce the max memory size of\nsomething like \"rev-list\", because rev-list uses\nget_cached_commit_buffer() to decide not to show each\ncommit's output (and due to the design of slab_at, accessing\nthe slab requires us to extend it, allocating exactly the\nsame number of buffer pointers we dropped from the commit\nstructs).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n commit.c | 20 +++++++++++++-------\n commit.h |  1 -\n 2 files changed, 13 insertions(+), 8 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 1903dde..e289c78 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -245,14 +245,17 @@ int unregister_shallow(const unsigned char *sha1)\n \treturn 0;\n }\n \n+define_commit_slab(buffer_slab, void *);\n+static struct buffer_slab buffer_slab = COMMIT_SLAB_INIT(1, buffer_slab);\n+\n void set_commit_buffer(struct commit *commit, void *buffer)\n {\n-\tcommit->buffer = buffer;\n+\t*buffer_slab_at(&buffer_slab, commit) = buffer;\n }\n \n const void *get_cached_commit_buffer(const struct commit *commit)\n {\n-\treturn commit->buffer;\n+\treturn *buffer_slab_at(&buffer_slab, commit);\n }\n \n const void *get_commit_buffer(const struct commit *commit)\n@@ -274,20 +277,23 @@ const void *get_commit_buffer(const struct commit *commit)\n \n void unuse_commit_buffer(const struct commit *commit, const void *buffer)\n {\n-\tif (commit->buffer != buffer)\n+\tvoid *cached = *buffer_slab_at(&buffer_slab, commit);\n+\tif (cached != buffer)\n \t\tfree((void *)buffer);\n }\n \n void free_commit_buffer(struct commit *commit)\n {\n-\tfree(commit->buffer);\n-\tcommit->buffer = NULL;\n+\tvoid **b = buffer_slab_at(&buffer_slab, commit);\n+\tfree(*b);\n+\t*b = NULL;\n }\n \n const void *detach_commit_buffer(struct commit *commit)\n {\n-\tvoid *ret = commit->buffer;\n-\tcommit->buffer = NULL;\n+\tvoid **b = buffer_slab_at(&buffer_slab, commit);\n+\tvoid *ret = *b;\n+\t*b = NULL;\n \treturn ret;\n }\n \ndiff --git a/commit.h b/commit.h\nindex 5ce5ce7..e1c2569 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -20,7 +20,6 @@ struct commit {\n \tunsigned long date;\n \tstruct commit_list *parents;\n \tstruct tree *tree;\n-\tchar *buffer;\n };\n \n extern int save_commit_buffer;\n-- \n2.0.0.729.g520999f\n"},{"id":"243802","messageId":"20140610214412.GQ19147@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140610213509.GA26979@sigill.intra.peff.net","subject":"[PATCH 17/17] commit: record buffer length in cache","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T21:44:13Z","receivedAt":"2014-06-10T21:44:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Most callsites which use the commit buffer try to use the\ncached version attached to the commit, rather than\nre-reading from disk. Unfortunately, that interface provides\nonly a pointer to the NUL-terminated buffer, with no\nindication of the original length.\n\nFor the most part, this doesn't matter. People do not put\nNULs in their commit messages, and the log code is happy to\ntreat it all as a NUL-terminated string. However, some code\npaths do care. For example, when checking signatures, we\nwant to be very careful that we verify all the bytes to\navoid malicious trickery.\n\nThis patch just adds an optional \"size\" out-pointer to\nget_commit_buffer and friends. The existing callers all pass\nNULL (there did not seem to be any obvious sites where we\ncould avoid an immediate strlen() call, though perhaps with\nsome further refactoring we could).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nSince v1, fixes dubious use of strbuf as pointed out by Christian.\n\n builtin/blame.c         | 14 ++++++++++++-\n builtin/fast-export.c   |  2 +-\n builtin/fmt-merge-msg.c |  2 +-\n builtin/index-pack.c    |  2 +-\n builtin/log.c           |  2 +-\n builtin/rev-list.c      |  2 +-\n commit.c                | 54 ++++++++++++++++++++++++++++++++-----------------\n commit.h                |  8 ++++----\n fsck.c                  |  2 +-\n log-tree.c              |  2 +-\n merge-recursive.c       |  2 +-\n notes-merge.c           |  2 +-\n object.c                |  4 ++--\n pretty.c                |  4 ++--\n sequencer.c             |  2 +-\n sha1_name.c             |  2 +-\n 16 files changed, 68 insertions(+), 38 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex cde19eb..53f43ab 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2266,6 +2266,18 @@ static void append_merge_parents(struct commit_list **tail)\n }\n \n /*\n+ * This isn't as simple as passing sb->buf and sb->len, because we\n+ * want to transfer ownership of the buffer to the commit (so we\n+ * must use detach).\n+ */\n+static void set_commit_buffer_from_strbuf(struct commit *c, struct strbuf *sb)\n+{\n+\tsize_t len;\n+\tvoid *buf = strbuf_detach(sb, &len);\n+\tset_commit_buffer(c, buf, len);\n+}\n+\n+/*\n  * Prepare a dummy commit that represents the work tree (or staged) item.\n  * Note that annotating work tree item never works in the reverse.\n  */\n@@ -2313,7 +2325,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \t\t    ident, ident, path,\n \t\t    (!contents_from ? path :\n \t\t     (!strcmp(contents_from, \"-\") ? \"standard input\" : contents_from)));\n-\tset_commit_buffer(commit, strbuf_detach(&msg, NULL));\n+\tset_commit_buffer_from_strbuf(commit, &msg);\n \n \tif (!contents_from || strcmp(\"-\", contents_from)) {\n \t\tstruct stat st;\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 7ee5e08..05d161f 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -289,7 +289,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev)\n \trev->diffopt.output_format = DIFF_FORMAT_CALLBACK;\n \n \tparse_commit_or_die(commit);\n-\tcommit_buffer = get_commit_buffer(commit);\n+\tcommit_buffer = get_commit_buffer(commit, NULL);\n \tauthor = strstr(commit_buffer, \"\\nauthor \");\n \tif (!author)\n \t\tdie (\"Could not find author in commit %s\",\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex 01f6d59..ef8b254 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -236,7 +236,7 @@ static void record_person(int which, struct string_list *people,\n \tconst char *field;\n \n \tfield = (which == 'a') ? \"\\nauthor \" : \"\\ncommitter \";\n-\tbuffer = get_commit_buffer(commit);\n+\tbuffer = get_commit_buffer(commit, NULL);\n \tname = strstr(buffer, field);\n \tif (!name)\n \t\treturn;\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 995df39..25e3e9b 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -786,7 +786,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\t}\n \t\t\tif (obj->type == OBJ_COMMIT) {\n \t\t\t\tstruct commit *commit = (struct commit *) obj;\n-\t\t\t\tdetach_commit_buffer(commit);\n+\t\t\t\tdetach_commit_buffer(commit, NULL);\n \t\t\t}\n \t\t\tobj->flags |= FLAG_CHECKED;\n \t\t}\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 2c74260..c599eac 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -919,7 +919,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \t\t\t\t&need_8bit_cte);\n \n \tfor (i = 0; !need_8bit_cte && i < nr; i++) {\n-\t\tconst char *buf = get_commit_buffer(list[i]);\n+\t\tconst char *buf = get_commit_buffer(list[i], NULL);\n \t\tif (has_non_ascii(buf))\n \t\t\tneed_8bit_cte = 1;\n \t\tunuse_commit_buffer(list[i], buf);\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 3fcbf21..ff84a82 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -106,7 +106,7 @@ static void show_commit(struct commit *commit, void *data)\n \telse\n \t\tputchar('\\n');\n \n-\tif (revs->verbose_header && get_cached_commit_buffer(commit)) {\n+\tif (revs->verbose_header && get_cached_commit_buffer(commit, NULL)) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tstruct pretty_print_context ctx = {0};\n \t\tctx.abbrev = revs->abbrev;\ndiff --git a/commit.c b/commit.c\nindex e289c78..a036e18 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -245,22 +245,31 @@ int unregister_shallow(const unsigned char *sha1)\n \treturn 0;\n }\n \n-define_commit_slab(buffer_slab, void *);\n+struct commit_buffer {\n+\tvoid *buffer;\n+\tunsigned long size;\n+};\n+define_commit_slab(buffer_slab, struct commit_buffer);\n static struct buffer_slab buffer_slab = COMMIT_SLAB_INIT(1, buffer_slab);\n \n-void set_commit_buffer(struct commit *commit, void *buffer)\n+void set_commit_buffer(struct commit *commit, void *buffer, unsigned long size)\n {\n-\t*buffer_slab_at(&buffer_slab, commit) = buffer;\n+\tstruct commit_buffer *v = buffer_slab_at(&buffer_slab, commit);\n+\tv->buffer = buffer;\n+\tv->size = size;\n }\n \n-const void *get_cached_commit_buffer(const struct commit *commit)\n+const void *get_cached_commit_buffer(const struct commit *commit, unsigned long *sizep)\n {\n-\treturn *buffer_slab_at(&buffer_slab, commit);\n+\tstruct commit_buffer *v = buffer_slab_at(&buffer_slab, commit);\n+\tif (sizep)\n+\t\t*sizep = v->size;\n+\treturn v->buffer;\n }\n \n-const void *get_commit_buffer(const struct commit *commit)\n+const void *get_commit_buffer(const struct commit *commit, unsigned long *sizep)\n {\n-\tconst void *ret = get_cached_commit_buffer(commit);\n+\tconst void *ret = get_cached_commit_buffer(commit, sizep);\n \tif (!ret) {\n \t\tenum object_type type;\n \t\tunsigned long size;\n@@ -271,29 +280,38 @@ const void *get_commit_buffer(const struct commit *commit)\n \t\tif (type != OBJ_COMMIT)\n \t\t\tdie(\"expected commit for %s, got %s\",\n \t\t\t    sha1_to_hex(commit->object.sha1), typename(type));\n+\t\tif (sizep)\n+\t\t\t*sizep = size;\n \t}\n \treturn ret;\n }\n \n void unuse_commit_buffer(const struct commit *commit, const void *buffer)\n {\n-\tvoid *cached = *buffer_slab_at(&buffer_slab, commit);\n-\tif (cached != buffer)\n+\tstruct commit_buffer *v = buffer_slab_at(&buffer_slab, commit);\n+\tif (v->buffer != buffer)\n \t\tfree((void *)buffer);\n }\n \n void free_commit_buffer(struct commit *commit)\n {\n-\tvoid **b = buffer_slab_at(&buffer_slab, commit);\n-\tfree(*b);\n-\t*b = NULL;\n+\tstruct commit_buffer *v = buffer_slab_at(&buffer_slab, commit);\n+\tfree(v->buffer);\n+\tv->buffer = NULL;\n+\tv->size = 0;\n }\n \n-const void *detach_commit_buffer(struct commit *commit)\n+const void *detach_commit_buffer(struct commit *commit, unsigned long *sizep)\n {\n-\tvoid **b = buffer_slab_at(&buffer_slab, commit);\n-\tvoid *ret = *b;\n-\t*b = NULL;\n+\tstruct commit_buffer *v = buffer_slab_at(&buffer_slab, commit);\n+\tvoid *ret;\n+\n+\tret = v->buffer;\n+\tif (sizep)\n+\t\t*sizep = v->size;\n+\n+\tv->buffer = NULL;\n+\tv->size = 0;\n \treturn ret;\n }\n \n@@ -374,7 +392,7 @@ int parse_commit(struct commit *item)\n \t}\n \tret = parse_commit_buffer(item, buffer, size);\n \tif (save_commit_buffer && !ret) {\n-\t\tset_commit_buffer(item, buffer);\n+\t\tset_commit_buffer(item, buffer, size);\n \t\treturn 0;\n \t}\n \tfree(buffer);\n@@ -589,7 +607,7 @@ 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);\n+\tconst char *buffer = get_commit_buffer(commit, NULL);\n \tstruct ident_split ident;\n \tchar *date_end;\n \tunsigned long date;\ndiff --git a/commit.h b/commit.h\nindex e1c2569..61559a9 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -54,20 +54,20 @@ void parse_commit_or_die(struct commit *item);\n  * Associate an object buffer with the commit. The ownership of the\n  * memory is handed over to the commit, and must be free()-able.\n  */\n-void set_commit_buffer(struct commit *, void *buffer);\n+void set_commit_buffer(struct commit *, void *buffer, unsigned long size);\n \n /*\n  * Get any cached object buffer associated with the commit. Returns NULL\n  * if none. The resulting memory should not be freed.\n  */\n-const void *get_cached_commit_buffer(const struct commit *);\n+const void *get_cached_commit_buffer(const struct commit *, unsigned long *size);\n \n /*\n  * Get the commit's object contents, either from cache or by reading the object\n  * from disk. The resulting memory should not be modified, and must be given\n  * to unuse_commit_buffer when the caller is done.\n  */\n-const void *get_commit_buffer(const struct commit *);\n+const void *get_commit_buffer(const struct commit *, unsigned long *size);\n \n /*\n  * Tell the commit subsytem that we are done with a particular commit buffer.\n@@ -86,7 +86,7 @@ void free_commit_buffer(struct commit *);\n  * Disassociate any cached object buffer from the commit, but do not free it.\n  * The buffer (or NULL, if none) is returned.\n  */\n-const void *detach_commit_buffer(struct commit *);\n+const void *detach_commit_buffer(struct commit *, unsigned long *sizep);\n \n /* Find beginning and length of commit subject. */\n int find_commit_subject(const char *commit_buffer, const char **subject);\ndiff --git a/fsck.c b/fsck.c\nindex 8223780..a7233c8 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -339,7 +339,7 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n \n static int fsck_commit(struct commit *commit, fsck_error error_func)\n {\n-\tconst char *buffer = get_commit_buffer(commit);\n+\tconst char *buffer = get_commit_buffer(commit, NULL);\n \tint ret = fsck_commit_buffer(commit, buffer, error_func);\n \tunuse_commit_buffer(commit, buffer);\n \treturn ret;\ndiff --git a/log-tree.c b/log-tree.c\nindex e9ef8ab..4447021 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -588,7 +588,7 @@ void show_log(struct rev_info *opt)\n \t\tshow_mergetag(opt, commit);\n \t}\n \n-\tif (!get_cached_commit_buffer(commit))\n+\tif (!get_cached_commit_buffer(commit, NULL))\n \t\treturn;\n \n \tif (opt->show_notes) {\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 0dd6039..d38a3b2 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -190,7 +190,7 @@ static void output_commit_title(struct merge_options *o, struct commit *commit)\n \t\t\tprintf(_(\"(bad commit)\\n\"));\n \t\telse {\n \t\t\tconst char *title;\n-\t\t\tconst char *msg = get_commit_buffer(commit);\n+\t\t\tconst char *msg = get_commit_buffer(commit, NULL);\n \t\t\tint len = find_commit_subject(msg, &title);\n \t\t\tif (len)\n \t\t\t\tprintf(\"%.*s\\n\", len, title);\ndiff --git a/notes-merge.c b/notes-merge.c\nindex e804db2..fd5fae2 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -672,7 +672,7 @@ int notes_merge_commit(struct notes_merge_options *o,\n \tDIR *dir;\n \tstruct dirent *e;\n \tstruct strbuf path = STRBUF_INIT;\n-\tconst char *buffer = get_commit_buffer(partial_commit);\n+\tconst char *buffer = get_commit_buffer(partial_commit, NULL);\n \tconst char *msg = strstr(buffer, \"\\n\\n\");\n \tint baselen;\n \ndiff --git a/object.c b/object.c\nindex 67b6e35..9c31e9a 100644\n--- a/object.c\n+++ b/object.c\n@@ -197,8 +197,8 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t\n \t\tif (commit) {\n \t\t\tif (parse_commit_buffer(commit, buffer, size))\n \t\t\t\treturn NULL;\n-\t\t\tif (!get_cached_commit_buffer(commit)) {\n-\t\t\t\tset_commit_buffer(commit, buffer);\n+\t\t\tif (!get_cached_commit_buffer(commit, NULL)) {\n+\t\t\t\tset_commit_buffer(commit, buffer, size);\n \t\t\t\t*eaten_p = 1;\n \t\t\t}\n \t\t\tobj = &commit->object;\ndiff --git a/pretty.c b/pretty.c\nindex 8fd60cd..280d7d0 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -613,7 +613,7 @@ const char *logmsg_reencode(const struct commit *commit,\n \tstatic const char *utf8 = \"UTF-8\";\n \tconst char *use_encoding;\n \tchar *encoding;\n-\tconst char *msg = get_commit_buffer(commit);\n+\tconst char *msg = get_commit_buffer(commit, NULL);\n \tchar *out;\n \n \tif (!output_encoding || !*output_encoding) {\n@@ -642,7 +642,7 @@ const char *logmsg_reencode(const struct commit *commit,\n \t\t * the cached copy from get_commit_buffer, we need to duplicate it\n \t\t * to avoid munging the cached copy.\n \t\t */\n-\t\tif (msg == get_cached_commit_buffer(commit))\n+\t\tif (msg == get_cached_commit_buffer(commit, NULL))\n \t\t\tout = xstrdup(msg);\n \t\telse\n \t\t\tout = (char *)msg;\ndiff --git a/sequencer.c b/sequencer.c\nindex 5609ab3..d83f810 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -666,7 +666,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,\n \tint subject_len;\n \n \tfor (cur = todo_list; cur; cur = cur->next) {\n-\t\tconst char *commit_buffer = get_commit_buffer(cur->item);\n+\t\tconst char *commit_buffer = get_commit_buffer(cur->item, NULL);\n \t\tsha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);\n \t\tsubject_len = find_commit_subject(commit_buffer, &subject);\n \t\tstrbuf_addf(buf, \"%s %s %.*s\\n\", action_str, sha1_abbrev,\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 0a65d23..c2c938c 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -869,7 +869,7 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1,\n \t\tcommit = pop_most_recent_commit(&list, ONELINE_SEEN);\n \t\tif (!parse_object(commit->object.sha1))\n \t\t\tcontinue;\n-\t\tbuf = get_commit_buffer(commit);\n+\t\tbuf = get_commit_buffer(commit, NULL);\n \t\tp = strstr(buf, \"\\n\\n\");\n \t\tmatches = p && !regexec(&regex, p + 2, 0, NULL, 0);\n \t\tunuse_commit_buffer(commit, buf);\n-- \n2.0.0.729.g520999f\n"},{"id":"243804","messageId":"20140610214616.GA19107@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140610213509.GA26979@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/17] store length of commit->buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T21:46:16Z","receivedAt":"2014-06-10T21:46:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 10, 2014 at 05:35:09PM -0400, Jeff King wrote:\n\n> Here's a re-roll of the commit-slab series. It fixes the issues pointed\n> out by Eric and Christian (thanks, both).\n\nSide note: I marked this as v2, but forgot to do so in each individual\npatch (I write my cover letters first, and then issue format-patch as a\nseparate step, and I sometimes forget -v2 there). How big an\ninconvenience is this?\n\nWhen I was acting maintainer, it didn't bother me, as I picked the\npatches up via threading. But if it is annoying, I can try to teach my\nscripts to do it automatically, so I stop forgetting.\n\n-Peff\n"},{"id":"243806","messageId":"20140610214911.GB19107@sigill.intra.peff.net","threadId":"36874","inReplyTo":"xmqqbnu08n2j.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 03/15] do not create \"struct commit\" with xcalloc","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T21:49:11Z","receivedAt":"2014-06-10T21:49:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 10, 2014 at 02:35:48PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > In both blame and merge-recursive, we sometimes create a\n> > \"fake\" commit struct for convenience (e.g., to represent the\n> > HEAD state as if we would commit it). By allocating\n> > ourselves rather than using alloc_commit_node, we do not\n> > properly set the \"index\" field of the commit. This can\n> > produce subtle bugs if we then use commit-slab on the\n> > resulting commit, as we will share the \"0\" index with\n> > another commit.\n> \n> The change I see here is all good, but I wonder if it is too late to\n> start the real index from 1 and catch anything that has 0 index with\n> a BUG(\"do not hand-allocate a commit\").\n\nYeah, I had a similar thought while writing the series. I do not think\nit is too late at all. It would just waste one commit's worth of memory\nin the slab, but that is not a big deal. Other than slab_at, no callers\nshould care.\n\nIt would cost an extra conditional in each call to slab_at. That\nprobably doesn't matter, though the original goal was to make slab_at as\nfast as an indirect pointer dereference (it's not quite these days,\nthough, as we have to do a little division to find the right section of\nthe slab).\n\n-Peff\n"},{"id":"243807","messageId":"20140610215022.GC19107@sigill.intra.peff.net","threadId":"36874","inReplyTo":"xmqq7g4o8mup.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 05/15] sequencer: use logmsg_reencode in get_message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-10T21:50:22Z","receivedAt":"2014-06-10T21:50:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 10, 2014 at 02:40:30PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Note that we may be fixing a bug here. The existing code\n> > does:\n> >\n> >   if (same_encoding(to, from))\n> > \t  reencode_string(buf, to, from);\n> >\n> > That probably should have been \"!same_encoding\".\n> >\n> > Signed-off-by: Jeff King <peff@peff.net>\n> > ---\n> > I didn't actually test for the bug, so it's possible that I'm missing\n> > something clever...\n> \n> Thanks for spotting.  There is nothing clever going on.\n> \n> 0e18bcd5 (reencode_string(): introduce and use same_encoding(),\n> 2012-10-18) has this misconversion.\n\nAh, thanks, I tried digging but didn't see anything obvious (and somehow\nmy eyes glossed over the s/same_encoding/strcmp/ while reading the\nhistory). I feel much better about the change now.\n\n-Peff\n"},{"id":"243975","messageId":"xmqqmwdi6o0r.fsf@gitster.dls.corp.google.com","threadId":"36874","inReplyTo":"20140610214616.GA19107@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/17] store length of commit->buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-12T17:22:44Z","receivedAt":"2014-06-12T17:22:44Z","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, Jun 10, 2014 at 05:35:09PM -0400, Jeff King wrote:\n>\n>> Here's a re-roll of the commit-slab series. It fixes the issues pointed\n>> out by Eric and Christian (thanks, both).\n>\n> Side note: I marked this as v2, but forgot to do so in each individual\n> patch (I write my cover letters first, and then issue format-patch as a\n> separate step, and I sometimes forget -v2 there). How big an\n> inconvenience is this?\n\nNot an inconvenience to me (I do not speak for others, obviously),\nbut they look a bit confusing to me in the thread list in my MUA\n(but not too annoying).\n"},{"id":"244008","messageId":"xmqq7g4m6lka.fsf@gitster.dls.corp.google.com","threadId":"36874","inReplyTo":"20140610214251.GO19147@sigill.intra.peff.net","subject":"Re: [PATCH 15/17] commit-slab: provide a static initializer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-12T18:15:49Z","receivedAt":"2014-06-12T18:15:49Z","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> Callers currently must use init_foo_slab() at runtime before\n> accessing a slab. For global slabs, it's much nicer if we\n> can initialize them in BSS, so that each user does not have\n> to add code to check-and-initialize.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> There was no comment on this one in v1. I'd be curious if anyone has\n> comments on what I wrote in:\n>\n>   http://article.gmane.org/gmane.comp.version-control.git/251099\n\nWhy do we need an initialiser at this point (in other words, how\nhave other existing slab users coped without having one)?\n\nI think they call init_*_slab() when the slab is needed/used the\nfirst time (e.g. it is not even worth initialising indegree slab\nunless we are sorting in topo order).\n\nUnlike the author-date and indegree slabs, there are too many entry\npoints that may want access to the buffer slab (save_commit_buffer's\ndefault being on does not help us either), so it would be much less\nerror prone to always initialise a static slab like this patch does,\nI guess.\n\n\n>  commit-slab.h | 12 ++++++++++++\n>  1 file changed, 12 insertions(+)\n>\n> diff --git a/commit-slab.h b/commit-slab.h\n> index cc114b5..375c9c7 100644\n> --- a/commit-slab.h\n> +++ b/commit-slab.h\n> @@ -117,4 +117,16 @@ static int stat_ ##slabname## realloc\n>   * catch because GCC silently parses it by default.\n>   */\n>  \n> +/*\n> + * Statically initialize a commit slab named \"var\". Note that this\n> + * evaluates \"stride\" multiple times! Example:\n> + *\n> + *   struct indegree indegrees = COMMIT_SLAB_INIT(1, indegrees);\n> + *\n> + */\n> +#define COMMIT_SLAB_INIT(stride, var) { \\\n> +\tCOMMIT_SLAB_SIZE / sizeof(**((var).slab)) / (stride), \\\n> +\t(stride), 0, NULL \\\n> +}\n> +\n>  #endif /* COMMIT_SLAB_H */\n"},{"id":"244010","messageId":"xmqqzjhi56oo.fsf@gitster.dls.corp.google.com","threadId":"36874","inReplyTo":"20140610214005.GH19147@sigill.intra.peff.net","subject":"Re: [PATCH 08/17] provide a helper to free commit buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-12T18:22:31Z","receivedAt":"2014-06-12T18:22:31Z","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> This converts two lines into one at each caller. But more\n> importantly, it abstracts the concept of freeing the buffer,\n> which will make it easier to change later.\n>\n> Note that we also need to provide a \"detach\" mechanism for\n> the weird case in fsck which passes the buffer back to be\n> freed.\n\nI find that last sentence a bit of white lie ;-).\n\nThe sole caller of \"detach\" is in index-pack, and discards the\nreturn value, which is not wrong per-se because it still has the\npointer to the piece of memory it fed to parse_object_buffer(),\nknows and/or assumes that the return value must be the same as the\none it already has, and it handles the freeing of that memory\nwithout relying on the object layer at all.\n\nBut that is an even more weird special case than the white-lie\nversion.  As an API, \"detach\" that returns the buffer to be freed\nlooks much less weird than what is really going on in the current\ncaller.\n\nI however have to wonder if\n\n\tif (detach_commit_buffer(commit) != data)\n        \tdie(\"BUG\");\n\nmight want to be there.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/fsck.c       |  3 +--\n>  builtin/index-pack.c |  2 +-\n>  builtin/log.c        |  6 ++----\n>  builtin/rev-list.c   |  3 +--\n>  commit.c             | 13 +++++++++++++\n>  commit.h             | 11 +++++++++++\n>  6 files changed, 29 insertions(+), 9 deletions(-)\n>\n> diff --git a/builtin/fsck.c b/builtin/fsck.c\n> index fc150c8..8aadca1 100644\n> --- a/builtin/fsck.c\n> +++ b/builtin/fsck.c\n> @@ -310,8 +310,7 @@ static int fsck_obj(struct object *obj)\n>  \tif (obj->type == OBJ_COMMIT) {\n>  \t\tstruct commit *commit = (struct commit *) obj;\n>  \n> -\t\tfree(commit->buffer);\n> -\t\tcommit->buffer = NULL;\n> +\t\tfree_commit_buffer(commit);\n>  \n>  \t\tif (!commit->parents && show_root)\n>  \t\t\tprintf(\"root %s\\n\", sha1_to_hex(commit->object.sha1));\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index 18f57de..995df39 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -786,7 +786,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n>  \t\t\t}\n>  \t\t\tif (obj->type == OBJ_COMMIT) {\n>  \t\t\t\tstruct commit *commit = (struct commit *) obj;\n> -\t\t\t\tcommit->buffer = NULL;\n> +\t\t\t\tdetach_commit_buffer(commit);\n>  \t\t\t}\n>  \t\t\tobj->flags |= FLAG_CHECKED;\n>  \t\t}\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 39e8836..226f8f2 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -349,8 +349,7 @@ static int cmd_log_walk(struct rev_info *rev)\n>  \t\t\trev->max_count++;\n>  \t\tif (!rev->reflog_info) {\n>  \t\t\t/* we allow cycles in reflog ancestry */\n> -\t\t\tfree(commit->buffer);\n> -\t\t\tcommit->buffer = NULL;\n> +\t\t\tfree_commit_buffer(commit);\n>  \t\t}\n>  \t\tfree_commit_list(commit->parents);\n>  \t\tcommit->parents = NULL;\n> @@ -1508,8 +1507,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n>  \t\t    reopen_stdout(rev.numbered_files ? NULL : commit, NULL, &rev, quiet))\n>  \t\t\tdie(_(\"Failed to create output files\"));\n>  \t\tshown = log_tree_commit(&rev, commit);\n> -\t\tfree(commit->buffer);\n> -\t\tcommit->buffer = NULL;\n> +\t\tfree_commit_buffer(commit);\n>  \n>  \t\t/* We put one extra blank line between formatted\n>  \t\t * patches and this flag is used by log-tree code\n> diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> index 9f92905..e012ebe 100644\n> --- a/builtin/rev-list.c\n> +++ b/builtin/rev-list.c\n> @@ -173,8 +173,7 @@ static void finish_commit(struct commit *commit, void *data)\n>  \t\tfree_commit_list(commit->parents);\n>  \t\tcommit->parents = NULL;\n>  \t}\n> -\tfree(commit->buffer);\n> -\tcommit->buffer = NULL;\n> +\tfree_commit_buffer(commit);\n>  }\n>  \n>  static void finish_object(struct object *obj,\n> diff --git a/commit.c b/commit.c\n> index fbdc480..11a05c1 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -245,6 +245,19 @@ int unregister_shallow(const unsigned char *sha1)\n>  \treturn 0;\n>  }\n>  \n> +void free_commit_buffer(struct commit *commit)\n> +{\n> +\tfree(commit->buffer);\n> +\tcommit->buffer = NULL;\n> +}\n> +\n> +const void *detach_commit_buffer(struct commit *commit)\n> +{\n> +\tvoid *ret = commit->buffer;\n> +\tcommit->buffer = NULL;\n> +\treturn ret;\n> +}\n> +\n>  int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long size)\n>  {\n>  \tconst char *tail = buffer;\n> diff --git a/commit.h b/commit.h\n> index 4df48cb..d72ed43 100644\n> --- a/commit.h\n> +++ b/commit.h\n> @@ -51,6 +51,17 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s\n>  int parse_commit(struct commit *item);\n>  void parse_commit_or_die(struct commit *item);\n>  \n> +/*\n> + * Free any cached object buffer associated with the commit.\n> + */\n> +void free_commit_buffer(struct commit *);\n> +\n> +/*\n> + * Disassociate any cached object buffer from the commit, but do not free it.\n> + * The buffer (or NULL, if none) is returned.\n> + */\n> +const void *detach_commit_buffer(struct commit *);\n> +\n>  /* Find beginning and length of commit subject. */\n>  int find_commit_subject(const char *commit_buffer, const char **subject);\n"},{"id":"244029","messageId":"20140612195127.GA4468@sigill.intra.peff.net","threadId":"36874","inReplyTo":"xmqq7g4m6lka.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 15/17] commit-slab: provide a static initializer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-12T19:51:28Z","receivedAt":"2014-06-12T19:51:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 12, 2014 at 11:15:49AM -0700, Junio C Hamano wrote:\n\n> Why do we need an initialiser at this point (in other words, how\n> have other existing slab users coped without having one)?\n> \n> I think they call init_*_slab() when the slab is needed/used the\n> first time (e.g. it is not even worth initialising indegree slab\n> unless we are sorting in topo order).\n> \n> Unlike the author-date and indegree slabs, there are too many entry\n> points that may want access to the buffer slab (save_commit_buffer's\n> default being on does not help us either), so it would be much less\n> error prone to always initialise a static slab like this patch does,\n> I guess.\n\nYes, and also because the lifetime of this slab is different. For the\nindegree and author-date slabs, the slab lives for one operation.  So we\ninit the slab at the beginning of the operation, do the sort, then clear\nit at the end.\n\nThis slab does not exist for one operation. It is a global cache that\ncan be used by any code, just like the \"struct commit\" cache itself.. It\nshould always be initialized from the start, and continue until the\nprogram exits.\n\n-Peff\n"},{"id":"244031","messageId":"20140612200855.GB4468@sigill.intra.peff.net","threadId":"36874","inReplyTo":"xmqqzjhi56oo.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 08/17] provide a helper to free commit buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-12T20:08:55Z","receivedAt":"2014-06-12T20:08:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 12, 2014 at 11:22:31AM -0700, Junio C Hamano wrote:\n\n> > Note that we also need to provide a \"detach\" mechanism for\n> > the weird case in fsck which passes the buffer back to be\n> > freed.\n> \n> I find that last sentence a bit of white lie ;-).\n> \n> The sole caller of \"detach\" is in index-pack, and discards the\n> return value, which is not wrong per-se because it still has the\n> pointer to the piece of memory it fed to parse_object_buffer(),\n> knows and/or assumes that the return value must be the same as the\n> one it already has, and it handles the freeing of that memory\n> without relying on the object layer at all.\n> \n> But that is an even more weird special case than the white-lie\n> version.  As an API, \"detach\" that returns the buffer to be freed\n> looks much less weird than what is really going on in the current\n> caller.\n> \n> I however have to wonder if\n> \n> \tif (detach_commit_buffer(commit) != data)\n>         \tdie(\"BUG\");\n> \n> might want to be there.\n\nYeah, it is a tricky site. It knows that parse_object_buffer may attach\nthe buffer we hand it to \"commit->buffer\", even though we would prefer\nto keep the buffer ourselves. So the existing code really just wants to\nsay \"erase that attachment\". And of course I started with:\n\n  void detach_commit_buffer(struct commit *commit)\n  {\n\tcommit->buffer = NULL;\n  }\n\nBoth before and after it's a bit of a layering violation; we know how\nthe internals of buffer attaching work, and that we can detach to get\nour original.\n\nI then expanded it to the strbuf-inspired detach you see, even though\nthere are no callers who actually care about the return value. That\nmakes more sense to me as a general API. I don't think it actually makes\nthe layering violation worse (we are still making the exact same\nassumption), but I think it _seems_ worse, because the API now seems\nmore fully formed. And note that we make the exact same assumptions\nabotu \"struct tree\" a few lines above.\n\nThe safety check you mention above makes sense to me. There are two\nthings I'd _rather_ do, but they end up more complicated:\n\n  1. It would be nice to ask parse_object_buffer not to attach the\n     buffer in the first place; then we could get rid of the detach\n     function entirely. But that attachment is necessary for all of the\n     fsck sub-functions we call. Factoring those to take a separate\n     buffer would be rather disruptive.\n\n  2. Instead of confirming that detach returns the same buffer, just\n     assume our buffer was eaten (as promised by set_commit_buffer),\n     and continue on with whatever detach_commit_buffer returns.\n     But it is not _our_ buffer in the first place. It is passed in by\n     the caller, so this replacement would have to bubble back up to the\n     original allocator.\n\nSo just putting in the safety check is probably the least-disruptive\nthing. It doesn't automatically adapt to a change in the underlying\ncommit_buffer code, but it would at least notice it.\n\n-Peff\n"},{"id":"244039","messageId":"20140612220537.GA14445@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140612200855.GB4468@sigill.intra.peff.net","subject":"Re: [PATCH 08/17] provide a helper to free commit buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-12T22:05:37Z","receivedAt":"2014-06-12T22:05:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 12, 2014 at 04:08:55PM -0400, Jeff King wrote:\n\n> So just putting in the safety check is probably the least-disruptive\n> thing. It doesn't automatically adapt to a change in the underlying\n> commit_buffer code, but it would at least notice it.\n\nHere is that commit with the check you suggested, and a better\nexplanation in the commit message of what is going on.\n\nI don't know if it's easier for you to just replace patch 8 with this\n(there will be a minor textual conflict in the final commit then), or if\nyou want me to re-post the whole 17-patch series.\n\nArguably this could be broken into two commits (one to add the simple\n\"free\" and one for this more complicated \"detach\" case), but that makes\nit little more complicated for you to apply. We need some way of\nemailing patches coupled with rebase instructions. :)\n\n-- >8 --\nSubject: provide a helper to free commit buffer\n\nThis converts two lines into one at each caller. But more\nimportantly, it abstracts the concept of freeing the buffer,\nwhich will make it easier to change later.\n\nNote that we also need to provide a \"detach\" mechanism for a\ntricky case in index-pack. We are passed a buffer for the\nobject generated by processing the incoming pack.. If we are\nnot using --strict, we just calculate the sha1 on that\nbuffer and return, leaving the caller to free it.  But if we\nare using --strict, we actually attach that buffer to an\nobject, pass the object to the fsck functions, and then\ndetach the buffer from the object again (so that the caller\ncan free it as usual).  In this case, we don't want to free\nthe buffer ourselves, but just make sure it is no longer\nassociated with the commit.\n\nNote that we are making the assumption here that the\nattach/detach process does not impact the buffer at all\n(e.g., it is never reallocated or modified). That holds true\nnow, and we have no plans to change that. However, as we\nabstract the commit_buffer code, this dependency becomes\nless obvious. So when we detach, let's also make sure that\nwe get back the same buffer that we gave to the\ncommit_buffer code.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/fsck.c       |  3 +--\n builtin/index-pack.c |  3 ++-\n builtin/log.c        |  6 ++----\n builtin/rev-list.c   |  3 +--\n commit.c             | 13 +++++++++++++\n commit.h             | 11 +++++++++++\n 6 files changed, 30 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex fc150c8..8aadca1 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -310,8 +310,7 @@ static int fsck_obj(struct object *obj)\n \tif (obj->type == OBJ_COMMIT) {\n \t\tstruct commit *commit = (struct commit *) obj;\n \n-\t\tfree(commit->buffer);\n-\t\tcommit->buffer = NULL;\n+\t\tfree_commit_buffer(commit);\n \n \t\tif (!commit->parents && show_root)\n \t\t\tprintf(\"root %s\\n\", sha1_to_hex(commit->object.sha1));\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 18f57de..3f88b12 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -786,7 +786,8 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\t}\n \t\t\tif (obj->type == OBJ_COMMIT) {\n \t\t\t\tstruct commit *commit = (struct commit *) obj;\n-\t\t\t\tcommit->buffer = NULL;\n+\t\t\t\tif (detach_commit_buffer(commit) != data)\n+\t\t\t\t\tdie(\"BUG: parse_object_buffer transmogrified our buffer\");\n \t\t\t}\n \t\t\tobj->flags |= FLAG_CHECKED;\n \t\t}\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 39e8836..226f8f2 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -349,8 +349,7 @@ static int cmd_log_walk(struct rev_info *rev)\n \t\t\trev->max_count++;\n \t\tif (!rev->reflog_info) {\n \t\t\t/* we allow cycles in reflog ancestry */\n-\t\t\tfree(commit->buffer);\n-\t\t\tcommit->buffer = NULL;\n+\t\t\tfree_commit_buffer(commit);\n \t\t}\n \t\tfree_commit_list(commit->parents);\n \t\tcommit->parents = NULL;\n@@ -1508,8 +1507,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t    reopen_stdout(rev.numbered_files ? NULL : commit, NULL, &rev, quiet))\n \t\t\tdie(_(\"Failed to create output files\"));\n \t\tshown = log_tree_commit(&rev, commit);\n-\t\tfree(commit->buffer);\n-\t\tcommit->buffer = NULL;\n+\t\tfree_commit_buffer(commit);\n \n \t\t/* We put one extra blank line between formatted\n \t\t * patches and this flag is used by log-tree code\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 9f92905..e012ebe 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -173,8 +173,7 @@ static void finish_commit(struct commit *commit, void *data)\n \t\tfree_commit_list(commit->parents);\n \t\tcommit->parents = NULL;\n \t}\n-\tfree(commit->buffer);\n-\tcommit->buffer = NULL;\n+\tfree_commit_buffer(commit);\n }\n \n static void finish_object(struct object *obj,\ndiff --git a/commit.c b/commit.c\nindex fbdc480..11a05c1 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -245,6 +245,19 @@ int unregister_shallow(const unsigned char *sha1)\n \treturn 0;\n }\n \n+void free_commit_buffer(struct commit *commit)\n+{\n+\tfree(commit->buffer);\n+\tcommit->buffer = NULL;\n+}\n+\n+const void *detach_commit_buffer(struct commit *commit)\n+{\n+\tvoid *ret = commit->buffer;\n+\tcommit->buffer = NULL;\n+\treturn ret;\n+}\n+\n int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long size)\n {\n \tconst char *tail = buffer;\ndiff --git a/commit.h b/commit.h\nindex 4df48cb..d72ed43 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -51,6 +51,17 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s\n int parse_commit(struct commit *item);\n void parse_commit_or_die(struct commit *item);\n \n+/*\n+ * Free any cached object buffer associated with the commit.\n+ */\n+void free_commit_buffer(struct commit *);\n+\n+/*\n+ * Disassociate any cached object buffer from the commit, but do not free it.\n+ * The buffer (or NULL, if none) is returned.\n+ */\n+const void *detach_commit_buffer(struct commit *);\n+\n /* Find beginning and length of commit subject. */\n int find_commit_subject(const char *commit_buffer, const char **subject);\n \n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244056","messageId":"20140613063211.GA7552@sigill.intra.peff.net","threadId":"36874","inReplyTo":"20140610213509.GA26979@sigill.intra.peff.net","subject":"[PATCH v2] reuse cached commit buffer when parsing signatures","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-13T06:32:11Z","receivedAt":"2014-06-13T06:32:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 10, 2014 at 05:35:09PM -0400, Jeff King wrote:\n\n> Here's a re-roll of the commit-slab series.\n\nAnd here is a re-roll of the --show-signature speedup on top (i.e., the\npoint of the series in the first place), adjusted to use\nget_commit_buffer.\n\n-- >8 --\nSubject: reuse cached commit buffer when parsing signatures\n\nWhen we call show_signature or show_mergetag, we read the\ncommit object fresh via read_sha1_file and reparse its\nheaders. However, in most cases we already have the object\ndata available, attached to the \"struct commit\". This is\npartially laziness in dealing with the memory allocation\nissues, but partially defensive programming, in that we\nwould always want to verify a clean version of the buffer\n(not one that might have been munged by other users of the\ncommit).\n\nHowever, we do not currently ever munge the commit buffer,\nand not using the already-available buffer carries a fairly\nbig performance penalty when we are looking at a large\nnumber of commits. Here are timings on linux.git:\n\n  [baseline, no signatures]\n  $ time git log >/dev/null\n  real    0m4.902s\n  user    0m4.784s\n  sys     0m0.120s\n\n  [before]\n  $ time git log --show-signature >/dev/null\n  real    0m14.735s\n  user    0m9.964s\n  sys     0m0.944s\n\n  [after]\n  $ time git log --show-signature >/dev/null\n  real    0m9.981s\n  user    0m5.260s\n  sys     0m0.936s\n\nNote that our user CPU time drops almost in half, close to\nthe non-signature case, but we do still spend more\nwall-clock and system time, presumably from dealing with\ngpg.\n\nAn alternative to this is to note that most commits do not\nhave signatures (less than 1% in this repo), yet we pay the\nre-parsing cost for every commit just to find out if it has\na mergetag or signature. If we checked that when parsing the\ncommit initially, we could avoid re-examining most commits\nlater on. Even if we did pursue that direction, however,\nthis would still speed up the cases where we _do_ have\nsignatures. So it's probably worth doing either way.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n commit.c   | 27 ++++++++++-----------------\n commit.h   |  2 +-\n log-tree.c |  2 +-\n 3 files changed, 12 insertions(+), 19 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex a036e18..ebd7ad8 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1138,17 +1138,14 @@ static int do_sign_commit(struct strbuf *buf, const char *keyid)\n \treturn 0;\n }\n \n-int parse_signed_commit(const unsigned char *sha1,\n+int parse_signed_commit(const struct commit *commit,\n \t\t\tstruct strbuf *payload, struct strbuf *signature)\n {\n+\n \tunsigned long size;\n-\tenum object_type type;\n-\tchar *buffer = read_sha1_file(sha1, &type, &size);\n+\tconst char *buffer = get_commit_buffer(commit, &size);\n \tint in_signature, saw_signature = -1;\n-\tchar *line, *tail;\n-\n-\tif (!buffer || type != OBJ_COMMIT)\n-\t\tgoto cleanup;\n+\tconst char *line, *tail;\n \n \tline = buffer;\n \ttail = buffer + size;\n@@ -1156,7 +1153,7 @@ int parse_signed_commit(const unsigned char *sha1,\n \tsaw_signature = 0;\n \twhile (line < tail) {\n \t\tconst char *sig = NULL;\n-\t\tchar *next = memchr(line, '\\n', tail - line);\n+\t\tconst char *next = memchr(line, '\\n', tail - line);\n \n \t\tnext = next ? next + 1 : tail;\n \t\tif (in_signature && line[0] == ' ')\n@@ -1177,8 +1174,7 @@ int parse_signed_commit(const unsigned char *sha1,\n \t\t}\n \t\tline = next;\n \t}\n- cleanup:\n-\tfree(buffer);\n+\tunuse_commit_buffer(commit, buffer);\n \treturn saw_signature;\n }\n \n@@ -1269,8 +1265,7 @@ void check_commit_signature(const struct commit* commit, struct signature_check\n \n \tsigc->result = 'N';\n \n-\tif (parse_signed_commit(commit->object.sha1,\n-\t\t\t\t&payload, &signature) <= 0)\n+\tif (parse_signed_commit(commit, &payload, &signature) <= 0)\n \t\tgoto out;\n \tstatus = verify_signed_buffer(payload.buf, payload.len,\n \t\t\t\t      signature.buf, signature.len,\n@@ -1315,11 +1310,9 @@ struct commit_extra_header *read_commit_extra_headers(struct commit *commit,\n {\n \tstruct commit_extra_header *extra = NULL;\n \tunsigned long size;\n-\tenum object_type type;\n-\tchar *buffer = read_sha1_file(commit->object.sha1, &type, &size);\n-\tif (buffer && type == OBJ_COMMIT)\n-\t\textra = read_commit_extra_header_lines(buffer, size, exclude);\n-\tfree(buffer);\n+\tconst char *buffer = get_commit_buffer(commit, &size);\n+\textra = read_commit_extra_header_lines(buffer, size, exclude);\n+\tunuse_commit_buffer(commit, buffer);\n \treturn extra;\n }\n \ndiff --git a/commit.h b/commit.h\nindex 61559a9..2e1492a 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -325,7 +325,7 @@ struct merge_remote_desc {\n  */\n struct commit *get_merge_parent(const char *name);\n \n-extern int parse_signed_commit(const unsigned char *sha1,\n+extern int parse_signed_commit(const struct commit *commit,\n \t\t\t       struct strbuf *message, struct strbuf *signature);\n extern void print_commit_list(struct commit_list *list,\n \t\t\t      const char *format_cur,\ndiff --git a/log-tree.c b/log-tree.c\nindex 4447021..10e6844 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -376,7 +376,7 @@ static void show_signature(struct rev_info *opt, struct commit *commit)\n \tstruct strbuf gpg_output = STRBUF_INIT;\n \tint status;\n \n-\tif (parse_signed_commit(commit->object.sha1, &payload, &signature) <= 0)\n+\tif (parse_signed_commit(commit, &payload, &signature) <= 0)\n \t\tgoto out;\n \n \tstatus = verify_signed_buffer(payload.buf, payload.len,\n-- \n2.0.0.566.gfe3e6b2\n"}]}