Hi Junio,
On 10 Oct 2023, at 18:14, Junio C Hamano wrote:
> "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.
sounds good
>
> 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).
yes, that should be the case.
>
> 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.
sounds good to me, thanks
>
>> 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.
good point--I think this logic is easier to follow.
>
>> +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.
Just checked and it does seem like we don't have a test case that covers the
error case. Will add in the next version.
>
>> @@ -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.
yeah, might as well.
>
>> + 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.
thanks!
John
