[PATCH 0/2] tree-walk improvements

[PATCH 0/2] tree-walk improvements

[David Turner]

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.

Email address note 2: I'm not subscribed to the mailing list these
days, so please CC me (at novalis@novalis.org) on replies.

David Turner (1):
  fsck: handle bad trees like other errors

Jeff King (1):
  tree-walk: be more specific about corrupt tree errors

 fsck.c                                             |  18 +++--
 t/t1007-hash-object.sh                             |  15 +++-
 t/t1007/tree-with-empty-filename                   | Bin 0 -> 28 bytes
 t/t1007/tree-with-malformed-mode                   | Bin 0 -> 39 bytes
 t/t1450-fsck.sh                                    |  17 ++++-
 .../307e300745b82417cc1a903f875c7d22e45ef907       |   4 +
 .../f506a346749bb96f52d8605ffba9fb93d46b5ffd       | Bin 0 -> 45 bytes
 tree-walk.c                                        |  83 ++++++++++++++++++---
 tree-walk.h                                        |   8 ++
 9 files changed, 125 insertions(+), 20 deletions(-)
 create mode 100644 t/t1007/tree-with-empty-filename
 create mode 100644 t/t1007/tree-with-malformed-mode
 create mode 100644 t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907
 create mode 100644 t/t1450/bad-objects/f506a346749bb96f52d8605ffba9fb93d46b5ffd

-- 
2.8.0.rc4.22.g8ae061a

[PATCH 1/2] tree-walk: be more specific about corrupt tree errors

[David Turner]

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>
---
 t/t1007-hash-object.sh           |  15 +++++++++++++--
 t/t1007/tree-with-empty-filename | Bin 0 -> 28 bytes
 t/t1007/tree-with-malformed-mode | Bin 0 -> 39 bytes
 tree-walk.c                      |  12 +++++++-----
 4 files changed, 20 insertions(+), 7 deletions(-)
 create mode 100644 t/t1007/tree-with-empty-filename
 create mode 100644 t/t1007/tree-with-malformed-mode

diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
index acca9ac..cd10c73 100755
--- a/t/t1007-hash-object.sh
+++ b/t/t1007-hash-object.sh
@@ -183,9 +183,20 @@ for args in "-w --stdin-paths" "--stdin-paths -w"; do
 	pop_repo
 done
 
-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
+'
+
+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
+'
+
+test_expect_success 'empty filename in tree' '
+	test_must_fail git hash-object -t tree ../t1007/tree-with-empty-filename 2>err &&
+	grep "empty filename in tree entry for tree" err
 '
 
 test_expect_success 'corrupt commit' '
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

