[MERGE] Redo annotate more simply, using just the public interfaces for VersionedFiles.

[John Arbash Meinel]

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

Martin Pool wrote:
> I added this additional patch to make test_annotate pass in the
> "named-nograph-knit-pack) case, where get_parents returns None as the
> parents of all requested revisions.
> 
> This patch is not quite satisfactory because it seems like a bit of a
> hack when the code is not being clear about just what should be
> expected in these values.  Feedback would be very welcome.

You mean get_parent_map() not get_parents(), or is this a layering issue?
get_parent_map() should be returning nothing for entries that are not present,
not None.
If we have one that is returning None, I think we should change it, so that we
don't have lots of hackery in all of our callers.

As they aren't ghosts, though, it should *probably* be returning either the
empty list or the (NULL_REVISION,) key.

I'm *really* unhappy that I don't have a straight answer for you. Personally,
I find the (NULL_REVISION,) hack a bit crufty, and I never quite know when we
need to insert it or not. The api I feel is cleanest would be:

1) get_parent_map(['existing_key']) => {'existing_key':('parent1', 'parent2')}

2) get_parent_map(['ghost']) => {}

3) get_parent_map(['first-rev']) => {'first-rev':()}

I suppose with the VersionedFiles changes, all those become tuples. Now, I'm
somewhat okay with changing (3) to be:

3b) get_parent_map(['first-rev']) => {'first-rev':(NULL_REVISION,)}
    get_parent_map(['NULL_REVISION']) => {'NULL_REVISION':()}

However, if we do that, I would really like to see our records on disk change.
So that we store NULL_REVISION as the parent of a record with no other
parents. Right now, we have a lot of friction, and have to loop over
everything we read, to make sure that we don't need to insert NULL_REVISION
here and there, and strip it off here and there.

I prefer the original (3) because it doesn't lead to cases where we ask a
knit/repository/whatever for the text for NULL_REVISION, and *it* has to
special case to recognize that it is a special revision.

Anyway, we are where we are, and it sounds like get_parent_map() for these
objects needs to be returning (NULL_REVISION,) instead of None everywhere
Because I *think* it is saying, I have these revisions, but they have no
parents. If it is actually saying "I don't have these revisions" then it
should be returning an empty dict.

John
=:->



> 
> === modified file 'bzrlib/graph.py'
> --- bzrlib/graph.py	2008-05-29 20:17:37 +0000
> +++ bzrlib/graph.py	2008-07-07 07:06:46 +0000
> @@ -1228,6 +1228,8 @@
>          parent_map = self._parents_provider.get_parent_map(revisions)
>          found_revisions.update(parent_map)
>          for rev_id, parents in parent_map.iteritems():
> +            if parents is None:
> +                continue
>              new_found_parents = [p for p in parents if p not in self.seen]
>              if new_found_parents:
>                  # Calling set.update() with an empty generator is actually
> 
> === modified file 'bzrlib/knit.py'
> --- bzrlib/knit.py	2008-07-04 04:32:12 +0000
> +++ bzrlib/knit.py	2008-07-07 07:26:08 +0000
> @@ -2714,10 +2714,17 @@
>          for record in self._knit.get_record_stream(keys, 'topological', True):
>              key = record.key
>              fulltext = split_lines(record.get_bytes_as('fulltext'))
> -            parent_lines = [parent_cache[parent] for parent in parent_map[key]]
> +            parents = parent_map[key]
> +            if parents is not None:
> +                parent_lines = [parent_cache[parent] for parent in
> parent_map[key]]
> +            else:
> +                parent_lines = []
>              parent_cache[key] = list(
>                  reannotate(parent_lines, fulltext, key, None, head_cache))
> -        return parent_cache[key]
> +        try:
> +            return parent_cache[key]
> +        except KeyError, e:
> +            raise errors.RevisionNotPresent(key, self._knit)
> 
> 
>  try:
> 

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFIci2TJdeBCYSNAAMRApbHAKCQH08W/v1xcEmP1axW49D5tnR1OgCgrvjZ
wFow2DzWR+osn5sXzfDg/Y4=
=CICb
-----END PGP SIGNATURE-----