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

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

[Jonas Fonseca]

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(-)

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

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

[Junio C Hamano]

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.

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

[Jonas Fonseca]

Junio C Hamano <junkio@cox.net> wrote Sun, Aug 27, 2006:
> 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