diff --git a/t/t1007/tree-with-malformed-mode b/t/t1007/tree-with-malformed-mode
new file mode 100644
index 0000000000000000000000000000000000000000..24aa84d60ef8e269fb0b29c67b5208639b9da3ae
GIT binary patch
literal 39
vcmYewPcJRb%}+^HNXyJg%}dNpWq3CC(d<nZuQ_{nYpyGgx^d`9Pw+$lU*Quk

literal 0
HcmV?d00001

diff --git a/tree-walk.c b/tree-walk.c
index ce27842..ba544cf 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -27,12 +27,14 @@ static void decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned
 	const char *path;
 	unsigned int mode, len;
 
-	if (size < 24 || buf[size - 21])
-		die("corrupt tree file");
+	if (size < 23 || buf[size - 21])
+		die("too-short tree object");
 
 	path = get_mode(buf, &mode);
-	if (!path || !*path)
-		die("corrupt tree file");
+	if (!path)
+		die("malformed mode in tree entry for tree");
+	if (!*path)
+		die("empty filename in tree entry for tree");
 	len = strlen(path) + 1;
 
 	/* Initialize the descriptor entry */
@@ -81,7 +83,7 @@ void update_tree_entry(struct tree_desc *desc)
 	unsigned long len = end - (const unsigned char *)buf;
 
 	if (size < len)
-		die("corrupt tree file");
+		die("too-short tree file");
 	buf = end;
 	size -= len;
 	desc->buffer = buf;
-- 
2.8.0.rc4.22.g8ae061a

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

[David Turner]

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 +
 .../f506a346749bb96f52d8605ffba9fb93d46b5ffd       | Bin 0 -> 45 bytes
 tree-walk.c                                        |  83 ++++++++++++++++++---
 tree-walk.h                                        |   8 ++
 6 files changed, 111 insertions(+), 19 deletions(-)
 create mode 100644 t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907
 create mode 100644 t/t1450/bad-objects/f506a346749bb96f52d8605ffba9fb93d46b5ffd

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)) {
 		struct object *obj;
 		int result;
 
@@ -520,7 +521,7 @@ static int verify_ordered(unsigned mode1, const char *name1, unsigned mode2, con
 
 static int fsck_tree(struct tree *item, struct fsck_options *options)
 {
-	int retval;
+	int retval = 0;
 	int has_null_sha1 = 0;
 	int has_full_path = 0;
 	int has_empty_name = 0;
@@ -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;
+	}
 
 	o_mode = 0;
 	o_name = NULL;
@@ -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;
+		}
 
 		switch (mode) {
 		/*
@@ -597,7 +604,6 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)
 		o_name = name;
 	}
 
-	retval = 0;
 	if (has_null_sha1)
 		retval += report(options, &item->object, FSCK_MSG_NULL_SHA1, "contains entries pointing to null sha1");
 	if (has_full_path)
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 8f52da2..f456963 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -188,8 +188,7 @@ test_expect_success 'commit with NUL in header' '
 	grep "error in commit $new.*unterminated header: NUL at offset" out
 '
 
-test_expect_success 'malformatted tree object' '
-	test_when_finished "git update-ref -d refs/tags/wrong" &&
+test_expect_success 'tree object with duplicate entries' '
 	test_when_finished "remove_object \$T" &&
 	T=$(
 		GIT_INDEX_FILE=test-index &&
@@ -208,6 +207,20 @@ test_expect_success 'malformatted tree object' '
 	grep "error in tree .*contains duplicate file entries" out
 '
 
+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
+'
+
 test_expect_success 'tag pointing to nonexistent' '
 	cat >invalid-tag <<-\EOF &&
 	object ffffffffffffffffffffffffffffffffffffffff
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\x01ŽA\x0e \x10E]sŠ¹€f°@Ä\x18\x17\x1eÁ\v0\x05Z\x12[\x12
+õú¢é	\ýüÅ{ÿ\x0fižc\x01IòP²÷\x104\x1aÛ)Ó+b&\x13ôÙ]\fê\x10ØR`êœ2Üš\x13¶–)exØ-:xÖ¼ø\f×%mö\x15×ûž§”Ç^[HÕd\x12{-áˆ
+Q\f¿ÍÒ€\x7fè\x1d‡w,\x13p\x1aë
+ßçâ\x03&ë?Þ
\ No newline at end of file
diff --git a/t/t1450/bad-objects/f506a346749bb96f52d8605ffba9fb93d46b5ffd b/t/t1450/bad-objects/f506a346749bb96f52d8605ffba9fb93d46b5ffd
new file mode 100644
index 0000000000000000000000000000000000000000..9111a7fc3c8578906e13c930a0fbd3cae047762e
GIT binary patch
literal 45
zcmb=Jqpj)X8)~pA!NA18z}PS_p~CF@#W%j<>n*Fxv)5_&?<#!Z>Hoon;loq@NdS%f
B6F2|>

literal 0
HcmV?d00001

diff --git a/tree-walk.c b/tree-walk.c
index ba544cf..0fb830b 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -22,33 +22,60 @@ static const char *get_mode(const char *str, unsigned int *modep)
 	return str;
 }
 
-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)
 {
 	const char *path;
 	unsigned int mode, len;
 
-	if (size < 23 || buf[size - 21])
-		die("too-short tree object");
+	if (size < 23 || buf[size - 21]) {
+		strbuf_addstr(err, "too-short tree object");
+		return -1;
+	}
 
 	path = get_mode(buf, &mode);
-	if (!path)
-		die("malformed mode in tree entry for tree");
-	if (!*path)
-		die("empty filename in tree entry for tree");
+	if (!path) {
+		strbuf_addstr(err, "malformed mode in tree entry");
+		return -1;
+	}
+	if (!*path) {
+		strbuf_addstr(err, "empty filename in tree entry");
+		return -1;
+	}
 	len = strlen(path) + 1;
 
 	/* Initialize the descriptor entry */
 	desc->entry.path = path;
 	desc->entry.mode = canon_mode(mode);
 	desc->entry.oid  = (const struct object_id *)(path + len);
+
+	return 0;
 }
 
