Re: [PATCH v2 09/10] builtin/fsck: move multi-pack index verification into the packed source
- From
Toon Claes <toon@iotcl.com>
- Date
- Sep 11, 2026, 11:14 UTC
- Message-ID
- <877bksnior.fsf@emacs.iotcl.com>
- In-Reply-To
- <20260831-pks-odb-source-fsck-v2-9-f9b16ef4957b@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 72 quoted lines
> 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)?
Show 24 quoted lines
> + 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?
Show 34 quoted lines
> + 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