Commit 7697e54176ccab22ed6d4597d7256e9a1e6ff202

Russell Belfer 2013-12-11T15:02:20

Test cancel from indexer progress callback This adds tests that try canceling an indexer operation from within the progress callback. After writing the tests, I wanted to run this under valgrind and had a number of errors in that situation because mmap wasn't working. I added a CMake option to force emulation of mmap and consolidated the Amiga-specific code into that new place (so we don't actually need separate Amiga code now, just have to turn on -DNO_MMAP). Additionally, I made the indexer code propagate error codes more reliably than it used to.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
381
382
383
384
385
386
387
388
389
390
391
392
393
394
395
396
397
398
399
400
401
402
403
404
405
406
407
408
409
410
411
412
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 9c19a5a..48cbccb 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -33,6 +33,7 @@ OPTION( ANDROID				"Build for android NDK"	 				OFF )
 
 OPTION( USE_ICONV			"Link with and use iconv library" 		OFF )
 OPTION( USE_SSH				"Link with libssh to enable SSH support" ON )
+OPTION( VALGRIND			"Configure build for valgrind"			OFF )
 
 IF(${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
 	SET( USE_ICONV ON )
@@ -340,9 +341,11 @@ IF (WIN32 AND NOT CYGWIN)
 	ADD_DEFINITIONS(-DWIN32 -D_WIN32_WINNT=0x0501)
 	FILE(GLOB SRC_OS src/win32/*.c src/win32/*.h)
 ELSEIF (AMIGA)
-	ADD_DEFINITIONS(-DNO_ADDRINFO -DNO_READDIR_R)
-	FILE(GLOB SRC_OS src/amiga/*.c src/amiga/*.h)
+	ADD_DEFINITIONS(-DNO_ADDRINFO -DNO_READDIR_R -DNO_MMAP)
 ELSE()
+	IF (VALGRIND)
+		ADD_DEFINITIONS(-DNO_MMAP)
+	ENDIF()
 	FILE(GLOB SRC_OS src/unix/*.c src/unix/*.h)
 ENDIF()
 FILE(GLOB SRC_GIT2 src/*.c src/*.h src/transports/*.c src/transports/*.h src/xdiff/*.c src/xdiff/*.h)
diff --git a/include/git2/pack.h b/include/git2/pack.h
index 88a2716..11bb559 100644
--- a/include/git2/pack.h
+++ b/include/git2/pack.h
@@ -52,7 +52,7 @@ typedef enum {
 	GIT_PACKBUILDER_ADDING_OBJECTS = 0,
 	GIT_PACKBUILDER_DELTAFICATION = 1,
 } git_packbuilder_stage_t;
-	
+
 /**
  * Initialize a new packbuilder
  *
@@ -143,6 +143,7 @@ GIT_EXTERN(int) git_packbuilder_write(
 GIT_EXTERN(const git_oid *) git_packbuilder_hash(git_packbuilder *pb);
 
 typedef int (*git_packbuilder_foreach_cb)(void *buf, size_t size, void *payload);
+
 /**
  * Create the new pack and pass each object to the callback
  *
diff --git a/src/amiga/map.c b/src/amiga/map.c
deleted file mode 100644
index 0ba7995..0000000
--- a/src/amiga/map.c
+++ /dev/null
@@ -1,48 +0,0 @@
-/*
- * Copyright (C) the libgit2 contributors. All rights reserved.
- *
- * This file is part of libgit2, distributed under the GNU GPL v2 with
- * a Linking Exception. For full terms see the included COPYING file.
- */
-#include <git2/common.h>
-
-#ifndef GIT_WIN32
-
-#include "posix.h"
-#include "map.h"
-#include <errno.h>
-
-int p_mmap(git_map *out, size_t len, int prot, int flags, int fd, git_off_t offset)
-{
-	GIT_MMAP_VALIDATE(out, len, prot, flags);
-
-	out->data = NULL;
-	out->len = 0;
-
-	if ((prot & GIT_PROT_WRITE) && ((flags & GIT_MAP_TYPE) == GIT_MAP_SHARED)) {
-		giterr_set(GITERR_OS, "Trying to map shared-writeable");
-		return -1;
-	}
-
-	out->data = malloc(len);
-	GITERR_CHECK_ALLOC(out->data);
-
-	if ((p_lseek(fd, offset, SEEK_SET) < 0) || ((size_t)p_read(fd, out->data, len) != len)) {
-		giterr_set(GITERR_OS, "mmap emulation failed");
-		return -1;
-	}
-
-	out->len = len;
-	return 0;
-}
-
-int p_munmap(git_map *map)
-{
-	assert(map != NULL);
-	free(map->data);
-
-	return 0;
-}
-
-#endif
-
diff --git a/src/indexer.c b/src/indexer.c
index 88897d0..718c698 100644
--- a/src/indexer.c
+++ b/src/indexer.c
@@ -441,8 +441,8 @@ int git_indexer_append(git_indexer *idx, const void *data, size_t size, git_tran
 
 	processed = stats->indexed_objects;
 
-	if (git_filebuf_write(&idx->pack_file, data, size) < 0)
-		return -1;
+	if ((error = git_filebuf_write(&idx->pack_file, data, size)) < 0)
+		return error;
 
 	hash_partially(idx, data, (int)size);
 
@@ -450,12 +450,12 @@ int git_indexer_append(git_indexer *idx, const void *data, size_t size, git_tran
 	if (idx->opened_pack) {
 		idx->pack->mwf.size += size;
 	} else {
-		if (open_pack(&idx->pack, idx->pack_file.path_lock) < 0)
-			return -1;
+		if ((error = open_pack(&idx->pack, idx->pack_file.path_lock)) < 0)
+			return error;
 		idx->opened_pack = 1;
 		mwf = &idx->pack->mwf;
-		if (git_mwindow_file_register(&idx->pack->mwf) < 0)
-			return -1;
+		if ((error = git_mwindow_file_register(&idx->pack->mwf)) < 0)
+			return error;
 	}
 
 	if (!idx->parsed_header) {
@@ -464,8 +464,8 @@ int git_indexer_append(git_indexer *idx, const void *data, size_t size, git_tran
 		if ((unsigned)idx->pack->mwf.size < sizeof(struct git_pack_header))
 			return 0;
 
-		if (parse_header(&idx->hdr, idx->pack) < 0)
-			return -1;
+		if ((error = parse_header(&idx->hdr, idx->pack)) < 0)
+			return error;
 
 		idx->parsed_header = 1;
 		idx->nr_objects = ntohl(hdr->hdr_entries);
@@ -503,6 +503,7 @@ int git_indexer_append(git_indexer *idx, const void *data, size_t size, git_tran
 
 	/* As the file grows any windows we try to use will be out of date */
 	git_mwindow_free_all(mwf);
+
 	while (processed < idx->nr_objects) {
 		git_packfile_stream *stream = &idx->stream;
 		git_off_t entry_start = idx->off;
@@ -520,7 +521,7 @@ int git_indexer_append(git_indexer *idx, const void *data, size_t size, git_tran
 				return 0;
 			}
 			if (error < 0)
-				return error;
+				goto on_error;
 
 			git_mwindow_close(&w);
 			idx->entry_start = entry_start;
@@ -533,7 +534,7 @@ int git_indexer_append(git_indexer *idx, const void *data, size_t size, git_tran
 					return 0;
 				}
 				if (error < 0)
-					return error;
+					goto on_error;
 
 				idx->have_delta = 1;
 			} else {
@@ -542,9 +543,10 @@ int git_indexer_append(git_indexer *idx, const void *data, size_t size, git_tran
 			}
 
 			idx->have_stream = 1;
-			if (git_packfile_stream_open(stream, idx->pack, idx->off) < 0)
-				goto on_error;
 
+			error = git_packfile_stream_open(stream, idx->pack, idx->off);
+			if (error < 0)
+				goto on_error;
 		}
 
 		if (idx->have_delta) {
@@ -858,7 +860,7 @@ int git_indexer_commit(git_indexer *idx, git_transfer_progress *stats)
 
 	/* Test for this before resolve_deltas(), as it plays with idx->off */
 	if (idx->off < idx->pack->mwf.size - 20) {
-		giterr_set(GITERR_INDEXER, "unexpected data at the end of the pack");
+		giterr_set(GITERR_INDEXER, "Unexpected data at the end of the pack");
 		return -1;
 	}
 
diff --git a/src/posix.c b/src/posix.c
index b75109b..525785f 100644
--- a/src/posix.c
+++ b/src/posix.c
@@ -203,4 +203,40 @@ int p_write(git_file fd, const void *buf, size_t cnt)
 	return 0;
 }
 
+#ifdef NO_MMAP
 
+#include "map.h"
+
+int p_mmap(git_map *out, size_t len, int prot, int flags, int fd, git_off_t offset)
+{
+	GIT_MMAP_VALIDATE(out, len, prot, flags);
+
+	out->data = NULL;
+	out->len = 0;
+
+	if ((prot & GIT_PROT_WRITE) && ((flags & GIT_MAP_TYPE) == GIT_MAP_SHARED)) {
+		giterr_set(GITERR_OS, "Trying to map shared-writeable");
+		return -1;
+	}
+
+	out->data = malloc(len);
+	GITERR_CHECK_ALLOC(out->data);
+
+	if ((p_lseek(fd, offset, SEEK_SET) < 0) || ((size_t)p_read(fd, out->data, len) != len)) {
+		giterr_set(GITERR_OS, "mmap emulation failed");
+		return -1;
+	}
+
+	out->len = len;
+	return 0;
+}
+
+int p_munmap(git_map *map)
+{
+	assert(map != NULL);
+	free(map->data);
+
+	return 0;
+}
+
+#endif
diff --git a/src/unix/map.c b/src/unix/map.c
index 7de99c9..e62ab3e 100644
--- a/src/unix/map.c
+++ b/src/unix/map.c
@@ -6,7 +6,7 @@
  */
 #include <git2/common.h>
 
-#ifndef GIT_WIN32
+#if !defined(GIT_WIN32) && !defined(NO_MMAP)
 
 #include "map.h"
 #include <sys/mman.h>
diff --git a/src/win32/map.c b/src/win32/map.c
index 44c6c4e..902ea39 100644
--- a/src/win32/map.c
+++ b/src/win32/map.c
@@ -8,6 +8,7 @@
 #include "map.h"
 #include <errno.h>
 
+#ifndef NO_MMAP
 
 static DWORD get_page_size(void)
 {
@@ -112,4 +113,4 @@ int p_munmap(git_map *map)
 	return error;
 }
 
-
+#endif
diff --git a/tests/pack/indexer.c b/tests/pack/indexer.c
index 07963a9..084f8e6 100644
--- a/tests/pack/indexer.c
+++ b/tests/pack/indexer.c
@@ -11,7 +11,7 @@
  * This is a packfile with three objects. The second is a delta which
  * depends on the third, which is also a delta.
  */
-unsigned char out_of_order_pack[] = {
+static const unsigned char out_of_order_pack[] = {
   0x50, 0x41, 0x43, 0x4b, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x03,
   0x32, 0x78, 0x9c, 0x63, 0x67, 0x00, 0x00, 0x00, 0x10, 0x00, 0x08, 0x76,
   0xe6, 0x8f, 0xe8, 0x12, 0x9b, 0x54, 0x6b, 0x10, 0x1a, 0xee, 0x95, 0x10,
@@ -23,13 +23,13 @@ unsigned char out_of_order_pack[] = {
   0x19, 0x87, 0x58, 0x80, 0x61, 0x09, 0x9a, 0x33, 0xca, 0x7a, 0x31, 0x92,
   0x6f, 0xae, 0x66, 0x75
 };
-unsigned int out_of_order_pack_len = 112;
+static const unsigned int out_of_order_pack_len = 112;
 
 /*
  * Packfile with two objects. The second is a delta against an object
  * which is not in the packfile
  */
-unsigned char thin_pack[] = {
+static const unsigned char thin_pack[] = {
   0x50, 0x41, 0x43, 0x4b, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x02,
   0x32, 0x78, 0x9c, 0x63, 0x67, 0x00, 0x00, 0x00, 0x10, 0x00, 0x08, 0x76,
   0xe6, 0x8f, 0xe8, 0x12, 0x9b, 0x54, 0x6b, 0x10, 0x1a, 0xee, 0x95, 0x10,
@@ -38,18 +38,19 @@ unsigned char thin_pack[] = {
   0x3a, 0x6f, 0x39, 0xd1, 0xfe, 0x66, 0x68, 0x6b, 0xa5, 0xe5, 0xe2, 0x97,
   0xac, 0x94, 0x6c, 0x76, 0x0b, 0x04
 };
-unsigned int thin_pack_len = 78;
+static const unsigned int thin_pack_len = 78;
 
-unsigned char base_obj[] = { 07, 076 };
-unsigned int base_obj_len = 2;
+static const unsigned char base_obj[] = { 07, 076 };
+static const unsigned int base_obj_len = 2;
 
 void test_pack_indexer__out_of_order(void)
 {
-	git_indexer *idx;
-	git_transfer_progress stats;
+	git_indexer *idx = 0;
+	git_transfer_progress stats = { 0 };
 
 	cl_git_pass(git_indexer_new(&idx, ".", 0, NULL, NULL, NULL));
-	cl_git_pass(git_indexer_append(idx, out_of_order_pack, out_of_order_pack_len, &stats));
+	cl_git_pass(git_indexer_append(
+		idx, out_of_order_pack, out_of_order_pack_len, &stats));
 	cl_git_pass(git_indexer_commit(idx, &stats));
 
 	cl_assert_equal_i(stats.total_objects, 3);
@@ -61,8 +62,8 @@ void test_pack_indexer__out_of_order(void)
 
 void test_pack_indexer__fix_thin(void)
 {
-	git_indexer *idx;
-	git_transfer_progress stats;
+	git_indexer *idx = NULL;
+	git_transfer_progress stats = { 0 };
 	git_repository *repo;
 	git_odb *odb;
 	git_oid id, should_id;
diff --git a/tests/pack/packbuilder.c b/tests/pack/packbuilder.c
index 1ae2322..53db818 100644
--- a/tests/pack/packbuilder.c
+++ b/tests/pack/packbuilder.c
@@ -12,6 +12,7 @@ static git_packbuilder *_packbuilder;
 static git_indexer *_indexer;
 static git_vector _commits;
 static int _commits_is_initialized;
+static git_transfer_progress _stats;
 
 void test_pack_packbuilder__initialize(void)
 {
@@ -20,6 +21,7 @@ void test_pack_packbuilder__initialize(void)
 	cl_git_pass(git_packbuilder_new(&_packbuilder, _repo));
 	cl_git_pass(git_vector_init(&_commits, 0, NULL));
 	_commits_is_initialized = 1;
+	memset(&_stats, 0, sizeof(_stats));
 }
 
 void test_pack_packbuilder__cleanup(void)
@@ -184,11 +186,10 @@ void test_pack_packbuilder__permissions_readwrite(void)
 	test_write_pack_permission(0666, 0666);
 }
 
-static git_transfer_progress stats;
 static int foreach_cb(void *buf, size_t len, void *payload)
 {
 	git_indexer *idx = (git_indexer *) payload;
-	cl_git_pass(git_indexer_append(idx, buf, len, &stats));
+	cl_git_pass(git_indexer_append(idx, buf, len, &_stats));
 	return 0;
 }
 
@@ -199,6 +200,24 @@ void test_pack_packbuilder__foreach(void)
 	seed_packbuilder();
 	cl_git_pass(git_indexer_new(&idx, ".", 0, NULL, NULL, NULL));
 	cl_git_pass(git_packbuilder_foreach(_packbuilder, foreach_cb, idx));
-	cl_git_pass(git_indexer_commit(idx, &stats));
+	cl_git_pass(git_indexer_commit(idx, &_stats));
+	git_indexer_free(idx);
+}
+
+static int foreach_cancel_cb(void *buf, size_t len, void *payload)
+{
+	git_indexer *idx = (git_indexer *)payload;
+	cl_git_pass(git_indexer_append(idx, buf, len, &_stats));
+	return (_stats.total_objects > 2) ? -1111 : 0;
+}
+
+void test_pack_packbuilder__foreach_with_cancel(void)
+{
+	git_indexer *idx;
+
+	seed_packbuilder();
+	cl_git_pass(git_indexer_new(&idx, ".", 0, NULL, NULL, NULL));
+	cl_git_fail_with(
+		git_packbuilder_foreach(_packbuilder, foreach_cancel_cb, idx), -1111);
 	git_indexer_free(idx);
 }
diff --git a/tests/valgrind-supp-mac.txt b/tests/valgrind-supp-mac.txt
index 99833d0..0cdc975 100644
--- a/tests/valgrind-supp-mac.txt
+++ b/tests/valgrind-supp-mac.txt
@@ -103,14 +103,6 @@
 	fun:ssl23_connect
 }
 {
-	mac-ssl-uninitialized-4
-	Memcheck:Param
-	...
-	obj:/usr/lib/libcrypto.0.9.8.dylib
-	...
-	fun:ssl23_connect
-}
-{
 	mac-ssl-leak-1
 	Memcheck:Leak
 	...