Centralize get_change() in the SVN backends.

Review Request #5982 — Created June 11, 2014 and submitted

Information

Review Board
release-2.0.x
193369a...

Reviewers

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.

Unit tests pass for both backends.

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 …

daviddavid

I'm not sure if the diff command can raise exceptions, but the .decode could. We should probably wrap this in …

daviddavid

Same comment about "encoded" vs "unicode object"

daviddavid

Should we check err here?

daviddavid

undefined name '_'

reviewbotreviewbot
reviewbot
  1. 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:
    
    
  2. 
      
reviewbot
  1. 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:
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/scmtools/svn/pysvn.py (Diff revision 1)
     
     
    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.

  3. reviewboard/scmtools/svn/pysvn.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
    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.

  4. reviewboard/scmtools/svn/subvertpy.py (Diff revision 1)
     
     
    Show all issues

    Same comment about "encoded" vs "unicode object"

  5. reviewboard/scmtools/svn/subvertpy.py (Diff revision 1)
     
     
    Show all issues

    Should we check err here?

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

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

  6. 
      
chipx86
reviewbot
  1. 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:
    
    
  2. 
      
reviewbot
  1. 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:
    
    
  2. reviewboard/scmtools/svn/subvertpy.py (Diff revision 2)
     
     
    Show all issues
     undefined name '_'
    
  3. 
      
chipx86
reviewbot
  1. 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:
    
    
  2. 
      
reviewbot
  1. 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:
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/scmtools/svn/subvertpy.py (Diff revision 3)
     
     

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

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

  3. 
      
david
  1. Ship It!

  2. 
      
chipx86
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.0.x (0edad0e)