Subject: Re: [PATCH v6 01/19] fsck: Introduce fsck options

From: Junio C Hamano <[email-masked]>
Date: Fri, 19 Jun 2015 12:03:25 -0700

Johannes Schindelin <[email-masked]> writes:

> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 48fa472..87ae9ba 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -75,6 +75,7 @@ static int nr_threads;
>  static int from_stdin;
>  static int strict;
>  static int do_fsck_object;
> +static struct fsck_options fsck_options = FSCK_OPTIONS_STRICT;

So there is a global fsck_options used throughout the entire
session here.

> @@ -838,10 +839,10 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,
>  			if (!obj)
>  				die(_("invalid %s"), typename(type));
>  			if (do_fsck_object &&
> -			    fsck_object(obj, buf, size, 1,
> -				    fsck_error_function))
> +			    fsck_object(obj, buf, size, &fsck_options))
>  				die(_("Error in object"));

And that is used here to inspect each and every object we encounter.

> -			if (fsck_walk(obj, mark_link, NULL))
> +			fsck_options.walk = mark_link;

Then we do a call to fsck_walk() starting from this object, letting
mark_link() to inspect it and set the LINK bit.

> +			if (fsck_walk(obj, NULL, &fsck_options))
>  				die(_("Not all child objects of %s are reachable"), sha1_to_hex(obj->sha1));

Since nobody else sets fsck_options.walk to any other value, and
nobody else calls fsck_walk(), shouldn't that assignment be done
only once somewhere a lot higher in the callchain?  The apparent
"overriding while inspecting this object" that does not have any
corresponding "now we are done, so revert it to the original value"
puzzled me, and I am sure it would puzzle future readers of this
code.