Re: [PATCH 3/6] fsck: prepare dummy objects for --connectivity-check

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 17, 2017, 21:15 UTC
Message-ID: <xmqqa8ape6b6.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170116213204.e7ykwowqzafkexqd@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 7 quoted lines

> +static void mark_object_for_connectivity(const unsigned char *sha1)
> +{
> +	struct object *obj = lookup_object(sha1);
> +
> +	/*
> +	 * Setting the object type here isn't strictly necessary here for a
> +	 * connectivity check.

Drop one of these "here"s?

> The cmd_fsck() part of the diff is pretty nasty without
> "-b".

True. I also wonder if swapping the if/else arms make the end result and the patch easier to read. i.e.

+	if (connectivity_only) {
+		mark loose for connectivity;
+		mark packed for connectivity;
+	} else {
		... existing code comes here reindented ...
	}

But the patch makes sense.

Show 38 quoted lines

> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
> index e88ec7747..c1b2dda33 100755
> --- a/t/t1450-fsck.sh
> +++ b/t/t1450-fsck.sh
> @@ -523,9 +523,21 @@ test_expect_success 'fsck --connectivity-only' '
>  		touch empty &&
>  		git add empty &&
>  		test_commit empty &&
> +
> +		# Drop the index now; we want to be sure that we
> +		# recursively notice that we notice the broken objects
> +		# because they are reachable from refs, not because
> +		# they are in the index.
> +		rm -f .git/index &&
> +
> +		# corrupt the blob, but in a way that we can still identify
> +		# its type. That lets us see that --connectivity-only is
> +		# not actually looking at the contents, but leaves it
> +		# free to examine the type if it chooses.
>  		empty=.git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 &&
> -		rm -f $empty &&
> -		echo invalid >$empty &&
> +		blob=$(echo unrelated | git hash-object -w --stdin) &&
> +		mv $(sha1_file $blob) $empty &&
> +
>  		test_must_fail git fsck --strict &&
>  		git fsck --strict --connectivity-only &&
>  		tree=$(git rev-parse HEAD:) &&
> @@ -537,6 +549,13 @@ test_expect_success 'fsck --connectivity-only' '
>  	)
>  '
>  
> +test_expect_success 'fsck --connectivity-only with explicit head' '
> +	(
> +		cd connectivity-only &&
> +		test_must_fail git fsck --connectivity-only $tree
> +	)
> +'

Most of the earlier "tree=..." assignments are done in subshells, and it is not clear which tree this refers to. Is this the one that was written in 'rev-list --verify-objects with bad sha1' that has been removed in its when-finished handler?

>  remove_loose_object () {
>  	sha1="$(git rev-parse "$1")" &&
>  	remainder=${sha1#??} &&