bug https://bugs.launchpad.net/bzr/+bug/235055

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Tom Cato Amundsen wrote:
| Hello! Is the following a safe workaround for bug
| https://bugs.launchpad.net/bzr/+bug/235055 ?
| #235055 is a problem for GNU Solfege that converted the from using gnu
| arch to bzr.
|
| Tom Cato
|
| === modified file 'bzrlib/repository.py'
| --- bzrlib/repository.py        2008-05-29 12:27:24 +0000
| +++ bzrlib/repository.py        2008-06-02 07:59:38 +0000
| @@ -1567,7 +1567,10 @@
|              # RevisionNotPresent here until we see a use for it,
| because of the
|              # cost in an inner loop that is by its very nature O(history).
|              # Robert Collins 20080326
| -            parents = graph.get_parent_map([next_id])[next_id]
| +            try:
| +                parents = graph.get_parent_map([next_id])[next_id]
| +            except KeyError, e:
| +                return
|              if len(parents) == 0:
|                  return
|              else:
|
|

I would like Robert to comment on this one, as he is the one who changed the
code last.

Basically, in bzr <1.3 the 'Branch.revision_history()' would just stop when it
reached a ghost. So let's say your ancestry looked something like:

~ A
~ |
~ B
~ |\
~ C D
~ |/
~ E

But A is a ghost. I believe in older formats (Branch5) we would record the
mainline history as just "B C E".
In newer formats (Branch6) we only store the final pointer "E" and work out the
rest of the revisions on the fly (iter_reverse_revision_history()).

In <= 1.2 we did this:
while True:
~    yield next_id
~    parents = versionedfile.get_parents(next_id)
~    if len(parents) == 0:
~        return
~    else:
~        next_id = parents[0]

~From what I can tell, get_parents() actually tries to raise RevisionNotPresent
if it actually encounters a ghost. With one caveat, it won't return a parent if
it is a ghost:

def get_parents(self, version_id):
~    """Return parents of specified version ignoring ghosts."""
~    return [parent for parent in self._cache[version_id][4]
~            if parent in self._cache]

Which means that it would return:

E => C, C => B, B => [] (because it knew that A was a ghost and would thus
filter it out.)

In 1.4? we switched to:

parents = graph.get_parent_map([next_id])[next_id]

And now this returns
E => C, C => B, B => A, A => key error (ghost)

I believe I mentioned this to Robert at the time (though possibly not quite as
clearly as this.) I believe his comment was that he wanted it to explicitly
fail, rather than silently succeed. I could understand why he would want it, but
it does mean that 'bzr log' dies on a branch that has a ghost in its mainline
history. Which, as you see, can happen in some circumstances. (Mainly in
conversions from Arch, which had an easy time losing parts of history, because
it didn't copy history between archives.)


I should also mention why we switched apis. We stopped using "get_parents()"
because it *had* to determine if a parent was a ghost. Which meant that for
every revision, it ended up querying all parents to see if they were ghosts, and
then queried them again when we actually got there. So if you had perfectly
linear history, it doubled the number of requests. On most projects with a bunch
of merges in the middle, it would double the number of requests *plus* an
additional request for all merged revisions, even though we wouldn't use that
information.

Personally, I'm fine with the try/except KeyError: return change. Though we
would want to add a test for how "iter_reverse_revision_history()" should work
in the presence of a mainline ghost. Something like:

~ tree = self.make_branch_and_tree('tree')
~ tree.set_parent_ids(['ghost'], allow_leftmost_ghost=True)
~ rev1 = tree.commit('with ghost parent')
~ rh = tree.branch.revision_history()
~ self.assertEqual([????], rh)
~ self.assertEqual(reversed(rh),
~     list(tree.branch.repository.iter_reverse_revision_history(rev1))

If we chose to go this route, then ???? would just be [rev1]. Other
possibilities would be ['ghost', rev1], since we know that we *should* be
stepping to 'ghost', we just don't know what lies beyond it. (I don't know what
that would do to 'log' though, to be passing it a ghost in the mainline
history.) Suppressing the revision is "wrong" in a sense, but at least nobody
tries to do anything with it.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkhEEw0ACgkQJdeBCYSNAAPqDACeJ6xeO/zqy49c9V5XKnINn3Cb
MkUAnibFNujk/2pU2xKVW1kegm+tZgWQ
=fzzK
-----END PGP SIGNATURE-----