Re: [PATCH] {fetch,receive}-pack: skip sha-1 integrity test on objects from new pack
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 14, 2012, 19:05 UTC
- Message-ID
- <7v4ntrq9z5.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1331736055-21019-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Another try. Exclude all objects reachable from refs and the new pack > from --verify-objects tests.
Thanks. Looks much nicer in general.
> I keep CHECK_CONNECT_QUIET change because it seems a good change > anyway.
It appears that you lost the "one int for a single boolean to flag" conversion that was the point of CHECK_CONNECT_QUIET symbol.
Show 29 quoted lines
> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index 4c4d404..21d714b 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -180,8 +180,24 @@ static void finish_object(struct object *obj,
> struct rev_list_info *info = cb_data;
> if (obj->type == OBJ_BLOB && !has_sha1_file(obj->sha1))
> die("missing blob object '%s'", sha1_to_hex(obj->sha1));
> - if (info->revs->verify_objects && !obj->parsed && obj->type != OBJ_COMMIT)
> - parse_object(obj->sha1);
> + if (info->revs->verify_objects &&
> + !obj->parsed && obj->type != OBJ_COMMIT) {
> + const char *safe_pack = info->revs->safe_pack;
> + struct object_info oi;
> + int safe = 0;
> + memset(&oi, 0, sizeof(oi));
> + if (*safe_pack &&
> + sha1_object_info_extended(obj->sha1, &oi) >= 0 &&
> + oi.whence == OI_PACKED) {
> + const char *pack = oi.u.packed.pack->pack_name;
> + int len = strlen(pack);
> + assert(strncmp(pack + len - 51, "/pack-", 6) == 0);
> + assert(strcmp(pack + len - 5, ".pack") == 0);
> + safe = !memcmp(safe_pack, pack + len - 45, 40);
> + }
> + if (!safe)
> + parse_object(obj->sha1);
> + }
> }This looks unnecessarily complex, and I think the complexity comes only because the new info->revs->safe_pack is a hextual packfile ID. Wouldn't it make more sense to store a pointer to "struct packed_git" there, so that oi.u.packed.pack can be compared with it?
The caller may need a new API in sha1_file.c to let it find a packed git with a given packfile ID.
Show 23 quoted lines
> diff --git a/connected.c b/connected.c
> index d762423..af81049 100644
> --- a/connected.c
> +++ b/connected.c
> @@ -14,28 +14,43 @@
> *
> * Returns 0 if everything is connected, non-zero otherwise.
> */
> -int check_everything_connected(sha1_iterate_fn fn, int quiet, void *cb_data)
> +int check_everything_connected(sha1_iterate_fn fn, unsigned int flags,
> + const char *pack_lockfile, void *cb_data)
> {
> ...
> + if (pack_lockfile) {
> + strbuf_addstr(&packfile, pack_lockfile);
> + /* xxx/pack-%40s.keep */
> + assert(strcmp(packfile.buf + packfile.len - 5, ".keep") == 0);
> + assert(strncmp(packfile.buf + packfile.len - 51, "/pack-", 6) == 0);
> + strbuf_setlen(&packfile, packfile.len - 5);
> + strbuf_remove(&packfile, 0, packfile.len - 40);
> + argv[ac++] = "--safe-pack";
> + argv[ac++] = packfile.buf;
> + }That looks like a very round-about way. Why not check the ".keep" and "/pack-" substrings in pack_lockfile (as a side effect of doing so, you will learn the offset of the 40-byte substring you want to put in the packfile strbuf) and strbuf_add(&packfile, pack_lockfile + offset, 40)?
> + assert(ac < ARRAY_SIZE(argv) && argv[ac] == NULL);
Perhaps you want to use argv_list API instead of adding an assert() here.
Show 13 quoted lines
> diff --git a/revision.c b/revision.c
> index 819ff01..1c2d017 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1451,6 +1451,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
> revs->tree_objects = 1;
> revs->blob_objects = 1;
> revs->verify_objects = 1;
> + } else if (!strcmp(arg, "--safe-pack")) {
> + if (strlen(argv[1]) != 40)
> + die("--safe-pack requires an SHA-1 as pack id, not %s", argv[1]);
> + strcpy(revs->safe_pack, argv[1]);
> + return 2;The condition for the above "die()" does not check if that random 40-byte string is an SHA-1, let alone it names an existing packfile. It would be solved automatically if you change the type of revs.safe_pack to a pointer to "struct packed_git", though.