Merge pull request #1732 from libgit2/revwalk-glob-should-ignore-invalid

Invalid refs on disk cause revwalk globbing to fail

src/fileops.c

@@ -630,13 +630,11 @@ static git_futils_dirs_guess_cb git_futils__dir_guess[GIT_FUTILS_DIR__MAX] = {
 int git_futils_dirs_global_init(void)
 {
 	git_futils_dir_t i;
-	git_buf *path;
+	const git_buf *path;
 	int error = 0;
 
-	for (i = 0; i < GIT_FUTILS_DIR__MAX; i++) {
-		if ((error = git_futils_dirs_get(&path, i)) < 0)
-			break;
-	}
+	for (i = 0; !error && i < GIT_FUTILS_DIR__MAX; i++)
+		error = git_futils_dirs_get(&path, i);
 
 	return error;
 }

src/refdb_fs.c

@@ -452,6 +452,9 @@ static int loose_lookup(
 	git_buf ref_file = GIT_BUF_INIT;
 	int error = 0;
 
+	if (out)
+		*out = NULL;
+
 	error = reference_read(&ref_file, NULL, backend->path, ref_name, NULL);
 
 	if (error < 0)
@@ -465,15 +468,17 @@ static int loose_lookup(
 			goto done;
 		}
 
+		if (out)
 			*out = git_reference__alloc_symbolic(ref_name, target);
 	} else {
 		if ((error = loose_parse_oid(&oid, ref_name, &ref_file)) < 0)
 			goto done;
 
+		if (out)
 			*out = git_reference__alloc(ref_name, &oid, NULL);
 	}
 
-	if (*out == NULL)
+	if (out && *out == NULL)
 		error = -1;
 
 done:
@@ -555,7 +560,10 @@ typedef struct {
 	git_reference_iterator parent;
 
 	char *glob;
+
+	git_pool pool;
 	git_vector loose;
+
 	unsigned int loose_pos;
 	khiter_t packed_pos;
 } refdb_fs_iter;
@@ -563,24 +571,18 @@ typedef struct {
 static void refdb_fs_backend__iterator_free(git_reference_iterator *_iter)
 {
 	refdb_fs_iter *iter = (refdb_fs_iter *) _iter;
-	char *loose_path;
-	size_t i;
-
-	git_vector_foreach(&iter->loose, i, loose_path) {
-		git__free(loose_path);
-	}
 
 	git_vector_free(&iter->loose);
-
-	git__free(iter->glob);
+	git_pool_clear(&iter->pool);
 	git__free(iter);
 }
 
 static int iter_load_loose_paths(refdb_fs_backend *backend, refdb_fs_iter *iter)
 {
+	int error = 0;
 	git_strmap *packfile = backend->refcache.packfile;
 	git_buf path = GIT_BUF_INIT;
-	git_iterator *fsit;
+	git_iterator *fsit = NULL;
 	const git_index_entry *entry = NULL;
 
 	if (!backend->path) /* do nothing if no path for loose refs */
@@ -589,15 +591,16 @@ static int iter_load_loose_paths(refdb_fs_backend *backend, refdb_fs_iter *iter)
 	if (git_buf_printf(&path, "%s/refs", backend->path) < 0)
 		return -1;
 
-	if (git_iterator_for_filesystem(&fsit, git_buf_cstr(&path), 0, NULL, NULL) < 0)
-		return -1;
-
-	git_vector_init(&iter->loose, 8, NULL);
-	git_buf_sets(&path, GIT_REFS_DIR);
+	if ((error = git_iterator_for_filesystem(
+			&fsit, git_buf_cstr(&path), 0, NULL, NULL)) < 0 ||
+		(error = git_vector_init(&iter->loose, 8, NULL)) < 0 ||
+		(error = git_buf_sets(&path, GIT_REFS_DIR)) < 0)
+		goto cleanup;
 
 	while (!git_iterator_advance(&entry, fsit)) {
 		const char *ref_name;
 		khiter_t pos;
+		char *ref_dup;
 
 		git_buf_truncate(&path, strlen(GIT_REFS_DIR));
 		git_buf_puts(&path, entry->path);
@@ -613,9 +616,16 @@ static int iter_load_loose_paths(refdb_fs_backend *backend, refdb_fs_iter *iter)
 			ref->flags |= PACKREF_SHADOWED;
 		}
 
-		git_vector_insert(&iter->loose, git__strdup(ref_name));
+		if (!(ref_dup = git_pool_strdup(&iter->pool, ref_name))) {
+			error = -1;
+			goto cleanup;
 		}
 
+		if ((error = git_vector_insert(&iter->loose, ref_dup)) < 0)
+			goto cleanup;
+	}
+
+cleanup:
 	git_iterator_free(fsit);
 	git_buf_free(&path);
 
@@ -679,6 +689,11 @@ static int refdb_fs_backend__iterator_next_name(
 		if (git_strmap_exists(packfile, path))
 			continue;
 
+		if (loose_lookup(NULL, backend, path) != 0) {
+			giterr_clear();
+			continue;
+		}
+
 		*out = path;
 		return 0;
 	}
@@ -717,20 +732,26 @@ static int refdb_fs_backend__iterator(
 	iter = git__calloc(1, sizeof(refdb_fs_iter));
 	GITERR_CHECK_ALLOC(iter);
 
-	if (glob != NULL)
-		iter->glob = git__strdup(glob);
+	if (git_pool_init(&iter->pool, 1, 0) < 0)
+		goto fail;
+
+	if (glob != NULL &&
+		(iter->glob = git_pool_strdup(&iter->pool, glob)) == NULL)
+		goto fail;
 
 	iter->parent.next = refdb_fs_backend__iterator_next;
 	iter->parent.next_name = refdb_fs_backend__iterator_next_name;
 	iter->parent.free = refdb_fs_backend__iterator_free;
 
-	if (iter_load_loose_paths(backend, iter) < 0) {
-		refdb_fs_backend__iterator_free((git_reference_iterator *)iter);
-		return -1;
-	}
+	if (iter_load_loose_paths(backend, iter) < 0)
+		goto fail;
 
 	*out = (git_reference_iterator *)iter;
 	return 0;
+
+fail:
+	refdb_fs_backend__iterator_free((git_reference_iterator *)iter);
+	return -1;
 }
 
 static bool ref_is_available(

tests-clar/revwalk/basic.c

@@ -98,27 +98,46 @@ static int test_walk(git_revwalk *walk, const git_oid *root,
 	return test_walk_only(walk, possible_results, results_count);
 }
 
-static git_repository *_repo;
-static git_revwalk *_walk;
+static git_repository *_repo = NULL;
+static git_revwalk *_walk = NULL;
+static const char *_fixture = NULL;
 
 void test_revwalk_basic__initialize(void)
 {
-	cl_git_pass(git_repository_open(&_repo, cl_fixture("testrepo.git")));
-	cl_git_pass(git_revwalk_new(&_walk, _repo));
 }
 
 void test_revwalk_basic__cleanup(void)
 {
 	git_revwalk_free(_walk);
-	_walk = NULL;
+
+	if (_fixture)
+		cl_git_sandbox_cleanup();
+	else
 		git_repository_free(_repo);
+
+	_fixture = NULL;
 	_repo = NULL;
+	_walk = NULL;
+}
+
+static void revwalk_basic_setup_walk(const char *fixture)
+{
+	if (fixture) {
+		_fixture = fixture;
+		_repo = cl_git_sandbox_init(fixture);
+	} else {
+		cl_git_pass(git_repository_open(&_repo, cl_fixture("testrepo.git")));
+	}
+
+	cl_git_pass(git_revwalk_new(&_walk, _repo));
 }
 
 void test_revwalk_basic__sorting_modes(void)
 {
 	git_oid id;
 
+	revwalk_basic_setup_walk(NULL);
+
 	git_oid_fromstr(&id, commit_head);
 
 	cl_git_pass(test_walk(_walk, &id, GIT_SORT_TIME, commit_sorting_time, 1));
@@ -132,6 +151,8 @@ void test_revwalk_basic__glob_heads(void)
 	int i = 0;
 	git_oid oid;
 
+	revwalk_basic_setup_walk(NULL);
+
 	cl_git_pass(git_revwalk_push_glob(_walk, "heads"));
 
 	while (git_revwalk_next(&oid, _walk) == 0) {
@@ -142,11 +163,30 @@ void test_revwalk_basic__glob_heads(void)
 	cl_assert(i == 14);
 }
 
+void test_revwalk_basic__glob_heads_with_invalid(void)
+{
+	int i;
+	git_oid oid;
+
+	revwalk_basic_setup_walk("testrepo");
+
+	cl_git_mkfile("testrepo/.git/refs/heads/garbage", "not-a-ref");
+	cl_git_pass(git_revwalk_push_glob(_walk, "heads"));
+
+	for (i = 0; !git_revwalk_next(&oid, _walk); ++i)
+		/* walking */;
+
+	/* git log --branches --oneline | wc -l => 16 */
+	cl_assert_equal_i(16, i);
+}
+
 void test_revwalk_basic__push_head(void)
 {
 	int i = 0;
 	git_oid oid;
 
+	revwalk_basic_setup_walk(NULL);
+
 	cl_git_pass(git_revwalk_push_head(_walk));
 
 	while (git_revwalk_next(&oid, _walk) == 0) {
@@ -162,6 +202,8 @@ void test_revwalk_basic__push_head_hide_ref(void)
 	int i = 0;
 	git_oid oid;
 
+	revwalk_basic_setup_walk(NULL);
+
 	cl_git_pass(git_revwalk_push_head(_walk));
 	cl_git_pass(git_revwalk_hide_ref(_walk, "refs/heads/packed-test"));
 
@@ -178,6 +220,8 @@ void test_revwalk_basic__push_head_hide_ref_nobase(void)
 	int i = 0;
 	git_oid oid;
 
+	revwalk_basic_setup_walk(NULL);
+
 	cl_git_pass(git_revwalk_push_head(_walk));
 	cl_git_pass(git_revwalk_hide_ref(_walk, "refs/heads/packed"));
 
@@ -193,12 +237,16 @@ void test_revwalk_basic__disallow_non_commit(void)
 {
 	git_oid oid;
 
+	revwalk_basic_setup_walk(NULL);
+
 	cl_git_pass(git_oid_fromstr(&oid, "521d87c1ec3aef9824daf6d96cc0ae3710766d91"));
 	cl_git_fail(git_revwalk_push(_walk, &oid));
 }
 
 void test_revwalk_basic__push_range(void)
 {
+	revwalk_basic_setup_walk(NULL);
+
 	git_revwalk_reset(_walk);
 	git_revwalk_sorting(_walk, 0);
 	cl_git_pass(git_revwalk_push_range(_walk, "9fd738e~2..9fd738e"));