[PATCH 00/10] odb: make consistency checks pluggable

[PATCH 00/10] odb: make consistency checks pluggable

[Patrick Steinhardt]

Hi,

this patch series makes object database consistency checks pluggable.

This series is built on top of 2c3adbb2c4 (The 18th batch, 2026-08-24)
with the following two dependencsie merged into it:

  - ps/odb-eagerly-load-alternates at 0076dc9f81 (odb: drop
    `alternates_db` field, 2026-08-17)

  - ps/odb-pluggable-pack-generation at 5176dd3d05 (bundle: generate
    packfiles via the object database, 2026-08-21)

Thanks!

Patrick

---
Patrick Steinhardt (10):
      builtin/fsck: use `fsck_obj_buffer()` when checking loose objects
      builtin/fsck: merge `fsck_obj_buffer()` and `fsck_obj()`
      builtin/fsck: de-globalize option handling
      builtin/fsck: don't check alternates with "--no-full"
      odb: provide infrastructure for pluggable fsck checks
      builtin/fsck: move packfile verification into the packed source
      builtin/fsck: move reverse index verification into the packed source
      builtin/fsck: move bitmap verification into the packed source
      builtin/fsck: move multi-pack index verification into the packed source
      builtin/fsck: move loose object verification into the loose source

 builtin/fsck.c                | 296 ++++++++----------------------------------
 odb.c                         |   9 ++
 odb.h                         |  33 +++++
 odb/source-files.c            |  13 ++
 odb/source-inmemory.c         |   8 ++
 odb/source-loose.c            |  92 +++++++++++++
 odb/source-packed.c           | 117 +++++++++++++++++
 odb/source.h                  |  21 +++
 pack-bitmap.c                 |  26 ++--
 pack-bitmap.h                 |   2 +-
 t/t1450-fsck.sh               |   5 +
 t/t5319-multi-pack-index.sh   |  13 ++
 t/t5325-reverse-index.sh      |   8 ++
 t/t5326-multi-pack-bitmaps.sh |  10 +-
 14 files changed, 394 insertions(+), 259 deletions(-)


---
base-commit: 6b08999fb1b3ad0bad04d492dc206ad42839e274
change-id: 20260810-pks-odb-source-fsck-e64772c7ee5f

[PATCH 01/10] builtin/fsck: use `fsck_obj_buffer()` when checking loose objects

[Patrick Steinhardt]

When checking loose objects we manually parse the object buffer we have
read from the on-disk file, mark the object and then call `fsck_obj()`.
Almost the exact same steps are also performed by `fsck_obj_buffer()`.

Stop open-coding this logic and call `fsck_obj_buffer()` instead.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 892c5661d9..3c4127f4d8 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -722,7 +722,6 @@ static int fsck_loose(const struct object_id *oid, const char *path,
 		      void *cb_data)
 {
 	struct for_each_loose_cb *data = cb_data;
-	struct object *obj;
 	enum object_type type = OBJ_NONE;
 	size_t size;
 	void *contents = NULL;
@@ -751,21 +750,7 @@ static int fsck_loose(const struct object_id *oid, const char *path,
 	if (!contents && type != OBJ_BLOB)
 		BUG("read_loose_object streamed a non-blob");
 
-	obj = parse_object_buffer(data->repo, oid, type, size,
-				  contents, &eaten);
-
-	if (!obj) {
-		errors_found |= ERROR_OBJECT;
-		error(_("%s: object could not be parsed: %s"),
-		      oid_to_hex(oid), path);
-		if (!eaten)
-			free(contents);
-		return 0; /* keep checking other objects */
-	}
-
-	obj->flags &= ~(REACHABLE | SEEN);
-	obj->flags |= HAS_OBJ;
-	if (fsck_obj(data->repo, obj, contents, size))
+	if (fsck_obj_buffer(oid, type, size, contents, &eaten, data->repo))
 		errors_found |= ERROR_OBJECT;
 
 	if (!eaten)

-- 
2.55.0.822.g20453c30eb.dirty

[PATCH 02/10] builtin/fsck: merge `fsck_obj_buffer()` and `fsck_obj()`

[Patrick Steinhardt]

The interfaces of the functions `fsck_obj()` and `fsck_obj_buffer()` are
somewhat similar to one another. The only difference between those two
is that `fsck_obj()` takes an already-parsed object as input, whereas
`fsck_obj_buffer()` parses the buffer and then calls `fsck_obj()`.

Furthermore, `fsck_obj()` has no callers other than `fsck_obj_buffer()`.

Refactor the code by merging those two functions. This makes it obvious
which function does what, and it allows us to get rid of the early in
`fsck_obj()` in case `SEEN` is set as the only caller unconditionally
clears that bit before calling it anyway.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c | 47 ++++++++++++++++++++---------------------------
 1 file changed, 20 insertions(+), 27 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 3c4127f4d8..bed8481893 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -401,14 +401,27 @@ static void check_connectivity(struct repository *repo)
 	}
 }
 
