verify-pack: use index-pack --verify

This finally gets rid of the inefficient verify-pack implementation that
walks objects in the packfile in their object name order and replaces it
with a call to index-pack --verify. As a side effect, it also removes
packed_object_info_detail() API which is rather expensive.

As this changes the way errors are reported (verify-pack used to rely on
the usual runtime error detection routine unpack_entry() to diagnose the
CRC errors in an entry in the *.idx file; index-pack --verify checks the
whole *.idx file in one go), update a test that expected the string "CRC"
to appear in the error message.

Signed-off-by: Junio C Hamano <[email protected]>

Author: gitster

builtin/verify-pack.c

@@ -1,134 +1,53 @@
 #include "builtin.h"
 #include "cache.h"
-#include "pack.h"
-#include "pack-revindex.h"
+#include "run-command.h"
 #include "parse-options.h"
 
-#define MAX_CHAIN 50
-
 #define VERIFY_PACK_VERBOSE 01
 #define VERIFY_PACK_STAT_ONLY 02
 
-static void show_pack_info(struct packed_git *p, unsigned int flags)
-{
-	uint32_t nr_objects, i;
-	int cnt;
-	int stat_only = flags & VERIFY_PACK_STAT_ONLY;
-	unsigned long chain_histogram[MAX_CHAIN+1], baseobjects;
-
-	nr_objects = p->num_objects;
-	memset(chain_histogram, 0, sizeof(chain_histogram));
-	baseobjects = 0;
-
-	for (i = 0; i < nr_objects; i++) {
-		const unsigned char *sha1;
-		unsigned char base_sha1[20];
-		const char *type;
-		unsigned long size;
-		unsigned long store_size;
-		off_t offset;
-		unsigned int delta_chain_length;
-
-		sha1 = nth_packed_object_sha1(p, i);
-		if (!sha1)
-			die("internal error pack-check nth-packed-object");
-		offset = nth_packed_object_offset(p, i);
-		type = packed_object_info_detail(p, offset, &size, &store_size,
-						 &delta_chain_length,
-						 base_sha1);
-		if (!stat_only)
-			printf("%s ", sha1_to_hex(sha1));
-		if (!delta_chain_length) {
-			if (!stat_only)
-				printf("%-6s %lu %lu %"PRIuMAX"\n",
-				       type, size, store_size, (uintmax_t)offset);
-			baseobjects++;
-		}
-		else {
-			if (!stat_only)
-				printf("%-6s %lu %lu %"PRIuMAX" %u %s\n",
-				       type, size, store_size, (uintmax_t)offset,
-				       delta_chain_length, sha1_to_hex(base_sha1));
-			if (delta_chain_length <= MAX_CHAIN)
-				chain_histogram[delta_chain_length]++;
-			else
-				chain_histogram[0]++;
-		}
-	}
-
-	if (baseobjects)
-		printf("non delta: %lu object%s\n",
-		       baseobjects, baseobjects > 1 ? "s" : "");
-
-	for (cnt = 1; cnt <= MAX_CHAIN; cnt++) {
-		if (!chain_histogram[cnt])
-			continue;
-		printf("chain length = %d: %lu object%s\n", cnt,
-		       chain_histogram[cnt],
-		       chain_histogram[cnt] > 1 ? "s" : "");
-	}
-	if (chain_histogram[0])
-		printf("chain length > %d: %lu object%s\n", MAX_CHAIN,
-		       chain_histogram[0],
-		       chain_histogram[0] > 1 ? "s" : "");
-}
-
 static int verify_one_pack(const char *path, unsigned int flags)
 {
-	char arg[PATH_MAX];
-	int len;
+	struct child_process index_pack;
+	const char *argv[] = {"index-pack", NULL, NULL, NULL };
+	struct strbuf arg = STRBUF_INIT;
 	int verbose = flags & VERIFY_PACK_VERBOSE;
 	int stat_only = flags & VERIFY_PACK_STAT_ONLY;
-	struct packed_git *pack;
 	int err;
 
-	len = strlcpy(arg, path, PATH_MAX);
-	if (len >= PATH_MAX)
-		return error("name too long: %s", path);
-
-	/*
-	 * In addition to "foo.idx" we accept "foo.pack" and "foo";
-	 * normalize these forms to "foo.idx" for add_packed_git().
-	 */
-	if (has_extension(arg, ".pack")) {
-		strcpy(arg + len - 5, ".idx");
-		len--;
-	} else if (!has_extension(arg, ".idx")) {
-		if (len + 4 >= PATH_MAX)
-			return error("name too long: %s.idx", arg);
-		strcpy(arg + len, ".idx");
-		len += 4;
-	}
+	if (stat_only)
+		argv[1] = "--verify-stat-only";
+	else if (verbose)
+		argv[1] = "--verify-stat";
+	else
+		argv[1] = "--verify";
 
 	/*
-	 * add_packed_git() uses our buffer (containing "foo.idx") to
-	 * build the pack filename ("foo.pack").  Make sure it fits.
+	 * In addition to "foo.pack" we accept "foo.idx" and "foo";
+	 * normalize these forms to "foo.pack" for "index-pack --verify".
 	 */
-	if (len + 1 >= PATH_MAX) {
-		arg[len - 4] = '\0';
-		return error("name too long: %s.pack", arg);
-	}
-
-	pack = add_packed_git(arg, len, 1);
-	if (!pack)
-		return error("packfile %s not found.", arg);
+	strbuf_addstr(&arg, path);
+	if (has_extension(arg.buf, ".idx"))
+		strbuf_splice(&arg, arg.len - 3, 3, "pack", 4);
+	else if (!has_extension(arg.buf, ".pack"))
+		strbuf_add(&arg, ".pack", 5);
+	argv[2] = arg.buf;
 
-	install_packed_git(pack);
+	memset(&index_pack, 0, sizeof(index_pack));
+	index_pack.argv = argv;
+	index_pack.git_cmd = 1;
 
-	if (!stat_only)
-		err = verify_pack(pack);
-	else
-		err = open_pack_index(pack);
+	err = run_command(&index_pack);
 
 	if (verbose || stat_only) {
 		if (err)
-			printf("%s: bad\n", pack->pack_name);
+			printf("%s: bad\n", arg.buf);
 		else {
-			show_pack_info(pack, flags);
 			if (!stat_only)
-				printf("%s: ok\n", pack->pack_name);
+				printf("%s: ok\n", arg.buf);
 		}
 	}
+	strbuf_release(&arg);
 
 	return err;
 }
