Re: [PATCH/RFC 2/2] Make path-limiting be incremental when possible.

Linus Torvalds [off-list ref] writes:

Sadly, it seems to react really badly to Junio's new --boundary logic for 
some reason that I haven't quite figured out yet.

There already was a report that --boundary stuff is not quite
right, so what you are seeing might be that the new code exposes
its original breakage even more.  I haven't looked into the
breakage of the original version yet either, so I cannot really
say how your change breaks it.

That reaction is independent of the actual pathname restriction, and seems 
to be related to how the --boundary logic expected 
pop_most_recent_commit() to work. In particular:

	...
                        if (commit->object.flags & BOUNDARY) {
                                /* this is already uninteresting,
                                 * so there is no point popping its
                                 * parents into the list.
                                 */

that code is magic, and seems to depend on something subtle going on with 
the list, and the incremental thing already popped the parent earlier and 
screwed up whatever magic that the BOUNDARY code depends on.

This was not so magic, but the magic was actually in the code
added to limit_list().  Usually, "newlist" consists interesting
commits, and what are found interesting initially but marked as
uninteresting when a different ancestry chain coming from an
uninteresting head leading to it was later discovered.  The
magic code looks at still-interesting commits, and re-injects
its parents that are uninteresting to the list (and I just
spotted a bug there -- it does not check if what is being "re-"
injected are already on the list -- should I check for SEEN flag
there perhaps?), while marking them as boundary.  This was done
to make sure that all the open-circle (in gitk) commits are on
the resulting list.

The part of the code you quoted is just a short-cut for not
doing pop_most_recent_commit() -- we used to have
pop_most_recent_commit() there, which pushed the parents of the
commit being processed into the list.  Because we are processing
a boundary commit, we know it is uninteresting -- and by
definition, its parents are uninteresting and that is why it
just advances the list without calling pop_most_recent_commit(),
bypassing its side effect to push its parents into the list.

Since the new code has already advanced the list immediately
after the beginning of do {} block, I think you can remove the
entire "if (revs->max_count) {}" block.  As the code currently
stands, you are skipping what happens to be next to the boundary
commit on the result list.

Junio? I think you did some funky special case with BOUNDARY commits, and 
I broke it for you, can you look at the patch and see if you can see it? 
I'd really like to have the incremental path-limiter, because it really 
makes a huge difference in the usability of "git log pathname".

Oh, there is no question about making it streamable in more
cases is a good thing.