{"thread":{"id":"7710","subject":"[PATCH] Make sure quickfetch is not fooled with a previous, incomplete fetch.","startedAt":"2007-04-17T06:40:01Z","lastAt":"2007-04-17T06:58:53Z","messageCount":2,"participants":["Junio C Hamano","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"39613","messageId":"11767920013264-git-send-email-junkio@cox.net","threadId":"7710","inReplyTo":null,"subject":"[PATCH] Make sure quickfetch is not fooled with a previous, incomplete fetch.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-17T06:40:01Z","receivedAt":"2007-04-17T06:40:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This adds a new option --check-blob to rev-list command (and it\neven documents it ;-), and uses it to make sure the quick-fetch\noptimization we introduced earlier is not fooled by a previous\nincomplete fetch.\n\nThe quick-fetch optimization works by running this command:\n\n\tgit rev-list --objects <<commit-list>> --not --all\n\nwhere <<commit-list>> is a list of commits that we are going to\nfetch from the other side.  If there is any object missing to\ncomplete the <<commit-list>>, the rev-list would fail and die\n(say, the commit was in our repository, but its tree wasn't --\nthen it will barf while trying to list the blobs the tree\ncontains because it cannot read that tree).\n\nUsually we do not have the objects (otherwise why would we\nfetching?), but in one important special case we do: when the\nremote repository is used as an alternate object store\n(i.e. pointed by .git/objects/info/alternates).  We could check\n.git/objects/info/alternates to see if the remote we are\ninteracting with is one of them (or is used as an alternate,\nrecursively, by one of them), but that check is more cumbersome\nthan it is worth.\n\nThe above check however does not catch missing blob, because\nusual object listing code does not read nor check blob objects,\nknowing that blobs do not contain any further references to\nother objects.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\n\nI've benched this with\n\n\tgit rev-list --objects --all >/dev/null\n\nin the kernel repository, with three different implementations\nof the \"check-blob\".\n\n - Checking with has_sha1_file() has negligible (unmeasurable)\n   performance penalty, which is what this patch has.\n\n - Checking with sha1_object_info() makes it somewhat slower,\n   perhaps by 5%.\n\n - Checking with read_sha1_file() to cause a fully re-validation\n   is prohibitively expensive (about 4 times as much runtime).\n\nI am tempted to make the version with has_sha1_file() the\ndefault, getting rid of this new --check-blob option.\n\n Documentation/git-rev-list.txt |   13 +++++-\n builtin-rev-list.c             |   10 ++++\n git-fetch.sh                   |    2 +-\n t/t5502-quickfetch.sh          |   89 ++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 112 insertions(+), 2 deletions(-)\n create mode 100755 t/t5502-quickfetch.sh\n\ndiff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt\nindex 12b71ed..3efd3d8 100644\n--- a/Documentation/git-rev-list.txt\n+++ b/Documentation/git-rev-list.txt\n@@ -24,7 +24,7 @@ SYNOPSIS\n \t     [ \\--left-right ]\n \t     [ \\--encoding[=<encoding>] ]\n \t     [ \\--(author|committer|grep)=<pattern> ]\n-\t     [ [\\--objects | \\--objects-edge] [ \\--unpacked ] ]\n+\t     [ [\\--objects | \\--objects-edge] [ \\--unpacked ] [ \\--check-blob] ]\n \t     [ \\--pretty | \\--header ]\n \t     [ \\--bisect ]\n \t     [ \\--bisect-vars ]\n@@ -335,6 +335,17 @@ These options are mostly targeted for packing of git repositories.\n \tobjects in deltified form based on objects contained in these\n \texcluded commits to reduce network traffic.\n \n+--check-blob::\n+\n+\tWhen the command lists objects contained in commits with\n+\t`--objects` (or `--objects-edge`) option, it does not\n+\tusually check if the listed blob objects exist in the\n+\trepository (this is usually not a problem, as the\n+\tcommand is typically used to dig from the existing refs,\n+\tand objects reachable from refs are by definition\n+\tcomplete).  This command makes the command additionally\n+\tensure the blobs it lists exist.\n+\n --unpacked::\n \n \tOnly useful with '--objects'; print the object IDs that are not\ndiff --git a/builtin-rev-list.c b/builtin-rev-list.c\nindex 09774f9..3404766 100644\n--- a/builtin-rev-list.c\n+++ b/builtin-rev-list.c\n@@ -44,6 +44,7 @@ static const char rev_list_usage[] =\n static struct rev_info revs;\n \n static int bisect_list;\n+static int check_blob;\n static int show_timestamp;\n static int hdr_termination;\n static const char *header_prefix;\n@@ -113,6 +114,11 @@ static void show_object(struct object_array_entry *p)\n \t * confuse downstream git-pack-objects very badly.\n \t */\n \tconst char *ep = strchr(p->name, '\\n');\n+\n+\tif (check_blob && p->item->type == OBJ_BLOB\n+\t    && !has_sha1_file(p->item->sha1))\n+\t\tdie(\"missing blob object '%s'\", sha1_to_hex(p->item->sha1));\n+\n \tif (ep) {\n \t\tprintf(\"%s %.*s\\n\", sha1_to_hex(p->item->sha1),\n \t\t       (int) (ep - p->name),\n@@ -490,6 +496,10 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t\tbisect_show_vars = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(arg, \"--check-blob\")) {\n+\t\t\tcheck_blob = 1;\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!strcmp(arg, \"--stdin\")) {\n \t\t\tif (read_from_stdin++)\n \t\t\t\tdie(\"--stdin given twice?\");\ndiff --git a/git-fetch.sh b/git-fetch.sh\nindex 832b20c..e95a9ec 100755\n--- a/git-fetch.sh\n+++ b/git-fetch.sh\n@@ -195,7 +195,7 @@ fetch_all_at_once () {\n \t\t\t# This will barf when $theirs reach an object that\n \t\t\t# we do not have in our repository.  Otherwise,\n \t\t\t# we already have everything the fetch would bring in.\n-\t\t\tgit-rev-list --objects $theirs --not --all \\\n+\t\t\tgit-rev-list --objects --check-blob $theirs --not --all \\\n \t\t\t\t>/dev/null 2>/dev/null\n \t\tthen\n \t\t\tgit-fetch--tool pick-rref \"$rref\" \"$ls_remote_result\"\ndiff --git a/t/t5502-quickfetch.sh b/t/t5502-quickfetch.sh\nnew file mode 100755\nindex 0000000..b4760f2\n--- /dev/null\n+++ b/t/t5502-quickfetch.sh\n@@ -0,0 +1,89 @@\n+#!/bin/sh\n+\n+test_description='test quickfetch from local'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\n+\ttest_tick &&\n+\techo ichi >file &&\n+\tgit add file &&\n+\tgit commit -m initial &&\n+\n+\tcnt=$( (\n+\t\tgit count-objects | sed -e \"s/ *objects,.*//\"\n+\t) ) &&\n+\ttest $cnt -eq 3\n+'\n+\n+test_expect_success 'clone without alternate' '\n+\n+\t(\n+\t\tmkdir cloned &&\n+\t\tcd cloned &&\n+\t\tgit init-db &&\n+\t\tgit remote add -f origin ..\n+\t) &&\n+\tcnt=$( (\n+\t\tcd cloned &&\n+\t\tgit count-objects | sed -e \"s/ *objects,.*//\"\n+\t) ) &&\n+\ttest $cnt -eq 3\n+'\n+\n+test_expect_success 'further commits in the original' '\n+\n+\ttest_tick &&\n+\techo ni >file &&\n+\tgit commit -a -m second &&\n+\n+\tcnt=$( (\n+\t\tgit count-objects | sed -e \"s/ *objects,.*//\"\n+\t) ) &&\n+\ttest $cnt -eq 6\n+'\n+\n+test_expect_success 'copy commit and tree but not blob by hand' '\n+\n+\tgit rev-list --objects HEAD |\n+\tgit pack-objects --stdout |\n+\t(\n+\t\tcd cloned &&\n+\t\tgit unpack-objects\n+\t) &&\n+\n+\tcnt=$( (\n+\t\tcd cloned &&\n+\t\tgit count-objects | sed -e \"s/ *objects,.*//\"\n+\t) ) &&\n+\ttest $cnt -eq 6\n+\n+\tblob=$(git rev-parse HEAD:file | sed -e \"s|..|&/|\") &&\n+\ttest -f \"cloned/.git/objects/$blob\" &&\n+\trm -f \"cloned/.git/objects/$blob\" &&\n+\n+\tcnt=$( (\n+\t\tcd cloned &&\n+\t\tgit count-objects | sed -e \"s/ *objects,.*//\"\n+\t) ) &&\n+\ttest $cnt -eq 5\n+\n+'\n+\n+test_expect_success 'quickfetch should not leave a corrupted repository' '\n+\n+\t(\n+\t\tcd cloned &&\n+\t\tgit fetch\n+\t) &&\n+\n+\tcnt=$( (\n+\t\tcd cloned &&\n+\t\tgit count-objects | sed -e \"s/ *objects,.*//\"\n+\t) ) &&\n+\ttest $cnt -eq 6\n+\n+'\n+\n+test_done\n-- \n1.5.1.1.821.g88bdb\n"},{"id":"39614","messageId":"20070417065853.GR2229@spearce.org","threadId":"7710","inReplyTo":"11767920013264-git-send-email-junkio@cox.net","subject":"Re: [PATCH] Make sure quickfetch is not fooled with a previous, incomplete fetch.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-04-17T06:58:53Z","receivedAt":"2007-04-17T06:58:53Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano <junkio@cox.net> wrote:\n> in the kernel repository, with three different implementations\n> of the \"check-blob\".\n> \n>  - Checking with has_sha1_file() has negligible (unmeasurable)\n>    performance penalty, which is what this patch has.\n> \n>  - Checking with sha1_object_info() makes it somewhat slower,\n>    perhaps by 5%.\n> \n>  - Checking with read_sha1_file() to cause a fully re-validation\n>    is prohibitively expensive (about 4 times as much runtime).\n> \n> I am tempted to make the version with has_sha1_file() the\n> default, getting rid of this new --check-blob option.\n\nMisses in has_sha1_file suck, as we rescan for packfiles after\nfailing to find it in the current known packfiles and running a\nfailed stat().  Per object.\n\nThough if --objects aborts with an error on the first failure, that's\nperfectly reasonable performance, and actually quite sane behavior.\nSo I'm all for having --objects always imply has_sha1_file().\n\nIf the object exists enough to satisfy has_sha1_file() I think its\nsane enough to assume its safe for a fast-fetch path.  If we didn't\nhave fast-fetch implemented and we fetched <100 objects we'd use\nunpack-objects, which would only call has_sha1_file() anyway, and\nfind that possibly corrupted object in the source repository...,\noh, and that cannot happen as the source repository had to have a\ngood copy of the damn object to send it to us in the first place.\nSo has_sha1_file() is sufficient.\n\nIn my opinion, I think the --check-blob option of rev-list should\nimply doing the sha1_object_info() type test at least, if not doing\na full-up SHA-1 validation of the blob content.  Which yea, that's\nbasically most of an fsck...\n\n\nSo I'm in favor of making --objects imply has_sha1_file(), aborting\non the first false return from that, and either make --check-blob\ndo the stricter checking work, or don't define it at this time.\n\n-- \nShawn.\n"}]}