[PATCH] Refactor sha1_pack_index_name and sha1_pack_name to use a common backend

Jonas Fonseca· Aug 28, 2006, 00:16 UTC · lore

Signed-off-by: Jonas Fonseca <fonseca@diku.dk>
---

Tested with doing a local cg-clone, since there doesn't seem to be any tests of users, such as git-local-fetch, under t/.

This adds another pair of static buffers, if that's a problem and the cleanup is still wanted I can change it to use malloc.

 sha1_file.c |   64 +++++++++++++++++++++++------------------------------------
 1 files changed, 25 insertions(+), 39 deletions(-)

Show changes to sha1_file.c +25 −39

diff --git a/sha1_file.c b/sha1_file.c
index 01aa745..5a846f5 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -147,65 +147,51 @@ char *sha1_file_name(const unsigned char
 	return base;
 }
 
-char *sha1_pack_name(const unsigned char *sha1)
+static int fill_sha1_pack_name(const unsigned char *sha1, char base[], size_t baselen,
+			       int dir_offset, const char *extension)
 {
 	static const char hex[] = "0123456789abcdef";
-	static char *name, *base, *buf;
-	static const char *last_objdir;
 	const char *sha1_file_directory = get_object_directory();
+	char *buf;
 	int i;
 
-	if (!last_objdir || strcmp(last_objdir, sha1_file_directory)) {
-		int len = strlen(sha1_file_directory);
-		if (base)
-			free(base);
-		base = xmalloc(len + 60);
-		sprintf(base, "%s/pack/pack-1234567890123456789012345678901234567890.pack", sha1_file_directory);
-		name = base + len + 11;
-		if (last_objdir)
-			free((char *) last_objdir);
-		last_objdir = strdup(sha1_file_directory);
+	base[dir_offset] = 0;
+	if (strcmp(base, sha1_file_directory)) {
+		dir_offset = strlen(sha1_file_directory);
+		if (snprintf(base, baselen,
+			     "%s/pack/pack-1234567890123456789012345678901234567890.%s",
+			     sha1_file_directory, extension) >= baselen)
+			die("pack name too long");
 	}
+	base[dir_offset] = '/';
 
-	buf = name;
+	buf = base + dir_offset + 11;
 
 	for (i = 0; i < 20; i++) {
 		unsigned int val = *sha1++;
+
 		*buf++ = hex[val >> 4];
 		*buf++ = hex[val & 0xf];
 	}
-	
-	return base;
+
+	return dir_offset;
 }
 
 char *sha1_pack_index_name(const unsigned char *sha1)
 {
-	static const char hex[] = "0123456789abcdef";
-	static char *name, *base, *buf;
-	static const char *last_objdir;
-	const char *sha1_file_directory = get_object_directory();
-	int i;
+	static char base[PATH_MAX + 1];
+	static int offset;
 
-	if (!last_objdir || strcmp(last_objdir, sha1_file_directory)) {
-		int len = strlen(sha1_file_directory);
-		if (base)
-			free(base);
-		base = xmalloc(len + 60);
-		sprintf(base, "%s/pack/pack-1234567890123456789012345678901234567890.idx", sha1_file_directory);
-		name = base + len + 11;
-		if (last_objdir)
-			free((char *) last_objdir);
-		last_objdir = strdup(sha1_file_directory);
-	}
+	offset = fill_sha1_pack_name(sha1, base, sizeof(base), offset, "idx");
+	return base;
+}
 
-	buf = name;
+char *sha1_pack_name(const unsigned char *sha1)
+{
+	static char base[PATH_MAX + 1];
+	static int offset;
 
-	for (i = 0; i < 20; i++) {
-		unsigned int val = *sha1++;
-		*buf++ = hex[val >> 4];
-		*buf++ = hex[val & 0xf];
-	}
-	
+	offset = fill_sha1_pack_name(sha1, base, sizeof(base), offset, "pack");
 	return base;
 }

-- 
1.4.2.g2f76-dirty

-- 
Jonas Fonseca

Junio C Hamano· Aug 28, 2006, 04:40 UTC · re: Jonas Fonseca · lore

Re: [PATCH] Refactor sha1_pack_index_name and sha1_pack_name to use a common backend

Jonas Fonseca <fonseca@diku.dk> writes:

> This adds another pair of static buffers, if that's a problem and the
> cleanup is still wanted I can change it to use malloc.

To be perfectly honest, I would rather get rid of these two functions, and also sha1[] member from struct packed_git.

The real core git does not impose any limit to the name of the packfiles, other than that the name of the datafile ends with ".pack" and that its corresponding pack index file has the same name ending with ".idx". It is only the packfile support which was later bolted on to commit walkers by commit bf592c5 that introduced this silly expectation that they are always of form "pack-[0-9a-f]{40}.pack" and "pack-[0-9a-f]{40}.idx".

For example, fetch_pack() in local-fetch.c is given an object name and tries to find the packfile that contains it by looking at ".idx" files downloaded from the other side. When it finds one, it fabricates a name for the pack file by what is in target->sha1 (where target is of type "struct packed_git").

However, struct packed_git already has perfectly good place that is meant to store the real name of the packfile, so this is totally unnecessary.

As a clean-up, I'd rather see a patch that removes the need for these two functions and one struct member, rather than keeping these two misguided functions and consolidating their implementations.

Jonas Fonseca· Aug 28, 2006, 13:34 UTC · re: Junio C Hamano · lore

Re: [PATCH] Refactor sha1_pack_index_name and sha1_pack_name to use a common backend

Junio C Hamano <junkio@cox.net> wrote Sun, Aug 27, 2006:

Show 9 quoted lines

> Jonas Fonseca <fonseca@diku.dk> writes:
> 
> > This adds another pair of static buffers, if that's a problem and the
> > cleanup is still wanted I can change it to use malloc.
> 
> As a clean-up, I'd rather see a patch that removes the need for
> these two functions and one struct member, rather than keeping
> these two misguided functions and consolidating their
> implementations.

Thanks for the outline, I will look into removing this limitation.

-- 
Jonas Fonseca

patchRefactor sha1_pack_index_name and sha1_pack_name to use a common backend