| Message ID | dadb822da99772cd277417f564cf672f65d1cc24.1696967380.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | attr: add attr.tree config | expand |
"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.
Hi Junio, On 10 Oct 2023, at 18:14, Junio C Hamano wrote: > "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. 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
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. 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; + } + if (!default_attr_source_tree_object_name && startup_info->have_repository && is_bare_repository()) { diff --git a/attr.h b/attr.h index 2b745df4054..127998ae013 100644 --- a/attr.h +++ b/attr.h @@ -236,4 +236,6 @@ const char *git_attr_global_file(void); /* Return whether the system gitattributes file is enabled and should be used. */ int git_attr_system_is_enabled(void); +extern const char *git_attr_tree; + #endif /* ATTR_H */ diff --git a/config.c b/config.c index 3846a37be97..21a1590b505 100644 --- a/config.c +++ b/config.c @@ -18,6 +18,7 @@ #include "repository.h" #include "lockfile.h" #include "mailmap.h" +#include "attr.h" #include "exec-cmd.h" #include "strbuf.h" #include "quote.h" @@ -1904,6 +1905,16 @@ static int git_default_mailmap_config(const char *var, const char *value) return 0; } +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. */ + return 0; +} + int git_default_config(const char *var, const char *value, const struct config_context *ctx, void *cb) { @@ -1927,6 +1938,9 @@ int git_default_config(const char *var, const char *value, if (starts_with(var, "mailmap.")) return git_default_mailmap_config(var, value); + if (starts_with(var, "attr.")) + return git_default_attr_config(var, value); + if (starts_with(var, "advice.") || starts_with(var, "color.advice")) return git_default_advice_config(var, value); diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh index e6b1a117228..b0949125f26 100755 --- a/t/t0003-attributes.sh +++ b/t/t0003-attributes.sh @@ -40,6 +40,10 @@ attr_check_source () { test_cmp expect actual && test_must_be_empty err + git $git_opts -c "attr.tree=$source" check-attr test -- "$path" >actual 2>err && + test_cmp expect actual && + test_must_be_empty err + GIT_ATTR_SOURCE="$source" git $git_opts check-attr test -- "$path" >actual 2>err && test_cmp expect actual && test_must_be_empty err @@ -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" + +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 && + 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 && + 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 + ) +' test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' ' test_when_finished rm -rf test bare_with_gitattribute && @@ -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' ' + 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 &&