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

> 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
>
>