On Tue, 31 Jan 2012, Junio C Hamano wrote:
> Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> 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