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

> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> 44451a2e5e (attr: teach "--attr-source=" global option to "git", 2023-05-06)
>> provided the ability to pass in a treeish as the attr source. When a
>> revision does not resolve to a valid tree is passed, Git will die. At
>> GitLab, we server repositories as bare repos and would like to always read
>> attributes from the default branch, so we'd like to pass in HEAD as the
>> treeish to read gitattributes from on every command. In this context we
>> would not want Git to die if HEAD is unborn, like in the case of empty
>> repositories.
>>
>> Instead of modifying the default behavior of --attr-source, create a new
>> config attr.tree with which an admin can configure a ref for all commands to
>> read gitattributes from. Also make the default tree to read from HEAD on
>> bare repositories.
>>
>> Changes since v2:
>>
>>  * relax the restrictions around attr.tree so that if it does not resolve to
>>    a valid treeish, ignore it.
>>  * add a commit to default to HEAD in bare repositories
>>
>> Changes since v1:
>>
>>  * Added a commit to add attr.tree config
>
> THis is v4 so there must be some changes since v3 that we are missing?

>
>> Range-diff vs v3:
>>
>>  1:  cef206d47c7 ! 1:  eaa27c47810 attr: read attributes from HEAD when bare repo
>>      @@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
>>       +test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '
>>       +	test_when_finished rm -rf test bare_with_gitattribute &&
>>       +	git init test &&
>>      -+	(
>>      -+		cd test &&
>>      -+		test_commit gitattributes .gitattributes "f/path test=val"
>>      -+	) &&
>>      ++	test_commit -C test gitattributes .gitattributes "f/path test=val" &&
>
> OK.
>
>>  2:  dadb822da99 ! 2:  749d8a8082e attr: add attr.tree for setting the treeish to read attributes from
>>      @@ Documentation/config.txt: other popular tools, and describe them in your documen
>>
>>        ## Documentation/config/attr.txt (new) ##
>>       @@
>>      -+attr.tree:
>>      -+	A <tree-ish> to read gitattributes from instead of the worktree. See
>>      -+	linkgit:gitattributes[5]. If `attr.tree` does not resolve to a valid tree,
>>      -+	treat it as an empty tree. --attr-source and GIT_ATTR_SOURCE take
>>      -+	precedence over attr.tree.
>>      ++attr.tree::
>>      ++	A reference to a tree in the repository from which to read attributes,
>>      ++	instead of the `.gitattributes` file in the working tree. In a bare
>>      ++	repository, this defaults to `HEAD:.gitattributes`. If the value does
>>      ++	not resolve to a valid tree object, an empty tree is used instead.
>>      ++	When the `GIT_ATTR_SOURCE` environment variable or `--attr-source`
>>      ++	command line option are used, this configuration variable has no effect.
>
> OK.
>
>>      -+	if (!default_attr_source_tree_object_name) {
>>      ++	if (!default_attr_source_tree_object_name && git_attr_tree) {
>>       +		default_attr_source_tree_object_name = git_attr_tree;
>>       +		ignore_bad_attr_tree = 1;
>>       +	}
>
> Makes sense.
>
>>      @@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
>>
>>       +bad_attr_source_err="fatal: bad --attr-source or GIT_ATTR_SOURCE"
>>       +
>>      ++test_expect_success '--attr-source is bad' '
>>      ++	test_when_finished rm -rf empty &&
>>      ++	git init empty &&
>>      ++	(
>>      ++		cd empty &&
>>      ++		echo "$bad_attr_source_err" >expect_err &&
>>      ++		test_must_fail git --attr-source=HEAD check-attr test -- f/path 2>err &&
>>      ++		test_cmp expect_err err
>>      ++	)
>>      ++'
>
> OK.  We fail when explicitly given a bad attr-source.
>
>>       +test_expect_success 'attr.tree when HEAD is unborn' '
>>       +	test_when_finished rm -rf empty &&
>>       +	git init empty &&
>>       +	(
>>       +		cd empty &&
>>      -+		echo $bad_attr_source_err >expect_err &&
>>       +		echo "f/path: test: unspecified" >expect &&
>>       +		git -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&
>>       +		test_must_be_empty err &&
>
> But we silently ignore when given via a configuration variable.
>
>>      @@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
>>       +	git init empty &&
>>       +	(
>>       +		cd empty &&
>>      -+		echo $bad_attr_source_err >expect_err &&
>>       +		echo "f/path: test: unspecified" >expect &&
>>       +		git -c attr.tree=refs/does/not/exist check-attr test -- f/path >actual 2>err &&
>>       +		test_must_be_empty err &&
>
> Ditto.  Is this any different from the above?  Both points at an
> object that does not exist.  If one were pointing at an object that
> does not exist (e.g., HEAD before the initial commit) and the other
> were pointing at an object that is not a tree-ish (e.g., a blob),
> then having two separate tests may make sense, but otherwise, I am
> not sure about the value proposition of the second test.

>
>>      @@ t/t0003-attributes.sh: test_expect_success 'bare repo defaults to reading .gitat
>>        	test_cmp expect actual
>>        '
>>
>>      -+test_expect_success '--attr-source and GIT_ATTR_SOURCE take precedence over attr.tree' '
>>      ++test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tree' '
>>       +	test_when_finished rm -rf empty &&
>>       +	git init empty &&
>>       +	(
>>      @@ t/t0003-attributes.sh: test_expect_success 'bare repo defaults to reading .gitat
>>       +		test_commit "val2" .gitattributes "f/path test=val2" &&
>>       +		git checkout attr-source &&
>>       +		echo "f/path: test: val1" >expect &&
>>      -+		git -c attr.tree=attr-tree --attr-source=attr-source check-attr test -- f/path >actual &&
>>      ++		GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree --attr-source=attr-source \
>>      ++		check-attr test -- f/path >actual &&
>>       +		test_cmp expect actual &&
>>      -+		GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree check-attr test -- f/path >actual &&
>>      ++		GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree \
>>      ++		check-attr test -- f/path >actual &&
>>       +		test_cmp expect actual
>>       +	)
>>       +'
>
> Looking good.
>
> Thanks.  Queued.