[RFC] Make diffstat work with bazaar 1.5

[John Arbash Meinel]

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

Russ Brown wrote:
| Hi,
|
| I'd appreciate a review of this since it's my first go at doing anything
| with bzrlib. This diff gets the diffstat plugin working with bzr 1.5.
|
| The crux of the issue was that diff_cmd_helper had been removed from
| bzrlib. With some help from IRC I figured that show_diff_trees was what
| it should be using instead. This lead to a number of other changes being
required too.
|
| Specifics that I think need reviewing are the uses of various functions
| that obtain the trees to compare, and also how the revision number
| parameters that are passed (if any) in are used. I've taken it to work
| like this:
|
|  * If no revision is given, show HEAD -> WC
|  * If one revision is given, show REV -> WC
|  * If two revisions are given, show REV1 -> REV2
|
| Seems sensible to me, but others might not think so.
|
| Thanks,
| -- Russ
|
|

That is the logic we use for 'bzr diff' as well, so it seems reasonable to me.

I know there are some small issues with 'diff' itself, because it can take a lot
of possible permutations.

One issue is that you do:

branch = bzrdir.open_branch()
....

~  tree = bzrdir.open_workingtree()

That has the problem that you have 2 objects that are accessing the branch.
(tree.branch is a separate object from the 'branch' you have here.)

Also, when possible you want to use 'wt.basis_tree()', not "branch.basis_tree()"
~ (it enables a fast code path when doing diff.)

So I would do it as:

if not revision:
~  new_tree = bzrdir.open_workingtree()
~  branch = new_tree.branch
~  old_tree = new_tree.basis_tree()
elif len(revision) == 1:
~  new_tree = bzrdir.open_workingtree()
~  branch = new_tree.branch
~  old_tree = branch.repository.revision_tree(...)
else:
~  # Two revisions, just use the branch
~  branch = bzrdir.open_branch()
~  old_tree = ...
~  new_tree = ...


I would also avoid using '__diff' and instead call it '_diff'. As __diff cannot
be overridden by children, which is not very "friendly".


It also honestly seems like we should be able to generate 'diffstat' a lot
cheaper than having to actually process the diff output. Certainly at a lower
level we have all the information you need, in structured format.

But in the short term, what you have seems okay. Oh, except everything should be
locked. (And I just noticed that you don't actually use the Branch later, nor
the bzrdir). So I would do:

locked = []
try:
~  if not revision:
~    new_tree = bzrdir.open_workingtree()
~    new_tree.lock_read()
~    locked.append(new_tree)
~    old_tree = new_tree.basis_tree()
~    old_tree.lock_read()
~    locked.append(old_tree)
~  elif len(revision == 1):
~    new_tree = bzrdir.open_workingtree()
~    new_tree.lock_read()
~    locked.append(new_tree)
~    old_tree = new_tree.branch.repository.revision_tree(
	revision[0].as_revision_id(new_tree.branch))
~    old_tree.lock_read()
~    locked.append(old_tree)
~  else:
~    branch = bzrdir.open_branch()
~    branch.lock_read()
~    locked.append(branch)
~    new_tree = ...
~    old_tree = ...

~  ds = self._diff(old_tree, new_tree, file_list)
~  ...

finally:
~  for item in reversed(locked):
~    item.unlock()

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

iEYEARECAAYFAkhEa0UACgkQJdeBCYSNAANQ/QCaAnlw6ZpwUJpavw6cZVeTV7OC
z0MAoKoU6dToFyFb+gy1dXtfCSYsrrzC
=/atc
-----END PGP SIGNATURE-----