Repository navigation
fix(refdb): propagate reference iterator errors - #7453
Open
Pallav-Biyala wants to merge 2 commits into
Open
Pallav-Biyala wants to merge 2 commits into
Pallav-Biyala wants to merge 2 commits into
Conversation
|
Thanks for picking this up! I reported #7451 and tried this branch (9719464) on Windows (MSVC) and on Linux (Ubuntu 24.04, as a non-root user), using the tests in the patch below:
With the patch, all Three small points:
Patch (applies on 9719464)diff --git a/src/libgit2/refdb_fs.c b/src/libgit2/refdb_fs.c
index fd5c37fab..e72a83897 100644
--- a/src/libgit2/refdb_fs.c
+++ b/src/libgit2/refdb_fs.c
@@ -1042,6 +1042,17 @@ done:
return error;
}
+/*
+ * Skip a loose ref that vanished or whose file is corrupt, as git ignores
+ * broken refs; fail on any other error (path too long, I/O) instead of
+ * silently returning a shorter list.
+ */
+static bool iter_loose_lookup_skippable(int error)
+{
+ return error == GIT_ENOTFOUND ||
+ git_error_last()->klass == GIT_ERROR_REFERENCE;
+}
+
static int refdb_fs_backend__iterator_next(
git_reference **out, git_reference_iterator *_iter)
{
@@ -1051,22 +1062,21 @@ static int refdb_fs_backend__iterator_next(
struct packref *ref;
while (iter->loose_pos < iter->loose.length) {
- const char *path = git_vector_get(&iter->loose, iter->loose_pos++);
- int lookup_error;
+ const char *path = git_vector_get(&iter->loose, iter->loose_pos++);
- if ((lookup_error = loose_lookup(out, backend, path)) == 0) {
- ref = git_sortedcache_lookup(iter->cache, path);
- if (ref)
- ref->flags |= PACKREF_SHADOWED;
+ if ((error = loose_lookup(out, backend, path)) == 0) {
+ ref = git_sortedcache_lookup(iter->cache, path);
+ if (ref)
+ ref->flags |= PACKREF_SHADOWED;
- return 0;
- }
+ return 0;
+ }
- if (lookup_error != GIT_ENOTFOUND)
- return lookup_error;
+ if (!iter_loose_lookup_skippable(error))
+ return error;
- git_error_clear();
- }
+ git_error_clear();
+ }
error = GIT_ITEROVER;
while (iter->packed_pos < git_sortedcache_entrycount(iter->cache)) {
@@ -1096,23 +1106,22 @@ static int refdb_fs_backend__iterator_next_name(
struct packref *ref;
while (iter->loose_pos < iter->loose.length) {
- const char *path = git_vector_get(&iter->loose, iter->loose_pos++);
- int lookup_error;
+ const char *path = git_vector_get(&iter->loose, iter->loose_pos++);
- if ((lookup_error = loose_lookup(NULL, backend, path)) == 0) {
- ref = git_sortedcache_lookup(iter->cache, path);
- if (ref)
- ref->flags |= PACKREF_SHADOWED;
+ if ((error = loose_lookup(NULL, backend, path)) == 0) {
+ ref = git_sortedcache_lookup(iter->cache, path);
+ if (ref)
+ ref->flags |= PACKREF_SHADOWED;
- *out = path;
- return 0;
- }
+ *out = path;
+ return 0;
+ }
- if (lookup_error != GIT_ENOTFOUND)
- return lookup_error;
+ if (!iter_loose_lookup_skippable(error))
+ return error;
- git_error_clear();
- }
+ git_error_clear();
+ }
error = GIT_ITEROVER;
while (iter->packed_pos < git_sortedcache_entrycount(iter->cache)) {
diff --git a/tests/libgit2/refs/iterator.c b/tests/libgit2/refs/iterator.c
index 0b2e65a71..9f859182d 100644
--- a/tests/libgit2/refs/iterator.c
+++ b/tests/libgit2/refs/iterator.c
@@ -276,3 +276,79 @@ void test_refs_iterator__concurrent_delete(void)
cl_assert_equal_i(full_count, concurrent_count);
}
+
+void test_refs_iterator__skips_corrupted_loose_ref(void)
+{
+ git_reference_iterator *iter;
+ git_vector output;
+ const char *name;
+ int error;
+
+ if (!cl_repo_has_ref_format(repo, "files"))
+ cl_skip();
+
+ /* an empty ref file, as left behind by a crash during a write */
+ cl_git_mkfile("testrepo.git/refs/heads/broken", "");
+
+ cl_git_pass(git_vector_init(&output, 33, &refcmp_cb));
+ cl_git_pass(git_reference_foreach(repo, refs_foreach_cb, &output));
+ assert_all_refnames_match(refnames, &output);
+
+ cl_git_pass(git_reference_iterator_new(&iter, repo));
+ while ((error = git_reference_next_name(&name, iter)) == 0)
+ cl_assert(strcmp(name, "refs/heads/broken") != 0);
+ git_reference_iterator_free(iter);
+ cl_assert_equal_i(GIT_ITEROVER, error);
+}
+
+void test_refs_iterator__unreadable_loose_ref_fails(void)
+{
+ git_reference_iterator *iter;
+ const char *name;
+ int error;
+
+#ifdef GIT_WIN32
+ cl_skip();
+#else
+ if (geteuid() == 0)
+ cl_skip();
+#endif
+ if (!cl_repo_has_ref_format(repo, "files"))
+ cl_skip();
+
+ cl_must_pass(p_chmod("testrepo.git/refs/heads/br2", 0000));
+
+ cl_git_pass(git_reference_iterator_new(&iter, repo));
+ while ((error = git_reference_next_name(&name, iter)) == 0)
+ ;
+ git_reference_iterator_free(iter);
+
+ cl_must_pass(p_chmod("testrepo.git/refs/heads/br2", 0644));
+ cl_git_fail(error);
+ cl_assert(error != GIT_ITEROVER);
+}
+
+void test_refs_iterator__long_loose_ref_path_fails(void)
+{
+#ifndef GIT_WIN32
+ cl_skip();
+#else
+ git_reference_iterator *iter;
+ git_str path = GIT_STR_INIT;
+
+ if (!cl_repo_has_ref_format(repo, "files"))
+ cl_skip();
+
+ /* a loose ref beyond MAX_PATH, as git writes with core.longpaths */
+ cl_git_pass(git_str_joinpath(&path, git_repository_path(repo), "refs/heads/long"));
+ while (path.size <= MAX_PATH)
+ cl_git_pass(git_str_puts(&path, "/xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"));
+ cl_git_pass(git_futils_mkpath2file(path.ptr, 0777));
+ cl_git_mkfile(path.ptr, "a65fedf39aefe402d3bb6e24df4d4f5fe4547750\n");
+
+ cl_git_fail_with(git_reference_iterator_new(&iter, repo), GIT_ERROR);
+ cl_assert_equal_i(GIT_ERROR_FILESYSTEM, git_error_last()->klass);
+
+ git_str_dispose(&path);
+#endif
+}Take any of it as it is, or adapt it however you like. Raised with the help of an AI assistant (Claude) while debugging DMW. |
Author
|
Thanks for sharing this! This looks like a useful regression test for the Windows long-path case. I'll review it against the existing reference iterator tests and adapt it for the PR if needed. Appreciate the suggestion! |
Author
|
I've updated the implementation to propagate unexpected iterator errors while continuing to skip missing and corrupted loose references. I've also added regression tests covering corrupted references, unreadable loose references, and long loose-reference paths. The iterator test suite passes on Linux (10 passed, 1 platform-specific test skipped). I also attempted to align the indentation with the repository's conventions, but I haven't been able to get the formatting completely consistent yet. The changed production lines use tabs, as requested. I'll appreciate any specific guidance on the remaining formatting differences. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes a reference iterator error-handling bug in which filesystem errors could cause iteration to stop prematurely, silently truncating the list of references and omitting subsequent loose references.
It addresses two places where errors were being swallowed:
iter_load_paths(): Errors returned bygit_iterator_advance()other thanGIT_ITEROVERcould be discarded, allowing the function to return success with a partially populated vector.refdb_fs_backend__iterator_next()andrefdb_fs_backend__iterator_next_name(): Errors fromloose_lookup()were unconditionally cleared, potentially hiding filesystem errors such as path-length limitations and I/O failures.Changes
iter_load_paths()to preserve and propagate errors returned by the filesystem iterator, while treatingGIT_ITEROVERas normal completion.refdb_fs_backend__iterator_next()andrefdb_fs_backend__iterator_next_name()to ignoreGIT_ENOTFOUNDwhile propagating other errors, preserving the existing handling of references that disappear during iteration.Scope
This PR focuses on correcting reference iterator error propagation. Support for honoring
core.longpathsinside the repository's.gitdirectory is intentionally outside the scope of this change, as discussed in #7451.Relates to #7451