Re: [PATCH v2 2/2] sha1_file: use strncmp for string comparison

["René Scharfe" <l.s.r@web.de>]

Am 30.06.2014 15:43, schrieb Jeff King:
> On Sun, Jun 29, 2014 at 07:56:25AM +0200, René Scharfe wrote:
> 
>> Avoid overrunning the existing pack name (p->pack_name, a C string) in
>> the case that the new path is longer by using strncmp instead of memcmp
>> for comparing.  While at it, replace the magic constant 4 with a
>> strlen call to document its meaning.
> 
> An unwritten assumption with this code is that we have "foo.idx" and we
> want to make sure that we are matching "foo.pack" from the existing pack
> list. Both before and after your patch, I think we would match
> "foobar.pack".

Yes, indeed.

> It's probably not a big deal, as we don't expect random
> junk in the pack directory, but I wonder if it would be better to be
> explicit, like:

<snip>

Here's a simpler approach:

-- >8 --
Subject: [PATCH v2 3/2] sha1_file: more precise packname matching

Consider the full length of the already loaded pack names when checking
for duplicates.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 sha1_file.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index b7ad6c1..a13fbbd 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1206,10 +1206,12 @@ static void prepare_packed_git_one(char *objdir, int local)
 		strbuf_addstr(&path, de->d_name);
 
 		if (has_extension(de->d_name, ".idx")) {
+			size_t len = path.len - strlen(".idx");
+
 			/* Don't reopen a pack we already have. */
 			for (p = packed_git; p; p = p->next) {
-				if (!strncmp(path.buf, p->pack_name,
-					     path.len - strlen(".idx")))
+				if (!strncmp(p->pack_name, path.buf, len) &&
+				    !strcmp(p->pack_name + len, ".pack"))
 					break;
 			}
 			if (p == NULL &&
-- 
2.0.0