[RFC] Refactor diffing

[Robert Collins]

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

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

> 2. Support "diff --diff-options -p": allow the caller to supply a text
> differ that does not take precedence over other file differs.
>
> 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):
    """Common logic for Diff that only works on some paths."""

    def diff(self, ...):
        if not (self._path_select.search(old_path) and
            self._path_select.search(new_path)):
            return Diff.cannot_diff
        return self._diff(...)

    def _diff(self, ...):
        """Helper function called when a path is selected for diff."""
        raise NotImplementedError(self)


> 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:
    self.old_tree = old_tree
    self.new_tree = new_tree
    self.to_file = to_file
    self.path_encoding = path_encoding
    self.differs = []
    differ_factories = []
    if extra_differ_factories is not None:
        differ_factories.extend(extra_differ_factories)
    differ_factories.extend(TreeDiffer._differ_factories)
    for differ_factory in differ_factories:
        self.differs.append(differ_factory(self))

This will couple the TreeDiffer and the Diff family together, but it
seems to reduce duplication the most. I only recall one place in your
patch where this would be a bit annoying, and that was in code that we
want to overhaul eventually anyway.

> Some additional questions:
> 
> Is the current grouping of files by change type (added, deleted,
> modified, renamed...) desirable?   Personally, I'd rather see it
> sorted
> in alphabetical order.  That would also allow us to use _iter_changes
> directly, and reduce the number of similar loops in our code.

I have no opinion about that.

> If we switch to using _iter_changes, what about using ChangeReporter?
> 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?

-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/7e5cd072/attachment.pgp