Hi,
I think I either found a bug in git log, or I'm working with a broken
repository. I can reproduce this with current git master.
I'm trying to list the last n commits since a given commit on a given
branch like this:
git log -n N commit.. branch
The problem is, if there are less than N commits that match the
criteria, git log also prints the very first commit of the repository.
I'm operating on a bare repository here. When I actually check out the
branch I'm interested in, git log behaves as expected.
I've uploaded the .git directory to
http://crux.nu/~tilman/broken_repo.tar.bz2 (use -C to extract!)
Reproduce like this:
mkdir /tmp/blah
cd /tmp/blah
tar xjf broken_repo.tar.bz2
git log -n 3 --abbrev-commit --pretty=oneline \
1dd567d596b072e3ce44ea5ad8c373871686b078.. 2.4
The output I'm getting is:
47f585a... syslinux: Updated 3.54 -> 3.60
b3444e1... lzma: 4.32.4 -> 4.32.5
d5d6fa1... Created repository
When I check out the "2.4" branch and run the git log command again, I
get the expected output:
47f585a... syslinux: Updated 3.54 -> 3.60
b3444e1... lzma: 4.32.4 -> 4.32.5
Any idea on what's going on there?
Thanks,
Tilman
--
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?
The space betwen ".." and "2.4" means that they are two separate
arguments. Thus the second part of your ".." operator is blank, which is
treated as HEAD. Thus it is equivalent to:
1dd567d596b072e3ce44ea5ad8c373871686b078..HEAD 2.4
When you switch to branch 2.4, then 2.4 becomes your HEAD.
That being said, the commit in your 'master' branch _is_ part of
1dd567d5, and should be culled. So I'm not clear on why it shows up only
when you ask to see both branches, and that may be a bug.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:44:09
On Sat, Feb 02, 2008 at 10:00:54PM -0500, Jeff King wrote:
That being said, the commit in your 'master' branch _is_ part of
1dd567d5, and should be culled. So I'm not clear on why it shows up only
when you ask to see both branches, and that may be a bug.
OK, there is definitely a bug here, but I'm having some trouble figuring
out the correct fix. It's in the revision walker, so I have cc'd those
who are more clueful than I.
You can recreate a problematic repo using this script:
-- >8 --
mkdir repo && cd repo
git init
touch file && git add file
commit() {
echo $1 >file && git commit -a -m $1 && git tag $1
}
commit one
commit two
commit three
git checkout -b other two
commit alt-three
git checkout master
git merge other || true
commit merged
commit four
-- 8< --
So a fairly simple repo, but with the key element that it contains a
merge. Now try this:
git log one --not four
You get the 'one' commit, even though it should be removed by "--not
four". But if you try this:
git log one --not two
you correctly get no output.
It seems that in limit_list, we do two things:
- first add the 'one' commit to the new list (since we process it
before it gets marked uninteresting)
- then traverse from 'four', marking commits and their parents as
uninteresting as we go
However, the traversal seems to have trouble going over the merge. We
add the parents, but we end up marking them all as uninteresting, and
the everybody_uninteresting() optimization triggers, quitting the limit
before we have a chance to reach back to 'one' and mark it. The patch
below fixes it, but I'm very uncertain whether there is something else
going on that I'm missing that should be handling this case.
---
OK, there is definitely a bug here, but I'm having some trouble figuring
out the correct fix. It's in the revision walker, so I have cc'd those
who are more clueful than I.
Ok, I agree that there is a bug, and your two-liner fix is a "fix" in that
it works, but I think it's absolutely the wrogn fix because it is totally
unacceptable from a performance angle. We obviously need to break out of
the loop before we have walked the whole commit chain.
if (obj->flags & UNINTERESTING) {
mark_parents_uninteresting(commit);
- if (everybody_uninteresting(list))
- break;
continue;
}
So I think the real problem here is not that the logic is wrong in
general, but that there is one *special* case where the logic to break out
is wrong.
And that special case is when we hit the root commit which isn't negative.
That case is special because *normally*, if we have a positive commit, we
will always continue to walk the parents of that positive commit, so the
"everybody_interesting()" check will not trigger. BUT! If we hit a root
commit and it is positive, that won't happen (since, by definition, it has
no parents to keep the list populated with), and now we break out early.
So I think your fix is wrong, but it's "close" to right: I suspect that we
can fix it by marking the "we hit the root commit" case, and just
disabling it for that case.
This patch is untested and obviously won't even compile (I didn't actually
add the "hit_root" bitfield to the revision struct), but shows what I
*think* should fix this issue, without the performance problem.
But maybe I haven't thought it entirely through, and there is some other
case that can trigger this bug.
So please somebody double-check my thinking.
Linus
---
This patch is untested and obviously won't even compile (I didn't actually
add the "hit_root" bitfield to the revision struct), but shows what I
*think* should fix this issue, without the performance problem.
Ok, so I was lazy. Here's the updated patch that actually compiles and is
also now verified to fix Junio's test-case.
(Same patch, just the added bitfield declaration, and the testing ;)
Linus
---
revision.c | 5 ++++-
revision.h | 3 ++-
2 files changed, 6 insertions(+), 2 deletions(-)