[MERGE] Refactor diffing

Robert Collins robertc at robertcollins.net
Wed Nov 21 19:39:51 GMT 2007


On Wed, 2007-11-21 at 13:06 -0500, Aaron Bentley wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Hi all,
> 
> This patch refactors diffing:
> - - It removes a common use of the Tree.inventory member.  We would
> ideally like to make .inventory an implementation detail of several tree
> types, rather than part of the public interface.  This is incremental
> progress toward that goal
> - - It cleans up the code significantly
> - - By moving responsibility to a Command object, it should make it easier
> to use different diff implementations in the future (currently hacked
> around by difftools and bzrtools' colordiff).
> - - Its handling of file-kind changes is clearly defined.

bb:approve

I think this is fine as it stands. However I have a proposal about the
hooking in of diff that I'd like you to consider.

The use case is to make it possible for external plugins to do things
like 'diff gpg keys only'

So I'd like to suggest that you need to classes:
TreeDiffer - a command object 
 - show_diff
 - from_trees_options
 - has a new select_diff(file_id, old_path, new_path, old_label,
   new_label) method which returns a Differ instance to use.

Diff(object):
    """"Base class for diff operations between files."""

    - has a single public diff method() which returns a result
      unchanged/changed/could-not-handle

Now we don't want a bazjillion instances being created when many files
are changed, so having a list of instances to be used by TreeDiffer to
decide what to use to diff seems reasonable. You could transform the
code in your patch into a single instance, where diff() does the same
thing as at the moment. But possible having:

SymlinkDiff(Diff)
TextDiff(Diff)
BinaryDiff(Diff)

classes, so your selection list is by default:

[SymlinkDiff(), TextDiff(), BinaryDiff()]

will get the same behaviour.

Then to plug in arbitrary diffs, such as diffing for gpg files, a plugin
just needs some way to get it's own Diff subclass inserted at the front
of the list.

-Rob
-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20071122/a58ee1e1/attachment.pgp 


More information about the bazaar mailing list