{"thread":{"id":"30579","subject":"[PATCH 0/7] Fix some sequencer leaks","startedAt":"2012-05-21T14:56:02Z","lastAt":"2012-05-22T20:40:15Z","messageCount":23,"participants":["Christian Couder","Jonathan Nieder","Ramkumar Ramachandra"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"191807","messageId":"20120521143309.1911.94302.chriscool@tuxfamily.org","threadId":"30579","inReplyTo":null,"subject":"[PATCH 0/7] Fix some sequencer leaks","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2012-05-21T14:56:02Z","receivedAt":"2012-05-21T14:56:02Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Here is another iteration of a series to fix some leaks in the cherry-pick,\nrevert and sequencer code.\n\nThe previous iteration only had patch 1 and 2. They haven't changed.\nI just added the last 5 patches.\n\nMaybe the last 2 patches are a bit overkill. But with this series, the\nvalgrind output with --leak-check=full is much cleaner.\n\nThere most important leaks are still there though. One of them is\nrelated to ce_entries, like this: \n\n==2015== 522 bytes in 6 blocks are definitely lost in loss record 51 of 54\n==2015==    at 0x4C29DB4: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)\n==2015==    by 0x53113B: xcalloc (wrapper.c:119)\n==2015==    by 0x52BBE9: create_ce_entry (unpack-trees.c:559)\n==2015==    by 0x52BDC6: unpack_nondirectories (unpack-trees.c:609)\n==2015==    by 0x52C45B: unpack_callback (unpack-trees.c:799)\n==2015==    by 0x529B9B: traverse_trees (tree-walk.c:407)\n==2015==    by 0x52CE53: unpack_trees (unpack-trees.c:1091)\n==2015==    by 0x4D1B89: git_merge_trees (merge-recursive.c:241)\n==2015==    by 0x4D673A: merge_trees (merge-recursive.c:1819)\n==2015==    by 0x509521: do_recursive_merge (sequencer.c:225)\n==2015==    by 0x509F5B: do_pick_commit (sequencer.c:457)\n==2015==    by 0x50B3B4: pick_commits (sequencer.c:878)\n\nThe other one is related to the index:\n\n==2015== 574 (400 direct, 174 indirect) bytes in 2 blocks are definitely lost in loss record 52 of 54\n==2015==    at 0x4C29DB4: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)\n==2015==    by 0x53113B: xcalloc (wrapper.c:119)\n==2015==    by 0x4EF8CF: read_index_from (read-cache.c:1441)\n==2015==    by 0x4EF316: read_index (read-cache.c:1282)\n==2015==    by 0x50940B: do_recursive_merge (sequencer.c:211)\n==2015==    by 0x509F5B: do_pick_commit (sequencer.c:457)\n==2015==    by 0x50B3B4: pick_commits (sequencer.c:878)\n==2015==    by 0x50B7C6: sequencer_pick_revisions (sequencer.c:994)\n==2015==    by 0x47A6D5: cmd_cherry_pick (revert.c:237)\n==2015==    by 0x404FEE: run_builtin (git.c:308)\n==2015==    by 0x405181: handle_internal_command (git.c:468)\n==2015==    by 0x40529B: run_argv (git.c:514)\n\nChristian Couder (7):\n  sequencer: fix leaked todo_list memory\n  sequencer: release a strbuf used in save_head()\n  merge-recursive: free some string lists\n  revert: free opts.revs to do a bit of cleanup\n  revert: free revs->cmdline.rev\n  unpack-trees: record which unpack error messages should be freed\n  Properly free unpack trees error messages\n\n builtin/checkout.c |  1 +\n builtin/merge.c    |  6 ++++--\n builtin/revert.c   |  6 ++++++\n merge-recursive.c  |  5 ++++-\n sequencer.c        | 16 +++++++++++++---\n unpack-trees.c     | 50 +++++++++++++++++++++++++++++++++++---------------\n unpack-trees.h     |  3 +++\n 7 files changed, 66 insertions(+), 21 deletions(-)\n\n-- \n1.7.10.2.555.g6528037\n"},{"id":"191810","messageId":"20120521145610.1911.87598.chriscool@tuxfamily.org","threadId":"30579","inReplyTo":"20120521143309.1911.94302.chriscool@tuxfamily.org","subject":"[PATCH 1/7] sequencer: fix leaked todo_list memory","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2012-05-21T14:56:03Z","receivedAt":"2012-05-21T14:56:03Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n sequencer.c | 15 ++++++++++++---\n 1 file changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 3c384b9..762c527 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -900,6 +900,7 @@ static int continue_single_pick(void)\n static int sequencer_continue(struct replay_opts *opts)\n {\n \tstruct commit_list *todo_list = NULL;\n+\tint res;\n \n \tif (!file_exists(git_path(SEQ_TODO_FILE)))\n \t\treturn continue_single_pick();\n@@ -915,8 +916,11 @@ static int sequencer_continue(struct replay_opts *opts)\n \t}\n \tif (index_differs_from(\"HEAD\", 0))\n \t\treturn error_dirty_index(opts);\n-\ttodo_list = todo_list->next;\n-\treturn pick_commits(todo_list, opts);\n+\tres = pick_commits(todo_list->next, opts);\n+\n+\tfree_commit_list(todo_list);\n+\n+\treturn res;\n }\n \n static int single_pick(struct commit *cmit, struct replay_opts *opts)\n@@ -929,6 +933,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n {\n \tstruct commit_list *todo_list = NULL;\n \tunsigned char sha1[20];\n+\tint res;\n \n \tif (opts->subcommand == REPLAY_NONE)\n \t\tassert(opts->revs);\n@@ -985,5 +990,9 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \t}\n \tsave_head(sha1_to_hex(sha1));\n \tsave_opts(opts);\n-\treturn pick_commits(todo_list, opts);\n+\tres = pick_commits(todo_list, opts);\n+\n+\tfree_commit_list(todo_list);\n+\n+\treturn res;\n }\n-- \n1.7.10.2.555.g6528037\n"},{"id":"191809","messageId":"20120521145610.1911.61154.chriscool@tuxfamily.org","threadId":"30579","inReplyTo":"20120521143309.1911.94302.chriscool@tuxfamily.org","subject":"[PATCH 2/7] sequencer: release a strbuf used in save_head()","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2012-05-21T14:56:04Z","receivedAt":"2012-05-21T14:56:04Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n sequencer.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 762c527..ad1bbea 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -741,6 +741,7 @@ static void save_head(const char *head)\n \t\tdie_errno(_(\"Could not write to %s\"), head_file);\n \tif (commit_lock_file(&head_lock) < 0)\n \t\tdie(_(\"Error wrapping up %s.\"), head_file);\n+\tstrbuf_release(&buf);\n }\n \n static int reset_for_rollback(const unsigned char *sha1)\n-- \n1.7.10.2.555.g6528037\n"},{"id":"191808","messageId":"20120521145610.1911.82257.chriscool@tuxfamily.org","threadId":"30579","inReplyTo":"20120521143309.1911.94302.chriscool@tuxfamily.org","subject":"[PATCH 3/7] merge-recursive: free some string lists","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2012-05-21T14:56:05Z","receivedAt":"2012-05-21T14:56:05Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n merge-recursive.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 680937c..9c3570a 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1856,7 +1856,9 @@ int merge_trees(struct merge_options *o,\n \t\tstring_list_clear(re_merge, 0);\n \t\tstring_list_clear(re_head, 0);\n \t\tstring_list_clear(entries, 1);\n-\n+\t\tfree(re_merge);\n+\t\tfree(re_head);\n+\t\tfree(entries);\n \t}\n \telse\n \t\tclean = 1;\n-- \n1.7.10.2.555.g6528037\n"},{"id":"191812","messageId":"20120521145610.1911.57879.chriscool@tuxfamily.org","threadId":"30579","inReplyTo":"20120521143309.1911.94302.chriscool@tuxfamily.org","subject":"[PATCH 4/7] revert: free opts.revs to do a bit of cleanup","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2012-05-21T14:56:06Z","receivedAt":"2012-05-21T14:56:06Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n builtin/revert.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 82d1bf8..5f82a84 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -217,6 +217,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n \tgit_config(git_default_config, NULL);\n \tparse_args(argc, argv, &opts);\n \tres = sequencer_pick_revisions(&opts);\n+\tfree(opts.revs);\n \tif (res < 0)\n \t\tdie(_(\"revert failed\"));\n \treturn res;\n@@ -232,6 +233,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n \tgit_config(git_default_config, NULL);\n \tparse_args(argc, argv, &opts);\n \tres = sequencer_pick_revisions(&opts);\n+\tfree(opts.revs);\n \tif (res < 0)\n \t\tdie(_(\"cherry-pick failed\"));\n \treturn res;\n-- \n1.7.10.2.555.g6528037\n"},{"id":"191813","messageId":"20120521145610.1911.46356.chriscool@tuxfamily.org","threadId":"30579","inReplyTo":"20120521143309.1911.94302.chriscool@tuxfamily.org","subject":"[PATCH 5/7] revert: free revs->cmdline.rev","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2012-05-21T14:56:07Z","receivedAt":"2012-05-21T14:56:07Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"add_rev_cmdline() in revision.c is (re)allocating an array of\nstruct rev_cmdline_entry. This patch releases it.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n builtin/revert.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 5f82a84..1d32ed5 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -217,6 +217,8 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n \tgit_config(git_default_config, NULL);\n \tparse_args(argc, argv, &opts);\n \tres = sequencer_pick_revisions(&opts);\n+\tif (opts.revs)\n+\t\tfree(opts.revs->cmdline.rev);\n \tfree(opts.revs);\n \tif (res < 0)\n \t\tdie(_(\"revert failed\"));\n@@ -233,6 +235,8 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n \tgit_config(git_default_config, NULL);\n \tparse_args(argc, argv, &opts);\n \tres = sequencer_pick_revisions(&opts);\n+\tif (opts.revs)\n+\t\tfree(opts.revs->cmdline.rev);\n \tfree(opts.revs);\n \tif (res < 0)\n \t\tdie(_(\"cherry-pick failed\"));\n-- \n1.7.10.2.555.g6528037\n"},{"id":"191811","messageId":"20120521145610.1911.60207.chriscool@tuxfamily.org","threadId":"30579","inReplyTo":"20120521143309.1911.94302.chriscool@tuxfamily.org","subject":"[PATCH 6/7] unpack-trees: record which unpack error messages should be freed","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2012-05-21T14:56:08Z","receivedAt":"2012-05-21T14:56:08Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"\"struct unpack_trees_options\" has a \"const char *msgs[]\" field\nthat is setup with string values in setup_unpack_trees_porcelain().\nUnfortunately this field is setup with strings that are sometimes\nallocated on the stack, sometimes not.\n\nThe goal of this patch is to add some infrastructure to record which\nof the string have been allocated on the stack so they can be\nproperly freed when not needed anymore.\n\nWe use a bitfield in struct unpack_trees_options to record that.\nWe add a function to both setup the error messages and record\nwether they should be freed. And we add another function to\nproperly free all the messages that should be freed.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n unpack-trees.c | 50 +++++++++++++++++++++++++++++++++++---------------\n unpack-trees.h |  3 +++\n 2 files changed, 38 insertions(+), 15 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex bcee99c..3537fea 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -50,11 +50,29 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {\n \t  ? ((o)->msgs[(type)])      \\\n \t  : (unpack_plumbing_errors[(type)]) )\n \n+static void setup_unpack_error_msg(struct unpack_trees_options *opts,\n+\t\t\t\t   enum unpack_trees_error_types err_type,\n+\t\t\t\t   char *message, int to_free)\n+{\n+\t/* Check that we can record which messages should be freed */\n+\tassert(NB_UNPACK_TREES_ERROR_TYPES <= 8 * sizeof(opts->msgs_to_be_freed));\n+\n+\topts->msgs[err_type] = message;\n+\topts->msgs_to_be_freed |= (to_free ? 1u : 0) << err_type;\n+}\n+\n+void free_unpack_error_msgs(struct unpack_trees_options *opts)\n+{\n+\tint i;\n+\tfor (i = 0; i < NB_UNPACK_TREES_ERROR_TYPES; i++)\n+\t\tif (opts->msgs_to_be_freed & 1u << i)\n+\t\t\tfree((char *)opts->msgs[i]);\n+}\n+\n void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n \t\t\t\t  const char *cmd)\n {\n \tint i;\n-\tconst char **msgs = opts->msgs;\n \tconst char *msg;\n \tchar *tmp;\n \tconst char *cmd2 = strcmp(cmd, \"checkout\") ? cmd : \"switch branches\";\n@@ -65,11 +83,11 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n \t\tmsg = \"Your local changes to the following files would be overwritten by %s:\\n%%s\";\n \ttmp = xmalloc(strlen(msg) + strlen(cmd) + strlen(cmd2) - 2);\n \tsprintf(tmp, msg, cmd, cmd2);\n-\tmsgs[ERROR_WOULD_OVERWRITE] = tmp;\n-\tmsgs[ERROR_NOT_UPTODATE_FILE] = tmp;\n+\tsetup_unpack_error_msg(opts, ERROR_WOULD_OVERWRITE, tmp, 1);\n+\tsetup_unpack_error_msg(opts, ERROR_NOT_UPTODATE_FILE, tmp, 0);\n \n-\tmsgs[ERROR_NOT_UPTODATE_DIR] =\n-\t\t\"Updating the following directories would lose untracked files in it:\\n%s\";\n+\ttmp = \"Updating the following directories would lose untracked files in it:\\n%s\";\n+\tsetup_unpack_error_msg(opts, ERROR_NOT_UPTODATE_DIR, tmp, 0);\n \n \tif (advice_commit_before_merge)\n \t\tmsg = \"The following untracked working tree files would be %s by %s:\\n%%s\"\n@@ -78,23 +96,25 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n \t\tmsg = \"The following untracked working tree files would be %s by %s:\\n%%s\";\n \ttmp = xmalloc(strlen(msg) + strlen(cmd) + strlen(\"removed\") + strlen(cmd2) - 4);\n \tsprintf(tmp, msg, \"removed\", cmd, cmd2);\n-\tmsgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] = tmp;\n+\tsetup_unpack_error_msg(opts, ERROR_WOULD_LOSE_UNTRACKED_REMOVED, tmp, 1);\n \ttmp = xmalloc(strlen(msg) + strlen(cmd) + strlen(\"overwritten\") + strlen(cmd2) - 4);\n \tsprintf(tmp, msg, \"overwritten\", cmd, cmd2);\n-\tmsgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = tmp;\n+\tsetup_unpack_error_msg(opts, ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, tmp, 1);\n \n \t/*\n \t * Special case: ERROR_BIND_OVERLAP refers to a pair of paths, we\n \t * cannot easily display it as a list.\n \t */\n-\tmsgs[ERROR_BIND_OVERLAP] = \"Entry '%s' overlaps with '%s'.  Cannot bind.\";\n-\n-\tmsgs[ERROR_SPARSE_NOT_UPTODATE_FILE] =\n-\t\t\"Cannot update sparse checkout: the following entries are not up-to-date:\\n%s\";\n-\tmsgs[ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN] =\n-\t\t\"The following Working tree files would be overwritten by sparse checkout update:\\n%s\";\n-\tmsgs[ERROR_WOULD_LOSE_ORPHANED_REMOVED] =\n-\t\t\"The following Working tree files would be removed by sparse checkout update:\\n%s\";\n+\ttmp = \"Entry '%s' overlaps with '%s'.  Cannot bind.\";\n+\tsetup_unpack_error_msg(opts, ERROR_BIND_OVERLAP, tmp, 0);\n+\n+\ttmp = \"Cannot update sparse checkout: the following entries are not up-to-date:\\n%s\";\n+\tsetup_unpack_error_msg(opts, ERROR_SPARSE_NOT_UPTODATE_FILE, tmp, 0);\n+\n+\ttmp = \"The following Working tree files would be overwritten by sparse checkout update:\\n%s\";\n+\tsetup_unpack_error_msg(opts, ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN, tmp, 0);\n+\ttmp = \"The following Working tree files would be removed by sparse checkout update:\\n%s\";\n+\tsetup_unpack_error_msg(opts, ERROR_WOULD_LOSE_ORPHANED_REMOVED, tmp, 0);\n \n \topts->show_all_errors = 1;\n \t/* rejected paths may not have a static buffer */\ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex 5e432f5..9b9d6ab 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -30,6 +30,7 @@ enum unpack_trees_error_types {\n  */\n void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n \t\t\t\t  const char *cmd);\n+void free_unpack_error_msgs(struct unpack_trees_options *opts);\n \n struct unpack_trees_options {\n \tunsigned int reset,\n@@ -55,6 +56,8 @@ struct unpack_trees_options {\n \tstruct pathspec *pathspec;\n \tmerge_fn_t fn;\n \tconst char *msgs[NB_UNPACK_TREES_ERROR_TYPES];\n+\t/* Bit array to know which msgs must be freed */\n+\tunsigned int msgs_to_be_freed;\n \t/*\n \t * Store error messages in an array, each case\n \t * corresponding to a error message type\n-- \n1.7.10.2.555.g6528037\n"},{"id":"191814","messageId":"20120521145610.1911.31963.chriscool@tuxfamily.org","threadId":"30579","inReplyTo":"20120521143309.1911.94302.chriscool@tuxfamily.org","subject":"[PATCH 7/7] Properly free unpack trees error messages","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2012-05-21T14:56:09Z","receivedAt":"2012-05-21T14:56:09Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n builtin/checkout.c | 1 +\n builtin/merge.c    | 6 ++++--\n merge-recursive.c  | 1 +\n 3 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 3ddda34..df8fef0 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -435,6 +435,7 @@ static int merge_working_tree(struct checkout_opts *opts,\n \t\tinit_tree_desc(&trees[1], tree->buffer, tree->size);\n \n \t\tret = unpack_trees(2, trees, &topts);\n+\t\tfree_unpack_error_msgs(&topts);\n \t\tif (ret == -1) {\n \t\t\t/*\n \t\t\t * Unpack couldn't do a trivial merge; either\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 470fc57..b96a9ae 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -768,7 +768,7 @@ int checkout_fast_forward(const unsigned char *head, const unsigned char *remote\n \tstruct tree *trees[MAX_UNPACK_TREES];\n \tstruct unpack_trees_options opts;\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n-\tint i, fd, nr_trees = 0;\n+\tint i, fd, res, nr_trees = 0;\n \tstruct dir_struct dir;\n \tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n \n@@ -805,7 +805,9 @@ int checkout_fast_forward(const unsigned char *head, const unsigned char *remote\n \t\tparse_tree(trees[i]);\n \t\tinit_tree_desc(t+i, trees[i]->buffer, trees[i]->size);\n \t}\n-\tif (unpack_trees(nr_trees, t, &opts))\n+\tres = unpack_trees(nr_trees, t, &opts);\n+\tfree_unpack_error_msgs(&opts);\n+\tif (res)\n \t\treturn -1;\n \tif (write_cache(fd, active_cache, active_nr) ||\n \t\tcommit_locked_index(lock_file))\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 9c3570a..e196c45 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -239,6 +239,7 @@ static int git_merge_trees(int index_only,\n \tinit_tree_desc_from_tree(t+2, merge);\n \n \trc = unpack_trees(3, t, &opts);\n+\tfree_unpack_error_msgs(&opts);\n \tcache_tree_free(&active_cache_tree);\n \treturn rc;\n }\n-- \n1.7.10.2.555.g6528037\n"},{"id":"191827","messageId":"20120521203941.GB28331@burratino","threadId":"30579","inReplyTo":"20120521145610.1911.46356.chriscool@tuxfamily.org","subject":"Re: [PATCH 5/7] revert: free revs->cmdline.rev","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-05-21T20:39:41Z","receivedAt":"2012-05-21T20:39:41Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nChristian Couder wrote:\n\n> add_rev_cmdline() in revision.c is (re)allocating an array of\n> struct rev_cmdline_entry. This patch releases it.\n[...]\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -217,6 +217,8 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n>  \tgit_config(git_default_config, NULL);\n>  \tparse_args(argc, argv, &opts);\n>  \tres = sequencer_pick_revisions(&opts);\n> +\tif (opts.revs)\n> +\t\tfree(opts.revs->cmdline.rev);\n>  \tfree(opts.revs);\n>  \tif (res < 0)\n>  \t\tdie(_(\"revert failed\"));\n\nQuick thoughts:\n\nThis feels like a layering violation.  Avoidable?  Maybe revision.c\ncould gain a helper to allow this to be written like so:\n\n\tfree_rev_info(opts.revs);\n\tfree(opts.revs);\n\nSince this is a one-time allocation it is probably worth mentioning in\nthe log message that this is a futureproofing/valgrind-cleanliness\nmeasure and is not actually fixing a leak.\n\nMicronit: it would feel slightly more comfortable if the free() were\nafter the die(), even though the die() should probably be changed to\nexit().  That way someone wanting to add code after the die() that\ncontinues to assume opts.revs is valid would be able to.\n\nOf course I'd imagine the largest leak in cherry-pick is the\ndeliberately constantly growing object hash table.  It would be very\ninteresting to fix that --- do you know how libgit2 handles it?\n\nThanks much and hope that helps,\nJonathan\n"},{"id":"191828","messageId":"20120521204350.GC28331@burratino","threadId":"30579","inReplyTo":"20120521145610.1911.60207.chriscool@tuxfamily.org","subject":"Re: [PATCH 6/7] unpack-trees: record which unpack error messages should be freed","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-05-21T20:43:50Z","receivedAt":"2012-05-21T20:43:50Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Christian Couder wrote:\n\n> \"struct unpack_trees_options\" has a \"const char *msgs[]\" field\n> that is setup with string values in setup_unpack_trees_porcelain().\n\nHmm.  Incidentally, should callers (e.g., the 100 single-picks\ninvolved in a large multi-pick) be reusing these strings instead of\nallocating them again and again?\n"},{"id":"191829","messageId":"20120521205701.GD28331@burratino","threadId":"30579","inReplyTo":"20120521145610.1911.87598.chriscool@tuxfamily.org","subject":"Re: [PATCH 1/7] sequencer: fix leaked todo_list memory","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-05-21T20:57:01Z","receivedAt":"2012-05-21T20:57:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Christian Couder wrote:\n\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -900,6 +900,7 @@ static int continue_single_pick(void)\n>  static int sequencer_continue(struct replay_opts *opts)\n>  {\n>  \tstruct commit_list *todo_list = NULL;\n> +\tint res;\n>  \n>  \tif (!file_exists(git_path(SEQ_TODO_FILE)))\n>  \t\treturn continue_single_pick();\n> @@ -915,8 +916,11 @@ static int sequencer_continue(struct replay_opts *opts)\n>  \t}\n>  \tif (index_differs_from(\"HEAD\", 0))\n>  \t\treturn error_dirty_index(opts);\n> -\ttodo_list = todo_list->next;\n> -\treturn pick_commits(todo_list, opts);\n> +\tres = pick_commits(todo_list->next, opts);\n> +\n> +\tfree_commit_list(todo_list);\n> +\treturn res;\n[...]\n> @@ -929,6 +933,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n[same change there]\n\nMakes sense.  This is a one-time allocation, but it is in a somewhat\npublic function and freeing saves human readers and automatic\ndebugging tools like valgrind some distraction.\n\nShouldn't the error paths do this too?\n\n\twalk_revs_populate_todo(&todo_list, opts);\n\tif (create_seq_dir())\n\t\tgoto fail;\n\tif (get_sha1(\"HEAD\", sha1)) {\n\t\terror(opts->action == REPLAY_REVERT ?\n\t\t\t_(\"Can't revert as initial commit\") :\n\t\t\t_(\"Can't cherry-pick into empty head\"));\n\t\tgoto fail;\n\t}\n\tsave_head(sha1_to_hex(sha1));\n\tsave_opts(opts);\n\tresult = pick_commits(todo_list, opts);\n\n\tfree_commit_list(todo_list);\n\treturn result;\n\nfail:\n\tfree_commit_list(todo_list);\n\treturn -1;\n\nIt does add another O(n) to the running time.  Some impatient person\nwatching a long post-multipick pause might want to add an optional\nSEQUENCER_FORGET_THE_MEMORY_WE_WILL_BE_DYING_SOON_ANYWAY flag\nargument, but probably that should happen as a separate change.\n\nSo for what it's worth, I like this change as long as it is done\nconsistently (meaning \"in all code paths that return from the\nfunction, not just the success case\").\n\nThanks.\nJonathan\n"},{"id":"191866","messageId":"CALkWK0m9F6EU43v0HbJxWUVtHTiw+ZvjCjwqbSVdQfomb6f4Aw@mail.gmail.com","threadId":"30579","inReplyTo":"20120521145610.1911.61154.chriscool@tuxfamily.org","subject":"Re: [PATCH 2/7] sequencer: release a strbuf used in save_head()","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-05-22T04:12:09Z","receivedAt":"2012-05-22T04:12:09Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Christian,\n\nChristian Couder wrote:\n> diff --git a/sequencer.c b/sequencer.c\n> index 762c527..ad1bbea 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -741,6 +741,7 @@ static void save_head(const char *head)\n>                die_errno(_(\"Could not write to %s\"), head_file);\n>        if (commit_lock_file(&head_lock) < 0)\n>                die(_(\"Error wrapping up %s.\"), head_file);\n> +       strbuf_release(&buf);\n>  }\n>\n>  static int reset_for_rollback(const unsigned char *sha1)\n\nThanks.  Shouldn't we be polluting the die() pathways with this\nstrbuf_release(&buf) the way save_todo() does it consistently?\n\n\tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n\t\tstrbuf_release(&buf);\n\t\tdie_errno(_(\"Could not write to %s\"), todo_file);\n\t}\n\tif (commit_lock_file(&todo_lock) < 0) {\n\t\tstrbuf_release(&buf);\n\t\tdie(_(\"Error wrapping up %s.\"), todo_file);\n\t}\n\tstrbuf_release(&buf);\n\nRam\n"},{"id":"191867","messageId":"20120522042316.GA3080@burratino","threadId":"30579","inReplyTo":"CALkWK0m9F6EU43v0HbJxWUVtHTiw+ZvjCjwqbSVdQfomb6f4Aw@mail.gmail.com","subject":"Re: [PATCH 2/7] sequencer: release a strbuf used in save_head()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-05-22T04:23:17Z","receivedAt":"2012-05-22T04:23:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n> Christian Couder wrote:\n\n>> diff --git a/sequencer.c b/sequencer.c\n>> index 762c527..ad1bbea 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -741,6 +741,7 @@ static void save_head(const char *head)\n>>                die_errno(_(\"Could not write to %s\"), head_file);\n>>        if (commit_lock_file(&head_lock) < 0)\n>>                die(_(\"Error wrapping up %s.\"), head_file);\n>> +       strbuf_release(&buf);\n>>  }\n>>\n>>  static int reset_for_rollback(const unsigned char *sha1)\n>\n> Thanks.  Shouldn't we be polluting the die() pathways with this\n> strbuf_release(&buf) the way save_todo() does it consistently?\n> \n> \tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n> \t\tstrbuf_release(&buf);\n> \t\tdie_errno(_(\"Could not write to %s\"), todo_file);\n\nI don't see why.  Doesn't exit take care of freeing everything?  And\nlooking at it from the other side, doesn't using exit mean that you\ncannot be valgrind-clean anyway, since allocations by functions higher\nin the call chain do not get a chance to be freed?\n\nJonathan\n"},{"id":"191872","messageId":"CALkWK0kvjr3NSx6-8svz=PKb5ta_UwOUiF4uqh7GriwuJYncUA@mail.gmail.com","threadId":"30579","inReplyTo":"20120522042316.GA3080@burratino","subject":"Re: [PATCH 2/7] sequencer: release a strbuf used in save_head()","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-05-22T05:07:36Z","receivedAt":"2012-05-22T05:07:36Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jonathan,\n\nJonathan Nieder wrote:\n> Ramkumar Ramachandra wrote:\n>> Christian Couder wrote:\n>>> [...]\n>>       if (write_in_full(fd, buf.buf, buf.len) < 0) {\n>>               strbuf_release(&buf);\n>>               die_errno(_(\"Could not write to %s\"), todo_file);\n>\n> [...]\n> And\n> looking at it from the other side, doesn't using exit mean that you\n> cannot be valgrind-clean anyway, since allocations by functions higher\n> in the call chain do not get a chance to be freed?\n\nGood point; save_todo() sets a bad example.  For symmetry, should\nthese two instances of strbuf_release() before die() be removed in a\nseparate patch?\n\nRam\n"},{"id":"191874","messageId":"CALkWK0=1a_LvQ=zhdAEt91j1sMsgp6J=_CyeNFOkB95dw15pnQ@mail.gmail.com","threadId":"30579","inReplyTo":"20120521145610.1911.57879.chriscool@tuxfamily.org","subject":"Re: [PATCH 4/7] revert: free opts.revs to do a bit of cleanup","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-05-22T05:14:59Z","receivedAt":"2012-05-22T05:14:59Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Christian,\n\nChristian Couder wrote:\n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index 82d1bf8..5f82a84 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -217,6 +217,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n>        git_config(git_default_config, NULL);\n>        parse_args(argc, argv, &opts);\n>        res = sequencer_pick_revisions(&opts);\n> +       free(opts.revs);\n>        if (res < 0)\n>                die(_(\"revert failed\"));\n>        return res;\n> @@ -232,6 +233,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n>        git_config(git_default_config, NULL);\n>        parse_args(argc, argv, &opts);\n>        res = sequencer_pick_revisions(&opts);\n> +       free(opts.revs);\n>        if (res < 0)\n>                die(_(\"cherry-pick failed\"));\n>        return res;\n\nTechnically, memory is allocated to both opts->revs and opts->xopts in\nparse_args().  Why not free both?\n\nRam\n"},{"id":"191875","messageId":"20120522051827.GA4486@burratino","threadId":"30579","inReplyTo":"CALkWK0kvjr3NSx6-8svz=PKb5ta_UwOUiF4uqh7GriwuJYncUA@mail.gmail.com","subject":"Re: [PATCH 2/7] sequencer: release a strbuf used in save_head()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-05-22T05:18:48Z","receivedAt":"2012-05-22T05:18:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n> Jonathan Nieder wrote:\n\n>> And\n>> looking at it from the other side, doesn't using exit mean that you\n>> cannot be valgrind-clean anyway, since allocations by functions higher\n>> in the call chain do not get a chance to be freed?\n>\n> Good point; save_todo() sets a bad example.  For symmetry, should\n> these two instances of strbuf_release() before die() be removed in a\n> separate patch?\n\nI can't find myself caring much either way. :)\n\nA single free() doesn't hurt performance much, so my hunch would be to\nleave it alone unless some other practical reason to keep or remove\nthe free()s comes up.\n"},{"id":"191897","messageId":"20120522141735.GA6660@burratino","threadId":"30579","inReplyTo":"20120521145610.1911.61154.chriscool@tuxfamily.org","subject":"Re: [PATCH 2/7] sequencer: release a strbuf used in save_head()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-05-22T14:17:35Z","receivedAt":"2012-05-22T14:17:35Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nChristian Couder wrote:\n \n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n>  sequencer.c | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 762c527..ad1bbea 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -741,6 +741,7 @@ static void save_head(const char *head)\n>  \t\tdie_errno(_(\"Could not write to %s\"), head_file);\n>  \tif (commit_lock_file(&head_lock) < 0)\n>  \t\tdie(_(\"Error wrapping up %s.\"), head_file);\n> +\tstrbuf_release(&buf);\n\nMakes good sense.  Actually, I'm not sure why this allocation is\nneeded in the first place.  Would something like the following work?\n\n-- >8 --\nSubject: sequencer: avoid a one-time leak in save_head()\n\nThis function uses the lockfile API to make its change to the\n.git/sequencer/head file effectively atomic, so there is no need to\ngather output intended for that file in a new buffer to write it in\none go.\n\nNoticed with valgrind.\n\nReported-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nBy the way, the lockfile API usage looks fishy.  What kind of races is\nthe locking meant to prevent?  What happens if someone tries \"git\ncherry-pick --abort\" in another window after my multipick has started\nand before it finishes?\n\n sequencer.c |    5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git i/sequencer.c w/sequencer.c\nindex 3c384b94..75b6a995 100644\n--- i/sequencer.c\n+++ w/sequencer.c\n@@ -732,12 +732,11 @@ static void save_head(const char *head)\n {\n \tconst char *head_file = git_path(SEQ_HEAD_FILE);\n \tstatic struct lock_file head_lock;\n-\tstruct strbuf buf = STRBUF_INIT;\n \tint fd;\n \n \tfd = hold_lock_file_for_update(&head_lock, head_file, LOCK_DIE_ON_ERROR);\n-\tstrbuf_addf(&buf, \"%s\\n\", head);\n-\tif (write_in_full(fd, buf.buf, buf.len) < 0)\n+\tif (write_in_full(fd, head, strlen(head)) < 0 ||\n+\t    write_in_full(fd, \"\\n\", 1) < 0)\n \t\tdie_errno(_(\"Could not write to %s\"), head_file);\n \tif (commit_lock_file(&head_lock) < 0)\n \t\tdie(_(\"Error wrapping up %s.\"), head_file);\n"},{"id":"191931","messageId":"20120522.220142.31626389365969573.chriscool@tuxfamily.org","threadId":"30579","inReplyTo":"20120521203941.GB28331@burratino","subject":"Re: [PATCH 5/7] revert: free revs->cmdline.rev","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2012-05-22T20:01:42Z","receivedAt":"2012-05-22T20:01:42Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nFrom: Jonathan Nieder <jrnieder@gmail.com>\n> Hi,\n> \n> Christian Couder wrote:\n> \n>> add_rev_cmdline() in revision.c is (re)allocating an array of\n>> struct rev_cmdline_entry. This patch releases it.\n> [...]\n>> --- a/builtin/revert.c\n>> +++ b/builtin/revert.c\n>> @@ -217,6 +217,8 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n>>  \tgit_config(git_default_config, NULL);\n>>  \tparse_args(argc, argv, &opts);\n>>  \tres = sequencer_pick_revisions(&opts);\n>> +\tif (opts.revs)\n>> +\t\tfree(opts.revs->cmdline.rev);\n>>  \tfree(opts.revs);\n>>  \tif (res < 0)\n>>  \t\tdie(_(\"revert failed\"));\n> \n> Quick thoughts:\n> \n> This feels like a layering violation.  Avoidable?  Maybe revision.c\n> could gain a helper to allow this to be written like so:\n> \n> \tfree_rev_info(opts.revs);\n> \tfree(opts.revs);\n> \n> Since this is a one-time allocation it is probably worth mentioning in\n> the log message that this is a futureproofing/valgrind-cleanliness\n> measure and is not actually fixing a leak.\n\nOk, I will create the free_rev_info() helper and improve the commit\nmessage.\n\n> Micronit: it would feel slightly more comfortable if the free() were\n> after the die(), even though the die() should probably be changed to\n> exit().  That way someone wanting to add code after the die() that\n> continues to assume opts.revs is valid would be able to.\n> \n> Of course I'd imagine the largest leak in cherry-pick is the\n> deliberately constantly growing object hash table.  It would be very\n> interesting to fix that --- do you know how libgit2 handles it?\n\nAbout the constantly growing hash table, perhaps it should be taken\ncare of in a try_to_free_routine() used by xmalloc and other such\nfunctions. And no I don't know much about libgit2.\n\n> Thanks much and hope that helps,\n> Jonathan\n\nThanks for your kind review,\nChristian.\n"},{"id":"191932","messageId":"20120522.220310.2276783773227531713.chriscool@tuxfamily.org","threadId":"30579","inReplyTo":"CALkWK0=1a_LvQ=zhdAEt91j1sMsgp6J=_CyeNFOkB95dw15pnQ@mail.gmail.com","subject":"Re: [PATCH 4/7] revert: free opts.revs to do a bit of cleanup","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2012-05-22T20:03:10Z","receivedAt":"2012-05-22T20:03:10Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Ram,\n\nFrom: Ramkumar Ramachandra <artagnon@gmail.com>\n> Hi Christian,\n> \n> Christian Couder wrote:\n>> diff --git a/builtin/revert.c b/builtin/revert.c\n>> index 82d1bf8..5f82a84 100644\n>> --- a/builtin/revert.c\n>> +++ b/builtin/revert.c\n>> @@ -217,6 +217,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n>>        git_config(git_default_config, NULL);\n>>        parse_args(argc, argv, &opts);\n>>        res = sequencer_pick_revisions(&opts);\n>> +       free(opts.revs);\n>>        if (res < 0)\n>>                die(_(\"revert failed\"));\n>>        return res;\n>> @@ -232,6 +233,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n>>        git_config(git_default_config, NULL);\n>>        parse_args(argc, argv, &opts);\n>>        res = sequencer_pick_revisions(&opts);\n>> +       free(opts.revs);\n>>        if (res < 0)\n>>                die(_(\"cherry-pick failed\"));\n>>        return res;\n> \n> Technically, memory is allocated to both opts->revs and opts->xopts in\n> parse_args().  Why not free both?\n\nI didn't see any leak with opts->xopts but I will have a look.\n\nThanks,\nChristian.\n"},{"id":"191935","messageId":"20120522.222242.2217647953969252595.chriscool@tuxfamily.org","threadId":"30579","inReplyTo":"20120521204350.GC28331@burratino","subject":"Re: [PATCH 6/7] unpack-trees: record which unpack error messages should be freed","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2012-05-22T20:22:42Z","receivedAt":"2012-05-22T20:22:42Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Jonathan Nieder <jrnieder@gmail.com>\n> Christian Couder wrote:\n> \n>> \"struct unpack_trees_options\" has a \"const char *msgs[]\" field\n>> that is setup with string values in setup_unpack_trees_porcelain().\n> \n> Hmm.  Incidentally, should callers (e.g., the 100 single-picks\n> involved in a large multi-pick) be reusing these strings instead of\n> allocating them again and again?\n\nSome strings filling the msgs field are created with sprintf() using\nthe cmd argument passed to setup_unpack_trees_porcelain(). So they\ncould be different if different cmd arguments are used. In practice\nnow cmd is either \"checkout\" or \"merge\", so we could probably reuse\nthe same strings. But if we ever add a checkout command to the\nsequencer for example, we might want to use different error messages.\n\nAnother possibility is to xmalloc all the strings and then free them\nall.\n\nThanks,\nChristian.\n"},{"id":"191937","messageId":"20120522.222431.25966410445815006.chriscool@tuxfamily.org","threadId":"30579","inReplyTo":"20120521205701.GD28331@burratino","subject":"Re: [PATCH 1/7] sequencer: fix leaked todo_list memory","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2012-05-22T20:24:31Z","receivedAt":"2012-05-22T20:24:31Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Jonathan Nieder <jrnieder@gmail.com>\n> \n> So for what it's worth, I like this change as long as it is done\n> consistently (meaning \"in all code paths that return from the\n> function, not just the success case\").\n\nOk, I will do that.\n\nThanks,\nChristian.\n"},{"id":"191940","messageId":"20120522.223045.876953043846314069.chriscool@tuxfamily.org","threadId":"30579","inReplyTo":"20120522141735.GA6660@burratino","subject":"Re: [PATCH 2/7] sequencer: release a strbuf used in save_head()","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2012-05-22T20:30:45Z","receivedAt":"2012-05-22T20:30:45Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Jonathan Nieder <jrnieder@gmail.com>\n>\n>> +\tstrbuf_release(&buf);\n> \n> Makes good sense.  Actually, I'm not sure why this allocation is\n> needed in the first place.  Would something like the following work?\n\nOk, I will use your patch in the next iteration.\n\nThanks,\nChristian.\n"},{"id":"191945","messageId":"20120522204014.GA19789@burratino","threadId":"30579","inReplyTo":"20120522.220142.31626389365969573.chriscool@tuxfamily.org","subject":"Re: [PATCH 5/7] revert: free revs->cmdline.rev","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-05-22T20:40:15Z","receivedAt":"2012-05-22T20:40:15Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Christian Couder wrote:\n\n> About the constantly growing hash table, perhaps it should be taken\n> care of in a try_to_free_routine() used by xmalloc and other such\n> functions. And no I don't know much about libgit2.\n\nHm.  At first glance that sounds interesting (some kind of\nmark-and-sweep garbage collection, so at least allocation would never\n_fail_ due to too large a multipick).  At second glance it is less\nexciting, since the logic would only kick in when malloc() fails, and\nif I have a lot of swap then my system will have slowed to a crawl\nlong before then.\n\nHere are a couple of alternative ideas, with no code to back them up.\n\nA. Save a copy of obj_hash after revision traversal and before\ncherry-picking anything, and reset the table to that state after every\n(let's say) 5 commits cherry-picked.  Or perhaps empty the table\ncompletely after every 5 commits or so cherry-picked.\n\nB. Re-exec git using \"git sequence --continue\" so everything gets\nallocated anew after every 5 commits or so.\n\nThat second idea sounds pretty ugly, but it would probably be\neffective.  On Windows it's fork(), not execve(), that is expensive,\nright? :)\n\nJust musing,\nJonathan\n"}]}