[PATCH] log -L: do not free parents lists we might need again
- From
Thomas Rast <trast@student.ethz.ch>
- Date
- Sep 11, 2010, 21:10 UTC
- Message-ID
- <92a37c8a8d7157aa8bf4d654f4e5bd713c69382d.1284239099.git.trast@student.ethz.ch>
- In-Reply-To
- <20100830171007.GC21441@burratino>
The parent rewriting code of 'git log -L' was too aggressive in freeing memory: assign_range_to_parent() will free the commit->parents field when it sees that a parent cannot pass off any blame (is a root commit in rewritten history).
Its caller assign_parents_range() however will, upon finding the first parent that takes *full* blame for all ranges, rewind and reinstate all previous parents' line ranges and parent lists. This resurrects pointers to ranges that were freed in assign_range_to_parent() under some circumstances.
Furthermore, we must not empty the parent lists either: the same rewind/reinstate code relies on them.
Do both only if the commit was an ordinary (not merge or root) commit, in which case the merge code-path discussed here is never taken.
Reported-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Thomas Rast <trast@student.ethz.ch> --- [sorry for the double post, forgot to cc the list]
After staring at it for some time, I think this is the problem.
Sadly I cannot come up with an independent test that reproduces it (or at least generates a valgrind warning). Based on my analysis I tried
diff --git a/t/t4302-log-line-merge-history.sh b/t/t4302-log-line-merge-history.sh index 8634116..7c86903 100755 --- a/t/t4302-log-line-merge-history.sh +++ b/t/t4302-log-line-merge-history.sh @@ -171,4 +171,17 @@ test_expect_success 'validate the graph output.' ' test_cmp current-graph expected-graph ' +test_expect_success 'set up trivial side merge' ' + git checkout -b trivial-side && + echo new_line >> path0 && + git add path0 && + git commit -m new_line && + git checkout master && + git merge --no-ff trivial-side +' + +test_expect_success 'log -L on the trivial-merged file' ' + git log -L /new_line/,+1 path0 +' + test_done
but that does not fail. I am hesitant to add your original test because it strays into the directory where git was built, to test with git's own history. It just feels wrong.
line.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/line.c b/line.c index 63dd19a..b0deafb 100644 --- a/line.c +++ b/line.c @@ -961,8 +961,10 @@ static int assign_range_to_parent(struct rev_info *rev, struct commit *c, * If there is no new ranges assigned to the parent, * we should mark it as a 'root' commit. */ - free(c->parents); - c->parents = NULL; + if (c->parents && !c->parents->next) { + free(c->parents); + c->parents = NULL; + } } /* and the ranges of current commit c is updated */
-- 1.7.3.rc1.333.gf62981