From: Nguyen Thai Ngoc Duy Date: Tue, 31 Jan 2012 02:01:11 GMT Subject: Re: [PATCH] find_pack_entry(): do not keep packed_git pointer locally Message-ID: In-Reply-To: <7v62fsai1i.fsf@alter.siamese.dyndns.org> (Pulling Nico in for Q2 below. No snipping so he has a context) 2012/1/31 Junio C Hamano : > Nguyễn Thái Ngọc Duy   writes: > >> Commit f7c22cc (always start looking up objects in the last used pack >> first - 2007-05-30) introduces a static packed_git* pointer as an >> optimization.  The kept pointer however may become invalid if >> free_pack_by_name() happens to free that particular pack. >> >> Current code base does not access packs after calling >> free_pack_by_name() so it should not be a problem. Anyway, move the >> pointer out so that free_pack_by_name() can reset it to avoid running >> into troubles in future. >> >> Signed-off-by: Nguyễn Thái Ngọc Duy >> --- > > Thanks. Two curiosities: > > - Why is there no hunk to actually clear the pointer in > free_pack_by_name() in this patch? I think it's there (the patch did work for me when I tried to integrate repack to pack-objects). -- 8<-- > @@ -720,6 +722,8 @@ void free_pack_by_name(const char *pack_name) > close_pack_index(p); > free(p->bad_object_sha1); > *pp = p->next; > + if (find_pack_entry_last_found == p) > + find_pack_entry_last_found = (void*)1; > free(p); > return; > } -- 8< -- > - Could we make the magic (void *)1 value a #define'd constant? Perhaps > we could even use NULL for that purpose? Q1. Sure. Q2. No NULL is probably not suitable. I think Nico wanted to express "we tried to find but found none (i.e. NULL)" too and 1 means "no we have not tried". >> diff --git a/sha1_file.c b/sha1_file.c >> index 88f2151..4ecc953 100644 >> --- a/sha1_file.c >> +++ b/sha1_file.c >> @@ -54,6 +54,8 @@ static struct cached_object empty_tree = { >>       0 >>  }; >> >> +static struct packed_git *find_pack_entry_last_found = (void *)1; >> + >>  static struct cached_object *find_cached_object(const unsigned char *sha1) >>  { >>       int i; >> @@ -720,6 +722,8 @@ void free_pack_by_name(const char *pack_name) >>                       close_pack_index(p); >>                       free(p->bad_object_sha1); >>                       *pp = p->next; >> +                     if (find_pack_entry_last_found == p) >> +                             find_pack_entry_last_found = (void*)1; >>                       free(p); >>                       return; >>               } >> @@ -2012,14 +2016,13 @@ int is_pack_valid(struct packed_git *p) >> >>  static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e) >>  { >> -     static struct packed_git *last_found = (void *)1; >>       struct packed_git *p; >>       off_t offset; >> >>       prepare_packed_git(); >>       if (!packed_git) >>               return 0; >> -     p = (last_found == (void *)1) ? packed_git : last_found; >> +     p = (find_pack_entry_last_found == (void *)1) ? packed_git : find_pack_entry_last_found; >> >>       do { >>               if (p->num_bad_objects) { >> @@ -2046,16 +2049,16 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e) >>                       e->offset = offset; >>                       e->p = p; >>                       hashcpy(e->sha1, sha1); >> -                     last_found = p; >> +                     find_pack_entry_last_found = p; >>                       return 1; >>               } >> >>               next: >> -             if (p == last_found) >> +             if (p == find_pack_entry_last_found) >>                       p = packed_git; >>               else >>                       p = p->next; >> -             if (p == last_found) >> +             if (p == find_pack_entry_last_found) >>                       p = p->next; >>       } while (p); >>       return 0; -- Duy