diff mbox series

[v2,05/10] log-tree: use ref_namespaces instead of if/else-if

Message ID 53b15a0b7932f892505d07a509909b62c473037e.1659122979.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series log: create tighter default decoration filter | expand

Commit Message

Derrick Stolee July 29, 2022, 7:29 p.m. UTC 
From: Derrick Stolee <derrickstolee@github.com>

The add_ref_decoration() method uses an if/else-if chain to determine if
a ref matches a known ref namespace that has a special decoration
category. That decoration type is later used to assign a color when
writing to stdout.

The newly-added ref_namespaces array contains all namespaces, along
with information about their decoration type. Check this array instead
of this if/else-if chain. This reduces our dependency on string literals
being embedded in the decoration logic.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 log-tree.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

Comments

Junio C Hamano Aug. 3, 2022, 6:31 a.m. UTC | #1 
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <derrickstolee@github.com>
>
> The add_ref_decoration() method uses an if/else-if chain to determine if
> a ref matches a known ref namespace that has a special decoration
> category. That decoration type is later used to assign a color when
> writing to stdout.
>
> The newly-added ref_namespaces array contains all namespaces, along
> with information about their decoration type. Check this array instead
> of this if/else-if chain. This reduces our dependency on string literals
> being embedded in the decoration logic.
>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
>  log-tree.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/log-tree.c b/log-tree.c
> index bb80f1a94d2..6075bdd334e 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -137,6 +137,7 @@ static int ref_filter_match(const char *refname,
>  static int add_ref_decoration(const char *refname, const struct object_id *oid,
>  			      int flags, void *cb_data)
>  {
> +	int i;
>  	struct object *obj;
>  	enum object_type objtype;
>  	enum decoration_type deco_type = DECORATION_NONE;
> @@ -167,16 +168,21 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,
>  		return 0;
>  	obj = lookup_object_by_type(the_repository, oid, objtype);
>  
> -	if (starts_with(refname, "refs/heads/"))
> -		deco_type = DECORATION_REF_LOCAL;
> -	else if (starts_with(refname, "refs/remotes/"))
> -		deco_type = DECORATION_REF_REMOTE;
> -	else if (starts_with(refname, "refs/tags/"))
> -		deco_type = DECORATION_REF_TAG;
> -	else if (!strcmp(refname, "refs/stash"))
> -		deco_type = DECORATION_REF_STASH;
> -	else if (!strcmp(refname, "HEAD"))
> -		deco_type = DECORATION_REF_HEAD;
> +	for (i = 0; i < NAMESPACE__COUNT; i++) {
> +		struct ref_namespace_info *info = &ref_namespaces[i];
> +
> +		if (!info->decoration)
> +			continue;
> +		if (info->exact) {
> +			if (!strcmp(refname, info->ref)) {
> +				deco_type = info->decoration;
> +				break;
> +			}
> +		} else if (starts_with(refname, info->ref)) {
> +			deco_type = info->decoration;
> +			break;
> +		}
> +	}

Very nice.  The double-dash in the NAMESPACE__COUNT constant somehow
looks strange.  As we scan through ref_namespace[] array densely,

	for (i = 0; i < ARRAY_SIZE(ref_namespace); i++)
		...

without having to use the constant would probably be more in line
with the way how the rest of the codebase works.
Derrick Stolee Aug. 4, 2022, 1:31 p.m. UTC | #2 
On 8/3/2022 2:31 AM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Derrick Stolee <derrickstolee@github.com>
>> +	for (i = 0; i < NAMESPACE__COUNT; i++) {
>> +		struct ref_namespace_info *info = &ref_namespaces[i];
>> +
>> +		if (!info->decoration)
>> +			continue;
>> +		if (info->exact) {
>> +			if (!strcmp(refname, info->ref)) {
>> +				deco_type = info->decoration;
>> +				break;
>> +			}
>> +		} else if (starts_with(refname, info->ref)) {
>> +			deco_type = info->decoration;
>> +			break;
>> +		}
>> +	}
> 
> Very nice.  The double-dash in the NAMESPACE__COUNT constant somehow
> looks strange.  As we scan through ref_namespace[] array densely,
> 
> 	for (i = 0; i < ARRAY_SIZE(ref_namespace); i++)
> 		...
> 
> without having to use the constant would probably be more in line
> with the way how the rest of the codebase works.

Ah, I did not know about that trick. Thanks!

-Stolee
Derrick Stolee Aug. 4, 2022, 2:34 p.m. UTC | #3 
On 8/4/2022 9:31 AM, Derrick Stolee wrote:
> On 8/3/2022 2:31 AM, Junio C Hamano wrote:
>> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> From: Derrick Stolee <derrickstolee@github.com>
>>> +	for (i = 0; i < NAMESPACE__COUNT; i++) {
>>> +		struct ref_namespace_info *info = &ref_namespaces[i];
>>> +
>>> +		if (!info->decoration)
>>> +			continue;
>>> +		if (info->exact) {
>>> +			if (!strcmp(refname, info->ref)) {
>>> +				deco_type = info->decoration;
>>> +				break;
>>> +			}
>>> +		} else if (starts_with(refname, info->ref)) {
>>> +			deco_type = info->decoration;
>>> +			break;
>>> +		}
>>> +	}
>>
>> Very nice.  The double-dash in the NAMESPACE__COUNT constant somehow
>> looks strange.  As we scan through ref_namespace[] array densely,
>>
>> 	for (i = 0; i < ARRAY_SIZE(ref_namespace); i++)
>> 		...
>>
>> without having to use the constant would probably be more in line
>> with the way how the rest of the codebase works.
> 
> Ah, I did not know about that trick. Thanks!

...except that it doesn't work because the array is declared as
'extern' so we don't know its size outside of refs.c.

This motivates the use of NAMESPACE__COUNT (and the double
underscores differentiates the "COUNT" from other namespaces, so
there is no confusion about "COUNT" being a namespace). If there
is another way around this, then I would love to hear it!

Thanks,
-Stolee
Derrick Stolee Aug. 4, 2022, 2:50 p.m. UTC | #4 
On 8/4/2022 10:34 AM, Derrick Stolee wrote:
> On 8/4/2022 9:31 AM, Derrick Stolee wrote:
>> On 8/3/2022 2:31 AM, Junio C Hamano wrote:

>>> Very nice.  The double-dash in the NAMESPACE__COUNT constant somehow
>>> looks strange.  As we scan through ref_namespace[] array densely,
>>>
>>> 	for (i = 0; i < ARRAY_SIZE(ref_namespace); i++)
>>> 		...
>>>
>>> without having to use the constant would probably be more in line
>>> with the way how the rest of the codebase works.
>>
>> Ah, I did not know about that trick. Thanks!
> 
> ...except that it doesn't work because the array is declared as
> 'extern' so we don't know its size outside of refs.c.
> 
> This motivates the use of NAMESPACE__COUNT (and the double
> underscores differentiates the "COUNT" from other namespaces, so
> there is no confusion about "COUNT" being a namespace). If there
> is another way around this, then I would love to hear it!

My current workaround is to define the size of the array in the
header file:

---

enum ref_namespace {
	NAMESPACE_HEAD,
	NAMESPACE_BRANCHES,
	NAMESPACE_TAGS,
	NAMESPACE_REMOTE_REFS,
	NAMESPACE_STASH,
	NAMESPACE_REPLACE,
	NAMESPACE_NOTES,
	NAMESPACE_PREFETCH,
	NAMESPACE_REWRITTEN,

	/* Must be last */
	NAMESPACE__COUNT
};

/* See refs.c for the contents of this array. */
extern struct ref_namespace_info ref_namespaces[NAMESPACE__COUNT];

---

Then ARRAY_SIZE(ref_namespaces) works properly.

Thanks,
-Stolee
Junio C Hamano Aug. 4, 2022, 5:40 p.m. UTC | #5 
Derrick Stolee <derrickstolee@github.com> writes:

> My current workaround is to define the size of the array in the
> header file:
>
> ---
>
> enum ref_namespace {
> 	NAMESPACE_HEAD,
> 	NAMESPACE_BRANCHES,
> 	NAMESPACE_TAGS,
> 	NAMESPACE_REMOTE_REFS,
> 	NAMESPACE_STASH,
> 	NAMESPACE_REPLACE,
> 	NAMESPACE_NOTES,
> 	NAMESPACE_PREFETCH,
> 	NAMESPACE_REWRITTEN,
>
> 	/* Must be last */
> 	NAMESPACE__COUNT
> };
>
> /* See refs.c for the contents of this array. */
> extern struct ref_namespace_info ref_namespaces[NAMESPACE__COUNT];

Because there is no reason why we want to keep the size of the array
opaque in this particular case, it may even be preferrable over the
original ref_namespace[] array of unspecified size.  Nice.

BTW, I prefer to name my arrays with a singular noun, when the
predominant use of it in the code that is an API customer is to name
an individual element and use it.

In this case, many API users do things like

   ref_namespace[NAMESPACE_NOTES}.ref = ...;

so a singular name would be more appropriate.  thing[4] that means
the fourth thing feels more intuitive than things[4], at least to
me.

On the other hand, when API customers mostly pass the whole thing
around as an almost opaque collection to the API function, then a
plural name is more appropriate.  The reason why I focus on API
customers is because API implementation of course has to go in to
each individual element of the array and work on it at some level,
and "individual access means singular name" rule would become non
workable.  The names are mostly to help API customers, so if an
array is perceived by them as mostly a collection of things, then
naming it in plural would make it more natural.
Derrick Stolee Aug. 4, 2022, 8:17 p.m. UTC | #6 
On 8/4/2022 1:40 PM, Junio C Hamano wrote:
> Derrick Stolee <derrickstolee@github.com> writes:
> 
>> My current workaround is to define the size of the array in the
>> header file:
>>
>> ---
>>
>> enum ref_namespace {
>> 	NAMESPACE_HEAD,
>> 	NAMESPACE_BRANCHES,
>> 	NAMESPACE_TAGS,
>> 	NAMESPACE_REMOTE_REFS,
>> 	NAMESPACE_STASH,
>> 	NAMESPACE_REPLACE,
>> 	NAMESPACE_NOTES,
>> 	NAMESPACE_PREFETCH,
>> 	NAMESPACE_REWRITTEN,
>>
>> 	/* Must be last */
>> 	NAMESPACE__COUNT
>> };
>>
>> /* See refs.c for the contents of this array. */
>> extern struct ref_namespace_info ref_namespaces[NAMESPACE__COUNT];
> 
> Because there is no reason why we want to keep the size of the array
> opaque in this particular case, it may even be preferrable over the
> original ref_namespace[] array of unspecified size.  Nice.
> 
> BTW, I prefer to name my arrays with a singular noun, when the
> predominant use of it in the code that is an API customer is to name
> an individual element and use it.
> 
> In this case, many API users do things like
> 
>    ref_namespace[NAMESPACE_NOTES}.ref = ...;
> 
> so a singular name would be more appropriate.  thing[4] that means
> the fourth thing feels more intuitive than things[4], at least to
> me.

This makes sense to me.

> On the other hand, when API customers mostly pass the whole thing
> around as an almost opaque collection to the API function, then a
> plural name is more appropriate.  The reason why I focus on API
> customers is because API implementation of course has to go in to
> each individual element of the array and work on it at some level,
> and "individual access means singular name" rule would become non
> workable.  The names are mostly to help API customers, so if an
> array is perceived by them as mostly a collection of things, then
> naming it in plural would make it more natural.

Understood. I think this array does not seem to fit this second
case, so I will change it to be singular.

Thanks,
-Stolee
diff mbox series

Patch

diff --git a/log-tree.c b/log-tree.c
index bb80f1a94d2..6075bdd334e 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -137,6 +137,7 @@  static int ref_filter_match(const char *refname,
 static int add_ref_decoration(const char *refname, const struct object_id *oid,
 			      int flags, void *cb_data)
 {
+	int i;
 	struct object *obj;
 	enum object_type objtype;
 	enum decoration_type deco_type = DECORATION_NONE;
@@ -167,16 +168,21 @@  static int add_ref_decoration(const char *refname, const struct object_id *oid,
 		return 0;
 	obj = lookup_object_by_type(the_repository, oid, objtype);
 
-	if (starts_with(refname, "refs/heads/"))
-		deco_type = DECORATION_REF_LOCAL;
-	else if (starts_with(refname, "refs/remotes/"))
-		deco_type = DECORATION_REF_REMOTE;
-	else if (starts_with(refname, "refs/tags/"))
-		deco_type = DECORATION_REF_TAG;
-	else if (!strcmp(refname, "refs/stash"))
-		deco_type = DECORATION_REF_STASH;
-	else if (!strcmp(refname, "HEAD"))
-		deco_type = DECORATION_REF_HEAD;
+	for (i = 0; i < NAMESPACE__COUNT; i++) {
+		struct ref_namespace_info *info = &ref_namespaces[i];
+
+		if (!info->decoration)
+			continue;
+		if (info->exact) {
+			if (!strcmp(refname, info->ref)) {
+				deco_type = info->decoration;
+				break;
+			}
+		} else if (starts_with(refname, info->ref)) {
+			deco_type = info->decoration;
+			break;
+		}
+	}
 
 	add_name_decoration(deco_type, refname, obj);
 	while (obj->type == OBJ_TAG) {