-void init_tree_desc(struct tree_desc *desc, const void *buffer, unsigned long size)
+static int init_tree_desc_internal(struct tree_desc *desc, const void *buffer, unsigned long size, struct strbuf *err)
 {
 	desc->buffer = buffer;
 	desc->size = size;
 	if (size)
-		decode_tree_entry(desc, buffer, size);
+		return decode_tree_entry(desc, buffer, size, err);
+	return 0;
+}
+
+void init_tree_desc(struct tree_desc *desc, const void *buffer, unsigned long size)
+{
+	struct strbuf err = STRBUF_INIT;
+	if (init_tree_desc_internal(desc, buffer, size, &err))
+		die("%s", err.buf);
+	strbuf_release(&err);
+}
+
+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;
 }
 
 void *fill_tree_descriptor(struct tree_desc *desc, const unsigned char *sha1)
@@ -75,7 +102,7 @@ static void entry_extract(struct tree_desc *t, struct name_entry *a)
 	*a = t->entry;
 }
 
-void update_tree_entry(struct tree_desc *desc)
+static int update_tree_entry_internal(struct tree_desc *desc, struct strbuf *err)
 {
 	const void *buf = desc->buffer;
 	const unsigned char *end = desc->entry.oid->hash + 20;
@@ -89,7 +116,30 @@ void update_tree_entry(struct tree_desc *desc)
 	desc->buffer = buf;
 	desc->size = size;
 	if (size)
-		decode_tree_entry(desc, buf, size);
+		return decode_tree_entry(desc, buf, size, err);
+	return 0;
+}
+
+void update_tree_entry(struct tree_desc *desc)
+{
+	struct strbuf err = STRBUF_INIT;
+	if (update_tree_entry_internal(desc, &err))
+		die("%s", err.buf);
+	strbuf_release(&err);
+}
+
+int update_tree_entry_gently(struct tree_desc *desc)
+{
+	struct strbuf err = STRBUF_INIT;
+	if (update_tree_entry_internal(desc, &err)) {
+		warning("%s", err.buf);
+		strbuf_release(&err);
+		/* Stop processing this tree after error */
+		desc->size = 0;
+		return -1;
+	}
+	strbuf_release(&err);
+	return 0;
 }
 
 int tree_entry(struct tree_desc *desc, struct name_entry *entry)
@@ -102,6 +152,17 @@ int tree_entry(struct tree_desc *desc, struct name_entry *entry)
 	return 1;
 }
 
+int tree_entry_gently(struct tree_desc *desc, struct name_entry *entry)
+{
+	if (!desc->size)
+		return 0;
+
+	*entry = desc->entry;
+	if (update_tree_entry_gently(desc))
+		return 0;
+	return 1;
+}
+
 void setup_traverse_info(struct traverse_info *info, const char *base)
 {
 	int pathlen = strlen(base);
diff --git a/tree-walk.h b/tree-walk.h
index 97a7d69..68bb78b 100644
--- a/tree-walk.h
+++ b/tree-walk.h
@@ -25,14 +25,22 @@ static inline int tree_entry_len(const struct name_entry *ne)
 	return (const char *)ne->oid - ne->path - 1;
 }
 
+/*
+ * The _gently versions of these functions warn and return false on a
+ * corrupt tree entry rather than dying,
+ */
+
 void update_tree_entry(struct tree_desc *);
+int update_tree_entry_gently(struct tree_desc *);
 void init_tree_desc(struct tree_desc *desc, const void *buf, unsigned long size);
+int init_tree_desc_gently(struct tree_desc *desc, const void *buf, unsigned long size);
 
 /*
  * Helper function that does both tree_entry_extract() and update_tree_entry()
  * and returns true for success
  */
 int tree_entry(struct tree_desc *, struct name_entry *);
+int tree_entry_gently(struct tree_desc *, struct name_entry *);
 
 void *fill_tree_descriptor(struct tree_desc *desc, const unsigned char *sha1);
 
-- 
2.8.0.rc4.22.g8ae061a

Re: [PATCH 0/2] tree-walk improvements

[Stefan Beller]

On Mon, Sep 26, 2016 at 12:32 PM, David Turner <novalis@novalis.org> wrote:
> 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.

Mind sending a patch for .mailmap to reflect that different email
addresses are still the same person? ;)

Re: [PATCH 0/2] tree-walk improvements

[Junio C Hamano]

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?

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

That seems like not an uncommon practice ;-)

