From b71071313f4800840ecc48cb18e5cedec8ef250e Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Mon, 22 Jul 2013 11:01:19 -0700 Subject: [PATCH 1/4] git_reference_next_name must match git_reference_next The git_reference_next API silently skips invalid references when scanning the loose refs. The git_reference_next_name API should skip the same ones even though it isn't creating the reference object. This adds a test with a an invalid loose reference and makes sure that both APIs skip the same entries and generate the same results. --- src/refdb_fs.c | 16 +++++++++++++--- tests-clar/revwalk/basic.c | 22 ++++++++++++++++++++++ 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/src/refdb_fs.c b/src/refdb_fs.c index b9e283ac5..2f1a5b26c 100644 --- a/src/refdb_fs.c +++ b/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; } - *out = git_reference__alloc_symbolic(ref_name, target); + if (out) + *out = git_reference__alloc_symbolic(ref_name, target); } else { if ((error = loose_parse_oid(&oid, ref_name, &ref_file)) < 0) goto done; - *out = git_reference__alloc(ref_name, &oid, NULL); + if (out) + *out = git_reference__alloc(ref_name, &oid, NULL); } - if (*out == NULL) + if (out && *out == NULL) error = -1; done: @@ -679,6 +684,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; } diff --git a/tests-clar/revwalk/basic.c b/tests-clar/revwalk/basic.c index e82776260..7c2fc000d 100644 --- a/tests-clar/revwalk/basic.c +++ b/tests-clar/revwalk/basic.c @@ -142,6 +142,28 @@ 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; + + test_revwalk_basic__cleanup(); + + _repo = cl_git_sandbox_init("testrepo"); + cl_git_mkfile("testrepo/.git/refs/heads/garbage", "not-a-ref"); + + cl_git_pass(git_revwalk_new(&_walk, _repo)); + 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); + + cl_fixture_cleanup("testrepo"); +} + void test_revwalk_basic__push_head(void) { int i = 0; From c77342ef1c956f99f84f217359c73e8de65cdd1c Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Mon, 22 Jul 2013 11:20:34 -0700 Subject: [PATCH 2/4] Use pool for loose refdb string allocations Instead of using lots of strdup calls, this adds a memory pool to the loose refs iteration code and uses it for keeping track of the loose refs array. Memory usage could probably be reduced even further by eliminating the vector and just scanning by adding the strlen of each ref, but that would be a more intrusive changes. This also updates the error handling to be more thorough about checking for failed allocations, etc. --- src/refdb_fs.c | 55 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 33 insertions(+), 22 deletions(-) diff --git a/src/refdb_fs.c b/src/refdb_fs.c index 2f1a5b26c..acd82594b 100644 --- a/src/refdb_fs.c +++ b/src/refdb_fs.c @@ -560,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; @@ -568,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 */ @@ -594,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); @@ -618,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); @@ -727,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( @@ -792,7 +803,7 @@ static int reference_path_available( return -1; } }); - + return 0; } From 989710d9828ef0f90b494903bd1e1bd8b20a8914 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Mon, 22 Jul 2013 11:22:55 -0700 Subject: [PATCH 3/4] Fix warning message about mismatched types --- src/fileops.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/fileops.c b/src/fileops.c index db53d4fce..c01ff64db 100644 --- a/src/fileops.c +++ b/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; } From 4cee9b8618b949f55233f2350654d771a0dc2ece Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Mon, 22 Jul 2013 11:41:23 -0700 Subject: [PATCH 4/4] Update init and clean for revwalk::basic tests The new tests don't always want to use the same fixture data as the old ones so this makes it configurable on a per-test basis. --- tests-clar/revwalk/basic.c | 50 +++++++++++++++++++++++++++++--------- 1 file changed, 38 insertions(+), 12 deletions(-) diff --git a/tests-clar/revwalk/basic.c b/tests-clar/revwalk/basic.c index 7c2fc000d..cb8fcb9c7 100644 --- a/tests-clar/revwalk/basic.c +++ b/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; - git_repository_free(_repo); + + 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) { @@ -147,12 +168,9 @@ void test_revwalk_basic__glob_heads_with_invalid(void) int i; git_oid oid; - test_revwalk_basic__cleanup(); + revwalk_basic_setup_walk("testrepo"); - _repo = cl_git_sandbox_init("testrepo"); cl_git_mkfile("testrepo/.git/refs/heads/garbage", "not-a-ref"); - - cl_git_pass(git_revwalk_new(&_walk, _repo)); cl_git_pass(git_revwalk_push_glob(_walk, "heads")); for (i = 0; !git_revwalk_next(&oid, _walk); ++i) @@ -160,8 +178,6 @@ void test_revwalk_basic__glob_heads_with_invalid(void) /* git log --branches --oneline | wc -l => 16 */ cl_assert_equal_i(16, i); - - cl_fixture_cleanup("testrepo"); } void test_revwalk_basic__push_head(void) @@ -169,6 +185,8 @@ 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) { @@ -184,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")); @@ -200,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")); @@ -215,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"));