# [PATCH 0/4] cherry-pick: fix memory leaks 16 messages from 2013-05-30 to 2013-06-02. Participants: Felipe Contreras, Thomas Rast, Duy Nguyen, Junio C Hamano. Thread: https://gitlist.dev/t/33986 ## Felipe Contreras, 2013-05-30 11:58 Subject: [PATCH 0/4] cherry-pick: fix memory leaks Message-ID: <1369915136-4248-1-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1369915136-4248-1-git-send-email-felipe.contreras%40gmail.com ``` Hi, I took a shot at fixing the memory leaks of cherry-pick, and at least in my tests the memory doesn't seem to increase any more. Felipe Contreras (4): commit: reload cache properly read-cache: plug small memory leak unpack-trees: plug a memory leak unpack-trees: free created cache entries builtin/commit.c | 2 ++ read-cache.c | 2 ++ unpack-trees.c | 16 +++++++++++++--- 3 files changed, 17 insertions(+), 3 deletions(-) -- 1.8.3.rc3.312.g47657de ``` ## Felipe Contreras, 2013-05-30 11:58 Subject: [PATCH 1/4] commit: reload cache properly Message-ID: <1369915136-4248-2-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1369915136-4248-2-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1369915136-4248-1-git-send-email-felipe.contreras@gmail.com> ``` We are supposedly adding files, to to which cache if 'the_index' is discarded? Signed-off-by: Felipe Contreras --- builtin/commit.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/builtin/commit.c b/builtin/commit.c index d2f30d9..092b49e 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -244,6 +244,8 @@ static void create_base_index(const struct commit *current_head) if (!current_head) { discard_cache(); + if (read_cache() < 0) + die(_("cannot read the index")); return; } -- 1.8.3.rc3.312.g47657de ``` ## Felipe Contreras, 2013-05-30 11:58 Subject: [PATCH 2/4] read-cache: plug small memory leak Message-ID: <1369915136-4248-3-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1369915136-4248-3-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1369915136-4248-1-git-send-email-felipe.contreras@gmail.com> ``` We free each entry, but not the array of entries themselves. Signed-off-by: Felipe Contreras --- read-cache.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/read-cache.c b/read-cache.c index 04ed561..9d9b886 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1510,6 +1510,8 @@ int discard_index(struct index_state *istate) for (i = 0; i < istate->cache_nr; i++) free(istate->cache[i]); + free(istate->cache); + istate->cache = NULL; resolve_undo_clear_index(istate); istate->cache_nr = 0; istate->cache_changed = 0; -- 1.8.3.rc3.312.g47657de ``` ## Felipe Contreras, 2013-05-30 11:58 Subject: [PATCH 3/4] unpack-trees: plug a memory leak Message-ID: <1369915136-4248-4-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1369915136-4248-4-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1369915136-4248-1-git-send-email-felipe.contreras@gmail.com> ``` Before overwriting the destination index, first let's discard it's contents. Signed-off-by: Felipe Contreras --- unpack-trees.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/unpack-trees.c b/unpack-trees.c index ede4299..eff2944 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -1146,8 +1146,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options o->src_index = NULL; ret = check_updates(o) ? (-2) : 0; - if (o->dst_index) + if (o->dst_index) { + discard_index(o->dst_index); *o->dst_index = o->result; + } done: clear_exclude_list(&el); -- 1.8.3.rc3.312.g47657de ``` ## Felipe Contreras, 2013-05-30 11:58 Subject: [PATCH 4/4] unpack-trees: free created cache entries Message-ID: <1369915136-4248-5-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1369915136-4248-5-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1369915136-4248-1-git-send-email-felipe.contreras@gmail.com> ``` We created them, and nobody else is going to destroy them. Signed-off-by: Felipe Contreras --- unpack-trees.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/unpack-trees.c b/unpack-trees.c index eff2944..9f19d01 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -590,8 +590,16 @@ static int unpack_nondirectories(int n, unsigned long mask, src[i + o->merge] = create_ce_entry(info, names + i, stage); } - if (o->merge) - return call_unpack_fn(src, o); + if (o->merge) { + int ret = call_unpack_fn(src, o); + for (i = 0; i < n; i++) { + struct cache_entry *ce = src[i + o->merge]; + if (!ce || ce == o->df_conflict_entry) + continue; + free(ce); + } + return ret; + } for (i = 0; i < n; i++) if (src[i] && src[i] != o->df_conflict_entry) -- 1.8.3.rc3.312.g47657de ``` ## Thomas Rast, 2013-05-30 12:17 Subject: Re: [PATCH 1/4] commit: reload cache properly Message-ID: <87ehcok6gl.fsf@linux-k42r.v.cablecom.net> URL: https://gitlist.dev/e/87ehcok6gl.fsf%40linux-k42r.v.cablecom.net In-Reply-To: <1369915136-4248-2-git-send-email-felipe.contreras@gmail.com> ``` Felipe Contreras writes: > We are supposedly adding files, to to which cache if 'the_index' is > discarded? [...] > if (!current_head) { > discard_cache(); > + if (read_cache() < 0) > + die(_("cannot read the index")); > return; > } It is not obvious to me that this is a correct change. discard_cache() without subsequent reloading could also legitimately be used to empty the index. So if you are fixing a bug, please justify the change and provide a testcase to guard against it in the future. -- Thomas Rast trast@{inf,student}.ethz.ch ``` ## Felipe Contreras, 2013-05-30 12:35 Subject: Re: [PATCH 1/4] commit: reload cache properly Message-ID: URL: https://gitlist.dev/e/CAMP44s1O%3DVTu8EZi%2ByOfGMccCpS%2BpozvZJuDW1mK95U8-YEquA%40mail.gmail.com In-Reply-To: <87ehcok6gl.fsf@linux-k42r.v.cablecom.net> ``` On Thu, May 30, 2013 at 7:17 AM, Thomas Rast wrote: > Felipe Contreras writes: > >> We are supposedly adding files, to to which cache if 'the_index' is >> discarded? > [...] >> if (!current_head) { >> discard_cache(); >> + if (read_cache() < 0) >> + die(_("cannot read the index")); >> return; >> } > > It is not obvious to me that this is a correct change. discard_cache() > without subsequent reloading could also legitimately be used to empty > the index. So if you are fixing a bug, please justify the change and > provide a testcase to guard against it in the future. So istate->initialized is false, yet somebody can still add entries to the cache? What happens when somebody else tries to initialize this cache? All the entries there will be lost, even though nobody discarded it afterwards. -- Felipe Contreras ``` ## Thomas Rast, 2013-05-30 12:58 Subject: Re: [PATCH 1/4] commit: reload cache properly Message-ID: <8761y0k4ja.fsf@linux-k42r.v.cablecom.net> URL: https://gitlist.dev/e/8761y0k4ja.fsf%40linux-k42r.v.cablecom.net In-Reply-To: ``` Felipe Contreras writes: > On Thu, May 30, 2013 at 7:17 AM, Thomas Rast wrote: >> Felipe Contreras writes: >> >>> We are supposedly adding files, to to which cache if 'the_index' is >>> discarded? >> [...] >>> if (!current_head) { >>> discard_cache(); >>> + if (read_cache() < 0) >>> + die(_("cannot read the index")); >>> return; >>> } >> >> It is not obvious to me that this is a correct change. discard_cache() >> without subsequent reloading could also legitimately be used to empty >> the index. So if you are fixing a bug, please justify the change and >> provide a testcase to guard against it in the future. > > So istate->initialized is false, yet somebody can still add entries to > the cache? What happens when somebody else tries to initialize this > cache? All the entries there will be lost, even though nobody > discarded it afterwards. And yet it works, and your patch breaks it. diff --git i/t/t7501-commit.sh w/t/t7501-commit.sh index 195e747..1608254 100755 --- i/t/t7501-commit.sh +++ w/t/t7501-commit.sh @@ -524,4 +524,16 @@ test_expect_success 'commit a file whose name is a dash' ' test_i18ngrep " changed, 5 insertions" output ' +test_expect_success '--only works on to-be-born branch' ' + git checkout --orphan orphan && + echo foo >newfile && + git add newfile && + git commit --only newfile -m"--only on unborn branch" && + cat >expected <actual && + test_cmp expected actual +' + test_done -- Thomas Rast trast@{inf,student}.ethz.ch ``` ## Felipe Contreras, 2013-05-30 13:07 Subject: Re: [PATCH 1/4] commit: reload cache properly Message-ID: URL: https://gitlist.dev/e/CAMP44s2_q6-bfxw-3jTQA_aiXynGc7adE_-GebP%3DD5FY-ou_wg%40mail.gmail.com In-Reply-To: <8761y0k4ja.fsf@linux-k42r.v.cablecom.net> ``` On Thu, May 30, 2013 at 7:58 AM, Thomas Rast wrote: > Felipe Contreras writes: > >> On Thu, May 30, 2013 at 7:17 AM, Thomas Rast wrote: >>> Felipe Contreras writes: >>> >>>> We are supposedly adding files, to to which cache if 'the_index' is >>>> discarded? >>> [...] >>>> if (!current_head) { >>>> discard_cache(); >>>> + if (read_cache() < 0) >>>> + die(_("cannot read the index")); >>>> return; >>>> } >>> >>> It is not obvious to me that this is a correct change. discard_cache() >>> without subsequent reloading could also legitimately be used to empty >>> the index. So if you are fixing a bug, please justify the change and >>> provide a testcase to guard against it in the future. >> >> So istate->initialized is false, yet somebody can still add entries to >> the cache? What happens when somebody else tries to initialize this >> cache? All the entries there will be lost, even though nobody >> discarded it afterwards. > > And yet it works, and your patch breaks it. It might work, but the API doesn't make any sense. -- Felipe Contreras ``` ## Duy Nguyen, 2013-05-30 13:08 Subject: Re: [PATCH 1/4] commit: reload cache properly Message-ID: URL: https://gitlist.dev/e/CACsJy8BuSaY%3DH9Pb6FuTEiQkm18h-kW86ZOVtaaDpMkjFEnqcQ%40mail.gmail.com In-Reply-To: <87ehcok6gl.fsf@linux-k42r.v.cablecom.net> ``` On Thu, May 30, 2013 at 7:17 PM, Thomas Rast wrote: > Felipe Contreras writes: > >> We are supposedly adding files, to to which cache if 'the_index' is >> discarded? > [...] >> if (!current_head) { >> discard_cache(); >> + if (read_cache() < 0) >> + die(_("cannot read the index")); >> return; >> } > > It is not obvious to me that this is a correct change. discard_cache() > without subsequent reloading could also legitimately be used to empty > the index. So if you are fixing a bug, please justify the change and > provide a testcase to guard against it in the future. That discard_cache line I think came from fa9dcf8 (Fix performance regression for partial commits - 2008-01-13). The code flow back then was if (initial_commit) discard_cache(); add_remove_files(); /* do something more */ A quick look from current code seems to indicate this pattern is still valid for creating partial commits, where temporary index will be thrown away afterwards. But I may be wrong. -- Duy ``` ## Duy Nguyen, 2013-06-01 09:09 Subject: Re: [PATCH 1/4] commit: reload cache properly Message-ID: URL: https://gitlist.dev/e/CACsJy8A6Hq_LEqAxUTLKP9-TA6aTq%2B91Wx2vYLhSuMSmRvpYiA%40mail.gmail.com In-Reply-To: <8761y0k4ja.fsf@linux-k42r.v.cablecom.net> ``` On Thu, May 30, 2013 at 7:58 PM, Thomas Rast wrote: > Felipe Contreras writes: > >> On Thu, May 30, 2013 at 7:17 AM, Thomas Rast wrote: > diff --git i/t/t7501-commit.sh w/t/t7501-commit.sh > index 195e747..1608254 100755 > --- i/t/t7501-commit.sh > +++ w/t/t7501-commit.sh > @@ -524,4 +524,16 @@ test_expect_success 'commit a file whose name is a dash' ' > test_i18ngrep " changed, 5 insertions" output > ' > > +test_expect_success '--only works on to-be-born branch' ' > + git checkout --orphan orphan && > + echo foo >newfile && > + git add newfile && > + git commit --only newfile -m"--only on unborn branch" && > + cat >expected < +100644 blob 257cc5642cb1a054f08cc83f2d943e56fd3ebe99 newfile > +EOF > + git ls-tree -r HEAD >actual && > + test_cmp expected actual > +' > + > test_done Thomas, can you resubmit this as a patch to Junio? It's good that the test suite covers all correct behaviors (and the incorrect ones). -- Duy ``` ## Thomas Rast, 2013-06-01 11:02 Subject: [PATCH] Test 'commit --only' after 'checkout --orphan' Message-ID: <78176f4fca45213961f0e4d15ac72012d6a0a10b.1370084247.git.trast@inf.ethz.ch> URL: https://gitlist.dev/e/78176f4fca45213961f0e4d15ac72012d6a0a10b.1370084247.git.trast%40inf.ethz.ch In-Reply-To: ``` There are some index handling subtleties in 'commit --only' that are best tested when we have an existing index, but an unborn or empty HEAD. These circumstances are easily produced by 'checkout --orphan', but we did not previously have a test for it. The main expected failure mode would be: erroneously loading the existing index contents when building the temporary index that is used for --only. Cf. http://article.gmane.org/gmane.comp.version-control.git/225969 and subsequent discussion. Signed-off-by: Thomas Rast --- t/t7501-commit.sh | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh index 195e747..99ce36f 100755 --- a/t/t7501-commit.sh +++ b/t/t7501-commit.sh @@ -524,4 +524,17 @@ test_expect_success 'commit a file whose name is a dash' ' test_i18ngrep " changed, 5 insertions" output ' +test_expect_success '--only works on to-be-born branch' ' + # This test relies on having something in the index, as it + # would not otherwise actually prove much. So check this. + test -n "$(git ls-files)" && + git checkout --orphan orphan && + echo foo >newfile && + git add newfile && + git commit --only newfile -m"--only on unborn branch" && + echo newfile >expected && + git ls-tree -r --name-only HEAD >actual && + test_cmp expected actual +' + test_done -- 1.8.3.509.g0de0faa ``` ## Junio C Hamano, 2013-06-02 19:33 Subject: Re: [PATCH 3/4] unpack-trees: plug a memory leak Message-ID: <7vfvx0nw7t.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vfvx0nw7t.fsf%40alter.siamese.dyndns.org In-Reply-To: <1369915136-4248-4-git-send-email-felipe.contreras@gmail.com> ``` Felipe Contreras writes: > Before overwriting the destination index, first let's discard it's > contents. > > Signed-off-by: Felipe Contreras > --- > unpack-trees.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/unpack-trees.c b/unpack-trees.c > index ede4299..eff2944 100644 > --- a/unpack-trees.c > +++ b/unpack-trees.c > @@ -1146,8 +1146,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options > > o->src_index = NULL; > ret = check_updates(o) ? (-2) : 0; > - if (o->dst_index) > + if (o->dst_index) { > + discard_index(o->dst_index); > *o->dst_index = o->result; > + } I seem to recall that many callers set src_index and dst_index to the same istate, and expect that the original istate pointed by the src_index to remain usable. Is it safe to discard it like this at this point? > > done: > clear_exclude_list(&el); ``` ## Felipe Contreras, 2013-06-02 19:51 Subject: Re: [PATCH 3/4] unpack-trees: plug a memory leak Message-ID: URL: https://gitlist.dev/e/CAMP44s3aF8VsNqJA3GNLN%3Dcw6Vp4iJEcTn5WDq2XPs-Le--szg%40mail.gmail.com In-Reply-To: <7vfvx0nw7t.fsf@alter.siamese.dyndns.org> ``` On Sun, Jun 2, 2013 at 2:33 PM, Junio C Hamano wrote: > Felipe Contreras writes: > >> Before overwriting the destination index, first let's discard it's >> contents. >> >> Signed-off-by: Felipe Contreras >> --- >> unpack-trees.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/unpack-trees.c b/unpack-trees.c >> index ede4299..eff2944 100644 >> --- a/unpack-trees.c >> +++ b/unpack-trees.c >> @@ -1146,8 +1146,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options >> >> o->src_index = NULL; >> ret = check_updates(o) ? (-2) : 0; >> - if (o->dst_index) >> + if (o->dst_index) { >> + discard_index(o->dst_index); >> *o->dst_index = o->result; >> + } > > I seem to recall that many callers set src_index and dst_index to > the same istate, and expect that the original istate pointed by the > src_index to remain usable. Is it safe to discard it like this at > this point? Who expects that? -- Felipe Contreras ``` ## Junio C Hamano, 2013-06-02 22:17 Subject: Re: [PATCH 3/4] unpack-trees: plug a memory leak Message-ID: <7vehckma34.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vehckma34.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Felipe Contreras writes: > On Sun, Jun 2, 2013 at 2:33 PM, Junio C Hamano wrote: >> Felipe Contreras writes: >> >>> Before overwriting the destination index, first let's discard it's >>> contents. >>> >>> Signed-off-by: Felipe Contreras >>> --- >>> unpack-trees.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/unpack-trees.c b/unpack-trees.c >>> index ede4299..eff2944 100644 >>> --- a/unpack-trees.c >>> +++ b/unpack-trees.c >>> @@ -1146,8 +1146,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options >>> >>> o->src_index = NULL; >>> ret = check_updates(o) ? (-2) : 0; >>> - if (o->dst_index) >>> + if (o->dst_index) { >>> + discard_index(o->dst_index); >>> *o->dst_index = o->result; >>> + } >> >> I seem to recall that many callers set src_index and dst_index to >> the same istate, and expect that the original istate pointed by the >> src_index to remain usable. Is it safe to discard it like this at >> this point? > > Who expects that? The patch you posted expects that no such caller depends on src_index being left alone by the call, and I was asking if that expectantion holds, i.e. if it is safe to discard. I think your answer can be one of "Yes, it is safe, as no current caller does so", "I dunno, I did not check", or "No, this and that caller need to be adjusted". ``` ## Felipe Contreras, 2013-06-02 22:45 Subject: Re: [PATCH 3/4] unpack-trees: plug a memory leak Message-ID: URL: https://gitlist.dev/e/CAMP44s2WSmu-JfKFPpNyQzDMfULMy9hCVb9Tu_Qd_bcKotOYjQ%40mail.gmail.com In-Reply-To: <7vehckma34.fsf@alter.siamese.dyndns.org> ``` On Sun, Jun 2, 2013 at 5:17 PM, Junio C Hamano wrote: > Felipe Contreras writes: > >> On Sun, Jun 2, 2013 at 2:33 PM, Junio C Hamano wrote: >>> Felipe Contreras writes: >>> >>>> Before overwriting the destination index, first let's discard it's >>>> contents. >>>> >>>> Signed-off-by: Felipe Contreras >>>> --- >>>> unpack-trees.c | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/unpack-trees.c b/unpack-trees.c >>>> index ede4299..eff2944 100644 >>>> --- a/unpack-trees.c >>>> +++ b/unpack-trees.c >>>> @@ -1146,8 +1146,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options >>>> >>>> o->src_index = NULL; >>>> ret = check_updates(o) ? (-2) : 0; >>>> - if (o->dst_index) >>>> + if (o->dst_index) { >>>> + discard_index(o->dst_index); >>>> *o->dst_index = o->result; >>>> + } >>> >>> I seem to recall that many callers set src_index and dst_index to >>> the same istate, and expect that the original istate pointed by the >>> src_index to remain usable. Is it safe to discard it like this at >>> this point? >> >> Who expects that? > > The patch you posted expects that no such caller depends on > src_index being left alone by the call, and I was asking if that > expectantion holds, i.e. if it is safe to discard. No, it expects that no caller depends on dst_index being left alone. > I think your answer can be one of "Yes, it is safe, as no current > caller does so", "I dunno, I did not check", or "No, this and that > caller need to be adjusted". If what you say is true, it would not be safe, but AFAIK what you said is not true, so it is safe. I wouldn't have sent the patch otherwise. -- Felipe Contreras ```