"John Cai via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> From: John Cai <johncai86@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> ---
> 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.
