{"thread":{"id":"29735","subject":"sha-1 check in rev-list --verify-objects redundant?","startedAt":"2012-02-25T17:15:19Z","lastAt":"2012-02-27T18:41:41Z","messageCount":9,"participants":["Nguyen Thai Ngoc Duy","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"185432","messageId":"CACsJy8D_BdV14dGc2YsK91FrX8S=70DJOY3cU=oH3y41N2Ar0w@mail.gmail.com","threadId":"29735","inReplyTo":null,"subject":"sha-1 check in rev-list --verify-objects redundant?","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-25T17:15:19Z","receivedAt":"2012-02-25T17:15:19Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Hi,\n\n\"git rev-list --verify-objects\" calls parse_object() on all except\ncommits. This function in turn does check_sha1_signature() which\nrehashes object content to verify the content matches its sha-1\nsignature. This is an expensive operation. I wonder if this is a\nredundant check. --verify-objects is called to verify new packs.\nindex-pack is also (always?) called on new packs, and index-pack\nhashes all object content, saves the hashes as signature in pack index\nfile. So they must match. Am I missing something here?\n-- \nDuy\n"},{"id":"185450","messageId":"7vk43af14m.fsf@alter.siamese.dyndns.org","threadId":"29735","inReplyTo":"CACsJy8D_BdV14dGc2YsK91FrX8S=70DJOY3cU=oH3y41N2Ar0w@mail.gmail.com","subject":"Re: sha-1 check in rev-list --verify-objects redundant?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-26T08:30:01Z","receivedAt":"2012-02-26T08:30:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n> ... I wonder if this is a\n> redundant check. --verify-objects is called to verify new packs.\n\nI do not think --verify-objects does not have anything to do with\nverifying the integrity of packs, whether new or old.\n\nThe check is about the integrity of the *history* we _might_ already have\non our side, when we find ourselves wanting to fetch up to a commit $X,\nwhose reachability from the tips of our refs (i.e. the objects that are\nguaranteed to be present in our repository) is unknown, and we somehow\nalready have the commit $X itself in the repository.\n\nWe cannot just declare victory upon seeing commit $X and omit fetching the\nhistory leading to the commit, because we may or may not have its parent\ncommit object, or the tree object that is recorded in it (it may be that\nwe killed an HTTP walker after we fetched $X but not its parents or\ntrees). We need to walk back from $X until we hit one of the tips of our\nrefs, and while doing so, we also need to make sure the trees and blobs\nreferenced from the walked commits are also healthy.\n\nAs 5a48d24 (rev-list --verify-object, 2011-09-01) explains, we used to do\nthis with --objects instead, but that check does not even make sure blobs\nexist [*1*] let alone checking to see if these blobs were healthy. The\nwhole point of using --verify-objects instead of --objects is to make sure\nthat we do not miss blob objects.\n\n\n[Footnote]\n\n*1* The --objects code reads the commits and trees in order to _list_\nobjects to the blob level, so implicitly, it validates that commits and\ntrees reachable from the commit $X we happened to have in our repository,\nrelying on the fact that we would error out if we fail to read them.\n"},{"id":"185452","messageId":"7v8vjqez6v.fsf@alter.siamese.dyndns.org","threadId":"29735","inReplyTo":"7vk43af14m.fsf@alter.siamese.dyndns.org","subject":"Re: sha-1 check in rev-list --verify-objects redundant?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-26T09:11:52Z","receivedAt":"2012-02-26T09:11:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Nguyen Thai Ngoc Duy writes:\n>\n>> ... I wonder if this is a\n>> redundant check. --verify-objects is called to verify new packs.\n>\n> I do not think --verify-objects does not have anything to do with\n> verifying the integrity of packs, whether new or old.\n\nA typo/grammo that should be obvious from the context, sorry. The above\nshould be \"I do not think --verify-objects has anything to do with ...\"\n"},{"id":"185453","messageId":"CACsJy8BUeedTZSq_ay=JmqUt3wrnm6n1eOcFt0WPkEo2B-1zwA@mail.gmail.com","threadId":"29735","inReplyTo":"7vk43af14m.fsf@alter.siamese.dyndns.org","subject":"Re: sha-1 check in rev-list --verify-objects redundant?","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-26T11:11:30Z","receivedAt":"2012-02-26T11:11:30Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Feb 26, 2012 at 3:30 PM, Junio C Hamano wrote:\n> Nguyen Thai Ngoc Duy writes:\n>\n>> ... I wonder if this is a\n>> redundant check. --verify-objects is called to verify new packs.\n>\n> I do not think --verify-objects does not have anything to do with\n> verifying the integrity of packs, whether new or old.\n>\n> The check is about the integrity of the *history* we _might_ already have\n> on our side, when we find ourselves wanting to fetch up to a commit $X,\n> whose reachability from the tips of our refs (i.e. the objects that are\n> guaranteed to be present in our repository) is unknown, and we somehow\n> already have the commit $X itself in the repository.\n>\n> We cannot just declare victory upon seeing commit $X and omit fetching the\n> history leading to the commit, because we may or may not have its parent\n> commit object, or the tree object that is recorded in it (it may be that\n> we killed an HTTP walker after we fetched $X but not its parents or\n> trees).  We need to walk back from $X until we hit one of the tips of our\n> refs, and while doing so, we also need to make sure the trees and blobs\n> referenced from the walked commits are also healthy.\n>\n> As 5a48d24 (rev-list --verify-object, 2011-09-01) explains, we used to do\n> this with --objects instead, but that check does not even make sure blobs\n> exist [*1*] let alone checking to see if these blobs were healthy.  The\n> whole point of using --verify-objects instead of --objects is to make sure\n> that we do not miss blob objects.\n\n\"rev-list --objects\" does check for blob existence, in finish_object().\n\nOn the well-formedness, unless I'm mistaken, --verify-objects is\n_always_ used in conjunction with index-pack. --verify-object is not\neven documented. index-pack makes sure all (new) object signatures\nreflect their content. Commit and tree content are validated by\nrev-list walking them. So at least when --verify-objects is used with\nindex-pack, I don't see the point rehashing every new object _again_.\n\n> [Footnote]\n>\n> *1* The --objects code reads the commits and trees in order to _list_\n> objects to the blob level, so implicitly, it validates that commits and\n> trees reachable from the commit $X we happened to have in our repository,\n> relying on the fact that we would error out if we fail to read them.\n\n\n\n-- \nDuy\n"},{"id":"185469","messageId":"7v1uphff8o.fsf@alter.siamese.dyndns.org","threadId":"29735","inReplyTo":"CACsJy8BUeedTZSq_ay=JmqUt3wrnm6n1eOcFt0WPkEo2B-1zwA@mail.gmail.com","subject":"Re: sha-1 check in rev-list --verify-objects redundant?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-26T21:37:27Z","receivedAt":"2012-02-26T21:37:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n> On the well-formedness, unless I'm mistaken, --verify-objects is\n> _always_ used in conjunction with index-pack.\n\nHmm, you are making my head hurt. Is the above \"always\" a typo of\n\"never\"?\n\nThe static check_everything_connected() function in builtin/fetch.c is a\ndirect callsite of \"rev-list --verify-objects\", and the function is used\nin two codepaths:\n\n * store_updated_refs() that is used after we receive and store objects\n from the other end. We may or may not have run index-pack in this\n codepath; in either case we need to make sure the other side did send\n everything that is needed to complete the history between what we used\n to have and what they claimed to supply us, to protect us from a broken\n remote side.\n\n * quickfetch() that is called even before we get any object from the\n other end, to optimize the transfer when we already have what we need.\n\nThe latter is the original use to protect against unconnected island of\nchain I explained in the previous message, but the former is also abot the\nsame protection, in a different callchain.\n\nIn both cases, the check by --verify-objects is about completeness of the\nhistory (is everything connected to the tips of refs we have?), and is\ndifferent from integrity of individual objects (is each individual object\nwell formed and hash correctly?). Both kinds of sanity need to be\nchecked, as they are orthogonal concepts.\n\nIn order to check the history completeness, we need to read the objects\nthat we walk during the check. I wouldn't be surprised if the codepath to\ndo this is written overly defensive, taking a belt-and-suspender approach,\nand check the well-formedness of an object before it reads it to find out\nthe other objects pointed by it.\n\nIf we _know_ that we have checked the integrity of all the necessary\nindividual objects before we start reading them in order to check the\ncompleteness of the history, there is an opportunity to optimize by\nteaching --verify-objects paths to optionally be looser than it currently\nis, to avoid checking the object integrity twice.\n"},{"id":"185483","messageId":"CACsJy8C9=BJvKvFB3XrSqNXV-y3=o1pTJPtFBXK3jSheBff3Uw@mail.gmail.com","threadId":"29735","inReplyTo":"7v1uphff8o.fsf@alter.siamese.dyndns.org","subject":"Re: sha-1 check in rev-list --verify-objects redundant?","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-27T00:48:11Z","receivedAt":"2012-02-27T00:48:11Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Feb 27, 2012 at 4:37 AM, Junio C Hamano wrote:\n> Nguyen Thai Ngoc Duy writes:\n>\n>> On the well-formedness, unless I'm mistaken, --verify-objects is\n>> _always_ used in conjunction with index-pack.\n>\n> Hmm, you are making my head hurt.  Is the above \"always\" a typo of\n> \"never\"?\n>\n> The static check_everything_connected() function in builtin/fetch.c is a\n> direct callsite of \"rev-list --verify-objects\", and the function is used\n> in two codepaths:\n>\n>  * store_updated_refs() that is used after we receive and store objects\n>   from the other end.  We may or may not have run index-pack in this\n>   codepath; in either case we need to make sure the other side did send\n>   everything that is needed to complete the history between what we used\n>   to have and what they claimed to supply us, to protect us from a broken\n>   remote side.\n\nI stand corrected. --verify-objects is _usually_ used in conjunction\nwith index-pack, when the media is a pack (i.e. no remote helpers)\n\n>  * quickfetch() that is called even before we get any object from the\n>   other end, to optimize the transfer when we already have what we need.\n>\n> The latter is the original use to protect against unconnected island of\n> chain I explained in the previous message, but the former is also abot the\n> same protection, in a different callchain.\n\nI think we can trust what we already have, so in the latter case (and\nthe former when the medium is a pack), --objects should suffice.\n\n> In both cases, the check by --verify-objects is about completeness of the\n> history (is everything connected to the tips of refs we have?), and is\n> different from integrity of individual objects (is each individual object\n> well formed and hash correctly?).  Both kinds of sanity need to be\n> checked, as they are orthogonal concepts.\n>\n> In order to check the history completeness, we need to read the objects\n> that we walk during the check. I wouldn't be surprised if the codepath to\n> do this is written overly defensive, taking a belt-and-suspender approach,\n> and check the well-formedness of an object before it reads it to find out\n> the other objects pointed by it.\n>\n> If we _know_ that we have checked the integrity of all the necessary\n> individual objects before we start reading them in order to check the\n> completeness of the history, there is an opportunity to optimize by\n> teaching --verify-objects paths to optionally be looser than it currently\n> is, to avoid checking the object integrity twice.\n\nOk, will cook something. The reason I raised it is because\n--verify-objects --all on git.git could take ~1m10s, but if we assume\nobject integrity is fine and skip it, it could drop to 10s (I suspect\n--objects gives the same number).\n-- \nDuy\n"},{"id":"185484","messageId":"7vd391cbmp.fsf@alter.siamese.dyndns.org","threadId":"29735","inReplyTo":"CACsJy8C9=BJvKvFB3XrSqNXV-y3=o1pTJPtFBXK3jSheBff3Uw@mail.gmail.com","subject":"Re: sha-1 check in rev-list --verify-objects redundant?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-27T01:23:42Z","receivedAt":"2012-02-27T01:23:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n>> * quickfetch() that is called even before we get any object from the\n>> other end, to optimize the transfer when we already have what we need.\n>>\n>> The latter is the original use to protect against unconnected island of\n>> chain I explained in the previous message, but the former is also abot the\n>> same protection, in a different callchain.\n>\n> I think we can trust what we already have, so in the latter case (and\n> the former when the medium is a pack), --objects should suffice.\n\nHrm, what you seem to be missing is that in the latter case, the objects\nwe are walking to make sure the connectivity are *not* yet considered as\npart of \"what we already have\" (hence we can trust). The whole point of\nquickfetch is to turn these untrustworthy bits that happen to exist in our\nrepository into a part of trustable history without having to fetch from\nthe other side to complete them. In the former case, when we know that\npack has been indexed with recent index-pack/unpack-objects that are more\npicky (didn't Shawn and I recently tighten them?), we may be able to do\nwithout object integrity checks.\n\n>> If we _know_ that we have checked the integrity of all the necessary\n>> individual objects before we start reading them in order to check the\n>> completeness of the history, there is an opportunity to optimize by\n>> teaching --verify-objects paths to optionally be looser than it currently\n>> is, to avoid checking the object integrity twice.\n>\n> Ok, will cook something. The reason I raised it is because\n> --verify-objects --all on git.git could take ~1m10s, but if we assume\n> object integrity is fine and skip it, it could drop to 10s (I suspect\n> --objects gives the same number).\n\nThat is a big assumption that I personally do not want to sacrifice the\nintegrity of my history on. I would 100% agree with the above with s/if\nwe assume/if we know/, though.\n"},{"id":"185516","messageId":"20120227130141.GA8980@do","threadId":"29735","inReplyTo":"CACsJy8BUeedTZSq_ay=JmqUt3wrnm6n1eOcFt0WPkEo2B-1zwA@mail.gmail.com","subject":"Re: sha-1 check in rev-list --verify-objects redundant?","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-27T13:01:41Z","receivedAt":"2012-02-27T13:01:41Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Feb 26, 2012 at 06:11:30PM +0700, Nguyen Thai Ngoc Duy wrote:\n> \"rev-list --objects\" does check for blob existence, in finish_object().\n\nEck.. I think \"--quiet --verify-objects\" becomes \"--quiet --objects\"\nbecause of this code:\n\n-- 8< --\n\ttraverse_commit_list(&revs,\n\t\t\t quiet ? finish_commit : show_commit,\n\t\t\t quiet ? finish_object : show_object,\n\t\t\t &info);\n-- 8< --\n\nUnless that's intentional, shouldn't we apply this patch? --quiet's\ninterfering with rev-list's business sounds weird to me.\n\n-- 8< --\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 264e3ae..95fb605 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -172,19 +172,18 @@ static void finish_object(struct object *obj,\n \t\t\t const struct name_path *path, const char *name,\n \t\t\t void *cb_data)\n {\n+\tstruct rev_list_info *info = cb_data;\n \tif (obj->type == OBJ_BLOB && !has_sha1_file(obj->sha1))\n \t\tdie(\"missing blob object '%s'\", sha1_to_hex(obj->sha1));\n+\tif (info->revs->verify_objects && !obj->parsed && obj->type != OBJ_COMMIT)\n+\t\tparse_object(obj->sha1);\n }\n \n static void show_object(struct object *obj,\n \t\t\tconst struct name_path *path, const char *component,\n \t\t\tvoid *cb_data)\n {\n-\tstruct rev_list_info *info = cb_data;\n-\n \tfinish_object(obj, path, component, cb_data);\n-\tif (info->revs->verify_objects && !obj->parsed && obj->type != OBJ_COMMIT)\n-\t\tparse_object(obj->sha1);\n \tshow_object_with_name(stdout, obj, path, component);\n } \n-- 8< --\n-- \nDuy\n"},{"id":"185539","messageId":"7vehtg9l0a.fsf@alter.siamese.dyndns.org","threadId":"29735","inReplyTo":"20120227130141.GA8980@do","subject":"Re: sha-1 check in rev-list --verify-objects redundant?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-27T18:41:41Z","receivedAt":"2012-02-27T18:41:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n> On Sun, Feb 26, 2012 at 06:11:30PM +0700, Nguyen Thai Ngoc Duy wrote:\n>> \"rev-list --objects\" does check for blob existence, in finish_object().\n>\n> Eck.. I think \"--quiet --verify-objects\" becomes \"--quiet --objects\"\n> because of this code:\n>\n> -- 8< --\n> \ttraverse_commit_list(&revs,\n> \t\t\t quiet ? finish_commit : show_commit,\n> \t\t\t quiet ? finish_object : show_object,\n> \t\t\t &info);\n> -- 8< --\n>\n> Unless that's intentional, shouldn't we apply this patch? --quiet's\n> interfering with rev-list's business sounds weird to me.\n\nGood thinking. Anything we are missing by calling finish_* other than\nprinting is a similar bug waiting to happen.\n\nCan't we push the quiet bit in the info structure and have a single pair\nof callback functions, so that we can make sure this kind of glitch would\nnever happen?\n"}]}