[v2,1/2] attr: add attr.tree for setting the treeish to read attributes from

Commit Message

John Cai Oct. 4, 2023, 6:18 p.m. UTC

From: John Cai <[e-mail]>

44451a2e5e (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 <[e-mail]>
---
 Documentation/config.txt      | 2 ++
 Documentation/config/attr.txt | 5 +++++
 attr.c                        | 7 +++++++
 t/t0003-attributes.sh         | 4 ++++
 4 files changed, 18 insertions(+)
 create mode 100644 Documentation/config/attr.txt

Comments

Junio C Hamano Oct. 4, 2023, 7:58 p.m. UTC | #1

"John Cai via GitGitGadget" <[e-mail]> writes:

> From: John Cai <[e-mail]>
>
> 44451a2e5e (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.

Hmph, I wonder if we want to go all the way to emulate how the
mailmap.blob was done, including

 - Default the value of attr.tree to HEAD in a bare repository;

 - Notice but ignore errors if the attr.tree does not point at a
   tree object, and pretend as if attr.tree specified an empty tree;

which does not seem to be in this patch.  With such a change,
probably we do not even need [2/2] of the series, perhaps?

Junio C Hamano Oct. 4, 2023, 11:45 p.m. UTC | #2

[jc: JTan CC'ed as he seems to have took over the polishing of
b1bda751 (parse: separate out parsing functions from config.h,
2023-09-29)]

"John Cai via GitGitGadget" <[e-mail]> writes:

> diff --git a/attr.c b/attr.c
> index 71c84fbcf86..bb0d54eb967 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -1205,6 +1205,13 @@ 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) {
> +		char *attr_tree;
> +
> +		if (!git_config_get_string("attr.tree", &attr_tree))
> +			default_attr_source_tree_object_name = attr_tree;
> +	}
> +
>  	if (!default_attr_source_tree_object_name || !is_null_oid(attr_source))
>  		return;

As this adds a new call to git_config_get_string(), which will only
be available by including <config.h>, a merge-fix into 'seen' of
this topic needs to revert what b1bda751 (parse: separate out
parsing functions from config.h, 2023-09-29) did, which made this
file include only <parse.h>.

As this configuration variable was invented to improve the way the
attribute source tree is supported by emulating how mailmap.blob is
done, it deserves a bit of comparison.

The way mailmap.c does this is not have any code that reads or
parses configuration in mailmap.c (which is a rather library-ish
place), and leaves it up to callers to pre-populate the global
variable git_mailmap_blob with config.c:git_default_config().  That
way, they do not need to include <config.h> (nor <parse.h>) that is
closer to the UI layer.  I am wondering why we are not doing the
same, and instead making an ad-hoc call to git_config_get_string()
in this code, and if it is a good direction to move the codebase to
(in which case we may want to make sure that the same pattern is
followed in other places).

Folks interested in libification, as to the direction of that
effort, what's your plan on where to draw a line between "library"
and "userland"?  Should library-ish code be allowed to call
git_config_anything()?  I somehow suspect that it might be cleaner
if they didn't, and instead have the user of the "attr" module to
supply the necessary values from outside.

On the other hand, once the part we have historically called
"config" API gets a reasonably solid abstraction so that they become
pluggable and replaceable, random ad-hoc calls from library code
outside the "config" library code may not be a huge problem, as long
as we plumb the necessary object handles around (so "attr" library
would need to be told which "config" backend is in use, probably in
the form of a struct that holds the various states in to replace
the current use of globals, plus a vtable to point at
implementations of the "config" service, and git_config_get_string()
call in such a truly libified world would grab the value of the named
variable transparently from whichever "config" backend is currently
in use).

Anyway, I think I wiggled this patch into 'seen' so I'll push out
today's integration result shortly.

Jeff King Oct. 5, 2023, 5:07 p.m. UTC | #3

On Wed, Oct 04, 2023 at 12:58:43PM -0700, Junio C Hamano wrote:

> "John Cai via GitGitGadget" <[e-mail]> writes:
> 
> > From: John Cai <[e-mail]>
> >
> > 44451a2e5e (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.
> 
> Hmph, I wonder if we want to go all the way to emulate how the
> mailmap.blob was done, including
> 
>  - Default the value of attr.tree to HEAD in a bare repository;
> 
>  - Notice but ignore errors if the attr.tree does not point at a
>    tree object, and pretend as if attr.tree specified an empty tree;
> 
> which does not seem to be in this patch.  With such a change,
> probably we do not even need [2/2] of the series, perhaps?

Oh good, this was exactly what I was going to write in a review, so now
I don't have to. :)

Even though it creates behavior differences between attr.tree and
--attr-source, I think that is justifiable. Config options apply across
a wider set of contexts, so loosening the error handling may make sense
where it would not for a command-line option. But we should document
both that, and also how the two interact (I assume "git --attr-source"
would override attr.tree completely).

-Peff

John Cai Oct. 5, 2023, 7:46 p.m. UTC | #4

Hi Peff,

On 5 Oct 2023, at 13:07, Jeff King wrote:

> On Wed, Oct 04, 2023 at 12:58:43PM -0700, Junio C Hamano wrote:
>
>> "John Cai via GitGitGadget" <[e-mail]> writes:
>>
>>> From: John Cai <[e-mail]>
>>>
>>> 44451a2e5e (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.
>>
>> Hmph, I wonder if we want to go all the way to emulate how the
>> mailmap.blob was done, including
>>
>>  - Default the value of attr.tree to HEAD in a bare repository;
>>
>>  - Notice but ignore errors if the attr.tree does not point at a
>>    tree object, and pretend as if attr.tree specified an empty tree;
>>
>> which does not seem to be in this patch.  With such a change,
>> probably we do not even need [2/2] of the series, perhaps?
>
> Oh good, this was exactly what I was going to write in a review, so now
> I don't have to. :)
>
> Even though it creates behavior differences between attr.tree and
> --attr-source, I think that is justifiable. Config options apply across
> a wider set of contexts, so loosening the error handling may make sense
> where it would not for a command-line option.

Makes sense. Will adjust in the next version.

> But we should document both that, and also how the two interact (I assume
> "git --attr-source" would override attr.tree completely).

Yes, --attr-source would take precedence over attr.tree

>
> -Peff

thanks!
John

Jonathan Tan Oct. 6, 2023, 5:20 p.m. UTC | #5

Junio C Hamano <[e-mail]> writes:
> As this adds a new call to git_config_get_string(), which will only
> be available by including <config.h>, a merge-fix into 'seen' of
> this topic needs to revert what b1bda751 (parse: separate out
> parsing functions from config.h, 2023-09-29) did, which made this
> file include only <parse.h>.
> 
> As this configuration variable was invented to improve the way the
> attribute source tree is supported by emulating how mailmap.blob is
> done, it deserves a bit of comparison.
> 
> The way mailmap.c does this is not have any code that reads or
> parses configuration in mailmap.c (which is a rather library-ish
> place), and leaves it up to callers to pre-populate the global
> variable git_mailmap_blob with config.c:git_default_config().  That
> way, they do not need to include <config.h> (nor <parse.h>) that is
> closer to the UI layer.  I am wondering why we are not doing the
> same, and instead making an ad-hoc call to git_config_get_string()
> in this code, and if it is a good direction to move the codebase to
> (in which case we may want to make sure that the same pattern is
> followed in other places).
> 
> Folks interested in libification, as to the direction of that
> effort, what's your plan on where to draw a line between "library"
> and "userland"?  Should library-ish code be allowed to call
> git_config_anything()?  I somehow suspect that it might be cleaner
> if they didn't, and instead have the user of the "attr" module to
> supply the necessary values from outside.

I think that ideally library-ish code shouldn't be allowed to call
config, yes. However I think what's practical would be for libraries
that use very few config variables to get the necessary values from
outside, and libraries that use many config variables (e.g. fetch, if it
becomes a library) to call config.

> On the other hand, once the part we have historically called
> "config" API gets a reasonably solid abstraction so that they become
> pluggable and replaceable, random ad-hoc calls from library code
> outside the "config" library code may not be a huge problem, as long
> as we plumb the necessary object handles around (so "attr" library
> would need to be told which "config" backend is in use, probably in
> the form of a struct that holds the various states in to replace
> the current use of globals, plus a vtable to point at
> implementations of the "config" service, and git_config_get_string()
> call in such a truly libified world would grab the value of the named
> variable transparently from whichever "config" backend is currently
> in use).

This is true, but if we were ever to use the attr library elsewhere
(whether in the git.git repo itself to unit test this library, or
in another software project), we would need to supply a mock/stub of
config. If attr uses very few config variables, I think it's clearer if
it takes in the information from outside.

Patch

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..e4f2122b7ab
--- /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]. This is equivalent to setting the
+	`GIT_ATTR_SOURCE` environment variable, or passing in --attr-source to
+	the Git command.
diff --git a/attr.c b/attr.c
index 71c84fbcf86..bb0d54eb967 100644
--- a/attr.c
+++ b/attr.c
@@ -1205,6 +1205,13 @@  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) {
+		char *attr_tree;
+
+		if (!git_config_get_string("attr.tree", &attr_tree))
+			default_attr_source_tree_object_name = attr_tree;
+	}
+
 	if (!default_attr_source_tree_object_name || !is_null_oid(attr_source))
 		return;
 
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 26e082f05b4..6342187c751 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