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

[Junio C Hamano <junkio@cox.net>]

Linus Torvalds <torvalds@osdl.org> 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.