threads / patch / 33986
Subject: [PATCH 0/4] cherry-pick: fix memory leaks
Felipe ContrerasHi,
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[PATCH 1/4] commit: reload cache properly
We are supposedly adding files, to to which cache if 'the_index' is discarded?
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- 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
Thomas RastRe: [PATCH 1/4] commit: reload cache properly
Felipe Contreras <felipe.contreras@gmail.com> 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.chFelipe ContrerasRe: [PATCH 1/4] commit: reload cache properly
On Thu, May 30, 2013 at 7:17 AM, Thomas Rast <trast@inf.ethz.ch> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> 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 RastRe: [PATCH 1/4] commit: reload cache properly
Felipe Contreras <felipe.contreras@gmail.com> writes:
> On Thu, May 30, 2013 at 7:17 AM, Thomas Rast <trast@inf.ethz.ch> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> 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 <<EOF && +100644 blob 257cc5642cb1a054f08cc83f2d943e56fd3ebe99 newfile +EOF + git ls-tree -r HEAD >actual && + test_cmp expected actual +' + test_done
--
Thomas Rast
trast@{inf,student}.ethz.chFelipe ContrerasRe: [PATCH 1/4] commit: reload cache properly
On Thu, May 30, 2013 at 7:58 AM, Thomas Rast <trast@inf.ethz.ch> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Thu, May 30, 2013 at 7:17 AM, Thomas Rast <trast@inf.ethz.ch> wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> 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 NguyenRe: [PATCH 1/4] commit: reload cache properly
On Thu, May 30, 2013 at 7:58 PM, Thomas Rast <trast@inf.ethz.ch> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes: > >> On Thu, May 30, 2013 at 7:17 AM, Thomas Rast <trast@inf.ethz.ch> 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 <<EOF && > +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[PATCH] Test 'commit --only' after 'checkout --orphan'
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 <trast@inf.ethz.ch> --- 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
Duy NguyenRe: [PATCH 1/4] commit: reload cache properly
On Thu, May 30, 2013 at 7:17 PM, Thomas Rast <trast@inf.ethz.ch> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> 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
Felipe Contreras[PATCH 2/4] read-cache: plug small memory leak
We free each entry, but not the array of entries themselves.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- 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[PATCH 3/4] unpack-trees: plug a memory leak
Before overwriting the destination index, first let's discard it's contents.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- 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
Junio C HamanoRe: [PATCH 3/4] unpack-trees: plug a memory leak
Felipe Contreras <felipe.contreras@gmail.com> writes:
> Before overwriting the destination index, first let's discard it's
> contents.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
> 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 ContrerasRe: [PATCH 3/4] unpack-trees: plug a memory leak
On Sun, Jun 2, 2013 at 2:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Before overwriting the destination index, first let's discard it's
>> contents.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>> 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 HamanoRe: [PATCH 3/4] unpack-trees: plug a memory leak
Felipe Contreras <felipe.contreras@gmail.com> writes:
> On Sun, Jun 2, 2013 at 2:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> Before overwriting the destination index, first let's discard it's
>>> contents.
>>>
>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>> ---
>>> 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 ContrerasRe: [PATCH 3/4] unpack-trees: plug a memory leak
On Sun, Jun 2, 2013 at 5:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Sun, Jun 2, 2013 at 2:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>>> Before overwriting the destination index, first let's discard it's
>>>> contents.
>>>>
>>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>>> ---
>>>> 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
Felipe Contreras[PATCH 4/4] unpack-trees: free created cache entries
We created them, and nobody else is going to destroy them.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- 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