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

[Toon Claes <toon@iotcl.com>]

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