> Email address note 2: I'm not subscribed to the mailing list these
> days, so please CC me (at novalis@novalis.org) on replies.

It is good to tell others this, but I suspect that it is known by
those who are likely to respond to these messages that always CC'ing
to individual is the norm on this list ;-)

Thanks.

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

[Junio C Hamano]

David Turner <novalis@novalis.org> writes:

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

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

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

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

[Junio C Hamano]

David Turner <novalis@novalis.org> writes:

> 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

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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

I suspect that the t/t1007 directory in 1/2 may deserve a similar
treatment.

Re: [PATCH 0/2] tree-walk improvements

[David Turner]

On Mon, 2016-09-26 at 12:43 -0700, Junio C Hamano wrote:
> 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?

Because truncated, to me, means "something that has been cut off". Here,
the recorded length is too short, so it's probably not the case that
something was cut off -- it was never right to begin with.

Re: [PATCH 0/2] tree-walk improvements

[Junio C Hamano]

David Turner <novalis@novalis.org> writes:

> Hopefully the encoding works correctly on these patches. If not, you
> can fetch from
> https://github.com/novalis/git/
> on branch dturner/bad-trees

This does not test cleanly here, unfortunately.  Specifically, tests
30 and 31 t1007 do fine with 1/2 alone, but they seem to break with
2/2 applied.

I didn't dug further.  At least not yet.

Re: [PATCH 0/2] tree-walk improvements

[Junio C Hamano]

David Turner <novalis@novalis.org> writes:

> Because truncated, to me, means "something that has been cut off". Here,
> the recorded length is too short, so it's probably not the case that
> something was cut off -- it was never right to begin with.

That's perfectly sensible. Thanks.

Re: [PATCH 1/2] tree-walk: be more specific about corrupt tree errors

[Jeff King]

On Mon, Sep 26, 2016 at 03:32:44PM -0400, David Turner wrote:

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

Yay. This has been on my "to look at and repost" list for literally 2
years. Thanks for picking it up (see kids, procrastination _does_ pay
off).

>  t/t1007-hash-object.sh           |  15 +++++++++++++--
>  t/t1007/tree-with-empty-filename | Bin 0 -> 28 bytes
>  t/t1007/tree-with-malformed-mode | Bin 0 -> 39 bytes

Ooh, and tests. Exciting.

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

Should this be test_i18ngrep? Even if the message is not translated now,
it seems like a good proactive measure (and probably it _should_ be
translated).

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

> 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

This is rather opaque, of course. :)

I wonder if it would be possible to generate the test vector with
something like:

  # any 20 bytes will do
  bin_sha1='\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0'

  printf "100644 \0$bin_sha1" >tree-with-empty-filename

I know that is longer and possibly more error-prone to run, but I think
it makes the test much easier to read and modify later.

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.

I also think it would be nice if hash-object had a "--binary-sha1"
option to avoid the perl grossness. :)

> diff --git a/tree-walk.c b/tree-walk.c
> index ce27842..ba544cf 100644

The code change itself looks brilliant, naturally. :)

-Peff

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

