{"thread":{"id":"33986","subject":"[PATCH 0/4] cherry-pick: fix memory leaks","startedAt":"2013-05-30T11:58:52Z","lastAt":"2013-06-02T22:45:06Z","messageCount":16,"participants":["Felipe Contreras","Thomas Rast","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"218977","messageId":"1369915136-4248-1-git-send-email-felipe.contreras@gmail.com","threadId":"33986","inReplyTo":null,"subject":"[PATCH 0/4] cherry-pick: fix memory leaks","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-30T11:58:52Z","receivedAt":"2013-05-30T11:58:52Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nI took a shot at fixing the memory leaks of cherry-pick, and at least in my\ntests the memory doesn't seem to increase any more.\n\nFelipe Contreras (4):\n commit: reload cache properly\n read-cache: plug small memory leak\n unpack-trees: plug a memory leak\n unpack-trees: free created cache entries\n\n builtin/commit.c | 2 ++\n read-cache.c | 2 ++\n unpack-trees.c | 16 +++++++++++++---\n 3 files changed, 17 insertions(+), 3 deletions(-)\n\n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218978","messageId":"1369915136-4248-2-git-send-email-felipe.contreras@gmail.com","threadId":"33986","inReplyTo":"1369915136-4248-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 1/4] commit: reload cache properly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-30T11:58:53Z","receivedAt":"2013-05-30T11:58:53Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"We are supposedly adding files, to to which cache if 'the_index' is\ndiscarded?\n\nSigned-off-by: Felipe Contreras \n---\n builtin/commit.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex d2f30d9..092b49e 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -244,6 +244,8 @@ static void create_base_index(const struct commit *current_head)\n \n \tif (!current_head) {\n \t\tdiscard_cache();\n+\t\tif (read_cache() < 0)\n+\t\t\tdie(_(\"cannot read the index\"));\n \t\treturn;\n \t}\n \n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218979","messageId":"1369915136-4248-3-git-send-email-felipe.contreras@gmail.com","threadId":"33986","inReplyTo":"1369915136-4248-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 2/4] read-cache: plug small memory leak","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-30T11:58:54Z","receivedAt":"2013-05-30T11:58:54Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"We free each entry, but not the array of entries themselves.\n\nSigned-off-by: Felipe Contreras \n---\n read-cache.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 04ed561..9d9b886 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1510,6 +1510,8 @@ int discard_index(struct index_state *istate)\n \n \tfor (i = 0; i < istate->cache_nr; i++)\n \t\tfree(istate->cache[i]);\n+\tfree(istate->cache);\n+\tistate->cache = NULL;\n \tresolve_undo_clear_index(istate);\n \tistate->cache_nr = 0;\n \tistate->cache_changed = 0;\n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218980","messageId":"1369915136-4248-4-git-send-email-felipe.contreras@gmail.com","threadId":"33986","inReplyTo":"1369915136-4248-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 3/4] unpack-trees: plug a memory leak","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-30T11:58:55Z","receivedAt":"2013-05-30T11:58:55Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Before overwriting the destination index, first let's discard it's\ncontents.\n\nSigned-off-by: Felipe Contreras \n---\n unpack-trees.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex ede4299..eff2944 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1146,8 +1146,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \n \to->src_index = NULL;\n \tret = check_updates(o) ? (-2) : 0;\n-\tif (o->dst_index)\n+\tif (o->dst_index) {\n+\t\tdiscard_index(o->dst_index);\n \t\t*o->dst_index = o->result;\n+\t}\n \n done:\n \tclear_exclude_list(&el);\n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218981","messageId":"1369915136-4248-5-git-send-email-felipe.contreras@gmail.com","threadId":"33986","inReplyTo":"1369915136-4248-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 4/4] unpack-trees: free created cache entries","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-30T11:58:56Z","receivedAt":"2013-05-30T11:58:56Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"We created them, and nobody else is going to destroy them.\n\nSigned-off-by: Felipe Contreras \n---\n unpack-trees.c | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex eff2944..9f19d01 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -590,8 +590,16 @@ 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 ret = call_unpack_fn(src, o);\n+\t\tfor (i = 0; i < n; i++) {\n+\t\t\tstruct cache_entry *ce = src[i + o->merge];\n+\t\t\tif (!ce || ce == o->df_conflict_entry)\n+\t\t\t\tcontinue;\n+\t\t\tfree(ce);\n+\t\t}\n+\t\treturn ret;\n+\t}\n \n \tfor (i = 0; i < n; i++)\n \t\tif (src[i] && src[i] != o->df_conflict_entry)\n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218985","messageId":"87ehcok6gl.fsf@linux-k42r.v.cablecom.net","threadId":"33986","inReplyTo":"1369915136-4248-2-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 1/4] commit: reload cache properly","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-05-30T12:17:14Z","receivedAt":"2013-05-30T12:17:14Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Felipe Contreras writes:\n\n> We are supposedly adding files, to to which cache if 'the_index' is\n> discarded?\n[...]\n> \tif (!current_head) {\n> \t\tdiscard_cache();\n> +\t\tif (read_cache() < 0)\n> +\t\t\tdie(_(\"cannot read the index\"));\n> \t\treturn;\n> \t}\n\nIt is not obvious to me that this is a correct change. discard_cache()\nwithout subsequent reloading could also legitimately be used to empty\nthe index. So if you are fixing a bug, please justify the change and\nprovide a testcase to guard against it in the future.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"218987","messageId":"CAMP44s1O=VTu8EZi+yOfGMccCpS+pozvZJuDW1mK95U8-YEquA@mail.gmail.com","threadId":"33986","inReplyTo":"87ehcok6gl.fsf@linux-k42r.v.cablecom.net","subject":"Re: [PATCH 1/4] commit: reload cache properly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-30T12:35:53Z","receivedAt":"2013-05-30T12:35:53Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, May 30, 2013 at 7:17 AM, Thomas Rast wrote:\n> Felipe Contreras writes:\n>\n>> We are supposedly adding files, to to which cache if 'the_index' is\n>> discarded?\n> [...]\n>> if (!current_head) {\n>> discard_cache();\n>> + if (read_cache() < 0)\n>> + die(_(\"cannot read the index\"));\n>> return;\n>> }\n>\n> It is not obvious to me that this is a correct change. discard_cache()\n> without subsequent reloading could also legitimately be used to empty\n> the index. So if you are fixing a bug, please justify the change and\n> provide a testcase to guard against it in the future.\n\nSo istate->initialized is false, yet somebody can still add entries to\nthe cache? What happens when somebody else tries to initialize this\ncache? All the entries there will be lost, even though nobody\ndiscarded it afterwards.\n\n-- \nFelipe Contreras\n"},{"id":"218989","messageId":"8761y0k4ja.fsf@linux-k42r.v.cablecom.net","threadId":"33986","inReplyTo":"CAMP44s1O=VTu8EZi+yOfGMccCpS+pozvZJuDW1mK95U8-YEquA@mail.gmail.com","subject":"Re: [PATCH 1/4] commit: reload cache properly","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-05-30T12:58:49Z","receivedAt":"2013-05-30T12:58:49Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Felipe Contreras writes:\n\n> On Thu, May 30, 2013 at 7:17 AM, Thomas Rast wrote:\n>> Felipe Contreras writes:\n>>\n>>> We are supposedly adding files, to to which cache if 'the_index' is\n>>> discarded?\n>> [...]\n>>> if (!current_head) {\n>>> discard_cache();\n>>> + if (read_cache() < 0)\n>>> + die(_(\"cannot read the index\"));\n>>> return;\n>>> }\n>>\n>> It is not obvious to me that this is a correct change. discard_cache()\n>> without subsequent reloading could also legitimately be used to empty\n>> the index. So if you are fixing a bug, please justify the change and\n>> provide a testcase to guard against it in the future.\n>\n> So istate->initialized is false, yet somebody can still add entries to\n> the cache? What happens when somebody else tries to initialize this\n> cache? All the entries there will be lost, even though nobody\n> discarded it afterwards.\n\nAnd yet it works, and your patch breaks it.\n\ndiff --git i/t/t7501-commit.sh w/t/t7501-commit.sh\nindex 195e747..1608254 100755\n--- i/t/t7501-commit.sh\n+++ w/t/t7501-commit.sh\n@@ -524,4 +524,16 @@ test_expect_success 'commit a file whose name is a dash' '\n \ttest_i18ngrep \" changed, 5 insertions\" output\n '\n \n+test_expect_success '--only works on to-be-born branch' '\n+\tgit checkout --orphan orphan &&\n+\techo foo >newfile &&\n+\tgit add newfile &&\n+\tgit commit --only newfile -m\"--only on unborn branch\" &&\n+\tcat >expected <actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"218990","messageId":"CAMP44s2_q6-bfxw-3jTQA_aiXynGc7adE_-GebP=D5FY-ou_wg@mail.gmail.com","threadId":"33986","inReplyTo":"8761y0k4ja.fsf@linux-k42r.v.cablecom.net","subject":"Re: [PATCH 1/4] commit: reload cache properly","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-30T13:07:57Z","receivedAt":"2013-05-30T13:07:57Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, May 30, 2013 at 7:58 AM, Thomas Rast wrote:\n> Felipe Contreras writes:\n>\n>> On Thu, May 30, 2013 at 7:17 AM, Thomas Rast wrote:\n>>> Felipe Contreras writes:\n>>>\n>>>> We are supposedly adding files, to to which cache if 'the_index' is\n>>>> discarded?\n>>> [...]\n>>>> if (!current_head) {\n>>>> discard_cache();\n>>>> + if (read_cache() < 0)\n>>>> + die(_(\"cannot read the index\"));\n>>>> return;\n>>>> }\n>>>\n>>> It is not obvious to me that this is a correct change. discard_cache()\n>>> without subsequent reloading could also legitimately be used to empty\n>>> the index. So if you are fixing a bug, please justify the change and\n>>> provide a testcase to guard against it in the future.\n>>\n>> So istate->initialized is false, yet somebody can still add entries to\n>> the cache? What happens when somebody else tries to initialize this\n>> cache? All the entries there will be lost, even though nobody\n>> discarded it afterwards.\n>\n> And yet it works, and your patch breaks it.\n\nIt might work, but the API doesn't make any sense.\n\n-- \nFelipe Contreras\n"},{"id":"218991","messageId":"CACsJy8BuSaY=H9Pb6FuTEiQkm18h-kW86ZOVtaaDpMkjFEnqcQ@mail.gmail.com","threadId":"33986","inReplyTo":"87ehcok6gl.fsf@linux-k42r.v.cablecom.net","subject":"Re: [PATCH 1/4] commit: reload cache properly","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-05-30T13:08:01Z","receivedAt":"2013-05-30T13:08:01Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, May 30, 2013 at 7:17 PM, Thomas Rast wrote:\n> Felipe Contreras writes:\n>\n>> We are supposedly adding files, to to which cache if 'the_index' is\n>> discarded?\n> [...]\n>> if (!current_head) {\n>> discard_cache();\n>> + if (read_cache() < 0)\n>> + die(_(\"cannot read the index\"));\n>> return;\n>> }\n>\n> It is not obvious to me that this is a correct change. discard_cache()\n> without subsequent reloading could also legitimately be used to empty\n> the index. So if you are fixing a bug, please justify the change and\n> provide a testcase to guard against it in the future.\n\nThat discard_cache line I think came from fa9dcf8 (Fix performance\nregression for partial commits - 2008-01-13). The code flow back then\nwas\n\nif (initial_commit)\n discard_cache();\n\nadd_remove_files();\n/* do something more */\n\nA quick look from current code seems to indicate this pattern is still\nvalid for creating partial commits, where temporary index will be\nthrown away afterwards. But I may be wrong.\n--\nDuy\n"},{"id":"219115","messageId":"CACsJy8A6Hq_LEqAxUTLKP9-TA6aTq+91Wx2vYLhSuMSmRvpYiA@mail.gmail.com","threadId":"33986","inReplyTo":"8761y0k4ja.fsf@linux-k42r.v.cablecom.net","subject":"Re: [PATCH 1/4] commit: reload cache properly","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-06-01T09:09:39Z","receivedAt":"2013-06-01T09:09:39Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, May 30, 2013 at 7:58 PM, Thomas Rast wrote:\n> Felipe Contreras writes:\n>\n>> On Thu, May 30, 2013 at 7:17 AM, Thomas Rast wrote:\n> diff --git i/t/t7501-commit.sh w/t/t7501-commit.sh\n> index 195e747..1608254 100755\n> --- i/t/t7501-commit.sh\n> +++ w/t/t7501-commit.sh\n> @@ -524,4 +524,16 @@ test_expect_success 'commit a file whose name is a dash' '\n> test_i18ngrep \" changed, 5 insertions\" output\n> '\n>\n> +test_expect_success '--only works on to-be-born branch' '\n> + git checkout --orphan orphan &&\n> + echo foo >newfile &&\n> + git add newfile &&\n> + git commit --only newfile -m\"--only on unborn branch\" &&\n> + cat >expected < +100644 blob 257cc5642cb1a054f08cc83f2d943e56fd3ebe99 newfile\n> +EOF\n> + git ls-tree -r HEAD >actual &&\n> + test_cmp expected actual\n> +'\n> +\n> test_done\n\nThomas, can you resubmit this as a patch to Junio? It's good that the\ntest suite covers all correct behaviors (and the incorrect ones).\n--\nDuy\n"},{"id":"219130","messageId":"78176f4fca45213961f0e4d15ac72012d6a0a10b.1370084247.git.trast@inf.ethz.ch","threadId":"33986","inReplyTo":"CACsJy8A6Hq_LEqAxUTLKP9-TA6aTq+91Wx2vYLhSuMSmRvpYiA@mail.gmail.com","subject":"[PATCH] Test 'commit --only' after 'checkout --orphan'","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-06-01T11:02:00Z","receivedAt":"2013-06-01T11:02:00Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"There are some index handling subtleties in 'commit --only' that are\nbest tested when we have an existing index, but an unborn or empty\nHEAD. These circumstances are easily produced by 'checkout --orphan',\nbut we did not previously have a test for it.\n\nThe main expected failure mode would be: erroneously loading the\nexisting index contents when building the temporary index that is used\nfor --only. Cf.\n\n http://article.gmane.org/gmane.comp.version-control.git/225969\n\nand subsequent discussion.\n\nSigned-off-by: Thomas Rast \n---\n t/t7501-commit.sh | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\nindex 195e747..99ce36f 100755\n--- a/t/t7501-commit.sh\n+++ b/t/t7501-commit.sh\n@@ -524,4 +524,17 @@ test_expect_success 'commit a file whose name is a dash' '\n \ttest_i18ngrep \" changed, 5 insertions\" output\n '\n \n+test_expect_success '--only works on to-be-born branch' '\n+\t# This test relies on having something in the index, as it\n+\t# would not otherwise actually prove much. So check this.\n+\ttest -n \"$(git ls-files)\" &&\n+\tgit checkout --orphan orphan &&\n+\techo foo >newfile &&\n+\tgit add newfile &&\n+\tgit commit --only newfile -m\"--only on unborn branch\" &&\n+\techo newfile >expected &&\n+\tgit ls-tree -r --name-only HEAD >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n1.8.3.509.g0de0faa\n"},{"id":"219185","messageId":"7vfvx0nw7t.fsf@alter.siamese.dyndns.org","threadId":"33986","inReplyTo":"1369915136-4248-4-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 3/4] unpack-trees: plug a memory leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-02T19:33:58Z","receivedAt":"2013-06-02T19:33:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> Before overwriting the destination index, first let's discard it's\n> contents.\n>\n> Signed-off-by: Felipe Contreras \n> ---\n> unpack-trees.c | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index ede4299..eff2944 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -1146,8 +1146,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n> \n> \to->src_index = NULL;\n> \tret = check_updates(o) ? (-2) : 0;\n> -\tif (o->dst_index)\n> +\tif (o->dst_index) {\n> +\t\tdiscard_index(o->dst_index);\n> \t\t*o->dst_index = o->result;\n> +\t}\n\nI seem to recall that many callers set src_index and dst_index to\nthe same istate, and expect that the original istate pointed by the\nsrc_index to remain usable. Is it safe to discard it like this at\nthis point?\n\n> \n> done:\n> \tclear_exclude_list(&el);\n"},{"id":"219187","messageId":"CAMP44s3aF8VsNqJA3GNLN=cw6Vp4iJEcTn5WDq2XPs-Le--szg@mail.gmail.com","threadId":"33986","inReplyTo":"7vfvx0nw7t.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/4] unpack-trees: plug a memory leak","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-02T19:51:27Z","receivedAt":"2013-06-02T19:51:27Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Jun 2, 2013 at 2:33 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> Before overwriting the destination index, first let's discard it's\n>> contents.\n>>\n>> Signed-off-by: Felipe Contreras \n>> ---\n>> unpack-trees.c | 4 +++-\n>> 1 file changed, 3 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/unpack-trees.c b/unpack-trees.c\n>> index ede4299..eff2944 100644\n>> --- a/unpack-trees.c\n>> +++ b/unpack-trees.c\n>> @@ -1146,8 +1146,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n>>\n>> o->src_index = NULL;\n>> ret = check_updates(o) ? (-2) : 0;\n>> - if (o->dst_index)\n>> + if (o->dst_index) {\n>> + discard_index(o->dst_index);\n>> *o->dst_index = o->result;\n>> + }\n>\n> I seem to recall that many callers set src_index and dst_index to\n> the same istate, and expect that the original istate pointed by the\n> src_index to remain usable. Is it safe to discard it like this at\n> this point?\n\nWho expects that?\n\n-- \nFelipe Contreras\n"},{"id":"219195","messageId":"7vehckma34.fsf@alter.siamese.dyndns.org","threadId":"33986","inReplyTo":"CAMP44s3aF8VsNqJA3GNLN=cw6Vp4iJEcTn5WDq2XPs-Le--szg@mail.gmail.com","subject":"Re: [PATCH 3/4] unpack-trees: plug a memory leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-02T22:17:19Z","receivedAt":"2013-06-02T22:17:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> On Sun, Jun 2, 2013 at 2:33 PM, Junio C Hamano wrote:\n>> Felipe Contreras writes:\n>>\n>>> Before overwriting the destination index, first let's discard it's\n>>> contents.\n>>>\n>>> Signed-off-by: Felipe Contreras \n>>> ---\n>>> unpack-trees.c | 4 +++-\n>>> 1 file changed, 3 insertions(+), 1 deletion(-)\n>>>\n>>> diff --git a/unpack-trees.c b/unpack-trees.c\n>>> index ede4299..eff2944 100644\n>>> --- a/unpack-trees.c\n>>> +++ b/unpack-trees.c\n>>> @@ -1146,8 +1146,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n>>>\n>>> o->src_index = NULL;\n>>> ret = check_updates(o) ? (-2) : 0;\n>>> - if (o->dst_index)\n>>> + if (o->dst_index) {\n>>> + discard_index(o->dst_index);\n>>> *o->dst_index = o->result;\n>>> + }\n>>\n>> I seem to recall that many callers set src_index and dst_index to\n>> the same istate, and expect that the original istate pointed by the\n>> src_index to remain usable. Is it safe to discard it like this at\n>> this point?\n>\n> Who expects that?\n\nThe patch you posted expects that no such caller depends on\nsrc_index being left alone by the call, and I was asking if that\nexpectantion holds, i.e. if it is safe to discard.\n\nI think your answer can be one of \"Yes, it is safe, as no current\ncaller does so\", \"I dunno, I did not check\", or \"No, this and that\ncaller need to be adjusted\".\n"},{"id":"219199","messageId":"CAMP44s2WSmu-JfKFPpNyQzDMfULMy9hCVb9Tu_Qd_bcKotOYjQ@mail.gmail.com","threadId":"33986","inReplyTo":"7vehckma34.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/4] unpack-trees: plug a memory leak","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-02T22:45:06Z","receivedAt":"2013-06-02T22:45:06Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Jun 2, 2013 at 5:17 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> On Sun, Jun 2, 2013 at 2:33 PM, Junio C Hamano wrote:\n>>> Felipe Contreras writes:\n>>>\n>>>> Before overwriting the destination index, first let's discard it's\n>>>> contents.\n>>>>\n>>>> Signed-off-by: Felipe Contreras \n>>>> ---\n>>>> unpack-trees.c | 4 +++-\n>>>> 1 file changed, 3 insertions(+), 1 deletion(-)\n>>>>\n>>>> diff --git a/unpack-trees.c b/unpack-trees.c\n>>>> index ede4299..eff2944 100644\n>>>> --- a/unpack-trees.c\n>>>> +++ b/unpack-trees.c\n>>>> @@ -1146,8 +1146,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n>>>>\n>>>> o->src_index = NULL;\n>>>> ret = check_updates(o) ? (-2) : 0;\n>>>> - if (o->dst_index)\n>>>> + if (o->dst_index) {\n>>>> + discard_index(o->dst_index);\n>>>> *o->dst_index = o->result;\n>>>> + }\n>>>\n>>> I seem to recall that many callers set src_index and dst_index to\n>>> the same istate, and expect that the original istate pointed by the\n>>> src_index to remain usable. Is it safe to discard it like this at\n>>> this point?\n>>\n>> Who expects that?\n>\n> The patch you posted expects that no such caller depends on\n> src_index being left alone by the call, and I was asking if that\n> expectantion holds, i.e. if it is safe to discard.\n\nNo, it expects that no caller depends on dst_index being left alone.\n\n> I think your answer can be one of \"Yes, it is safe, as no current\n> caller does so\", \"I dunno, I did not check\", or \"No, this and that\n> caller need to be adjusted\".\n\nIf what you say is true, it would not be safe, but AFAIK what you said\nis not true, so it is safe. I wouldn't have sent the patch otherwise.\n\n-- \nFelipe Contreras\n"}]}