[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