{"thread":{"id":"29475","subject":"[PATCH] find_pack_entry(): do not keep packed_git pointer locally","startedAt":"2012-01-30T11:25:50Z","lastAt":"2012-02-02T13:53:51Z","messageCount":14,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Nguyen Thai Ngoc Duy","Nicolas Pitre"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"183306","messageId":"1327922750-12106-1-git-send-email-pclouds@gmail.com","threadId":"29475","inReplyTo":null,"subject":"[PATCH] find_pack_entry(): do not keep packed_git pointer locally","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-01-30T11:25:50Z","receivedAt":"2012-01-30T11:25:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Commit f7c22cc (always start looking up objects in the last used pack\nfirst - 2007-05-30) introduces a static packed_git* pointer as an\noptimization.  The kept pointer however may become invalid if\nfree_pack_by_name() happens to free that particular pack.\n\nCurrent code base does not access packs after calling\nfree_pack_by_name() so it should not be a problem. Anyway, move the\npointer out so that free_pack_by_name() can reset it to avoid running\ninto troubles in future.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n sha1_file.c |   13 ++++++++-----\n 1 files changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 88f2151..4ecc953 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -54,6 +54,8 @@ static struct cached_object empty_tree = {\n \t0\n };\n \n+static struct packed_git *find_pack_entry_last_found = (void *)1;\n+\n static struct cached_object *find_cached_object(const unsigned char *sha1)\n {\n \tint i;\n@@ -720,6 +722,8 @@ void free_pack_by_name(const char *pack_name)\n \t\t\tclose_pack_index(p);\n \t\t\tfree(p->bad_object_sha1);\n \t\t\t*pp = p->next;\n+\t\t\tif (find_pack_entry_last_found == p)\n+\t\t\t\tfind_pack_entry_last_found = (void*)1;\n \t\t\tfree(p);\n \t\t\treturn;\n \t\t}\n@@ -2012,14 +2016,13 @@ int is_pack_valid(struct packed_git *p)\n \n static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n {\n-\tstatic struct packed_git *last_found = (void *)1;\n \tstruct packed_git *p;\n \toff_t offset;\n \n \tprepare_packed_git();\n \tif (!packed_git)\n \t\treturn 0;\n-\tp = (last_found == (void *)1) ? packed_git : last_found;\n+\tp = (find_pack_entry_last_found == (void *)1) ? packed_git : find_pack_entry_last_found;\n \n \tdo {\n \t\tif (p->num_bad_objects) {\n@@ -2046,16 +2049,16 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n \t\t\te->offset = offset;\n \t\t\te->p = p;\n \t\t\thashcpy(e->sha1, sha1);\n-\t\t\tlast_found = p;\n+\t\t\tfind_pack_entry_last_found = p;\n \t\t\treturn 1;\n \t\t}\n \n \t\tnext:\n-\t\tif (p == last_found)\n+\t\tif (p == find_pack_entry_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\tif (p == find_pack_entry_last_found)\n \t\t\tp = p->next;\n \t} while (p);\n \treturn 0;\n-- \n1.7.8.36.g69ee2\n"},{"id":"183396","messageId":"7v62fsai1i.fsf@alter.siamese.dyndns.org","threadId":"29475","inReplyTo":"1327922750-12106-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] find_pack_entry(): do not keep packed_git pointer locally","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-30T23:26:33Z","receivedAt":"2012-01-30T23:26:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> Commit f7c22cc (always start looking up objects in the last used pack\n> first - 2007-05-30) introduces a static packed_git* pointer as an\n> optimization.  The kept pointer however may become invalid if\n> free_pack_by_name() happens to free that particular pack.\n>\n> Current code base does not access packs after calling\n> free_pack_by_name() so it should not be a problem. Anyway, move the\n> pointer out so that free_pack_by_name() can reset it to avoid running\n> into troubles in future.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n\nThanks. Two curiosities:\n\n - Why is there no hunk to actually clear the pointer in\n   free_pack_by_name() in this patch?\n\n - Could we make the magic (void *)1 value a #define'd constant? Perhaps\n   we could even use NULL for that purpose?\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 88f2151..4ecc953 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -54,6 +54,8 @@ static struct cached_object empty_tree = {\n>  \t0\n>  };\n>  \n> +static struct packed_git *find_pack_entry_last_found = (void *)1;\n> +\n>  static struct cached_object *find_cached_object(const unsigned char *sha1)\n>  {\n>  \tint i;\n> @@ -720,6 +722,8 @@ void free_pack_by_name(const char *pack_name)\n>  \t\t\tclose_pack_index(p);\n>  \t\t\tfree(p->bad_object_sha1);\n>  \t\t\t*pp = p->next;\n> +\t\t\tif (find_pack_entry_last_found == p)\n> +\t\t\t\tfind_pack_entry_last_found = (void*)1;\n>  \t\t\tfree(p);\n>  \t\t\treturn;\n>  \t\t}\n> @@ -2012,14 +2016,13 @@ int is_pack_valid(struct packed_git *p)\n>  \n>  static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n>  {\n> -\tstatic struct packed_git *last_found = (void *)1;\n>  \tstruct packed_git *p;\n>  \toff_t offset;\n>  \n>  \tprepare_packed_git();\n>  \tif (!packed_git)\n>  \t\treturn 0;\n> -\tp = (last_found == (void *)1) ? packed_git : last_found;\n> +\tp = (find_pack_entry_last_found == (void *)1) ? packed_git : find_pack_entry_last_found;\n>  \n>  \tdo {\n>  \t\tif (p->num_bad_objects) {\n> @@ -2046,16 +2049,16 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n>  \t\t\te->offset = offset;\n>  \t\t\te->p = p;\n>  \t\t\thashcpy(e->sha1, sha1);\n> -\t\t\tlast_found = p;\n> +\t\t\tfind_pack_entry_last_found = p;\n>  \t\t\treturn 1;\n>  \t\t}\n>  \n>  \t\tnext:\n> -\t\tif (p == last_found)\n> +\t\tif (p == find_pack_entry_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\tif (p == find_pack_entry_last_found)\n>  \t\t\tp = p->next;\n>  \t} while (p);\n>  \treturn 0;\n"},{"id":"183403","messageId":"CACsJy8AS0nZOoXZZfz0OEwoWe88wp2aGR5NGqpG7xQmUvwi7TA@mail.gmail.com","threadId":"29475","inReplyTo":"7v62fsai1i.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] find_pack_entry(): do not keep packed_git pointer locally","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-01-31T02:01:11Z","receivedAt":"2012-01-31T02:01:11Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"(Pulling Nico in for Q2 below. No snipping so he has a context)\n\n2012/1/31 Junio C Hamano <gitster@pobox.com>:\n> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n>\n>> Commit f7c22cc (always start looking up objects in the last used pack\n>> first - 2007-05-30) introduces a static packed_git* pointer as an\n>> optimization.  The kept pointer however may become invalid if\n>> free_pack_by_name() happens to free that particular pack.\n>>\n>> Current code base does not access packs after calling\n>> free_pack_by_name() so it should not be a problem. Anyway, move the\n>> pointer out so that free_pack_by_name() can reset it to avoid running\n>> into troubles in future.\n>>\n>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n>> ---\n>\n> Thanks. Two curiosities:\n>\n> - Why is there no hunk to actually clear the pointer in\n>  free_pack_by_name() in this patch?\n\nI think it's there (the patch did work for me when I tried to\nintegrate repack to pack-objects).\n\n-- 8<--\n> @@ -720,6 +722,8 @@ void free_pack_by_name(const char *pack_name)\n>                       close_pack_index(p);\n>                       free(p->bad_object_sha1);\n>                       *pp = p->next;\n> +                     if (find_pack_entry_last_found == p)\n> +                             find_pack_entry_last_found = (void*)1;\n>                       free(p);\n>                       return;\n>               }\n-- 8< --\n\n>  - Could we make the magic (void *)1 value a #define'd constant? Perhaps\n>   we could even use NULL for that purpose?\n\nQ1. Sure.\n\nQ2. No NULL is probably not suitable. I think Nico wanted to express\n\"we tried to find but found none (i.e. NULL)\" too and 1 means \"no we\nhave not tried\".\n\n>> diff --git a/sha1_file.c b/sha1_file.c\n>> index 88f2151..4ecc953 100644\n>> --- a/sha1_file.c\n>> +++ b/sha1_file.c\n>> @@ -54,6 +54,8 @@ static struct cached_object empty_tree = {\n>>       0\n>>  };\n>>\n>> +static struct packed_git *find_pack_entry_last_found = (void *)1;\n>> +\n>>  static struct cached_object *find_cached_object(const unsigned char *sha1)\n>>  {\n>>       int i;\n>> @@ -720,6 +722,8 @@ void free_pack_by_name(const char *pack_name)\n>>                       close_pack_index(p);\n>>                       free(p->bad_object_sha1);\n>>                       *pp = p->next;\n>> +                     if (find_pack_entry_last_found == p)\n>> +                             find_pack_entry_last_found = (void*)1;\n>>                       free(p);\n>>                       return;\n>>               }\n>> @@ -2012,14 +2016,13 @@ int is_pack_valid(struct packed_git *p)\n>>\n>>  static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n>>  {\n>> -     static struct packed_git *last_found = (void *)1;\n>>       struct packed_git *p;\n>>       off_t offset;\n>>\n>>       prepare_packed_git();\n>>       if (!packed_git)\n>>               return 0;\n>> -     p = (last_found == (void *)1) ? packed_git : last_found;\n>> +     p = (find_pack_entry_last_found == (void *)1) ? packed_git : find_pack_entry_last_found;\n>>\n>>       do {\n>>               if (p->num_bad_objects) {\n>> @@ -2046,16 +2049,16 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n>>                       e->offset = offset;\n>>                       e->p = p;\n>>                       hashcpy(e->sha1, sha1);\n>> -                     last_found = p;\n>> +                     find_pack_entry_last_found = p;\n>>                       return 1;\n>>               }\n>>\n>>               next:\n>> -             if (p == last_found)\n>> +             if (p == find_pack_entry_last_found)\n>>                       p = packed_git;\n>>               else\n>>                       p = p->next;\n>> -             if (p == last_found)\n>> +             if (p == find_pack_entry_last_found)\n>>                       p = p->next;\n>>       } while (p);\n>>       return 0;\n\n\n\n-- \nDuy\n"},{"id":"183404","messageId":"alpine.LFD.2.02.1201302301150.2759@xanadu.home","threadId":"29475","inReplyTo":"CACsJy8AS0nZOoXZZfz0OEwoWe88wp2aGR5NGqpG7xQmUvwi7TA@mail.gmail.com","subject":"Re: [PATCH] find_pack_entry(): do not keep packed_git pointer locally","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2012-01-31T04:19:54Z","receivedAt":"2012-01-31T04:19:54Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 31 Jan 2012, Nguyen Thai Ngoc Duy wrote:\n\n> (Pulling Nico in for Q2 below. No snipping so he has a context)\n> \n> 2012/1/31 Junio C Hamano <gitster@pobox.com>:\n> > Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n> >\n> >> Commit f7c22cc (always start looking up objects in the last used pack\n> >> first - 2007-05-30) introduces a static packed_git* pointer as an\n> >> optimization.  The kept pointer however may become invalid if\n> >> free_pack_by_name() happens to free that particular pack.\n\nHmmm, good point.\n\n> >> Current code base does not access packs after calling\n> >> free_pack_by_name() so it should not be a problem. Anyway, move the\n> >> pointer out so that free_pack_by_name() can reset it to avoid running\n> >> into troubles in future.\n> >>\n> >> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> >> ---\n[...]\n> >  - Could we make the magic (void *)1 value a #define'd constant? Perhaps\n> >   we could even use NULL for that purpose?\n> \n> Q1. Sure.\n\nIndeed.  The idea might have been to use a non null value that cannot \nmatch any pointer...\n\n> Q2. No NULL is probably not suitable. I think Nico wanted to express\n> \"we tried to find but found none (i.e. NULL)\" too and 1 means \"no we\n> have not tried\".\n\nWell, I could imagine I might have thought about something like that.  \nHowever, looking at the latest code in the master branch I can't see \nany way for last_found to ever be assigned a NULL value.  So if the\n(void*)1 value might have been useful, it is certainly not anymore.\n\n\nNicolas\n"},{"id":"183422","messageId":"7vr4yf92dg.fsf@alter.siamese.dyndns.org","threadId":"29475","inReplyTo":"1328010239-29669-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2] find_pack_entry(): do not keep packed_git pointer locally","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-31T18:02:35Z","receivedAt":"2012-01-31T18:02:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:\n\n>  Changing INVALID_PACK to NULL breaks at least t1050.4. We may\n>  restructure the p assignments at the end of find_pack_entry() to\n>  allow setting INVALID_PACK to NULL.\n>  \n>  But I'm not really excited about that.\n\nThe patch to do so should be a trivial one on top of this anyway\n(attached).\n\n> -\t\tif (p == last_found)\n> +\t\tif (p == find_pack_entry_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/*\n> +\t\t * p can be NULL from the else clause above, if initial\n> +\t\t * f_p_e_last_found value (i.e. INVALID_PACK) is NULL, we may\n> +\t\t * advance p again to an imaginary pack in invalid memory\n> +\t\t */\n> +\t\tif (p == find_pack_entry_last_found)\n>  \t\t\tp = p->next;\n\nBut I think the real issue is that the original loop is written in an\nobscure way.  The conversion in f7c22cc (always start looking up objects\nin the last used pack first, 2007-05-30) wanted to turn the traversal that\nalways went from the tip of a linked list to instead first probe the\npromising one, and then scan the list from the tip like it used to do,\nexcept that it did not want to probe the one it thought promising again.\n\nSo perhaps restructuring the loop by making the logic to probe into a\nsingle pack into a helper function, e.g.\n\nstatic int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n{\n\tif (last_found) {\n\t\tif (find_one(sha1, e, last_found))\n\t\t\treturn 1;\n\t}\n        for (p = packed_git; p; p = p->next) {\n        \tif (p == last_found || !find_one(sha1, e, p))\n\t\t\tcontinue;\n\t\tlast_found = p;\n\t\treturn 1;\n\t}\n        return 0;\n}\n\nwould make the resulting flow far easier to follow, no?\n\n\n sha1_file.c |   16 +++++-----------\n 1 files changed, 5 insertions(+), 11 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 77512a3..2286789 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -54,9 +54,7 @@ static struct cached_object empty_tree = {\n \t0\n };\n \n-/* INVALID_PACK cannot be NULL, see comments in find_pack_entry */\n-#define INVALID_PACK (struct packed_git *)1\n-static struct packed_git *find_pack_entry_last_found = INVALID_PACK;\n+static struct packed_git *find_pack_entry_last_found;\n \n static struct cached_object *find_cached_object(const unsigned char *sha1)\n {\n@@ -725,7 +723,7 @@ void free_pack_by_name(const char *pack_name)\n \t\t\tfree(p->bad_object_sha1);\n \t\t\t*pp = p->next;\n \t\t\tif (find_pack_entry_last_found == p)\n-\t\t\t\tfind_pack_entry_last_found = INVALID_PACK;\n+\t\t\t\tfind_pack_entry_last_found = NULL;\n \t\t\tfree(p);\n \t\t\treturn;\n \t\t}\n@@ -2024,7 +2022,7 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n \tprepare_packed_git();\n \tif (!packed_git)\n \t\treturn 0;\n-\tp = find_pack_entry_last_found == INVALID_PACK ? packed_git : find_pack_entry_last_found;\n+\tp = !find_pack_entry_last_found ? packed_git : find_pack_entry_last_found;\n \n \tdo {\n \t\tif (p->num_bad_objects) {\n@@ -2060,12 +2058,8 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n \t\t\tp = packed_git;\n \t\telse\n \t\t\tp = p->next;\n-\t\t/*\n-\t\t * p can be NULL from the else clause above, if initial\n-\t\t * f_p_e_last_found value (i.e. INVALID_PACK) is NULL, we may\n-\t\t * advance p again to an imaginary pack in invalid memory\n-\t\t */\n-\t\tif (p == find_pack_entry_last_found)\n+\n+\t\tif (p && p == find_pack_entry_last_found)\n \t\t\tp = p->next;\n \t} while (p);\n \treturn 0;\n"},{"id":"183426","messageId":"alpine.LFD.2.02.1201311418430.2759@xanadu.home","threadId":"29475","inReplyTo":"7vr4yf92dg.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] find_pack_entry(): do not keep packed_git pointer locally","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2012-01-31T19:28:50Z","receivedAt":"2012-01-31T19:28:50Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 31 Jan 2012, Junio C Hamano wrote:\n\n> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:\n> \n> > +\t\t/*\n> > +\t\t * p can be NULL from the else clause above, if initial\n> > +\t\t * f_p_e_last_found value (i.e. INVALID_PACK) is NULL, we may\n> > +\t\t * advance p again to an imaginary pack in invalid memory\n> > +\t\t */\n\nAh!  OK so that's why I initially came up with that (void*)1 value.\n\n> But I think the real issue is that the original loop is written in an\n> obscure way.\n\nCan't disagree with that, especially when the original author (myself) \ndoesn't see clearly through it anymore.\n\n> The conversion in f7c22cc (always start looking up objects in the last \n> used pack first, 2007-05-30) wanted to turn the traversal that always \n> went from the tip of a linked list to instead first probe the \n> promising one, and then scan the list from the tip like it used to do, \n> except that it did not want to probe the one it thought promising \n> again.\n\nExact.\n\n> So perhaps restructuring the loop by making the logic to probe into a\n> single pack into a helper function, e.g.\n> \n> static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n> {\n> \tif (last_found) {\n> \t\tif (find_one(sha1, e, last_found))\n> \t\t\treturn 1;\n> \t}\n>         for (p = packed_git; p; p = p->next) {\n>         \tif (p == last_found || !find_one(sha1, e, p))\n> \t\t\tcontinue;\n> \t\tlast_found = p;\n> \t\treturn 1;\n> \t}\n>         return 0;\n> }\n> \n> would make the resulting flow far easier to follow, no?\n\nIndeed.\n\n\nNicolas\n"},{"id":"183471","messageId":"1328104135-475-1-git-send-email-pclouds@gmail.com","threadId":"29475","inReplyTo":"1328010239-29669-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v3 1/2] Factor find_pack_entry()'s core out","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-01T13:48:54Z","receivedAt":"2012-02-01T13:48:54Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n sha1_file.c |   59 +++++++++++++++++++++++++++++++++--------------------------\n 1 files changed, 33 insertions(+), 26 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 88f2151..ff5bf42 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2010,11 +2010,42 @@ int is_pack_valid(struct packed_git *p)\n \treturn !open_packed_git(p);\n }\n \n+static int find_pack_entry_1(const unsigned char *sha1,\n+\t\t\t     struct packed_git *p, struct pack_entry *e)\n+{\n+\toff_t offset;\n+\tif (p->num_bad_objects) {\n+\t\tunsigned i;\n+\t\tfor (i = 0; i < p->num_bad_objects; i++)\n+\t\t\tif (!hashcmp(sha1, p->bad_object_sha1 + 20 * i))\n+\t\t\t\treturn 0;\n+\t}\n+\n+\toffset = find_pack_entry_one(sha1, p);\n+\tif (!offset)\n+\t\treturn 0;\n+\n+\t/*\n+\t * We are about to tell the caller where they can locate the\n+\t * requested object.  We better make sure the packfile is\n+\t * still here and can be accessed before supplying that\n+\t * answer, as it may have been deleted since the index was\n+\t * loaded!\n+\t */\n+\tif (!is_pack_valid(p)) {\n+\t\twarning(\"packfile %s cannot be accessed\", p->pack_name);\n+\t\treturn 0;\n+\t}\n+\te->offset = offset;\n+\te->p = p;\n+\thashcpy(e->sha1, sha1);\n+\treturn 1;\n+}\n+\n static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n {\n \tstatic struct packed_git *last_found = (void *)1;\n \tstruct packed_git *p;\n-\toff_t offset;\n \n \tprepare_packed_git();\n \tif (!packed_git)\n@@ -2022,35 +2053,11 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n \tp = (last_found == (void *)1) ? packed_git : last_found;\n \n \tdo {\n-\t\tif (p->num_bad_objects) {\n-\t\t\tunsigned i;\n-\t\t\tfor (i = 0; i < p->num_bad_objects; i++)\n-\t\t\t\tif (!hashcmp(sha1, p->bad_object_sha1 + 20 * i))\n-\t\t\t\t\tgoto next;\n-\t\t}\n-\n-\t\toffset = find_pack_entry_one(sha1, p);\n-\t\tif (offset) {\n-\t\t\t/*\n-\t\t\t * We are about to tell the caller where they can\n-\t\t\t * locate the requested object.  We better make\n-\t\t\t * sure the packfile is still here and can be\n-\t\t\t * accessed before supplying that answer, as\n-\t\t\t * it may have been deleted since the index\n-\t\t\t * was loaded!\n-\t\t\t */\n-\t\t\tif (!is_pack_valid(p)) {\n-\t\t\t\twarning(\"packfile %s cannot be accessed\", p->pack_name);\n-\t\t\t\tgoto next;\n-\t\t\t}\n-\t\t\te->offset = offset;\n-\t\t\te->p = p;\n-\t\t\thashcpy(e->sha1, sha1);\n+\t\tif (find_pack_entry_1(sha1, p, e)) {\n \t\t\tlast_found = p;\n \t\t\treturn 1;\n \t\t}\n \n-\t\tnext:\n \t\tif (p == last_found)\n \t\t\tp = packed_git;\n \t\telse\n-- \n1.7.8.36.g69ee2\n"},{"id":"183472","messageId":"1328104135-475-2-git-send-email-pclouds@gmail.com","threadId":"29475","inReplyTo":"1328104135-475-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v3 2/2] find_pack_entry(): do not keep packed_git pointer locally","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-01T13:48:55Z","receivedAt":"2012-02-01T13:48:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Commit f7c22cc (always start looking up objects in the last used pack\nfirst - 2007-05-30) introduce a static packed_git* pointer as an\noptimization.  The kept pointer however may become invalid if\nfree_pack_by_name() happens to free that particular pack.\n\nCurrent code base does not access packs after calling\nfree_pack_by_name() so it should not be a problem. Anyway, move the\npointer out so that free_pack_by_name() can reset it to avoid running\ninto troubles in future.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Since Junio's already done the hard work. It'd be silly of me not to\n take advantage and credit for free :)\n\n The new loop looks much better.\n\n sha1_file.c |   27 +++++++++++++--------------\n 1 files changed, 13 insertions(+), 14 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex ff5bf42..ebe77b3 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -54,6 +54,8 @@ static struct cached_object empty_tree = {\n \t0\n };\n \n+static struct packed_git *last_found_pack;\n+\n static struct cached_object *find_cached_object(const unsigned char *sha1)\n {\n \tint i;\n@@ -720,6 +722,8 @@ void free_pack_by_name(const char *pack_name)\n \t\t\tclose_pack_index(p);\n \t\t\tfree(p->bad_object_sha1);\n \t\t\t*pp = p->next;\n+\t\t\tif (last_found_pack == p)\n+\t\t\t\tlast_found_pack = NULL;\n \t\t\tfree(p);\n \t\t\treturn;\n \t\t}\n@@ -2044,27 +2048,22 @@ static int find_pack_entry_1(const unsigned char *sha1,\n \n static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n {\n-\tstatic struct packed_git *last_found = (void *)1;\n \tstruct packed_git *p;\n \n \tprepare_packed_git();\n \tif (!packed_git)\n \t\treturn 0;\n-\tp = (last_found == (void *)1) ? packed_git : last_found;\n \n-\tdo {\n-\t\tif (find_pack_entry_1(sha1, p, e)) {\n-\t\t\tlast_found = p;\n-\t\t\treturn 1;\n-\t\t}\n+\tif (last_found_pack && find_pack_entry_1(sha1, last_found_pack, e))\n+\t\treturn 1;\n \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} while (p);\n+\tfor (p = packed_git; p; p = p->next) {\n+\t\tif (p == last_found_pack || !find_pack_entry_1(sha1, p, e))\n+\t\t\tcontinue;\n+\n+\t\tlast_found_pack = p;\n+\t\treturn 1;\n+\t}\n \treturn 0;\n }\n \n-- \n1.7.8.36.g69ee2\n"},{"id":"183479","messageId":"alpine.LFD.2.02.1202011056140.2759@xanadu.home","threadId":"29475","inReplyTo":"1328104135-475-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v3 1/2] Factor find_pack_entry()'s core out","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2012-02-01T15:59:26Z","receivedAt":"2012-02-01T15:59:26Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 1 Feb 2012, Nguyễn Thái Ngọc Duy wrote:\n\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  sha1_file.c |   59 +++++++++++++++++++++++++++++++++--------------------------\n>  1 files changed, 33 insertions(+), 26 deletions(-)\n> \n> diff --git a/sha1_file.c b/sha1_file.c\n> index 88f2151..ff5bf42 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2010,11 +2010,42 @@ int is_pack_valid(struct packed_git *p)\n>  \treturn !open_packed_git(p);\n>  }\n>  \n> +static int find_pack_entry_1(const unsigned char *sha1,\n> +\t\t\t     struct packed_git *p, struct pack_entry *e)\n\nThis looks all goot but the name.  Pretty please, try to find something \nthat is more descriptive than \"1\".  Suggestions: \n\"find_pack_entry_lookup\", \"find_pack_entry_inner\", etc.\n\nWith that fixed, you can add:\n\nAcked-by: Nicolas Pitre <nico@fluxnic.net>\n\n> +{\n> +\toff_t offset;\n> +\tif (p->num_bad_objects) {\n> +\t\tunsigned i;\n> +\t\tfor (i = 0; i < p->num_bad_objects; i++)\n> +\t\t\tif (!hashcmp(sha1, p->bad_object_sha1 + 20 * i))\n> +\t\t\t\treturn 0;\n> +\t}\n> +\n> +\toffset = find_pack_entry_one(sha1, p);\n> +\tif (!offset)\n> +\t\treturn 0;\n> +\n> +\t/*\n> +\t * We are about to tell the caller where they can locate the\n> +\t * requested object.  We better make sure the packfile is\n> +\t * still here and can be accessed before supplying that\n> +\t * answer, as it may have been deleted since the index was\n> +\t * loaded!\n> +\t */\n> +\tif (!is_pack_valid(p)) {\n> +\t\twarning(\"packfile %s cannot be accessed\", p->pack_name);\n> +\t\treturn 0;\n> +\t}\n> +\te->offset = offset;\n> +\te->p = p;\n> +\thashcpy(e->sha1, sha1);\n> +\treturn 1;\n> +}\n> +\n>  static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n>  {\n>  \tstatic struct packed_git *last_found = (void *)1;\n>  \tstruct packed_git *p;\n> -\toff_t offset;\n>  \n>  \tprepare_packed_git();\n>  \tif (!packed_git)\n> @@ -2022,35 +2053,11 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n>  \tp = (last_found == (void *)1) ? packed_git : last_found;\n>  \n>  \tdo {\n> -\t\tif (p->num_bad_objects) {\n> -\t\t\tunsigned i;\n> -\t\t\tfor (i = 0; i < p->num_bad_objects; i++)\n> -\t\t\t\tif (!hashcmp(sha1, p->bad_object_sha1 + 20 * i))\n> -\t\t\t\t\tgoto next;\n> -\t\t}\n> -\n> -\t\toffset = find_pack_entry_one(sha1, p);\n> -\t\tif (offset) {\n> -\t\t\t/*\n> -\t\t\t * We are about to tell the caller where they can\n> -\t\t\t * locate the requested object.  We better make\n> -\t\t\t * sure the packfile is still here and can be\n> -\t\t\t * accessed before supplying that answer, as\n> -\t\t\t * it may have been deleted since the index\n> -\t\t\t * was loaded!\n> -\t\t\t */\n> -\t\t\tif (!is_pack_valid(p)) {\n> -\t\t\t\twarning(\"packfile %s cannot be accessed\", p->pack_name);\n> -\t\t\t\tgoto next;\n> -\t\t\t}\n> -\t\t\te->offset = offset;\n> -\t\t\te->p = p;\n> -\t\t\thashcpy(e->sha1, sha1);\n> +\t\tif (find_pack_entry_1(sha1, p, e)) {\n>  \t\t\tlast_found = p;\n>  \t\t\treturn 1;\n>  \t\t}\n>  \n> -\t\tnext:\n>  \t\tif (p == last_found)\n>  \t\t\tp = packed_git;\n>  \t\telse\n> -- \n> 1.7.8.36.g69ee2\n> \n"},{"id":"183480","messageId":"alpine.LFD.2.02.1202011100010.2759@xanadu.home","threadId":"29475","inReplyTo":"1328104135-475-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v3 2/2] find_pack_entry(): do not keep packed_git pointer locally","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2012-02-01T16:02:52Z","receivedAt":"2012-02-01T16:02:52Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 1 Feb 2012, Nguyễn Thái Ngọc Duy wrote:\n\n> Commit f7c22cc (always start looking up objects in the last used pack\n> first - 2007-05-30) introduce a static packed_git* pointer as an\n> optimization.  The kept pointer however may become invalid if\n> free_pack_by_name() happens to free that particular pack.\n> \n> Current code base does not access packs after calling\n> free_pack_by_name() so it should not be a problem. Anyway, move the\n> pointer out so that free_pack_by_name() can reset it to avoid running\n> into troubles in future.\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n\nAcked-by: Nicolas Pitre <nico@fluxnic.net>\n\n\n>  Since Junio's already done the hard work. It'd be silly of me not to\n>  take advantage and credit for free :)\n\nMaybe a little \"Thanks to Junio for code layout suggestions\" in the \ncommit message would give him some credit back.\n\n>  The new loop looks much better.\n\nIndeed.\n\n>  sha1_file.c |   27 +++++++++++++--------------\n>  1 files changed, 13 insertions(+), 14 deletions(-)\n> \n> diff --git a/sha1_file.c b/sha1_file.c\n> index ff5bf42..ebe77b3 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -54,6 +54,8 @@ static struct cached_object empty_tree = {\n>  \t0\n>  };\n>  \n> +static struct packed_git *last_found_pack;\n> +\n>  static struct cached_object *find_cached_object(const unsigned char *sha1)\n>  {\n>  \tint i;\n> @@ -720,6 +722,8 @@ void free_pack_by_name(const char *pack_name)\n>  \t\t\tclose_pack_index(p);\n>  \t\t\tfree(p->bad_object_sha1);\n>  \t\t\t*pp = p->next;\n> +\t\t\tif (last_found_pack == p)\n> +\t\t\t\tlast_found_pack = NULL;\n>  \t\t\tfree(p);\n>  \t\t\treturn;\n>  \t\t}\n> @@ -2044,27 +2048,22 @@ static int find_pack_entry_1(const unsigned char *sha1,\n>  \n>  static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n>  {\n> -\tstatic struct packed_git *last_found = (void *)1;\n>  \tstruct packed_git *p;\n>  \n>  \tprepare_packed_git();\n>  \tif (!packed_git)\n>  \t\treturn 0;\n> -\tp = (last_found == (void *)1) ? packed_git : last_found;\n>  \n> -\tdo {\n> -\t\tif (find_pack_entry_1(sha1, p, e)) {\n> -\t\t\tlast_found = p;\n> -\t\t\treturn 1;\n> -\t\t}\n> +\tif (last_found_pack && find_pack_entry_1(sha1, last_found_pack, e))\n> +\t\treturn 1;\n>  \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} while (p);\n> +\tfor (p = packed_git; p; p = p->next) {\n> +\t\tif (p == last_found_pack || !find_pack_entry_1(sha1, p, e))\n> +\t\t\tcontinue;\n> +\n> +\t\tlast_found_pack = p;\n> +\t\treturn 1;\n> +\t}\n>  \treturn 0;\n>  }\n>  \n> -- \n> 1.7.8.36.g69ee2\n> \n"},{"id":"183504","messageId":"7vaa522oum.fsf@alter.siamese.dyndns.org","threadId":"29475","inReplyTo":"alpine.LFD.2.02.1202011056140.2759@xanadu.home","subject":"Re: [PATCH v3 1/2] Factor find_pack_entry()'s core out","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-01T22:03:29Z","receivedAt":"2012-02-01T22:03:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre <nico@fluxnic.net> writes:\n\n>> +static int find_pack_entry_1(const unsigned char *sha1,\n>> +\t\t\t     struct packed_git *p, struct pack_entry *e)\n>\n> This looks all goot but the name.  Pretty please, try to find something \n> that is more descriptive than \"1\".  Suggestions: \n> \"find_pack_entry_lookup\", \"find_pack_entry_inner\", etc.\n\nPerhaps \"find_pack_entry_in_pack(sha1, e, p)\"?\nThat would go well with the caller \"find_pack_entry(sha1, e)\".\n"},{"id":"183508","messageId":"7v1uqe2ngh.fsf@alter.siamese.dyndns.org","threadId":"29475","inReplyTo":"7vaa522oum.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 1/2] Factor find_pack_entry()'s core out","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-01T22:33:34Z","receivedAt":"2012-02-01T22:33:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Nicolas Pitre <nico@fluxnic.net> writes:\n>\n>>> +static int find_pack_entry_1(const unsigned char *sha1,\n>>> +\t\t\t     struct packed_git *p, struct pack_entry *e)\n>>\n>> This looks all goot but the name.  Pretty please, try to find something \n>> that is more descriptive than \"1\".  Suggestions: \n>> \"find_pack_entry_lookup\", \"find_pack_entry_inner\", etc.\n>\n> Perhaps \"find_pack_entry_in_pack(sha1, e, p)\"?\n> That would go well with the caller \"find_pack_entry(sha1, e)\".\n\nI amended 1/2 (and adjusted 2/2 to match) to call it fill_pack_entry.\nWill push out the result tonight.\n"},{"id":"183516","messageId":"alpine.LFD.2.02.1202011836560.2759@xanadu.home","threadId":"29475","inReplyTo":"7v1uqe2ngh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 1/2] Factor find_pack_entry()'s core out","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2012-02-01T23:37:17Z","receivedAt":"2012-02-01T23:37:17Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 1 Feb 2012, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Nicolas Pitre <nico@fluxnic.net> writes:\n> >\n> >>> +static int find_pack_entry_1(const unsigned char *sha1,\n> >>> +\t\t\t     struct packed_git *p, struct pack_entry *e)\n> >>\n> >> This looks all goot but the name.  Pretty please, try to find something \n> >> that is more descriptive than \"1\".  Suggestions: \n> >> \"find_pack_entry_lookup\", \"find_pack_entry_inner\", etc.\n> >\n> > Perhaps \"find_pack_entry_in_pack(sha1, e, p)\"?\n> > That would go well with the caller \"find_pack_entry(sha1, e)\".\n> \n> I amended 1/2 (and adjusted 2/2 to match) to call it fill_pack_entry.\n> Will push out the result tonight.\n\nLooks fine to me.\n\n\nNicolas\n"},{"id":"183601","messageId":"1328190831-26580-1-git-send-email-pclouds@gmail.com","threadId":"29475","inReplyTo":"alpine.LFD.2.02.1202011100010.2759@xanadu.home","subject":"[PATCH v4 2/2] find_pack_entry(): do not keep packed_git pointer locally","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-02T13:53:51Z","receivedAt":"2012-02-02T13:53:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Commit f7c22cc (always start looking up objects in the last used pack\nfirst - 2007-05-30) introduce a static packed_git* pointer as an\noptimization.  The kept pointer however may become invalid if\nfree_pack_by_name() happens to free that particular pack.\n\nCurrent code base does not access packs after calling\nfree_pack_by_name() so it should not be a problem. Anyway, move the\npointer out so that free_pack_by_name() can reset it to avoid running\ninto troubles in future.\n\nThanks to Junio for code layout suggestions.\n\nAcked-by: Nicolas Pitre <nico@fluxnic.net>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Credit where credit is due. No code changes from pu.\n\n sha1_file.c |   27 +++++++++++++--------------\n 1 files changed, 13 insertions(+), 14 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 61e51ed..6b1b512 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -54,6 +54,8 @@ static struct cached_object empty_tree = {\n \t0\n };\n \n+static struct packed_git *last_found_pack;\n+\n static struct cached_object *find_cached_object(const unsigned char *sha1)\n {\n \tint i;\n@@ -720,6 +722,8 @@ void free_pack_by_name(const char *pack_name)\n \t\t\tclose_pack_index(p);\n \t\t\tfree(p->bad_object_sha1);\n \t\t\t*pp = p->next;\n+\t\t\tif (last_found_pack == p)\n+\t\t\t\tlast_found_pack = NULL;\n \t\t\tfree(p);\n \t\t\treturn;\n \t\t}\n@@ -2046,27 +2050,22 @@ static int fill_pack_entry(const unsigned char *sha1,\n \n static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n {\n-\tstatic struct packed_git *last_found = (void *)1;\n \tstruct packed_git *p;\n \n \tprepare_packed_git();\n \tif (!packed_git)\n \t\treturn 0;\n-\tp = (last_found == (void *)1) ? packed_git : last_found;\n \n-\tdo {\n-\t\tif (fill_pack_entry(sha1, e, p)) {\n-\t\t\tlast_found = p;\n-\t\t\treturn 1;\n-\t\t}\n+\tif (last_found_pack && fill_pack_entry(sha1, e, last_found_pack))\n+\t\treturn 1;\n \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} while (p);\n+\tfor (p = packed_git; p; p = p->next) {\n+\t\tif (p == last_found_pack || !fill_pack_entry(sha1, e, p))\n+\t\t\tcontinue;\n+\n+\t\tlast_found_pack = p;\n+\t\treturn 1;\n+\t}\n \treturn 0;\n }\n \n-- \n1.7.8.36.g69ee2\n"}]}