[MERGE] Patience diff

Aaron Bentley aaron.bentley at utoronto.ca
Tue May 23 01:07:48 BST 2006


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

John Arbash Meinel wrote:
| First off, in your 'patience-test.py' you do:
|
|   new_file = file(file_path, 'rb')
|   for line_a, line_b in zip(new_file, new_lines):
|       assert line_a == line_b
|
| Which is great, except 'zip()' stops as soon as one of the generators
| runs out. So if one has extra stuff at the end, you won't detect it.
| Since 'assert line_a == line_b' doesn't actually say anything about what
| is different, I would tend to change it to:
|
| new_file = file(file_path, 'rb')
| assert list(new_file) == new_lines

Makes sense.

|
| We should go through and make sure that our copyright statements have
| 2006, and use # all the way through. (bzrlib/cdv/__init__.py doesn't
| have the right lines in the beginning)

It's also attributed to Canonical, and I think it should be attributed
to Bram Cohen, at least in part.

| We should also check that we have the latest 'nofrillsprecisemerge.py'
| files. I tried hard to make sure I was using an unmodified version so
| that any updates could be easily obtained.

Unfortunately, there's a lot of stuff in the original
nofrillsprecisemerge.py that's not related to what this patcheset is
meant to do.  I removed a bunch of stuff we're not (yet) using, because
it seems easier to make single-purpose changes to bzr, rather doing
several things at once.

| We probably should change the names from 'cdv' to 'patience'. I'm not
| stuck on either, but I'm guessing Codeville isn't the right name to use.

Perhaps we should pull the nofrills stuff into cdvdifflib, and rename it
to bzrlib/patiencediff.py

| In merge3.py we have a commented out import. I think we prefer to just
| remove them now.

Right.

| We also used to use a junk expression so that lines that only contained
| space,tab or # would be ignored. It probably isn't needed anymore, but
| we should be aware that the junk_re should be deleted.
|
| test_diff.py has a bunch of pep8 whitespace issues.
|
| We are using the new cdv diff for all of the Weave code, but it doesn't
| look like anything has been done to use it for the 'knit' code. (It
| should still be used for 'bzr diff')

The knit code has its own SequenceMatcher implementation, so I was
unsure whether we should replace it.

| At one point, I was thinking we could provide a flag to switch between
| the diff engines. Is there any interest in that?

I don't think it would be used often enough to put it in.

| So, +0.5 from me. It just needs a little bit of cleanup still.

Okay.

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

iD8DBQFEclJT0F+nu1YWqI0RAv47AKCA+8XlBWFT54/eFzr/nVvHjLVRuQCeL6pz
xi4KsXcU1EdOLOesYYvxRsU=
=tAib
-----END PGP SIGNATURE-----




More information about the bazaar mailing list