[03/11] attr: don't recompute default attribute source

Commit Message

Patrick Steinhardt April 19, 2024, 9:51 a.m. UTC

The `default_attr_source()` function lazily computes the attr source
supposedly once, only. This is done via a static variable `attr_source`
that contains the resolved object ID of the attr source's tree. If the
variable is the null object ID then we try to look up the attr source,
otherwise we skip over it.

This has approach is flawed though: the variable will never be set to
anything else but the null object ID in case there is no attr source.
Consequently, we re-compute the information on every call. And in the
worst case, when we silently ignore bad trees, this will cause us to try
and look up the treeish every single time.

Improve this by introducing a separate variable `has_attr_source` to
track whether we already computed the attr source and, if so, whether we
have an attr source or not.

This also allows us to convert the `ignore_bad_attr_tree` to not be
static anymore as the code will only be executed once anyway.

Signed-off-by: Patrick Steinhardt <[e-mail]>
---
 attr.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

Comments

Justin Tobler April 23, 2024, 12:32 a.m. UTC | #1

On 24/04/19 11:51AM, Patrick Steinhardt wrote:
> The `default_attr_source()` function lazily computes the attr source
> supposedly once, only. This is done via a static variable `attr_source`
> that contains the resolved object ID of the attr source's tree. If the
> variable is the null object ID then we try to look up the attr source,
> otherwise we skip over it.
> 
> This has approach is flawed though: the variable will never be set to

Remove "has"

-Justin

> anything else but the null object ID in case there is no attr source.
> Consequently, we re-compute the information on every call. And in the
> worst case, when we silently ignore bad trees, this will cause us to try
> and look up the treeish every single time.
> 
> Improve this by introducing a separate variable `has_attr_source` to
> track whether we already computed the attr source and, if so, whether we
> have an attr source or not.
> 
> This also allows us to convert the `ignore_bad_attr_tree` to not be
> static anymore as the code will only be executed once anyway.
> 
> Signed-off-by: Patrick Steinhardt <[e-mail]>
...

Patch

diff --git a/attr.c b/attr.c
index 679e42258c..9d911aeb31 100644
--- a/attr.c
+++ b/attr.c
@@ -1206,15 +1206,16 @@  static void collect_some_attrs(struct index_state *istate,
 }
 
 static const char *default_attr_source_tree_object_name;
-static int ignore_bad_attr_tree;
 
 void set_git_attr_source(const char *tree_object_name)
 {
 	default_attr_source_tree_object_name = xstrdup(tree_object_name);
 }
 
-static void compute_default_attr_source(struct object_id *attr_source)
+static int compute_default_attr_source(struct object_id *attr_source)
 {
+	int ignore_bad_attr_tree = 0;
+
 	if (!default_attr_source_tree_object_name)
 		default_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);
 
@@ -1230,22 +1231,28 @@  static void compute_default_attr_source(struct object_id *attr_source)
 		ignore_bad_attr_tree = 1;
 	}
 
-	if (!default_attr_source_tree_object_name || !is_null_oid(attr_source))
-		return;
+	if (!default_attr_source_tree_object_name)
+		return 0;
 
 	if (repo_get_oid_treeish(the_repository,
 				 default_attr_source_tree_object_name,
-				 attr_source) && !ignore_bad_attr_tree)
-		die(_("bad --attr-source or GIT_ATTR_SOURCE"));
+				 attr_source)) {
+		if (!ignore_bad_attr_tree)
+			die(_("bad --attr-source or GIT_ATTR_SOURCE"));
+		return 0;
+	}
+
+	return 1;
 }
 
 static struct object_id *default_attr_source(void)
 {
 	static struct object_id attr_source;
+	static int has_attr_source = -1;
 
-	if (is_null_oid(&attr_source))
-		compute_default_attr_source(&attr_source);
-	if (is_null_oid(&attr_source))
+	if (has_attr_source < 0)
+		has_attr_source = compute_default_attr_source(&attr_source);
+	if (!has_attr_source)
 		return NULL;
 	return &attr_source;
 }