Re: [PATCH v2] find_pack_entry(): do not keep packed_git pointer locally
- From
Nicolas Pitre <nico@fluxnic.net>
- Date
- Jan 31, 2012, 19:28 UTC
- Message-ID
- <alpine.LFD.2.02.1201311418430.2759@xanadu.home>
- In-Reply-To
- <7vr4yf92dg.fsf@alter.siamese.dyndns.org>
On Tue, 31 Jan 2012, Junio C Hamano wrote:
Show 7 quoted lines
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > > > + /* > > + * p can be NULL from the else clause above, if initial > > + * f_p_e_last_found value (i.e. INVALID_PACK) is NULL, we may > > + * advance p again to an imaginary pack in invalid memory > > + */
Ah! OK so that's why I initially came up with that (void*)1 value.
> But I think the real issue is that the original loop is written in an > obscure way.
Can't disagree with that, especially when the original author (myself) doesn't see clearly through it anymore.
Show 6 quoted lines
> The conversion in f7c22cc (always start looking up objects in the last > used pack first, 2007-05-30) wanted to turn the traversal that always > went from the tip of a linked list to instead first probe the > promising one, and then scan the list from the tip like it used to do, > except that it did not want to probe the one it thought promising > again.
Exact.
Show 19 quoted lines
> So perhaps restructuring the loop by making the logic to probe into a
> single pack into a helper function, e.g.
>
> static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
> {
> if (last_found) {
> if (find_one(sha1, e, last_found))
> return 1;
> }
> for (p = packed_git; p; p = p->next) {
> if (p == last_found || !find_one(sha1, e, p))
> continue;
> last_found = p;
> return 1;
> }
> return 0;
> }
>
> would make the resulting flow far easier to follow, no?Indeed.
Nicolas