[PATCH] Fix packname hash generation.

This changes the generation of hash packfiles have in their names, from
"hash of object names as fed to us" to "hash of object names in the
resulting pack, in the order they appear in the index file".  The new
"git-index-pack" command is taught to output the computed hash value
to its standard output.

With this, we can store downloaded pack in a temporary file without
knowing its final name, run git-index-pack to generate idx for it
while finding out its final name, and then rename the pack and idx to
their final names.

Signed-off-by: Junio C Hamano <redacted>

---

 * Right now, the pack "hash" name only serves the collision
   avoidance purposes, but not true identification.  The same
   set of objects can be fed to pack-objects in different order,
   produce the same pack, and still end up with different
   names.

   This will be used in the next experiment, "git-clone not
   exploding the downloaded pack".

 index-pack.c   |   15 +++++++++++++--
 pack-objects.c |   14 ++++++++++----
 2 files changed, 23 insertions(+), 6 deletions(-)

applies-to: ea37b42d53264d65f746b3e42577349e8a44d5c4
3b97470e3711d7af3505baddc34428a0d9bd8214

diff --git a/index-pack.c b/index-pack.c
index badbeab..785fe71 100644
--- a/index-pack.c
+++ b/index-pack.c

@@ -349,7 +349,7 @@ static int sha1_compare(const void *_a, 
 	return memcmp(a->sha1, b->sha1, 20);
 }
 
-static void write_index_file(const char *index_name)
+static void write_index_file(const char *index_name, unsigned char *sha1)
 {
 	struct sha1file *f;
 	struct object_entry **sorted_by_sha =

@@ -358,6 +358,7 @@ static void write_index_file(const char 
 	struct object_entry **last = sorted_by_sha + nr_objects;
 	unsigned int array[256];
 	int i;
+	SHA_CTX ctx;
 
 	for (i = 0; i < nr_objects; ++i)
 		sorted_by_sha[i] = &objects[i];

@@ -385,6 +386,11 @@ static void write_index_file(const char 
 	}
 	sha1write(f, array, 256 * sizeof(int));
 
+	/* recompute the SHA1 hash of sorted object names.
+	 * currently pack-objects does not do this, but that
+	 * can be fixed.
+	 */
+	SHA1_Init(&ctx);
 	/*
 	 * Write the actual SHA1 entries..
 	 */

@@ -394,10 +400,12 @@ static void write_index_file(const char 
 		unsigned int offset = htonl(obj->offset);
 		sha1write(f, &offset, 4);
 		sha1write(f, obj->sha1, 20);
+		SHA1_Update(&ctx, obj->sha1, 20);
 	}
 	sha1write(f, pack_base + pack_size - 20, 20);
 	sha1close(f, NULL, 1);
 	free(sorted_by_sha);
+	SHA1_Final(sha1, &ctx);
 }
 
 int main(int argc, char **argv)

@@ -405,6 +413,7 @@ int main(int argc, char **argv)
 	int i;
 	char *index_name = NULL;
 	char *index_name_buf = NULL;
+	unsigned char sha1[20];
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];

@@ -443,9 +452,11 @@ int main(int argc, char **argv)
 	deltas = xcalloc(nr_objects, sizeof(struct delta_entry));
 	parse_pack_objects();
 	free(deltas);
-	write_index_file(index_name);
+	write_index_file(index_name, sha1);
 	free(objects);
 	free(index_name_buf);
 
+	printf("%s\n", sha1_to_hex(sha1));
+
 	return 0;
 }

diff --git a/pack-objects.c b/pack-objects.c
index 3d62278..ef55cab 100644
--- a/pack-objects.c
+++ b/pack-objects.c

@@ -393,6 +393,7 @@ int main(int argc, char **argv)
 	SHA_CTX ctx;
 	char line[PATH_MAX + 20];
 	int window = 10, depth = 10, pack_to_stdout = 0;
+	struct object_entry **list;
 	int i;
 
 	for (i = 1; i < argc; i++) {

@@ -435,7 +436,6 @@ int main(int argc, char **argv)
 	if (pack_to_stdout != !base_name)
 		usage(pack_usage);
 
-	SHA1_Init(&ctx);
 	while (fgets(line, sizeof(line), stdin) != NULL) {
 		unsigned int hash;
 		char *p;

@@ -451,10 +451,8 @@ int main(int argc, char **argv)
 				continue;
 			hash = hash * 11 + c;
 		}
-		if (add_object_entry(sha1, hash))
-			SHA1_Update(&ctx, sha1, 20);
+		add_object_entry(sha1, hash);
 	}
