pack-objects: use strcspn(3) in name_cmp_len()

Commit Message

René Scharfe Feb. 5, 2023, 10:42 a.m. UTC

Call strcspn(3) to find the length of a string terminated by NUL, NL or
slash instead of open-coding it.  Adopt its return type, size_t, to
support strings of arbitrary length.  Use that type in callers as well
for variables and function parameters that receive the return value.

Signed-off-by: René Scharfe <[e-mail]>
---
Formatted with --function-context for easier review.

 builtin/pack-objects.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

--
2.39.1

Comments

Ævar Arnfjörð Bjarmason Feb. 5, 2023, 8:59 p.m. UTC | #1

On Sun, Feb 05 2023, René Scharfe wrote:

> -static int name_cmp_len(const char *name)
> +static size_t name_cmp_len(const char *name)
>  {
> -	int i;
> -	for (i = 0; name[i] && name[i] != '\n' && name[i] != '/'; i++)
> -		;
> -	return i;
> +	return strcspn(name, "\n/");
>  }

I wonder if this name_cmp_len() is worth keeping at all. If all we're
doing is wrapping strcspn() (which b.t.w, seem to be less "open-coding"
and just that it wasn't known to the original author in 5d4a6003354
(Make git-pack-objects a builtin, 2006-08-03)), then just inlining that
in the two name_cmp_len() invocations would be better, or maybe:

	strcspn(..., object_reject);

Where we previously declared "object_reject" (or same better name for
such a variable) to be "\n/".

>  static void add_pbase_object(struct tree_desc *tree,
>  			     const char *name,
> -			     int cmplen,
> +			     size_t cmplen,
>  			     const char *fullname)
>  {
>  	struct name_entry entry;
>  	int cmp;
>
>  	while (tree_entry(tree,&entry)) {
>  		if (S_ISGITLINK(entry.mode))
>  			continue;
>  		cmp = tree_entry_len(&entry) != cmplen ? 1 :
>  		      memcmp(name, entry.path, cmplen);

The return of tree_entry_len() is "int", so that's a new implicit cast,
but that seems OK in this case (and the "namelen" is another thing we
should convert to "size_t").

Junio C Hamano Feb. 6, 2023, 10:35 p.m. UTC | #2

Ævar Arnfjörð Bjarmason <[e-mail]> writes:

> I wonder if this name_cmp_len() is worth keeping at all. If all we're
> doing is wrapping strcspn() (which b.t.w, seem to be less "open-coding"
> and just that it wasn't known to the original author in 5d4a6003354
> (Make git-pack-objects a builtin, 2006-08-03)), then just inlining that
> in the two name_cmp_len() invocations would be better, or maybe:

Even if the stop candidate bytes were a constant, or if there were
only a single callsite, I am not sure if it is a good idea, simply
because with this

> 	strcspn(..., object_reject);

or with a literal "\n/" to make it easier to see where in the string
we are stopping, it is hard without named function to tell what
length we are computing.

The function being file-scope static, decent compilers hopefully
would inline the calls by two callers.

Patch

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index cabace4abb..536edc3a48 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1708,50 +1708,47 @@  static void pbase_tree_put(struct pbase_tree_cache *cache)
 	free(cache);
 }

-static int name_cmp_len(const char *name)
+static size_t name_cmp_len(const char *name)
 {
-	int i;
-	for (i = 0; name[i] && name[i] != '\n' && name[i] != '/'; i++)
-		;
-	return i;
+	return strcspn(name, "\n/");
 }

 static void add_pbase_object(struct tree_desc *tree,
 			     const char *name,
-			     int cmplen,
+			     size_t cmplen,
 			     const char *fullname)
 {
 	struct name_entry entry;
 	int cmp;

 	while (tree_entry(tree,&entry)) {
 		if (S_ISGITLINK(entry.mode))
 			continue;
 		cmp = tree_entry_len(&entry) != cmplen ? 1 :
 		      memcmp(name, entry.path, cmplen);
 		if (cmp > 0)
 			continue;
 		if (cmp < 0)
 			return;
 		if (name[cmplen] != '/') {
 			add_object_entry(&entry.oid,
 					 object_type(entry.mode),
 					 fullname, 1);
 			return;
 		}
 		if (S_ISDIR(entry.mode)) {
 			struct tree_desc sub;
 			struct pbase_tree_cache *tree;
 			const char *down = name+cmplen+1;
-			int downlen = name_cmp_len(down);
+			size_t downlen = name_cmp_len(down);

 			tree = pbase_tree_get(&entry.oid);
 			if (!tree)
 				return;
 			init_tree_desc(&sub, tree->tree_data, tree->tree_size);

 			add_pbase_object(&sub, down, downlen, fullname);
 			pbase_tree_put(tree);
 		}
 	}
 }
@@ -1795,21 +1792,21 @@  static int check_pbase_path(unsigned hash)
 static void add_preferred_base_object(const char *name)
 {
 	struct pbase_tree *it;
-	int cmplen;
+	size_t cmplen;
 	unsigned hash = pack_name_hash(name);

 	if (!num_preferred_base || check_pbase_path(hash))
 		return;

 	cmplen = name_cmp_len(name);
 	for (it = pbase_tree; it; it = it->next) {
 		if (cmplen == 0) {
 			add_object_entry(&it->pcache.oid, OBJ_TREE, NULL, 1);
 		}
 		else {
 			struct tree_desc tree;
 			init_tree_desc(&tree, it->pcache.tree_data, it->pcache.tree_size);
 			add_pbase_object(&tree, name, cmplen, name);
 		}
 	}
 }