patch, 2 partstree-walk improvements

> From: Jeff King <peff@peff.net>
> 
> When the tree-walker runs into an error, it just calls
> die(), and the message is always "corrupt tree file".
> However, we are actually covering several cases here; let's
> give the user a hint about what happened.
> 
> Let's also avoid using the word "corrupt", which makes it
> seem like the data bit-rotted on disk. Our sha1 check would
> already have found that. These errors are ones of data that
> is malformed in the first place.
> 
> Signed-off-by: David Turner <dturner@twosigma.com>
> Signed-off-by: Jeff King <peff@peff.net>

> -test_expect_success 'corrupt tree' '
> +test_expect_success 'too-short tree' '
>  	echo abc >malformed-tree &&
> -	test_must_fail git hash-object -t tree malformed-tree
> +	test_must_fail git hash-object -t tree malformed-tree 2>err &&
> +	grep "too-short tree object" err
> +'

> diff --git a/t/t1007/tree-with-empty-filename b/t/t1007/tree-with-empty-filename
> new file mode 100644
> index 0000000000000000000000000000000000000000..aeb1ceb20e485eebd0acbb81c974d1c6fedcc1fe
> GIT binary patch
> literal 28
> kcmXpsFfcPQQDAsB_tET47q2;ccWbUIkGgT_Nl)-Z0Hx{;SO5S3
> 
> literal 0
> HcmV?d00001

>> +test_expect_success 'malformed mode in tree' '
>> +	test_must_fail git hash-object -t tree ../t1007/tree-with-malformed-mode 2>err &&
>> +	grep "malformed mode in tree entry for tree" err
>> +'
>
> This ".." will break when the test is run with "--root". You should use
>
>   "$TEST_DIRECTORY"/t1007/...
>
> instead. And ditto in the second test, of course.

> I also wonder if $bin_sha1 should actually be more like:
> 
>   hex_sha1=$(echo foo | git hash-object --stdin -w)
>   bin_sha1=$(echo $hex_sha1 | perl -ne 'printf "\\%3o", ord for /./g')
> 
> so that it's a real sha1 (or maybe it is in your original, from an
> object that happens to be in the repo; it's hard to tell). I wouldn't
> expect the code to actually get to the point of looking at the sha1, but
> it's perhaps a more realistic test.

> @@ -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;
> +	}

> @@ -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;
> +		}

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

> From: David Turner <dturner@twosigma.com>
>
> 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.
>
> Signed-off-by: David Turner <dturner@twosigma.com>
> ---
>  fsck.c                                             |  18 +++--
>  t/t1450-fsck.sh                                    |  17 ++++-
>  .../307e300745b82417cc1a903f875c7d22e45ef907       |   4 +

> To prevent further headaches in this directory, can we have
> .gitattributes that tells us that everything in there are binary
> files?  Something like the attached.
>
> The other object was transferred as a binary patch, but I have no
> faith in what I applied from your e-mail message for this file that
> went though latin-1 to utf-8 conversion X-<.
>
>  t/t1450/bad-objects/.gitattributes | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/t/t1450/bad-objects/.gitattributes b/t/t1450/bad-objects/.gitattributes
> new file mode 100644
> index 0000000..a173f27
> --- /dev/null
> +++ b/t/t1450/bad-objects/.gitattributes
> @@ -0,0 +1 @@
> +[0-9a-f]*[0-9a-f]	-diff

