[RFC] Refactor diffing
Robert Collins
robertc at robertcollins.net
Thu Nov 22 20:54:32 GMT 2007
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
More information about the bazaar
mailing list