[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@xxxxxxxxx> 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.
