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

[Nicolas Pitre <nico@fluxnic.net>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1509 bytes --]

On Tue, 31 Jan 2012, Junio C Hamano wrote:

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

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

> 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