[PATCH 0/4] index-pack (continued)

[PATCH 0/4] index-pack (continued)

[Junio C Hamano]

This is a contination of the earlier index-pack series that has been
in "stalled" state. I do not plan to push this forward for 1.7.6 (the
endgame of this series is a 2.0 material but it will be a long way to
get there).

Junio C Hamano (4):
  index-pack: a miniscule refactor
  index-pack: start learning to emulate "verify-pack -v"
  index-pack: show histogram when emulating "verify-pack -v"
  verify-pack: use index-pack --verify

 builtin/index-pack.c  |   74 +++++++++++++++++++++++++--
 builtin/verify-pack.c |  132 +++++++++---------------------------------------
 cache.h               |    1 -
 sha1_file.c           |   55 --------------------
 t/t5302-pack-index.sh |    5 +-
 5 files changed, 95 insertions(+), 172 deletions(-)

-- 
1.7.6.rc0.106.g68174

[PATCH 1/4] index-pack: a miniscule refactor

[Junio C Hamano]

Introduce a helper function that takes the type of an object and
tell if it is a delta, as we seem to use this check in many places.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/index-pack.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 513fbbc..0216af7 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -498,12 +498,17 @@ static void sha1_object(const void *data, unsigned long size,
 	}
 }
 
+static int is_delta_type(enum object_type type)
+{
+	return (type == OBJ_REF_DELTA || type == OBJ_OFS_DELTA);
+}
+
 static void *get_base_data(struct base_data *c)
 {
 	if (!c->data) {
 		struct object_entry *obj = c->obj;
 
-		if (obj->type == OBJ_REF_DELTA || obj->type == OBJ_OFS_DELTA) {
+		if (is_delta_type(obj->type)) {
 			void *base = get_base_data(c->base);
 			void *raw = get_data_from_pack(obj);
 			c->data = patch_delta(
@@ -629,7 +634,7 @@ static void parse_pack_objects(unsigned char *sha1)
 		struct object_entry *obj = &objects[i];
 		void *data = unpack_raw_entry(obj, &delta->base);
 		obj->real_type = obj->type;
-		if (obj->type == OBJ_REF_DELTA || obj->type == OBJ_OFS_DELTA) {
+		if (is_delta_type(obj->type)) {
 			nr_deltas++;
 			delta->obj_no = i;
 			delta++;
@@ -676,7 +681,7 @@ static void parse_pack_objects(unsigned char *sha1)
 		struct object_entry *obj = &objects[i];
 		struct base_data base_obj;
 
-		if (obj->type == OBJ_REF_DELTA || obj->type == OBJ_OFS_DELTA)
+		if (is_delta_type(obj->type))
 			continue;
 		base_obj.obj = obj;
 		base_obj.data = NULL;
-- 
1.7.6.rc0.106.g68174

[PATCH 2/4] index-pack: start learning to emulate "verify-pack -v"

[Junio C Hamano]

The "index-pack" machinery already has almost enough knowledge to produce
the same output as "verify-pack -v". Fill small gaps in its bookkeeping,
and teach it to show what it knows.

Add a few more command line options that do not have to be advertised to
the end users. They will be used internally when verify-pack calls this.
The eventual goal is to remove verify-pack implementation and redo it as a
thin wrapper around the index-pack, so that we can remove the rather
expensive packed_object_info_detail() API.

This still does not do the delta-chain-depth histogram yet but that part
is easy.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/index-pack.c |   43 ++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 0216af7..aa3c9c6 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -20,6 +20,8 @@ struct object_entry
 	unsigned int hdr_size;
 	enum object_type type;
 	enum object_type real_type;
+	unsigned delta_depth;
+	int base_object_no;
 };
 
 union delta_base {
@@ -535,6 +537,8 @@ static void resolve_delta(struct object_entry *delta_obj,
 	void *base_data, *delta_data;
 
 	delta_obj->real_type = base->obj->real_type;
+	delta_obj->delta_depth = base->obj->delta_depth + 1;
+	delta_obj->base_object_no = base->obj - objects;
 	delta_data = get_data_from_pack(delta_obj);
 	base_data = get_base_data(base);
 	result->obj = delta_obj;
@@ -967,9 +971,32 @@ static void read_idx_option(struct pack_idx_option *opts, const char *pack_name)
 	free(p);
 }
 
+static void show_pack_info(int stat_only)
+{
+	int i;
+	for (i = 0; i < nr_objects; i++) {
+		struct object_entry *obj = &objects[i];
+
+		/* NEEDSWORK: Compute data necessary for the "histogram" here */
+
+		if (stat_only)
+			continue;
+		printf("%s %-6s %lu %lu %"PRIuMAX,
+		       sha1_to_hex(obj->idx.sha1),
+		       typename(obj->real_type), obj->size,
+		       (unsigned long)(obj[1].idx.offset - obj->idx.offset),
+		       (uintmax_t)obj->idx.offset);
+		if (is_delta_type(obj->type)) {
+			struct object_entry *bobj = &objects[obj->base_object_no];
+			printf(" %u %s", obj->delta_depth, sha1_to_hex(bobj->idx.sha1));
+		}
+		putchar('\n');
+	}
+}
+
 int cmd_index_pack(int argc, const char **argv, const char *prefix)
 {
-	int i, fix_thin_pack = 0, verify = 0;
+	int i, fix_thin_pack = 0, verify = 0, stat_only = 0, stat = 0;
 	const char *curr_pack, *curr_index;
 	const char *index_name = NULL, *pack_name = NULL;
 	const char *keep_name = NULL, *keep_msg = NULL;
@@ -1000,6 +1027,13 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 				strict = 1;
 			} else if (!strcmp(arg, "--verify")) {
 				verify = 1;
+			} else if (!strcmp(arg, "--verify-stat")) {
+				verify = 1;
+				stat = 1;
+			} else if (!strcmp(arg, "--verify-stat-only")) {
+				verify = 1;
+				stat = 1;
+				stat_only = 1;
 			} else if (!strcmp(arg, "--keep")) {
 				keep_msg = "";
 			} else if (!prefixcmp(arg, "--keep=")) {
@@ -1075,8 +1109,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 
 	curr_pack = open_pack_file(pack_name);
 	parse_pack_header();
-	objects = xmalloc((nr_objects + 1) * sizeof(struct object_entry));
-	deltas = xmalloc(nr_objects * sizeof(struct delta_entry));
+	objects = xcalloc(nr_objects + 1, sizeof(struct object_entry));
+	deltas = xcalloc(nr_objects, sizeof(struct delta_entry));
 	parse_pack_objects(pack_sha1);
 	if (nr_deltas == nr_resolved_deltas) {
 		stop_progress(&progress);
@@ -1116,6 +1150,9 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 	if (strict)
 		check_objects();
 
+	if (stat)
+		show_pack_info(stat_only);
+
 	idx_objects = xmalloc((nr_objects) * sizeof(struct pack_idx_entry *));
 	for (i = 0; i < nr_objects; i++)
 		idx_objects[i] = &objects[i].idx;
-- 
1.7.6.rc0.106.g68174

[PATCH 3/4] index-pack: show histogram when emulating "verify-pack -v"

[Junio C Hamano]

The histogram produced by "verify-pack -v" always had an artificial
limit of 50, but index-pack knows what the maximum delta depth is, so
we do not have to limit it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * We could limit it, but I do not see a practical value of doing so.

 builtin/index-pack.c |   26 +++++++++++++++++++++++---
 1 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index aa3c9c6..ed4c3bb 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -70,6 +70,7 @@ static struct progress *progress;
 static unsigned char input_buffer[4096];
 static unsigned int input_offset, input_len;
 static off_t consumed_bytes;
+static unsigned deepest_delta;
 static git_SHA_CTX input_ctx;
 static uint32_t input_crc32;
 static int input_fd, output_fd, pack_fd;
@@ -538,6 +539,8 @@ static void resolve_delta(struct object_entry *delta_obj,
 
 	delta_obj->real_type = base->obj->real_type;
 	delta_obj->delta_depth = base->obj->delta_depth + 1;
+	if (deepest_delta < delta_obj->delta_depth)
+		deepest_delta = delta_obj->delta_depth;
 	delta_obj->base_object_no = base->obj - objects;
 	delta_data = get_data_from_pack(delta_obj);
 	base_data = get_base_data(base);
@@ -973,12 +976,17 @@ static void read_idx_option(struct pack_idx_option *opts, const char *pack_name)
 
 static void show_pack_info(int stat_only)
 {
-	int i;
+	int i, baseobjects = nr_objects - nr_deltas;
+	unsigned long *chain_histogram = NULL;
+
+	if (deepest_delta)
+		chain_histogram = xcalloc(deepest_delta, sizeof(unsigned long));
+
 	for (i = 0; i < nr_objects; i++) {
 		struct object_entry *obj = &objects[i];
 
-		/* NEEDSWORK: Compute data necessary for the "histogram" here */
-
+		if (is_delta_type(obj->type))
+			chain_histogram[obj->delta_depth - 1]++;
 		if (stat_only)
 			continue;
 		printf("%s %-6s %lu %lu %"PRIuMAX,
@@ -992,6 +1000,18 @@ static void show_pack_info(int stat_only)
 		}
 		putchar('\n');
 	}
+
+	if (baseobjects)
+		printf("non delta: %d object%s\n",
+		       baseobjects, baseobjects > 1 ? "s" : "");
+	for (i = 0; i < deepest_delta; i++) {
+		if (!chain_histogram[i])
+			continue;
+		printf("chain length = %d: %lu object%s\n",
+		       i + 1,
+		       chain_histogram[i],
+		       chain_histogram[i] > 1 ? "s" : "");
+	}
 }
 
 int cmd_index_pack(int argc, const char **argv, const char *prefix)
-- 
1.7.6.rc0.106.g68174

[PATCH 4/4] verify-pack: use index-pack --verify

[Junio C Hamano]

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 <gitster@pobox.com>
---
 builtin/verify-pack.c |  132 +++++++++---------------------------------------
 cache.h               |    1 -
 sha1_file.c           |   55 --------------------
 t/t5302-pack-index.sh |    5 +-
 4 files changed, 27 insertions(+), 166 deletions(-)

diff --git a/builtin/verify-pack.c b/builtin/verify-pack.c
index b6079ae..e841b4a 100644
--- a/builtin/verify-pack.c
+++ b/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;
diff --git a/cache.h b/cache.h
index 08a9022..edea69e 100644
--- a/cache.h
+++ b/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);
diff --git a/sha1_file.c b/sha1_file.c
index d949b35..ca87e3d 100644
--- a/sha1_file.c
+++ b/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)
 {
diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh
index 76bcaca..f8fa924 100755
--- a/t/t5302-pack-index.sh
+++ b/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/* &&
-- 
1.7.6.rc0.106.g68174