[MERGE] annotate on working tree with edits

[Aaron Bentley]

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

Ian Clatworthy wrote:
> With patch this time. :-(
>  def annotate_file(branch, rev_id, file_id, verbose=False, full=False,
> -                  to_file=None, show_ids=False):
> +                  to_file=None, show_ids=False, tree=None):

bb:resubmit

This is an awful lot of parameters.  At first glance, a lot of them seem
redundant.

In theory, we don't need branch, because we have rev_id.

It would be really nice if there was a Tree.closest_revision() method.
For revision trees, it would be equivalent to
RevisionTree.get_revision_id, and for WorkingTrees, it would be
equivalent to WorkingTree.last_revision().  I've run into several places
(e.g. merge code) where we wanted something like this.

If we had Tree.closest_revision, we wouldn't need rev_id either.

So we could just have a Tree and a Repository as the inputs.

That being said, since our API isn't there, I'd like to see tree as a
mandatory parameter.

I think you should factor the body out into another method.
I'd like the call to be annotate_file(file_id, tree, rev_id, branch,
...), showing the dependencies better.

> +    """Annonate a file.

I think you mean "Annotate"


> +
> +    :param branch: the associated branch
> +    :param rev_id: the revision identifier

^^^ more needs to be said, if this is really the closest_revision

> +    :param file_id: the identifier of the file

^^^^ description is redundant with name.

> +    :param verbose: if True, show more information

^^^ a very terse description of verbosity

> +    :param full: if True, show information against every line

^^^ And if False?

> +    :param show_ids: if True, show just a revision id column in
> +        addition to the text

^^^ This should be called revision_ids_only or something

> +    :param tree: if non None, the tree to get the annotations from
> +        (using just the file_id) instead of getting them from the
> +        branch (using the file_id and rev_id)

^^^ a description of why you would want to supply a tree would help.
(i.e. that it enables new functionality, not just optimization)

Also, I'm not sure that requiring a file is a good idea.

> +def _annotations(repo, file_id, rev_id, tree):

> +    """Return the list of (origin,text) for a revision of a file.
> +    
> +    :param tree: if non None, the tree to get the annotations from
> +        (using just the file_id) instead of getting them from the
> +        repository (using the file_id and rev_id)

I think the tree should be mandatory here.

> +                file_version = tree.inventory[file_id].revision
> +                annotate_file(branch, file_version, file_id, long, all,
> +                    self.outf, show_ids, tree)

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

iD8DBQFHBZME0F+nu1YWqI0RAtg2AJ4orORiCS2M4WYKnngqaFY9XIxcTgCeIcYm
SOi4wLRNb4DHTDC3D1g1Yu0=
=VyZ3
-----END PGP SIGNATURE-----