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] Thomas Braun pointed out several documentation shortcomings. Tests are run only if ulimit -s is available. This means they cannot be run on Windows. Signed-off-by: Jean-Jacques Lafay <jeanjacques.lafay@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Tested-by: Stepan Kasal <kasal@ucw.cz> Reviewed-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jean-Jacques Lafay committed
Apr 24, 2014 at 14:24 UTC
cbc60b67201e083a4970c8731c5382a575357e36
2 files changed
+101
-15
builtin/tag.c
+75
-15
@@ -80,11 +80,19 @@ static int in_commit_list(const struct commit_list *want, struct commit *c)
80
return 0;
81
}
82
83
-static int contains_recurse(struct commit *candidate,
83
+enum contains_result {
84
+ CONTAINS_UNKNOWN = -1,
85
+ CONTAINS_NO = 0,
86
+ CONTAINS_YES = 1,
87
+};
88
+
89
+/*
90
+ * Test whether the candidate or one of its parents is contained in the list.
91
+ * Do not recurse to find out, though, but return -1 if inconclusive.
92
+ */
93
+static enum contains_result contains_test(struct commit *candidate,
94
const struct commit_list *want)
95
{
86
- struct commit_list *p;
87
-
96
/* was it previously marked as containing a want commit? */
97
if (candidate->object.flags & TMP_MARK)
98
return 1;
@@ -92,26 +100,78 @@ static int contains_recurse(struct commit *candidate,
100
if (candidate->object.flags & UNINTERESTING)
101
return 0;
102
/* or are we it? */
95
- if (in_commit_list(want, candidate))
103
+ if (in_commit_list(want, candidate)) {
104
+ candidate->object.flags |= TMP_MARK;
105
return 1;
106
+ }
107
108
if (parse_commit(candidate) < 0)
109
return 0;
110
101
- /* Otherwise recurse and mark ourselves for future traversals. */
102
- for (p = candidate->parents; p; p = p->next) {
103
- if (contains_recurse(p->item, want)) {
104
- candidate->object.flags |= TMP_MARK;
105
- return 1;
106
- }
107
- }
108
- candidate->object.flags |= UNINTERESTING;
109
- return 0;
111
+ return -1;
112
}
113
112
-static int contains(struct commit *candidate, const struct commit_list *want)
114
+/*
115
+ * Mimicking the real stack, this stack lives on the heap, avoiding stack
116
+ * overflows.
117
+ *
118
+ * At each recursion step, the stack items points to the commits whose
119
+ * ancestors are to be inspected.
120
+ */
121
+struct stack {
122
+ int nr, alloc;
123
+ struct stack_entry {
124
+ struct commit *commit;
125
+ struct commit_list *parents;
126
+ } *stack;
127
+};
128
+
129
+static void push_to_stack(struct commit *candidate, struct stack *stack)
130
+{
131
+ int index = stack->nr++;
132
+ ALLOC_GROW(stack->stack, stack->nr, stack->alloc);
133
+ stack->stack[index].commit = candidate;
134
+ stack->stack[index].parents = candidate->parents;
135
+}
136
+
137
+static enum contains_result contains(struct commit *candidate,
138
+ const struct commit_list *want)
139
{
114
- return contains_recurse(candidate, want);
140
+ struct stack stack = { 0, 0, NULL };
141
+ int result = contains_test(candidate, want);
142
+
143
+ if (result != CONTAINS_UNKNOWN)
144
+ return result;
145
+
146
+ push_to_stack(candidate, &stack);
147
+ while (stack.nr) {
148
+ struct stack_entry *entry = &stack.stack[stack.nr - 1];
149
+ struct commit *commit = entry->commit;
150
+ struct commit_list *parents = entry->parents;
151
+
152
+ if (!parents) {
153
+ commit->object.flags |= UNINTERESTING;
154
+ stack.nr--;
155
+ }
156
+ /*
157
+ * If we just popped the stack, parents->item has been marked,
158
+ * therefore contains_test will return a meaningful 0 or 1.
159
+ */
160
+ else switch (contains_test(parents->item, want)) {
161
+ case CONTAINS_YES:
162
+ commit->object.flags |= TMP_MARK;
163
+ stack.nr--;
164
+ break;
165
+ case CONTAINS_NO:
166
+ entry->parents = parents->next;
167
+ break;
168
+ case CONTAINS_UNKNOWN:
169
+ push_to_stack(parents->item, &stack);
170
+ break;
171
+ }
172
+ }
173
+ free(stack.stack);
174
+ return contains_test(candidate, want);
175
}
176
177
static void show_tag_lines(const unsigned char *sha1, int lines)
t/t7004-tag.sh
+26
@@ -1423,4 +1423,30 @@ EOF
1423
test_cmp expect actual
1424
'
1425
1426
+run_with_limited_stack () {
1427
+ (ulimit -s 64 && "$@")
1428
+}
1429
+
1430
+test_lazy_prereq ULIMIT 'run_with_limited_stack true'
1431
+
1432
+# we require ulimit, this excludes Windows
1433
+test_expect_success ULIMIT '--contains works in a deep repo' '
1434
+ >expect &&
1435
+ i=1 &&
1436
+ while test $i -lt 4000
1437
+ do
1438
+ echo "commit refs/heads/master
1439
+committer A U Thor <author@example.com> $((1000000000 + $i * 100)) +0200
1440
+data <<EOF
1441
+commit #$i
1442
+EOF"
1443
+ test $i = 1 && echo "from refs/heads/master^0"
1444
+ i=$(($i + 1))
1445
+ done | git fast-import &&
1446
+ git checkout master &&
1447
+ git tag far-far-away HEAD^ &&
1448
+ run_with_limited_stack git tag --contains HEAD >actual &&
1449
+ test_cmp expect actual
1450
+'
1451
+
1452
test_done