On Mon, Sep 26, 2016 at 03:32:45PM -0400, David Turner wrote:
> Instead of dying when fsck hits a malformed tree object, log the error
> like any other and continue. Now fsck can tell the user which tree is
> bad, too.
Cool. I think the lack of this is what made me drag my feet on the first
patch. Thanks for finishing it off.
> diff --git a/fsck.c b/fsck.c
> index c9cf3de..4a3069e 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -347,8 +347,9 @@ static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *op
> return -1;
>
> name = get_object_name(options, &tree->object);
> - init_tree_desc(&desc, tree->buffer, tree->size);
> - while (tree_entry(&desc, &entry)) {
> + if (init_tree_desc_gently(&desc, tree->buffer, tree->size))
> + return -1;
> + while (tree_entry_gently(&desc, &entry)) {
I wondered if other callers would be happy with init_tree_desc_gently().
Grepping for init_tree_desc(), it seems like it would be a fairly
trivial conversion for most of them, because they almost invariably run
unpack_trees() right afterwards, and so have to deal with errors from
it.
So perhaps in the long run we can convert them all. But certainly that
does not need to be part of this series.
> +test_expect_success 'unparseable tree object' '
> + test_when_finished "git update-ref -d refs/heads/wrong" &&
> + test_when_finished "remove_object 307e300745b82417cc1a903f875c7d22e45ef907" &&
> + test_when_finished "remove_object f506a346749bb96f52d8605ffba9fb93d46b5ffd" &&
> + mkdir -p .git/objects/30 mkdir -p .git/objects/f5 &&
> + cp ../t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907 .git/objects/30/7e300745b82417cc1a903f875c7d22e45ef907 &&
> + cp ../t1450/bad-objects/f506a346749bb96f52d8605ffba9fb93d46b5ffd .git/objects/f5/06a346749bb96f52d8605ffba9fb93d46b5ffd &&
This needs the same $TEST_DIRECTORY treatment as t1007.
> + git update-ref refs/heads/wrong 307e300745b82417cc1a903f875c7d22e45ef907 &&
> + test_must_fail git fsck 2>out &&
> + grep "warning: empty filename in tree entry" out &&
> + grep "f506a346749bb96f52d8605ffba9fb93d46b5ffd" out &&
> + ! grep "fatal: empty filename in tree entry" out
> +'
I'd also expect these to be test_i18ngrep, but I see that t1450 is quite
bad about this in general. I'm OK with adding them as greps and leaving
a conversion of the whole script until later.
> diff --git a/t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907 b/t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907
> new file mode 100644
> index 0000000..6e23d62
> --- /dev/null
> +++ b/t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907
> @@ -0,0 +1,4 @@
> +x��A�E]s���f�@�� �0Z[
> +� \�i�cI�4��b&��R`�ܚ��)ex�:xּ������H�{-�> +Q ����w,p� +���
> \ No newline at end of file
Yikes. :)
I wonder if some printfs, similar to what I showed in the last patch,
combined with "hash-object --literally", could make these tests more
readable and avoid the binary goo.
> -static void decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size)
> +static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size, struct strbuf *err)
> {
I know we used the "err" strbuf pattern in the ref code, and it makes
sense there where we have a lot of different functions with public
interfaces. But here, we literally just feed the result to die() or
warning(). I wonder if a nicer interface would be:
typedef void (*err_fn)(const char *, ...);
static int decode_tree_entry(struct tree_desc *desc,
const char *buf, unsigned long size,
err_fn err)
{
...
if (size < 23 || buf[size - 21]) {
err("too-short tree object");
return -1;
}
}
I dunno. Maybe that is overengineering. I guess we only hit the strbufs
in the error path (which used to die!), so nobody really cares that much
about the extra allocation.
> +int init_tree_desc_gently(struct tree_desc *desc, const void *buffer, unsigned long size)
> +{
> + struct strbuf err = STRBUF_INIT;
> + int result = init_tree_desc_internal(desc, buffer, size, &err);
> + if (result)
> + warning("%s", err.buf);
> + strbuf_release(&err);
> + return result;
> }
I also wonder if this ought to be "error()" and not "warning()". I think
it's pretty common for fsck to spit out errors from sub-code but keep going.
-Peff