[PATCH 1/3] t5510: harden the way verify-pack is used

[PATCH 1/3] t5510: harden the way verify-pack is used

[Junio C Hamano]

The test ignored the exit status from verify pack command, and also relied
on not seeing any delta chain statistics.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5510-fetch.sh |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index bee3424..d13c806 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -9,6 +9,11 @@ test_description='Per branch config variables affects "git fetch".
 
 D=`pwd`
 
+test_bundle_object_count () {
+	git verify-pack -v "$1" >verify.out &&
+	test "$2" = $(grep '^[0-9a-f]\{40\} ' verify.out | wc -l)
+}
+
 test_expect_success setup '
 	echo >file original &&
 	git add file &&
@@ -146,6 +151,7 @@ test_expect_success 'unbundle 1' '
 	test_must_fail git fetch "$D/bundle1" master:master
 '
 
+
 test_expect_success 'bundle 1 has only 3 files ' '
 	cd "$D" &&
 	(
@@ -156,8 +162,7 @@ test_expect_success 'bundle 1 has only 3 files ' '
 		cat
 	) <bundle1 >bundle.pack &&
 	git index-pack bundle.pack &&
-	verify=$(git verify-pack -v bundle.pack) &&
-	test 4 = $(echo "$verify" | wc -l)
+	test_bundle_object_count bundle.pack 3
 '
 
 test_expect_success 'unbundle 2' '
@@ -180,7 +185,7 @@ test_expect_success 'bundle does not prerequisite objects' '
 		cat
 	) <bundle3 >bundle.pack &&
 	git index-pack bundle.pack &&
-	test 4 = $(git verify-pack -v bundle.pack | wc -l)
+	test_bundle_object_count bundle.pack 3
 '
 
 test_expect_success 'bundle should be able to create a full history' '
-- 
1.6.4.151.g786db

[PATCH 2/3] verify-pack -v: do not report "chain length 0"

[Junio C Hamano]

When making a histogram of delta chain length in the pack, the program
collects number of objects whose delta depth exceeds the MAX_CHAIN limit
in histogram[0], and showed it as the number of items that exceeds the
limit correctly.  HOWEVER, it also showed the same number labeled as
"chain length = 0".

In fact, we are not showing the number of objects whose chain length is
zero, i.e. the base objects.  Correct this.

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

diff --git a/builtin-verify-pack.c b/builtin-verify-pack.c
index ebd6dff..b5bd28e 100644
--- a/builtin-verify-pack.c
+++ b/builtin-verify-pack.c
@@ -8,10 +8,13 @@
 
 static void show_pack_info(struct packed_git *p)
 {
-	uint32_t nr_objects, i, chain_histogram[MAX_CHAIN+1];
+	uint32_t nr_objects, i;
+	int cnt;
+	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;
@@ -30,9 +33,11 @@ static void show_pack_info(struct packed_git *p)
 						 &delta_chain_length,
 						 base_sha1);
 		printf("%s ", sha1_to_hex(sha1));
-		if (!delta_chain_length)
+		if (!delta_chain_length) {
 			printf("%-6s %lu %lu %"PRIuMAX"\n",
 			       type, size, store_size, (uintmax_t)offset);
+			baseobjects++;
+		}
 		else {
 			printf("%-6s %lu %lu %"PRIuMAX" %u %s\n",
 			       type, size, store_size, (uintmax_t)offset,
@@ -44,15 +49,21 @@ static void show_pack_info(struct packed_git *p)
 		}
 	}
 
-	for (i = 0; i <= MAX_CHAIN; i++) {
-		if (!chain_histogram[i])
+	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 = %"PRIu32": %"PRIu32" object%s\n", i,
-		       chain_histogram[i], chain_histogram[i] > 1 ? "s" : "");
+		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: %"PRIu32" object%s\n", MAX_CHAIN,
-		       chain_histogram[0], chain_histogram[0] > 1 ? "s" : "");
+		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, int verbose)
-- 
1.6.4.151.g786db

[PATCH 3/3] verify-pack --stat-only: show histogram without verifying

[Junio C Hamano]

When this option is given, the command does not verify the pack contents,
but shows the delta chain histogram.  If used with --verbose, the usual
list of objects is also shown.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-verify-pack.txt |    8 +++++-
 builtin-verify-pack.c             |   49 +++++++++++++++++++++++++------------
 2 files changed, 40 insertions(+), 17 deletions(-)

diff --git a/Documentation/git-verify-pack.txt b/Documentation/git-verify-pack.txt
index d791a80..97f7f91 100644
--- a/Documentation/git-verify-pack.txt
+++ b/Documentation/git-verify-pack.txt
@@ -25,7 +25,13 @@ OPTIONS
 -v::
 --verbose::
 	After verifying the pack, show list of objects contained
