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

From: Junio C Hamano <junkio@cox.net>
Date: Mar 31, 2006, 06:45 UTC
Message-ID: <7v3bgzxgbg.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.64.0603302153350.27203@g5.osdl.org>

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.

Show 14 quoted lines

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