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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 4, 2023, 23:45 UTC
Message-ID: <xmqqv8bmlzoi.fsf@gitster.g>
In-Reply-To: <446bce03a96836f35f94e9ef8548cf4a2b041ba8.1696443502.git.gitgitgadget@gmail.com>

[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" <gitgitgadget@gmail.com> writes:

Show 17 quoted lines

> 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.