[PATCH 0/4] cherry-pick: fix memory leaks

[PATCH 0/4] cherry-pick: fix memory leaks

[Felipe Contreras]

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

[PATCH 1/4] commit: reload cache properly

[Felipe Contreras]

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

Re: [PATCH 1/4] commit: reload cache properly

[Thomas Rast]

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.ch

Re: [PATCH 1/4] commit: reload cache properly

[Felipe Contreras]

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

Re: [PATCH 1/4] commit: reload cache properly

[Thomas Rast]

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.ch

Re: [PATCH 1/4] commit: reload cache properly

[Felipe Contreras]

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

Re: [PATCH 1/4] commit: reload cache properly

[Duy Nguyen]

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

[PATCH] Test 'commit --only' after 'checkout --orphan'

[Thomas Rast]

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

Re: [PATCH 1/4] commit: reload cache properly

[Duy Nguyen]

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

[PATCH 2/4] read-cache: plug small memory leak

[Felipe Contreras]

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

[PATCH 3/4] unpack-trees: plug a memory leak

[Felipe Contreras]

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

Re: [PATCH 3/4] unpack-trees: plug a memory leak

[Junio C Hamano]

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);

Re: [PATCH 3/4] unpack-trees: plug a memory leak

[Felipe Contreras]

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

Re: [PATCH 3/4] unpack-trees: plug a memory leak

[Junio C Hamano]

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".

Re: [PATCH 3/4] unpack-trees: plug a memory leak

[Felipe Contreras]

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

[PATCH 4/4] unpack-trees: free created cache entries

[Felipe Contreras]

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