* [PATCH 0/4] cherry-pick: fix memory leaks
@ 2013-05-30 11:58 Felipe Contreras
2013-05-30 11:58 ` [PATCH 1/4] commit: reload cache properly Felipe Contreras
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Felipe Contreras @ 2013-05-30 11:58 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, René Scharfe,
Nguyễn Thái Ngọc Duy, Adam Spiers,
Ramkumar Ramachandra, Stephen Boyd, 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/4] commit: reload cache properly
2013-05-30 11:58 [PATCH 0/4] cherry-pick: fix memory leaks Felipe Contreras
@ 2013-05-30 11:58 ` Felipe Contreras
2013-05-30 12:17 ` Thomas Rast
2013-05-30 11:58 ` [PATCH 2/4] read-cache: plug small memory leak Felipe Contreras
` (2 subsequent siblings)
3 siblings, 1 reply; 16+ messages in thread
From: Felipe Contreras @ 2013-05-30 11:58 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, René Scharfe,
Nguyễn Thái Ngọc Duy, Adam Spiers,
Ramkumar Ramachandra, Stephen Boyd, 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/4] read-cache: plug small memory leak
2013-05-30 11:58 [PATCH 0/4] cherry-pick: fix memory leaks Felipe Contreras
2013-05-30 11:58 ` [PATCH 1/4] commit: reload cache properly Felipe Contreras
@ 2013-05-30 11:58 ` Felipe Contreras
2013-05-30 11:58 ` [PATCH 3/4] unpack-trees: plug a " Felipe Contreras
2013-05-30 11:58 ` [PATCH 4/4] unpack-trees: free created cache entries Felipe Contreras
3 siblings, 0 replies; 16+ messages in thread
From: Felipe Contreras @ 2013-05-30 11:58 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, René Scharfe,
Nguyễn Thái Ngọc Duy, Adam Spiers,
Ramkumar Ramachandra, Stephen Boyd, 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/4] unpack-trees: plug a memory leak
2013-05-30 11:58 [PATCH 0/4] cherry-pick: fix memory leaks Felipe Contreras
2013-05-30 11:58 ` [PATCH 1/4] commit: reload cache properly Felipe Contreras
2013-05-30 11:58 ` [PATCH 2/4] read-cache: plug small memory leak Felipe Contreras
@ 2013-05-30 11:58 ` Felipe Contreras
2013-06-02 19:33 ` Junio C Hamano
2013-05-30 11:58 ` [PATCH 4/4] unpack-trees: free created cache entries Felipe Contreras
3 siblings, 1 reply; 16+ messages in thread
From: Felipe Contreras @ 2013-05-30 11:58 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, René Scharfe,
Nguyễn Thái Ngọc Duy, Adam Spiers,
Ramkumar Ramachandra, Stephen Boyd, 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 4/4] unpack-trees: free created cache entries
2013-05-30 11:58 [PATCH 0/4] cherry-pick: fix memory leaks Felipe Contreras
` (2 preceding siblings ...)
2013-05-30 11:58 ` [PATCH 3/4] unpack-trees: plug a " Felipe Contreras
@ 2013-05-30 11:58 ` Felipe Contreras
3 siblings, 0 replies; 16+ messages in thread
From: Felipe Contreras @ 2013-05-30 11:58 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, René Scharfe,
Nguyễn Thái Ngọc Duy, Adam Spiers,
Ramkumar Ramachandra, Stephen Boyd, 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] commit: reload cache properly
2013-05-30 11:58 ` [PATCH 1/4] commit: reload cache properly Felipe Contreras
@ 2013-05-30 12:17 ` Thomas Rast
2013-05-30 12:35 ` Felipe Contreras
2013-05-30 13:08 ` [PATCH 1/4] commit: reload cache properly Duy Nguyen
0 siblings, 2 replies; 16+ messages in thread
From: Thomas Rast @ 2013-05-30 12:17 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Junio C Hamano, René Scharfe,
Nguyễn Thái Ngọc Duy, Adam Spiers,
Ramkumar Ramachandra, Stephen Boyd
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] commit: reload cache properly
2013-05-30 12:17 ` Thomas Rast
@ 2013-05-30 12:35 ` Felipe Contreras
2013-05-30 12:58 ` Thomas Rast
2013-05-30 13:08 ` [PATCH 1/4] commit: reload cache properly Duy Nguyen
1 sibling, 1 reply; 16+ messages in thread
From: Felipe Contreras @ 2013-05-30 12:35 UTC (permalink / raw)
To: Thomas Rast
Cc: git, Junio C Hamano, René Scharfe,
Nguyễn Thái Ngọc, Adam Spiers,
Ramkumar Ramachandra, Stephen Boyd
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] commit: reload cache properly
2013-05-30 12:35 ` Felipe Contreras
@ 2013-05-30 12:58 ` Thomas Rast
2013-05-30 13:07 ` Felipe Contreras
2013-06-01 9:09 ` Duy Nguyen
0 siblings, 2 replies; 16+ messages in thread
From: Thomas Rast @ 2013-05-30 12:58 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Junio C Hamano, René Scharfe,
Nguyễn Thái Ngọc, Adam Spiers,
Ramkumar Ramachandra, Stephen Boyd
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] commit: reload cache properly
2013-05-30 12:58 ` Thomas Rast
@ 2013-05-30 13:07 ` Felipe Contreras
2013-06-01 9:09 ` Duy Nguyen
1 sibling, 0 replies; 16+ messages in thread
From: Felipe Contreras @ 2013-05-30 13:07 UTC (permalink / raw)
To: Thomas Rast
Cc: git, Junio C Hamano, René Scharfe,
Nguyễn Thái Ngọc, Adam Spiers,
Ramkumar Ramachandra, Stephen Boyd
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] commit: reload cache properly
2013-05-30 12:17 ` Thomas Rast
2013-05-30 12:35 ` Felipe Contreras
@ 2013-05-30 13:08 ` Duy Nguyen
1 sibling, 0 replies; 16+ messages in thread
From: Duy Nguyen @ 2013-05-30 13:08 UTC (permalink / raw)
To: Thomas Rast
Cc: Felipe Contreras, Git Mailing List, Junio C Hamano,
René Scharfe, Adam Spiers, Ramkumar Ramachandra,
Stephen Boyd
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] commit: reload cache properly
2013-05-30 12:58 ` Thomas Rast
2013-05-30 13:07 ` Felipe Contreras
@ 2013-06-01 9:09 ` Duy Nguyen
2013-06-01 11:02 ` [PATCH] Test 'commit --only' after 'checkout --orphan' Thomas Rast
1 sibling, 1 reply; 16+ messages in thread
From: Duy Nguyen @ 2013-06-01 9:09 UTC (permalink / raw)
To: Thomas Rast
Cc: Felipe Contreras, Git Mailing List, Junio C Hamano,
René Scharfe, Adam Spiers, Ramkumar Ramachandra,
Stephen Boyd
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] Test 'commit --only' after 'checkout --orphan'
2013-06-01 9:09 ` Duy Nguyen
@ 2013-06-01 11:02 ` Thomas Rast
0 siblings, 0 replies; 16+ messages in thread
From: Thomas Rast @ 2013-06-01 11:02 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Nguyễn Thái Ngọc Duy, René Scharfe,
Ramkumar Ramachandra, Stephen Boyd, Felipe Contreras
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 3/4] unpack-trees: plug a memory leak
2013-05-30 11:58 ` [PATCH 3/4] unpack-trees: plug a " Felipe Contreras
@ 2013-06-02 19:33 ` Junio C Hamano
2013-06-02 19:51 ` Felipe Contreras
0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2013-06-02 19:33 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, René Scharfe, Nguyễn Thái Ngọc Duy,
Adam Spiers, Ramkumar Ramachandra, Stephen Boyd
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);
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/4] unpack-trees: plug a memory leak
2013-06-02 19:33 ` Junio C Hamano
@ 2013-06-02 19:51 ` Felipe Contreras
2013-06-02 22:17 ` Junio C Hamano
0 siblings, 1 reply; 16+ messages in thread
From: Felipe Contreras @ 2013-06-02 19:51 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, René Scharfe, Nguyễn Thái Ngọc,
Adam Spiers, Ramkumar Ramachandra, Stephen Boyd
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/4] unpack-trees: plug a memory leak
2013-06-02 19:51 ` Felipe Contreras
@ 2013-06-02 22:17 ` Junio C Hamano
2013-06-02 22:45 ` Felipe Contreras
0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2013-06-02 22:17 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, René Scharfe, Nguyễn Thái Ngọc,
Adam Spiers, Ramkumar Ramachandra, Stephen Boyd
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".
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/4] unpack-trees: plug a memory leak
2013-06-02 22:17 ` Junio C Hamano
@ 2013-06-02 22:45 ` Felipe Contreras
0 siblings, 0 replies; 16+ messages in thread
From: Felipe Contreras @ 2013-06-02 22:45 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, René Scharfe, Nguyễn Thái Ngọc,
Adam Spiers, Ramkumar Ramachandra, Stephen Boyd
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
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2013-06-02 22:45 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-30 11:58 [PATCH 0/4] cherry-pick: fix memory leaks Felipe Contreras
2013-05-30 11:58 ` [PATCH 1/4] commit: reload cache properly Felipe Contreras
2013-05-30 12:17 ` Thomas Rast
2013-05-30 12:35 ` Felipe Contreras
2013-05-30 12:58 ` Thomas Rast
2013-05-30 13:07 ` Felipe Contreras
2013-06-01 9:09 ` Duy Nguyen
2013-06-01 11:02 ` [PATCH] Test 'commit --only' after 'checkout --orphan' Thomas Rast
2013-05-30 13:08 ` [PATCH 1/4] commit: reload cache properly Duy Nguyen
2013-05-30 11:58 ` [PATCH 2/4] read-cache: plug small memory leak Felipe Contreras
2013-05-30 11:58 ` [PATCH 3/4] unpack-trees: plug a " Felipe Contreras
2013-06-02 19:33 ` Junio C Hamano
2013-06-02 19:51 ` Felipe Contreras
2013-06-02 22:17 ` Junio C Hamano
2013-06-02 22:45 ` Felipe Contreras
2013-05-30 11:58 ` [PATCH 4/4] unpack-trees: free created cache entries Felipe Contreras
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).