Jeff King <p...@peff.net> writes:

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

> 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#??} &&

Reply via email to