| Message ID | pull.1577.v4.git.git.1697044422.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | attr: add attr.tree config | expand |
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> 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? > 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. > @@ 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.
Hi Junio, On 11 Oct 2023, at 18:09, Junio C Hamano wrote: > "John Cai via GitGitGadget" <gitgitgadget@gmail.com> 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
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 John Cai (2): attr: read attributes from HEAD when bare repo attr: add attr.tree for setting the treeish to read attributes from Documentation/config.txt | 2 + Documentation/config/attr.txt | 7 +++ attr.c | 19 +++++++- attr.h | 2 + config.c | 16 +++++++ t/t0003-attributes.sh | 84 +++++++++++++++++++++++++++++++++++ t/t5001-archive-attr.sh | 2 +- 7 files changed, 130 insertions(+), 2 deletions(-) create mode 100644 Documentation/config/attr.txt base-commit: 1fc548b2d6a3596f3e1c1f8b1930d8dbd1e30bf3 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1577%2Fjohn-cai%2Fjc%2Fconfig-attr-invalid-source-v4 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1577/john-cai/jc/config-attr-invalid-source-v4 Pull-Request: https://github.com/git/git/pull/1577 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" && + git clone --bare test bare_with_gitattribute && + echo "f/path: test: val" >expect && + git -C bare_with_gitattribute check-attr test -- f/path >actual && 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. ## attr.c ## @@ @@ attr.c: 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) { ++ if (!default_attr_source_tree_object_name && git_attr_tree) { + default_attr_source_tree_object_name = git_attr_tree; + ignore_bad_attr_tree = 1; + } @@ config.c: static int git_default_mailmap_config(const char *var, const char *val + 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. */ ++ /* ++ * Add other attribute related config variables here and to ++ * Documentation/config/attr.txt. ++ */ + return 0; +} + @@ 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 ++ ) ++' ++ +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 && @@ 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 && @@ 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 + ) +'