[RFC] Refactor diffing

[Robert Collins]

On Thu, 2007-11-22 at 15:36 -0500, Aaron Bentley wrote:

> > 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.

Oh it doesn't; it just makes it simpler to make one, and avoids what is
for various differs just noise.

> >> 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.

Ah great. That makes more sense. (I'd actually like to combine things
here - a diff that selects on path and an external diff invoker, so as
to get xmldiff on .xml files, etc).

> >> 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.

Violent agreement then :).

> >> 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.

[I must stop eliding thoughts.]*50

Its quite easy to create a factory that binds additional state to a
factory. Heck the factory could even be a trivial bound method e.g.

class SomeDiff()..
...
    def set_tree_diff(self, tree_diff):
        self.tree_diff = tree_diff
        return self

...
mydiff = SomeDiff(...)
TreeDiff.from_options(extra_diff_factories=[SomeDiff.set_tree_diff])

> >> 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".

Sounds nice.

I'll review your patch shortly.

Cheers,
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/20071123/0847b30c/attachment-0001.pgp