[Jeff King]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 4422 bytes --]

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\x01ŽA\x0e \x10E]sŠ¹€f°@Ä\x18\x17\x1eÁ\v0\x05Z\x12[\x12
> +õú¢é	\ýüÅ{ÿ\x0fižc\x01IòP²÷\x104\x1aÛ)Ó+b&\x13ôÙ]\fê\x10ØR`êœ2Üš\x13¶–)exØ-:xÖ¼ø\f×%mö\x15×ûž§”Ç^[HÕd\x12{-áˆ
> +Q\f¿ÍÒ€\x7fè\x1d‡w,\x13p\x1aë
> +ßçâ\x03&ë?Þ
> \ 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

Re: [PATCH 1/2] tree-walk: be more specific about corrupt tree errors

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Ahh, that explains the breakage I saw.

Thanks.

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

[David Turner]

On Tue, 2016-09-27 at 01:27 -0400, Jeff King wrote:
> > -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.

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.

Thanks for all of the rest of the commentary; I've incorporated it and
will re-roll shortly.

Re: [PATCH 1/2] tree-walk: be more specific about corrupt tree errors

[David Turner]

On Tue, 2016-09-27 at 01:14 -0400, Jeff King wrote:
> 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.

I'm going to do this for this patch, but for the second patch, I don't
think it matters, and it's a bit simpler to avoid it.  I guess it's
probably better to test both ways anyway.

thoughts on error passing, was Re: [PATCH 2/2] fsck: handle bad trees like other errors

[Jeff King]

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

  - 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 context that collects errors in a strbuf (or list, etc)

       struct strbuf err_buf = STRBUF_INIT;
       struct error_context = STRBUF_ERR_CONTEXT(&err_buf);

And that error_context can be passed around like a baton through several
functions.

I remember having a big discussion about error-passing patterns around
the ref refactoring, but I don't remember this particular thing coming
up. 

Anyway, this is all way outside the scope of what we should consider for
your current series. If we were to do something like this, it would make
sense to start using it consistently. This discussion just made me think
of it.

-Peff

Re: thoughts on error passing, was Re: [PATCH 2/2] fsck: handle bad trees like other errors

[David Turner]

On Tue, 2016-09-27 at 15:19 -0400, Jeff King wrote:
> 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

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

Re: thoughts on error passing, was Re: [PATCH 2/2] fsck: handle bad trees like other errors

[Michael Haggerty]

On 09/27/2016 09:19 PM, Jeff King wrote:
> [...]
> 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;
>   }

I like this idea. It's nicely flexible (more so than the `struct strbuf
*err` that is currently used for reference transactions) without being
cumbersome.

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

There could also be a global for chaining to `warn()` or `die()`.

> [...]

Michael

Re: thoughts on error passing, was Re: [PATCH 2/2] fsck: handle bad trees like other errors

[Jeff King]

On Tue, Sep 27, 2016 at 06:57:34PM -0400, David Turner wrote:

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

Yeah, I waffled on that. If you look carefully, you'll note that
the report_error() I showed above would actually require such a
"{ NULL, NULL }" global.

I don't plan to make any patches immediately for this, but I'll let it
percolate and consider whether it makes sense to try out for a future
series.

-Peff

Re: thoughts on error passing, was Re: [PATCH 2/2] fsck: handle bad trees like other errors

[Jeff King]

On Wed, Sep 28, 2016 at 07:01:38AM +0200, Michael Haggerty wrote:

> >   - 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()`.

I played around a little with this. The latter actually makes a lot of
code cleaner, because we can rely on the functions not returning at all.
So for example, you get:

diff --git a/branch.c b/branch.c
index a5a8dcb..53404b8 100644
--- a/branch.c
+++ b/branch.c
@@ -303,17 +303,13 @@ void create_branch(const char *head,
 
 	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).

Cases that call:

  error("%s", err.buf);

can drop the strbuf handling, but of course still need to retain their
conditionals. So they're better, but not as much. I did a half-hearted
conversion of some of the ref code that uses strbufs, and it seems like
it would save a few hundred lines of boilerplate.

There are some cases that are _worse_, because they want to prefix the
error. E.g., in init-db, we have:

  struct strbuf err = STRBUF_INIT;
  ...
  if (refs_init_db(&err))
	die("failed to set up refs db: %s", err.buf);

which is fairly clean. Using an error_context adds slightly to the
boilerplate:

  struct strbuf err_buf = STRBUF_INIT;
  struct error_context err = STRBUF_ERR(&err_buf);
  ...
  if (refs_init_db(&err))
	die("failed to set up refs db: %s", err_buf.buf);

Though if we wanted to get really magical, the err_buf/err pattern could
be its own single-line macro.

You could solve this more generally with something like:

  struct error_prefix_data err;

  error_prefix(&err, &error_die, "failed to set up refs db");
  refs_init_db(&err.err);

where error_prefix() basically sets us up to call back a function which
concatenates the prefix to the real error, then chains to error_die.
But to cover all cases, error_prefix() would actually have to format the
prefix string. Because some callers would be more like:

  error_prefix(&err, &error_print, "unable to frob %s", foo);
  do_frob(foo, &err);

We can't just save the va_list passed to error_prefix(), because it's
not valid after we return. So you have to format the prefix into a
buffer, even though in most cases we won't see an error at all (and
doing it completely correctly would involve using a strbuf, which means
there needs to be a cleanup step; yuck).

-Peff

Re: thoughts on error passing, was Re: [PATCH 2/2] fsck: handle bad trees like other errors

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Yes but... grepping for die() got a lot harder, which may not be a
good thing.

I do like the flexibility such a mechanism offers, but
wrapping/hiding die in it is probably an example that the
flexibility went a bit too far.