[MERGE] annotate on working tree with edits
Aaron Bentley
aaron.bentley at utoronto.ca
Fri Oct 5 02:27:32 BST 2007
-----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-----
More information about the bazaar
mailing list