Support extra revision string for svn diff

Review Request #3749 — Created Jan. 16, 2013 and submitted

t2y
Review Board
reviewboard
Parsing revision number in Subversion is only supported in English environment.

$ svn diff  # LANG=C
Index: dir1/test1.txt
===================================================================
--- dir1/test1.txt	(revision 3)
+++ dir1/test1.txt	(working copy)

For example, these diff header is localized in Japanese environment as follows.

$ svn diff  # LANG=ja_JP.UTF-8
Index: dir1/test1.txt
===================================
--- dir1/test1.txt	(????? 3)
+++ dir1/test1.txt	(?????)

SVNTool.parse_diff_revision() function cannot parse such as localized revision number, because its regular expression doesn't match.
I added an additional "revision_str" item in Advanced Settings for Repository so that it can be passed for the regular expression.

Could you review it?
I welcome any comments from you!
$ ./reviewboard/manage.py test -- reviewboard.scmtools.tests:SubversionTests

Creating test database for alias 'default'...
Testing parsing SVN diff with binary file ... ok
Testing SVNTool.get_file ... ok
Testing basic SVNTool API ... ok
Testing parsing SVN diff with keywords ... ok
Testing revision number parsing ... ok
Testing a SSH-backed Subversion repository ... SKIP: Cannot perform SSH access tests. The local user's SSH public key must be in the /Users/t2y/.ssh/authorized_keys file and SSH must be enabled.
Testing a SSH-backed Subversion repository with a LocalSite ... SKIP: Cannot perform SSH access tests. The local user's SSH public key must be in the /Users/t2y/.ssh/authorized_keys file and SSH must be enabled.
Testing parsing SVN 1.6 diff with property changes ... ok
Testing parsing SVN 1.7+ diff with property changes ... ok
Testing parsing SVN diff with unterminated keywords ... ok

----------------------------------------------------------------------
Ran 10 tests in 3.082s

OK (SKIP=2)
Destroying test database for alias 'default'...
Loading file attachments...

Description From Last Updated

Col: 34 E231 missing whitespace after ','

reviewbotreviewbot

Col: 29 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 53 E231 missing whitespace after ','

reviewbotreviewbot

Col: 53 E231 missing whitespace after ','

reviewbotreviewbot

Col: 53 E231 missing whitespace after ','

reviewbotreviewbot
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/scmtools/tests.py
        reviewboard/scmtools/admin.py
        reviewboard/scmtools/models.py
        reviewboard/scmtools/svn.py
      Ignored Files:
    
    
  2. reviewboard/scmtools/admin.py (Diff revision 1)
     
     
    Col: 34
     E231 missing whitespace after ','
    
  3. reviewboard/scmtools/svn.py (Diff revision 1)
     
     
    Col: 29
     E127 continuation line over-indented for visual indent
    
  4. reviewboard/scmtools/tests.py (Diff revision 1)
     
     
    Col: 53
     E231 missing whitespace after ','
    
  5. reviewboard/scmtools/tests.py (Diff revision 1)
     
     
    Col: 53
     E231 missing whitespace after ','
    
  6. reviewboard/scmtools/tests.py (Diff revision 1)
     
     
    Col: 53
     E231 missing whitespace after ','
    
  7. 
      
T2
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/scmtools/tests.py
        reviewboard/scmtools/admin.py
        reviewboard/scmtools/models.py
        reviewboard/scmtools/svn.py
      Ignored Files:
    
    
  2. 
      
T2
david
  1. I don't think that putting this in the database is quite the right solution, because it's just a small incremental improvement which still forces everyone to use the same locale. Different users may be using different locales, especially in globally distributed teams.
    
    What I'd prefer to see is just improving our existing regular expression to cope with the different localized revision strings that svn might produce.
    
    Note that if you use post-review to create your diffs, it will already handle this problem by ensuring that svn is always run in the C locale.
    1. Thank you for your review!
      
      > Different users may be using different locales, especially in globally distributed teams.
      Yes. So, the "revision_str" can include utf-8 encoded multi strings split by ",". For example, like this "?????,wersja".
      
      > Note that if you use post-review to create your diffs
      I have misunderstood the post-review command feature. That's nice what you said.
      
      > What I'd prefer to see is just improving our existing regular expression to cope with the different localized revision strings that svn might produce.
      I can investigate the different localized strings in svn, and add them into the regular expression with hard coding, instead of repository settings. However, I'm not sure other developer want these improvements. Additionally, the post-review command works enough. What do you think?
    2. Thank you for your review!
      
      > Different users may be using different locales, especially in globally distributed teams.
      Yes. So, the "revision_str" can include utf-8 encoded multi strings split by ",". For example, like this "?????,wersja".
      
      > Note that if you use post-review to create your diffs
      I have misunderstood the post-review command feature. That's nice what you said.
      
      > What I'd prefer to see is just improving our existing regular expression to cope with the different localized revision strings that svn might produce.
      I can investigate the different localized strings in svn, and add them into the regular expression with hard coding, instead of repository settings. However, I'm not sure other developer want these improvements. Additionally, the post-review command works enough. What do you think?
    3. Given that multiple people have hit this case, I think supporting the different localized patches that svn diff produces would be a good thing. We try to recommend that people use post-review but of course not everyone wants to (or can) for various reasons.
    4. I see. I will try to investigate the different localized revision strings and remake that patch. After that, I wish some developers re-review my patch.
  2. 
      
T2
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/scmtools/tests.py
        reviewboard/scmtools/svn.py
      Ignored Files:
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
T2
Review request changed

Status: Closed (submitted)

Change Summary:

I made a few changes (mostly with regards to how things are commented) and pushed to release-1.7.x (c6500fd). Thanks!
Loading...