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 <gitster@pobox.com>

diff --git a/builtin/verify-pack.c b/builtin/verify-pack.c
index b6079ae6cb03c7f3112c6eebc8c9a012d690a125..e841b4a38d2b47c39d95683a5747cbc19da1f48b 100644 (file)
--- 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 08a902210c6374c65f94a1238d895a65c7cde43a..edea69e6a6b80dd104468cde5fe123d92e4918a0 100644 (file)
--- 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 d949b35c3308ae5c06774da7f4fecdc84778d5eb..ca87e3d51b7eda8a43fbce8f9c014167ec0c0ecb 100644 (file)
--- 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 76bcaca9889b701f1fe490421be977b5e5875f1d..f8fa92446cfc46309468b4ecf142b74b1a812985 100755 (executable)
--- 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/* &&