@@ -159,7 +78,6 @@ int cmd_verify_pack(int argc, const char **argv, const char *prefix)
 	for (i = 0; i < argc; i++) {
 		if (verify_one_pack(argv[i], flags))
 			err = 1;
-		discard_revindex();
 	}
 
 	return err;

cache.h

@@ -991,7 +991,6 @@ extern off_t find_pack_entry_one(const unsigned char *, struct packed_git *);
 extern void *unpack_entry(struct packed_git *, off_t, enum object_type *, unsigned long *);
 extern unsigned long unpack_object_header_buffer(const unsigned char *buf, unsigned long len, enum object_type *type, unsigned long *sizep);
 extern unsigned long get_size_from_delta(struct packed_git *, struct pack_window **, off_t);
-extern const char *packed_object_info_detail(struct packed_git *, off_t, unsigned long *, unsigned long *, unsigned int *, unsigned char *);
 
 /* Dumb servers support */
 extern int update_server_info(int);

sha1_file.c

@@ -1496,61 +1496,6 @@ static int unpack_object_header(struct packed_git *p,
 	return type;
 }
 
-const char *packed_object_info_detail(struct packed_git *p,
-				      off_t obj_offset,
-				      unsigned long *size,
-				      unsigned long *store_size,
-				      unsigned int *delta_chain_length,
-				      unsigned char *base_sha1)
-{
-	struct pack_window *w_curs = NULL;
-	off_t curpos;
-	unsigned long dummy;
-	unsigned char *next_sha1;
-	enum object_type type;
-	struct revindex_entry *revidx;
-
-	*delta_chain_length = 0;
-	curpos = obj_offset;
-	type = unpack_object_header(p, &w_curs, &curpos, size);
-
-	revidx = find_pack_revindex(p, obj_offset);
-	*store_size = revidx[1].offset - obj_offset;
-
-	for (;;) {
-		switch (type) {
-		default:
-			die("pack %s contains unknown object type %d",
-			    p->pack_name, type);
-		case OBJ_COMMIT:
-		case OBJ_TREE:
-		case OBJ_BLOB:
-		case OBJ_TAG:
-			unuse_pack(&w_curs);
-			return typename(type);
-		case OBJ_OFS_DELTA:
-			obj_offset = get_delta_base(p, &w_curs, &curpos, type, obj_offset);
-			if (!obj_offset)
-				die("pack %s contains bad delta base reference of type %s",
-				    p->pack_name, typename(type));
-			if (*delta_chain_length == 0) {
-				revidx = find_pack_revindex(p, obj_offset);
-				hashcpy(base_sha1, nth_packed_object_sha1(p, revidx->nr));
-			}
-			break;
-		case OBJ_REF_DELTA:
-			next_sha1 = use_pack(p, &w_curs, curpos, NULL);
-			if (*delta_chain_length == 0)
-				hashcpy(base_sha1, next_sha1);
-			obj_offset = find_pack_entry_one(next_sha1, p);
-			break;
-		}
-		(*delta_chain_length)++;
-		curpos = obj_offset;
-		type = unpack_object_header(p, &w_curs, &curpos, &dummy);
-	}
-}
-
 static int packed_object_info(struct packed_git *p, off_t obj_offset,
 			      unsigned long *sizep)
 {

t/t5302-pack-index.sh

@@ -226,9 +226,8 @@ test_expect_success \
      ( while read obj
        do git cat-file -p $obj >/dev/null || exit 1
        done <obj-list ) &&
-     err=$(test_must_fail git verify-pack \
-       ".git/objects/pack/pack-${pack1}.pack" 2>&1) &&
-     echo "$err" | grep "CRC mismatch"'
+     test_must_fail git verify-pack ".git/objects/pack/pack-${pack1}.pack"
+'
 
 test_expect_success 'running index-pack in the object store' '
     rm -f .git/objects/pack/* &&