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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 13, 2023, 20:31 UTC
Message-ID: <xmqqa5smz2lk.fsf@gitster.g>
In-Reply-To: <xmqqmswmz76w.fsf@gitster.g>

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

Show 16 quoted lines

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