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

[Junio C Hamano <gitster@pobox.com>]

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