Re: [PATCH] find_pack_entry(): do not keep packed_git pointer locally
- From
Nguyen Thai Ngoc Duy <pclouds@gmail.com>
- Date
- Jan 31, 2012, 02:01 UTC
- Message-ID
- <CACsJy8AS0nZOoXZZfz0OEwoWe88wp2aGR5NGqpG7xQmUvwi7TA@mail.gmail.com>
- 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 <gitster@pobox.com>:
Show 19 quoted lines
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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 <pclouds@gmail.com> >> --- > > 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<--
Show 9 quoted lines
> @@ -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".
Show 58 quoted lines
>> 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