Re: [PATCH 2/2] fsck: handle bad trees like other errors

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 26, 2016, 19:51 UTC
Message-ID: <xmqqlgyeqw20.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1474918365-10937-3-git-send-email-novalis@novalis.org>

David Turner <novalis@novalis.org> writes:

Show 9 quoted lines

> @@ -535,7 +536,10 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)
>  	unsigned o_mode;
>  	const char *o_name;
>  
> -	init_tree_desc(&desc, item->buffer, item->size);
> +	if (init_tree_desc_gently(&desc, item->buffer, item->size)) {
> +		retval += report(options, &item->object, FSCK_MSG_BAD_TREE, "cannot be parsed as a tree");
> +		return retval;
> +	}

Good. If BAD_TREE is being ignored, this may report a non-error, but we won't descend into the unreadable tree so it is OK.

Show 9 quoted lines

> @@ -556,7 +560,10 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)
>  			       is_hfs_dotgit(name) ||
>  			       is_ntfs_dotgit(name));
>  		has_zero_pad |= *(char *)desc.buffer == '0';
> -		update_tree_entry(&desc);
> +		if (update_tree_entry_gently(&desc)) {
> +			retval += report(options, &item->object, FSCK_MSG_BAD_TREE, "cannot be parsed as a tree");
> +			break;
> +		}

Likewise; breaking out of the loop will stop us from reading further into the corrupted tree data, so this is good.

Show 5 quoted lines

> @@ -597,7 +604,6 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)
>  		o_name = name;
>  	}
>  
> -	retval = 0;

Good code hygiene that you moved this to the very top where it is defined, so anybody before this step can set it if it wants to.

Reading purely from the text of this function, it was surprising that you can do without a gently variant of tree_entry_extract(), but it merely reads into two variables and does not do any error detection (which happens all in the caller), so it is not at all surprising after all ;-)

I didn't see anything objectionable in this patch. Thanks for working on this.