-	SHA1_Final(object_list_sha1, &ctx);
 	if (non_empty && !nr_objects)
 		return 0;
 	get_object_details();

@@ -462,6 +460,14 @@ int main(int argc, char **argv)
 	fprintf(stderr, "Packing %d objects\n", nr_objects);
 
 	sorted_by_sha = create_sorted_list(sha1_sort);
+	SHA1_Init(&ctx);
+	list = sorted_by_sha;
+	for (i = 0; i < nr_objects; i++) {
+		struct object_entry *entry = *list++;
+		SHA1_Update(&ctx, entry->sha1, 20);
+	}
+	SHA1_Final(object_list_sha1, &ctx);
+
 	sorted_by_type = create_sorted_list(type_size_sort);
 	if (window && depth)
 		find_deltas(sorted_by_type, window+1, depth);

---
0.99.8.GIT

Junio C Hamano [off-list ref] writes:

This changes the generation of hash packfiles have in their names, from
"hash of object names as fed to us" to "hash of object names in the
resulting pack, in the order they appear in the index file".  The new
"git-index-pack" command is taught to output the computed hash value
to its standard output.

In case it was not obvious, this is not a backward incompatible
change.  Your existing packs will be valid after this change.

What those 40-byte hashes were buying us was that we did not
have to worry about name clashes.  We could have said "these two
packs have the same name so they must have the same set of
objects", but there is no tool that relies on this fact.  We
could not even say "these two packs have different names so the
set of objects contained by them must be different" -- the
resulting pack name depended on the order of objects fed to
git-pack-objects, even if you fed the same set of objects.

The really core part never cared about how packfiles and their
indices are named.  The only restrictions were that they live
immediately under .git/objects/pack/, have .pack and .idx suffix
respectively, and their basename match with each other.

The commit walkers (anything that link with fetch.c) impose
another limitation that their basenames are "pack-" followed by
40-byte hexadecimal digits.  But they do not check if the name
is consistent with the set of objects in the pack (checking it
was computationally infeasible for huge packs in the previous
hashing mechanism -- you have to feed all permutations of
objects contained in the pack to SHA1 hash and see if any
produces the same hash as the pack name).  We _could_ now do
this additional check if we wanted to (the same goes to the
really core part in sha1_file.c::check_packed_git_idx()).

In short, it does not matter if your existing packs are named
using the old hashing mechanism.  They will continue to be
valid.

But if you really care about consistency, here is an easy way to
rename your existing packs to their new names the new hashing
scheme would produce.

#!/bin/sh

: ${GIT_DIR=.git}
: ${GIT_OBJECT_DIRECTORY="${GIT_DIR}/objects"}

O="$GIT_OBJECT_DIRECTORY"
P="$GIT_OBJECT_DIRECTORY/pack"
for existing in `cd "$GIT_OBJECT_DIRECTORY" &&
		 find pack -name '*.pack' -print`
do
    idx=`expr "$existing" : '\(.*\)\.pack$'`.idx &&
    test -f "$O/$idx" || {
        echo >&2 "Missing idx $idx?"
        continue
    }
    new=`git-index-pack -o tmp-idx "$O/$existing"` || {
        echo >&2 "Corrupt pack $existing?"
        continue
    }           
    # index generated for an existing pack should match.
    cmp "$O/$idx" tmp-idx || {
        echo >&2 "Corrupt idx $idx?"
        continue
    }
    if test "pack/pack-$new.pack" = "$existing"
    then
        echo >&2 "Already converted $existing."
        continue
    fi
    if test -f "$P/pack-$new.pack" || test -f "$P/pack-$new.idx"
    then
        echo >&2 "Name clash! $new"
        continue
    fi
    mv "$O/$existing" "$P/pack-$new.pack" &&
    mv "$O/$idx" "$P/pack-$new.idx" || {
        echo >&2 "Cannot move $existing to $new"
        continue
    }
    echo >&2 "Renamed $existing -> $new"
done