threads / patch / 59199

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

Subject: [PATCH] pack-objects: use strcspn(3) in name_cmp_len()

## tl;dr

3 messages between Feb 5, 2023 and Feb 6, 2023. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

René Scharfe· Feb 5, 2023, 10:42 UTC · lore

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 <l.s.r@web.de>
---
Formatted with --function-context for easier review.
 builtin/pack-objects.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)
Show changes to builtin/pack-objects.c +5 −9
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);
 		}
 	}
 }
--
2.39.1
Ævar Arnfjörð Bjarmason· Feb 5, 2023, 20:59 UTC · re: René Scharfe · lore

Re: [PATCH] pack-objects: use strcspn(3) in name_cmp_len()

On Sun, Feb 05 2023, René Scharfe wrote:
Show 9 quoted lines
> -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/".

Show 14 quoted lines
>  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, 22:35 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] pack-objects: use strcspn(3) in name_cmp_len()

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 5 quoted lines
> 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.

← back to recent threads