> 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)) {

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

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

> 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�c
I�P��4�)�+b&��]��R`�2ܚ��)ex�-:xּ��%m�������H�d{-�
> +Q��Ҁ��w,p�
> +���&�?�
> \ No newline at end of file

> +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;
>  }

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

> >   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.
> 
> I don't really like err_fn because:
> (a) without a baton, it's somewhat less general (or less thread-safe)
> than the strbuf approach and
> (b) with a baton, it's two arguments instead of one.

> On Tue, Sep 27, 2016 at 11:19:34AM -0400, David Turner wrote:
> 
> > >   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.
> > 
> > I don't really like err_fn because:
> > (a) without a baton, it's somewhat less general (or less thread-safe)
> > than the strbuf approach and
> > (b) with a baton, it's two arguments instead of one.
> 
> I'm going to ramble for a minute, and I don't think it's worth exploring
> for this patch series in particular, so feel free to ignore me.
> 
> I think this error concept could be extended fairly elegantly with
> something like:
> 
>   typedef void (*err_fn)(void *, const char *fmt, va_list ap)
>   struct error_context {
>         err_fn fn;
>         void *data;
>   };
> 
>   int report_error(struct error_context *err, const char *fmt, ...)
>   {
>         if (err->fn) {
>                 va_list ap;
>                 va_start(ap, fmt);
>                 err->fn(err->data, fmt, ap);
>                 va_end(ap);
>         }
>         return -1;
>   }
> 
> Then low-level functions just take a context and do:
> 
>   return report_error(&err, "some error: %s", foo);
> 
> And then the callers would pick one of a few generic error contexts:
> 
>   - passing NULL silences the errors

> >   int report_error(struct error_context *err, const char *fmt, ...)
> >   {
> >         if (err->fn) {
> >                 va_list ap;
> >                 va_start(ap, fmt);
> >                 err->fn(err->data, fmt, ap);
> >                 va_end(ap);
> >         }
> >         return -1;
> >   }
> > 
> > Then low-level functions just take a context and do:
> > 
> >   return report_error(&err, "some error: %s", foo);
> > 
> > And then the callers would pick one of a few generic error contexts:
> > 
> >   - passing NULL silences the errors
> 
> Overall, +1.
> 
> I guess I would rather have a sentinel value for silencing errors,
> because I'm worried that someone might read NULL as "don't handle the
> errors, just die".  Of course, code review would hopefully catch this,
> but even so, it would be easier to read foo(x, y, silence_errors) than
> foo(x, y, null).

> [...]
> I'm going to ramble for a minute, and I don't think it's worth exploring
> for this patch series in particular, so feel free to ignore me.
> 
> I think this error concept could be extended fairly elegantly with
> something like:
> 
>   typedef void (*err_fn)(void *, const char *fmt, va_list ap)
>   struct error_context {
>         err_fn fn;
>         void *data;
>   };
> 
>   int report_error(struct error_context *err, const char *fmt, ...)
>   {
>         if (err->fn) {
>                 va_list ap;
>                 va_start(ap, fmt);
>                 err->fn(err->data, fmt, ap);
>                 va_end(ap);
>         }
>         return -1;
>   }

> Then low-level functions just take a context and do:
> 
>   return report_error(&err, "some error: %s", foo);
> 
> And then the callers would pick one of a few generic error contexts:
> 
>   - passing NULL silences the errors
> 
>   - a global for chaining to error, like:
> 
>        struct error_context print_errors = {
>           error, /* actually a wrapper to handle va_list and NULL data */
>           NULL
>        };

> >   - a global for chaining to error, like:
> > 
> >        struct error_context print_errors = {
> >           error, /* actually a wrapper to handle va_list and NULL data */
> >           NULL
> >        };
> 
> There could also be a global for chaining to `warn()` or `die()`.

>  	if (!dont_change_ref) {
>  		struct ref_transaction *transaction;
> -		struct strbuf err = STRBUF_INIT;
> -
> -		transaction = ref_transaction_begin(&err);
> -		if (!transaction ||
> -		    ref_transaction_update(transaction, ref.buf,
> -					   sha1, forcing ? NULL : null_sha1,
> -					   0, msg, &err) ||
> -		    ref_transaction_commit(transaction, &err))
> -			die("%s", err.buf);
> +
> +		transaction = ref_transaction_begin(&error_die);
> +		ref_transaction_update(transaction, ref.buf,
> +				       sha1, forcing ? NULL : null_sha1,
> +				       0, msg, &error_die);
> +		ref_transaction_commit(transaction, &error_die);
>  		ref_transaction_free(transaction);
> -		strbuf_release(&err);
>  	}
>  
>  	if (real_ref && track)
>
> which is much shorter and to the point (it does rely on the called
> functions always calling report_error() and never just returning NULL or
> "-1", but that should be the already. If it isn't, we'd be printing
> "fatal: " with no message).

> The first patch is a re-roll of Peff's patch from 2014 -- here's
> the archive message:
>
> http://git.661346.n2.nabble.com/PATCH-tree-walk-be-more-specific-about-corrupt-tree-errors-td7603558.html
>
> Not sure why this wasn't applied then, but I thought it looked pretty
> good, so I added a few tests.
>
> Hopefully the encoding works correctly on these patches. If not, you
> can fetch from
> https://github.com/novalis/git/
> on branch dturner/bad-trees
>
> Email address note 1: my employer wants me to use my company address,
> but not my company computer, for patches I write on work time.  This
> means that I'm going to continue corresponding from
> novalis@novalis.org, but will send patches with the @twosigma.com
> address in the author line.

> The first patch is a re-roll of Peff's patch from 2014 -- here's
> the archive message:
>
> http://git.661346.n2.nabble.com/PATCH-tree-walk-be-more-specific-about-corrupt-tree-errors-td7603558.html
>
> Not sure why this wasn't applied then, but I thought it looked pretty
> good, so I added a few tests.

> Email address note 1: my employer wants me to use my company address,
> but not my company computer, for patches I write on work time.  This
> means that I'm going to continue corresponding from
> novalis@novalis.org, but will send patches with the @twosigma.com
> address in the author line.

> David Turner <novalis@novalis.org> writes:
> 
> > The first patch is a re-roll of Peff's patch from 2014 -- here's
> > the archive message:
> >
> > http://git.661346.n2.nabble.com/PATCH-tree-walk-be-more-specific-about-corrupt-tree-errors-td7603558.html
> >
> > Not sure why this wasn't applied then, but I thought it looked pretty
> > good, so I added a few tests.
> 
> Thanks.  Adding tests is very much appreciated.  I however wonder
> why you needed to reword a perfectly readable "truncated" to
> something else, though?