Re: [PATCH v3 0/4] Speed up unpack_trees()

Junio C Hamano [off-list ref] writes:

Junio C Hamano [off-list ref] writes:

quoted

not used behind is *not* OK.  And lack of restoring the bottom in
the new codepath makes me suspect exactly such a bug _after_ the
traversal exits the subtree we are using this new optimization in
and moves on.

Hmph, thinking about this further, I cannot convince myself that
lack of bottom adjustment can lead to a triggerable bug.  The only
case that a subtree traversal need to skip some unpacked entries in
the index and then revisit them by rewinding, e.g. entries "t-i" and
"t-j" that are left unprocessed while entries "t/1", "t/2", etc. are
processed, in the illustration of da165f47 ("unpack-trees.c: prepare
for looking ahead in the index", 2010-01-07), is when one of the
trees have a non-tree with the same name as the subtree we are
trying to descend into, and as long as we know all trees have the
thing as a tree, I do not think of a case where such ordering
inversion would get in the way.

One more, and hopefully the final, note.

A paranoid may be soothed by a simple "cache_bottom must match pos
at this point" at the beginning of the optimized traversal.  Just
like you already have an assert to ensure that pos points at the
first entry in the directory in index_pos_by_traverse_info(), there
should not be any unused entry in the index before that entry and
the bottom pointer must be pointing at it.  It is a cheap check, and
if violated, would indicate that the above "I do not think of a
case ..." was incomplete.

That was the only thing I found questionable in 2/4, which is the
most important piece in the series, so we probably are OK.

Thanks for working on this one.

On Wed, Aug 8, 2018 at 8:39 PM Junio C Hamano [off-list ref] wrote:

One more, and hopefully the final, note.

..

Much appreciated. I don't think I could figure all this out no matter
how long I stare at those commits and current code.
-- 
Duy