-static int fsck_obj(struct repository *repo,
-		    struct object *obj, void *buffer, unsigned long size)
+static int fsck_obj_buffer(const struct object_id *oid, enum object_type type,
+			   unsigned long size, void *buffer, int *eaten, void *cb_data)
 {
+	struct repository *repo = cb_data;
+	struct object *obj;
 	int err;
 
-	if (obj->flags & SEEN)
-		return 0;
-	obj->flags |= SEEN;
+	/*
+	 * Note, buffer may be NULL if type is OBJ_BLOB. See
+	 * verify_packfile(), data_valid variable for details.
+	 */
+	obj = parse_object_buffer(repo, oid, type, size, buffer, eaten);
+	if (!obj) {
+		errors_found |= ERROR_OBJECT;
+		err = error(_("%s: object corrupt or missing"),
+			    oid_to_hex(oid));
+		goto out;
+	}
+
+	obj->flags &= ~REACHABLE;
+	obj->flags |= HAS_OBJ | SEEN;
 
 	if (verbose)
 		fprintf_ln(stderr, _("Checking %s %s"),
@@ -417,6 +430,7 @@ static int fsck_obj(struct repository *repo,
 
 	if (fsck_walk(obj, NULL, &fsck_obj_options))
 		objerror(repo, obj, _("broken links"));
+
 	err = fsck_object(obj, buffer, size, &fsck_obj_options);
 	if (err)
 		goto out;
@@ -442,32 +456,11 @@ static int fsck_obj(struct repository *repo,
 	}
 
 out:
-	if (obj->type == OBJ_TREE)
+	if (obj && obj->type == OBJ_TREE)
 		free_tree_buffer((struct tree *)obj);
 	return err;
 }
 
-static int fsck_obj_buffer(const struct object_id *oid, enum object_type type,
-			   unsigned long size, void *buffer, int *eaten, void *cb_data)
-{
-	struct repository *repo = cb_data;
-	struct object *obj;
-
-	/*
-	 * Note, buffer may be NULL if type is OBJ_BLOB. See
-	 * verify_packfile(), data_valid variable for details.
-	 */
-	obj = parse_object_buffer(repo, oid, type, size, buffer, eaten);
-	if (!obj) {
-		errors_found |= ERROR_OBJECT;
-		return error(_("%s: object corrupt or missing"),
-			     oid_to_hex(oid));
-	}
-	obj->flags &= ~(REACHABLE | SEEN);
-	obj->flags |= HAS_OBJ;
-	return fsck_obj(repo, obj, buffer, size);
-}
-
 static int default_refs;
 
 static void fsck_handle_reflog_oid(struct repository *repo,

-- 
2.55.0.822.g20453c30eb.dirty

[PATCH 03/10] builtin/fsck: de-globalize option handling

[Patrick Steinhardt]

In subsequent commits we're about to rework some of the option handling
in git-fsck(1) a bit. It is currently a bit of a mess though due to lots
of global state that makes it hard to see which flags are used where
exactly.

Refactor the code by moving the fsck options into `cmd_fsck()`. This
allows us to convert some of the options into function-local variables.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c | 45 ++++++++++++++++++++++-----------------------
 1 file changed, 22 insertions(+), 23 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index bed8481893..5132ff0f15 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -37,10 +37,8 @@ static int show_root;
 static int show_tags;
 static int show_unreachable;
 static int include_reflogs = 1;
-static int check_full = 1;
 static int connectivity_only;
 static int check_strict;
-static int keep_cache_objects;
 static struct fsck_options fsck_walk_options;
 static struct fsck_options fsck_obj_options;
 static int errors_found;
@@ -48,8 +46,6 @@ static int write_lost_and_found;
 static int verbose;
 static int show_progress = -1;
 static int show_dangling = 1;
-static int name_objects;
-static int check_references = 1;
 static timestamp_t now;
 #define ERROR_OBJECT 01
 #define ERROR_REACHABLE 02
@@ -964,30 +960,33 @@ static char const * const fsck_usage[] = {
 	NULL
 };
 
-static struct option fsck_opts[] = {
-	OPT__VERBOSE(&verbose, N_("be verbose")),
-	OPT_BOOL(0, "unreachable", &show_unreachable, N_("show unreachable objects")),
-	OPT_BOOL(0, "dangling", &show_dangling, N_("show dangling objects")),
-	OPT_BOOL(0, "tags", &show_tags, N_("report tags")),
-	OPT_BOOL(0, "root", &show_root, N_("report root nodes")),
-	OPT_BOOL(0, "cache", &keep_cache_objects, N_("make index objects head nodes")),
-	OPT_BOOL(0, "reflogs", &include_reflogs, N_("make reflogs head nodes (default)")),
-	OPT_BOOL(0, "full", &check_full, N_("also consider packs and alternate objects")),
-	OPT_BOOL(0, "connectivity-only", &connectivity_only, N_("check only connectivity")),
-	OPT_BOOL(0, "strict", &check_strict, N_("enable more strict checking")),
-	OPT_BOOL(0, "lost-found", &write_lost_and_found,
-				N_("write dangling objects in .git/lost-found")),
-	OPT_BOOL(0, "progress", &show_progress, N_("show progress")),
-	OPT_BOOL(0, "name-objects", &name_objects, N_("show verbose names for reachable objects")),
-	OPT_BOOL(0, "references", &check_references, N_("check reference database consistency")),
-	OPT_END(),
-};
-
 int cmd_fsck(int argc,
 	     const char **argv,
 	     const char *prefix,
 	     struct repository *repo)
 {
+	int check_full = 1;
+	int keep_cache_objects = 0;
+	int name_objects = 0;
+	int check_references = 1;
+	struct option fsck_opts[] = {
+		OPT__VERBOSE(&verbose, N_("be verbose")),
+		OPT_BOOL(0, "unreachable", &show_unreachable, N_("show unreachable objects")),
+		OPT_BOOL(0, "dangling", &show_dangling, N_("show dangling objects")),
+		OPT_BOOL(0, "tags", &show_tags, N_("report tags")),
+		OPT_BOOL(0, "root", &show_root, N_("report root nodes")),
+		OPT_BOOL(0, "cache", &keep_cache_objects, N_("make index objects head nodes")),
+		OPT_BOOL(0, "reflogs", &include_reflogs, N_("make reflogs head nodes (default)")),
+		OPT_BOOL(0, "full", &check_full, N_("also consider packs and alternate objects")),
+		OPT_BOOL(0, "connectivity-only", &connectivity_only, N_("check only connectivity")),
+		OPT_BOOL(0, "strict", &check_strict, N_("enable more strict checking")),
+		OPT_BOOL(0, "lost-found", &write_lost_and_found,
+					N_("write dangling objects in .git/lost-found")),
+		OPT_BOOL(0, "progress", &show_progress, N_("show progress")),
+		OPT_BOOL(0, "name-objects", &name_objects, N_("show verbose names for reachable objects")),
+		OPT_BOOL(0, "references", &check_references, N_("check reference database consistency")),
+		OPT_END(),
+	};
 	struct odb_source *source;
 	struct snapshot snap = {
 		.nr = 0,

-- 
2.55.0.822.g20453c30eb.dirty

[PATCH 04/10] builtin/fsck: don't check alternates with "--no-full"

[Patrick Steinhardt]

According to git-fsck(1), the "--full" option behaves in the following
way:

  Check not just objects in GIT_OBJECT_DIRECTORY ($GIT_DIR/objects), but
  also the ones found in alternate object pools listed in
  GIT_ALTERNATE_OBJECT_DIRECTORIES or $GIT_DIR/objects/info/alternates,
  and in packed Git archives found in $GIT_DIR/objects/pack and
  corresponding pack subdirectories in alternate object pools.

So ultimately, it is supposed to control two things: (1) whether we only
check the main object directory, and (2) whether we check packfiles.

In its current state though, the flag only controls whether we check
packfiles or not, and if so we verify packfiles of all attached sources.
But we also have checks for loose objects in git-fsck(1), and here we
unconditionally check them in all sources.

The flag is arguably conflating two unrelated concerns with one another,
and it really should be split up into two flags: one that controls how
thorough we want to check individual sources, and one that controls
which sources we want to check in the first place. So ideally, we would
have:

  - "--include-alternates": check all sources, not only the local one.

  - "--include-optimized-objects": check not only loose objects, but
    also those that have been packed. Note that we explicitly don't say
    "--include-packed-objects" here to be more backend-agnostic.

  - "--full": implies both of the above flags.

This feels out of scope for this series though. So for now, simply fix
the code by honoring locality of the sources for loose objects.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c  | 3 ++-
 t/t1450-fsck.sh | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 5132ff0f15..3f6056535f 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -1047,7 +1047,8 @@ int cmd_fsck(int argc,
 				    mark_object_for_connectivity, repo, 0);
 	} else {
 		for (source = repo->objects->sources; source; source = source->next)
-			fsck_source(repo, source);
+			if (check_full || source->local)
+				fsck_source(repo, source);
 
 		if (check_full) {
 			struct packed_git *p;
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 77cd96de78..1b4074304c 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -844,6 +844,11 @@ test_expect_success 'alternate objects are correctly blamed' '
 	echo "../../alt.git/objects" >.git/objects/info/alternates &&
 	mkdir alt.git/objects/$(dirname $path) &&
 	>alt.git/objects/$(dirname $path)/$(basename $path) &&
+
+	# Without "--full", only the local object source is checked.
+	git fsck --no-full >out 2>&1 &&
+	test_must_be_empty out &&
+
 	test_must_fail git fsck >out 2>&1 &&
 	test_grep alt.git out
 '

-- 
2.55.0.822.g20453c30eb.dirty

[PATCH 05/10] odb: provide infrastructure for pluggable fsck checks

[Patrick Steinhardt]

The on-disk consistency checks in git-fsck(1) are conceptually
backend-specific: while connectivity checks and object-level parsing
checks are generic, verifying the physical integrity of packfiles and
loose objects is meaningful only to backends that use these formats:
Having these checks live in "builtin/fsck.c" violates that layering,
because it forces the command to reach directly into format-specific
internals.

Provide new infrastructure to make these format-specific checks
pluggable and implement stubs for the different source types we already
have. In subsequent commits we'll move functionality over piece by
piece.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c        | 16 +++++++++++-----
 odb.c                 |  9 +++++++++
 odb.h                 | 23 +++++++++++++++++++++++
 odb/source-files.c    | 13 +++++++++++++
 odb/source-inmemory.c |  8 ++++++++
 odb/source-loose.c    |  7 +++++++
 odb/source-packed.c   |  8 ++++++++
 odb/source.h          | 21 +++++++++++++++++++++
 8 files changed, 100 insertions(+), 5 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 3f6056535f..adbe192e56 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -965,7 +965,9 @@ int cmd_fsck(int argc,
 	     const char *prefix,
 	     struct repository *repo)
 {
-	int check_full = 1;
+	struct odb_fsck_options odb_fsck_opts = {
+		.flags = ODB_FSCK_FULL,
+	};
 	int keep_cache_objects = 0;
 	int name_objects = 0;
 	int check_references = 1;
@@ -977,7 +979,8 @@ int cmd_fsck(int argc,
 		OPT_BOOL(0, "root", &show_root, N_("report root nodes")),
 		OPT_BOOL(0, "cache", &keep_cache_objects, N_("make index objects head nodes")),
 		OPT_BOOL(0, "reflogs", &include_reflogs, N_("make reflogs head nodes (default)")),
-		OPT_BOOL(0, "full", &check_full, N_("also consider packs and alternate objects")),
+		OPT_BIT(0, "full", &odb_fsck_opts.flags,
+			N_("also consider packs and alternate objects"), ODB_FSCK_FULL),
 		OPT_BOOL(0, "connectivity-only", &connectivity_only, N_("check only connectivity")),
 		OPT_BOOL(0, "strict", &check_strict, N_("enable more strict checking")),
 		OPT_BOOL(0, "lost-found", &write_lost_and_found,
@@ -1018,7 +1021,7 @@ int cmd_fsck(int argc,
 		show_progress = 0;
 
 	if (write_lost_and_found) {
-		check_full = 1;
+		odb_fsck_opts.flags |= ODB_FSCK_FULL;
 		include_reflogs = 0;
 	}
 
@@ -1047,10 +1050,13 @@ int cmd_fsck(int argc,
 				    mark_object_for_connectivity, repo, 0);
 	} else {
 		for (source = repo->objects->sources; source; source = source->next)
-			if (check_full || source->local)
+			if ((odb_fsck_opts.flags & ODB_FSCK_FULL) || source->local)
 				fsck_source(repo, source);
 
-		if (check_full) {
+		if (odb_fsck(repo->objects, &odb_fsck_opts) < 0)
+			errors_found |= ERROR_OBJECT;
+
+		if (odb_fsck_opts.flags & ODB_FSCK_FULL) {
 			struct packed_git *p;
 			uint32_t total = 0, count = 0;
 			struct progress *progress = NULL;
diff --git a/odb.c b/odb.c
index 1fe20808eb..766043b685 100644
--- a/odb.c
+++ b/odb.c
@@ -1177,3 +1177,12 @@ void odb_reprepare(struct object_database *o)
 {
 	odb_prepare(o, ODB_PREPARE_FLUSH_CACHES);
 }
+
+int odb_fsck(struct object_database *odb, struct odb_fsck_options *options)
+{
+	int ret = 0;
+	for (struct odb_source *source = odb->sources; source; source = source->next)
+		if ((options->flags & ODB_FSCK_FULL) || source->local)
+			ret |= odb_source_fsck(source, options);
+	return ret;
+}
diff --git a/odb.h b/odb.h
index e60174070f..76c15e48f5 100644
--- a/odb.h
+++ b/odb.h
@@ -206,6 +206,29 @@ void odb_prepare(struct object_database *o, enum odb_prepare_flags flags);
 /* Equivalent to `odb_prepare(o, ODB_PREPARE_FLUSH_CACHES)`. */
 void odb_reprepare(struct object_database *o);
 
+enum odb_fsck_flags {
+	/*
+	 * If set, perform a full consistency check for the full object
+	 * database, including all of its sources and the contents of their
+	 * optimized formats. Otherwise, only check the local source, and
+	 * restrict checks of its optimized formats to cheap structural
+	 * verification of their metadata.
+	 */
+	ODB_FSCK_FULL = (1 << 0),
+};
+
+/* Options that shall be passed to `odb_fsck()`. */
+struct odb_fsck_options {
+	enum odb_fsck_flags flags;
+};
+
+/*
+ * Run backend-specific integrity checks on all object sources. Each source
+ * performs the checks appropriate to its type. Returns 0 on success, a
+ * negative error code otherwise.
+ */
+int odb_fsck(struct object_database *odb, struct odb_fsck_options *opts);
+
 /*
  * Find source by its object directory path. Returns a `NULL` pointer in case
  * the source could not be found.
diff --git a/odb/source-files.c b/odb/source-files.c
index bd4fdf3a6c..f6fb560d2e 100644
--- a/odb/source-files.c
+++ b/odb/source-files.c
@@ -893,6 +893,18 @@ static int odb_source_files_generate_pack(struct odb_source *source UNUSED,
 	return 0;
 }
 
+static int odb_source_files_fsck(struct odb_source *source,
+				 struct odb_fsck_options *opts)
+{
+	struct odb_source_files *files = odb_source_files_downcast(source);
+	int ret = 0;
+
+	ret |= odb_source_fsck(&files->loose->base, opts);
+	ret |= odb_source_fsck(&files->packed->base, opts);
+
+	return ret;
+}
+
 struct odb_source_files *odb_source_files_new(struct object_database *odb,
 					      const char *path,
 					      bool local)
@@ -908,6 +920,7 @@ struct odb_source_files *odb_source_files_new(struct object_database *odb,
 	files->base.close = odb_source_files_close;
 	files->base.create_on_disk = odb_source_files_create_on_disk;
 	files->base.prepare = odb_source_files_prepare;
+	files->base.fsck = odb_source_files_fsck;
 	files->base.read_object_info = odb_source_files_read_object_info;
 	files->base.read_object_stream = odb_source_files_read_object_stream;
 	files->base.for_each_object = odb_source_files_for_each_object;
diff --git a/odb/source-inmemory.c b/odb/source-inmemory.c
index 795672adf2..ba0f86da26 100644
--- a/odb/source-inmemory.c
+++ b/odb/source-inmemory.c
@@ -1,6 +1,7 @@
 #include "git-compat-util.h"
 #include "object-file.h"
 #include "odb.h"
+#include "fsck.h"
 #include "odb/source-inmemory.h"
 #include "odb/streaming.h"
 #include "oidtree.h"
@@ -368,6 +369,12 @@ static void odb_source_inmemory_free(struct odb_source *source)
 	free(inmemory);
 }
 
+static int odb_source_inmemory_fsck(struct odb_source *source UNUSED,
+				    struct odb_fsck_options *opts UNUSED)
+{
+	return 0;
+}
+
 struct odb_source_inmemory *odb_source_inmemory_new(struct object_database *odb)
 {
 	struct odb_source_inmemory *source;
@@ -378,6 +385,7 @@ struct odb_source_inmemory *odb_source_inmemory_new(struct object_database *odb)
 	source->base.free = odb_source_inmemory_free;
 	source->base.close = odb_source_inmemory_close;
 	source->base.prepare = odb_source_inmemory_prepare;
+	source->base.fsck = odb_source_inmemory_fsck;
 	source->base.read_object_info = odb_source_inmemory_read_object_info;
 	source->base.read_object_stream = odb_source_inmemory_read_object_stream;
 	source->base.for_each_object = odb_source_inmemory_for_each_object;
diff --git a/odb/source-loose.c b/odb/source-loose.c
index bb3455dfbd..f68d3c4d6c 100644
--- a/odb/source-loose.c
+++ b/odb/source-loose.c
@@ -1031,6 +1031,12 @@ static void odb_source_loose_free(struct odb_source *source)
 	free(loose);
 }
 
+static int odb_source_loose_fsck(struct odb_source *source UNUSED,
+				 struct odb_fsck_options *opts UNUSED)
+{
+	return 0;
+}
+
 struct odb_source_loose *odb_source_loose_new(struct object_database *odb,
 					      const char *path,
 					      bool local)
@@ -1043,6 +1049,7 @@ struct odb_source_loose *odb_source_loose_new(struct object_database *odb,
 	loose->base.free = odb_source_loose_free;
 	loose->base.close = odb_source_loose_close;
 	loose->base.prepare = odb_source_loose_prepare;
+	loose->base.fsck = odb_source_loose_fsck;
 	loose->base.read_object_info = odb_source_loose_read_object_info;
 	loose->base.read_object_stream = odb_source_loose_read_object_stream;
 	loose->base.for_each_object = odb_source_loose_for_each_object;
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 630d955585..7aacf4bc45 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -2,6 +2,7 @@
 #include "abspath.h"
 #include "chdir-notify.h"
 #include "dir.h"
+#include "fsck.h"
 #include "git-zlib.h"
 #include "list-objects-filter-options.h"
 #include "mergesort.h"
@@ -826,6 +827,12 @@ static void odb_source_packed_free(struct odb_source *source)
 	free(packed);
 }
 
+static int odb_source_packed_fsck(struct odb_source *source UNUSED,
+				  struct odb_fsck_options *opts UNUSED)
+{
+	return 0;
+}
+
 struct odb_source_packed *odb_source_packed_new(struct object_database *odb,
 						const char *path,
 						bool local)
@@ -839,6 +846,7 @@ struct odb_source_packed *odb_source_packed_new(struct object_database *odb,
 	packed->base.free = odb_source_packed_free;
 	packed->base.close = odb_source_packed_close;
 	packed->base.prepare = odb_source_packed_prepare;
+	packed->base.fsck = odb_source_packed_fsck;
 	packed->base.read_object_info = odb_source_packed_read_object_info;
 	packed->base.read_object_stream = odb_source_packed_read_object_stream;
 	packed->base.for_each_object = odb_source_packed_for_each_object;
diff --git a/odb/source.h b/odb/source.h
index 559e2ea2e9..10a5dd5194 100644
--- a/odb/source.h
+++ b/odb/source.h
@@ -320,6 +320,17 @@ struct odb_source {
 	int (*generate_pack)(struct odb_source *source,
 			     struct odb_pack_generator **out,
 			     const struct odb_generate_pack_options *opts);
+
+	/*
+	 * This callback is expected to check the integrity of the object source
+	 * and report any errors found via the fsck options. The checks performed
+	 * are backend-specific.
+	 *
+	 * The callback is expected to return 0 on success, a negative error
+	 * code otherwise.
+	 */
+	int (*fsck)(struct odb_source *source,
+		    struct odb_fsck_options *options);
 };
 
 /*
@@ -588,4 +599,14 @@ static inline int odb_source_generate_pack(struct odb_source *source,
 	return source->generate_pack(source, out, opts);
 }
 
+/*
+ * Check the integrity of the object database source. The checks performed
+ * are backend-specific. Returns 0 on success, a negative error code otherwise.
+ */
+static inline int odb_source_fsck(struct odb_source *source,
+				  struct odb_fsck_options *opts)
+{
+	return source->fsck(source, opts);
+}
+
 #endif

-- 
2.55.0.822.g20453c30eb.dirty

[PATCH 06/10] builtin/fsck: move packfile verification into the packed source

[Patrick Steinhardt]

Move the packfile verification out of `cmd_fsck()` and into the "packed"
source. While doing so, thread the progress meter and object callback
through the newly introduced `struct odb_fsck_options` so that the
caller's preferences are honoured without exposing those details at the
"builtin/fsck.c" level.

Note that the old code reported failures when verifying packfiles with
the `ERROR_PACK` bit, which gets returned to the caller via the exit
code. This bit is neither exercised in our test suite nor is it
documented anywhere in our codebase. Furthermore, this bit is highly
specific to the object storage backend, which makes it a bad fit for the
new pluggable infrastructure. So instead of retaining these semantics,
we drop them and return the generic `ERROR_OBJECT` bit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c      | 33 ++++-----------------------------
 odb.h               |  7 +++++++
 odb/source-packed.c | 46 +++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 54 insertions(+), 32 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index adbe192e56..e504dae904 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -7,7 +7,6 @@
 #include "blob.h"
 #include "tag.h"
 #include "refs.h"
-#include "pack.h"
 #include "cache-tree.h"
 #include "fsck.h"
 #include "parse-options.h"
@@ -49,7 +48,6 @@ static int show_dangling = 1;
 static timestamp_t now;
 #define ERROR_OBJECT 01
 #define ERROR_REACHABLE 02
-#define ERROR_PACK 04
 #define ERROR_REFS 010
 #define ERROR_COMMIT_GRAPH 020
 #define ERROR_MULTI_PACK_INDEX 040
@@ -967,6 +965,8 @@ int cmd_fsck(int argc,
 {
 	struct odb_fsck_options odb_fsck_opts = {
 		.flags = ODB_FSCK_FULL,
+		.object_cb = fsck_obj_buffer,
+		.object_payload = repo,
 	};
 	int keep_cache_objects = 0;
 	int name_objects = 0;
@@ -1019,6 +1019,8 @@ int cmd_fsck(int argc,
 		show_progress = isatty(2);
 	if (verbose)
 		show_progress = 0;
+	if (show_progress)
+		odb_fsck_opts.flags |= ODB_FSCK_PROGRESS;
 
 	if (write_lost_and_found) {
 		odb_fsck_opts.flags |= ODB_FSCK_FULL;
@@ -1056,33 +1058,6 @@ int cmd_fsck(int argc,
 		if (odb_fsck(repo->objects, &odb_fsck_opts) < 0)
 			errors_found |= ERROR_OBJECT;
 
-		if (odb_fsck_opts.flags & ODB_FSCK_FULL) {
-			struct packed_git *p;
-			uint32_t total = 0, count = 0;
-			struct progress *progress = NULL;
-
-			if (show_progress) {
-				repo_for_each_pack(repo, p) {
-					if (open_pack_index(p))
-						continue;
-					total += p->num_objects;
-				}
-
-				progress = start_progress(repo,
-							  _("Checking objects"), total);
-			}
-
-			repo_for_each_pack(repo, p) {
-				/* verify gives error messages itself */
-				if (verify_pack(repo,
-						p, fsck_obj_buffer, repo,
-						progress, count))
-					errors_found |= ERROR_PACK;
-				count += p->num_objects;
-			}
-			stop_progress(&progress);
-		}
-
 		if (fsck_finish(&fsck_obj_options))
 			errors_found |= ERROR_OBJECT;
 	}
diff --git a/odb.h b/odb.h
index 76c15e48f5..0bf6c8d7d2 100644
--- a/odb.h
+++ b/odb.h
@@ -215,11 +215,18 @@ enum odb_fsck_flags {
 	 * verification of their metadata.
 	 */
 	ODB_FSCK_FULL = (1 << 0),
+
+	/* Display a progress meter, if sensible. */
+	ODB_FSCK_PROGRESS = (1 << 1),
 };
 
 /* Options that shall be passed to `odb_fsck()`. */
 struct odb_fsck_options {
 	enum odb_fsck_flags flags;
+
+	int (*object_cb)(const struct object_id *oid, enum object_type type,
+			 unsigned long size, void *buffer, int *eaten, void *cb_data);
+	void *object_payload;
 };
 
 /*
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 7aacf4bc45..0d3599f8fe 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -9,8 +9,10 @@
 #include "midx.h"
 #include "odb/source-packed.h"
 #include "odb/streaming.h"
+#include "pack.h"
 #include "packfile.h"
 #include "pack-bitmap.h"
+#include "progress.h"
 
 static int find_pack_entry(struct odb_source_packed *store,
 			   const struct object_id *oid,
@@ -827,10 +829,48 @@ static void odb_source_packed_free(struct odb_source *source)
 	free(packed);
 }
 
-static int odb_source_packed_fsck(struct odb_source *source UNUSED,
-				  struct odb_fsck_options *opts UNUSED)
+static int verify_packs(struct odb_source_packed *source,
+			struct odb_fsck_options *opts)
 {
-	return 0;
+	struct progress *progress = NULL;
+	struct packfile_list_entry *e;
+	uint32_t total = 0, count = 0;
+	int ret = 0;
+
+	if (opts->flags & ODB_FSCK_PROGRESS) {
+		for (e = packfile_store_get_packs(source); e; e = e->next) {
+			if (open_pack_index(e->pack))
+				continue;
+			total += e->pack->num_objects;
+		}
+
+		progress = start_progress(source->base.odb->repo,
+					  _("Checking objects"), total);
+	}
+
+	for (e = packfile_store_get_packs(source); e; e = e->next) {
+		/* verify gives error messages itself */
+		if (verify_pack(source->base.odb->repo, e->pack,
+				opts->object_cb, opts->object_payload,
+				progress, count))
+			ret = -1;
+		count += e->pack->num_objects;
+	}
+	stop_progress(&progress);
+
+	return ret;
+}
+
+static int odb_source_packed_fsck(struct odb_source *source,
+				  struct odb_fsck_options *opts)
+{
+	struct odb_source_packed *packed = odb_source_packed_downcast(source);
+	int ret = 0;
+
+	if ((opts->flags & ODB_FSCK_FULL) && verify_packs(packed, opts) < 0)
+		ret = -1;
+
+	return ret;
 }
 
 struct odb_source_packed *odb_source_packed_new(struct object_database *odb,

-- 
2.55.0.822.g20453c30eb.dirty

[PATCH 07/10] builtin/fsck: move reverse index verification into the packed source

[Patrick Steinhardt]

The checks for reverse indexes live in `check_pack_rev_indexes()`, which
is hosted in "builtin/fsck.c". These checks are obviously specific to
the "packed" backend.

Move the logic into `odb_source_packed_fsck()`. As in the preceding
commit, drop the dedicated `ERROR_PACK_REV_INDEX` bit and instead use
the generic `ERROR_OBJECT` bit.

Note that this changes behaviour in two ways:

  - The checks are now skipped when "--connectivity-only" was passed.
    This is because we don't even run `odb_fsck()` at all when that
    flag has been passed by the user, and not verifying data structures
    of the object database matches the documented intent of that flag,
    which is to only check the connectivity of reachable objects.

  - The checks are now skipped for non-local sources when "--no-full"
    was passed. This is, again, in line with the documented intent of
    that flag.

Add a test to cast these semantics into stone.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c           | 37 -------------------------------------
 odb/source-packed.c      | 39 +++++++++++++++++++++++++++++++++++++++
 t/t5325-reverse-index.sh |  8 ++++++++
 3 files changed, 47 insertions(+), 37 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index e504dae904..06e72877f3 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -23,7 +23,6 @@
 #include "run-command.h"
 #include "sparse-index.h"
 #include "worktree.h"
-#include "pack-revindex.h"
 #include "pack-bitmap.h"
 
 #define REACHABLE 0x0001
@@ -51,7 +50,6 @@ static timestamp_t now;
 #define ERROR_REFS 010
 #define ERROR_COMMIT_GRAPH 020
 #define ERROR_MULTI_PACK_INDEX 040
-#define ERROR_PACK_REV_INDEX 0100
 #define ERROR_BITMAP 0200
 
 static const char *describe_object(const struct object_id *oid)
@@ -890,40 +888,6 @@ static int mark_object_for_connectivity(const struct object_id *oid,
 	return 0;
 }
 
-static int check_pack_rev_indexes(struct repository *r, int show_progress)
-{
-	struct progress *progress = NULL;
-	struct packed_git *p;
-	uint32_t pack_count = 0;
-	int res = 0;
-
-	if (show_progress) {
-		repo_for_each_pack(r, p)
-			pack_count++;
-		progress = start_delayed_progress(r,
-						  "Verifying reverse pack-indexes", pack_count);
-		pack_count = 0;
-	}
-
-	repo_for_each_pack(r, p) {
-		int load_error = load_pack_revindex_from_disk(p);
-
-		if (load_error < 0) {
-			error(_("unable to load rev-index for pack '%s'"), p->pack_name);
-			res = ERROR_PACK_REV_INDEX;
-		} else if (!load_error &&
-			   !load_pack_revindex(r, p) &&
-			   verify_pack_revindex(p)) {
-			error(_("invalid rev-index for pack '%s'"), p->pack_name);
-			res = ERROR_PACK_REV_INDEX;
-		}
-		display_progress(progress, ++pack_count);
-	}
-	stop_progress(&progress);
-
-	return res;
-}
-
 static void fsck_refs(struct repository *r)
 {
 	struct child_process refs_verify = CHILD_PROCESS_INIT;
@@ -1104,7 +1068,6 @@ int cmd_fsck(int argc,
 		free_worktrees(worktrees);
 	}
 
-	errors_found |= check_pack_rev_indexes(repo, show_progress);
 	if (verify_bitmap_files(repo))
 		errors_found |= ERROR_BITMAP;
 
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 0d3599f8fe..e5e69636dd 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -10,6 +10,7 @@
 #include "odb/source-packed.h"
 #include "odb/streaming.h"
 #include "pack.h"
+#include "pack-revindex.h"
 #include "packfile.h"
 #include "pack-bitmap.h"
 #include "progress.h"
@@ -861,6 +862,41 @@ static int verify_packs(struct odb_source_packed *source,
 	return ret;
 }
 
+static int verify_reverse_indices(struct odb_source_packed *source,
+				  struct odb_fsck_options *opts)
+{
+	struct progress *progress = NULL;
+	struct packfile_list_entry *e;
+	uint32_t pack_count = 0;
+	int res = 0;
+
+	if (opts->flags & ODB_FSCK_PROGRESS) {
+		for (e = packfile_store_get_packs(source); e; e = e->next)
+			pack_count++;
+		progress = start_delayed_progress(source->base.odb->repo,
+						  "Verifying reverse pack-indexes", pack_count);
+		pack_count = 0;
+	}
+
+	for (e = packfile_store_get_packs(source); e; e = e->next) {
+		int load_error = load_pack_revindex_from_disk(e->pack);
+
+		if (load_error < 0) {
+			error(_("unable to load rev-index for pack '%s'"), e->pack->pack_name);
+			res = -1;
+		} else if (!load_error &&
+			   !load_pack_revindex(source->base.odb->repo, e->pack) &&
+			   verify_pack_revindex(e->pack)) {
+			error(_("invalid rev-index for pack '%s'"), e->pack->pack_name);
+			res = -1;
+		}
+		display_progress(progress, ++pack_count);
+	}
+	stop_progress(&progress);
+
+	return res;
+}
+
 static int odb_source_packed_fsck(struct odb_source *source,
 				  struct odb_fsck_options *opts)
 {
@@ -870,6 +906,9 @@ static int odb_source_packed_fsck(struct odb_source *source,
 	if ((opts->flags & ODB_FSCK_FULL) && verify_packs(packed, opts) < 0)
 		ret = -1;
 
+	if (verify_reverse_indices(packed, opts) < 0)
+		ret = -1;
+
 	return ret;
 }
 
diff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh
index 5493791938..6b81abf663 100755
--- a/t/t5325-reverse-index.sh
+++ b/t/t5325-reverse-index.sh
@@ -204,4 +204,12 @@ test_expect_success 'fsck catches invalid header: hash function' '
 		"reverse-index file .* has unsupported hash id"
 '
 
+test_expect_success 'fsck --no-full checks rev-index, --connectivity-only does not' '
+	test_must_fail git -C corrupt fsck --no-full 2>err &&
+	test_grep "has unsupported hash id" err &&
+
+	git -C corrupt fsck --connectivity-only 2>err &&
+	test_grep ! "has unsupported hash id" err
+'
+
 test_done

-- 
2.55.0.822.g20453c30eb.dirty

[PATCH 08/10] builtin/fsck: move bitmap verification into the packed source

[Patrick Steinhardt]

The checks for bitmaps live in `verify_bitmap_files()`, which is called
by "builtin/fsck.c". These checks are obviously specific to the "packed"
backend.

Move the logic into `odb_source_packed_fsck()`. As in preceding commits,
this means that we now properly honor both "--connectivity-only" and
"--no-full". Furthermore, we drop the dedicated `ERROR_BITMAP` bit and
instead use the generic `ERROR_OBJECT` bit.

Note that this change also adapts `verify_bitmap_files()` to be
focussed on a single "packed" source instead of verifying bitmaps from
all sources. This change is required as we already know to loop around
the sources in `odb_fsck()` itself.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c                |  5 -----
 odb/source-packed.c           |  3 +++
 pack-bitmap.c                 | 26 ++++++++++----------------
 pack-bitmap.h                 |  2 +-
 t/t5326-multi-pack-bitmaps.sh | 10 +++++++++-
 5 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 06e72877f3..2f7d29aa56 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -23,7 +23,6 @@
 #include "run-command.h"
 #include "sparse-index.h"
 #include "worktree.h"
-#include "pack-bitmap.h"
 
 #define REACHABLE 0x0001
 #define SEEN      0x0002
@@ -50,7 +49,6 @@ static timestamp_t now;
 #define ERROR_REFS 010
 #define ERROR_COMMIT_GRAPH 020
 #define ERROR_MULTI_PACK_INDEX 040
-#define ERROR_BITMAP 0200
 
 static const char *describe_object(const struct object_id *oid)
 {
@@ -1068,9 +1066,6 @@ int cmd_fsck(int argc,
 		free_worktrees(worktrees);
 	}
 
-	if (verify_bitmap_files(repo))
-		errors_found |= ERROR_BITMAP;
-
 	check_connectivity(repo);
 
 	if (repo->settings.core_commit_graph) {
diff --git a/odb/source-packed.c b/odb/source-packed.c
index e5e69636dd..2b5dc502f5 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -909,6 +909,9 @@ static int odb_source_packed_fsck(struct odb_source *source,
 	if (verify_reverse_indices(packed, opts) < 0)
 		ret = -1;
 
+	if (verify_bitmap_files(packed))
+		ret = -1;
+
 	return ret;
 }
 
diff --git a/pack-bitmap.c b/pack-bitmap.c
index e0fb57d332..3de8e9590c 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -3410,28 +3410,22 @@ static int verify_bitmap_file(const struct git_hash_algo *algop,
 	return res;
 }
 
-int verify_bitmap_files(struct repository *r)
+int verify_bitmap_files(struct odb_source_packed *source)
 {
-	struct odb_source *source;
-	struct packed_git *p;
+	struct packfile_list_entry *e;
+	struct multi_pack_index *m;
 	int res = 0;
 
-	for (source = r->objects->sources; source; source = source->next) {
-		struct odb_source_files *files = odb_source_files_downcast(source);
-		struct multi_pack_index *m = get_multi_pack_index(files->packed);
-		char *midx_bitmap_name;
-
-		if (!m)
-			continue;
-
-		midx_bitmap_name = midx_bitmap_filename(m);
-		res |= verify_bitmap_file(r->hash_algo, midx_bitmap_name);
+	m = get_multi_pack_index(source);
+	if (m) {
+		char *midx_bitmap_name = midx_bitmap_filename(m);
+		res |= verify_bitmap_file(source->base.odb->repo->hash_algo, midx_bitmap_name);
 		free(midx_bitmap_name);
 	}
 
-	repo_for_each_pack(r, p) {
-		char *pack_bitmap_name = pack_bitmap_filename(p);
-		res |= verify_bitmap_file(r->hash_algo, pack_bitmap_name);
+	for (e = packfile_store_get_packs(source); e; e = e->next) {
+		char *pack_bitmap_name = pack_bitmap_filename(e->pack);
+		res |= verify_bitmap_file(source->base.odb->repo->hash_algo, pack_bitmap_name);
 		free(pack_bitmap_name);
 	}
 
diff --git a/pack-bitmap.h b/pack-bitmap.h
index 1385027c1f..847ad4762d 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -205,7 +205,7 @@ int bitmap_is_midx(struct bitmap_index *bitmap_git);
 
 int bitmap_is_preferred_refname(struct repository *r, const char *refname);
 
-int verify_bitmap_files(struct repository *r);
+int verify_bitmap_files(struct odb_source_packed *source);
 
 struct ewah_bitmap *read_bitmap(const unsigned char *map,
 				size_t map_size, size_t *map_pos);
diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh
index 86beab1dae..8047459b00 100755
--- a/t/t5326-multi-pack-bitmaps.sh
+++ b/t/t5326-multi-pack-bitmaps.sh
@@ -498,7 +498,15 @@ test_expect_success 'git fsck correctly identifies good and bad bitmaps' '
 	corrupt_file "$packbitmap" &&
 	test_must_fail git fsck 2>err &&
 	test_grep "bitmap file '\''$midxbitmap'\'' has invalid checksum" err &&
-	test_grep "bitmap file '\''$packbitmap'\'' has invalid checksum" err
+	test_grep "bitmap file '\''$packbitmap'\'' has invalid checksum" err &&
+
+	# The bitmap checks are performed with "--no-full", but not with
+	# "--connectivity-only".
+	test_must_fail git fsck --no-full 2>err &&
+	test_grep "bitmap file '\''$midxbitmap'\'' has invalid checksum" err &&
+	test_grep "bitmap file '\''$packbitmap'\'' has invalid checksum" err &&
+	git fsck --connectivity-only 2>err &&
+	test_grep ! "invalid checksum" err
 '
 
 test_expect_success 'corrupt MIDX with bitmap causes fallback' '

-- 
2.55.0.822.g20453c30eb.dirty

[PATCH 09/10] builtin/fsck: move multi-pack index verification into the packed source

[Patrick Steinhardt]

The checks for multi-pack indexes are hosted in `cmd_fsck()` directly.
These checks are obviously specific to the "packed" backend.

Move the logic into `odb_source_packed_fsck()`. As in preceding commits,
this means that we now properly honor both "--connectivity-only" and
"--no-full". Furthermore, we drop the dedicated `ERROR_MULTI_PACK_INDEX`
bit and instead use the generic `ERROR_OBJECT` bit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c              | 18 ------------------
 odb/source-packed.c         | 27 +++++++++++++++++++++++++++
 t/t5319-multi-pack-index.sh | 13 +++++++++++++
 3 files changed, 40 insertions(+), 18 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 2f7d29aa56..7eaea340b0 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -48,7 +48,6 @@ static timestamp_t now;
 #define ERROR_REACHABLE 02
 #define ERROR_REFS 010
 #define ERROR_COMMIT_GRAPH 020
-#define ERROR_MULTI_PACK_INDEX 040
 
 static const char *describe_object(const struct object_id *oid)
 {
@@ -1085,23 +1084,6 @@ int cmd_fsck(int argc,
 		}
 	}
 
-	if (repo->settings.core_multi_pack_index) {
-		struct child_process midx_verify = CHILD_PROCESS_INIT;
-
-		for (source = repo->objects->sources; source; source = source->next) {
-			child_process_init(&midx_verify);
-			midx_verify.git_cmd = 1;
-			strvec_pushl(&midx_verify.args, "multi-pack-index",
-				     "verify", "--object-dir", source->path, NULL);
-			if (show_progress)
-				strvec_push(&midx_verify.args, "--progress");
-			else
-				strvec_push(&midx_verify.args, "--no-progress");
-			if (run_command(&midx_verify))
-				errors_found |= ERROR_MULTI_PACK_INDEX;
-		}
-	}
-
 	free_snapshot_refs(&snap);
 	return errors_found;
 }
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 2b5dc502f5..9f42552377 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -14,6 +14,7 @@
 #include "packfile.h"
 #include "pack-bitmap.h"
 #include "progress.h"
+#include "run-command.h"
 
 static int find_pack_entry(struct odb_source_packed *store,
 			   const struct object_id *oid,
@@ -897,6 +898,29 @@ static int verify_reverse_indices(struct odb_source_packed *source,
 	return res;
 }
 
+static int verify_midx(struct odb_source_packed *source,
+		       struct odb_fsck_options *opts)
+{
+	struct child_process midx_verify = CHILD_PROCESS_INIT;
+	int ret = 0;
+
+	if (!source->base.odb->repo->settings.core_multi_pack_index)
+		return 0;
+
+	child_process_init(&midx_verify);
+	midx_verify.git_cmd = 1;
+	strvec_pushl(&midx_verify.args, "multi-pack-index",
+		     "verify", "--object-dir", source->base.path, NULL);
+	if (opts->flags & ODB_FSCK_PROGRESS)
+		strvec_push(&midx_verify.args, "--progress");
+	else
+		strvec_push(&midx_verify.args, "--no-progress");
+	if (run_command(&midx_verify))
+		ret = -1;
+
+	return ret;
+}
+
 static int odb_source_packed_fsck(struct odb_source *source,
 				  struct odb_fsck_options *opts)
 {
@@ -912,6 +936,9 @@ static int odb_source_packed_fsck(struct odb_source *source,
 	if (verify_bitmap_files(packed))
 		ret = -1;
 
+	if (verify_midx(packed, opts) < 0)
+		ret = -1;
+
 	return ret;
 }
 
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 68143cb5b7..20b010c33b 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -573,6 +573,19 @@ test_expect_success 'verify incorrect checksum' '
 		$objdir "incorrect checksum"
 '
 
+test_expect_success 'git fsck --no-full checks multi-pack-index, --connectivity-only does not' '
+	pos=$(($(wc -c <$objdir/pack/multi-pack-index) - 10)) &&
+	corrupt_midx_and_verify $pos \
+		"\377\377\377\377\377\377\377\377\377\377" \
+		$objdir "incorrect checksum" &&
+
+	test_must_fail git fsck --no-full 2>err &&
+	test_grep "incorrect checksum" err &&
+
+	git fsck --connectivity-only 2>err &&
+	test_grep ! "incorrect checksum" err
+'
+
 test_expect_success 'setup for v1-specific fsck tests' '
 	git -c midx.version=1 multi-pack-index write
 '

-- 
2.55.0.822.g20453c30eb.dirty

[PATCH 10/10] builtin/fsck: move loose object verification into the loose source

[Patrick Steinhardt]

The consistency checks for loose objects are hosted by "builtin/fsck.c".
These checks are obviously specific to the "loose" backend.

Move the logic into `odb_source_loose_fsck()`. Introduce a new "verbose"
flag so that we can properly retain semantics around whether or not we
want to print some status messages.

Note that this fixes a bug as a side effect: the progress meter was
captured in the callback data before `start_progress()` was even called,
so the per-subdirectory progress updates always operated on a NULL
pointer and the meter jumped straight from 0 to 256 upon completion. The
new code only sets up the callback data's progress meter after it has
been created, so the progress display now advances incrementally again.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c     | 91 ++----------------------------------------------------
 odb.h              |  3 ++
 odb/source-loose.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 93 insertions(+), 90 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 7eaea340b0..4af1d874cc 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -12,7 +12,6 @@
 #include "parse-options.h"
 #include "progress.h"
 #include "packfile.h"
-#include "object-file.h"
 #include "object-name.h"
 #include "odb.h"
 #include "odb/streaming.h"
@@ -695,88 +694,6 @@ static void process_refs(struct repository *repo, struct snapshot *snap)
 	}
 }
 
-struct for_each_loose_cb {
-	struct repository *repo;
-	struct progress *progress;
-};
-
-static int fsck_loose(const struct object_id *oid, const char *path,
-		      void *cb_data)
-{
-	struct for_each_loose_cb *data = cb_data;
-	enum object_type type = OBJ_NONE;
-	size_t size;
-	void *contents = NULL;
-	int eaten;
-	struct object_info oi = OBJECT_INFO_INIT;
-	struct object_id real_oid = *null_oid(data->repo->hash_algo);
-	int err = 0;
-
-	oi.sizep = &size;
-	oi.typep = &type;
-
-	if (read_loose_object(data->repo, path, oid, &real_oid, &contents, &oi) < 0) {
-		if (contents && !oideq(&real_oid, oid))
-			err = error(_("%s: hash-path mismatch, found at: %s"),
-				    oid_to_hex(&real_oid), path);
-		else
-			err = error(_("%s: object corrupt or missing: %s"),
-				    oid_to_hex(oid), path);
-	}
-	if (err < 0) {
-		errors_found |= ERROR_OBJECT;
-		free(contents);
-		return 0; /* keep checking other objects */
-	}
-
-	if (!contents && type != OBJ_BLOB)
-		BUG("read_loose_object streamed a non-blob");
-
-	if (fsck_obj_buffer(oid, type, size, contents, &eaten, data->repo))
-		errors_found |= ERROR_OBJECT;
-
-	if (!eaten)
-		free(contents);
-	return 0; /* keep checking other objects, even if we saw an error */
-}
-
-static int fsck_cruft(const char *basename, const char *path,
-		      void *data UNUSED)
-{
-	if (!starts_with(basename, "tmp_obj_"))
-		fprintf_ln(stderr, _("bad sha1 file: %s"), path);
-	return 0;
-}
-
-static int fsck_subdir(unsigned int nr, const char *path UNUSED, void *data)
-{
-	struct for_each_loose_cb *cb_data = data;
-	struct progress *progress = cb_data->progress;
-	display_progress(progress, nr + 1);
-	return 0;
-}
-
-static void fsck_source(struct repository *repo, struct odb_source *source)
-{
-	struct progress *progress = NULL;
-	struct for_each_loose_cb cb_data = {
-		.repo = source->odb->repo,
-		.progress = progress,
-	};
-
-	if (verbose)
-		fprintf_ln(stderr, _("Checking object directory"));
-
-	if (show_progress)
-		progress = start_progress(repo,
-					  _("Checking object directories"), 256);
-
-	for_each_loose_file_in_source(source, fsck_loose,
-				      fsck_cruft, fsck_subdir, &cb_data);
-	display_progress(progress, 256);
-	stop_progress(&progress);
-}
-
 static int fsck_cache_tree(struct repository *repo, struct cache_tree *it, const char *index_path)
 {
 	int i;
@@ -978,8 +895,10 @@ int cmd_fsck(int argc,
 
 	if (show_progress == -1)
 		show_progress = isatty(2);
-	if (verbose)
+	if (verbose) {
 		show_progress = 0;
+		odb_fsck_opts.flags |= ODB_FSCK_VERBOSE;
+	}
 	if (show_progress)
 		odb_fsck_opts.flags |= ODB_FSCK_PROGRESS;
 
@@ -1012,10 +931,6 @@ int cmd_fsck(int argc,
 		odb_for_each_object(repo->objects, NULL,
 				    mark_object_for_connectivity, repo, 0);
 	} else {
-		for (source = repo->objects->sources; source; source = source->next)
-			if ((odb_fsck_opts.flags & ODB_FSCK_FULL) || source->local)
-				fsck_source(repo, source);
-
 		if (odb_fsck(repo->objects, &odb_fsck_opts) < 0)
 			errors_found |= ERROR_OBJECT;
 
diff --git a/odb.h b/odb.h
index 0bf6c8d7d2..b87f281cbd 100644
--- a/odb.h
+++ b/odb.h
@@ -218,6 +218,9 @@ enum odb_fsck_flags {
 
 	/* Display a progress meter, if sensible. */
 	ODB_FSCK_PROGRESS = (1 << 1),
+
+	/* Be extra verbose when checking the database. */
+	ODB_FSCK_VERBOSE = (1 << 2),
 };
 
 /* Options that shall be passed to `odb_fsck()`. */
diff --git a/odb/source-loose.c b/odb/source-loose.c
index f68d3c4d6c..efef9ca61f 100644
--- a/odb/source-loose.c
+++ b/odb/source-loose.c
@@ -12,6 +12,7 @@
 #include "odb/streaming.h"
 #include "oidtree.h"
 #include "path.h"
+#include "progress.h"
 #include "repository.h"
 #include "strbuf.h"
 #include "tempfile.h"
@@ -1031,12 +1032,96 @@ static void odb_source_loose_free(struct odb_source *source)
 	free(loose);
 }
 
-static int odb_source_loose_fsck(struct odb_source *source UNUSED,
-				 struct odb_fsck_options *opts UNUSED)
+struct fsck_loose_data {
+	struct odb_source_loose *source;
+	struct odb_fsck_options *opts;
+	struct progress *progress;
+	bool error_found;
+};
+
+static int fsck_loose(const struct object_id *oid, const char *path,
+		      void *cb_data)
 {
+	struct fsck_loose_data *data = cb_data;
+	enum object_type type = OBJ_NONE;
+	size_t size;
+	void *contents = NULL;
+	int eaten = 0;
+	struct object_info oi = OBJECT_INFO_INIT;
+	struct object_id real_oid = *null_oid(data->source->base.odb->repo->hash_algo);
+	int err = 0;
+
+	oi.sizep = &size;
+	oi.typep = &type;
+
+	if (read_loose_object(data->source->base.odb->repo,
+			      path, oid, &real_oid, &contents, &oi) < 0) {
+		if (contents && !oideq(&real_oid, oid))
+			err = error(_("%s: hash-path mismatch, found at: %s"),
+				    oid_to_hex(&real_oid), path);
+		else
+			err = error(_("%s: object corrupt or missing: %s"),
+				    oid_to_hex(oid), path);
+	}
+	if (err < 0)
+		goto out;
+
+	if (!contents && type != OBJ_BLOB)
+		BUG("read_loose_object streamed a non-blob");
+
+	if (data->opts->object_cb(oid, type, size, contents, &eaten,
+				  data->opts->object_payload)) {
+		err = -1;
+		goto out;
+	}
+
+out:
+	if (err)
+		data->error_found = true;
+	if (!eaten)
+		free(contents);
+	return 0; /* keep checking other objects, even if we saw an error */
+}
+
+static int fsck_cruft(const char *basename, const char *path,
+		      void *data UNUSED)
+{
+	if (!starts_with(basename, "tmp_obj_"))
+		fprintf_ln(stderr, _("bad sha1 file: %s"), path);
+	return 0;
+}
+
+static int fsck_subdir(unsigned int nr, const char *path UNUSED, void *cb_data)
+{
+	struct fsck_loose_data *data = cb_data;
+	display_progress(data->progress, nr + 1);
 	return 0;
 }
 
+static int odb_source_loose_fsck(struct odb_source *source,
+				 struct odb_fsck_options *opts)
+{
+	struct odb_source_loose *loose = odb_source_loose_downcast(source);
+	struct fsck_loose_data data = {
+		.source = loose,
+		.opts = opts,
+	};
+
+	if (opts->flags & ODB_FSCK_VERBOSE)
+		fprintf_ln(stderr, _("Checking object directory"));
+
+	if (opts->flags & ODB_FSCK_PROGRESS)
+		data.progress = start_progress(source->odb->repo,
+					       _("Checking object directories"), 256);
+
+	for_each_loose_file_in_source(source, fsck_loose,
+				      fsck_cruft, fsck_subdir, &data);
+	display_progress(data.progress, 256);
+	stop_progress(&data.progress);
+
+	return data.error_found ? -1 : 0;
+}
+
 struct odb_source_loose *odb_source_loose_new(struct object_database *odb,
 					      const char *path,
 					      bool local)

-- 
2.55.0.822.g20453c30eb.dirty

Re: [PATCH 01/10] builtin/fsck: use `fsck_obj_buffer()` when checking loose objects

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 1944 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> When checking loose objects we manually parse the object buffer we have
> read from the on-disk file, mark the object and then call `fsck_obj()`.
> Almost the exact same steps are also performed by `fsck_obj_buffer()`.
>

I was wondering what the difference was, there seems to be none, nit:
perhaps we can drop 'Almost'.

> Stop open-coding this logic and call `fsck_obj_buffer()` instead.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/fsck.c | 17 +----------------
>  1 file changed, 1 insertion(+), 16 deletions(-)
>
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 892c5661d9..3c4127f4d8 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -722,7 +722,6 @@ static int fsck_loose(const struct object_id *oid, const char *path,
>  		      void *cb_data)
>  {
>  	struct for_each_loose_cb *data = cb_data;
> -	struct object *obj;
>  	enum object_type type = OBJ_NONE;
>  	size_t size;
>  	void *contents = NULL;
> @@ -751,21 +750,7 @@ static int fsck_loose(const struct object_id *oid, const char *path,
>  	if (!contents && type != OBJ_BLOB)
>  		BUG("read_loose_object streamed a non-blob");
>
> -	obj = parse_object_buffer(data->repo, oid, type, size,
> -				  contents, &eaten);
> -
> -	if (!obj) {
> -		errors_found |= ERROR_OBJECT;
> -		error(_("%s: object could not be parsed: %s"),
> -		      oid_to_hex(oid), path);
> -		if (!eaten)
> -			free(contents);

This is now moved to the bottom below fsck_obj_buffer() call. So that's
okay.

> -		return 0; /* keep checking other objects */
> -	}
> -
> -	obj->flags &= ~(REACHABLE | SEEN);
> -	obj->flags |= HAS_OBJ;
> -	if (fsck_obj(data->repo, obj, contents, size))
> +	if (fsck_obj_buffer(oid, type, size, contents, &eaten, data->repo))
>  		errors_found |= ERROR_OBJECT;
>

I see `fsck_obj_buffer()` also sets adds the `ERROR_OBJECT` flag, but
that's okay.

>  	if (!eaten)
>
> --
> 2.55.0.822.g20453c30eb.dirty

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 02/10] builtin/fsck: merge `fsck_obj_buffer()` and `fsck_obj()`

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 901 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> The interfaces of the functions `fsck_obj()` and `fsck_obj_buffer()` are
> somewhat similar to one another. The only difference between those two
> is that `fsck_obj()` takes an already-parsed object as input, whereas
> `fsck_obj_buffer()` parses the buffer and then calls `fsck_obj()`.
>
> Furthermore, `fsck_obj()` has no callers other than `fsck_obj_buffer()`.
>
> Refactor the code by merging those two functions. This makes it obvious
> which function does what, and it allows us to get rid of the early in

s/early/early return/ ?

> `fsck_obj()` in case `SEEN` is set as the only caller unconditionally
> clears that bit before calling it anyway.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/fsck.c | 47 ++++++++++++++++++++---------------------------
>  1 file changed, 20 insertions(+), 27 deletions(-)
>

The patch looks good!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 03/10] builtin/fsck: de-globalize option handling

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 611 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> In subsequent commits we're about to rework some of the option handling
> in git-fsck(1) a bit. It is currently a bit of a mess though due to lots
> of global state that makes it hard to see which flags are used where
> exactly.
>
> Refactor the code by moving the fsck options into `cmd_fsck()`. This
> allows us to convert some of the options into function-local variables.
>

Nice. I was wondering how much work this would involve, but it seems
like these variables are only used in `cmd_fsck()` anyway, so they
didn't even have to be global. Good spotting.

[snip]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 04/10] builtin/fsck: don't check alternates with "--no-full"

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 3249 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> According to git-fsck(1), the "--full" option behaves in the following
> way:
>
>   Check not just objects in GIT_OBJECT_DIRECTORY ($GIT_DIR/objects), but
>   also the ones found in alternate object pools listed in
>   GIT_ALTERNATE_OBJECT_DIRECTORIES or $GIT_DIR/objects/info/alternates,
>   and in packed Git archives found in $GIT_DIR/objects/pack and
>   corresponding pack subdirectories in alternate object pools.
>
> So ultimately, it is supposed to control two things: (1) whether we only
> check the main object directory, and (2) whether we check packfiles.
>
> In its current state though, the flag only controls whether we check
> packfiles or not, and if so we verify packfiles of all attached sources.
> But we also have checks for loose objects in git-fsck(1), and here we
> unconditionally check them in all sources.
>

To reiterate,

without '--full': Check local loose + alternate loose. No packed objects

with '--full': Check local loose + alternate loose. local packed +
alternates packed.

And we want to only do local loose in the latter. Makes sense.

>
> The flag is arguably conflating two unrelated concerns with one another,
> and it really should be split up into two flags: one that controls how
> thorough we want to check individual sources, and one that controls
> which sources we want to check in the first place. So ideally, we would
> have:
>
>   - "--include-alternates": check all sources, not only the local one.
>
>   - "--include-optimized-objects": check not only loose objects, but
>     also those that have been packed. Note that we explicitly don't say
>     "--include-packed-objects" here to be more backend-agnostic.
>
>   - "--full": implies both of the above flags.
>
> This feels out of scope for this series though. So for now, simply fix
> the code by honoring locality of the sources for loose objects.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/fsck.c  | 3 ++-
>  t/t1450-fsck.sh | 5 +++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 5132ff0f15..3f6056535f 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -1047,7 +1047,8 @@ int cmd_fsck(int argc,
>  				    mark_object_for_connectivity, repo, 0);
>  	} else {
>  		for (source = repo->objects->sources; source; source = source->next)
> -			fsck_source(repo, source);
> +			if (check_full || source->local)
> +				fsck_source(repo, source);
>

So we check the local bit and only fsck that source. Looks good.

>  		if (check_full) {
>  			struct packed_git *p;
> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
> index 77cd96de78..1b4074304c 100755
> --- a/t/t1450-fsck.sh
> +++ b/t/t1450-fsck.sh
> @@ -844,6 +844,11 @@ test_expect_success 'alternate objects are correctly blamed' '
>  	echo "../../alt.git/objects" >.git/objects/info/alternates &&
>  	mkdir alt.git/objects/$(dirname $path) &&
>  	>alt.git/objects/$(dirname $path)/$(basename $path) &&
> +
> +	# Without "--full", only the local object source is checked.
> +	git fsck --no-full >out 2>&1 &&
> +	test_must_be_empty out &&
> +
>  	test_must_fail git fsck >out 2>&1 &&
>  	test_grep alt.git out
>  '
>
> --
> 2.55.0.822.g20453c30eb.dirty

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 05/10] odb: provide infrastructure for pluggable fsck checks

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 3995 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> The on-disk consistency checks in git-fsck(1) are conceptually
> backend-specific: while connectivity checks and object-level parsing
> checks are generic, verifying the physical integrity of packfiles and
> loose objects is meaningful only to backends that use these formats:
> Having these checks live in "builtin/fsck.c" violates that layering,
> because it forces the command to reach directly into format-specific
> internals.
>
> Provide new infrastructure to make these format-specific checks
> pluggable and implement stubs for the different source types we already
> have. In subsequent commits we'll move functionality over piece by
> piece.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/fsck.c        | 16 +++++++++++-----
>  odb.c                 |  9 +++++++++
>  odb.h                 | 23 +++++++++++++++++++++++
>  odb/source-files.c    | 13 +++++++++++++
>  odb/source-inmemory.c |  8 ++++++++
>  odb/source-loose.c    |  7 +++++++
>  odb/source-packed.c   |  8 ++++++++
>  odb/source.h          | 21 +++++++++++++++++++++
>  8 files changed, 100 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 3f6056535f..adbe192e56 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -965,7 +965,9 @@ int cmd_fsck(int argc,
>  	     const char *prefix,
>  	     struct repository *repo)
>  {
> -	int check_full = 1;
> +	struct odb_fsck_options odb_fsck_opts = {
> +		.flags = ODB_FSCK_FULL,
> +	};
>  	int keep_cache_objects = 0;
>  	int name_objects = 0;
>  	int check_references = 1;
> @@ -977,7 +979,8 @@ int cmd_fsck(int argc,
>  		OPT_BOOL(0, "root", &show_root, N_("report root nodes")),
>  		OPT_BOOL(0, "cache", &keep_cache_objects, N_("make index objects head nodes")),
>  		OPT_BOOL(0, "reflogs", &include_reflogs, N_("make reflogs head nodes (default)")),
> -		OPT_BOOL(0, "full", &check_full, N_("also consider packs and alternate objects")),

Question: OPT_BOOL sets 'check_full' to 0 when using '--no-full', does
OPT_BIT provide similar functionality?

> +		OPT_BIT(0, "full", &odb_fsck_opts.flags,
> +			N_("also consider packs and alternate objects"), ODB_FSCK_FULL),
>  		OPT_BOOL(0, "connectivity-only", &connectivity_only, N_("check only connectivity")),
>  		OPT_BOOL(0, "strict", &check_strict, N_("enable more strict checking")),
>  		OPT_BOOL(0, "lost-found", &write_lost_and_found,
> @@ -1018,7 +1021,7 @@ int cmd_fsck(int argc,
>  		show_progress = 0;
>
>  	if (write_lost_and_found) {
> -		check_full = 1;
> +		odb_fsck_opts.flags |= ODB_FSCK_FULL;
>  		include_reflogs = 0;
>  	}
>
> @@ -1047,10 +1050,13 @@ int cmd_fsck(int argc,
>  				    mark_object_for_connectivity, repo, 0);
>  	} else {
>  		for (source = repo->objects->sources; source; source = source->next)
> -			if (check_full || source->local)
> +			if ((odb_fsck_opts.flags & ODB_FSCK_FULL) || source->local)
>  				fsck_source(repo, source);
>
> -		if (check_full) {
> +		if (odb_fsck(repo->objects, &odb_fsck_opts) < 0)
> +			errors_found |= ERROR_OBJECT;
> +

So most of the functionality will move into this and we'll cleanup
around in the following commits.

> +		if (odb_fsck_opts.flags & ODB_FSCK_FULL) {
>  			struct packed_git *p;
>  			uint32_t total = 0, count = 0;
>  			struct progress *progress = NULL;
> diff --git a/odb.c b/odb.c
> index 1fe20808eb..766043b685 100644
> --- a/odb.c
> +++ b/odb.c
> @@ -1177,3 +1177,12 @@ void odb_reprepare(struct object_database *o)
>  {
>  	odb_prepare(o, ODB_PREPARE_FLUSH_CACHES);
>  }
> +
> +int odb_fsck(struct object_database *odb, struct odb_fsck_options *options)
> +{
> +	int ret = 0;
> +	for (struct odb_source *source = odb->sources; source; source = source->next)
> +		if ((options->flags & ODB_FSCK_FULL) || source->local)
> +			ret |= odb_source_fsck(source, options);
> +	return ret;
> +}

The odb iterates over all the sources and does a consistency check,
looks good.

[snip]

The changes in this commit look to be in order.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 08/10] builtin/fsck: move bitmap verification into the packed source

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 774 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> The checks for bitmaps live in `verify_bitmap_files()`, which is called
> by "builtin/fsck.c". These checks are obviously specific to the "packed"
> backend.
>
> Move the logic into `odb_source_packed_fsck()`. As in preceding commits,
> this means that we now properly honor both "--connectivity-only" and
> "--no-full". Furthermore, we drop the dedicated `ERROR_BITMAP` bit and
> instead use the generic `ERROR_OBJECT` bit.
>
> Note that this change also adapts `verify_bitmap_files()` to be
> focussed on a single "packed" source instead of verifying bitmaps from

nit: s/focussed/focused

> all sources. This change is required as we already know to loop around
> the sources in `odb_fsck()` itself.
>

[snip]

changes look good

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 00/10] odb: make consistency checks pluggable

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 594 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> Hi,
>
> this patch series makes object database consistency checks pluggable.
>
> This series is built on top of 2c3adbb2c4 (The 18th batch, 2026-08-24)
> with the following two dependencsie merged into it:
>
>   - ps/odb-eagerly-load-alternates at 0076dc9f81 (odb: drop
>     `alternates_db` field, 2026-08-17)
>
>   - ps/odb-pluggable-pack-generation at 5176dd3d05 (bundle: generate
>     packfiles via the object database, 2026-08-21)
>
> Thanks!
>
> Patrick
>

Hi,

I reviewed the series and only have some small nits/questions. Rest
looks good :)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 02/10] builtin/fsck: merge `fsck_obj_buffer()` and `fsck_obj()`

[Patrick Steinhardt]

On Thu, Aug 27, 2026 at 06:03:08AM -0400, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > The interfaces of the functions `fsck_obj()` and `fsck_obj_buffer()` are
> > somewhat similar to one another. The only difference between those two
> > is that `fsck_obj()` takes an already-parsed object as input, whereas
> > `fsck_obj_buffer()` parses the buffer and then calls `fsck_obj()`.
> >
> > Furthermore, `fsck_obj()` has no callers other than `fsck_obj_buffer()`.
> >
> > Refactor the code by merging those two functions. This makes it obvious
> > which function does what, and it allows us to get rid of the early in
> 
> s/early/early return/ ?

Indeed. Will fix.

Patrick

Re: [PATCH 01/10] builtin/fsck: use `fsck_obj_buffer()` when checking loose objects

[Patrick Steinhardt]

On Thu, Aug 27, 2026 at 06:00:37AM -0400, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > When checking loose objects we manually parse the object buffer we have
> > read from the on-disk file, mark the object and then call `fsck_obj()`.
> > Almost the exact same steps are also performed by `fsck_obj_buffer()`.
> >
> 
> I was wondering what the difference was, there seems to be none, nit:
> perhaps we can drop 'Almost'.

I actually didn't have the "almost" initially but added it later, but
there isn't really much of a reason why specifically I did so. I'll drop
it again.

> > diff --git a/builtin/fsck.c b/builtin/fsck.c
> > index 892c5661d9..3c4127f4d8 100644
> > --- a/builtin/fsck.c
> > +++ b/builtin/fsck.c
> > @@ -751,21 +750,7 @@ static int fsck_loose(const struct object_id *oid, const char *path,
> >  	if (!contents && type != OBJ_BLOB)
> >  		BUG("read_loose_object streamed a non-blob");
> >
> > -	obj = parse_object_buffer(data->repo, oid, type, size,
> > -				  contents, &eaten);
> > -
> > -	if (!obj) {
> > -		errors_found |= ERROR_OBJECT;
> > -		error(_("%s: object could not be parsed: %s"),
> > -		      oid_to_hex(oid), path);
> > -		if (!eaten)
> > -			free(contents);
> 
> This is now moved to the bottom below fsck_obj_buffer() call. So that's
> okay.
> 
> > -		return 0; /* keep checking other objects */
> > -	}
> > -
> > -	obj->flags &= ~(REACHABLE | SEEN);
> > -	obj->flags |= HAS_OBJ;
> > -	if (fsck_obj(data->repo, obj, contents, size))
> > +	if (fsck_obj_buffer(oid, type, size, contents, &eaten, data->repo))
> >  		errors_found |= ERROR_OBJECT;
> >
> 
> I see `fsck_obj_buffer()` also sets adds the `ERROR_OBJECT` flag, but
> that's okay.

Yeah. We could just drop this, but then it'd feel a tiny bit weird as we
call the function without checking its return value at all. So I decided
to just keep this as-is.

Patrick

Re: [PATCH 04/10] builtin/fsck: don't check alternates with "--no-full"

[Patrick Steinhardt]

On Thu, Aug 27, 2026 at 06:12:13AM -0400, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > According to git-fsck(1), the "--full" option behaves in the following
> > way:
> >
> >   Check not just objects in GIT_OBJECT_DIRECTORY ($GIT_DIR/objects), but
> >   also the ones found in alternate object pools listed in
> >   GIT_ALTERNATE_OBJECT_DIRECTORIES or $GIT_DIR/objects/info/alternates,
> >   and in packed Git archives found in $GIT_DIR/objects/pack and
> >   corresponding pack subdirectories in alternate object pools.
> >
> > So ultimately, it is supposed to control two things: (1) whether we only
> > check the main object directory, and (2) whether we check packfiles.
> >
> > In its current state though, the flag only controls whether we check
> > packfiles or not, and if so we verify packfiles of all attached sources.
> > But we also have checks for loose objects in git-fsck(1), and here we
> > unconditionally check them in all sources.
> >
> 
> To reiterate,
> 
> without '--full': Check local loose + alternate loose. No packed objects
> 
> with '--full': Check local loose + alternate loose. local packed +
> alternates packed.
> 
> And we want to only do local loose in the latter. Makes sense.

s/latter/former/, but other than this: yes.

Patrick

Re: [PATCH 05/10] odb: provide infrastructure for pluggable fsck checks

[Patrick Steinhardt]

On Thu, Aug 27, 2026 at 06:49:38AM -0400, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/builtin/fsck.c b/builtin/fsck.c
> > index 3f6056535f..adbe192e56 100644
> > --- a/builtin/fsck.c
> > +++ b/builtin/fsck.c
> > @@ -977,7 +979,8 @@ int cmd_fsck(int argc,
> >  		OPT_BOOL(0, "root", &show_root, N_("report root nodes")),
> >  		OPT_BOOL(0, "cache", &keep_cache_objects, N_("make index objects head nodes")),
> >  		OPT_BOOL(0, "reflogs", &include_reflogs, N_("make reflogs head nodes (default)")),
> > -		OPT_BOOL(0, "full", &check_full, N_("also consider packs and alternate objects")),
> 
> Question: OPT_BOOL sets 'check_full' to 0 when using '--no-full', does
> OPT_BIT provide similar functionality?

Yes, it does.

> > @@ -1047,10 +1050,13 @@ int cmd_fsck(int argc,
> >  				    mark_object_for_connectivity, repo, 0);
> >  	} else {
> >  		for (source = repo->objects->sources; source; source = source->next)
> > -			if (check_full || source->local)
> > +			if ((odb_fsck_opts.flags & ODB_FSCK_FULL) || source->local)
> >  				fsck_source(repo, source);
> >
> > -		if (check_full) {
> > +		if (odb_fsck(repo->objects, &odb_fsck_opts) < 0)
> > +			errors_found |= ERROR_OBJECT;
> > +
> 
> So most of the functionality will move into this and we'll cleanup
> around in the following commits.

Yup, exactly.

Patrick

Re: [PATCH 08/10] builtin/fsck: move bitmap verification into the packed source

[Patrick Steinhardt]

On Thu, Aug 27, 2026 at 06:54:51AM -0400, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > The checks for bitmaps live in `verify_bitmap_files()`, which is called
> > by "builtin/fsck.c". These checks are obviously specific to the "packed"
> > backend.
> >
> > Move the logic into `odb_source_packed_fsck()`. As in preceding commits,
> > this means that we now properly honor both "--connectivity-only" and
> > "--no-full". Furthermore, we drop the dedicated `ERROR_BITMAP` bit and
> > instead use the generic `ERROR_OBJECT` bit.
> >
> > Note that this change also adapts `verify_bitmap_files()` to be
> > focussed on a single "packed" source instead of verifying bitmaps from
> 
> nit: s/focussed/focused

You can actually use both spellings [1], where "focussed" is more
commonly used in the UK. Anyway, I'll change this to help our American
friends out there.

Patrick

[1]: https://en.wiktionary.org/wiki/focussed

[PATCH v2 00/10] odb: make consistency checks pluggable

[Patrick Steinhardt]

Hi,

this patch series makes object database consistency checks pluggable.

This series is built on top of 2c3adbb2c4 (The 18th batch, 2026-08-24)
with the following two dependencsie merged into it:

  - ps/odb-eagerly-load-alternates at 0076dc9f81 (odb: drop
    `alternates_db` field, 2026-08-17)

  - ps/odb-pluggable-pack-generation at 5176dd3d05 (bundle: generate
    packfiles via the object database, 2026-08-21)

Changes in v2:
  - Some commit message improvements.
  - Link to v1: https://patch.msgid.link/20260825-pks-odb-source-fsck-v1-0-b756de0bf24f@pks.im

Thanks!

Patrick

---
Patrick Steinhardt (10):
      builtin/fsck: use `fsck_obj_buffer()` when checking loose objects
      builtin/fsck: merge `fsck_obj_buffer()` and `fsck_obj()`
      builtin/fsck: de-globalize option handling
      builtin/fsck: don't check alternates with "--no-full"
      odb: provide infrastructure for pluggable fsck checks
      builtin/fsck: move packfile verification into the packed source
      builtin/fsck: move reverse index verification into the packed source
      builtin/fsck: move bitmap verification into the packed source
      builtin/fsck: move multi-pack index verification into the packed source
      builtin/fsck: move loose object verification into the loose source

 builtin/fsck.c                | 296 ++++++++----------------------------------
 odb.c                         |   9 ++
 odb.h                         |  33 +++++
 odb/source-files.c            |  13 ++
 odb/source-inmemory.c         |   8 ++
 odb/source-loose.c            |  92 +++++++++++++
 odb/source-packed.c           | 117 +++++++++++++++++
 odb/source.h                  |  21 +++
 pack-bitmap.c                 |  26 ++--
 pack-bitmap.h                 |   2 +-
 t/t1450-fsck.sh               |   5 +
 t/t5319-multi-pack-index.sh   |  13 ++
 t/t5325-reverse-index.sh      |   8 ++
 t/t5326-multi-pack-bitmaps.sh |  10 +-
 14 files changed, 394 insertions(+), 259 deletions(-)

Range-diff versus v1:

 1:  cf49376600 !  1:  1aec903546 builtin/fsck: use `fsck_obj_buffer()` when checking loose objects
    @@ Commit message
     
         When checking loose objects we manually parse the object buffer we have
         read from the on-disk file, mark the object and then call `fsck_obj()`.
    -    Almost the exact same steps are also performed by `fsck_obj_buffer()`.
    +    The exact same steps are also performed by `fsck_obj_buffer()`.
     
         Stop open-coding this logic and call `fsck_obj_buffer()` instead.
     
 2:  da2ca27041 !  2:  3804f0339e builtin/fsck: merge `fsck_obj_buffer()` and `fsck_obj()`
    @@ Commit message
         Furthermore, `fsck_obj()` has no callers other than `fsck_obj_buffer()`.
     
         Refactor the code by merging those two functions. This makes it obvious
    -    which function does what, and it allows us to get rid of the early in
    -    `fsck_obj()` in case `SEEN` is set as the only caller unconditionally
    -    clears that bit before calling it anyway.
    +    which function does what, and it allows us to get rid of the early
    +    return in `fsck_obj()` in case `SEEN` is set as the only caller
    +    unconditionally clears that bit before calling it anyway.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
 3:  a24506f55e =  3:  b2cb9032cf builtin/fsck: de-globalize option handling
 4:  f6a407efd0 =  4:  10ee3b8baf builtin/fsck: don't check alternates with "--no-full"
 5:  31841a1f05 =  5:  1e65eec60e odb: provide infrastructure for pluggable fsck checks
 6:  2cd6d71983 =  6:  0b8cf751aa builtin/fsck: move packfile verification into the packed source
 7:  c0559f1820 =  7:  3a38a75549 builtin/fsck: move reverse index verification into the packed source
 8:  96ae1ce3c6 !  8:  dd3a4c6cea builtin/fsck: move bitmap verification into the packed source
    @@ Commit message
         instead use the generic `ERROR_OBJECT` bit.
     
         Note that this change also adapts `verify_bitmap_files()` to be
    -    focussed on a single "packed" source instead of verifying bitmaps from
    +    focused on a single "packed" source instead of verifying bitmaps from
         all sources. This change is required as we already know to loop around
         the sources in `odb_fsck()` itself.
     
 9:  4721f4b4ba =  9:  90ada56b7f builtin/fsck: move multi-pack index verification into the packed source
10:  0b36829fd9 = 10:  b0f6fccae8 builtin/fsck: move loose object verification into the loose source

---
base-commit: 6b08999fb1b3ad0bad04d492dc206ad42839e274
change-id: 20260810-pks-odb-source-fsck-e64772c7ee5f

[PATCH v2 01/10] builtin/fsck: use `fsck_obj_buffer()` when checking loose objects

[Patrick Steinhardt]

When checking loose objects we manually parse the object buffer we have
read from the on-disk file, mark the object and then call `fsck_obj()`.
The exact same steps are also performed by `fsck_obj_buffer()`.

Stop open-coding this logic and call `fsck_obj_buffer()` instead.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 892c5661d9..3c4127f4d8 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -722,7 +722,6 @@ static int fsck_loose(const struct object_id *oid, const char *path,
 		      void *cb_data)
 {
 	struct for_each_loose_cb *data = cb_data;
-	struct object *obj;
 	enum object_type type = OBJ_NONE;
 	size_t size;
 	void *contents = NULL;
@@ -751,21 +750,7 @@ static int fsck_loose(const struct object_id *oid, const char *path,
 	if (!contents && type != OBJ_BLOB)
 		BUG("read_loose_object streamed a non-blob");
 
-	obj = parse_object_buffer(data->repo, oid, type, size,
-				  contents, &eaten);
-
-	if (!obj) {
-		errors_found |= ERROR_OBJECT;
-		error(_("%s: object could not be parsed: %s"),
-		      oid_to_hex(oid), path);
-		if (!eaten)
-			free(contents);
-		return 0; /* keep checking other objects */
-	}
-
-	obj->flags &= ~(REACHABLE | SEEN);
-	obj->flags |= HAS_OBJ;
-	if (fsck_obj(data->repo, obj, contents, size))
+	if (fsck_obj_buffer(oid, type, size, contents, &eaten, data->repo))
 		errors_found |= ERROR_OBJECT;
 
 	if (!eaten)

-- 
2.55.0.979.g7e5102b832.dirty

[PATCH v2 02/10] builtin/fsck: merge `fsck_obj_buffer()` and `fsck_obj()`

[Patrick Steinhardt]

The interfaces of the functions `fsck_obj()` and `fsck_obj_buffer()` are
somewhat similar to one another. The only difference between those two
is that `fsck_obj()` takes an already-parsed object as input, whereas
`fsck_obj_buffer()` parses the buffer and then calls `fsck_obj()`.

Furthermore, `fsck_obj()` has no callers other than `fsck_obj_buffer()`.

Refactor the code by merging those two functions. This makes it obvious
which function does what, and it allows us to get rid of the early
return in `fsck_obj()` in case `SEEN` is set as the only caller
unconditionally clears that bit before calling it anyway.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c | 47 ++++++++++++++++++++---------------------------
 1 file changed, 20 insertions(+), 27 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 3c4127f4d8..bed8481893 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -401,14 +401,27 @@ static void check_connectivity(struct repository *repo)
 	}
 }
 
-static int fsck_obj(struct repository *repo,
-		    struct object *obj, void *buffer, unsigned long size)
+static int fsck_obj_buffer(const struct object_id *oid, enum object_type type,
+			   unsigned long size, void *buffer, int *eaten, void *cb_data)
 {
+	struct repository *repo = cb_data;
+	struct object *obj;
 	int err;
 
-	if (obj->flags & SEEN)
-		return 0;
-	obj->flags |= SEEN;
+	/*
+	 * Note, buffer may be NULL if type is OBJ_BLOB. See
+	 * verify_packfile(), data_valid variable for details.
+	 */
+	obj = parse_object_buffer(repo, oid, type, size, buffer, eaten);
+	if (!obj) {
+		errors_found |= ERROR_OBJECT;
+		err = error(_("%s: object corrupt or missing"),
+			    oid_to_hex(oid));
+		goto out;
+	}
+
+	obj->flags &= ~REACHABLE;
+	obj->flags |= HAS_OBJ | SEEN;
 
 	if (verbose)
 		fprintf_ln(stderr, _("Checking %s %s"),
@@ -417,6 +430,7 @@ static int fsck_obj(struct repository *repo,
 
 	if (fsck_walk(obj, NULL, &fsck_obj_options))
 		objerror(repo, obj, _("broken links"));
+
 	err = fsck_object(obj, buffer, size, &fsck_obj_options);
 	if (err)
 		goto out;
@@ -442,32 +456,11 @@ static int fsck_obj(struct repository *repo,
 	}
 
 out:
-	if (obj->type == OBJ_TREE)
+	if (obj && obj->type == OBJ_TREE)
 		free_tree_buffer((struct tree *)obj);
 	return err;
 }
 
-static int fsck_obj_buffer(const struct object_id *oid, enum object_type type,
-			   unsigned long size, void *buffer, int *eaten, void *cb_data)
-{
-	struct repository *repo = cb_data;
-	struct object *obj;
-
-	/*
-	 * Note, buffer may be NULL if type is OBJ_BLOB. See
-	 * verify_packfile(), data_valid variable for details.
-	 */
-	obj = parse_object_buffer(repo, oid, type, size, buffer, eaten);
-	if (!obj) {
-		errors_found |= ERROR_OBJECT;
-		return error(_("%s: object corrupt or missing"),
-			     oid_to_hex(oid));
-	}
-	obj->flags &= ~(REACHABLE | SEEN);
-	obj->flags |= HAS_OBJ;
-	return fsck_obj(repo, obj, buffer, size);
-}
-
 static int default_refs;
 
 static void fsck_handle_reflog_oid(struct repository *repo,

-- 
2.55.0.979.g7e5102b832.dirty

[PATCH v2 03/10] builtin/fsck: de-globalize option handling

[Patrick Steinhardt]

In subsequent commits we're about to rework some of the option handling
in git-fsck(1) a bit. It is currently a bit of a mess though due to lots
of global state that makes it hard to see which flags are used where
exactly.

Refactor the code by moving the fsck options into `cmd_fsck()`. This
allows us to convert some of the options into function-local variables.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c | 45 ++++++++++++++++++++++-----------------------
 1 file changed, 22 insertions(+), 23 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index bed8481893..5132ff0f15 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -37,10 +37,8 @@ static int show_root;
 static int show_tags;
 static int show_unreachable;
 static int include_reflogs = 1;
-static int check_full = 1;
 static int connectivity_only;
 static int check_strict;
-static int keep_cache_objects;
 static struct fsck_options fsck_walk_options;
 static struct fsck_options fsck_obj_options;
 static int errors_found;
@@ -48,8 +46,6 @@ static int write_lost_and_found;
 static int verbose;
 static int show_progress = -1;
 static int show_dangling = 1;
-static int name_objects;
-static int check_references = 1;
 static timestamp_t now;
 #define ERROR_OBJECT 01
 #define ERROR_REACHABLE 02
@@ -964,30 +960,33 @@ static char const * const fsck_usage[] = {
 	NULL
 };
 
-static struct option fsck_opts[] = {
-	OPT__VERBOSE(&verbose, N_("be verbose")),
-	OPT_BOOL(0, "unreachable", &show_unreachable, N_("show unreachable objects")),
-	OPT_BOOL(0, "dangling", &show_dangling, N_("show dangling objects")),
-	OPT_BOOL(0, "tags", &show_tags, N_("report tags")),
-	OPT_BOOL(0, "root", &show_root, N_("report root nodes")),
-	OPT_BOOL(0, "cache", &keep_cache_objects, N_("make index objects head nodes")),
-	OPT_BOOL(0, "reflogs", &include_reflogs, N_("make reflogs head nodes (default)")),
-	OPT_BOOL(0, "full", &check_full, N_("also consider packs and alternate objects")),
-	OPT_BOOL(0, "connectivity-only", &connectivity_only, N_("check only connectivity")),
-	OPT_BOOL(0, "strict", &check_strict, N_("enable more strict checking")),
-	OPT_BOOL(0, "lost-found", &write_lost_and_found,
-				N_("write dangling objects in .git/lost-found")),
-	OPT_BOOL(0, "progress", &show_progress, N_("show progress")),
-	OPT_BOOL(0, "name-objects", &name_objects, N_("show verbose names for reachable objects")),
-	OPT_BOOL(0, "references", &check_references, N_("check reference database consistency")),
-	OPT_END(),
-};
-
 int cmd_fsck(int argc,
 	     const char **argv,
 	     const char *prefix,
 	     struct repository *repo)
 {
+	int check_full = 1;
+	int keep_cache_objects = 0;
+	int name_objects = 0;
+	int check_references = 1;
+	struct option fsck_opts[] = {
+		OPT__VERBOSE(&verbose, N_("be verbose")),
+		OPT_BOOL(0, "unreachable", &show_unreachable, N_("show unreachable objects")),
+		OPT_BOOL(0, "dangling", &show_dangling, N_("show dangling objects")),
+		OPT_BOOL(0, "tags", &show_tags, N_("report tags")),
+		OPT_BOOL(0, "root", &show_root, N_("report root nodes")),
+		OPT_BOOL(0, "cache", &keep_cache_objects, N_("make index objects head nodes")),
+		OPT_BOOL(0, "reflogs", &include_reflogs, N_("make reflogs head nodes (default)")),
+		OPT_BOOL(0, "full", &check_full, N_("also consider packs and alternate objects")),
+		OPT_BOOL(0, "connectivity-only", &connectivity_only, N_("check only connectivity")),
+		OPT_BOOL(0, "strict", &check_strict, N_("enable more strict checking")),
+		OPT_BOOL(0, "lost-found", &write_lost_and_found,
+					N_("write dangling objects in .git/lost-found")),
+		OPT_BOOL(0, "progress", &show_progress, N_("show progress")),
+		OPT_BOOL(0, "name-objects", &name_objects, N_("show verbose names for reachable objects")),
+		OPT_BOOL(0, "references", &check_references, N_("check reference database consistency")),
+		OPT_END(),
+	};
 	struct odb_source *source;
 	struct snapshot snap = {
 		.nr = 0,

-- 
2.55.0.979.g7e5102b832.dirty

[PATCH v2 04/10] builtin/fsck: don't check alternates with "--no-full"

[Patrick Steinhardt]

According to git-fsck(1), the "--full" option behaves in the following
way:

  Check not just objects in GIT_OBJECT_DIRECTORY ($GIT_DIR/objects), but
  also the ones found in alternate object pools listed in
  GIT_ALTERNATE_OBJECT_DIRECTORIES or $GIT_DIR/objects/info/alternates,
  and in packed Git archives found in $GIT_DIR/objects/pack and
  corresponding pack subdirectories in alternate object pools.

So ultimately, it is supposed to control two things: (1) whether we only
check the main object directory, and (2) whether we check packfiles.

In its current state though, the flag only controls whether we check
packfiles or not, and if so we verify packfiles of all attached sources.
But we also have checks for loose objects in git-fsck(1), and here we
unconditionally check them in all sources.

The flag is arguably conflating two unrelated concerns with one another,
and it really should be split up into two flags: one that controls how
thorough we want to check individual sources, and one that controls
which sources we want to check in the first place. So ideally, we would
have:

  - "--include-alternates": check all sources, not only the local one.

  - "--include-optimized-objects": check not only loose objects, but
    also those that have been packed. Note that we explicitly don't say
    "--include-packed-objects" here to be more backend-agnostic.

  - "--full": implies both of the above flags.

This feels out of scope for this series though. So for now, simply fix
the code by honoring locality of the sources for loose objects.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c  | 3 ++-
 t/t1450-fsck.sh | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 5132ff0f15..3f6056535f 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -1047,7 +1047,8 @@ int cmd_fsck(int argc,
 				    mark_object_for_connectivity, repo, 0);
 	} else {
 		for (source = repo->objects->sources; source; source = source->next)
-			fsck_source(repo, source);
+			if (check_full || source->local)
+				fsck_source(repo, source);
 
 		if (check_full) {
 			struct packed_git *p;
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 77cd96de78..1b4074304c 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -844,6 +844,11 @@ test_expect_success 'alternate objects are correctly blamed' '
 	echo "../../alt.git/objects" >.git/objects/info/alternates &&
 	mkdir alt.git/objects/$(dirname $path) &&
 	>alt.git/objects/$(dirname $path)/$(basename $path) &&
+
+	# Without "--full", only the local object source is checked.
+	git fsck --no-full >out 2>&1 &&
+	test_must_be_empty out &&
+
 	test_must_fail git fsck >out 2>&1 &&
 	test_grep alt.git out
 '

-- 
2.55.0.979.g7e5102b832.dirty

[PATCH v2 05/10] odb: provide infrastructure for pluggable fsck checks

[Patrick Steinhardt]

The on-disk consistency checks in git-fsck(1) are conceptually
backend-specific: while connectivity checks and object-level parsing
checks are generic, verifying the physical integrity of packfiles and
loose objects is meaningful only to backends that use these formats:
Having these checks live in "builtin/fsck.c" violates that layering,
because it forces the command to reach directly into format-specific
internals.

Provide new infrastructure to make these format-specific checks
pluggable and implement stubs for the different source types we already
have. In subsequent commits we'll move functionality over piece by
piece.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c        | 16 +++++++++++-----
 odb.c                 |  9 +++++++++
 odb.h                 | 23 +++++++++++++++++++++++
 odb/source-files.c    | 13 +++++++++++++
 odb/source-inmemory.c |  8 ++++++++
 odb/source-loose.c    |  7 +++++++
 odb/source-packed.c   |  8 ++++++++
 odb/source.h          | 21 +++++++++++++++++++++
 8 files changed, 100 insertions(+), 5 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 3f6056535f..adbe192e56 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -965,7 +965,9 @@ int cmd_fsck(int argc,
 	     const char *prefix,
 	     struct repository *repo)
 {
-	int check_full = 1;
+	struct odb_fsck_options odb_fsck_opts = {
+		.flags = ODB_FSCK_FULL,
+	};
 	int keep_cache_objects = 0;
 	int name_objects = 0;
 	int check_references = 1;
@@ -977,7 +979,8 @@ int cmd_fsck(int argc,
 		OPT_BOOL(0, "root", &show_root, N_("report root nodes")),
 		OPT_BOOL(0, "cache", &keep_cache_objects, N_("make index objects head nodes")),
 		OPT_BOOL(0, "reflogs", &include_reflogs, N_("make reflogs head nodes (default)")),
-		OPT_BOOL(0, "full", &check_full, N_("also consider packs and alternate objects")),
+		OPT_BIT(0, "full", &odb_fsck_opts.flags,
+			N_("also consider packs and alternate objects"), ODB_FSCK_FULL),
 		OPT_BOOL(0, "connectivity-only", &connectivity_only, N_("check only connectivity")),
 		OPT_BOOL(0, "strict", &check_strict, N_("enable more strict checking")),
 		OPT_BOOL(0, "lost-found", &write_lost_and_found,
@@ -1018,7 +1021,7 @@ int cmd_fsck(int argc,
 		show_progress = 0;
 
 	if (write_lost_and_found) {
-		check_full = 1;
+		odb_fsck_opts.flags |= ODB_FSCK_FULL;
 		include_reflogs = 0;
 	}
 
@@ -1047,10 +1050,13 @@ int cmd_fsck(int argc,
 				    mark_object_for_connectivity, repo, 0);
 	} else {
 		for (source = repo->objects->sources; source; source = source->next)
-			if (check_full || source->local)
+			if ((odb_fsck_opts.flags & ODB_FSCK_FULL) || source->local)
 				fsck_source(repo, source);
 
-		if (check_full) {
+		if (odb_fsck(repo->objects, &odb_fsck_opts) < 0)
+			errors_found |= ERROR_OBJECT;
+
+		if (odb_fsck_opts.flags & ODB_FSCK_FULL) {
 			struct packed_git *p;
 			uint32_t total = 0, count = 0;
 			struct progress *progress = NULL;
diff --git a/odb.c b/odb.c
index 1fe20808eb..766043b685 100644
--- a/odb.c
+++ b/odb.c
@@ -1177,3 +1177,12 @@ void odb_reprepare(struct object_database *o)
 {
 	odb_prepare(o, ODB_PREPARE_FLUSH_CACHES);
 }
+
+int odb_fsck(struct object_database *odb, struct odb_fsck_options *options)
+{
+	int ret = 0;
+	for (struct odb_source *source = odb->sources; source; source = source->next)
+		if ((options->flags & ODB_FSCK_FULL) || source->local)
+			ret |= odb_source_fsck(source, options);
+	return ret;
+}
diff --git a/odb.h b/odb.h
index e60174070f..76c15e48f5 100644
--- a/odb.h
+++ b/odb.h
@@ -206,6 +206,29 @@ void odb_prepare(struct object_database *o, enum odb_prepare_flags flags);
 /* Equivalent to `odb_prepare(o, ODB_PREPARE_FLUSH_CACHES)`. */
 void odb_reprepare(struct object_database *o);
 
+enum odb_fsck_flags {
+	/*
+	 * If set, perform a full consistency check for the full object
+	 * database, including all of its sources and the contents of their
+	 * optimized formats. Otherwise, only check the local source, and
+	 * restrict checks of its optimized formats to cheap structural
+	 * verification of their metadata.
+	 */
+	ODB_FSCK_FULL = (1 << 0),
+};
+
+/* Options that shall be passed to `odb_fsck()`. */
+struct odb_fsck_options {
+	enum odb_fsck_flags flags;
+};
+
+/*
+ * Run backend-specific integrity checks on all object sources. Each source
+ * performs the checks appropriate to its type. Returns 0 on success, a
+ * negative error code otherwise.
+ */
+int odb_fsck(struct object_database *odb, struct odb_fsck_options *opts);
+
 /*
  * Find source by its object directory path. Returns a `NULL` pointer in case
  * the source could not be found.
diff --git a/odb/source-files.c b/odb/source-files.c
index bd4fdf3a6c..f6fb560d2e 100644
--- a/odb/source-files.c
+++ b/odb/source-files.c
@@ -893,6 +893,18 @@ static int odb_source_files_generate_pack(struct odb_source *source UNUSED,
 	return 0;
 }
 
+static int odb_source_files_fsck(struct odb_source *source,
+				 struct odb_fsck_options *opts)
+{
+	struct odb_source_files *files = odb_source_files_downcast(source);
+	int ret = 0;
+
+	ret |= odb_source_fsck(&files->loose->base, opts);
+	ret |= odb_source_fsck(&files->packed->base, opts);
+
+	return ret;
+}
+
 struct odb_source_files *odb_source_files_new(struct object_database *odb,
 					      const char *path,
 					      bool local)
@@ -908,6 +920,7 @@ struct odb_source_files *odb_source_files_new(struct object_database *odb,
 	files->base.close = odb_source_files_close;
 	files->base.create_on_disk = odb_source_files_create_on_disk;
 	files->base.prepare = odb_source_files_prepare;
+	files->base.fsck = odb_source_files_fsck;
 	files->base.read_object_info = odb_source_files_read_object_info;
 	files->base.read_object_stream = odb_source_files_read_object_stream;
 	files->base.for_each_object = odb_source_files_for_each_object;
diff --git a/odb/source-inmemory.c b/odb/source-inmemory.c
index 795672adf2..ba0f86da26 100644
--- a/odb/source-inmemory.c
+++ b/odb/source-inmemory.c
@@ -1,6 +1,7 @@
 #include "git-compat-util.h"
 #include "object-file.h"
 #include "odb.h"
+#include "fsck.h"
 #include "odb/source-inmemory.h"
 #include "odb/streaming.h"
 #include "oidtree.h"
@@ -368,6 +369,12 @@ static void odb_source_inmemory_free(struct odb_source *source)
 	free(inmemory);
 }
 
+static int odb_source_inmemory_fsck(struct odb_source *source UNUSED,
+				    struct odb_fsck_options *opts UNUSED)
+{
+	return 0;
+}
+
 struct odb_source_inmemory *odb_source_inmemory_new(struct object_database *odb)
 {
 	struct odb_source_inmemory *source;
@@ -378,6 +385,7 @@ struct odb_source_inmemory *odb_source_inmemory_new(struct object_database *odb)
 	source->base.free = odb_source_inmemory_free;
 	source->base.close = odb_source_inmemory_close;
 	source->base.prepare = odb_source_inmemory_prepare;
+	source->base.fsck = odb_source_inmemory_fsck;
 	source->base.read_object_info = odb_source_inmemory_read_object_info;
 	source->base.read_object_stream = odb_source_inmemory_read_object_stream;
 	source->base.for_each_object = odb_source_inmemory_for_each_object;
diff --git a/odb/source-loose.c b/odb/source-loose.c
index bb3455dfbd..f68d3c4d6c 100644
--- a/odb/source-loose.c
+++ b/odb/source-loose.c
@@ -1031,6 +1031,12 @@ static void odb_source_loose_free(struct odb_source *source)
 	free(loose);
 }
 
+static int odb_source_loose_fsck(struct odb_source *source UNUSED,
+				 struct odb_fsck_options *opts UNUSED)
+{
+	return 0;
+}
+
 struct odb_source_loose *odb_source_loose_new(struct object_database *odb,
 					      const char *path,
 					      bool local)
@@ -1043,6 +1049,7 @@ struct odb_source_loose *odb_source_loose_new(struct object_database *odb,
 	loose->base.free = odb_source_loose_free;
 	loose->base.close = odb_source_loose_close;
 	loose->base.prepare = odb_source_loose_prepare;
+	loose->base.fsck = odb_source_loose_fsck;
 	loose->base.read_object_info = odb_source_loose_read_object_info;
 	loose->base.read_object_stream = odb_source_loose_read_object_stream;
 	loose->base.for_each_object = odb_source_loose_for_each_object;
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 630d955585..7aacf4bc45 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -2,6 +2,7 @@
 #include "abspath.h"
 #include "chdir-notify.h"
 #include "dir.h"
+#include "fsck.h"
 #include "git-zlib.h"
 #include "list-objects-filter-options.h"
 #include "mergesort.h"
@@ -826,6 +827,12 @@ static void odb_source_packed_free(struct odb_source *source)
 	free(packed);
 }
 
+static int odb_source_packed_fsck(struct odb_source *source UNUSED,
+				  struct odb_fsck_options *opts UNUSED)
+{
+	return 0;
+}
+
 struct odb_source_packed *odb_source_packed_new(struct object_database *odb,
 						const char *path,
 						bool local)
@@ -839,6 +846,7 @@ struct odb_source_packed *odb_source_packed_new(struct object_database *odb,
 	packed->base.free = odb_source_packed_free;
 	packed->base.close = odb_source_packed_close;
 	packed->base.prepare = odb_source_packed_prepare;
+	packed->base.fsck = odb_source_packed_fsck;
 	packed->base.read_object_info = odb_source_packed_read_object_info;
 	packed->base.read_object_stream = odb_source_packed_read_object_stream;
 	packed->base.for_each_object = odb_source_packed_for_each_object;
diff --git a/odb/source.h b/odb/source.h
index 559e2ea2e9..10a5dd5194 100644
--- a/odb/source.h
+++ b/odb/source.h
@@ -320,6 +320,17 @@ struct odb_source {
 	int (*generate_pack)(struct odb_source *source,
 			     struct odb_pack_generator **out,
 			     const struct odb_generate_pack_options *opts);
+
+	/*
+	 * This callback is expected to check the integrity of the object source
+	 * and report any errors found via the fsck options. The checks performed
+	 * are backend-specific.
+	 *
+	 * The callback is expected to return 0 on success, a negative error
+	 * code otherwise.
+	 */
+	int (*fsck)(struct odb_source *source,
+		    struct odb_fsck_options *options);
 };
 
 /*
@@ -588,4 +599,14 @@ static inline int odb_source_generate_pack(struct odb_source *source,
 	return source->generate_pack(source, out, opts);
 }
 
+/*
+ * Check the integrity of the object database source. The checks performed
+ * are backend-specific. Returns 0 on success, a negative error code otherwise.
+ */
+static inline int odb_source_fsck(struct odb_source *source,
+				  struct odb_fsck_options *opts)
+{
+	return source->fsck(source, opts);
+}
+
 #endif

-- 
2.55.0.979.g7e5102b832.dirty

[PATCH v2 06/10] builtin/fsck: move packfile verification into the packed source

[Patrick Steinhardt]

Move the packfile verification out of `cmd_fsck()` and into the "packed"
source. While doing so, thread the progress meter and object callback
through the newly introduced `struct odb_fsck_options` so that the
caller's preferences are honoured without exposing those details at the
"builtin/fsck.c" level.

Note that the old code reported failures when verifying packfiles with
the `ERROR_PACK` bit, which gets returned to the caller via the exit
code. This bit is neither exercised in our test suite nor is it
documented anywhere in our codebase. Furthermore, this bit is highly
specific to the object storage backend, which makes it a bad fit for the
new pluggable infrastructure. So instead of retaining these semantics,
we drop them and return the generic `ERROR_OBJECT` bit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c      | 33 ++++-----------------------------
 odb.h               |  7 +++++++
 odb/source-packed.c | 46 +++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 54 insertions(+), 32 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index adbe192e56..e504dae904 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -7,7 +7,6 @@
 #include "blob.h"
 #include "tag.h"
 #include "refs.h"
-#include "pack.h"
 #include "cache-tree.h"
 #include "fsck.h"
 #include "parse-options.h"
@@ -49,7 +48,6 @@ static int show_dangling = 1;
 static timestamp_t now;
 #define ERROR_OBJECT 01
 #define ERROR_REACHABLE 02
-#define ERROR_PACK 04
 #define ERROR_REFS 010
 #define ERROR_COMMIT_GRAPH 020
 #define ERROR_MULTI_PACK_INDEX 040
@@ -967,6 +965,8 @@ int cmd_fsck(int argc,
 {
 	struct odb_fsck_options odb_fsck_opts = {
 		.flags = ODB_FSCK_FULL,
+		.object_cb = fsck_obj_buffer,
+		.object_payload = repo,
 	};
 	int keep_cache_objects = 0;
 	int name_objects = 0;
@@ -1019,6 +1019,8 @@ int cmd_fsck(int argc,
 		show_progress = isatty(2);
 	if (verbose)
 		show_progress = 0;
+	if (show_progress)
+		odb_fsck_opts.flags |= ODB_FSCK_PROGRESS;
 
 	if (write_lost_and_found) {
 		odb_fsck_opts.flags |= ODB_FSCK_FULL;
@@ -1056,33 +1058,6 @@ int cmd_fsck(int argc,
 		if (odb_fsck(repo->objects, &odb_fsck_opts) < 0)
 			errors_found |= ERROR_OBJECT;
 
-		if (odb_fsck_opts.flags & ODB_FSCK_FULL) {
-			struct packed_git *p;
-			uint32_t total = 0, count = 0;
-			struct progress *progress = NULL;
-
-			if (show_progress) {
-				repo_for_each_pack(repo, p) {
-					if (open_pack_index(p))
-						continue;
-					total += p->num_objects;
-				}
-
-				progress = start_progress(repo,
-							  _("Checking objects"), total);
-			}
-
-			repo_for_each_pack(repo, p) {
-				/* verify gives error messages itself */
-				if (verify_pack(repo,
-						p, fsck_obj_buffer, repo,
-						progress, count))
-					errors_found |= ERROR_PACK;
-				count += p->num_objects;
-			}
-			stop_progress(&progress);
-		}
-
 		if (fsck_finish(&fsck_obj_options))
 			errors_found |= ERROR_OBJECT;
 	}
diff --git a/odb.h b/odb.h
index 76c15e48f5..0bf6c8d7d2 100644
--- a/odb.h
+++ b/odb.h
@@ -215,11 +215,18 @@ enum odb_fsck_flags {
 	 * verification of their metadata.
 	 */
 	ODB_FSCK_FULL = (1 << 0),
+
+	/* Display a progress meter, if sensible. */
+	ODB_FSCK_PROGRESS = (1 << 1),
 };
 
 /* Options that shall be passed to `odb_fsck()`. */
 struct odb_fsck_options {
 	enum odb_fsck_flags flags;
+
+	int (*object_cb)(const struct object_id *oid, enum object_type type,
+			 unsigned long size, void *buffer, int *eaten, void *cb_data);
+	void *object_payload;
 };
 
 /*
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 7aacf4bc45..0d3599f8fe 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -9,8 +9,10 @@
 #include "midx.h"
 #include "odb/source-packed.h"
 #include "odb/streaming.h"
+#include "pack.h"
 #include "packfile.h"
 #include "pack-bitmap.h"
+#include "progress.h"
 
 static int find_pack_entry(struct odb_source_packed *store,
 			   const struct object_id *oid,
@@ -827,10 +829,48 @@ static void odb_source_packed_free(struct odb_source *source)
 	free(packed);
 }
 
-static int odb_source_packed_fsck(struct odb_source *source UNUSED,
-				  struct odb_fsck_options *opts UNUSED)
+static int verify_packs(struct odb_source_packed *source,
+			struct odb_fsck_options *opts)
 {
-	return 0;
+	struct progress *progress = NULL;
+	struct packfile_list_entry *e;
+	uint32_t total = 0, count = 0;
+	int ret = 0;
+
+	if (opts->flags & ODB_FSCK_PROGRESS) {
+		for (e = packfile_store_get_packs(source); e; e = e->next) {
+			if (open_pack_index(e->pack))
+				continue;
+			total += e->pack->num_objects;
+		}
+
+		progress = start_progress(source->base.odb->repo,
+					  _("Checking objects"), total);
+	}
+
+	for (e = packfile_store_get_packs(source); e; e = e->next) {
+		/* verify gives error messages itself */
+		if (verify_pack(source->base.odb->repo, e->pack,
+				opts->object_cb, opts->object_payload,
+				progress, count))
+			ret = -1;
+		count += e->pack->num_objects;
+	}
+	stop_progress(&progress);
+
+	return ret;
+}
+
+static int odb_source_packed_fsck(struct odb_source *source,
+				  struct odb_fsck_options *opts)
+{
+	struct odb_source_packed *packed = odb_source_packed_downcast(source);
+	int ret = 0;
+
+	if ((opts->flags & ODB_FSCK_FULL) && verify_packs(packed, opts) < 0)
+		ret = -1;
+
+	return ret;
 }
 
 struct odb_source_packed *odb_source_packed_new(struct object_database *odb,

-- 
2.55.0.979.g7e5102b832.dirty

[PATCH v2 07/10] builtin/fsck: move reverse index verification into the packed source

[Patrick Steinhardt]

The checks for reverse indexes live in `check_pack_rev_indexes()`, which
is hosted in "builtin/fsck.c". These checks are obviously specific to
the "packed" backend.

Move the logic into `odb_source_packed_fsck()`. As in the preceding
commit, drop the dedicated `ERROR_PACK_REV_INDEX` bit and instead use
the generic `ERROR_OBJECT` bit.

Note that this changes behaviour in two ways:

  - The checks are now skipped when "--connectivity-only" was passed.
    This is because we don't even run `odb_fsck()` at all when that
    flag has been passed by the user, and not verifying data structures
    of the object database matches the documented intent of that flag,
    which is to only check the connectivity of reachable objects.

  - The checks are now skipped for non-local sources when "--no-full"
    was passed. This is, again, in line with the documented intent of
    that flag.

Add a test to cast these semantics into stone.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c           | 37 -------------------------------------
 odb/source-packed.c      | 39 +++++++++++++++++++++++++++++++++++++++
 t/t5325-reverse-index.sh |  8 ++++++++
 3 files changed, 47 insertions(+), 37 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index e504dae904..06e72877f3 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -23,7 +23,6 @@
 #include "run-command.h"
 #include "sparse-index.h"
 #include "worktree.h"
-#include "pack-revindex.h"
 #include "pack-bitmap.h"
 
 #define REACHABLE 0x0001
@@ -51,7 +50,6 @@ static timestamp_t now;
 #define ERROR_REFS 010
 #define ERROR_COMMIT_GRAPH 020
 #define ERROR_MULTI_PACK_INDEX 040
-#define ERROR_PACK_REV_INDEX 0100
 #define ERROR_BITMAP 0200
 
 static const char *describe_object(const struct object_id *oid)
@@ -890,40 +888,6 @@ static int mark_object_for_connectivity(const struct object_id *oid,
 	return 0;
 }
 
-static int check_pack_rev_indexes(struct repository *r, int show_progress)
-{
-	struct progress *progress = NULL;
-	struct packed_git *p;
-	uint32_t pack_count = 0;
-	int res = 0;
-
-	if (show_progress) {
-		repo_for_each_pack(r, p)
-			pack_count++;
-		progress = start_delayed_progress(r,
-						  "Verifying reverse pack-indexes", pack_count);
-		pack_count = 0;
-	}
-
-	repo_for_each_pack(r, p) {
-		int load_error = load_pack_revindex_from_disk(p);
-
-		if (load_error < 0) {
-			error(_("unable to load rev-index for pack '%s'"), p->pack_name);
-			res = ERROR_PACK_REV_INDEX;
-		} else if (!load_error &&
-			   !load_pack_revindex(r, p) &&
-			   verify_pack_revindex(p)) {
-			error(_("invalid rev-index for pack '%s'"), p->pack_name);
-			res = ERROR_PACK_REV_INDEX;
-		}
-		display_progress(progress, ++pack_count);
-	}
-	stop_progress(&progress);
-
-	return res;
-}
-
 static void fsck_refs(struct repository *r)
 {
 	struct child_process refs_verify = CHILD_PROCESS_INIT;
@@ -1104,7 +1068,6 @@ int cmd_fsck(int argc,
 		free_worktrees(worktrees);
 	}
 
-	errors_found |= check_pack_rev_indexes(repo, show_progress);
 	if (verify_bitmap_files(repo))
 		errors_found |= ERROR_BITMAP;
 
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 0d3599f8fe..e5e69636dd 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -10,6 +10,7 @@
 #include "odb/source-packed.h"
 #include "odb/streaming.h"
 #include "pack.h"
+#include "pack-revindex.h"
 #include "packfile.h"
 #include "pack-bitmap.h"
 #include "progress.h"
@@ -861,6 +862,41 @@ static int verify_packs(struct odb_source_packed *source,
 	return ret;
 }
 
+static int verify_reverse_indices(struct odb_source_packed *source,
+				  struct odb_fsck_options *opts)
+{
+	struct progress *progress = NULL;
+	struct packfile_list_entry *e;
+	uint32_t pack_count = 0;
+	int res = 0;
+
+	if (opts->flags & ODB_FSCK_PROGRESS) {
+		for (e = packfile_store_get_packs(source); e; e = e->next)
+			pack_count++;
+		progress = start_delayed_progress(source->base.odb->repo,
+						  "Verifying reverse pack-indexes", pack_count);
+		pack_count = 0;
+	}
+
+	for (e = packfile_store_get_packs(source); e; e = e->next) {
+		int load_error = load_pack_revindex_from_disk(e->pack);
+
+		if (load_error < 0) {
+			error(_("unable to load rev-index for pack '%s'"), e->pack->pack_name);
+			res = -1;
+		} else if (!load_error &&
+			   !load_pack_revindex(source->base.odb->repo, e->pack) &&
+			   verify_pack_revindex(e->pack)) {
+			error(_("invalid rev-index for pack '%s'"), e->pack->pack_name);
+			res = -1;
+		}
+		display_progress(progress, ++pack_count);
+	}
+	stop_progress(&progress);
+
+	return res;
+}
+
 static int odb_source_packed_fsck(struct odb_source *source,
 				  struct odb_fsck_options *opts)
 {
@@ -870,6 +906,9 @@ static int odb_source_packed_fsck(struct odb_source *source,
 	if ((opts->flags & ODB_FSCK_FULL) && verify_packs(packed, opts) < 0)
 		ret = -1;
 
+	if (verify_reverse_indices(packed, opts) < 0)
+		ret = -1;
+
 	return ret;
 }
 
diff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh
index 5493791938..6b81abf663 100755
--- a/t/t5325-reverse-index.sh
+++ b/t/t5325-reverse-index.sh
@@ -204,4 +204,12 @@ test_expect_success 'fsck catches invalid header: hash function' '
 		"reverse-index file .* has unsupported hash id"
 '
 
+test_expect_success 'fsck --no-full checks rev-index, --connectivity-only does not' '
+	test_must_fail git -C corrupt fsck --no-full 2>err &&
+	test_grep "has unsupported hash id" err &&
+
+	git -C corrupt fsck --connectivity-only 2>err &&
+	test_grep ! "has unsupported hash id" err
+'
+
 test_done

-- 
2.55.0.979.g7e5102b832.dirty

[PATCH v2 08/10] builtin/fsck: move bitmap verification into the packed source

[Patrick Steinhardt]

The checks for bitmaps live in `verify_bitmap_files()`, which is called
by "builtin/fsck.c". These checks are obviously specific to the "packed"
backend.

Move the logic into `odb_source_packed_fsck()`. As in preceding commits,
this means that we now properly honor both "--connectivity-only" and
"--no-full". Furthermore, we drop the dedicated `ERROR_BITMAP` bit and
instead use the generic `ERROR_OBJECT` bit.

Note that this change also adapts `verify_bitmap_files()` to be
focused on a single "packed" source instead of verifying bitmaps from
all sources. This change is required as we already know to loop around
the sources in `odb_fsck()` itself.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c                |  5 -----
 odb/source-packed.c           |  3 +++
 pack-bitmap.c                 | 26 ++++++++++----------------
 pack-bitmap.h                 |  2 +-
 t/t5326-multi-pack-bitmaps.sh | 10 +++++++++-
 5 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 06e72877f3..2f7d29aa56 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -23,7 +23,6 @@
 #include "run-command.h"
 #include "sparse-index.h"
 #include "worktree.h"
-#include "pack-bitmap.h"
 
 #define REACHABLE 0x0001
 #define SEEN      0x0002
@@ -50,7 +49,6 @@ static timestamp_t now;
 #define ERROR_REFS 010
 #define ERROR_COMMIT_GRAPH 020
 #define ERROR_MULTI_PACK_INDEX 040
-#define ERROR_BITMAP 0200
 
 static const char *describe_object(const struct object_id *oid)
 {
@@ -1068,9 +1066,6 @@ int cmd_fsck(int argc,
 		free_worktrees(worktrees);
 	}
 
-	if (verify_bitmap_files(repo))
-		errors_found |= ERROR_BITMAP;
-
 	check_connectivity(repo);
 
 	if (repo->settings.core_commit_graph) {
diff --git a/odb/source-packed.c b/odb/source-packed.c
index e5e69636dd..2b5dc502f5 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -909,6 +909,9 @@ static int odb_source_packed_fsck(struct odb_source *source,
 	if (verify_reverse_indices(packed, opts) < 0)
 		ret = -1;
 
+	if (verify_bitmap_files(packed))
+		ret = -1;
+
 	return ret;
 }
 
diff --git a/pack-bitmap.c b/pack-bitmap.c
index e0fb57d332..3de8e9590c 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -3410,28 +3410,22 @@ static int verify_bitmap_file(const struct git_hash_algo *algop,
 	return res;
 }
 
-int verify_bitmap_files(struct repository *r)
+int verify_bitmap_files(struct odb_source_packed *source)
 {
-	struct odb_source *source;
-	struct packed_git *p;
+	struct packfile_list_entry *e;
+	struct multi_pack_index *m;
 	int res = 0;
 
-	for (source = r->objects->sources; source; source = source->next) {
-		struct odb_source_files *files = odb_source_files_downcast(source);
-		struct multi_pack_index *m = get_multi_pack_index(files->packed);
-		char *midx_bitmap_name;
-
-		if (!m)
-			continue;
-
-		midx_bitmap_name = midx_bitmap_filename(m);
-		res |= verify_bitmap_file(r->hash_algo, midx_bitmap_name);
+	m = get_multi_pack_index(source);
+	if (m) {
+		char *midx_bitmap_name = midx_bitmap_filename(m);
+		res |= verify_bitmap_file(source->base.odb->repo->hash_algo, midx_bitmap_name);
 		free(midx_bitmap_name);
 	}
 
-	repo_for_each_pack(r, p) {
-		char *pack_bitmap_name = pack_bitmap_filename(p);
-		res |= verify_bitmap_file(r->hash_algo, pack_bitmap_name);
+	for (e = packfile_store_get_packs(source); e; e = e->next) {
+		char *pack_bitmap_name = pack_bitmap_filename(e->pack);
+		res |= verify_bitmap_file(source->base.odb->repo->hash_algo, pack_bitmap_name);
 		free(pack_bitmap_name);
 	}
 
diff --git a/pack-bitmap.h b/pack-bitmap.h
index 1385027c1f..847ad4762d 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -205,7 +205,7 @@ int bitmap_is_midx(struct bitmap_index *bitmap_git);
 
 int bitmap_is_preferred_refname(struct repository *r, const char *refname);
 
-int verify_bitmap_files(struct repository *r);
+int verify_bitmap_files(struct odb_source_packed *source);
 
 struct ewah_bitmap *read_bitmap(const unsigned char *map,
 				size_t map_size, size_t *map_pos);
diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh
index 86beab1dae..8047459b00 100755
--- a/t/t5326-multi-pack-bitmaps.sh
+++ b/t/t5326-multi-pack-bitmaps.sh
@@ -498,7 +498,15 @@ test_expect_success 'git fsck correctly identifies good and bad bitmaps' '
 	corrupt_file "$packbitmap" &&
 	test_must_fail git fsck 2>err &&
 	test_grep "bitmap file '\''$midxbitmap'\'' has invalid checksum" err &&
-	test_grep "bitmap file '\''$packbitmap'\'' has invalid checksum" err
+	test_grep "bitmap file '\''$packbitmap'\'' has invalid checksum" err &&
+
+	# The bitmap checks are performed with "--no-full", but not with
+	# "--connectivity-only".
+	test_must_fail git fsck --no-full 2>err &&
+	test_grep "bitmap file '\''$midxbitmap'\'' has invalid checksum" err &&
+	test_grep "bitmap file '\''$packbitmap'\'' has invalid checksum" err &&
+	git fsck --connectivity-only 2>err &&
+	test_grep ! "invalid checksum" err
 '
 
 test_expect_success 'corrupt MIDX with bitmap causes fallback' '

-- 
2.55.0.979.g7e5102b832.dirty

[PATCH v2 09/10] builtin/fsck: move multi-pack index verification into the packed source

[Patrick Steinhardt]

The checks for multi-pack indexes are hosted in `cmd_fsck()` directly.
These checks are obviously specific to the "packed" backend.

Move the logic into `odb_source_packed_fsck()`. As in preceding commits,
this means that we now properly honor both "--connectivity-only" and
"--no-full". Furthermore, we drop the dedicated `ERROR_MULTI_PACK_INDEX`
bit and instead use the generic `ERROR_OBJECT` bit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c              | 18 ------------------
 odb/source-packed.c         | 27 +++++++++++++++++++++++++++
 t/t5319-multi-pack-index.sh | 13 +++++++++++++
 3 files changed, 40 insertions(+), 18 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 2f7d29aa56..7eaea340b0 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -48,7 +48,6 @@ static timestamp_t now;
 #define ERROR_REACHABLE 02
 #define ERROR_REFS 010
 #define ERROR_COMMIT_GRAPH 020
-#define ERROR_MULTI_PACK_INDEX 040
 
 static const char *describe_object(const struct object_id *oid)
 {
@@ -1085,23 +1084,6 @@ int cmd_fsck(int argc,
 		}
 	}
 
-	if (repo->settings.core_multi_pack_index) {
-		struct child_process midx_verify = CHILD_PROCESS_INIT;
-
-		for (source = repo->objects->sources; source; source = source->next) {
-			child_process_init(&midx_verify);
-			midx_verify.git_cmd = 1;
-			strvec_pushl(&midx_verify.args, "multi-pack-index",
-				     "verify", "--object-dir", source->path, NULL);
-			if (show_progress)
-				strvec_push(&midx_verify.args, "--progress");
-			else
-				strvec_push(&midx_verify.args, "--no-progress");
-			if (run_command(&midx_verify))
-				errors_found |= ERROR_MULTI_PACK_INDEX;
-		}
-	}
-
 	free_snapshot_refs(&snap);
 	return errors_found;
 }
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 2b5dc502f5..9f42552377 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -14,6 +14,7 @@
 #include "packfile.h"
 #include "pack-bitmap.h"
 #include "progress.h"
+#include "run-command.h"
 
 static int find_pack_entry(struct odb_source_packed *store,
 			   const struct object_id *oid,
@@ -897,6 +898,29 @@ static int verify_reverse_indices(struct odb_source_packed *source,
 	return res;
 }
 
+static int verify_midx(struct odb_source_packed *source,
+		       struct odb_fsck_options *opts)
+{
+	struct child_process midx_verify = CHILD_PROCESS_INIT;
+	int ret = 0;
+
+	if (!source->base.odb->repo->settings.core_multi_pack_index)
+		return 0;
+
+	child_process_init(&midx_verify);
+	midx_verify.git_cmd = 1;
+	strvec_pushl(&midx_verify.args, "multi-pack-index",
+		     "verify", "--object-dir", source->base.path, NULL);
+	if (opts->flags & ODB_FSCK_PROGRESS)
+		strvec_push(&midx_verify.args, "--progress");
+	else
+		strvec_push(&midx_verify.args, "--no-progress");
+	if (run_command(&midx_verify))
+		ret = -1;
+
+	return ret;
+}
+
 static int odb_source_packed_fsck(struct odb_source *source,
 				  struct odb_fsck_options *opts)
 {
@@ -912,6 +936,9 @@ static int odb_source_packed_fsck(struct odb_source *source,
 	if (verify_bitmap_files(packed))
 		ret = -1;
 
+	if (verify_midx(packed, opts) < 0)
+		ret = -1;
+
 	return ret;
 }
 
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 68143cb5b7..20b010c33b 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -573,6 +573,19 @@ test_expect_success 'verify incorrect checksum' '
 		$objdir "incorrect checksum"
 '
 
+test_expect_success 'git fsck --no-full checks multi-pack-index, --connectivity-only does not' '
+	pos=$(($(wc -c <$objdir/pack/multi-pack-index) - 10)) &&
+	corrupt_midx_and_verify $pos \
+		"\377\377\377\377\377\377\377\377\377\377" \
+		$objdir "incorrect checksum" &&
+
+	test_must_fail git fsck --no-full 2>err &&
+	test_grep "incorrect checksum" err &&
+
+	git fsck --connectivity-only 2>err &&
+	test_grep ! "incorrect checksum" err
+'
+
 test_expect_success 'setup for v1-specific fsck tests' '
 	git -c midx.version=1 multi-pack-index write
 '

-- 
2.55.0.979.g7e5102b832.dirty

[PATCH v2 10/10] builtin/fsck: move loose object verification into the loose source

[Patrick Steinhardt]

The consistency checks for loose objects are hosted by "builtin/fsck.c".
These checks are obviously specific to the "loose" backend.

Move the logic into `odb_source_loose_fsck()`. Introduce a new "verbose"
flag so that we can properly retain semantics around whether or not we
want to print some status messages.

Note that this fixes a bug as a side effect: the progress meter was
captured in the callback data before `start_progress()` was even called,
so the per-subdirectory progress updates always operated on a NULL
pointer and the meter jumped straight from 0 to 256 upon completion. The
new code only sets up the callback data's progress meter after it has
been created, so the progress display now advances incrementally again.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c     | 91 ++----------------------------------------------------
 odb.h              |  3 ++
 odb/source-loose.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 93 insertions(+), 90 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 7eaea340b0..4af1d874cc 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -12,7 +12,6 @@
 #include "parse-options.h"
 #include "progress.h"
 #include "packfile.h"
-#include "object-file.h"
 #include "object-name.h"
 #include "odb.h"
 #include "odb/streaming.h"
@@ -695,88 +694,6 @@ static void process_refs(struct repository *repo, struct snapshot *snap)
 	}
 }
 
-struct for_each_loose_cb {
-	struct repository *repo;
-	struct progress *progress;
-};
-
-static int fsck_loose(const struct object_id *oid, const char *path,
-		      void *cb_data)
-{
-	struct for_each_loose_cb *data = cb_data;
-	enum object_type type = OBJ_NONE;
-	size_t size;
-	void *contents = NULL;
-	int eaten;
-	struct object_info oi = OBJECT_INFO_INIT;
-	struct object_id real_oid = *null_oid(data->repo->hash_algo);
-	int err = 0;
-
-	oi.sizep = &size;
-	oi.typep = &type;
-
-	if (read_loose_object(data->repo, path, oid, &real_oid, &contents, &oi) < 0) {
-		if (contents && !oideq(&real_oid, oid))
-			err = error(_("%s: hash-path mismatch, found at: %s"),
-				    oid_to_hex(&real_oid), path);
-		else
-			err = error(_("%s: object corrupt or missing: %s"),
-				    oid_to_hex(oid), path);
-	}
-	if (err < 0) {
-		errors_found |= ERROR_OBJECT;
-		free(contents);
-		return 0; /* keep checking other objects */
-	}
-
-	if (!contents && type != OBJ_BLOB)
-		BUG("read_loose_object streamed a non-blob");
-
-	if (fsck_obj_buffer(oid, type, size, contents, &eaten, data->repo))
-		errors_found |= ERROR_OBJECT;
-
-	if (!eaten)
-		free(contents);
-	return 0; /* keep checking other objects, even if we saw an error */
-}
-
-static int fsck_cruft(const char *basename, const char *path,
-		      void *data UNUSED)
-{
-	if (!starts_with(basename, "tmp_obj_"))
-		fprintf_ln(stderr, _("bad sha1 file: %s"), path);
-	return 0;
-}
-
-static int fsck_subdir(unsigned int nr, const char *path UNUSED, void *data)
-{
-	struct for_each_loose_cb *cb_data = data;
-	struct progress *progress = cb_data->progress;
-	display_progress(progress, nr + 1);
-	return 0;
-}
-
-static void fsck_source(struct repository *repo, struct odb_source *source)
-{
-	struct progress *progress = NULL;
-	struct for_each_loose_cb cb_data = {
-		.repo = source->odb->repo,
-		.progress = progress,
-	};
-
-	if (verbose)
-		fprintf_ln(stderr, _("Checking object directory"));
-
-	if (show_progress)
-		progress = start_progress(repo,
-					  _("Checking object directories"), 256);
-
-	for_each_loose_file_in_source(source, fsck_loose,
-				      fsck_cruft, fsck_subdir, &cb_data);
-	display_progress(progress, 256);
-	stop_progress(&progress);
-}
-
 static int fsck_cache_tree(struct repository *repo, struct cache_tree *it, const char *index_path)
 {
 	int i;
@@ -978,8 +895,10 @@ int cmd_fsck(int argc,
 
 	if (show_progress == -1)
 		show_progress = isatty(2);
-	if (verbose)
+	if (verbose) {
 		show_progress = 0;
+		odb_fsck_opts.flags |= ODB_FSCK_VERBOSE;
+	}
 	if (show_progress)
 		odb_fsck_opts.flags |= ODB_FSCK_PROGRESS;
 
@@ -1012,10 +931,6 @@ int cmd_fsck(int argc,
 		odb_for_each_object(repo->objects, NULL,
 				    mark_object_for_connectivity, repo, 0);
 	} else {
-		for (source = repo->objects->sources; source; source = source->next)
-			if ((odb_fsck_opts.flags & ODB_FSCK_FULL) || source->local)
-				fsck_source(repo, source);
-
 		if (odb_fsck(repo->objects, &odb_fsck_opts) < 0)
 			errors_found |= ERROR_OBJECT;
 
diff --git a/odb.h b/odb.h
index 0bf6c8d7d2..b87f281cbd 100644
--- a/odb.h
+++ b/odb.h
@@ -218,6 +218,9 @@ enum odb_fsck_flags {
 
 	/* Display a progress meter, if sensible. */
 	ODB_FSCK_PROGRESS = (1 << 1),
+
+	/* Be extra verbose when checking the database. */
+	ODB_FSCK_VERBOSE = (1 << 2),
 };
 
 /* Options that shall be passed to `odb_fsck()`. */
diff --git a/odb/source-loose.c b/odb/source-loose.c
index f68d3c4d6c..efef9ca61f 100644
--- a/odb/source-loose.c
+++ b/odb/source-loose.c
@@ -12,6 +12,7 @@
 #include "odb/streaming.h"
 #include "oidtree.h"
 #include "path.h"
+#include "progress.h"
 #include "repository.h"
 #include "strbuf.h"
 #include "tempfile.h"
@@ -1031,12 +1032,96 @@ static void odb_source_loose_free(struct odb_source *source)
 	free(loose);
 }
 
-static int odb_source_loose_fsck(struct odb_source *source UNUSED,
-				 struct odb_fsck_options *opts UNUSED)
+struct fsck_loose_data {
+	struct odb_source_loose *source;
+	struct odb_fsck_options *opts;
+	struct progress *progress;
+	bool error_found;
+};
+
+static int fsck_loose(const struct object_id *oid, const char *path,
+		      void *cb_data)
 {
+	struct fsck_loose_data *data = cb_data;
+	enum object_type type = OBJ_NONE;
+	size_t size;
+	void *contents = NULL;
+	int eaten = 0;
+	struct object_info oi = OBJECT_INFO_INIT;
+	struct object_id real_oid = *null_oid(data->source->base.odb->repo->hash_algo);
+	int err = 0;
+
+	oi.sizep = &size;
+	oi.typep = &type;
+
+	if (read_loose_object(data->source->base.odb->repo,
+			      path, oid, &real_oid, &contents, &oi) < 0) {
+		if (contents && !oideq(&real_oid, oid))
+			err = error(_("%s: hash-path mismatch, found at: %s"),
+				    oid_to_hex(&real_oid), path);
+		else
+			err = error(_("%s: object corrupt or missing: %s"),
+				    oid_to_hex(oid), path);
+	}
+	if (err < 0)
+		goto out;
+
+	if (!contents && type != OBJ_BLOB)
+		BUG("read_loose_object streamed a non-blob");
+
+	if (data->opts->object_cb(oid, type, size, contents, &eaten,
+				  data->opts->object_payload)) {
+		err = -1;
+		goto out;
+	}
+
+out:
+	if (err)
+		data->error_found = true;
+	if (!eaten)
+		free(contents);
+	return 0; /* keep checking other objects, even if we saw an error */
+}
+
+static int fsck_cruft(const char *basename, const char *path,
+		      void *data UNUSED)
+{
+	if (!starts_with(basename, "tmp_obj_"))
+		fprintf_ln(stderr, _("bad sha1 file: %s"), path);
+	return 0;
+}
+
+static int fsck_subdir(unsigned int nr, const char *path UNUSED, void *cb_data)
+{
+	struct fsck_loose_data *data = cb_data;
+	display_progress(data->progress, nr + 1);
 	return 0;
 }
 
+static int odb_source_loose_fsck(struct odb_source *source,
+				 struct odb_fsck_options *opts)
+{
+	struct odb_source_loose *loose = odb_source_loose_downcast(source);
+	struct fsck_loose_data data = {
+		.source = loose,
+		.opts = opts,
+	};
+
+	if (opts->flags & ODB_FSCK_VERBOSE)
+		fprintf_ln(stderr, _("Checking object directory"));
+
+	if (opts->flags & ODB_FSCK_PROGRESS)
+		data.progress = start_progress(source->odb->repo,
+					       _("Checking object directories"), 256);
+
+	for_each_loose_file_in_source(source, fsck_loose,
+				      fsck_cruft, fsck_subdir, &data);
+	display_progress(data.progress, 256);
+	stop_progress(&data.progress);
+
+	return data.error_found ? -1 : 0;
+}
+
 struct odb_source_loose *odb_source_loose_new(struct object_database *odb,
 					      const char *path,
 					      bool local)

-- 
2.55.0.979.g7e5102b832.dirty

Re: [PATCH 08/10] builtin/fsck: move bitmap verification into the packed source

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 1093 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Thu, Aug 27, 2026 at 06:54:51AM -0400, Karthik Nayak wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>>
>> > The checks for bitmaps live in `verify_bitmap_files()`, which is called
>> > by "builtin/fsck.c". These checks are obviously specific to the "packed"
>> > backend.
>> >
>> > Move the logic into `odb_source_packed_fsck()`. As in preceding commits,
>> > this means that we now properly honor both "--connectivity-only" and
>> > "--no-full". Furthermore, we drop the dedicated `ERROR_BITMAP` bit and
>> > instead use the generic `ERROR_OBJECT` bit.
>> >
>> > Note that this change also adapts `verify_bitmap_files()` to be
>> > focussed on a single "packed" source instead of verifying bitmaps from
>>
>> nit: s/focussed/focused
>
> You can actually use both spellings [1], where "focussed" is more
> commonly used in the UK. Anyway, I'll change this to help our American
> friends out there.
>
> Patrick
>
> [1]: https://en.wiktionary.org/wiki/focussed

I usually follow the UK spellings, I didn't know the focussed <> focused
variability.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v2 00/10] odb: make consistency checks pluggable

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 4830 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> Hi,
>
> this patch series makes object database consistency checks pluggable.
>
> This series is built on top of 2c3adbb2c4 (The 18th batch, 2026-08-24)
> with the following two dependencsie merged into it:
>
>   - ps/odb-eagerly-load-alternates at 0076dc9f81 (odb: drop
>     `alternates_db` field, 2026-08-17)
>
>   - ps/odb-pluggable-pack-generation at 5176dd3d05 (bundle: generate
>     packfiles via the object database, 2026-08-21)
>
> Changes in v2:
>   - Some commit message improvements.
>   - Link to v1: https://patch.msgid.link/20260825-pks-odb-source-fsck-v1-0-b756de0bf24f@pks.im
>
> Thanks!
>
> Patrick
>
> ---
> Patrick Steinhardt (10):
>       builtin/fsck: use `fsck_obj_buffer()` when checking loose objects
>       builtin/fsck: merge `fsck_obj_buffer()` and `fsck_obj()`
>       builtin/fsck: de-globalize option handling
>       builtin/fsck: don't check alternates with "--no-full"
>       odb: provide infrastructure for pluggable fsck checks
>       builtin/fsck: move packfile verification into the packed source
>       builtin/fsck: move reverse index verification into the packed source
>       builtin/fsck: move bitmap verification into the packed source
>       builtin/fsck: move multi-pack index verification into the packed source
>       builtin/fsck: move loose object verification into the loose source
>
>  builtin/fsck.c                | 296 ++++++++----------------------------------
>  odb.c                         |   9 ++
>  odb.h                         |  33 +++++
>  odb/source-files.c            |  13 ++
>  odb/source-inmemory.c         |   8 ++
>  odb/source-loose.c            |  92 +++++++++++++
>  odb/source-packed.c           | 117 +++++++++++++++++
>  odb/source.h                  |  21 +++
>  pack-bitmap.c                 |  26 ++--
>  pack-bitmap.h                 |   2 +-
>  t/t1450-fsck.sh               |   5 +
>  t/t5319-multi-pack-index.sh   |  13 ++
>  t/t5325-reverse-index.sh      |   8 ++
>  t/t5326-multi-pack-bitmaps.sh |  10 +-
>  14 files changed, 394 insertions(+), 259 deletions(-)
>
> Range-diff versus v1:
>
>  1:  cf49376600 !  1:  1aec903546 builtin/fsck: use `fsck_obj_buffer()` when checking loose objects
>     @@ Commit message
>
>          When checking loose objects we manually parse the object buffer we have
>          read from the on-disk file, mark the object and then call `fsck_obj()`.
>     -    Almost the exact same steps are also performed by `fsck_obj_buffer()`.
>     +    The exact same steps are also performed by `fsck_obj_buffer()`.
>
>          Stop open-coding this logic and call `fsck_obj_buffer()` instead.
>
>  2:  da2ca27041 !  2:  3804f0339e builtin/fsck: merge `fsck_obj_buffer()` and `fsck_obj()`
>     @@ Commit message
>          Furthermore, `fsck_obj()` has no callers other than `fsck_obj_buffer()`.
>
>          Refactor the code by merging those two functions. This makes it obvious
>     -    which function does what, and it allows us to get rid of the early in
>     -    `fsck_obj()` in case `SEEN` is set as the only caller unconditionally
>     -    clears that bit before calling it anyway.
>     +    which function does what, and it allows us to get rid of the early
>     +    return in `fsck_obj()` in case `SEEN` is set as the only caller
>     +    unconditionally clears that bit before calling it anyway.
>
>          Signed-off-by: Patrick Steinhardt <ps@pks.im>
>
>  3:  a24506f55e =  3:  b2cb9032cf builtin/fsck: de-globalize option handling
>  4:  f6a407efd0 =  4:  10ee3b8baf builtin/fsck: don't check alternates with "--no-full"
>  5:  31841a1f05 =  5:  1e65eec60e odb: provide infrastructure for pluggable fsck checks
>  6:  2cd6d71983 =  6:  0b8cf751aa builtin/fsck: move packfile verification into the packed source
>  7:  c0559f1820 =  7:  3a38a75549 builtin/fsck: move reverse index verification into the packed source
>  8:  96ae1ce3c6 !  8:  dd3a4c6cea builtin/fsck: move bitmap verification into the packed source
>     @@ Commit message
>          instead use the generic `ERROR_OBJECT` bit.
>
>          Note that this change also adapts `verify_bitmap_files()` to be
>     -    focussed on a single "packed" source instead of verifying bitmaps from
>     +    focused on a single "packed" source instead of verifying bitmaps from
>          all sources. This change is required as we already know to loop around
>          the sources in `odb_fsck()` itself.
>
>  9:  4721f4b4ba =  9:  90ada56b7f builtin/fsck: move multi-pack index verification into the packed source
> 10:  0b36829fd9 = 10:  b0f6fccae8 builtin/fsck: move loose object verification into the loose source
>
> ---
> base-commit: 6b08999fb1b3ad0bad04d492dc206ad42839e274
> change-id: 20260810-pks-odb-source-fsck-e64772c7ee5f

The changes here look good to me, thanks!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v2 05/10] odb: provide infrastructure for pluggable fsck checks

[Toon Claes]

Patrick Steinhardt <ps@pks.im> writes:

> The on-disk consistency checks in git-fsck(1) are conceptually
> backend-specific: while connectivity checks and object-level parsing
> checks are generic, verifying the physical integrity of packfiles and
> loose objects is meaningful only to backends that use these formats:
> Having these checks live in "builtin/fsck.c" violates that layering,
> because it forces the command to reach directly into format-specific
> internals.
>
> Provide new infrastructure to make these format-specific checks
> pluggable and implement stubs for the different source types we already
> have. In subsequent commits we'll move functionality over piece by
> piece.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/fsck.c        | 16 +++++++++++-----
>  odb.c                 |  9 +++++++++
>  odb.h                 | 23 +++++++++++++++++++++++
>  odb/source-files.c    | 13 +++++++++++++
>  odb/source-inmemory.c |  8 ++++++++
>  odb/source-loose.c    |  7 +++++++
>  odb/source-packed.c   |  8 ++++++++
>  odb/source.h          | 21 +++++++++++++++++++++
>  8 files changed, 100 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 3f6056535f..adbe192e56 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -965,7 +965,9 @@ int cmd_fsck(int argc,
>  	     const char *prefix,
>  	     struct repository *repo)
>  {
> -	int check_full = 1;
> +	struct odb_fsck_options odb_fsck_opts = {
> +		.flags = ODB_FSCK_FULL,
> +	};
>  	int keep_cache_objects = 0;
>  	int name_objects = 0;
>  	int check_references = 1;
> @@ -977,7 +979,8 @@ int cmd_fsck(int argc,
>  		OPT_BOOL(0, "root", &show_root, N_("report root nodes")),
>  		OPT_BOOL(0, "cache", &keep_cache_objects, N_("make index objects head nodes")),
>  		OPT_BOOL(0, "reflogs", &include_reflogs, N_("make reflogs head nodes (default)")),
> -		OPT_BOOL(0, "full", &check_full, N_("also consider packs and alternate objects")),
> +		OPT_BIT(0, "full", &odb_fsck_opts.flags,
> +			N_("also consider packs and alternate objects"), ODB_FSCK_FULL),
>  		OPT_BOOL(0, "connectivity-only", &connectivity_only, N_("check only connectivity")),
>  		OPT_BOOL(0, "strict", &check_strict, N_("enable more strict checking")),
>  		OPT_BOOL(0, "lost-found", &write_lost_and_found,
> @@ -1018,7 +1021,7 @@ int cmd_fsck(int argc,
>  		show_progress = 0;
>  
>  	if (write_lost_and_found) {
> -		check_full = 1;
> +		odb_fsck_opts.flags |= ODB_FSCK_FULL;
>  		include_reflogs = 0;
>  	}
>  
> @@ -1047,10 +1050,13 @@ int cmd_fsck(int argc,
>  				    mark_object_for_connectivity, repo, 0);
>  	} else {
>  		for (source = repo->objects->sources; source; source = source->next)
> -			if (check_full || source->local)
> +			if ((odb_fsck_opts.flags & ODB_FSCK_FULL) || source->local)
>  				fsck_source(repo, source);
>  
> -		if (check_full) {
> +		if (odb_fsck(repo->objects, &odb_fsck_opts) < 0)
> +			errors_found |= ERROR_OBJECT;
> +
> +		if (odb_fsck_opts.flags & ODB_FSCK_FULL) {
>  			struct packed_git *p;
>  			uint32_t total = 0, count = 0;
>  			struct progress *progress = NULL;
> diff --git a/odb.c b/odb.c
> index 1fe20808eb..766043b685 100644
> --- a/odb.c
> +++ b/odb.c
> @@ -1177,3 +1177,12 @@ void odb_reprepare(struct object_database *o)
>  {
>  	odb_prepare(o, ODB_PREPARE_FLUSH_CACHES);
>  }
> +
> +int odb_fsck(struct object_database *odb, struct odb_fsck_options *options)
> +{
> +	int ret = 0;
> +	for (struct odb_source *source = odb->sources; source; source = source->next)
> +		if ((options->flags & ODB_FSCK_FULL) || source->local)
> +			ret |= odb_source_fsck(source, options);

Shouldn't it be the responsibility of the source to determine whether it
should be included due to the `--full` flag? In the future there might
be other types of sources which have possibly a different meaning for
"local". So would it make sense to have them check for ODB_FSCK_FULL
themselves.

-- 
Laters,
Toon

Re: [PATCH v2 07/10] builtin/fsck: move reverse index verification into the packed source

[Toon Claes]

Patrick Steinhardt <ps@pks.im> writes:

> The checks for reverse indexes live in `check_pack_rev_indexes()`, which
> is hosted in "builtin/fsck.c". These checks are obviously specific to
> the "packed" backend.
>
> Move the logic into `odb_source_packed_fsck()`. As in the preceding
> commit, drop the dedicated `ERROR_PACK_REV_INDEX` bit and instead use
> the generic `ERROR_OBJECT` bit.

It wasn't immediately obvious to me, but the check is moved to
odb_source_packed_fsck() which is the callback for `.fsck` which is
called by odb_fsck() in odb/odb.c. In builtin/fsck.c a negative return
value is converted to ERROR_OBJECT.

Because it's part of the ODB, that makes sense to me.

-- 
Laters,
Toon

Re: [PATCH v2 09/10] builtin/fsck: move multi-pack index verification into the packed source

[Toon Claes]

Patrick Steinhardt <ps@pks.im> writes:

> The checks for multi-pack indexes are hosted in `cmd_fsck()` directly.
> These checks are obviously specific to the "packed" backend.
>
> Move the logic into `odb_source_packed_fsck()`. As in preceding commits,
> this means that we now properly honor both "--connectivity-only" and
> "--no-full". Furthermore, we drop the dedicated `ERROR_MULTI_PACK_INDEX`
> bit and instead use the generic `ERROR_OBJECT` bit.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/fsck.c              | 18 ------------------
>  odb/source-packed.c         | 27 +++++++++++++++++++++++++++
>  t/t5319-multi-pack-index.sh | 13 +++++++++++++
>  3 files changed, 40 insertions(+), 18 deletions(-)
>
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 2f7d29aa56..7eaea340b0 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -48,7 +48,6 @@ static timestamp_t now;
>  #define ERROR_REACHABLE 02
>  #define ERROR_REFS 010
>  #define ERROR_COMMIT_GRAPH 020
> -#define ERROR_MULTI_PACK_INDEX 040
>  
>  static const char *describe_object(const struct object_id *oid)
>  {
> @@ -1085,23 +1084,6 @@ int cmd_fsck(int argc,
>  		}
>  	}
>  
> -	if (repo->settings.core_multi_pack_index) {
> -		struct child_process midx_verify = CHILD_PROCESS_INIT;
> -
> -		for (source = repo->objects->sources; source; source = source->next) {
> -			child_process_init(&midx_verify);
> -			midx_verify.git_cmd = 1;
> -			strvec_pushl(&midx_verify.args, "multi-pack-index",
> -				     "verify", "--object-dir", source->path, NULL);
> -			if (show_progress)
> -				strvec_push(&midx_verify.args, "--progress");
> -			else
> -				strvec_push(&midx_verify.args, "--no-progress");
> -			if (run_command(&midx_verify))
> -				errors_found |= ERROR_MULTI_PACK_INDEX;
> -		}
> -	}
> -
>  	free_snapshot_refs(&snap);
>  	return errors_found;
>  }
> diff --git a/odb/source-packed.c b/odb/source-packed.c
> index 2b5dc502f5..9f42552377 100644
> --- a/odb/source-packed.c
> +++ b/odb/source-packed.c
> @@ -14,6 +14,7 @@
>  #include "packfile.h"
>  #include "pack-bitmap.h"
>  #include "progress.h"
> +#include "run-command.h"
>  
>  static int find_pack_entry(struct odb_source_packed *store,
>  			   const struct object_id *oid,
> @@ -897,6 +898,29 @@ static int verify_reverse_indices(struct odb_source_packed *source,
>  	return res;
>  }
>  
> +static int verify_midx(struct odb_source_packed *source,
> +		       struct odb_fsck_options *opts)
> +{
> +	struct child_process midx_verify = CHILD_PROCESS_INIT;
> +	int ret = 0;

I don't see much reason to use a `ret` value instead of using early
returns instead.

> +
> +	if (!source->base.odb->repo->settings.core_multi_pack_index)

Because we cannot ensure where this function was called from, shall we
BUG() if (!settings.initialized)?

> +		return 0;
> +
> +	child_process_init(&midx_verify);
> +	midx_verify.git_cmd = 1;
> +	strvec_pushl(&midx_verify.args, "multi-pack-index",
> +		     "verify", "--object-dir", source->base.path, NULL);
> +	if (opts->flags & ODB_FSCK_PROGRESS)
> +		strvec_push(&midx_verify.args, "--progress");
> +	else
> +		strvec_push(&midx_verify.args, "--no-progress");
> +	if (run_command(&midx_verify))
> +		ret = -1;
> +
> +	return ret;
> +}
> +
>  static int odb_source_packed_fsck(struct odb_source *source,
>  				  struct odb_fsck_options *opts)
>  {
> @@ -912,6 +936,9 @@ static int odb_source_packed_fsck(struct odb_source *source,
>  	if (verify_bitmap_files(packed))
>  		ret = -1;
>  
> +	if (verify_midx(packed, opts) < 0)

Any reason why you're checking negative value here and not in the if
above?

> +		ret = -1;
> +
>  	return ret;
>  }
>  
> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> index 68143cb5b7..20b010c33b 100755
> --- a/t/t5319-multi-pack-index.sh
> +++ b/t/t5319-multi-pack-index.sh
> @@ -573,6 +573,19 @@ test_expect_success 'verify incorrect checksum' '
>  		$objdir "incorrect checksum"
>  '
>  
> +test_expect_success 'git fsck --no-full checks multi-pack-index, --connectivity-only does not' '
> +	pos=$(($(wc -c <$objdir/pack/multi-pack-index) - 10)) &&
> +	corrupt_midx_and_verify $pos \
> +		"\377\377\377\377\377\377\377\377\377\377" \
> +		$objdir "incorrect checksum" &&
> +
> +	test_must_fail git fsck --no-full 2>err &&
> +	test_grep "incorrect checksum" err &&
> +
> +	git fsck --connectivity-only 2>err &&
> +	test_grep ! "incorrect checksum" err
> +'
> +
>  test_expect_success 'setup for v1-specific fsck tests' '
>  	git -c midx.version=1 multi-pack-index write
>  '
>
> -- 
> 2.55.0.979.g7e5102b832.dirty
>
>

-- 
Laters,
Toon

Re: [PATCH v2 10/10] builtin/fsck: move loose object verification into the loose source

[Toon Claes]

Patrick Steinhardt <ps@pks.im> writes:

> The consistency checks for loose objects are hosted by "builtin/fsck.c".
> These checks are obviously specific to the "loose" backend.
>
> Move the logic into `odb_source_loose_fsck()`. Introduce a new "verbose"
> flag so that we can properly retain semantics around whether or not we
> want to print some status messages.
>
> Note that this fixes a bug as a side effect: the progress meter was
> captured in the callback data before `start_progress()` was even called,
> so the per-subdirectory progress updates always operated on a NULL
> pointer and the meter jumped straight from 0 to 256 upon completion. The
> new code only sets up the callback data's progress meter after it has
> been created, so the progress display now advances incrementally again.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/fsck.c     | 91 ++----------------------------------------------------
>  odb.h              |  3 ++
>  odb/source-loose.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 93 insertions(+), 90 deletions(-)
>
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 7eaea340b0..4af1d874cc 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -12,7 +12,6 @@
>  #include "parse-options.h"
>  #include "progress.h"
>  #include "packfile.h"
> -#include "object-file.h"
>  #include "object-name.h"
>  #include "odb.h"
>  #include "odb/streaming.h"
> @@ -695,88 +694,6 @@ static void process_refs(struct repository *repo, struct snapshot *snap)
>  	}
>  }
>  
> -struct for_each_loose_cb {
> -	struct repository *repo;
> -	struct progress *progress;
> -};
> -
> -static int fsck_loose(const struct object_id *oid, const char *path,
> -		      void *cb_data)
> -{
> -	struct for_each_loose_cb *data = cb_data;
> -	enum object_type type = OBJ_NONE;
> -	size_t size;
> -	void *contents = NULL;
> -	int eaten;
> -	struct object_info oi = OBJECT_INFO_INIT;
> -	struct object_id real_oid = *null_oid(data->repo->hash_algo);
> -	int err = 0;
> -
> -	oi.sizep = &size;
> -	oi.typep = &type;
> -
> -	if (read_loose_object(data->repo, path, oid, &real_oid, &contents, &oi) < 0) {
> -		if (contents && !oideq(&real_oid, oid))
> -			err = error(_("%s: hash-path mismatch, found at: %s"),
> -				    oid_to_hex(&real_oid), path);
> -		else
> -			err = error(_("%s: object corrupt or missing: %s"),
> -				    oid_to_hex(oid), path);
> -	}
> -	if (err < 0) {
> -		errors_found |= ERROR_OBJECT;
> -		free(contents);
> -		return 0; /* keep checking other objects */
> -	}
> -
> -	if (!contents && type != OBJ_BLOB)
> -		BUG("read_loose_object streamed a non-blob");
> -
> -	if (fsck_obj_buffer(oid, type, size, contents, &eaten, data->repo))
> -		errors_found |= ERROR_OBJECT;
> -
> -	if (!eaten)
> -		free(contents);
> -	return 0; /* keep checking other objects, even if we saw an error */
> -}
> -
> -static int fsck_cruft(const char *basename, const char *path,
> -		      void *data UNUSED)
> -{
> -	if (!starts_with(basename, "tmp_obj_"))
> -		fprintf_ln(stderr, _("bad sha1 file: %s"), path);
> -	return 0;
> -}
> -
> -static int fsck_subdir(unsigned int nr, const char *path UNUSED, void *data)
> -{
> -	struct for_each_loose_cb *cb_data = data;
> -	struct progress *progress = cb_data->progress;
> -	display_progress(progress, nr + 1);
> -	return 0;
> -}
> -
> -static void fsck_source(struct repository *repo, struct odb_source *source)
> -{
> -	struct progress *progress = NULL;
> -	struct for_each_loose_cb cb_data = {
> -		.repo = source->odb->repo,
> -		.progress = progress,
> -	};
> -
> -	if (verbose)
> -		fprintf_ln(stderr, _("Checking object directory"));
> -
> -	if (show_progress)
> -		progress = start_progress(repo,
> -					  _("Checking object directories"), 256);
> -
> -	for_each_loose_file_in_source(source, fsck_loose,
> -				      fsck_cruft, fsck_subdir, &cb_data);
> -	display_progress(progress, 256);
> -	stop_progress(&progress);
> -}
> -
>  static int fsck_cache_tree(struct repository *repo, struct cache_tree *it, const char *index_path)
>  {
>  	int i;
> @@ -978,8 +895,10 @@ int cmd_fsck(int argc,
>  
>  	if (show_progress == -1)
>  		show_progress = isatty(2);
> -	if (verbose)
> +	if (verbose) {
>  		show_progress = 0;
> +		odb_fsck_opts.flags |= ODB_FSCK_VERBOSE;
> +	}
>  	if (show_progress)
>  		odb_fsck_opts.flags |= ODB_FSCK_PROGRESS;
>  
> @@ -1012,10 +931,6 @@ int cmd_fsck(int argc,
>  		odb_for_each_object(repo->objects, NULL,
>  				    mark_object_for_connectivity, repo, 0);
>  	} else {
> -		for (source = repo->objects->sources; source; source = source->next)
> -			if ((odb_fsck_opts.flags & ODB_FSCK_FULL) || source->local)
> -				fsck_source(repo, source);
> -
>  		if (odb_fsck(repo->objects, &odb_fsck_opts) < 0)
>  			errors_found |= ERROR_OBJECT;
>  
> diff --git a/odb.h b/odb.h
> index 0bf6c8d7d2..b87f281cbd 100644
> --- a/odb.h
> +++ b/odb.h
> @@ -218,6 +218,9 @@ enum odb_fsck_flags {
>  
>  	/* Display a progress meter, if sensible. */
>  	ODB_FSCK_PROGRESS = (1 << 1),
> +
> +	/* Be extra verbose when checking the database. */
> +	ODB_FSCK_VERBOSE = (1 << 2),

Shall we document this one is mutually exclusive with ODB_FSCK_PROGRESS?

>  };
>  
>  /* Options that shall be passed to `odb_fsck()`. */
> diff --git a/odb/source-loose.c b/odb/source-loose.c
> index f68d3c4d6c..efef9ca61f 100644
> --- a/odb/source-loose.c
> +++ b/odb/source-loose.c
> @@ -12,6 +12,7 @@
>  #include "odb/streaming.h"
>  #include "oidtree.h"
>  #include "path.h"
> +#include "progress.h"
>  #include "repository.h"
>  #include "strbuf.h"
>  #include "tempfile.h"
> @@ -1031,12 +1032,96 @@ static void odb_source_loose_free(struct odb_source *source)
>  	free(loose);
>  }
>  
> -static int odb_source_loose_fsck(struct odb_source *source UNUSED,
> -				 struct odb_fsck_options *opts UNUSED)
> +struct fsck_loose_data {
> +	struct odb_source_loose *source;
> +	struct odb_fsck_options *opts;
> +	struct progress *progress;
> +	bool error_found;
> +};
> +
> +static int fsck_loose(const struct object_id *oid, const char *path,
> +		      void *cb_data)
>  {
> +	struct fsck_loose_data *data = cb_data;
> +	enum object_type type = OBJ_NONE;
> +	size_t size;
> +	void *contents = NULL;
> +	int eaten = 0;
> +	struct object_info oi = OBJECT_INFO_INIT;
> +	struct object_id real_oid = *null_oid(data->source->base.odb->repo->hash_algo);
> +	int err = 0;
> +
> +	oi.sizep = &size;
> +	oi.typep = &type;
> +
> +	if (read_loose_object(data->source->base.odb->repo,
> +			      path, oid, &real_oid, &contents, &oi) < 0) {
> +		if (contents && !oideq(&real_oid, oid))
> +			err = error(_("%s: hash-path mismatch, found at: %s"),
> +				    oid_to_hex(&real_oid), path);
> +		else
> +			err = error(_("%s: object corrupt or missing: %s"),
> +				    oid_to_hex(oid), path);
> +	}
> +	if (err < 0)
> +		goto out;
> +
> +	if (!contents && type != OBJ_BLOB)
> +		BUG("read_loose_object streamed a non-blob");
> +
> +	if (data->opts->object_cb(oid, type, size, contents, &eaten,
> +				  data->opts->object_payload)) {

Should we guard data->opts->object_cb being NULL?

> +		err = -1;
> +		goto out;
> +	}
> +
> +out:
> +	if (err)
> +		data->error_found = true;
> +	if (!eaten)
> +		free(contents);
> +	return 0; /* keep checking other objects, even if we saw an error */

Okay, this function is called by for_each_loose_file_in_source() so we
need to return 0 to keep that loop going. I'm not a huge fan of the
error_found construct, but that isn't changed, so it's fine.

> +}
> +
> +static int fsck_cruft(const char *basename, const char *path,
> +		      void *data UNUSED)
> +{
> +	if (!starts_with(basename, "tmp_obj_"))
> +		fprintf_ln(stderr, _("bad sha1 file: %s"), path);
> +	return 0;
> +}
> +
> +static int fsck_subdir(unsigned int nr, const char *path UNUSED, void *cb_data)
> +{
> +	struct fsck_loose_data *data = cb_data;
> +	display_progress(data->progress, nr + 1);
>  	return 0;
>  }
>  
> +static int odb_source_loose_fsck(struct odb_source *source,
> +				 struct odb_fsck_options *opts)
> +{
> +	struct odb_source_loose *loose = odb_source_loose_downcast(source);
> +	struct fsck_loose_data data = {
> +		.source = loose,
> +		.opts = opts,
> +	};
> +
> +	if (opts->flags & ODB_FSCK_VERBOSE)
> +		fprintf_ln(stderr, _("Checking object directory"));
> +
> +	if (opts->flags & ODB_FSCK_PROGRESS)
> +		data.progress = start_progress(source->odb->repo,
> +					       _("Checking object directories"), 256);
> +
> +	for_each_loose_file_in_source(source, fsck_loose,
> +				      fsck_cruft, fsck_subdir, &data);
> +	display_progress(data.progress, 256);
> +	stop_progress(&data.progress);
> +
> +	return data.error_found ? -1 : 0;
> +}
> +
>  struct odb_source_loose *odb_source_loose_new(struct object_database *odb,
>  					      const char *path,
>  					      bool local)
>
> -- 
> 2.55.0.979.g7e5102b832.dirty
>
>

-- 
Laters,
Toon

Re: [PATCH v2 05/10] odb: provide infrastructure for pluggable fsck checks

[Patrick Steinhardt]

On Fri, Sep 11, 2026 at 01:14:03PM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/odb.c b/odb.c
> > index 1fe20808eb..766043b685 100644
> > --- a/odb.c
> > +++ b/odb.c
> > @@ -1177,3 +1177,12 @@ void odb_reprepare(struct object_database *o)
> >  {
> >  	odb_prepare(o, ODB_PREPARE_FLUSH_CACHES);
> >  }
> > +
> > +int odb_fsck(struct object_database *odb, struct odb_fsck_options *options)
> > +{
> > +	int ret = 0;
> > +	for (struct odb_source *source = odb->sources; source; source = source->next)
> > +		if ((options->flags & ODB_FSCK_FULL) || source->local)
> > +			ret |= odb_source_fsck(source, options);
> 
> Shouldn't it be the responsibility of the source to determine whether it
> should be included due to the `--full` flag? In the future there might
> be other types of sources which have possibly a different meaning for
> "local". So would it make sense to have them check for ODB_FSCK_FULL
> themselves.

That's actually a sensible idea, doubly so because we're going to move
handling of alternates into the source itself. So at that point, we
would be forced to move it into the "files" backend anyway. Will adapt.

Patrick

Re: [PATCH v2 09/10] builtin/fsck: move multi-pack index verification into the packed source

[Patrick Steinhardt]

On Fri, Sep 11, 2026 at 01:14:44PM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/odb/source-packed.c b/odb/source-packed.c
> > index 2b5dc502f5..9f42552377 100644
> > --- a/odb/source-packed.c
> > +++ b/odb/source-packed.c
> > @@ -14,6 +14,7 @@
> >  #include "packfile.h"
> >  #include "pack-bitmap.h"
> >  #include "progress.h"
> > +#include "run-command.h"
> >  
> >  static int find_pack_entry(struct odb_source_packed *store,
> >  			   const struct object_id *oid,
> > @@ -897,6 +898,29 @@ static int verify_reverse_indices(struct odb_source_packed *source,
> >  	return res;
> >  }
> >  
> > +static int verify_midx(struct odb_source_packed *source,
> > +		       struct odb_fsck_options *opts)
> > +{
> > +	struct child_process midx_verify = CHILD_PROCESS_INIT;
> > +	int ret = 0;
> 
> I don't see much reason to use a `ret` value instead of using early
> returns instead.

Fair enough.

> > +
> > +	if (!source->base.odb->repo->settings.core_multi_pack_index)
> 
> Because we cannot ensure where this function was called from, shall we
> BUG() if (!settings.initialized)?

Good point, but I think it's preferable to call
`prepare_repo_settings()` instead.

> > @@ -912,6 +936,9 @@ static int odb_source_packed_fsck(struct odb_source *source,
> >  	if (verify_bitmap_files(packed))
> >  		ret = -1;
> >  
> > +	if (verify_midx(packed, opts) < 0)
> 
> Any reason why you're checking negative value here and not in the if
> above?

Not specifically, and in theory both could check for `< 0`. But I
refrained from doing so when moving around `verify_bitmap_file()`
because in the preimage we didn't check for a negative value, either,
and it would have thus caused more questions.

So I think I'd leave this part as-is.

Patrick

Re: [PATCH v2 10/10] builtin/fsck: move loose object verification into the loose source

[Patrick Steinhardt]

On Fri, Sep 11, 2026 at 01:15:06PM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/odb.h b/odb.h
> > index 0bf6c8d7d2..b87f281cbd 100644
> > --- a/odb.h
> > +++ b/odb.h
> > @@ -218,6 +218,9 @@ enum odb_fsck_flags {
> >  
> >  	/* Display a progress meter, if sensible. */
> >  	ODB_FSCK_PROGRESS = (1 << 1),
> > +
> > +	/* Be extra verbose when checking the database. */
> > +	ODB_FSCK_VERBOSE = (1 << 2),
> 
> Shall we document this one is mutually exclusive with ODB_FSCK_PROGRESS?

But is it really? Sure, we'll potentially have interleaving output where
we print log messages followed by progress output. But as far as I can
see, we have nothing where we fully interleave so that the progress
output would be mangled.

> > diff --git a/odb/source-loose.c b/odb/source-loose.c
> > index f68d3c4d6c..efef9ca61f 100644
> > --- a/odb/source-loose.c
> > +++ b/odb/source-loose.c
> > @@ -1031,12 +1032,96 @@ static void odb_source_loose_free(struct odb_source *source)
> >  	free(loose);
> >  }
> >  
> > -static int odb_source_loose_fsck(struct odb_source *source UNUSED,
> > -				 struct odb_fsck_options *opts UNUSED)
> > +struct fsck_loose_data {
> > +	struct odb_source_loose *source;
> > +	struct odb_fsck_options *opts;
> > +	struct progress *progress;
> > +	bool error_found;
> > +};
> > +
> > +static int fsck_loose(const struct object_id *oid, const char *path,
> > +		      void *cb_data)
> >  {
> > +	struct fsck_loose_data *data = cb_data;
> > +	enum object_type type = OBJ_NONE;
> > +	size_t size;
> > +	void *contents = NULL;
> > +	int eaten = 0;
> > +	struct object_info oi = OBJECT_INFO_INIT;
> > +	struct object_id real_oid = *null_oid(data->source->base.odb->repo->hash_algo);
> > +	int err = 0;
> > +
> > +	oi.sizep = &size;
> > +	oi.typep = &type;
> > +
> > +	if (read_loose_object(data->source->base.odb->repo,
> > +			      path, oid, &real_oid, &contents, &oi) < 0) {
> > +		if (contents && !oideq(&real_oid, oid))
> > +			err = error(_("%s: hash-path mismatch, found at: %s"),
> > +				    oid_to_hex(&real_oid), path);
> > +		else
> > +			err = error(_("%s: object corrupt or missing: %s"),
> > +				    oid_to_hex(oid), path);
> > +	}
> > +	if (err < 0)
> > +		goto out;
> > +
> > +	if (!contents && type != OBJ_BLOB)
> > +		BUG("read_loose_object streamed a non-blob");
> > +
> > +	if (data->opts->object_cb(oid, type, size, contents, &eaten,
> > +				  data->opts->object_payload)) {
> 
> Should we guard data->opts->object_cb being NULL?

I don't see a reason for that -- we don't currently have any callers
that do, and we can still introduce this check if we ever grow one.

Thanks!

Patrick

[PATCH v3 00/10] odb: make consistency checks pluggable

[Patrick Steinhardt]

Hi,

this patch series makes object database consistency checks pluggable.

This series is built on top of 2c3adbb2c4 (The 18th batch, 2026-08-24)
with the following two dependencsie merged into it:

  - ps/odb-eagerly-load-alternates at 0076dc9f81 (odb: drop
    `alternates_db` field, 2026-08-17)

  - ps/odb-pluggable-pack-generation at 5176dd3d05 (bundle: generate
    packfiles via the object database, 2026-08-21)

Changes in v3:
  - Move check for `ODB_FSCK_FULL || local` into the "files" backend.
  - Ensure that repo settings are prepared.
  - Drop a mostly-useless `ret` variable.
  - Link to v2: https://patch.msgid.link/20260831-pks-odb-source-fsck-v2-0-f9b16ef4957b@pks.im

Changes in v2:
  - Some commit message improvements.
  - Link to v1: https://patch.msgid.link/20260825-pks-odb-source-fsck-v1-0-b756de0bf24f@pks.im

Thanks!

Patrick

---
Patrick Steinhardt (10):
      builtin/fsck: use `fsck_obj_buffer()` when checking loose objects
      builtin/fsck: merge `fsck_obj_buffer()` and `fsck_obj()`
      builtin/fsck: de-globalize option handling
      builtin/fsck: don't check alternates with "--no-full"
      odb: provide infrastructure for pluggable fsck checks
      builtin/fsck: move packfile verification into the packed source
      builtin/fsck: move reverse index verification into the packed source
      builtin/fsck: move bitmap verification into the packed source
      builtin/fsck: move multi-pack index verification into the packed source
      builtin/fsck: move loose object verification into the loose source

 builtin/fsck.c                | 296 ++++++++----------------------------------
 odb.c                         |   8 ++
 odb.h                         |  33 +++++
 odb/source-files.c            |  16 +++
 odb/source-inmemory.c         |   8 ++
 odb/source-loose.c            |  92 +++++++++++++
 odb/source-packed.c           | 117 +++++++++++++++++
 odb/source.h                  |  21 +++
 pack-bitmap.c                 |  26 ++--
 pack-bitmap.h                 |   2 +-
 t/t1450-fsck.sh               |   5 +
 t/t5319-multi-pack-index.sh   |  13 ++
 t/t5325-reverse-index.sh      |   8 ++
 t/t5326-multi-pack-bitmaps.sh |  10 +-
 14 files changed, 396 insertions(+), 259 deletions(-)

Range-diff versus v2:

 1:  099ad8ddcd =  1:  575a49f5a1 builtin/fsck: use `fsck_obj_buffer()` when checking loose objects
 2:  4e223cfab6 =  2:  71040c9b34 builtin/fsck: merge `fsck_obj_buffer()` and `fsck_obj()`
 3:  108b522c55 =  3:  adb2e035ee builtin/fsck: de-globalize option handling
 4:  c686809406 =  4:  68d143fc9f builtin/fsck: don't check alternates with "--no-full"
 5:  a82a1a8ed2 !  5:  bb785f4f00 odb: provide infrastructure for pluggable fsck checks
    @@ odb.c: void odb_reprepare(struct object_database *o)
     +{
     +	int ret = 0;
     +	for (struct odb_source *source = odb->sources; source; source = source->next)
    -+		if ((options->flags & ODB_FSCK_FULL) || source->local)
    -+			ret |= odb_source_fsck(source, options);
    ++		ret |= odb_source_fsck(source, options);
     +	return ret;
     +}
     
    @@ odb/source-files.c: static int odb_source_files_generate_pack(struct odb_source
     +	struct odb_source_files *files = odb_source_files_downcast(source);
     +	int ret = 0;
     +
    ++	if (!(opts->flags & ODB_FSCK_FULL) && !source->local)
    ++		return 0;
    ++
     +	ret |= odb_source_fsck(&files->loose->base, opts);
     +	ret |= odb_source_fsck(&files->packed->base, opts);
     +
 6:  166d9ad073 =  6:  9df4ebd53e builtin/fsck: move packfile verification into the packed source
 7:  83f1b18308 =  7:  b7b28d2ab4 builtin/fsck: move reverse index verification into the packed source
 8:  a760738e7a =  8:  e0efbab606 builtin/fsck: move bitmap verification into the packed source
 9:  f0acd3bdff !  9:  5bebd3fded builtin/fsck: move multi-pack index verification into the packed source
    @@ odb/source-packed.c: static int verify_reverse_indices(struct odb_source_packed
     +		       struct odb_fsck_options *opts)
     +{
     +	struct child_process midx_verify = CHILD_PROCESS_INIT;
    -+	int ret = 0;
     +
    ++	prepare_repo_settings(source->base.odb->repo);
     +	if (!source->base.odb->repo->settings.core_multi_pack_index)
     +		return 0;
     +
    @@ odb/source-packed.c: static int verify_reverse_indices(struct odb_source_packed
     +	else
     +		strvec_push(&midx_verify.args, "--no-progress");
     +	if (run_command(&midx_verify))
    -+		ret = -1;
    ++		return -1;
     +
    -+	return ret;
    ++	return 0;
     +}
     +
      static int odb_source_packed_fsck(struct odb_source *source,
10:  d140d15980 = 10:  f0f00e573a builtin/fsck: move loose object verification into the loose source

---
base-commit: 6b08999fb1b3ad0bad04d492dc206ad42839e274
change-id: 20260810-pks-odb-source-fsck-e64772c7ee5f

[PATCH v3 01/10] builtin/fsck: use `fsck_obj_buffer()` when checking loose objects

[Patrick Steinhardt]

When checking loose objects we manually parse the object buffer we have
read from the on-disk file, mark the object and then call `fsck_obj()`.
The exact same steps are also performed by `fsck_obj_buffer()`.

Stop open-coding this logic and call `fsck_obj_buffer()` instead.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 892c5661d9..3c4127f4d8 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -722,7 +722,6 @@ static int fsck_loose(const struct object_id *oid, const char *path,
 		      void *cb_data)
 {
 	struct for_each_loose_cb *data = cb_data;
-	struct object *obj;
 	enum object_type type = OBJ_NONE;
 	size_t size;
 	void *contents = NULL;
@@ -751,21 +750,7 @@ static int fsck_loose(const struct object_id *oid, const char *path,
 	if (!contents && type != OBJ_BLOB)
 		BUG("read_loose_object streamed a non-blob");
 
-	obj = parse_object_buffer(data->repo, oid, type, size,
-				  contents, &eaten);
-
-	if (!obj) {
-		errors_found |= ERROR_OBJECT;
-		error(_("%s: object could not be parsed: %s"),
-		      oid_to_hex(oid), path);
-		if (!eaten)
-			free(contents);
-		return 0; /* keep checking other objects */
-	}
-
-	obj->flags &= ~(REACHABLE | SEEN);
-	obj->flags |= HAS_OBJ;
-	if (fsck_obj(data->repo, obj, contents, size))
+	if (fsck_obj_buffer(oid, type, size, contents, &eaten, data->repo))
 		errors_found |= ERROR_OBJECT;
 
 	if (!eaten)

-- 
2.55.0.1074.ge7621b4bad.dirty

[PATCH v3 02/10] builtin/fsck: merge `fsck_obj_buffer()` and `fsck_obj()`

[Patrick Steinhardt]

The interfaces of the functions `fsck_obj()` and `fsck_obj_buffer()` are
somewhat similar to one another. The only difference between those two
is that `fsck_obj()` takes an already-parsed object as input, whereas
`fsck_obj_buffer()` parses the buffer and then calls `fsck_obj()`.

Furthermore, `fsck_obj()` has no callers other than `fsck_obj_buffer()`.

Refactor the code by merging those two functions. This makes it obvious
which function does what, and it allows us to get rid of the early
return in `fsck_obj()` in case `SEEN` is set as the only caller
unconditionally clears that bit before calling it anyway.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c | 47 ++++++++++++++++++++---------------------------
 1 file changed, 20 insertions(+), 27 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 3c4127f4d8..bed8481893 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -401,14 +401,27 @@ static void check_connectivity(struct repository *repo)
 	}
 }
 
-static int fsck_obj(struct repository *repo,
-		    struct object *obj, void *buffer, unsigned long size)
+static int fsck_obj_buffer(const struct object_id *oid, enum object_type type,
+			   unsigned long size, void *buffer, int *eaten, void *cb_data)
 {
+	struct repository *repo = cb_data;
+	struct object *obj;
 	int err;
 
-	if (obj->flags & SEEN)
-		return 0;
-	obj->flags |= SEEN;
+	/*
+	 * Note, buffer may be NULL if type is OBJ_BLOB. See
+	 * verify_packfile(), data_valid variable for details.
+	 */
+	obj = parse_object_buffer(repo, oid, type, size, buffer, eaten);
+	if (!obj) {
+		errors_found |= ERROR_OBJECT;
+		err = error(_("%s: object corrupt or missing"),
+			    oid_to_hex(oid));
+		goto out;
+	}
+
+	obj->flags &= ~REACHABLE;
+	obj->flags |= HAS_OBJ | SEEN;
 
 	if (verbose)
 		fprintf_ln(stderr, _("Checking %s %s"),
@@ -417,6 +430,7 @@ static int fsck_obj(struct repository *repo,
 
 	if (fsck_walk(obj, NULL, &fsck_obj_options))
 		objerror(repo, obj, _("broken links"));
+
 	err = fsck_object(obj, buffer, size, &fsck_obj_options);
 	if (err)
 		goto out;
@@ -442,32 +456,11 @@ static int fsck_obj(struct repository *repo,
 	}
 
 out:
-	if (obj->type == OBJ_TREE)
+	if (obj && obj->type == OBJ_TREE)
 		free_tree_buffer((struct tree *)obj);
 	return err;
 }
 
-static int fsck_obj_buffer(const struct object_id *oid, enum object_type type,
-			   unsigned long size, void *buffer, int *eaten, void *cb_data)
-{
-	struct repository *repo = cb_data;
-	struct object *obj;
-
-	/*
-	 * Note, buffer may be NULL if type is OBJ_BLOB. See
-	 * verify_packfile(), data_valid variable for details.
-	 */
-	obj = parse_object_buffer(repo, oid, type, size, buffer, eaten);
-	if (!obj) {
-		errors_found |= ERROR_OBJECT;
-		return error(_("%s: object corrupt or missing"),
-			     oid_to_hex(oid));
-	}
-	obj->flags &= ~(REACHABLE | SEEN);
-	obj->flags |= HAS_OBJ;
-	return fsck_obj(repo, obj, buffer, size);
-}
-
 static int default_refs;
 
 static void fsck_handle_reflog_oid(struct repository *repo,

-- 
2.55.0.1074.ge7621b4bad.dirty

[PATCH v3 03/10] builtin/fsck: de-globalize option handling

[Patrick Steinhardt]

In subsequent commits we're about to rework some of the option handling
in git-fsck(1) a bit. It is currently a bit of a mess though due to lots
of global state that makes it hard to see which flags are used where
exactly.

Refactor the code by moving the fsck options into `cmd_fsck()`. This
allows us to convert some of the options into function-local variables.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c | 45 ++++++++++++++++++++++-----------------------
 1 file changed, 22 insertions(+), 23 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index bed8481893..5132ff0f15 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -37,10 +37,8 @@ static int show_root;
 static int show_tags;
 static int show_unreachable;
 static int include_reflogs = 1;
-static int check_full = 1;
 static int connectivity_only;
 static int check_strict;
-static int keep_cache_objects;
 static struct fsck_options fsck_walk_options;
 static struct fsck_options fsck_obj_options;
 static int errors_found;
@@ -48,8 +46,6 @@ static int write_lost_and_found;
 static int verbose;
 static int show_progress = -1;
 static int show_dangling = 1;
-static int name_objects;
-static int check_references = 1;
 static timestamp_t now;
 #define ERROR_OBJECT 01
 #define ERROR_REACHABLE 02
@@ -964,30 +960,33 @@ static char const * const fsck_usage[] = {
 	NULL
 };
 
-static struct option fsck_opts[] = {
-	OPT__VERBOSE(&verbose, N_("be verbose")),
-	OPT_BOOL(0, "unreachable", &show_unreachable, N_("show unreachable objects")),
-	OPT_BOOL(0, "dangling", &show_dangling, N_("show dangling objects")),
-	OPT_BOOL(0, "tags", &show_tags, N_("report tags")),
-	OPT_BOOL(0, "root", &show_root, N_("report root nodes")),
-	OPT_BOOL(0, "cache", &keep_cache_objects, N_("make index objects head nodes")),
-	OPT_BOOL(0, "reflogs", &include_reflogs, N_("make reflogs head nodes (default)")),
-	OPT_BOOL(0, "full", &check_full, N_("also consider packs and alternate objects")),
-	OPT_BOOL(0, "connectivity-only", &connectivity_only, N_("check only connectivity")),
-	OPT_BOOL(0, "strict", &check_strict, N_("enable more strict checking")),
-	OPT_BOOL(0, "lost-found", &write_lost_and_found,
-				N_("write dangling objects in .git/lost-found")),
-	OPT_BOOL(0, "progress", &show_progress, N_("show progress")),
-	OPT_BOOL(0, "name-objects", &name_objects, N_("show verbose names for reachable objects")),
-	OPT_BOOL(0, "references", &check_references, N_("check reference database consistency")),
-	OPT_END(),
-};
-
 int cmd_fsck(int argc,
 	     const char **argv,
 	     const char *prefix,
 	     struct repository *repo)
 {
+	int check_full = 1;
+	int keep_cache_objects = 0;
+	int name_objects = 0;
+	int check_references = 1;
+	struct option fsck_opts[] = {
+		OPT__VERBOSE(&verbose, N_("be verbose")),
+		OPT_BOOL(0, "unreachable", &show_unreachable, N_("show unreachable objects")),
+		OPT_BOOL(0, "dangling", &show_dangling, N_("show dangling objects")),
+		OPT_BOOL(0, "tags", &show_tags, N_("report tags")),
+		OPT_BOOL(0, "root", &show_root, N_("report root nodes")),
+		OPT_BOOL(0, "cache", &keep_cache_objects, N_("make index objects head nodes")),
+		OPT_BOOL(0, "reflogs", &include_reflogs, N_("make reflogs head nodes (default)")),
+		OPT_BOOL(0, "full", &check_full, N_("also consider packs and alternate objects")),
+		OPT_BOOL(0, "connectivity-only", &connectivity_only, N_("check only connectivity")),
+		OPT_BOOL(0, "strict", &check_strict, N_("enable more strict checking")),
+		OPT_BOOL(0, "lost-found", &write_lost_and_found,
+					N_("write dangling objects in .git/lost-found")),
+		OPT_BOOL(0, "progress", &show_progress, N_("show progress")),
+		OPT_BOOL(0, "name-objects", &name_objects, N_("show verbose names for reachable objects")),
+		OPT_BOOL(0, "references", &check_references, N_("check reference database consistency")),
+		OPT_END(),
+	};
 	struct odb_source *source;
 	struct snapshot snap = {
 		.nr = 0,

-- 
2.55.0.1074.ge7621b4bad.dirty

[PATCH v3 04/10] builtin/fsck: don't check alternates with "--no-full"

[Patrick Steinhardt]

According to git-fsck(1), the "--full" option behaves in the following
way:

  Check not just objects in GIT_OBJECT_DIRECTORY ($GIT_DIR/objects), but
  also the ones found in alternate object pools listed in
  GIT_ALTERNATE_OBJECT_DIRECTORIES or $GIT_DIR/objects/info/alternates,
  and in packed Git archives found in $GIT_DIR/objects/pack and
  corresponding pack subdirectories in alternate object pools.

So ultimately, it is supposed to control two things: (1) whether we only
check the main object directory, and (2) whether we check packfiles.

In its current state though, the flag only controls whether we check
packfiles or not, and if so we verify packfiles of all attached sources.
But we also have checks for loose objects in git-fsck(1), and here we
unconditionally check them in all sources.

The flag is arguably conflating two unrelated concerns with one another,
and it really should be split up into two flags: one that controls how
thorough we want to check individual sources, and one that controls
which sources we want to check in the first place. So ideally, we would
have:

  - "--include-alternates": check all sources, not only the local one.

  - "--include-optimized-objects": check not only loose objects, but
    also those that have been packed. Note that we explicitly don't say
    "--include-packed-objects" here to be more backend-agnostic.

  - "--full": implies both of the above flags.

This feels out of scope for this series though. So for now, simply fix
the code by honoring locality of the sources for loose objects.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c  | 3 ++-
 t/t1450-fsck.sh | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 5132ff0f15..3f6056535f 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -1047,7 +1047,8 @@ int cmd_fsck(int argc,
 				    mark_object_for_connectivity, repo, 0);
 	} else {
 		for (source = repo->objects->sources; source; source = source->next)
-			fsck_source(repo, source);
+			if (check_full || source->local)
+				fsck_source(repo, source);
 
 		if (check_full) {
 			struct packed_git *p;
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 77cd96de78..1b4074304c 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -844,6 +844,11 @@ test_expect_success 'alternate objects are correctly blamed' '
 	echo "../../alt.git/objects" >.git/objects/info/alternates &&
 	mkdir alt.git/objects/$(dirname $path) &&
 	>alt.git/objects/$(dirname $path)/$(basename $path) &&
+
+	# Without "--full", only the local object source is checked.
+	git fsck --no-full >out 2>&1 &&
+	test_must_be_empty out &&
+
 	test_must_fail git fsck >out 2>&1 &&
 	test_grep alt.git out
 '

-- 
2.55.0.1074.ge7621b4bad.dirty

[PATCH v3 05/10] odb: provide infrastructure for pluggable fsck checks

[Patrick Steinhardt]

The on-disk consistency checks in git-fsck(1) are conceptually
backend-specific: while connectivity checks and object-level parsing
checks are generic, verifying the physical integrity of packfiles and
loose objects is meaningful only to backends that use these formats:
Having these checks live in "builtin/fsck.c" violates that layering,
because it forces the command to reach directly into format-specific
internals.

Provide new infrastructure to make these format-specific checks
pluggable and implement stubs for the different source types we already
have. In subsequent commits we'll move functionality over piece by
piece.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c        | 16 +++++++++++-----
 odb.c                 |  8 ++++++++
 odb.h                 | 23 +++++++++++++++++++++++
 odb/source-files.c    | 16 ++++++++++++++++
 odb/source-inmemory.c |  8 ++++++++
 odb/source-loose.c    |  7 +++++++
 odb/source-packed.c   |  8 ++++++++
 odb/source.h          | 21 +++++++++++++++++++++
 8 files changed, 102 insertions(+), 5 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 3f6056535f..adbe192e56 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -965,7 +965,9 @@ int cmd_fsck(int argc,
 	     const char *prefix,
 	     struct repository *repo)
 {
-	int check_full = 1;
+	struct odb_fsck_options odb_fsck_opts = {
+		.flags = ODB_FSCK_FULL,
+	};
 	int keep_cache_objects = 0;
 	int name_objects = 0;
 	int check_references = 1;
@@ -977,7 +979,8 @@ int cmd_fsck(int argc,
 		OPT_BOOL(0, "root", &show_root, N_("report root nodes")),
 		OPT_BOOL(0, "cache", &keep_cache_objects, N_("make index objects head nodes")),
 		OPT_BOOL(0, "reflogs", &include_reflogs, N_("make reflogs head nodes (default)")),
-		OPT_BOOL(0, "full", &check_full, N_("also consider packs and alternate objects")),
+		OPT_BIT(0, "full", &odb_fsck_opts.flags,
+			N_("also consider packs and alternate objects"), ODB_FSCK_FULL),
 		OPT_BOOL(0, "connectivity-only", &connectivity_only, N_("check only connectivity")),
 		OPT_BOOL(0, "strict", &check_strict, N_("enable more strict checking")),
 		OPT_BOOL(0, "lost-found", &write_lost_and_found,
@@ -1018,7 +1021,7 @@ int cmd_fsck(int argc,
 		show_progress = 0;
 
 	if (write_lost_and_found) {
-		check_full = 1;
+		odb_fsck_opts.flags |= ODB_FSCK_FULL;
 		include_reflogs = 0;
 	}
 
@@ -1047,10 +1050,13 @@ int cmd_fsck(int argc,
 				    mark_object_for_connectivity, repo, 0);
 	} else {
 		for (source = repo->objects->sources; source; source = source->next)
-			if (check_full || source->local)
+			if ((odb_fsck_opts.flags & ODB_FSCK_FULL) || source->local)
 				fsck_source(repo, source);
 
-		if (check_full) {
+		if (odb_fsck(repo->objects, &odb_fsck_opts) < 0)
+			errors_found |= ERROR_OBJECT;
+
+		if (odb_fsck_opts.flags & ODB_FSCK_FULL) {
 			struct packed_git *p;
 			uint32_t total = 0, count = 0;
 			struct progress *progress = NULL;
diff --git a/odb.c b/odb.c
index 1fe20808eb..1c40da4cad 100644
--- a/odb.c
+++ b/odb.c
@@ -1177,3 +1177,11 @@ void odb_reprepare(struct object_database *o)
 {
 	odb_prepare(o, ODB_PREPARE_FLUSH_CACHES);
 }
+
+int odb_fsck(struct object_database *odb, struct odb_fsck_options *options)
+{
+	int ret = 0;
+	for (struct odb_source *source = odb->sources; source; source = source->next)
+		ret |= odb_source_fsck(source, options);
+	return ret;
+}
diff --git a/odb.h b/odb.h
index e60174070f..76c15e48f5 100644
--- a/odb.h
+++ b/odb.h
@@ -206,6 +206,29 @@ void odb_prepare(struct object_database *o, enum odb_prepare_flags flags);
 /* Equivalent to `odb_prepare(o, ODB_PREPARE_FLUSH_CACHES)`. */
 void odb_reprepare(struct object_database *o);
 
+enum odb_fsck_flags {
+	/*
+	 * If set, perform a full consistency check for the full object
+	 * database, including all of its sources and the contents of their
+	 * optimized formats. Otherwise, only check the local source, and
+	 * restrict checks of its optimized formats to cheap structural
+	 * verification of their metadata.
+	 */
+	ODB_FSCK_FULL = (1 << 0),
+};
+
+/* Options that shall be passed to `odb_fsck()`. */
+struct odb_fsck_options {
+	enum odb_fsck_flags flags;
+};
+
+/*
+ * Run backend-specific integrity checks on all object sources. Each source
+ * performs the checks appropriate to its type. Returns 0 on success, a
+ * negative error code otherwise.
+ */
+int odb_fsck(struct object_database *odb, struct odb_fsck_options *opts);
+
 /*
  * Find source by its object directory path. Returns a `NULL` pointer in case
  * the source could not be found.
diff --git a/odb/source-files.c b/odb/source-files.c
index bd4fdf3a6c..66a95e2b48 100644
--- a/odb/source-files.c
+++ b/odb/source-files.c
@@ -893,6 +893,21 @@ static int odb_source_files_generate_pack(struct odb_source *source UNUSED,
 	return 0;
 }
 
+static int odb_source_files_fsck(struct odb_source *source,
+				 struct odb_fsck_options *opts)
+{
+	struct odb_source_files *files = odb_source_files_downcast(source);
+	int ret = 0;
+
+	if (!(opts->flags & ODB_FSCK_FULL) && !source->local)
+		return 0;
+
+	ret |= odb_source_fsck(&files->loose->base, opts);
+	ret |= odb_source_fsck(&files->packed->base, opts);
+
+	return ret;
+}
+
 struct odb_source_files *odb_source_files_new(struct object_database *odb,
 					      const char *path,
 					      bool local)
@@ -908,6 +923,7 @@ struct odb_source_files *odb_source_files_new(struct object_database *odb,
 	files->base.close = odb_source_files_close;
 	files->base.create_on_disk = odb_source_files_create_on_disk;
 	files->base.prepare = odb_source_files_prepare;
+	files->base.fsck = odb_source_files_fsck;
 	files->base.read_object_info = odb_source_files_read_object_info;
 	files->base.read_object_stream = odb_source_files_read_object_stream;
 	files->base.for_each_object = odb_source_files_for_each_object;
diff --git a/odb/source-inmemory.c b/odb/source-inmemory.c
index 795672adf2..ba0f86da26 100644
--- a/odb/source-inmemory.c
+++ b/odb/source-inmemory.c
@@ -1,6 +1,7 @@
 #include "git-compat-util.h"
 #include "object-file.h"
 #include "odb.h"
+#include "fsck.h"
 #include "odb/source-inmemory.h"
 #include "odb/streaming.h"
 #include "oidtree.h"
@@ -368,6 +369,12 @@ static void odb_source_inmemory_free(struct odb_source *source)
 	free(inmemory);
 }
 
+static int odb_source_inmemory_fsck(struct odb_source *source UNUSED,
+				    struct odb_fsck_options *opts UNUSED)
+{
+	return 0;
+}
+
 struct odb_source_inmemory *odb_source_inmemory_new(struct object_database *odb)
 {
 	struct odb_source_inmemory *source;
@@ -378,6 +385,7 @@ struct odb_source_inmemory *odb_source_inmemory_new(struct object_database *odb)
 	source->base.free = odb_source_inmemory_free;
 	source->base.close = odb_source_inmemory_close;
 	source->base.prepare = odb_source_inmemory_prepare;
+	source->base.fsck = odb_source_inmemory_fsck;
 	source->base.read_object_info = odb_source_inmemory_read_object_info;
 	source->base.read_object_stream = odb_source_inmemory_read_object_stream;
 	source->base.for_each_object = odb_source_inmemory_for_each_object;
diff --git a/odb/source-loose.c b/odb/source-loose.c
index bb3455dfbd..f68d3c4d6c 100644
--- a/odb/source-loose.c
+++ b/odb/source-loose.c
@@ -1031,6 +1031,12 @@ static void odb_source_loose_free(struct odb_source *source)
 	free(loose);
 }
 
+static int odb_source_loose_fsck(struct odb_source *source UNUSED,
+				 struct odb_fsck_options *opts UNUSED)
+{
+	return 0;
+}
+
 struct odb_source_loose *odb_source_loose_new(struct object_database *odb,
 					      const char *path,
 					      bool local)
@@ -1043,6 +1049,7 @@ struct odb_source_loose *odb_source_loose_new(struct object_database *odb,
 	loose->base.free = odb_source_loose_free;
 	loose->base.close = odb_source_loose_close;
 	loose->base.prepare = odb_source_loose_prepare;
+	loose->base.fsck = odb_source_loose_fsck;
 	loose->base.read_object_info = odb_source_loose_read_object_info;
 	loose->base.read_object_stream = odb_source_loose_read_object_stream;
 	loose->base.for_each_object = odb_source_loose_for_each_object;
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 630d955585..7aacf4bc45 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -2,6 +2,7 @@
 #include "abspath.h"
 #include "chdir-notify.h"
 #include "dir.h"
+#include "fsck.h"
 #include "git-zlib.h"
 #include "list-objects-filter-options.h"
 #include "mergesort.h"
@@ -826,6 +827,12 @@ static void odb_source_packed_free(struct odb_source *source)
 	free(packed);
 }
 
+static int odb_source_packed_fsck(struct odb_source *source UNUSED,
+				  struct odb_fsck_options *opts UNUSED)
+{
+	return 0;
+}
+
 struct odb_source_packed *odb_source_packed_new(struct object_database *odb,
 						const char *path,
 						bool local)
@@ -839,6 +846,7 @@ struct odb_source_packed *odb_source_packed_new(struct object_database *odb,
 	packed->base.free = odb_source_packed_free;
 	packed->base.close = odb_source_packed_close;
 	packed->base.prepare = odb_source_packed_prepare;
+	packed->base.fsck = odb_source_packed_fsck;
 	packed->base.read_object_info = odb_source_packed_read_object_info;
 	packed->base.read_object_stream = odb_source_packed_read_object_stream;
 	packed->base.for_each_object = odb_source_packed_for_each_object;
diff --git a/odb/source.h b/odb/source.h
index 559e2ea2e9..10a5dd5194 100644
--- a/odb/source.h
+++ b/odb/source.h
@@ -320,6 +320,17 @@ struct odb_source {
 	int (*generate_pack)(struct odb_source *source,
 			     struct odb_pack_generator **out,
 			     const struct odb_generate_pack_options *opts);
+
+	/*
+	 * This callback is expected to check the integrity of the object source
+	 * and report any errors found via the fsck options. The checks performed
+	 * are backend-specific.
+	 *
+	 * The callback is expected to return 0 on success, a negative error
+	 * code otherwise.
+	 */
+	int (*fsck)(struct odb_source *source,
+		    struct odb_fsck_options *options);
 };
 
 /*
@@ -588,4 +599,14 @@ static inline int odb_source_generate_pack(struct odb_source *source,
 	return source->generate_pack(source, out, opts);
 }
 
+/*
+ * Check the integrity of the object database source. The checks performed
+ * are backend-specific. Returns 0 on success, a negative error code otherwise.
+ */
+static inline int odb_source_fsck(struct odb_source *source,
+				  struct odb_fsck_options *opts)
+{
+	return source->fsck(source, opts);
+}
+
 #endif

-- 
2.55.0.1074.ge7621b4bad.dirty

[PATCH v3 06/10] builtin/fsck: move packfile verification into the packed source

[Patrick Steinhardt]

Move the packfile verification out of `cmd_fsck()` and into the "packed"
source. While doing so, thread the progress meter and object callback
through the newly introduced `struct odb_fsck_options` so that the
caller's preferences are honoured without exposing those details at the
"builtin/fsck.c" level.

Note that the old code reported failures when verifying packfiles with
the `ERROR_PACK` bit, which gets returned to the caller via the exit
code. This bit is neither exercised in our test suite nor is it
documented anywhere in our codebase. Furthermore, this bit is highly
specific to the object storage backend, which makes it a bad fit for the
new pluggable infrastructure. So instead of retaining these semantics,
we drop them and return the generic `ERROR_OBJECT` bit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c      | 33 ++++-----------------------------
 odb.h               |  7 +++++++
 odb/source-packed.c | 46 +++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 54 insertions(+), 32 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index adbe192e56..e504dae904 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -7,7 +7,6 @@
 #include "blob.h"
 #include "tag.h"
 #include "refs.h"
-#include "pack.h"
 #include "cache-tree.h"
 #include "fsck.h"
 #include "parse-options.h"
@@ -49,7 +48,6 @@ static int show_dangling = 1;
 static timestamp_t now;
 #define ERROR_OBJECT 01
 #define ERROR_REACHABLE 02
-#define ERROR_PACK 04
 #define ERROR_REFS 010
 #define ERROR_COMMIT_GRAPH 020
 #define ERROR_MULTI_PACK_INDEX 040
@@ -967,6 +965,8 @@ int cmd_fsck(int argc,
 {
 	struct odb_fsck_options odb_fsck_opts = {
 		.flags = ODB_FSCK_FULL,
+		.object_cb = fsck_obj_buffer,
+		.object_payload = repo,
 	};
 	int keep_cache_objects = 0;
 	int name_objects = 0;
@@ -1019,6 +1019,8 @@ int cmd_fsck(int argc,
 		show_progress = isatty(2);
 	if (verbose)
 		show_progress = 0;
+	if (show_progress)
+		odb_fsck_opts.flags |= ODB_FSCK_PROGRESS;
 
 	if (write_lost_and_found) {
 		odb_fsck_opts.flags |= ODB_FSCK_FULL;
@@ -1056,33 +1058,6 @@ int cmd_fsck(int argc,
 		if (odb_fsck(repo->objects, &odb_fsck_opts) < 0)
 			errors_found |= ERROR_OBJECT;
 
-		if (odb_fsck_opts.flags & ODB_FSCK_FULL) {
-			struct packed_git *p;
-			uint32_t total = 0, count = 0;
-			struct progress *progress = NULL;
-
-			if (show_progress) {
-				repo_for_each_pack(repo, p) {
-					if (open_pack_index(p))
-						continue;
-					total += p->num_objects;
-				}
-
-				progress = start_progress(repo,
-							  _("Checking objects"), total);
-			}
-
-			repo_for_each_pack(repo, p) {
-				/* verify gives error messages itself */
-				if (verify_pack(repo,
-						p, fsck_obj_buffer, repo,
-						progress, count))
-					errors_found |= ERROR_PACK;
-				count += p->num_objects;
-			}
-			stop_progress(&progress);
-		}
-
 		if (fsck_finish(&fsck_obj_options))
 			errors_found |= ERROR_OBJECT;
 	}
diff --git a/odb.h b/odb.h
index 76c15e48f5..0bf6c8d7d2 100644
--- a/odb.h
+++ b/odb.h
@@ -215,11 +215,18 @@ enum odb_fsck_flags {
 	 * verification of their metadata.
 	 */
 	ODB_FSCK_FULL = (1 << 0),
+
+	/* Display a progress meter, if sensible. */
+	ODB_FSCK_PROGRESS = (1 << 1),
 };
 
 /* Options that shall be passed to `odb_fsck()`. */
 struct odb_fsck_options {
 	enum odb_fsck_flags flags;
+
+	int (*object_cb)(const struct object_id *oid, enum object_type type,
+			 unsigned long size, void *buffer, int *eaten, void *cb_data);
+	void *object_payload;
 };
 
 /*
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 7aacf4bc45..0d3599f8fe 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -9,8 +9,10 @@
 #include "midx.h"
 #include "odb/source-packed.h"
 #include "odb/streaming.h"
+#include "pack.h"
 #include "packfile.h"
 #include "pack-bitmap.h"
+#include "progress.h"
 
 static int find_pack_entry(struct odb_source_packed *store,
 			   const struct object_id *oid,
@@ -827,10 +829,48 @@ static void odb_source_packed_free(struct odb_source *source)
 	free(packed);
 }
 
-static int odb_source_packed_fsck(struct odb_source *source UNUSED,
-				  struct odb_fsck_options *opts UNUSED)
+static int verify_packs(struct odb_source_packed *source,
+			struct odb_fsck_options *opts)
 {
-	return 0;
+	struct progress *progress = NULL;
+	struct packfile_list_entry *e;
+	uint32_t total = 0, count = 0;
+	int ret = 0;
+
+	if (opts->flags & ODB_FSCK_PROGRESS) {
+		for (e = packfile_store_get_packs(source); e; e = e->next) {
+			if (open_pack_index(e->pack))
+				continue;
+			total += e->pack->num_objects;
+		}
+
+		progress = start_progress(source->base.odb->repo,
+					  _("Checking objects"), total);
+	}
+
+	for (e = packfile_store_get_packs(source); e; e = e->next) {
+		/* verify gives error messages itself */
+		if (verify_pack(source->base.odb->repo, e->pack,
+				opts->object_cb, opts->object_payload,
+				progress, count))
+			ret = -1;
+		count += e->pack->num_objects;
+	}
+	stop_progress(&progress);
+
+	return ret;
+}
+
+static int odb_source_packed_fsck(struct odb_source *source,
+				  struct odb_fsck_options *opts)
+{
+	struct odb_source_packed *packed = odb_source_packed_downcast(source);
+	int ret = 0;
+
+	if ((opts->flags & ODB_FSCK_FULL) && verify_packs(packed, opts) < 0)
+		ret = -1;
+
+	return ret;
 }
 
 struct odb_source_packed *odb_source_packed_new(struct object_database *odb,

-- 
2.55.0.1074.ge7621b4bad.dirty

[PATCH v3 07/10] builtin/fsck: move reverse index verification into the packed source

[Patrick Steinhardt]

The checks for reverse indexes live in `check_pack_rev_indexes()`, which
is hosted in "builtin/fsck.c". These checks are obviously specific to
the "packed" backend.

Move the logic into `odb_source_packed_fsck()`. As in the preceding
commit, drop the dedicated `ERROR_PACK_REV_INDEX` bit and instead use
the generic `ERROR_OBJECT` bit.

Note that this changes behaviour in two ways:

  - The checks are now skipped when "--connectivity-only" was passed.
    This is because we don't even run `odb_fsck()` at all when that
    flag has been passed by the user, and not verifying data structures
    of the object database matches the documented intent of that flag,
    which is to only check the connectivity of reachable objects.

  - The checks are now skipped for non-local sources when "--no-full"
    was passed. This is, again, in line with the documented intent of
    that flag.

Add a test to cast these semantics into stone.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c           | 37 -------------------------------------
 odb/source-packed.c      | 39 +++++++++++++++++++++++++++++++++++++++
 t/t5325-reverse-index.sh |  8 ++++++++
 3 files changed, 47 insertions(+), 37 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index e504dae904..06e72877f3 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -23,7 +23,6 @@
 #include "run-command.h"
 #include "sparse-index.h"
 #include "worktree.h"
-#include "pack-revindex.h"
 #include "pack-bitmap.h"
 
 #define REACHABLE 0x0001
@@ -51,7 +50,6 @@ static timestamp_t now;
 #define ERROR_REFS 010
 #define ERROR_COMMIT_GRAPH 020
 #define ERROR_MULTI_PACK_INDEX 040
-#define ERROR_PACK_REV_INDEX 0100
 #define ERROR_BITMAP 0200
 
 static const char *describe_object(const struct object_id *oid)
@@ -890,40 +888,6 @@ static int mark_object_for_connectivity(const struct object_id *oid,
 	return 0;
 }
 
-static int check_pack_rev_indexes(struct repository *r, int show_progress)
-{
-	struct progress *progress = NULL;
-	struct packed_git *p;
-	uint32_t pack_count = 0;
-	int res = 0;
-
-	if (show_progress) {
-		repo_for_each_pack(r, p)
-			pack_count++;
-		progress = start_delayed_progress(r,
-						  "Verifying reverse pack-indexes", pack_count);
-		pack_count = 0;
-	}
-
-	repo_for_each_pack(r, p) {
-		int load_error = load_pack_revindex_from_disk(p);
-
-		if (load_error < 0) {
-			error(_("unable to load rev-index for pack '%s'"), p->pack_name);
-			res = ERROR_PACK_REV_INDEX;
-		} else if (!load_error &&
-			   !load_pack_revindex(r, p) &&
-			   verify_pack_revindex(p)) {
-			error(_("invalid rev-index for pack '%s'"), p->pack_name);
-			res = ERROR_PACK_REV_INDEX;
-		}
-		display_progress(progress, ++pack_count);
-	}
-	stop_progress(&progress);
-
-	return res;
-}
-
 static void fsck_refs(struct repository *r)
 {
 	struct child_process refs_verify = CHILD_PROCESS_INIT;
@@ -1104,7 +1068,6 @@ int cmd_fsck(int argc,
 		free_worktrees(worktrees);
 	}
 
-	errors_found |= check_pack_rev_indexes(repo, show_progress);
 	if (verify_bitmap_files(repo))
 		errors_found |= ERROR_BITMAP;
 
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 0d3599f8fe..e5e69636dd 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -10,6 +10,7 @@
 #include "odb/source-packed.h"
 #include "odb/streaming.h"
 #include "pack.h"
+#include "pack-revindex.h"
 #include "packfile.h"
 #include "pack-bitmap.h"
 #include "progress.h"
@@ -861,6 +862,41 @@ static int verify_packs(struct odb_source_packed *source,
 	return ret;
 }
 
+static int verify_reverse_indices(struct odb_source_packed *source,
+				  struct odb_fsck_options *opts)
+{
+	struct progress *progress = NULL;
+	struct packfile_list_entry *e;
+	uint32_t pack_count = 0;
+	int res = 0;
+
+	if (opts->flags & ODB_FSCK_PROGRESS) {
+		for (e = packfile_store_get_packs(source); e; e = e->next)
+			pack_count++;
+		progress = start_delayed_progress(source->base.odb->repo,
+						  "Verifying reverse pack-indexes", pack_count);
+		pack_count = 0;
+	}
+
+	for (e = packfile_store_get_packs(source); e; e = e->next) {
+		int load_error = load_pack_revindex_from_disk(e->pack);
+
+		if (load_error < 0) {
+			error(_("unable to load rev-index for pack '%s'"), e->pack->pack_name);
+			res = -1;
+		} else if (!load_error &&
+			   !load_pack_revindex(source->base.odb->repo, e->pack) &&
+			   verify_pack_revindex(e->pack)) {
+			error(_("invalid rev-index for pack '%s'"), e->pack->pack_name);
+			res = -1;
+		}
+		display_progress(progress, ++pack_count);
+	}
+	stop_progress(&progress);
+
+	return res;
+}
+
 static int odb_source_packed_fsck(struct odb_source *source,
 				  struct odb_fsck_options *opts)
 {
@@ -870,6 +906,9 @@ static int odb_source_packed_fsck(struct odb_source *source,
 	if ((opts->flags & ODB_FSCK_FULL) && verify_packs(packed, opts) < 0)
 		ret = -1;
 
+	if (verify_reverse_indices(packed, opts) < 0)
+		ret = -1;
+
 	return ret;
 }
 
diff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh
index 5493791938..6b81abf663 100755
--- a/t/t5325-reverse-index.sh
+++ b/t/t5325-reverse-index.sh
@@ -204,4 +204,12 @@ test_expect_success 'fsck catches invalid header: hash function' '
 		"reverse-index file .* has unsupported hash id"
 '
 
+test_expect_success 'fsck --no-full checks rev-index, --connectivity-only does not' '
+	test_must_fail git -C corrupt fsck --no-full 2>err &&
+	test_grep "has unsupported hash id" err &&
+
+	git -C corrupt fsck --connectivity-only 2>err &&
+	test_grep ! "has unsupported hash id" err
+'
+
 test_done

-- 
2.55.0.1074.ge7621b4bad.dirty

[PATCH v3 08/10] builtin/fsck: move bitmap verification into the packed source

[Patrick Steinhardt]

The checks for bitmaps live in `verify_bitmap_files()`, which is called
by "builtin/fsck.c". These checks are obviously specific to the "packed"
backend.

Move the logic into `odb_source_packed_fsck()`. As in preceding commits,
this means that we now properly honor both "--connectivity-only" and
"--no-full". Furthermore, we drop the dedicated `ERROR_BITMAP` bit and
instead use the generic `ERROR_OBJECT` bit.

Note that this change also adapts `verify_bitmap_files()` to be
focused on a single "packed" source instead of verifying bitmaps from
all sources. This change is required as we already know to loop around
the sources in `odb_fsck()` itself.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c                |  5 -----
 odb/source-packed.c           |  3 +++
 pack-bitmap.c                 | 26 ++++++++++----------------
 pack-bitmap.h                 |  2 +-
 t/t5326-multi-pack-bitmaps.sh | 10 +++++++++-
 5 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 06e72877f3..2f7d29aa56 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -23,7 +23,6 @@
 #include "run-command.h"
 #include "sparse-index.h"
 #include "worktree.h"
-#include "pack-bitmap.h"
 
 #define REACHABLE 0x0001
 #define SEEN      0x0002
@@ -50,7 +49,6 @@ static timestamp_t now;
 #define ERROR_REFS 010
 #define ERROR_COMMIT_GRAPH 020
 #define ERROR_MULTI_PACK_INDEX 040
-#define ERROR_BITMAP 0200
 
 static const char *describe_object(const struct object_id *oid)
 {
@@ -1068,9 +1066,6 @@ int cmd_fsck(int argc,
 		free_worktrees(worktrees);
 	}
 
-	if (verify_bitmap_files(repo))
-		errors_found |= ERROR_BITMAP;
-
 	check_connectivity(repo);
 
 	if (repo->settings.core_commit_graph) {
diff --git a/odb/source-packed.c b/odb/source-packed.c
index e5e69636dd..2b5dc502f5 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -909,6 +909,9 @@ static int odb_source_packed_fsck(struct odb_source *source,
 	if (verify_reverse_indices(packed, opts) < 0)
 		ret = -1;
 
+	if (verify_bitmap_files(packed))
+		ret = -1;
+
 	return ret;
 }
 
diff --git a/pack-bitmap.c b/pack-bitmap.c
index e0fb57d332..3de8e9590c 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -3410,28 +3410,22 @@ static int verify_bitmap_file(const struct git_hash_algo *algop,
 	return res;
 }
 
-int verify_bitmap_files(struct repository *r)
+int verify_bitmap_files(struct odb_source_packed *source)
 {
-	struct odb_source *source;
-	struct packed_git *p;
+	struct packfile_list_entry *e;
+	struct multi_pack_index *m;
 	int res = 0;
 
-	for (source = r->objects->sources; source; source = source->next) {
-		struct odb_source_files *files = odb_source_files_downcast(source);
-		struct multi_pack_index *m = get_multi_pack_index(files->packed);
-		char *midx_bitmap_name;
-
-		if (!m)
-			continue;
-
-		midx_bitmap_name = midx_bitmap_filename(m);
-		res |= verify_bitmap_file(r->hash_algo, midx_bitmap_name);
+	m = get_multi_pack_index(source);
+	if (m) {
+		char *midx_bitmap_name = midx_bitmap_filename(m);
+		res |= verify_bitmap_file(source->base.odb->repo->hash_algo, midx_bitmap_name);
 		free(midx_bitmap_name);
 	}
 
-	repo_for_each_pack(r, p) {
-		char *pack_bitmap_name = pack_bitmap_filename(p);
-		res |= verify_bitmap_file(r->hash_algo, pack_bitmap_name);
+	for (e = packfile_store_get_packs(source); e; e = e->next) {
+		char *pack_bitmap_name = pack_bitmap_filename(e->pack);
+		res |= verify_bitmap_file(source->base.odb->repo->hash_algo, pack_bitmap_name);
 		free(pack_bitmap_name);
 	}
 
diff --git a/pack-bitmap.h b/pack-bitmap.h
index 1385027c1f..847ad4762d 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -205,7 +205,7 @@ int bitmap_is_midx(struct bitmap_index *bitmap_git);
 
 int bitmap_is_preferred_refname(struct repository *r, const char *refname);
 
-int verify_bitmap_files(struct repository *r);
+int verify_bitmap_files(struct odb_source_packed *source);
 
 struct ewah_bitmap *read_bitmap(const unsigned char *map,
 				size_t map_size, size_t *map_pos);
diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh
index 86beab1dae..8047459b00 100755
--- a/t/t5326-multi-pack-bitmaps.sh
+++ b/t/t5326-multi-pack-bitmaps.sh
@@ -498,7 +498,15 @@ test_expect_success 'git fsck correctly identifies good and bad bitmaps' '
 	corrupt_file "$packbitmap" &&
 	test_must_fail git fsck 2>err &&
 	test_grep "bitmap file '\''$midxbitmap'\'' has invalid checksum" err &&
-	test_grep "bitmap file '\''$packbitmap'\'' has invalid checksum" err
+	test_grep "bitmap file '\''$packbitmap'\'' has invalid checksum" err &&
+
+	# The bitmap checks are performed with "--no-full", but not with
+	# "--connectivity-only".
+	test_must_fail git fsck --no-full 2>err &&
+	test_grep "bitmap file '\''$midxbitmap'\'' has invalid checksum" err &&
+	test_grep "bitmap file '\''$packbitmap'\'' has invalid checksum" err &&
+	git fsck --connectivity-only 2>err &&
+	test_grep ! "invalid checksum" err
 '
 
 test_expect_success 'corrupt MIDX with bitmap causes fallback' '

-- 
2.55.0.1074.ge7621b4bad.dirty

[PATCH v3 09/10] builtin/fsck: move multi-pack index verification into the packed source

[Patrick Steinhardt]

The checks for multi-pack indexes are hosted in `cmd_fsck()` directly.
These checks are obviously specific to the "packed" backend.

Move the logic into `odb_source_packed_fsck()`. As in preceding commits,
this means that we now properly honor both "--connectivity-only" and
"--no-full". Furthermore, we drop the dedicated `ERROR_MULTI_PACK_INDEX`
bit and instead use the generic `ERROR_OBJECT` bit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c              | 18 ------------------
 odb/source-packed.c         | 27 +++++++++++++++++++++++++++
 t/t5319-multi-pack-index.sh | 13 +++++++++++++
 3 files changed, 40 insertions(+), 18 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 2f7d29aa56..7eaea340b0 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -48,7 +48,6 @@ static timestamp_t now;
 #define ERROR_REACHABLE 02
 #define ERROR_REFS 010
 #define ERROR_COMMIT_GRAPH 020
-#define ERROR_MULTI_PACK_INDEX 040
 
 static const char *describe_object(const struct object_id *oid)
 {
@@ -1085,23 +1084,6 @@ int cmd_fsck(int argc,
 		}
 	}
 
-	if (repo->settings.core_multi_pack_index) {
-		struct child_process midx_verify = CHILD_PROCESS_INIT;
-
-		for (source = repo->objects->sources; source; source = source->next) {
-			child_process_init(&midx_verify);
-			midx_verify.git_cmd = 1;
-			strvec_pushl(&midx_verify.args, "multi-pack-index",
-				     "verify", "--object-dir", source->path, NULL);
-			if (show_progress)
-				strvec_push(&midx_verify.args, "--progress");
-			else
-				strvec_push(&midx_verify.args, "--no-progress");
-			if (run_command(&midx_verify))
-				errors_found |= ERROR_MULTI_PACK_INDEX;
-		}
-	}
-
 	free_snapshot_refs(&snap);
 	return errors_found;
 }
diff --git a/odb/source-packed.c b/odb/source-packed.c
index 2b5dc502f5..9f54a5e83a 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -14,6 +14,7 @@
 #include "packfile.h"
 #include "pack-bitmap.h"
 #include "progress.h"
+#include "run-command.h"
 
 static int find_pack_entry(struct odb_source_packed *store,
 			   const struct object_id *oid,
@@ -897,6 +898,29 @@ static int verify_reverse_indices(struct odb_source_packed *source,
 	return res;
 }
 
+static int verify_midx(struct odb_source_packed *source,
+		       struct odb_fsck_options *opts)
+{
+	struct child_process midx_verify = CHILD_PROCESS_INIT;
+
+	prepare_repo_settings(source->base.odb->repo);
+	if (!source->base.odb->repo->settings.core_multi_pack_index)
+		return 0;
+
+	child_process_init(&midx_verify);
+	midx_verify.git_cmd = 1;
+	strvec_pushl(&midx_verify.args, "multi-pack-index",
+		     "verify", "--object-dir", source->base.path, NULL);
+	if (opts->flags & ODB_FSCK_PROGRESS)
+		strvec_push(&midx_verify.args, "--progress");
+	else
+		strvec_push(&midx_verify.args, "--no-progress");
+	if (run_command(&midx_verify))
+		return -1;
+
+	return 0;
+}
+
 static int odb_source_packed_fsck(struct odb_source *source,
 				  struct odb_fsck_options *opts)
 {
@@ -912,6 +936,9 @@ static int odb_source_packed_fsck(struct odb_source *source,
 	if (verify_bitmap_files(packed))
 		ret = -1;
 
+	if (verify_midx(packed, opts) < 0)
+		ret = -1;
+
 	return ret;
 }
 
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 68143cb5b7..20b010c33b 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -573,6 +573,19 @@ test_expect_success 'verify incorrect checksum' '
 		$objdir "incorrect checksum"
 '
 
+test_expect_success 'git fsck --no-full checks multi-pack-index, --connectivity-only does not' '
+	pos=$(($(wc -c <$objdir/pack/multi-pack-index) - 10)) &&
+	corrupt_midx_and_verify $pos \
+		"\377\377\377\377\377\377\377\377\377\377" \
+		$objdir "incorrect checksum" &&
+
+	test_must_fail git fsck --no-full 2>err &&
+	test_grep "incorrect checksum" err &&
+
+	git fsck --connectivity-only 2>err &&
+	test_grep ! "incorrect checksum" err
+'
+
 test_expect_success 'setup for v1-specific fsck tests' '
 	git -c midx.version=1 multi-pack-index write
 '

-- 
2.55.0.1074.ge7621b4bad.dirty

[PATCH v3 10/10] builtin/fsck: move loose object verification into the loose source

[Patrick Steinhardt]

The consistency checks for loose objects are hosted by "builtin/fsck.c".
These checks are obviously specific to the "loose" backend.

Move the logic into `odb_source_loose_fsck()`. Introduce a new "verbose"
flag so that we can properly retain semantics around whether or not we
want to print some status messages.

Note that this fixes a bug as a side effect: the progress meter was
captured in the callback data before `start_progress()` was even called,
so the per-subdirectory progress updates always operated on a NULL
pointer and the meter jumped straight from 0 to 256 upon completion. The
new code only sets up the callback data's progress meter after it has
been created, so the progress display now advances incrementally again.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c     | 91 ++----------------------------------------------------
 odb.h              |  3 ++
 odb/source-loose.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 93 insertions(+), 90 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 7eaea340b0..4af1d874cc 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -12,7 +12,6 @@
 #include "parse-options.h"
 #include "progress.h"
 #include "packfile.h"
-#include "object-file.h"
 #include "object-name.h"
 #include "odb.h"
 #include "odb/streaming.h"
@@ -695,88 +694,6 @@ static void process_refs(struct repository *repo, struct snapshot *snap)
 	}
 }
 
-struct for_each_loose_cb {
-	struct repository *repo;
-	struct progress *progress;
-};
-
-static int fsck_loose(const struct object_id *oid, const char *path,
-		      void *cb_data)
-{
-	struct for_each_loose_cb *data = cb_data;
-	enum object_type type = OBJ_NONE;
-	size_t size;
-	void *contents = NULL;
-	int eaten;
-	struct object_info oi = OBJECT_INFO_INIT;
-	struct object_id real_oid = *null_oid(data->repo->hash_algo);
-	int err = 0;
-
-	oi.sizep = &size;
-	oi.typep = &type;
-
-	if (read_loose_object(data->repo, path, oid, &real_oid, &contents, &oi) < 0) {
-		if (contents && !oideq(&real_oid, oid))
-			err = error(_("%s: hash-path mismatch, found at: %s"),
-				    oid_to_hex(&real_oid), path);
-		else
-			err = error(_("%s: object corrupt or missing: %s"),
-				    oid_to_hex(oid), path);
-	}
-	if (err < 0) {
-		errors_found |= ERROR_OBJECT;
-		free(contents);
-		return 0; /* keep checking other objects */
-	}
-
-	if (!contents && type != OBJ_BLOB)
-		BUG("read_loose_object streamed a non-blob");
-
-	if (fsck_obj_buffer(oid, type, size, contents, &eaten, data->repo))
-		errors_found |= ERROR_OBJECT;
-
-	if (!eaten)
-		free(contents);
-	return 0; /* keep checking other objects, even if we saw an error */
-}
-
-static int fsck_cruft(const char *basename, const char *path,
-		      void *data UNUSED)
-{
-	if (!starts_with(basename, "tmp_obj_"))
-		fprintf_ln(stderr, _("bad sha1 file: %s"), path);
-	return 0;
-}
-
-static int fsck_subdir(unsigned int nr, const char *path UNUSED, void *data)
-{
-	struct for_each_loose_cb *cb_data = data;
-	struct progress *progress = cb_data->progress;
-	display_progress(progress, nr + 1);
-	return 0;
-}
-
-static void fsck_source(struct repository *repo, struct odb_source *source)
-{
-	struct progress *progress = NULL;
-	struct for_each_loose_cb cb_data = {
-		.repo = source->odb->repo,
-		.progress = progress,
-	};
-
-	if (verbose)
-		fprintf_ln(stderr, _("Checking object directory"));
-
-	if (show_progress)
-		progress = start_progress(repo,
-					  _("Checking object directories"), 256);
-
-	for_each_loose_file_in_source(source, fsck_loose,
-				      fsck_cruft, fsck_subdir, &cb_data);
-	display_progress(progress, 256);
-	stop_progress(&progress);
-}
-
 static int fsck_cache_tree(struct repository *repo, struct cache_tree *it, const char *index_path)
 {
 	int i;
@@ -978,8 +895,10 @@ int cmd_fsck(int argc,
 
 	if (show_progress == -1)
 		show_progress = isatty(2);
-	if (verbose)
+	if (verbose) {
 		show_progress = 0;
+		odb_fsck_opts.flags |= ODB_FSCK_VERBOSE;
+	}
 	if (show_progress)
 		odb_fsck_opts.flags |= ODB_FSCK_PROGRESS;
 
@@ -1012,10 +931,6 @@ int cmd_fsck(int argc,
 		odb_for_each_object(repo->objects, NULL,
 				    mark_object_for_connectivity, repo, 0);
 	} else {
-		for (source = repo->objects->sources; source; source = source->next)
-			if ((odb_fsck_opts.flags & ODB_FSCK_FULL) || source->local)
-				fsck_source(repo, source);
-
 		if (odb_fsck(repo->objects, &odb_fsck_opts) < 0)
 			errors_found |= ERROR_OBJECT;
 
diff --git a/odb.h b/odb.h
index 0bf6c8d7d2..b87f281cbd 100644
--- a/odb.h
+++ b/odb.h
@@ -218,6 +218,9 @@ enum odb_fsck_flags {
 
 	/* Display a progress meter, if sensible. */
 	ODB_FSCK_PROGRESS = (1 << 1),
+
+	/* Be extra verbose when checking the database. */
+	ODB_FSCK_VERBOSE = (1 << 2),
 };
 
 /* Options that shall be passed to `odb_fsck()`. */
diff --git a/odb/source-loose.c b/odb/source-loose.c
index f68d3c4d6c..efef9ca61f 100644
--- a/odb/source-loose.c
+++ b/odb/source-loose.c
@@ -12,6 +12,7 @@
 #include "odb/streaming.h"
 #include "oidtree.h"
 #include "path.h"
+#include "progress.h"
 #include "repository.h"
 #include "strbuf.h"
 #include "tempfile.h"
@@ -1031,12 +1032,96 @@ static void odb_source_loose_free(struct odb_source *source)
 	free(loose);
 }
 
-static int odb_source_loose_fsck(struct odb_source *source UNUSED,
-				 struct odb_fsck_options *opts UNUSED)
+struct fsck_loose_data {
+	struct odb_source_loose *source;
+	struct odb_fsck_options *opts;
+	struct progress *progress;
+	bool error_found;
+};
+
+static int fsck_loose(const struct object_id *oid, const char *path,
+		      void *cb_data)
 {
+	struct fsck_loose_data *data = cb_data;
+	enum object_type type = OBJ_NONE;
+	size_t size;
+	void *contents = NULL;
+	int eaten = 0;
+	struct object_info oi = OBJECT_INFO_INIT;
+	struct object_id real_oid = *null_oid(data->source->base.odb->repo->hash_algo);
+	int err = 0;
+
+	oi.sizep = &size;
+	oi.typep = &type;
+
+	if (read_loose_object(data->source->base.odb->repo,
+			      path, oid, &real_oid, &contents, &oi) < 0) {
+		if (contents && !oideq(&real_oid, oid))
+			err = error(_("%s: hash-path mismatch, found at: %s"),
+				    oid_to_hex(&real_oid), path);
+		else
+			err = error(_("%s: object corrupt or missing: %s"),
+				    oid_to_hex(oid), path);
+	}
+	if (err < 0)
+		goto out;
+
+	if (!contents && type != OBJ_BLOB)
+		BUG("read_loose_object streamed a non-blob");
+
+	if (data->opts->object_cb(oid, type, size, contents, &eaten,
+				  data->opts->object_payload)) {
+		err = -1;
+		goto out;
+	}
+
+out:
+	if (err)
+		data->error_found = true;
+	if (!eaten)
+		free(contents);
+	return 0; /* keep checking other objects, even if we saw an error */
+}
+
+static int fsck_cruft(const char *basename, const char *path,
+		      void *data UNUSED)
+{
+	if (!starts_with(basename, "tmp_obj_"))
+		fprintf_ln(stderr, _("bad sha1 file: %s"), path);
+	return 0;
+}
+
+static int fsck_subdir(unsigned int nr, const char *path UNUSED, void *cb_data)
+{
+	struct fsck_loose_data *data = cb_data;
+	display_progress(data->progress, nr + 1);
 	return 0;
 }
 
+static int odb_source_loose_fsck(struct odb_source *source,
+				 struct odb_fsck_options *opts)
+{
+	struct odb_source_loose *loose = odb_source_loose_downcast(source);
+	struct fsck_loose_data data = {
+		.source = loose,
+		.opts = opts,
+	};
+
+	if (opts->flags & ODB_FSCK_VERBOSE)
+		fprintf_ln(stderr, _("Checking object directory"));
+
+	if (opts->flags & ODB_FSCK_PROGRESS)
+		data.progress = start_progress(source->odb->repo,
+					       _("Checking object directories"), 256);
+
+	for_each_loose_file_in_source(source, fsck_loose,
+				      fsck_cruft, fsck_subdir, &data);
+	display_progress(data.progress, 256);
+	stop_progress(&data.progress);
+
+	return data.error_found ? -1 : 0;
+}
+
 struct odb_source_loose *odb_source_loose_new(struct object_database *odb,
 					      const char *path,
 					      bool local)

-- 
2.55.0.1074.ge7621b4bad.dirty