{"thread":{"id":"18456","subject":"[PATCH v2 2/2] pack-objects: don't loosen objects available in alternate or kept packs","startedAt":"2009-03-21T22:26:11Z","lastAt":"2009-03-24T23:01:03Z","messageCount":7,"participants":["Brandon Casey","Junio C Hamano","Nicolas Pitre"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"108844","messageId":"ee63ef30903211526n47c40052mc40dc018f25c99fd@mail.gmail.com","threadId":"18456","inReplyTo":null,"subject":"[PATCH v2 2/2] pack-objects: don't loosen objects available in alternate or kept packs","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2009-03-21T22:26:11Z","receivedAt":"2009-03-21T22:26:11Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"If pack-objects is called with the --unpack-unreachable option then it will\nunpack (i.e. loosen) all unreferenced objects from local not-kept packs,\nincluding those that also exist in packs residing in an alternate object\ndatabase or a local kept pack. The primary(sole?) user of this option is\ngit-repack. In this case, repack will follow the call to pack-objects with\na call to prune-packed which will delete these newly loosened objects,\nmaking the act of loosening a waste of time. The unnecessary loosening can\nbe avoided by checking whether an object exists in a non-local pack or a\nlocal kept pack before loosening it.\n\nThis fixes the 'local packed unreachable obs that exist in alternate ODB\nare not loosened' test in t7700.\n\nSigned-off-by: Brandon Casey \n---\n\n\nSigned version of the previous version.\n\n\n builtin-pack-objects.c | 26 +++++++++++++++++++++++++-\n t/t7700-repack.sh | 2 +-\n 2 files changed, 26 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 6222f19..3f477c5 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -1944,6 +1944,29 @@ static void\nadd_objects_in_unpacked_packs(struct rev_info *revs)\n \tfree(in_pack.array);\n }\n\n+static int has_sha1_pack_kept_or_nonlocal(const unsigned char *sha1)\n+{\n+\tstatic struct packed_git *last_found = (void *)1;\n+\tstruct packed_git *p;\n+\n+\tp = (last_found == (void *)1) ? packed_git : last_found;\n+\n+\twhile (p) {\n+\t\tif ((!p->pack_local || p->pack_keep) &&\n+\t\t\tfind_pack_entry_one(sha1, p)) {\n+\t\t\tlast_found = p;\n+\t\t\treturn 1;\n+\t\t}\n+\t\tif (p == last_found)\n+\t\t\tp = packed_git;\n+\t\telse\n+\t\t\tp = p->next;\n+\t\tif (p == last_found)\n+\t\t\tp = p->next;\n+\t}\n+\treturn 0;\n+}\n+\n static void loosen_unused_packed_objects(struct rev_info *revs)\n {\n \tstruct packed_git *p;\n@@ -1959,7 +1982,8 @@ static void loosen_unused_packed_objects(struct\nrev_info *revs)\n\n \t\tfor (i = 0; i < p->num_objects; i++) {\n \t\t\tsha1 = nth_packed_object_sha1(p, i);\n-\t\t\tif (!locate_object_entry(sha1))\n+\t\t\tif (!locate_object_entry(sha1) &&\n+\t\t\t\t!has_sha1_pack_kept_or_nonlocal(sha1))\n \t\t\t\tif (force_object_loose(sha1, p->mtime))\n \t\t\t\t\tdie(\"unable to force loose object\");\n \t\t}\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 013e488..9ce546e 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -113,7 +113,7 @@ test_expect_success 'packed unreachable obs in\nalternate ODB are not loosened' '\n \ttest_must_fail git show $csha1\n '\n\n-test_expect_failure 'local packed unreachable obs that exist in\nalternate ODB are not loosened' '\n+test_expect_success 'local packed unreachable obs that exist in\nalternate ODB are not loosened' '\n \techo `pwd`/alt_objects > .git/objects/info/alternates &&\n \techo \"$csha1\" | git pack-objects --non-empty --all --reflog pack &&\n \trm -f .git/objects/pack/* &&\n-- \n1.6.2.12.g83676\n"},{"id":"108873","messageId":"7vbpru9nh9.fsf@gitster.siamese.dyndns.org","threadId":"18456","inReplyTo":"ee63ef30903211526n47c40052mc40dc018f25c99fd@mail.gmail.com","subject":"Re: [PATCH v2 2/2] pack-objects: don't loosen objects available in alternate or kept packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-22T04:43:14Z","receivedAt":"2009-03-22T04:43:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> If pack-objects is called with the --unpack-unreachable option then it will\n> unpack (i.e. loosen) all unreferenced objects from local not-kept packs,\n> including those that also exist in packs residing in an alternate object\n> database or a local kept pack. The primary(sole?) user of this option is\n> git-repack. In this case, repack will follow the call to pack-objects with\n> a call to prune-packed which will delete these newly loosened objects,\n> making the act of loosening a waste of time. The unnecessary loosening can\n> be avoided by checking whether an object exists in a non-local pack or a\n> local kept pack before loosening it.\n>\n> This fixes the 'local packed unreachable obs that exist in alternate ODB\n> are not loosened' test in t7700.\n>\n> Signed-off-by: Brandon Casey \n\nThanks.\n\nBoth patches are whitespace damaged, but I can cope. But I am not sure\nabout one thing...\n\n> builtin-pack-objects.c | 26 +++++++++++++++++++++++++-\n> t/t7700-repack.sh | 2 +-\n> 2 files changed, 26 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\n> index 6222f19..3f477c5 100644\n> --- a/builtin-pack-objects.c\n> +++ b/builtin-pack-objects.c\n> @@ -1944,6 +1944,29 @@ static void\n> add_objects_in_unpacked_packs(struct rev_info *revs)\n> \tfree(in_pack.array);\n> }\n>\n> +static int has_sha1_pack_kept_or_nonlocal(const unsigned char *sha1)\n> +{\n> +\tstatic struct packed_git *last_found = (void *)1;\n> +\tstruct packed_git *p;\n> +\n> +\tp = (last_found == (void *)1) ? packed_git : last_found;\n\nWhy (void *)1, not like:\n\n\tstatic struct packed_git *last_found;\n\tstruct packed_git *p = last_found ? last_found : packed_git;\n\nAm I missing something?\n\n> +\twhile (p) {\n> +\t\tif ((!p->pack_local || p->pack_keep) &&\n> +\t\t\tfind_pack_entry_one(sha1, p)) {\n> +\t\t\tlast_found = p;\n> +\t\t\treturn 1;\n> +\t\t}\n> +\t\tif (p == last_found)\n> +\t\t\tp = packed_git;\n> +\t\telse\n> +\t\t\tp = p->next;\n> +\t\tif (p == last_found)\n> +\t\t\tp = p->next;\n> +\t}\n> +\treturn 0;\n> +}\n"},{"id":"108900","messageId":"ee63ef30903220748q4e78e626t2ea3930809a670d6@mail.gmail.com","threadId":"18456","inReplyTo":"7vbpru9nh9.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2 2/2] pack-objects: don't loosen objects available in alternate or kept packs","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2009-03-22T14:48:15Z","receivedAt":"2009-03-22T14:48:15Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Sat, Mar 21, 2009 at 11:43 PM, Junio C Hamano wrote:\n> Brandon Casey writes:\n\n> Both patches are whitespace damaged, but I can cope.\n\nI just retrieved one of the patches from gmane and some long lines\nwere wrapped. I tried out the gmail imap instructions from\nSubmittingPatches for sending these patches. Those instructions say\nthat it is possible to send properly formatted patches through gmail,\nand seem to instruct to use the web interface for actually sending the\npatches. I wonder if there is some way to instruct gmail to not wrap\nlong lines? Or whether I did something else wrong?\n\nPreviously, I have used gmail's pop interface indirectly through my\nphone provider which strips out the \"From\" field and replaces it with\none that only has my email address and not my name.\n\n>  But I am not sure about one thing...\n>\n>> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\n>> index 6222f19..3f477c5 100644\n>> --- a/builtin-pack-objects.c\n>> +++ b/builtin-pack-objects.c\n>> @@ -1944,6 +1944,29 @@ static void\n>> add_objects_in_unpacked_packs(struct rev_info *revs)\n>>       free(in_pack.array);\n>>  }\n>>\n>> +static int has_sha1_pack_kept_or_nonlocal(const unsigned char *sha1)\n>> +{\n>> +     static struct packed_git *last_found = (void *)1;\n>> +     struct packed_git *p;\n>> +\n>> +     p = (last_found == (void *)1) ? packed_git : last_found;\n>\n> Why (void *)1, not like:\n>\n>        static struct packed_git *last_found;\n>        struct packed_git *p = last_found ? last_found : packed_git;\n>\n> Am I missing something?\n\nHeh, I am missing something too. Maybe I should have _thought_ more\nabout this code that I copied from sha1_file.c: find_pack_entry() and\nI would have asked the same question about _that_ code.\n\nMaybe Nico has some idea?\n\nI'll send a new patch, unless Nico has some thoughts.\n\n-brandon\n"},{"id":"108911","messageId":"alpine.LFD.2.00.0903221325380.30483@xanadu.home","threadId":"18456","inReplyTo":"ee63ef30903220748q4e78e626t2ea3930809a670d6@mail.gmail.com","subject":"Re: [PATCH v2 2/2] pack-objects: don't loosen objects available in alternate or kept packs","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-03-22T17:40:27Z","receivedAt":"2009-03-22T17:40:27Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sun, 22 Mar 2009, Brandon Casey wrote:\n\n> >> +static int has_sha1_pack_kept_or_nonlocal(const unsigned char *sha1)\n> >> +{\n> >> +     static struct packed_git *last_found = (void *)1;\n> >> +     struct packed_git *p;\n> >> +\n> >> +     p = (last_found == (void *)1) ? packed_git : last_found;\n> >\n> > Why (void *)1, not like:\n> >\n> >        static struct packed_git *last_found;\n> >        struct packed_git *p = last_found ? last_found : packed_git;\n> >\n> > Am I missing something?\n> \n> Heh, I am missing something too. Maybe I should have _thought_ more\n> about this code that I copied from sha1_file.c: find_pack_entry() and\n> I would have asked the same question about _that_ code.\n> \n> Maybe Nico has some idea?\n\nWell... I know this is my code, and I must have had a reason to keep \nthe NULL pointer distinct from the initial value. But at the moment I \njust can't remember about it, and the code doesn't show a need for it \neither.\n\n\nNicolas\n"},{"id":"108921","messageId":"7viqm15qda.fsf@gitster.siamese.dyndns.org","threadId":"18456","inReplyTo":"7vbpru9nh9.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2 2/2] pack-objects: don't loosen objects available in alternate or kept packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-22T19:06:41Z","receivedAt":"2009-03-22T19:06:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +static int has_sha1_pack_kept_or_nonlocal(const unsigned char *sha1)\n>> +{\n>> +\tstatic struct packed_git *last_found = (void *)1;\n>> +\tstruct packed_git *p;\n>> +\n>> +\tp = (last_found == (void *)1) ? packed_git : last_found;\n>\n> Why (void *)1, not like:\n>\n> \tstatic struct packed_git *last_found;\n> \tstruct packed_git *p = last_found ? last_found : packed_git;\n>\n> Am I missing something?\n>\n>> +\twhile (p) {\n>> +\t\tif ((!p->pack_local || p->pack_keep) &&\n>> +\t\t\tfind_pack_entry_one(sha1, p)) {\n>> +\t\t\tlast_found = p;\n>> +\t\t\treturn 1;\n>> +\t\t}\n>> +\t\tif (p == last_found)\n>> +\t\t\tp = packed_git;\n>> +\t\telse\n>> +\t\t\tp = p->next;\n>> +\t\tif (p == last_found)\n>> +\t\t\tp = p->next;\n>> +\t}\n>> +\treturn 0;\n>> +}\n\nYes I was.\n\nIf you allow (last_found == NULL), the loop iterates over packed_git list,\nand when it has its last element in p after discovering nothing matches\nthe criteria, this happens:\n\n\tif (p == last_found) /* false, p is the last element on the list */\n \tp = packed_git; /* not taken */\n\telse\n \tp = p->next; /* taken, p == NULL now */\n\tif (p == last_found) /* true! */\n\t\tp = p->next; /* OOPS */\n"},{"id":"108922","messageId":"alpine.LFD.2.00.0903221522341.30483@xanadu.home","threadId":"18456","inReplyTo":"7viqm15qda.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2 2/2] pack-objects: don't loosen objects available in alternate or kept packs","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-03-22T19:23:27Z","receivedAt":"2009-03-22T19:23:27Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sun, 22 Mar 2009, Junio C Hamano wrote:\n\n> If you allow (last_found == NULL), the loop iterates over packed_git list,\n> and when it has its last element in p after discovering nothing matches\n> the criteria, this happens:\n> \n> \tif (p == last_found) /* false, p is the last element on the list */\n> \tp = packed_git; /* not taken */\n> \telse\n> \tp = p->next; /* taken, p == NULL now */\n> \tif (p == last_found) /* true! */\n> \t\tp = p->next; /* OOPS */\n\nYeah, you got it. Now I remember.\n\n\nNicolas\n"},{"id":"109260","messageId":"rQW7WJIjEMSgMCBHRTSMXJu2FmnvjGxMF8nC2pFnNro@cipher.nrlssc.navy.mil","threadId":"18456","inReplyTo":"7viqm15qda.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2 2/2] pack-objects: don't loosen objects available in alternate or kept packs","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2009-03-24T23:01:03Z","receivedAt":"2009-03-24T23:01:03Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"\nJunio,\n\nI notice this patch is in next now (I noticed earlier than today,\nbut haven't said anything).\n\nI just want to point one minor thing. After figuring out why last_found\nwas initialized to (void *)1 rather then NULL, you seem to have committed\na version that was slightly different from what I submitted. The logic\nis the same, but now the following line is different from the line in\nsha1_file.c where I copied it from:\n\nbuiltin-pack-objects.c (line 1952):\n\n p = (last_found != (void *)1) ? last_found : packed_git;\n\nsha1_file.c (line 1931): \n\n p = (last_found == (void *)1) ? packed_git : last_found;\n\n\nPeople should be able to easily recognize the two procedures as \"the same\".\nThis small difference may give them pause. If it is possible to amend\nthis commit before applying to master, perhaps you would like to do so.\nOtherwise, no big deal.\n\n-brandon\n"}]}