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

[Toon Claes <toon@iotcl.com>]

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