[MERGE REVIEW] --reprocess support for weaves

[Aaron Bentley]

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

Martin Pool wrote:
+class TextMerge(object):
+    """Base class for text-mergers
+    Subclasses must implement _merge_struct.

| OK, but what is _merge_struct?  Perhaps add a base implementation  that
| raises NotImplementedError and has a docstring describing what  it
| should return.  (A sequence of tuples for conflicted/non- conflicted
| regions?)

Okay, I've added a NotImplemented method for it.

+
+    Many methods produce or consume structured merge information.
+    This is an iterable of tuples of lists of lines.
+    Each tuple may have a length of 1 - 3, depending on whether  the
region it
+    represents is conflicted.
+
+    Unconflicted region tuples have length 1.
+    Conflicted region tuples have length 2 or 3.  Index 1 is  text_a,
e.g. THIS.
+    Index 1 is text_b, e.g. OTHER.  Index 2 is optional.  If
present, it
+    represents BASE.
+    """


| This could do with more documentation: how do you get one of these
| objects, and what do you do with it when you have it?

It originates in _merge_struct(), is used as input for struct_to_lines,
iter_useful, and reprocess_struct, and is the output of iter_useful,
merge_struct, and reprocess_struct.

Callers will rarely need to call anything other than merge_lines(), but
I didn't see any harm in making the others public.

| The summary should be on the same line as the opening quote.

Done.

+
+    def __init__(self, lines_a, lines_b, a_marker='<<<<<<< \n',
+                 b_marker='>>>>>>> \n', split_marker='=======\n'):
+        TextMerge.__init__(self, a_marker, b_marker, split_marker)


| To avoid repetition perhaps the default markers should be constants  in
| the base class?

Okay.

- -                                  " type. %s" % merge_type)
+                                  " type. %s" % self.merge_type)


| May be better as

|   "Reprocess is not supported for merge type %s"

| Perhaps we should switch here to a more concrete name than  "reprocess",
| e.g. "conflict reduction".

Hmm.  Yes, I like this.

+
+class PlanWeaveMerge(TextMerge):
+    """Weave merge that takes a plan as its input.
+
+    This exists so that VersionedFile.plan_merge is  implementable.
Otherwise,
+    we'd just use WeaveMerge everywhere.
+    """


| I can understand having this class split out but I don't understand
| this comment.  Would it be OK to say

|   Most callers will want to use WeaveMerge instead.

Sure.

| Does this need to be a different class?  Why not just have a
| WeaveMerge.from_plan static method, and then the regular constructor
| will get the plan and call that.

My reasoning was that a from_plan static method would still need to
produce an instance of WeaveMerge, because it needs to return an object
that supports merge_lines.  Since the WeaveMerge constructor requires
data that it doesn't have (weave, version_a, version_b), it wouldn't be
able to create an instance of WeaveMerge.

An alternative would be to allow WeaveMerge.__init__ to accept a plan
instead of the weave, version_a and version_b.  I thought it was cleaner
to have two classes.

I've attached the updated patch.

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

iD8DBQFEOfCW0F+nu1YWqI0RAjdCAJ9J+lkEQwXPBpfByMJ8rsHwA1KbfwCfYvaO
CyCO3t/YNV6FiAepWluIxUU=
=MJ5v
-----END PGP SIGNATURE-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: weave-reprocess2.patch
Type: text/x-patch
Size: 28521 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060410/3782af89/attachment.bin