Linus Torvalds <torvalds@xxxxxxxx> 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.
-
: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html