From f172da6b28eb7008f68f313f10f1ee84e4e01c2a Mon Sep 17 00:00:00 2001 From: Jean-Jacques Lafay Date: Sat, 10 Nov 2012 18:36:10 +0100 Subject: [PATCH] git tag --contains : avoid stack overflow In large repos, the recursion implementation of contains(commit, commit_list) may result in a stack overflow. Replace the recursion with a loop to fix it. This problem is more apparent on Windows than on Linux, where the stack is more limited by default. See also this thread on the msysGit list: https://groups.google.com/d/topic/msysgit/FqT6boJrb2g/discussion [jes: re-written to imitate the original recursion more closely] Signed-off-by: Jean-Jacques Lafay Signed-off-by: Johannes Schindelin Tested-by: Stepan Kasal --- builtin/tag.c | 82 +++++++++++++++++++++++++++++++++++++++++--------- t/t7004-tag.sh | 22 ++++++++++++++ 2 files changed, 90 insertions(+), 14 deletions(-) diff --git a/builtin/tag.c b/builtin/tag.c index 74d3780b77..8869603595 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -73,11 +73,13 @@ static int in_commit_list(const struct commit_list *want, struct commit *c) return 0; } -static int contains_recurse(struct commit *candidate, +/* + * Test whether the candidate or one of its parents is contained in the list. + * Do not recurse to find out, though, but return -1 if inconclusive. + */ +static int contains_test(struct commit *candidate, const struct commit_list *want) { - struct commit_list *p; - /* was it previously marked as containing a want commit? */ if (candidate->object.flags & TMP_MARK) return 1; @@ -85,26 +87,78 @@ static int contains_recurse(struct commit *candidate, if (candidate->object.flags & UNINTERESTING) return 0; /* or are we it? */ - if (in_commit_list(want, candidate)) + if (in_commit_list(want, candidate)) { + candidate->object.flags |= TMP_MARK; return 1; + } if (parse_commit(candidate) < 0) return 0; - /* Otherwise recurse and mark ourselves for future traversals. */ - for (p = candidate->parents; p; p = p->next) { - if (contains_recurse(p->item, want)) { - candidate->object.flags |= TMP_MARK; - return 1; - } - } - candidate->object.flags |= UNINTERESTING; - return 0; + return -1; +} + +/* + * This stack on the heap mimics the real stack in the previous implementation + * of contains_recurse() that had to be replaced because it easily led to a + * stack overflow. + * + * Every stack item points at a certain commit whose parents are iterated over. + * Each item's current parent is the current next stack item's commit. + */ +struct stack { + int nr, alloc; + struct stack_entry { + struct commit *commit; + struct commit_list *parents; + } *stack; +}; + +static void push_to_stack(struct commit *candidate, struct stack *stack) +{ + int index = stack->nr++; + ALLOC_GROW(stack->stack, stack->nr, stack->alloc); + stack->stack[index].commit = candidate; + stack->stack[index].parents = candidate->parents; } static int contains(struct commit *candidate, const struct commit_list *want) { - return contains_recurse(candidate, want); + struct stack stack = { 0, 0, NULL }; + int result = contains_test(candidate, want); + + if (result >= 0) + return result; + + push_to_stack(candidate, &stack); + while (stack.nr) { + struct stack_entry *entry = &stack.stack[stack.nr - 1]; + struct commit *commit = entry->commit; + struct commit_list *parents = entry->parents; + + if (!parents) { + commit->object.flags = UNINTERESTING; + stack.nr--; + } + /* + * If we just popped the stack, parents->item has been marked, + * therefore contains_test will return a meaningful 0 or 1. + */ + else switch (contains_test(parents->item, want)) { + case 1: + commit->object.flags |= TMP_MARK; + stack.nr--; + break; + case 0: + entry->parents = parents->next; + break; + default: + push_to_stack(parents->item, &stack); + break; + } + } + free(stack.stack); + return contains_test(candidate, want); } static void show_tag_lines(const unsigned char *sha1, int lines) diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index c8d6e9f88c..2e008293fb 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -1380,4 +1380,26 @@ test_expect_success 'multiple --points-at are OR-ed together' ' test_cmp expect actual ' +# what about a deep repo ? + +>expect +test_expect_success MINGW '--contains works in a deep repo' ' + ulimit -s 64 + i=1 && + while test $i -lt 1000 + do + echo "commit refs/heads/master +committer A U Thor $((1000000000 + $i * 100)) +0200 +data <actual && + test_cmp expect actual +' + test_done