-	in the pack.
+	in the pack and a histogram of delta chain length.
+
+-s::
+--stat-only::
+	Do not verify the pack contents; only show the histogram of delta
+	chain length.  With `--verbose`, list of objects is also shown.
+
 \--::
 	Do not interpret any more arguments as options.
 
diff --git a/builtin-verify-pack.c b/builtin-verify-pack.c
index b5bd28e..b6079ae 100644
--- a/builtin-verify-pack.c
+++ b/builtin-verify-pack.c
@@ -6,10 +6,14 @@
 
 #define MAX_CHAIN 50
 
-static void show_pack_info(struct packed_git *p)
+#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;
@@ -32,16 +36,19 @@ static void show_pack_info(struct packed_git *p)
 		type = packed_object_info_detail(p, offset, &size, &store_size,
 						 &delta_chain_length,
 						 base_sha1);
-		printf("%s ", sha1_to_hex(sha1));
+		if (!stat_only)
+			printf("%s ", sha1_to_hex(sha1));
 		if (!delta_chain_length) {
-			printf("%-6s %lu %lu %"PRIuMAX"\n",
-			       type, size, store_size, (uintmax_t)offset);
+			if (!stat_only)
+				printf("%-6s %lu %lu %"PRIuMAX"\n",
+				       type, size, store_size, (uintmax_t)offset);
 			baseobjects++;
 		}
 		else {
-			printf("%-6s %lu %lu %"PRIuMAX" %u %s\n",
-			       type, size, store_size, (uintmax_t)offset,
-			       delta_chain_length, sha1_to_hex(base_sha1));
+			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
@@ -66,10 +73,12 @@ static void show_pack_info(struct packed_git *p)
 		       chain_histogram[0] > 1 ? "s" : "");
 }
 
-static int verify_one_pack(const char *path, int verbose)
+static int verify_one_pack(const char *path, unsigned int flags)
 {
 	char arg[PATH_MAX];
 	int len;
+	int verbose = flags & VERIFY_PACK_VERBOSE;
+	int stat_only = flags & VERIFY_PACK_STAT_ONLY;
 	struct packed_git *pack;
 	int err;
 
@@ -105,14 +114,19 @@ static int verify_one_pack(const char *path, int verbose)
 		return error("packfile %s not found.", arg);
 
 	install_packed_git(pack);
-	err = verify_pack(pack);
 
-	if (verbose) {
+	if (!stat_only)
+		err = verify_pack(pack);
+	else
+		err = open_pack_index(pack);
+
+	if (verbose || stat_only) {
 		if (err)
 			printf("%s: bad\n", pack->pack_name);
 		else {
-			show_pack_info(pack);
-			printf("%s: ok\n", pack->pack_name);
+			show_pack_info(pack, flags);
+			if (!stat_only)
+				printf("%s: ok\n", pack->pack_name);
 		}
 	}
 
@@ -120,17 +134,20 @@ static int verify_one_pack(const char *path, int verbose)
 }
 
 static const char * const verify_pack_usage[] = {
-	"git verify-pack [-v|--verbose] <pack>...",
+	"git verify-pack [-v|--verbose] [-s|--stat-only] <pack>...",
 	NULL
 };
 
 int cmd_verify_pack(int argc, const char **argv, const char *prefix)
 {
 	int err = 0;
-	int verbose = 0;
+	unsigned int flags = 0;
 	int i;
 	const struct option verify_pack_options[] = {
-		OPT__VERBOSE(&verbose),
+		OPT_BIT('v', "verbose", &flags, "verbose",
+			VERIFY_PACK_VERBOSE),
+		OPT_BIT('s', "stat-only", &flags, "show statistics only",
+			VERIFY_PACK_STAT_ONLY),
 		OPT_END()
 	};
 
@@ -140,7 +157,7 @@ int cmd_verify_pack(int argc, const char **argv, const char *prefix)
 	if (argc < 1)
 		usage_with_options(verify_pack_usage, verify_pack_options);
 	for (i = 0; i < argc; i++) {
-		if (verify_one_pack(argv[i], verbose))
+		if (verify_one_pack(argv[i], flags))
 			err = 1;
 		discard_revindex();
 	}
-- 
1.6.4.151.g786db

Re: [PATCH 2/3] verify-pack -v: do not report "chain length 0"

[Jeff King]

On Fri, Aug 07, 2009 at 08:36:33PM -0700, Junio C Hamano wrote:

> +	if (baseobjects)
> +		printf("non delta: %lu object%s\n",
> +		       baseobjects, baseobjects > 1 ? "s" : "");

Nit: this should be "non-delta" or "no delta".

-Peff