{"thread":{"id":"29816","subject":"[RFH] unpack-trees: cache_entry lifetime issue?","startedAt":"2012-03-02T17:25:54Z","lastAt":"2012-03-06T19:37:23Z","messageCount":5,"participants":["René Scharfe","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"185930","messageId":"4F5102A2.70303@lsrfire.ath.cx","threadId":"29816","inReplyTo":null,"subject":"[RFH] unpack-trees: cache_entry lifetime issue?","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-03-02T17:25:54Z","receivedAt":"2012-03-02T17:25:54Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Hello,\n\nbelow is the output of \"git grep -hnW unpack_nondirectories\",\nwhich shows some parts of unpack-trees.c that I use as context to\nask: Should we check for o->merge in line 775, before using src[0]?\n\nIf o->merge is 0, the src[0] will be NULL right up to the call of\nunpack_nondirectories() in line 772.  There it may be set (in line\n582).  In that case we'll end up at line 779, where mark_ce_used()\nis applied to it.\n\nI suspect that this is unintended and that line 775 should rather\nread \"if (o->merge && src[0]) {\".  Can someone with a better\nunderstanding of unpack-trees confirm or refute that suspicion?\n\nThanks,\nRené\n\n\n542:static int unpack_nondirectories(int n, unsigned long mask,\n543-\t\t\t\t unsigned long dirmask,\n544-\t\t\t\t struct cache_entry **src,\n545-\t\t\t\t const struct name_entry *names,\n546-\t\t\t\t const struct traverse_info *info)\n547-{\n548-\tint i;\n549-\tstruct unpack_trees_options *o = info->data;\n550-\tunsigned long conflicts;\n551-\n552-\t/* Do we have *only* directories? Nothing to do */\n553-\tif (mask == dirmask && !src[0])\n554-\t\treturn 0;\n555-\n556-\tconflicts = info->conflicts;\n557-\tif (o->merge)\n558-\t\tconflicts >>= 1;\n559-\tconflicts |= dirmask;\n560-\n561-\t/*\n562-\t * Ok, we've filled in up to any potential index entry in src[0],\n563-\t * now do the rest.\n564-\t */\n565-\tfor (i = 0; i < n; i++) {\n566-\t\tint stage;\n567-\t\tunsigned int bit = 1ul << i;\n568-\t\tif (conflicts & bit) {\n569-\t\t\tsrc[i + o->merge] = o->df_conflict_entry;\n570-\t\t\tcontinue;\n571-\t\t}\n572-\t\tif (!(mask & bit))\n573-\t\t\tcontinue;\n574-\t\tif (!o->merge)\n575-\t\t\tstage = 0;\n576-\t\telse if (i + 1 < o->head_idx)\n577-\t\t\tstage = 1;\n578-\t\telse if (i + 1 > o->head_idx)\n579-\t\t\tstage = 3;\n580-\t\telse\n581-\t\t\tstage = 2;\n\n582-\t\tsrc[i + o->merge] = create_ce_entry(info, names + i, stage);\n\n583-\t}\n584-\n585-\tif (o->merge)\n586-\t\treturn call_unpack_fn(src, o);\n587-\n588-\tfor (i = 0; i < n; i++)\n589-\t\tif (src[i] && src[i] != o->df_conflict_entry)\n590-\t\t\tadd_entry(o, src[i], 0, 0);\n591-\treturn 0;\n592-}\n593-\n--\n721=static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, struct name_entry *names, struct traverse_info *info)\n722-{\n723-\tstruct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };\n724-\tstruct unpack_trees_options *o = info->data;\n725-\tconst struct name_entry *p = names;\n726-\n727-\t/* Find first entry with a real name (we could use \"mask\" too) */\n728-\twhile (!p->mode)\n729-\t\tp++;\n730-\n731-\tif (o->debug_unpack)\n732-\t\tdebug_unpack_callback(n, mask, dirmask, names, info);\n733-\n734-\t/* Are we supposed to look at the index too? */\n735-\tif (o->merge) {\n736-\t\twhile (1) {\n737-\t\t\tint cmp;\n738-\t\t\tstruct cache_entry *ce;\n739-\n740-\t\t\tif (o->diff_index_cached)\n741-\t\t\t\tce = next_cache_entry(o);\n742-\t\t\telse\n743-\t\t\t\tce = find_cache_entry(info, p);\n744-\n745-\t\t\tif (!ce)\n746-\t\t\t\tbreak;\n747-\t\t\tcmp = compare_entry(ce, info, p);\n748-\t\t\tif (cmp < 0) {\n749-\t\t\t\tif (unpack_index_entry(ce, o) < 0)\n750-\t\t\t\t\treturn unpack_failed(o, NULL);\n751-\t\t\t\tcontinue;\n752-\t\t\t}\n753-\t\t\tif (!cmp) {\n754-\t\t\t\tif (ce_stage(ce)) {\n755-\t\t\t\t\t/*\n756-\t\t\t\t\t * If we skip unmerged index\n757-\t\t\t\t\t * entries, we'll skip this\n758-\t\t\t\t\t * entry *and* the tree\n759-\t\t\t\t\t * entries associated with it!\n760-\t\t\t\t\t */\n761-\t\t\t\t\tif (o->skip_unmerged) {\n762-\t\t\t\t\t\tadd_same_unmerged(ce, o);\n763-\t\t\t\t\t\treturn mask;\n764-\t\t\t\t\t}\n765-\t\t\t\t}\n766-\t\t\t\tsrc[0] = ce;\n767-\t\t\t}\n768-\t\t\tbreak;\n769-\t\t}\n770-\t}\n771-\n\n772:\tif (unpack_nondirectories(n, mask, dirmask, src, names, info) < 0)\n773-\t\treturn -1;\n774-\n775-\tif (src[0]) {\n776-\t\tif (ce_stage(src[0]))\n777-\t\t\tmark_ce_used_same_name(src[0], o);\n778-\t\telse\n779-\t\t\tmark_ce_used(src[0], o);\n780-\t}\n\n781-\n782-\t/* Now handle any directories.. */\n783-\tif (dirmask) {\n784-\t\tunsigned long conflicts = mask & ~dirmask;\n785-\t\tif (o->merge) {\n786-\t\t\tconflicts <<= 1;\n787-\t\t\tif (src[0])\n788-\t\t\t\tconflicts |= 1;\n789-\t\t}\n790-\n791-\t\t/* special case: \"diff-index --cached\" looking at a tree */\n792-\t\tif (o->diff_index_cached &&\n793-\t\t    n == 1 && dirmask == 1 && S_ISDIR(names->mode)) {\n794-\t\t\tint matches;\n795-\t\t\tmatches = cache_tree_matches_traversal(o->src_index->cache_tree,\n796-\t\t\t\t\t\t\t       names, info);\n797-\t\t\t/*\n798-\t\t\t * Everything under the name matches; skip the\n799-\t\t\t * entire hierarchy.  diff_index_cached codepath\n800-\t\t\t * special cases D/F conflicts in such a way that\n801-\t\t\t * it does not do any look-ahead, so this is safe.\n802-\t\t\t */\n803-\t\t\tif (matches) {\n804-\t\t\t\to->cache_bottom += matches;\n805-\t\t\t\treturn mask;\n806-\t\t\t}\n807-\t\t}\n808-\n809-\t\tif (traverse_trees_recursive(n, dirmask, conflicts,\n810-\t\t\t\t\t     names, info) < 0)\n811-\t\t\treturn -1;\n812-\t\treturn mask;\n813-\t}\n814-\n815-\treturn mask;\n816-}\n817-\n\nPS: These functions are a tad too long, aren't they? ;-)\n"},{"id":"185943","messageId":"7vk432dd89.fsf@alter.siamese.dyndns.org","threadId":"29816","inReplyTo":"4F5102A2.70303@lsrfire.ath.cx","subject":"Re: [RFH] unpack-trees: cache_entry lifetime issue?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-02T19:17:26Z","receivedAt":"2012-03-02T19:17:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> which shows some parts of unpack-trees.c that I use as context to\n> ask: Should we check for o->merge in line 775, before using src[0]?\n>\n> If o->merge is 0, the src[0] will be NULL right up to the call of\n> unpack_nondirectories() in line 772.  There it may be set (in line\n> 582).  In that case we'll end up at line 779, where mark_ce_used()\n> is applied to it.\n>\n> I suspect that this is unintended and that line 775 should rather\n> read \"if (o->merge && src[0]) {\".  Can someone with a better\n> understanding of unpack-trees confirm or refute that suspicion?\n\nYeah, src[0] is meant to hold the entry from the current index to take it\nas well as our tree into account during o->merge, and I do not think it\nshould affect when we are only reading tree(s) into the index.\n\nI think da165f4 (unpack-trees.c: prepare for looking ahead in the index,\n2010-01-07) simply forgot that the codepath also has to work when it is\nnot merging.\n\nHaving said that, I do not know offhand if we just should nothing in\nno-merge case, or we should be doing something else instead, without\nthinking a bit more.\n"},{"id":"185987","messageId":"4F522758.9050205@lsrfire.ath.cx","threadId":"29816","inReplyTo":"7vk432dd89.fsf@alter.siamese.dyndns.org","subject":"Re: [RFH] unpack-trees: cache_entry lifetime issue?","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-03-03T14:14:48Z","receivedAt":"2012-03-03T14:14:48Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 02.03.2012 20:17, schrieb Junio C Hamano:\n> René Scharfe<rene.scharfe@lsrfire.ath.cx>  writes:\n> \n>> which shows some parts of unpack-trees.c that I use as context to\n>> ask: Should we check for o->merge in line 775, before using src[0]?\n>>\n>> If o->merge is 0, the src[0] will be NULL right up to the call of\n>> unpack_nondirectories() in line 772.  There it may be set (in line\n>> 582).  In that case we'll end up at line 779, where mark_ce_used()\n>> is applied to it.\n>>\n>> I suspect that this is unintended and that line 775 should rather\n>> read \"if (o->merge&&  src[0]) {\".  Can someone with a better\n>> understanding of unpack-trees confirm or refute that suspicion?\n> \n> Yeah, src[0] is meant to hold the entry from the current index to take it\n> as well as our tree into account during o->merge, and I do not think it\n> should affect when we are only reading tree(s) into the index.\n> \n> I think da165f4 (unpack-trees.c: prepare for looking ahead in the index,\n> 2010-01-07) simply forgot that the codepath also has to work when it is\n> not merging.\n> \n> Having said that, I do not know offhand if we just should nothing in\n> no-merge case, or we should be doing something else instead, without\n> thinking a bit more.\n\nThanks.\n\nNext question: Should the function fn() in struct unpack_trees_options\nbe able to replace src[0], and unpack_callback() is then supposed to\nuse the new pointer after calling unpack_nondirectories()?  If not\nthen we can clean up things a bit by moving the src array into\nunpack_nondirectories().\n\nFor now, just this patch, which cleans up memory, but not the code:\n\n-- >8 --\nSubject: unpack-trees: plug minor memory leak\n\nThe allocations made by unpack_nondirectories() using create_ce_entry()\nare never freed.  In the case of a merge, we hand them to\ncall_unpack_fn() and never look at them again.  In the non-merge case,\nwe duplicate them using add_entry() and later only look at the first\nallocated element (src[0]), perhaps even only by mistake.\n\nTo clean up after ourselves, explicitly loop through the entries and\nfree their memory for merges.  For non-merges, split out the actual\naddition from add_entry() into the new helper do_add_entry().  Then\ncall that non-duplicating function instead of add_entry() to avoid the\nleak.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n unpack-trees.c |   35 ++++++++++++++++++++++++-----------\n 1 file changed, 24 insertions(+), 11 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 7c9ecf6..c594e4a 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -102,21 +102,28 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n \t\topts->unpack_rejects[i].strdup_strings = 1;\n }\n \n-static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,\n-\tunsigned int set, unsigned int clear)\n+static void do_add_entry(struct unpack_trees_options *o, struct cache_entry *ce,\n+\t\t\t unsigned int set, unsigned int clear)\n {\n-\tunsigned int size = ce_size(ce);\n-\tstruct cache_entry *new = xmalloc(size);\n-\n \tclear |= CE_HASHED | CE_UNHASHED;\n \n \tif (set & CE_REMOVE)\n \t\tset |= CE_WT_REMOVE;\n \n+\tce->next = NULL;\n+\tce->ce_flags = (ce->ce_flags & ~clear) | set;\n+\tadd_index_entry(&o->result, ce,\n+\t\t\tADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE);\n+}\n+\n+static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,\n+\tunsigned int set, unsigned int clear)\n+{\n+\tunsigned int size = ce_size(ce);\n+\tstruct cache_entry *new = xmalloc(size);\n+\n \tmemcpy(new, ce, size);\n-\tnew->next = NULL;\n-\tnew->ce_flags = (new->ce_flags & ~clear) | set;\n-\tadd_index_entry(&o->result, new, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n+\tdo_add_entry(o, new, set, clear);\n }\n \n /*\n@@ -582,12 +589,18 @@ static int unpack_nondirectories(int n, unsigned long mask,\n \t\tsrc[i + o->merge] = create_ce_entry(info, names + i, stage);\n \t}\n \n-\tif (o->merge)\n-\t\treturn call_unpack_fn(src, o);\n+\tif (o->merge) {\n+\t\tint rc = call_unpack_fn(src, o);\n+\t\tfor (i = 0; i < n; i++) {\n+\t\t\tif (src[i + 1] != o->df_conflict_entry)\n+\t\t\t\tfree(src[i + 1]);\n+\t\t}\n+\t\treturn rc;\n+\t}\n \n \tfor (i = 0; i < n; i++)\n \t\tif (src[i] && src[i] != o->df_conflict_entry)\n-\t\t\tadd_entry(o, src[i], 0, 0);\n+\t\t\tdo_add_entry(o, src[i], 0, 0);\n \treturn 0;\n }\n \n-- \n1.7.9.2\n"},{"id":"186147","messageId":"7vaa3uzozy.fsf@alter.siamese.dyndns.org","threadId":"29816","inReplyTo":"4F522758.9050205@lsrfire.ath.cx","subject":"Re: [RFH] unpack-trees: cache_entry lifetime issue?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-05T22:01:21Z","receivedAt":"2012-03-05T22:01:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> Next question: Should the function fn() in struct unpack_trees_options\n> be able to replace src[0], and unpack_callback() is then supposed to\n> use the new pointer after calling unpack_nondirectories()?  If not\n> then we can clean up things a bit by moving the src array into\n> unpack_nondirectories().\n\nSorry, I have no idea.  What kind of usage pattern do you have in\nmind?\n\n> For now, just this patch, which cleans up memory, but not the code:\n>\n> -- >8 --\n> Subject: unpack-trees: plug minor memory leak\n>\n> The allocations made by unpack_nondirectories() using create_ce_entry()\n> are never freed.  In the case of a merge, we hand them to\n> call_unpack_fn() and never look at them again.\n\nThat assumes that whatever callbacks that are called will only look\nat but never takes ownership of the cache entry given to them.  I\n*think* everybody eventually calls \"add_entry()\" that duplicates the\ncache entry before storing it to the index, but I didn't go through\nall the codepaths.  Assuming you did, I think this is a good change.\n\n> In the non-merge case,\n> we duplicate them using add_entry() and later only look at the first\n> allocated element (src[0]), perhaps even only by mistake.\n>\n> To clean up after ourselves, explicitly loop through the entries and\n> free their memory for merges.  For non-merges, split out the actual\n> addition from add_entry() into the new helper do_add_entry().  Then\n> call that non-duplicating function instead of add_entry() to avoid the\n> leak.\n>\n> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n> ---\n>  unpack-trees.c |   35 ++++++++++++++++++++++++-----------\n>  1 file changed, 24 insertions(+), 11 deletions(-)\n>\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 7c9ecf6..c594e4a 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -102,21 +102,28 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n>  \t\topts->unpack_rejects[i].strdup_strings = 1;\n>  }\n>  \n> -static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,\n> -\tunsigned int set, unsigned int clear)\n> +static void do_add_entry(struct unpack_trees_options *o, struct cache_entry *ce,\n> +\t\t\t unsigned int set, unsigned int clear)\n>  {\n> -\tunsigned int size = ce_size(ce);\n> -\tstruct cache_entry *new = xmalloc(size);\n> -\n>  \tclear |= CE_HASHED | CE_UNHASHED;\n>  \n>  \tif (set & CE_REMOVE)\n>  \t\tset |= CE_WT_REMOVE;\n>  \n> +\tce->next = NULL;\n> +\tce->ce_flags = (ce->ce_flags & ~clear) | set;\n> +\tadd_index_entry(&o->result, ce,\n> +\t\t\tADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE);\n> +}\n> +\n> +static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,\n> +\tunsigned int set, unsigned int clear)\n> +{\n> +\tunsigned int size = ce_size(ce);\n> +\tstruct cache_entry *new = xmalloc(size);\n> +\n>  \tmemcpy(new, ce, size);\n> -\tnew->next = NULL;\n> -\tnew->ce_flags = (new->ce_flags & ~clear) | set;\n> -\tadd_index_entry(&o->result, new, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n> +\tdo_add_entry(o, new, set, clear);\n>  }\n>  \n>  /*\n> @@ -582,12 +589,18 @@ static int unpack_nondirectories(int n, unsigned long mask,\n>  \t\tsrc[i + o->merge] = create_ce_entry(info, names + i, stage);\n>  \t}\n>  \n> -\tif (o->merge)\n> -\t\treturn call_unpack_fn(src, o);\n> +\tif (o->merge) {\n> +\t\tint rc = call_unpack_fn(src, o);\n> +\t\tfor (i = 0; i < n; i++) {\n> +\t\t\tif (src[i + 1] != o->df_conflict_entry)\n> +\t\t\t\tfree(src[i + 1]);\n> +\t\t}\n> +\t\treturn rc;\n> +\t}\n>  \n>  \tfor (i = 0; i < n; i++)\n>  \t\tif (src[i] && src[i] != o->df_conflict_entry)\n> -\t\t\tadd_entry(o, src[i], 0, 0);\n> +\t\t\tdo_add_entry(o, src[i], 0, 0);\n>  \treturn 0;\n>  }\n"},{"id":"186242","messageId":"4F566773.5060005@lsrfire.ath.cx","threadId":"29816","inReplyTo":"7vaa3uzozy.fsf@alter.siamese.dyndns.org","subject":"Re: [RFH] unpack-trees: cache_entry lifetime issue?","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-03-06T19:37:23Z","receivedAt":"2012-03-06T19:37:23Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.03.2012 23:01, schrieb Junio C Hamano:\n> René Scharfe<rene.scharfe@lsrfire.ath.cx>  writes:\n> \n>> Next question: Should the function fn() in struct unpack_trees_options\n>> be able to replace src[0], and unpack_callback() is then supposed to\n>> use the new pointer after calling unpack_nondirectories()?  If not\n>> then we can clean up things a bit by moving the src array into\n>> unpack_nondirectories().\n> \n> Sorry, I have no idea.  What kind of usage pattern do you have in\n> mind?\n\nWell, I can't imagine a merge callback that needs this ability, but\nthat really doesn't mean much.  Just want to avoid taking away options\nthat turn out to be useful later.\n\n>> For now, just this patch, which cleans up memory, but not the code:\n>>\n>> -- >8 --\n>> Subject: unpack-trees: plug minor memory leak\n>>\n>> The allocations made by unpack_nondirectories() using create_ce_entry()\n>> are never freed.  In the case of a merge, we hand them to\n>> call_unpack_fn() and never look at them again.\n> \n> That assumes that whatever callbacks that are called will only look\n> at but never takes ownership of the cache entry given to them.  I\n> *think* everybody eventually calls \"add_entry()\" that duplicates the\n> cache entry before storing it to the index, but I didn't go through\n> all the codepaths.  Assuming you did, I think this is a good change.\n\nI did that a while back and while doing that again just now with\ntired eyes after a day's work noticed how deep the call chains are\nand that it's certainly possible that I missed a place that assumed\nit owned the cache entry.\n\nFor increased confidence, perhaps it's better to first patch the\nnon-merge leak, then clean up and const'ify the merge case and only\nthen free().  Or to declare that merge functions can take ownership\nof the cache entries.  Timid patch for the first part below.\n\n-- >8 --\nSubject: unpack-trees: plug minor memory leak\n\nThe allocations made by unpack_nondirectories() using create_ce_entry()\nare never freed.\n\nIn the non-merge case, we duplicate them using add_entry() and later\nonly look at the first allocated element (src[0]), perhaps even only\nby mistake.  Split out the actual addition from add_entry() into the\nnew helper do_add_entry() and call this non-duplicating function\ninstead of add_entry() to avoid the leak.\n\nValgrind reports this for the command \"git archive v1.7.9\" without\nthe patch:\n\n  ==13372== LEAK SUMMARY:\n  ==13372==    definitely lost: 230,986 bytes in 2,325 blocks\n  ==13372==    indirectly lost: 0 bytes in 0 blocks\n  ==13372==      possibly lost: 98 bytes in 1 blocks\n  ==13372==    still reachable: 2,259,198 bytes in 3,243 blocks\n  ==13372==         suppressed: 0 bytes in 0 blocks\n\nAnd with the patch applied:\n\n  ==13375== LEAK SUMMARY:\n  ==13375==    definitely lost: 65 bytes in 1 blocks\n  ==13375==    indirectly lost: 0 bytes in 0 blocks\n  ==13375==      possibly lost: 0 bytes in 0 blocks\n  ==13375==    still reachable: 2,364,417 bytes in 3,245 blocks\n  ==13375==         suppressed: 0 bytes in 0 blocks\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n unpack-trees.c |   25 ++++++++++++++++---------\n 1 file changed, 16 insertions(+), 9 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 7c9ecf6..f9c74bd 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -102,21 +102,28 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,\n \t\topts->unpack_rejects[i].strdup_strings = 1;\n }\n \n-static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,\n-\tunsigned int set, unsigned int clear)\n+static void do_add_entry(struct unpack_trees_options *o, struct cache_entry *ce,\n+\t\t\t unsigned int set, unsigned int clear)\n {\n-\tunsigned int size = ce_size(ce);\n-\tstruct cache_entry *new = xmalloc(size);\n-\n \tclear |= CE_HASHED | CE_UNHASHED;\n \n \tif (set & CE_REMOVE)\n \t\tset |= CE_WT_REMOVE;\n \n+\tce->next = NULL;\n+\tce->ce_flags = (ce->ce_flags & ~clear) | set;\n+\tadd_index_entry(&o->result, ce,\n+\t\t\tADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE);\n+}\n+\n+static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,\n+\tunsigned int set, unsigned int clear)\n+{\n+\tunsigned int size = ce_size(ce);\n+\tstruct cache_entry *new = xmalloc(size);\n+\n \tmemcpy(new, ce, size);\n-\tnew->next = NULL;\n-\tnew->ce_flags = (new->ce_flags & ~clear) | set;\n-\tadd_index_entry(&o->result, new, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n+\tdo_add_entry(o, new, set, clear);\n }\n \n /*\n@@ -587,7 +594,7 @@ static int unpack_nondirectories(int n, unsigned long mask,\n \n \tfor (i = 0; i < n; i++)\n \t\tif (src[i] && src[i] != o->df_conflict_entry)\n-\t\t\tadd_entry(o, src[i], 0, 0);\n+\t\t\tdo_add_entry(o, src[i], 0, 0);\n \treturn 0;\n }\n \n-- \n1.7.9.2\n"}]}