[RFC] Refactor diffing

Aaron Bentley aaron.bentley at utoronto.ca
Thu Nov 22 20:36:27 GMT 2007


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

Robert Collins wrote:
> On Thu, 2007-11-22 at 09:53 -0500, Aaron Bentley wrote:
>>
>> That seems like a limiting approach; I deliberately moved those
>> parameters out of the rest of the code, because they only apply to a
>> particular diff output.
> 
> It is noise for most differs, yes.
> 
>> - Forcing all other FileDiffers to accept parameters that are only
>>   relevant to unified diffs seems ugly
>> - KindChangeDiffer needs totally different parameters
>>   (i.e. the differ list)
>> - Other differ types might want other parameters.
>>
>> So I'm thinking of something similar to, but not the same as, your
>> idea.
> 
> I woke up this morning thinking, 'why not just give Diff objects the
> TreeDiffer' - e.g. differs.append(KindChangeDiffer(self))

Well, I don't see how that supports plugins registering their own
differs to be used.

>> Here are the goals I'm trying to satisfy:
>>
>> 1. Support "diff --using colordiff": allow the caller to supply
>> one-time
>> instances, such that they take precedence over other file differs.
>> (These can take any desired parameters in their constructor)
> 
> So this plugin would hook in at two places I guess? One to register code
> for the option, and then in that code to add its colourising logic. (I
> guess you don't attempt to parse the output to colourise).

I'm not talking about the bzrtools colordiff plugin.  Perhaps it would
have been clearer to use "bzr diff --using xmldiff" or something.  The
idea is that it would launch an external program.  So the actual
DifferUsing would be in core, and would happily spawn arbitrary commands.

>> 3. Support per-type plugins:  allow plugins to register differs for
>> particular file types, such that they are automatically used by
>> TreeDiffer, and take precedence over the text differ, but not other
>> supplied differs.  (These must be constructible using a standardized
>> signature)
> 
> I was thinking that 3) would be handled by:
> 
> class SelectingDiff(Diff):

Well, I think we're looking at it in different ways, and answering
different questions.  3) to me is not controversial.  We just need to
provide a place where plugins can register to have their differ used.

I'm not sure what the Diff that you're deriving from is-- I think it's
FileDiffer?  Anyhow, SelectingDiff looks like a fine way to make it
simpler for plugin authors.  But I was talking about how to make it
*possible* for plugin authors.

>> Here's how I'd do it:
>>
>> TreeDiffer.__init__(self, old_tree_new_tree, text_differ,
>>                     extra_differs=None):
>>     self.differs = []
>>     if extra_differs is not None:
>>         self.differs.extend(extra_differs)
>>     for differ_factory in TreeDiffer._differ_factories:
>>         self.differs.append(differ_factory(old_tree, new_tree,
>> to_file,
>>                             path_encoding)
>>     self.differs.extend(text_differ, KindChangeDiffer(self.differs))
> 
> I think the extra_differs parameter is ok, though if those differs will
> need the same state - old_tree, new_tree, to_file, path_encoding - then
> I would recommend that it be extra_differ_factories, and you do:

No, they won't necessarily be constructed with the same state.  They may
take more state.

>> We could still support the same output, I believe, plus we could also
>> support short-status output.
> 
> short-status in diff? What would that look like?

Instead of
=== renamed directory 'me' => 'you'*

You'd get
=== R me/ => you/

And you'd additionally be able to get info about unknowns and things.
So that diff would be more like "status"+"diff".

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

iD8DBQFHRehL0F+nu1YWqI0RArS2AKCE+0tx46zWFIWhApL2BMoSASw54gCgiOP2
I21SKDte4ZFz5aUngK1FNRA=
=lrBL
-----END PGP SIGNATURE-----



More information about the bazaar mailing list