Re: [PATCH v5 0/2] attr: add attr.tree config

Junio C Hamano [off-list ref] writes:

	if (repo_get_oid_treeish(the_repository,
				 default_attr_source_tree_object_name,
				 attr_source) && !ignore_bad_attr_tree)
		die(_("bad --attr-source or GIT_ATTR_SOURCE"));

OOPS!  Sorry for not noticing earlier, but repo_get_oid_treeish()
does *NOT* error out when the discovered object is not a treeish, as
the suggested object type is merely supplied for disambiguation
purposes (e.g., with objects 012345 that is a tree and 012346 that
is a blob, you can still ask for treeish "01234" but if you ask for
an object "01234" it will fail).

So, the alternative test would have caught this bug, no?  Instead of
silently treating the non-treeish as an empty tree, we would have
died much later when the object supposedly a tree-ish turns out to
be a blob, or something?

There indeed is a bug, but not really.  If we add this test:

test_expect_success 'attr.tree that points at a non-treeish' '
	test_when_finished rm -rf empty &&
	git init empty &&
	(
		cd empty &&
		echo "f/path: test: unspecified" >expect &&
		H=$(git hash-object -t blob --stdin -w </dev/null) &&
		git -c attr.tree=$H check-attr test -- f/path >actual 2>err &&
		test_must_be_empty err &&
		test_cmp expect actual
	)
'

repo_get_oid_treeish() returns a blob object name and we end up
storing a blob object name in "attr_source" static variable of
default_attr_source() function.

Later this is fed to read_attr() by bootstrap_attr_stack() and then
to read_attr_from_blob() that uses it to call get_tree_entry(),
which fails for any path because it is a blob.  We do not give any
errors or error messages during the whole process.

So in a sense, for !!ignore_bad_attr_tree case, the code ends up
doing the right thing.  But if !ignore_bad_attr_tree is true, i.e.,
a blob object name is given via --attr-source or GIT_ATTR_SOURCE,
then the bug will be uncovered.

 t/t0003-attributes.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git c/t/t0003-attributes.sh w/t/t0003-attributes.sh
index ecf43ab545..0f02f22171 100755
--- c/t/t0003-attributes.sh
+++ w/t/t0003-attributes.sh

@@ -394,6 +394,18 @@ test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '
 	test_cmp expect actual
 '
 
+test_expect_success '--attr-source that points at a non-treeish' '
+	test_when_finished rm -rf empty &&
+	git init empty &&
+	(
+		cd empty &&
+		echo "$bad_attr_source_err" >expect_err &&
+		H=$(git hash-object -t blob --stdin -w </dev/null) &&
+		test_must_fail git --attr-source=$H check-attr test -- f/path 2>err &&
+		test_cmp expect_err err
+	)
+'
+
 test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tree' '
 	test_when_finished rm -rf empty &&
 	git init empty &&