Re: [PATCH v3 2/2] attr: add attr.tree for setting the treeish to read attributes from

> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: John Cai <johncai86@gmail.com>
>>
>> 44451a2 (attr: teach "--attr-source=<tree>" global option to "git",
>> 2023-05-06) provided the ability to pass in a treeish as the attr
>> source. In the context of serving Git repositories as bare repos like we
>> do at GitLab however, it would be easier to point --attr-source to HEAD
>> for all commands by setting it once.
>>
>> Add a new config attr.tree that allows this.
>>
>> Signed-off-by: John Cai <johncai86@gmail.com>
>> ---
>>  Documentation/config.txt      |  2 ++
>>  Documentation/config/attr.txt |  5 +++
>>  attr.c                        |  7 ++++
>>  attr.h                        |  2 ++
>>  config.c                      | 14 ++++++++
>>  t/t0003-attributes.sh         | 62 +++++++++++++++++++++++++++++++++++
>>  6 files changed, 92 insertions(+)
>>  create mode 100644 Documentation/config/attr.txt
>>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 229b63a454c..b1891c2b5af 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -371,6 +371,8 @@ other popular tools, and describe them in your documentation.
>>
>>  include::config/advice.txt[]
>>
>> +include::config/attr.txt[]
>> +
>>  include::config/core.txt[]
>>
>>  include::config/add.txt[]
>> diff --git a/Documentation/config/attr.txt b/Documentation/config/attr.txt
>> new file mode 100644
>> index 00000000000..be882523f8b
>> --- /dev/null
>> +++ b/Documentation/config/attr.txt
>> @@ -0,0 +1,5 @@
>> +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.
>
> Properly typeset `--attr-source` and `GIT_ATTR_SOURCE`.
>
> A quick "git grep" in Documentation/config/*.txt tells me that
> nobody refers to an object type like <tree-ish>.  Imitate what this
> was modeled after, namely Documentation/config/mailmap.txt, which
> says just
>
> 	... a reference to a blob in the repository.
>
> without any half mark-up.

>
> More importantly, the description makes one wonder what the
> precedence rule between these two (the general rule would be for
> command line parameter to override environment, if I recall
> correctly).

>
> I think the enumeration header usually is followed by double-colons
> among Documentation/config/*.txt files.  Let's be consistent.
>
> In the context of this expression, "worktree" is a wrong noun to
> use---the term of art refers to an instance of "working tree",
> together with some "per worktree" administrative files inside .git/
> directory.  On the other hand, "working tree" refers to the "files
> meant to be visible to build tools and editors part of the non-bare
> repository", which is what you want to use here.
>
> attr.tree::
> 	A tree object to read the attributes from, instead of the
> 	`.gitattributes` file in the working tree.  In a bare
> 	repository, this defaults to HEAD:.gitattributes".  If a
> 	given value does not resolve to a valid tree object, an
> 	empty tree is used instead.  When `GIT_ATTR_SOURCE`
> 	environment variable or `--attr-source` command line option
> 	is used, this configuration variable has no effect.
>
> or something along that line, perhaps.

>
>> diff --git a/attr.c b/attr.c
>> index bf2ea1626a6..0ae6852d12b 100644
>> --- a/attr.c
>> +++ b/attr.c
>> @@ -24,6 +24,8 @@
>>  #include "tree-walk.h"
>>  #include "object-name.h"
>>
>> +const char *git_attr_tree;
>> +
>>  const char git_attr__true[] = "(builtin)true";
>>  const char git_attr__false[] = "\0(builtin)false";
>>  static const char git_attr__unknown[] = "(builtin)unknown";
>> @@ -1206,6 +1208,11 @@ static void compute_default_attr_source(struct object_id *attr_source)
>>  	if (!default_attr_source_tree_object_name)
>>  		default_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);
>>
>> +	if (!default_attr_source_tree_object_name) {
>> +		default_attr_source_tree_object_name = git_attr_tree;
>> +		ignore_bad_attr_tree = 1;
>> +	}
>
> As long as "attr.tree" was read by calling git_default_attr_config()
> before we come here, git_attr_tree is not NULL and we allow bad attr
> tree in default_attr_source_tree_object_name.  But stepping back a
> bit, even if "attr.tree" is unspecified, i.e., git_attr_tree is
> NULL, we set ignore_bad_attr_tree to true here.
>
> What it means is that after the above if() statement, if
> default_attr_source_tree_object_name is still NULL, we know that
> ignore_bad_attr_tree is already set to true.
>
>>  	if (!default_attr_source_tree_object_name &&
>>  	    startup_info->have_repository &&
>>  	    is_bare_repository()) {
>
> So would it make more sense to remove the assignment to the same
> variable we made in [1/2] around here (not seen in the post
> context)?
>
> Alternatively, even though it makes the code a bit more verbose, the
> logic might become clearer if you wrote the "assign from the config"
> part like so:
>
> 	if (!default_attr_source_tree_object_name && git_attr_tree) {
>         	default_attr_source_tree_object_name = git_attr_tree;
> 		ignore_bad_attr_tree = 1;
> 	}
>
> It would leave more flexibility to the code around here.  You could
> for example add code that assigns a different value, a tree object
> that is required to exist, to default_attr_source_tree_object_name
> after this point, for example, without having to wonder what the
> "current" value of ignore_bad_attr_tree is.

>
>> +static int git_default_attr_config(const char *var, const char *value)
>> +{
>> +	if (!strcmp(var, "attr.tree"))
>> +		return git_config_string(&git_attr_tree, var, value);
>> +
>> +	/* Add other attribute related config variables here and to
>> +	   Documentation/config/attr.txt. */
>
> 	/*
> 	 * Our multi-line comments should look
>          * more like this; opening slash-asterisk
> 	 * and closing asterisk-slash sit on a line
> 	 * on its own.
> 	 */
>
>> @@ -342,6 +346,46 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' '
>>  	)
>>  '
>>
>> +bad_attr_source_err="fatal: bad --attr-source or GIT_ATTR_SOURCE"
>
> Not a fault of this two-patch series, but we probably should refine
> this error reporting so that the reader can tell which one is being
> complained about, and optionally what the offending value was.
>
>
>> +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 &&
>
> Let's not rely on words in the error message split with exactly one
> whitespace each and instead quote the variable properly.  I.e.,
>
> 		echo "$bad_attr_source_err" >expect_err &&
>
> But this is not even used, as we do not expect it to fail.  Perhaps
> remove it altogether?
>
>> +		echo "f/path: test: unspecified" >expect &&
>> +		git -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&
>> +		test_must_be_empty err &&
>> +		test_cmp expect actual
>> +	)
>> +'
>> +
>> +test_expect_success 'attr.tree points to non-existing ref' '
>> +	test_when_finished rm -rf empty &&
>> +	git init empty &&
>> +	(
>> +		cd empty &&
>> +		echo $bad_attr_source_err >expect_err &&
>
> Ditto.
>
>> +		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 &&
>> +		test_cmp expect actual
>> +	)
>> +'
>> +
>> +test_expect_success 'bad attr source defaults to reading .gitattributes file' '
>> +	test_when_finished rm -rf empty &&
>> +	git init empty &&
>> +	(
>> +		cd empty &&
>> +		echo "f/path test=val" >.gitattributes &&
>> +		echo "f/path: test: val" >expect &&
>> +		git -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&
>> +		test_must_be_empty err &&
>> +		test_cmp expect actual
>> +	)
>> +'
>
> In other words, with the additional tests, we do not check error
> cases (which may be perfectly OK, if they are covered by existing
> tests).  A bit curious.

>
>> @@ -356,6 +400,24 @@ test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '
>>  	test_cmp expect actual
>>  '
>>
>> +test_expect_success '--attr-source and GIT_ATTR_SOURCE take precedence over attr.tree' '
>
> Do we want to ensure which one takes precedence between the command
> line option and the environment?  It's not like the one that is
> given the last takes effect.

>
>> +	test_when_finished rm -rf empty &&
>> +	git init empty &&
>> +	(
>> +		cd empty &&
>> +		git checkout -b attr-source &&
>> +		test_commit "val1" .gitattributes "f/path test=val1" &&
>> +		git checkout -b attr-tree &&
>> +		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 &&
>> +		test_cmp expect actual &&
>> +		GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree check-attr test -- f/path >actual &&
>> +		test_cmp expect actual
>> +	)
>> +'
>> +
>>  test_expect_success 'bare repository: with --source' '
>>  	(
>>  		cd bare.git &&
>
> Other than that, looking great.  Thanks.