Re: [PATCH] find_pack_entry(): do not keep packed_git pointer locally

From: Nicolas Pitre <nico@fluxnic.net>
Date: Jan 31, 2012, 04:19 UTC
Message-ID: <alpine.LFD.2.02.1201302301150.2759@xanadu.home>
In-Reply-To: <CACsJy8AS0nZOoXZZfz0OEwoWe88wp2aGR5NGqpG7xQmUvwi7TA@mail.gmail.com>

On Tue, 31 Jan 2012, Nguyen Thai Ngoc Duy wrote:

Show 9 quoted lines

> (Pulling Nico in for Q2 below. No snipping so he has a context)
> 
> 2012/1/31 Junio C Hamano <gitster@pobox.com>:
> > 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.

Hmmm, good point.

Show 7 quoted lines

> >> 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>
> >> ---

[...]

> >  - Could we make the magic (void *)1 value a #define'd constant? Perhaps
> >   we could even use NULL for that purpose?
> 
> Q1. Sure.

Indeed. The idea might have been to use a non null value that cannot match any pointer...

> 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".

Well, I could imagine I might have thought about something like that. However, looking at the latest code in the master branch I can't see any way for last_found to ever be assigned a NULL value. So if the (void*)1 value might have been useful, it is certainly not anymore.

Nicolas