Commit cee695ae by Russell Belfer

Make iterators use GIT_ITEROVER & smart advance

1. internal iterators now return GIT_ITEROVER when you go past the
   last item in the iteration.
2. git_iterator_advance will "advance" to the first item in the
   iteration if it is called immediately after creating the
   iterator, which allows a simpler idiom for basic iteration.
3. if git_iterator_advance encounters an error reading data (e.g.
   a missing tree or an unreadable file), it returns the error
   but also attempts to advance past the invalid data to prevent
   an infinite loop.

Updated all tests and internal usage of iterators to account for
these new behaviors.
parent 1ed356dc
......@@ -495,12 +495,9 @@ static int checkout_action(
if (cmp == 0) {
if (wd->mode == GIT_FILEMODE_TREE) {
/* case 2 - entry prefixed by workdir tree */
if ((error = git_iterator_advance_into(&wd, workdir)) < 0) {
if (error != GIT_ENOTFOUND ||
git_iterator_advance(&wd, workdir) < 0)
error = git_iterator_advance_into_or_over(&wd, workdir);
if (error && error != GIT_ITEROVER)
goto fail;
}
*wditem_ptr = wd;
continue;
}
......@@ -515,8 +512,10 @@ static int checkout_action(
}
/* case 1 - handle wd item (if it matches pathspec) */
if (checkout_action_wd_only(data, workdir, wd, pathspec) < 0 ||
git_iterator_advance(&wd, workdir) < 0)
if (checkout_action_wd_only(data, workdir, wd, pathspec) < 0)
goto fail;
if ((error = git_iterator_advance(&wd, workdir)) < 0 &&
error != GIT_ITEROVER)
goto fail;
*wditem_ptr = wd;
......@@ -539,8 +538,9 @@ static int checkout_action(
if (delta->status == GIT_DELTA_TYPECHANGE) {
if (delta->old_file.mode == GIT_FILEMODE_TREE) {
act = checkout_action_with_wd(data, delta, wd);
if (git_iterator_advance_into(&wd, workdir) < 0)
wd = NULL;
if ((error = git_iterator_advance_into(&wd, workdir)) < 0 &&
error != GIT_ENOTFOUND)
goto fail;
*wditem_ptr = wd;
return act;
}
......@@ -550,8 +550,9 @@ static int checkout_action(
delta->old_file.mode == GIT_FILEMODE_COMMIT)
{
act = checkout_action_with_wd(data, delta, wd);
if (git_iterator_advance(&wd, workdir) < 0)
wd = NULL;
if ((error = git_iterator_advance(&wd, workdir)) < 0 &&
error != GIT_ITEROVER)
goto fail;
*wditem_ptr = wd;
return act;
}
......@@ -582,6 +583,9 @@ static int checkout_remaining_wd_items(
error = git_iterator_advance(&wd, workdir);
}
if (error == GIT_ITEROVER)
error = 0;
return error;
}
......@@ -603,7 +607,8 @@ static int checkout_get_actions(
git_pathspec_init(&pathspec, &data->opts.paths, &pathpool) < 0)
return -1;
if ((error = git_iterator_current(&wditem, workdir)) < 0)
if ((error = git_iterator_current(&wditem, workdir)) < 0 &&
error != GIT_ITEROVER)
goto fail;
deltas = &data->diff->deltas;
......
......@@ -796,6 +796,9 @@ static int diff_scan_inside_untracked_dir(
done:
git_buf_free(&base);
if (error == GIT_ITEROVER)
error = 0;
return error;
}
......@@ -981,8 +984,11 @@ static int handle_matched_item(
{
int error = 0;
if (!(error = maybe_modified(diff, info)) &&
!(error = git_iterator_advance(&info->oitem, info->old_iter)))
if ((error = maybe_modified(diff, info)) < 0)
return error;
if (!(error = git_iterator_advance(&info->oitem, info->old_iter)) ||
error == GIT_ITEROVER)
error = git_iterator_advance(&info->nitem, info->new_iter);
return error;
......@@ -1011,15 +1017,22 @@ int git_diff__from_iterators(
/* make iterators have matching icase behavior */
if (DIFF_FLAG_IS_SET(diff, GIT_DIFF_DELTAS_ARE_ICASE)) {
if (!(error = git_iterator_set_ignore_case(old_iter, true)))
error = git_iterator_set_ignore_case(new_iter, true);
if ((error = git_iterator_set_ignore_case(old_iter, true)) < 0 ||
(error = git_iterator_set_ignore_case(new_iter, true)) < 0)
goto cleanup;
}
/* finish initialization */
if (!error &&
!(error = diff_list_apply_options(diff, opts)) &&
!(error = git_iterator_current(&info.oitem, old_iter)))
error = git_iterator_current(&info.nitem, new_iter);
if ((error = diff_list_apply_options(diff, opts)) < 0)
goto cleanup;
if ((error = git_iterator_current(&info.oitem, old_iter)) < 0 &&
error != GIT_ITEROVER)
goto cleanup;
if ((error = git_iterator_current(&info.nitem, new_iter)) < 0 &&
error != GIT_ITEROVER)
goto cleanup;
error = 0;
/* run iterators building diffs */
while (!error && (info.oitem || info.nitem)) {
......@@ -1041,8 +1054,13 @@ int git_diff__from_iterators(
*/
else
error = handle_matched_item(diff, &info);
/* because we are iterating over two lists, ignore ITEROVER */
if (error == GIT_ITEROVER)
error = 0;
}
cleanup:
if (!error)
*diff_ptr = diff;
else
......
......@@ -142,9 +142,9 @@ GIT_INLINE(int) git_iterator_advance(
*
* If the current item is not a tree, this is a no-op.
*
* For working directory iterators only, a tree (i.e. directory) can be
* empty. In that case, this function returns GIT_ENOTFOUND and does not
* advance. That can't happen for tree and index iterators.
* For filesystem and working directory iterators, a tree (i.e. directory)
* can be empty. In that case, this function returns GIT_ENOTFOUND and
* does not advance. That can't happen for tree and index iterators.
*/
GIT_INLINE(int) git_iterator_advance_into(
const git_index_entry **entry, git_iterator *iter)
......@@ -152,18 +152,50 @@ GIT_INLINE(int) git_iterator_advance_into(
return iter->cb->advance_into(entry, iter);
}
/**
* Advance into a tree or skip over it if it is empty.
*
* Because `git_iterator_advance_into` may return GIT_ENOTFOUND if the
* directory is empty (only with filesystem and working directory
* iterators) and a common response is to just call `git_iterator_advance`
* when that happens, this bundles the two into a single simple call.
*/
GIT_INLINE(int) git_iterator_advance_into_or_over(
const git_index_entry **entry, git_iterator *iter)
{
int error = iter->cb->advance_into(entry, iter);
if (error == GIT_ENOTFOUND) {
giterr_clear();
error = iter->cb->advance(entry, iter);
}
return error;
}
/* Seek is currently unimplemented */
GIT_INLINE(int) git_iterator_seek(
git_iterator *iter, const char *prefix)
{
return iter->cb->seek(iter, prefix);
}
/**
* Go back to the start of the iteration.
*
* This resets the iterator to the start of the iteration. It also allows
* you to reset the `start` and `end` pathname boundaries of the iteration
* when doing so.
*/
GIT_INLINE(int) git_iterator_reset(
git_iterator *iter, const char *start, const char *end)
{
return iter->cb->reset(iter, start, end);
}
/**
* Check if the iterator is at the end
*
* @return 0 if not at end, >0 if at end
*/
GIT_INLINE(int) git_iterator_at_end(git_iterator *iter)
{
return iter->cb->at_end(iter);
......
......@@ -1259,7 +1259,8 @@ int git_merge_diff_list__find_differences(
/* Set up the iterators */
for (i = 0; i < 3; i++) {
if ((error = git_iterator_current(&items[i], iterators[i])) < 0)
error = git_iterator_current(&items[i], iterators[i]);
if (error < 0 && error != GIT_ITEROVER)
goto done;
}
......@@ -1313,11 +1314,16 @@ int git_merge_diff_list__find_differences(
error = merge_index_insert_conflict(diff_list, &df_data, cur_items);
else
error = merge_index_insert_unmodified(diff_list, cur_items);
if (error < 0)
goto done;
/* Advance each iterator that participated */
for (i = 0; i < 3; i++) {
if (cur_items[i] != NULL &&
(error = git_iterator_advance(&items[i], iterators[i])) < 0)
if (cur_items[i] == NULL)
continue;
error = git_iterator_advance(&items[i], iterators[i]);
if (error < 0 && error != GIT_ITEROVER)
goto done;
}
}
......@@ -1326,6 +1332,9 @@ done:
for (i = 0; i < 3; i++)
git_iterator_free(iterators[i]);
if (error == GIT_ITEROVER)
error = 0;
return error;
}
......
......@@ -647,18 +647,12 @@ int git_note_next(
const git_index_entry *item;
if ((error = git_iterator_current(&item, it)) < 0)
goto exit;
return error;
if (item != NULL) {
git_oid_cpy(note_id, &item->oid);
error = process_entry_path(item->path, annotated_id);
if (error >= 0)
error = git_iterator_advance(NULL, it);
} else {
error = GIT_ITEROVER;
}
if (!(error = process_entry_path(item->path, annotated_id)))
git_iterator_advance(NULL, it);
exit:
return error;
}
......@@ -582,6 +582,9 @@ static int iter_load_loose_paths(refdb_fs_backend *backend, refdb_fs_iter *iter)
git_iterator *fsit;
const git_index_entry *entry = NULL;
if (!backend->path) /* do nothing if no path for loose refs */
return 0;
if (git_buf_printf(&path, "%s/refs", backend->path) < 0)
return -1;
......@@ -591,7 +594,7 @@ static int iter_load_loose_paths(refdb_fs_backend *backend, refdb_fs_iter *iter)
git_vector_init(&iter->loose, 8, NULL);
git_buf_sets(&path, GIT_REFS_DIR);
while (!git_iterator_current(&entry, fsit) && entry) {
while (!git_iterator_advance(&entry, fsit)) {
const char *ref_name;
khiter_t pos;
......@@ -600,10 +603,8 @@ static int iter_load_loose_paths(refdb_fs_backend *backend, refdb_fs_iter *iter)
ref_name = git_buf_cstr(&path);
if (git__suffixcmp(ref_name, ".lock") == 0 ||
(iter->glob && p_fnmatch(iter->glob, ref_name, 0) != 0)) {
git_iterator_advance(NULL, fsit);
(iter->glob && p_fnmatch(iter->glob, ref_name, 0) != 0))
continue;
}
pos = git_strmap_lookup_index(packfile, ref_name);
if (git_strmap_valid_index(packfile, pos)) {
......@@ -612,7 +613,6 @@ static int iter_load_loose_paths(refdb_fs_backend *backend, refdb_fs_iter *iter)
}
git_vector_insert(&iter->loose, git__strdup(ref_name));
git_iterator_advance(NULL, fsit);
}
git_iterator_free(fsit);
......
......@@ -1143,9 +1143,7 @@ static int load_submodule_config_from_index(
(error = git_iterator_for_index(&i, index, 0, NULL, NULL)) < 0)
return error;
error = git_iterator_current(&entry, i);
while (!error && entry != NULL) {
while (!(error = git_iterator_advance(&entry, i))) {
if (S_ISGITLINK(entry->mode)) {
error = submodule_load_from_index(repo, entry);
......@@ -1158,10 +1156,11 @@ static int load_submodule_config_from_index(
if (strcmp(entry->path, GIT_MODULES_FILE) == 0)
git_oid_cpy(gitmodules_oid, &entry->oid);
}
error = git_iterator_advance(&entry, i);
}
if (error == GIT_ITEROVER)
error = 0;
git_iterator_free(i);
return error;
......@@ -1183,9 +1182,7 @@ static int load_submodule_config_from_head(
return error;
}
error = git_iterator_current(&entry, i);
while (!error && entry != NULL) {
while (!(error = git_iterator_advance(&entry, i))) {
if (S_ISGITLINK(entry->mode)) {
error = submodule_load_from_head(repo, entry->path, &entry->oid);
......@@ -1199,10 +1196,11 @@ static int load_submodule_config_from_head(
git_oid_iszero(gitmodules_oid))
git_oid_cpy(gitmodules_oid, &entry->oid);
}
error = git_iterator_advance(&entry, i);
}
if (error == GIT_ITEROVER)
error = 0;
git_iterator_free(i);
git_tree_free(head);
......
......@@ -31,7 +31,7 @@ static void tree_iterator_test(
git_tree *t;
git_iterator *i;
const git_index_entry *entry;
int count = 0, count_post_reset = 0;
int error, count = 0, count_post_reset = 0;
git_repository *repo = cl_git_sandbox_init(sandbox);
cl_assert(t = resolve_commit_oid_to_tree(repo, treeish));
......@@ -39,29 +39,30 @@ static void tree_iterator_test(
&i, t, GIT_ITERATOR_DONT_IGNORE_CASE, start, end));
/* test loop */
cl_git_pass(git_iterator_current(&entry, i));
while (entry != NULL) {
while (!(error = git_iterator_advance(&entry, i))) {
cl_assert(entry);
if (expected_values != NULL)
cl_assert_equal_s(expected_values[count], entry->path);
count++;
cl_git_pass(git_iterator_advance(&entry, i));
}
cl_assert_equal_i(GIT_ITEROVER, error);
cl_assert(!entry);
cl_assert_equal_i(expected_count, count);
/* test reset */
cl_git_pass(git_iterator_reset(i, NULL, NULL));
cl_git_pass(git_iterator_current(&entry, i));
while (entry != NULL) {
while (!(error = git_iterator_advance(&entry, i))) {
cl_assert(entry);
if (expected_values != NULL)
cl_assert_equal_s(expected_values[count_post_reset], entry->path);
count_post_reset++;
cl_git_pass(git_iterator_advance(&entry, i));
}
git_iterator_free(i);
cl_assert_equal_i(expected_count, count);
cl_assert_equal_i(GIT_ITEROVER, error);
cl_assert(!entry);
cl_assert_equal_i(count, count_post_reset);
git_iterator_free(i);
git_tree_free(t);
}
......@@ -298,7 +299,7 @@ void test_diff_iterator__tree_special_functions(void)
git_iterator *i;
const git_index_entry *entry;
git_repository *repo = cl_git_sandbox_init("attr");
int cases = 0;
int error, cases = 0;
const char *rootoid = "ce39a97a7fb1fa90bcf5e711249c1e507476ae0e";
t = resolve_commit_oid_to_tree(
......@@ -307,9 +308,10 @@ void test_diff_iterator__tree_special_functions(void)
cl_git_pass(git_iterator_for_tree(
&i, t, GIT_ITERATOR_DONT_IGNORE_CASE, NULL, NULL));
cl_git_pass(git_iterator_current(&entry, i));
while (entry != NULL) {
while (!(error = git_iterator_advance(&entry, i))) {
cl_assert(entry);
if (strcmp(entry->path, "sub/file") == 0) {
cases++;
check_tree_entry(
......@@ -338,11 +340,11 @@ void test_diff_iterator__tree_special_functions(void)
"2929de282ce999e95183aedac6451d3384559c4b",
rootoid, NULL);
}
cl_git_pass(git_iterator_advance(&entry, i));
}
cl_assert_equal_i(GIT_ITEROVER, error);
cl_assert(!entry);
cl_assert_equal_i(4, cases);
git_iterator_free(i);
git_tree_free(t);
}
......@@ -360,14 +362,15 @@ static void index_iterator_test(
git_index *index;
git_iterator *i;
const git_index_entry *entry;
int count = 0;
int error, count = 0;
git_repository *repo = cl_git_sandbox_init(sandbox);
cl_git_pass(git_repository_index(&index, repo));
cl_git_pass(git_iterator_for_index(&i, index, 0, start, end));
cl_git_pass(git_iterator_current(&entry, i));
while (entry != NULL) {
while (!(error = git_iterator_advance(&entry, i))) {
cl_assert(entry);
if (expected_names != NULL)
cl_assert_equal_s(expected_names[count], entry->path);
......@@ -378,13 +381,14 @@ static void index_iterator_test(
}
count++;
cl_git_pass(git_iterator_advance(&entry, i));
}
cl_assert_equal_i(GIT_ITEROVER, error);
cl_assert(!entry);
cl_assert_equal_i(expected_count, count);
git_iterator_free(i);
git_index_free(index);
cl_assert_equal_i(expected_count, count);
}
static const char *expected_index_0[] = {
......@@ -535,12 +539,15 @@ static void workdir_iterator_test(
{
git_iterator *i;
const git_index_entry *entry;
int count = 0, count_all = 0, count_all_post_reset = 0;
int error, count = 0, count_all = 0, count_all_post_reset = 0;
git_repository *repo = cl_git_sandbox_init(sandbox);
cl_git_pass(git_iterator_for_workdir(
&i, repo, GIT_ITERATOR_DONT_AUTOEXPAND, start, end));
cl_git_pass(git_iterator_current(&entry, i));
error = git_iterator_current(&entry, i);
cl_assert((error == 0 && entry != NULL) ||
(error == GIT_ITEROVER && entry == NULL));
while (entry != NULL) {
int ignored = git_iterator_current_is_ignored(i);
......@@ -560,29 +567,39 @@ static void workdir_iterator_test(
count++;
count_all++;
cl_git_pass(git_iterator_advance(&entry, i));
error = git_iterator_advance(&entry, i);
cl_assert((error == 0 && entry != NULL) ||
(error == GIT_ITEROVER && entry == NULL));
}
cl_assert_equal_i(expected_count, count);
cl_assert_equal_i(expected_count + expected_ignores, count_all);
cl_git_pass(git_iterator_reset(i, NULL, NULL));
cl_git_pass(git_iterator_current(&entry, i));
error = git_iterator_current(&entry, i);
cl_assert((error == 0 && entry != NULL) ||
(error == GIT_ITEROVER && entry == NULL));
while (entry != NULL) {
if (S_ISDIR(entry->mode)) {
cl_git_pass(git_iterator_advance_into(&entry, i));
continue;
}
if (expected_names != NULL)
cl_assert_equal_s(
expected_names[count_all_post_reset], entry->path);
count_all_post_reset++;
cl_git_pass(git_iterator_advance(&entry, i));
}
git_iterator_free(i);
error = git_iterator_advance(&entry, i);
cl_assert(error == 0 || error == GIT_ITEROVER);
}
cl_assert_equal_i(expected_count, count);
cl_assert_equal_i(expected_count + expected_ignores, count_all);
cl_assert_equal_i(count_all, count_all_post_reset);
git_iterator_free(i);
}
void test_diff_iterator__workdir_0(void)
......@@ -752,8 +769,10 @@ void test_diff_iterator__workdir_builtin_ignores(void)
{
/* it is possible to advance "into" a submodule */
cl_git_pass(git_iterator_advance_into(&entry, i));
} else
cl_git_pass(git_iterator_advance(&entry, i));
} else {
int error = git_iterator_advance(&entry, i);
cl_assert(!error || error == GIT_ITEROVER);
}
}
cl_assert(expected[idx].path == NULL);
......@@ -766,7 +785,7 @@ static void check_wd_first_through_third_range(
{
git_iterator *i;
const git_index_entry *entry;
int idx;
int error, idx;
static const char *expected[] = { "FIRST", "second", "THIRD", NULL };
cl_git_pass(git_iterator_for_workdir(
......@@ -776,7 +795,8 @@ static void check_wd_first_through_third_range(
for (idx = 0; entry != NULL; ++idx) {
cl_assert_equal_s(expected[idx], entry->path);
cl_git_pass(git_iterator_advance(&entry, i));
error = git_iterator_advance(&entry, i);
cl_assert(!error || error == GIT_ITEROVER);
}
cl_assert(expected[idx] == NULL);
......@@ -814,8 +834,7 @@ static void check_tree_range(
{
git_tree *head;
git_iterator *i;
const git_index_entry *entry;
int count;
int error, count;
cl_git_pass(git_repository_head_tree(&head, repo));
......@@ -824,13 +843,10 @@ static void check_tree_range(
ignore_case ? GIT_ITERATOR_IGNORE_CASE : GIT_ITERATOR_DONT_IGNORE_CASE,
start, end));
cl_git_pass(git_iterator_current(&entry, i));
for (count = 0; entry != NULL; ) {
++count;
cl_git_pass(git_iterator_advance(&entry, i));
}
for (count = 0; !(error = git_iterator_advance(NULL, i)); ++count)
/* count em up */;
cl_assert_equal_i(GIT_ITEROVER, error);
cl_assert_equal_i(expected_count, count);
git_iterator_free(i);
......@@ -872,8 +888,7 @@ static void check_index_range(
{
git_index *index;
git_iterator *i;
const git_index_entry *entry;
int count, caps;
int error, count, caps;
bool is_ignoring_case;
cl_git_pass(git_repository_index(&index, repo));
......@@ -888,13 +903,10 @@ static void check_index_range(
cl_assert(git_iterator_ignore_case(i) == ignore_case);
cl_git_pass(git_iterator_current(&entry, i));
for (count = 0; entry != NULL; ) {
++count;
cl_git_pass(git_iterator_advance(&entry, i));
}
for (count = 0; !(error = git_iterator_advance(NULL, i)); ++count)
/* count em up */;
cl_assert_equal_i(GIT_ITEROVER, error);
cl_assert_equal_i(expected_count, count);
git_iterator_free(i);
......
......@@ -31,12 +31,11 @@ static void expect_iterator_items(
if (expected_flat < 0) { v = true; expected_flat = -expected_flat; }
if (expected_total < 0) { v = true; expected_total = -expected_total; }
count = 0;
cl_git_pass(git_iterator_current(&entry, i));
if (v) fprintf(stderr, "== %s ==\n", no_trees ? "notrees" : "trees");
while (entry != NULL) {
count = 0;
while (!git_iterator_advance(&entry, i)) {
if (v) fprintf(stderr, " %s %07o\n", entry->path, (int)entry->mode);
if (no_trees)
......@@ -54,8 +53,6 @@ static void expect_iterator_items(
cl_assert(entry->mode != GIT_FILEMODE_TREE);
}
cl_git_pass(git_iterator_advance(&entry, i));
if (++count > expected_flat)
break;
}
......@@ -93,10 +90,14 @@ static void expect_iterator_items(
/* could return NOTFOUND if directory is empty */
cl_assert(!error || error == GIT_ENOTFOUND);
if (error == GIT_ENOTFOUND)
cl_git_pass(git_iterator_advance(&entry, i));
} else
cl_git_pass(git_iterator_advance(&entry, i));
if (error == GIT_ENOTFOUND) {
error = git_iterator_advance(&entry, i);
cl_assert(!error || error == GIT_ITEROVER);
}
} else {
error = git_iterator_advance(&entry, i);
cl_assert(!error || error == GIT_ITEROVER);
}
if (++count > expected_total)
break;
......
......@@ -370,12 +370,9 @@ void test_submodule_status__iterator(void)
cl_git_pass(git_iterator_for_workdir(&iter, g_repo,
GIT_ITERATOR_IGNORE_CASE | GIT_ITERATOR_INCLUDE_TREES, NULL, NULL));
cl_git_pass(git_iterator_current(&entry, iter));
for (i = 0; entry; ++i) {
for (i = 0; !git_iterator_advance(&entry, iter); ++i)
cl_assert_equal_s(expected[i], entry->path);
cl_git_pass(git_iterator_advance(&entry, iter));
}
git_iterator_free(iter);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment