[PATCH 0/5] fsck updates

[PATCH 0/5] fsck updates

[Junio C Hamano]

These go on top of the previous two patches I sent earlier.

Junio C Hamano (5):
  pack-check.c: minor formatting fix to match coding style
  verify_pack(): allow a quicker verification for a pack with version 2 idx
  verify-pack: add --quick
  fsck: three levels of validation
  [squash] fsck: revert --quick to the default and introduce --medium

 builtin-fsck.c        |   13 ++++++++++---
 builtin-verify-pack.c |   10 +++++++---
 http-push.c           |    2 +-
 http-walker.c         |    2 +-
 pack-check.c          |   36 ++++++++++++++++++++++++------------
 pack.h                |    5 ++++-
 6 files changed, 47 insertions(+), 21 deletions(-)

[PATCH 1/5] pack-check.c: minor formatting fix to match coding style

[Junio C Hamano]

Adjust misaligned columns and multi-line comments that violate our coding
style before touching this file.

Also fix an obvious typo.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 pack-check.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/pack-check.c b/pack-check.c
index 90c33b1..2c5f521 100644
--- a/pack-check.c
+++ b/pack-check.c
@@ -4,7 +4,7 @@
 
 struct idx_entry
 {
-	off_t                offset;
+	off_t offset;
 	const unsigned char *sha1;
 	unsigned int nr;
 };
@@ -43,7 +43,7 @@ int check_pack_crc(struct packed_git *p, struct pack_window **w_curs,
 }
 
 static int verify_packfile(struct packed_git *p,
-		struct pack_window **w_curs)
+			   struct pack_window **w_curs)
 {
 	off_t index_size = p->index_size;
 	const unsigned char *index_base = p->index_data;
@@ -54,7 +54,8 @@ static int verify_packfile(struct packed_git *p,
 	int err = 0;
 	struct idx_entry *entries;
 
-	/* Note that the pack header checks are actually performed by
+	/*
+	 * Note that the pack header checks are actually performed by
 	 * use_pack when it first opens the pack file.  If anything
 	 * goes wrong during those checks then the call will die out
 	 * immediately.
@@ -72,21 +73,23 @@ static int verify_packfile(struct packed_git *p,
 	git_SHA1_Final(sha1, &ctx);
 	pack_sig = use_pack(p, w_curs, pack_sig_ofs, NULL);
 	if (hashcmp(sha1, pack_sig))
-		err = error("%s SHA1 checksum mismatch",
-			    p->pack_name);
+		err = error("%s SHA1 checksum mismatch", p->pack_name);
 	if (hashcmp(index_base + index_size - 40, pack_sig))
-		err = error("%s SHA1 does not match its inddex",
-			    p->pack_name);
+		err = error("%s SHA1 does not match its index", p->pack_name);
 	unuse_pack(w_curs);
 
-	/* Make sure everything reachable from idx is valid.  Since we
+	/*
+	 * Make sure everything reachable from idx is valid.  Since we
 	 * have verified that nr_objects matches between idx and pack,
 	 * we do not do scan-streaming check on the pack file.
 	 */
 	nr_objects = p->num_objects;
 	entries = xmalloc((nr_objects + 1) * sizeof(*entries));
 	entries[nr_objects].offset = pack_sig_ofs;
-	/* first sort entries by pack offset, since unpacking them is more efficient that way */
+	/*
+	 * First sort entries by pack offset, since unpacking them is more
+	 * efficient that way.
+	 */
 	for (i = 0; i < nr_objects; i++) {
 		entries[i].sha1 = nth_packed_object_sha1(p, i);
 		if (!entries[i].sha1)
-- 
1.6.1.2.312.g5be3c

[PATCH 2/5] verify_pack(): allow a quicker verification for a pack with version 2 idx

[Junio C Hamano]

This adds an extra flag parameter to verify_pack() so that the caller can
affect what kind of checks are performed.

For a packfile, we always validate its whole SHA-1 checksum and the pack
header.  By default, we make sure that we can unpack all the objects in
the packfile, and the unpacked data actually matches their object name.
In addition, for a pack with version 2 idx that has per-object CRC, we
verify their CRC checksum match what is recorded.

By specifying VERIFY_PACK_QUICK, you can omit the expensive per-object
unpacking and object name validation. Per-object CRC check that is done
for objects in a pack with version 2 idx file is still performed as it is
inexpensive.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-fsck.c        |    2 +-
 builtin-verify-pack.c |    2 +-
 http-push.c           |    2 +-
 http-walker.c         |    2 +-
 pack-check.c          |   17 +++++++++++++----
 pack.h                |    5 ++++-
 6 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/builtin-fsck.c b/builtin-fsck.c
index 64dffa5..8dc7881 100644
--- a/builtin-fsck.c
+++ b/builtin-fsck.c
@@ -614,7 +614,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
 		prepare_packed_git();
 		for (p = packed_git; p; p = p->next)
 			/* verify gives error messages itself */
-			verify_pack(p);
+			verify_pack(p, 0);
 
 		for (p = packed_git; p; p = p->next) {
 			uint32_t j, num;
diff --git a/builtin-verify-pack.c b/builtin-verify-pack.c
index 25a29f1..42ae406 100644
--- a/builtin-verify-pack.c
+++ b/builtin-verify-pack.c
@@ -93,7 +93,7 @@ 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);
+	err = verify_pack(pack, 0);
 
 	if (verbose) {
 		if (err)
diff --git a/http-push.c b/http-push.c
index 59037df..0cd2926 100644
--- a/http-push.c
+++ b/http-push.c
@@ -809,7 +809,7 @@ static void finish_request(struct transfer_request *request)
 					lst = &((*lst)->next);
 				*lst = (*lst)->next;
 
-				if (!verify_pack(target))
+				if (!verify_pack(target, 0))
 					install_packed_git(target);
 				else
 					remote->can_update_info_refs = 0;
diff --git a/http-walker.c b/http-walker.c
index 0dbad3c..7f314b0 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -797,7 +797,7 @@ static int fetch_pack(struct walker *walker, struct alt_base *repo, unsigned cha
 		lst = &((*lst)->next);
 	*lst = (*lst)->next;
 
-	if (verify_pack(target))
+	if (verify_pack(target, 0))
 		return -1;
 	install_packed_git(target);
 
diff --git a/pack-check.c b/pack-check.c
index 2c5f521..256370c 100644
--- a/pack-check.c
+++ b/pack-check.c
@@ -42,8 +42,8 @@ int check_pack_crc(struct packed_git *p, struct pack_window **w_curs,
 	return data_crc != ntohl(*index_crc);
 }
 
-static int verify_packfile(struct packed_git *p,
-			   struct pack_window **w_curs)
+static int verify_packfile(struct packed_git *p, struct pack_window **w_curs,
+			   unsigned flag)
 {
 	off_t index_size = p->index_size;
 	const unsigned char *index_base = p->index_data;
@@ -78,6 +78,12 @@ static int verify_packfile(struct packed_git *p,
 		err = error("%s SHA1 does not match its index", p->pack_name);
 	unuse_pack(w_curs);
 
+	if ((flag & VERIFY_PACK_QUICK) && p->index_version <= 1) {
+		warning("contents not checked for %s as its .idx does not have CRC",
+			p->pack_name);
+		return err;
+	}
+
 	/*
 	 * Make sure everything reachable from idx is valid.  Since we
 	 * have verified that nr_objects matches between idx and pack,
@@ -114,6 +120,9 @@ static int verify_packfile(struct packed_git *p,
 					    sha1_to_hex(entries[i].sha1),
 					    p->pack_name, (uintmax_t)offset);
 		}
+
+		if (flag & VERIFY_PACK_QUICK)
+			continue;
 		data = unpack_entry(p, entries[i].offset, &type, &size);
 		if (!data) {
 			err = error("cannot unpack %s from %s at offset %"PRIuMAX"",
@@ -134,7 +143,7 @@ static int verify_packfile(struct packed_git *p,
 	return err;
 }
 
-int verify_pack(struct packed_git *p)
+int verify_pack(struct packed_git *p, unsigned flag)
 {
 	off_t index_size;
 	const unsigned char *index_base;
@@ -157,7 +166,7 @@ int verify_pack(struct packed_git *p)
 			    p->pack_name);
 
 	/* Verify pack file */
-	err |= verify_packfile(p, &w_curs);
+	err |= verify_packfile(p, &w_curs, flag);
 	unuse_pack(&w_curs);
 
 	return err;
diff --git a/pack.h b/pack.h
index a883334..b9e381c 100644
--- a/pack.h
+++ b/pack.h
@@ -57,7 +57,10 @@ struct pack_idx_entry {
 
 extern char *write_idx_file(char *index_name, struct pack_idx_entry **objects, int nr_objects, unsigned char *sha1);
 extern int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offset, off_t len, unsigned int nr);
-extern int verify_pack(struct packed_git *);
+
+#define VERIFY_PACK_QUICK 01
+extern int verify_pack(struct packed_git *, unsigned flag);
+
 extern void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t, unsigned char *, off_t);
 extern char *index_pack_lockfile(int fd);
 
-- 
1.6.1.2.312.g5be3c

[PATCH 3/5] verify-pack: add --quick

[Junio C Hamano]

This teaches "git verify-pack" a new option --quick, to trigger the
VERIFY_PACK_QUICK option.

"git fsck" is a more familiar command, and it checks more things at once
than "git verify-pack", and the case you do want to use the latter is when
really want a deep verification of a single pack.  For this reason, I do
not think anybody would want to actually use this option, but it was an
easy way to benchmark the change.

For a 32M packfile from the git repository on my AMD64X2:

    $ /usr/bin/time ./git-verify-pack $THE_PACKFILE
    22.16user 0.18system 0:22.35elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k
    0inputs+0outputs (0major+44166minor)pagefaults 0swaps

    $ /usr/bin/time ./git-verify-pack --quick $THE_PACKFILE
    0.43user 0.02system 0:00.45elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k
    0inputs+0outputs (0major+11569minor)pagefaults 0swaps

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

diff --git a/builtin-verify-pack.c b/builtin-verify-pack.c
index 42ae406..fdc04d1 100644
--- a/builtin-verify-pack.c
+++ b/builtin-verify-pack.c
@@ -54,12 +54,13 @@ static void show_pack_info(struct packed_git *p)
 		       chain_histogram[0], chain_histogram[0] > 1 ? "s" : "");
 }
 
-static int verify_one_pack(const char *path, int verbose)
+static int verify_one_pack(const char *path, int verbose, int quick)
 {
 	char arg[PATH_MAX];
 	int len;
 	struct packed_git *pack;
 	int err;
+	unsigned flag = (quick ? VERIFY_PACK_QUICK : 0);
 
 	len = strlcpy(arg, path, PATH_MAX);
 	if (len >= PATH_MAX)
@@ -93,7 +94,7 @@ 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, 0);
+	err = verify_pack(pack, flag);
 
 	if (verbose) {
 		if (err)
@@ -112,6 +113,7 @@ static const char verify_pack_usage[] = "git-verify-pack [-v] <pack>...";
 int cmd_verify_pack(int argc, const char **argv, const char *prefix)
 {
 	int err = 0;
+	int quick = 0;
 	int verbose = 0;
 	int no_more_options = 0;
 	int nothing_done = 1;
@@ -121,13 +123,15 @@ int cmd_verify_pack(int argc, const char **argv, const char *prefix)
 		if (!no_more_options && argv[1][0] == '-') {
 			if (!strcmp("-v", argv[1]))
 				verbose = 1;
+			else if (!strcmp("--quick", argv[1]))
+				quick = 1;
 			else if (!strcmp("--", argv[1]))
 				no_more_options = 1;
 			else
 				usage(verify_pack_usage);
 		}
 		else {
-			if (verify_one_pack(argv[1], verbose))
+			if (verify_one_pack(argv[1], verbose, quick))
 				err = 1;
 			discard_revindex();
 			nothing_done = 0;
-- 
1.6.1.2.312.g5be3c

[PATCH 4/5] fsck: three levels of validation

[Junio C Hamano]

Traditionally, "git fsck" only checked loose objects and "git fsck --full"
checked both loose and packed objects fully.

This introduces a new medium level of validation that checks loose objects
and runs the quicker packfile validation introduced earlier (in other
words, the individual objects in packfiles are not validated, but we still
make sure that the packfiles themselves are sound and individual objects
pass their own CRC checks when the pack has version 2 idx file), and makes
this new mode the default.  The original "loose objects only" mode can be
asked with --quick option.

In my private git project repository (almost fully packed but with may
dangling objects) on my AMD 64X2:

    $ /usr/bin/time ./git-fsck --quick
    0.18user 0.02system 0:00.20elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k
    0inputs+0outputs (0major+3205minor)pagefaults 0swaps

    $ /usr/bin/time ./git-fsck
    68.61user 0.43system 1:09.06elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k
    0inputs+0outputs (0major+116707minor)pagefaults 0swaps

    $ /usr/bin/time ./git-fsck --full
    90.36user 0.61system 1:31.00elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k
    0inputs+0outputs (0major+169641minor)pagefaults 0swaps

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-fsck.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/builtin-fsck.c b/builtin-fsck.c
index 8dc7881..72bf33b 100644
--- a/builtin-fsck.c
+++ b/builtin-fsck.c
@@ -20,6 +20,7 @@ static int show_tags;
 static int show_unreachable;
 static int include_reflogs = 1;
 static int check_full;
+static int check_quick;
 static int check_strict;
 static int keep_cache_objects;
 static unsigned char head_sha1[20];
@@ -576,7 +577,8 @@ static struct option fsck_opts[] = {
 	OPT_BOOLEAN(0, "root", &show_root, "report root nodes"),
 	OPT_BOOLEAN(0, "cache", &keep_cache_objects, "make index objects head nodes"),
 	OPT_BOOLEAN(0, "reflogs", &include_reflogs, "make reflogs head nodes (default)"),
-	OPT_BOOLEAN(0, "full", &check_full, "also consider alternate objects"),
+	OPT_BOOLEAN(0, "full", &check_full, "fully check packs"),
+	OPT_BOOLEAN(0, "quick", &check_quick, "only check loose objects"),
 	OPT_BOOLEAN(0, "strict", &check_strict, "enable more strict checking"),
 	OPT_BOOLEAN(0, "lost-found", &write_lost_and_found,
 				"write dangling objects in .git/lost-found"),
@@ -587,10 +589,14 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
 {
 	int i, heads;
 	struct alternate_object_database *alt;
+	unsigned verify_pack_flag;
 
 	errors_found = 0;
 
 	argc = parse_options(argc, argv, fsck_opts, fsck_usage, 0);
+	if (check_full && check_quick)
+		die("--full and --quick?  which one do you want?");
+
 	if (write_lost_and_found) {
 		check_full = 1;
 		include_reflogs = 0;
@@ -608,13 +614,14 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
 		fsck_object_dir(namebuf);
 	}
 
-	if (check_full) {
+	if (!check_quick) {
 		struct packed_git *p;
 
+		verify_pack_flag = check_full ? 0 : VERIFY_PACK_QUICK;
 		prepare_packed_git();
 		for (p = packed_git; p; p = p->next)
 			/* verify gives error messages itself */
-			verify_pack(p, 0);
+			verify_pack(p, verify_pack_flag);
 
 		for (p = packed_git; p; p = p->next) {
 			uint32_t j, num;
-- 
1.6.1.2.312.g5be3c

[PATCH 5/5] [squash] fsck: revert --quick to the default and introduce --medium

[Junio C Hamano]

(This commit log message is designed to replace the previous one when
patch text is squashed into the previous one).

Traditionally, "git fsck" only checked loose objects and "git fsck --full"
checked both loose and packed objects fully.

This introduces a new medium level of validation that checks loose objects
and runs the quicker packfile validation introduced earlier (in other
words, the individual objects in packfiles are not validated, but we still
make sure that the packfiles themselves are sound and individual objects
pass their own CRC checks when the pack has version 2 idx file).

I hoped that the new "medium" level to give a cursory look at the pack
data would be acceptable as the default, but given the benchmark data,
I think it is a bit too slow to be the default.  So you pass --medium
to trigger this new mode, instead of --full.

In my private git project repository (almost fully packed but with may
dangling objects) on my AMD 64X2:

    $ /usr/bin/time ./git-fsck
    0.18user 0.02system 0:00.20elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k
    0inputs+0outputs (0major+3205minor)pagefaults 0swaps

    $ /usr/bin/time ./git-fsck --medium
    68.61user 0.43system 1:09.06elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k
    0inputs+0outputs (0major+116707minor)pagefaults 0swaps

    $ /usr/bin/time ./git-fsck --full
    90.36user 0.61system 1:31.00elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k
    0inputs+0outputs (0major+169641minor)pagefaults 0swaps

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-fsck.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin-fsck.c b/builtin-fsck.c
index 72bf33b..83faa98 100644
--- a/builtin-fsck.c
+++ b/builtin-fsck.c
@@ -20,7 +20,7 @@ static int show_tags;
 static int show_unreachable;
 static int include_reflogs = 1;
 static int check_full;
-static int check_quick;
+static int check_medium;
 static int check_strict;
 static int keep_cache_objects;
 static unsigned char head_sha1[20];
@@ -578,7 +578,7 @@ static struct option fsck_opts[] = {
 	OPT_BOOLEAN(0, "cache", &keep_cache_objects, "make index objects head nodes"),
 	OPT_BOOLEAN(0, "reflogs", &include_reflogs, "make reflogs head nodes (default)"),
 	OPT_BOOLEAN(0, "full", &check_full, "fully check packs"),
-	OPT_BOOLEAN(0, "quick", &check_quick, "only check loose objects"),
+	OPT_BOOLEAN(0, "medium", &check_medium, "also check packs"),
 	OPT_BOOLEAN(0, "strict", &check_strict, "enable more strict checking"),
 	OPT_BOOLEAN(0, "lost-found", &write_lost_and_found,
 				"write dangling objects in .git/lost-found"),
@@ -594,8 +594,8 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
 	errors_found = 0;
 
 	argc = parse_options(argc, argv, fsck_opts, fsck_usage, 0);
-	if (check_full && check_quick)
-		die("--full and --quick?  which one do you want?");
+	if (check_full && check_medium)
+		die("--full and --medium?  which one do you want?");
 
 	if (write_lost_and_found) {
 		check_full = 1;
@@ -614,7 +614,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
 		fsck_object_dir(namebuf);
 	}
 
-	if (!check_quick) {
+	if (check_full || check_medium) {
 		struct packed_git *p;
 
 		verify_pack_flag = check_full ? 0 : VERIFY_PACK_QUICK;
-- 
1.6.1.2.312.g5be3c

Re: [PATCH 5/5] [squash] fsck: revert --quick to the default and introduce --medium

[Kjetil Barvik]

Junio C Hamano <gitster@pobox.com> writes:
<snipp>
> diff --git a/builtin-fsck.c b/builtin-fsck.c
> index 72bf33b..83faa98 100644
> --- a/builtin-fsck.c
> +++ b/builtin-fsck.c
> @@ -20,7 +20,7 @@ static int show_tags;
>  static int show_unreachable;
>  static int include_reflogs = 1;
>  static int check_full;
> -static int check_quick;
> +static int check_medium;
>  static int check_strict;
>  static int keep_cache_objects;
>  static unsigned char head_sha1[20];
> @@ -578,7 +578,7 @@ static struct option fsck_opts[] = {
>  	OPT_BOOLEAN(0, "cache", &keep_cache_objects, "make index objects head nodes"),
>  	OPT_BOOLEAN(0, "reflogs", &include_reflogs, "make reflogs head nodes (default)"),
>  	OPT_BOOLEAN(0, "full", &check_full, "fully check packs"),
> -	OPT_BOOLEAN(0, "quick", &check_quick, "only check loose objects"),
> +	OPT_BOOLEAN(0, "medium", &check_medium, "also check packs"),
>  	OPT_BOOLEAN(0, "strict", &check_strict, "enable more strict checking"),
>  	OPT_BOOLEAN(0, "lost-found", &write_lost_and_found,
>  				"write dangling objects in .git/lost-found"),
> @@ -594,8 +594,8 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
>  	errors_found = 0;
>  
>  	argc = parse_options(argc, argv, fsck_opts, fsck_usage, 0);
> -	if (check_full && check_quick)
> -		die("--full and --quick?  which one do you want?");
> +	if (check_full && check_medium)
> +		die("--full and --medium?  which one do you want?");
>  

  Would it not be better to have an "OPT_INT" argument named
  "check_level" or similar?

  Then you do not need those error checks, and people would maybe
  better understand the differences between the different check's?

  The documentation could then say that (for example) "level 2
  includes all check's that level 1 includes, in addition to ...".

  -- kjetil

Re: [PATCH 1/5] pack-check.c: minor formatting fix to match coding style

[Nanako Shiraishi]

Quoting Junio C Hamano <gitster@pobox.com>:

> Adjust misaligned columns and multi-line comments that violate our coding
> style before touching this file.
>
> Also fix an obvious typo.

What typo did you fix?

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: [PATCH 1/5] pack-check.c: minor formatting fix to match coding style

[Marius Storm-Olsen]

Nanako Shiraishi said the following on 31.01.2009 22:45:
> Quoting Junio C Hamano <gitster@pobox.com>:
> 
>> Adjust misaligned columns and multi-line comments that violate our coding
>> style before touching this file.
>>
>> Also fix an obvious typo.
> 
> What typo did you fix?

-		err = error("%s SHA1 does not match its inddex",
+		err = error("%s SHA1 does not match its index",

--
.marius

Re: [PATCH 1/5] pack-check.c: minor formatting fix to match coding style

[Nanako Shiraishi]

Quoting Marius Storm-Olsen <marius@trolltech.com>:

>Nanako Shiraishi said the following on 31.01.2009 22:45:
>> What typo did you fix?
>
>-		err = error("%s SHA1 does not match its inddex",
>+		err = error("%s SHA1 does not match its index",

Ah, you are right. Sorry for the noise.

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/