Summary

Support extra revision string for svn diff

Review Request #3749 — Created Jan. 16, 2013 and submitted Jan. 24, 2013, 9:38 p.m.

Information

Owner

t2y

Repository

Review Board

Branch

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

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!

Testing Done

$ ./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'...

Files

Issues

Description	From	Last Updated
Col: 34 E231 missing whitespace after ','	reviewbot	Jan. 16, 2013, 11:31 p.m.
Col: 29 E127 continuation line over-indented for visual indent	reviewbot	Jan. 16, 2013, 11:31 p.m.
Col: 53 E231 missing whitespace after ','	reviewbot	Jan. 16, 2013, 11:31 p.m.
Col: 53 E231 missing whitespace after ','	reviewbot	Jan. 16, 2013, 11:31 p.m.
Col: 53 E231 missing whitespace after ','	reviewbot	Jan. 16, 2013, 11:31 p.m.

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:

reviewboard/scmtools/admin.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 34
 E231 missing whitespace after ','
```
reviewboard/scmtools/svn.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 29
 E127 continuation line over-indented for visual indent
```
reviewboard/scmtools/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 53
 E231 missing whitespace after ','
```
reviewboard/scmtools/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 53
 E231 missing whitespace after ','
```
reviewboard/scmtools/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 53
 E231 missing whitespace after ','
```

Change Summary:

fixed pep8 errors.

Diff:

Revision 2 (+42 -4)

Show changes

	reviewboard/scmtools/admin.py
	reviewboard/scmtools/models.py
	reviewboard/scmtools/svn.py
	reviewboard/scmtools/tests.py

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:

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.

t2y Jan. 23, 2013, 5:11 p.m.

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?

t2y Jan. 23, 2013, 5:15 p.m.

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?

david Jan. 23, 2013, 8:41 p.m.

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.

t2y Jan. 23, 2013, 8:54 p.m.

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.

Change Summary:

I investigated the localized revision string in subversion-trunk. The result is grep_revision-workingcopy_from_pofile.log file.

I improved my patch to include these different revision strings for the regular expression. Could you re-review it!

Testing Done:

~		I made the "test_extra_revision_str_parsing" for my changes.
	~	$ ./reviewboard/manage.py test -- reviewboard.scmtools.tests:SubversionTests

~		$ ./reviewboard/manage.py test -- reviewboard.scmtools.tests:SubversionTests
	~	Creating test database for alias 'default'...
-		Creating test database for alias 'default'...
		Testing parsing SVN diff with binary file ... ok
-		Testing revision number parsing with extra revision_str ... 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 11 tests in 1.304s
	~	Ran 10 tests in 3.082s

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

Diff:

Revision 3 (+23 -2)

Show changes

	reviewboard/scmtools/svn.py
	reviewboard/scmtools/tests.py

Added Files:

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

Ship it!

```
Ship It!
```

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