Hi Junio,
On 11 Oct 2023, at 18:09, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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?
Oops I messed up the ordering of changes here. I'll fix in the (hopefully) final
re-roll
>
>> 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.
Yeah looking at it now, this test seems like it doesn't add much.
>
>> @@ 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.
thanks!
John
