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

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

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