Summary

Centralize get_change() in the SVN backends.

Review Request #5982 — Created June 11, 2014 and submitted June 12, 2014, 4:40 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-2.0.x

Bugs

Depends On

Commit

193369a...

Reviewers

Groups

reviewboard

People

Description

This provides a single implementation of get_change(). It makes use of

the new get_log(), and also a new diff() function in the backends. This

helps keep the code maintainable and bug-free.
I've removed the commit cache stuff that was part of get_change(), since

we never actually ever saved anything in the cache. It's probably

unlikely we'll need to fetch the same commit info very often, so instead

of fixing the caching, I've just opted to save some memory and accesses

and remove it.
There's a couple fixes in here I ran into for revision normalization as

well, and the removal of definitions and properties that are no longer

needed.

Testing Done

Unit tests pass for both backends.

Issues

Description	From	Last Updated
This should say that the returned diff is a unicode object. You're calling .decode() which means it's not encoded as …	david	June 12, 2014, 12:13 p.m.
I'm not sure if the diff command can raise exceptions, but the .decode could. We should probably wrap this in …	david	June 12, 2014, 12:14 p.m.
Same comment about "encoded" vs "unicode object"	david	June 12, 2014, 12:14 p.m.
Should we check err here?	david	June 12, 2014, 4:39 p.m.
undefined name '_'	reviewbot	June 12, 2014, 1:29 p.m.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/scmtools/svn/subvertpy.py
    reviewboard/scmtools/svn/pysvn.py
    reviewboard/scmtools/svn/__init__.py
    reviewboard/scmtools/svn/base.py
  Ignored Files:

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    reviewboard/scmtools/svn/subvertpy.py
    reviewboard/scmtools/svn/pysvn.py
    reviewboard/scmtools/svn/__init__.py
    reviewboard/scmtools/svn/base.py
  Ignored Files:

reviewboard/scmtools/svn/pysvn.py (Diff revision 1)

The issue has been resolved. Show all issues

This should say that the returned diff is a unicode object. You're calling .decode() which means it's not encoded as anything.

reviewboard/scmtools/svn/pysvn.py (Diff revision 1)

The issue has been resolved. Show all issues

I'm not sure if the diff command can raise exceptions, but the .decode could. We should probably wrap this in a try/finally to make sure that we call the rmtree.

reviewboard/scmtools/svn/subvertpy.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same comment about "encoded" vs "unicode object"
```

reviewboard/scmtools/svn/subvertpy.py (Diff revision 1)

The issue has been dropped. Show all issues

Should we check err here?

chipx86 June 12, 2014, 12:15 p.m.

Maybe. I have no idea what codes to expect here. The old code didn't check for it at all.

chipx86 June 12, 2014, 1:26 p.m.

Okay, err is a file handle that contains any error output generated by the diff process. Looks like it's just the STDERR of running diff, coming from libsvn, and there's no documentation about it's to be used. So, I'd like to ignore that for now :)

Change Summary:


Improved error handling when diffing.
Fixed docstrings to mention Unicode objects instead of UTF-8 strings.

Commit:

ace784c61ef013fda39136039eb03322eaef6f4b

e604f7382c4b29b0178f4c5fcc5f797f27cbf6bf

Diff:

Revision 2 (+116 -109)

Show changes

	reviewboard/scmtools/svn/__init__.py
	reviewboard/scmtools/svn/base.py
	reviewboard/scmtools/svn/pysvn.py
	reviewboard/scmtools/svn/subvertpy.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/scmtools/svn/subvertpy.py
    reviewboard/scmtools/svn/pysvn.py
    reviewboard/scmtools/svn/__init__.py
    reviewboard/scmtools/svn/base.py
  Ignored Files:

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    reviewboard/scmtools/svn/subvertpy.py
    reviewboard/scmtools/svn/pysvn.py
    reviewboard/scmtools/svn/__init__.py
    reviewboard/scmtools/svn/base.py
  Ignored Files:

reviewboard/scmtools/svn/subvertpy.py (Diff revision 2)
The issue has been resolved. Show all issues
```
 undefined name '_'
```

Change Summary:

Fixed a missing import for _()

Commit:

e604f7382c4b29b0178f4c5fcc5f797f27cbf6bf

193369af79689e89eb649ce8048dc7d2b0dadc82

Diff:

Revision 3 (+117 -109)

Show changes

	reviewboard/scmtools/svn/__init__.py
	reviewboard/scmtools/svn/base.py
	reviewboard/scmtools/svn/pysvn.py
	reviewboard/scmtools/svn/subvertpy.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/scmtools/svn/subvertpy.py
    reviewboard/scmtools/svn/pysvn.py
    reviewboard/scmtools/svn/__init__.py
    reviewboard/scmtools/svn/base.py
  Ignored Files:

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    reviewboard/scmtools/svn/subvertpy.py
    reviewboard/scmtools/svn/pysvn.py
    reviewboard/scmtools/svn/__init__.py
    reviewboard/scmtools/svn/base.py
  Ignored Files:

reviewboard/scmtools/svn/subvertpy.py (Diff revision 3)

The 'path' parameter doesn't seem to be used by get_change(). What is it for?

chipx86 June 12, 2014, 2:53 p.m.

It's not used. I tacked it on for "completeness," and because I was testing against it. Diffing two revisions gives you the diffs across all files, and the diff commands needed a path, so I figured it didn't hurt to keep a parameter to override that path, allowing a diff for an individual file.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.0.x (0edad0e)