Summary

More robust parsing of SVN client version

Review Request #7182 — Created April 7, 2015 and submitted April 9, 2015, 7:43 p.m.

Information

Owner

gmyers

Repository

RBTools

Branch

master

Bugs

3834

Depends On

Commit

dbd0aac...

Reviewers

Groups

rbtools

People

Description

A user of SlikSvn reported an error when parsing the svn client
version string.  For SlikSvn the version string is of the form:
  1.6.1-SlikSvn-tag-1.6.1
which is more complicated than the version string produced by
other svn clients where the format is:
  1.6.1

String parsing is updated to extract exactly the first three 
numbers in the version string.  If less than three numbers are
present, which is an unlikely occurrence based on historical 
SVN version numbers (https://subversion.apache.org/docs/release-notes/release-history.html)
then a warning is logged and a default version number of
0.0.0 is assumed.

Testing Done

1) Tested with simple version string produced by stock subversion client in CentOS 7.x.
2) Stepped through code and manually tested with more complicated SlikSvn version string.
3) Ran unit tests.

Issues

Description	From	Last Updated
This should have a leading underscore.	brennie	April 9, 2015, 6:12 p.m.
This code is getting a bit messy. Can we perhaps add a constant on the class that specifically captures the …	brennie	April 8, 2015, 7:04 p.m.
Blank line between these. (I know there's stuff violating this right above, but that can be fixed later.)	brennie	April 9, 2015, 5:42 p.m.
Use double quotes here for the inner string.	brennie	April 9, 2015, 5:42 p.m.
Col: 48 E231 missing whitespace after ','	reviewbot	April 8, 2015, 7:12 p.m.
Col: 50 E231 missing whitespace after ','	reviewbot	April 8, 2015, 7:13 p.m.

Tool: Pyflakes
Processed Files:
    rbtools/clients/svn.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/svn.py

rbtools/clients/svn.py (Diff revision 1)

The issue has been resolved. Show all issues

This code is getting a bit messy.
Can we perhaps add a constant on the class that specifically captures the first three numerical groups and then do something like self.subversion_client_version = tuple(map(int, m.groups)) where  m is the result of of doing the regular expression match?

gmyers April 7, 2015, 7:56 p.m.

Thanks for the feedback, Barret.

I'm not opposed to something like this, but I'm not sure I'm regex savvy enough to pull it off in a robust fashion :) I think I could use (\d+)\.(\d+)\.(\d+) to capture the first three decimal components, but it requires three components so something like 1.2 (rather than 1.2.3) doesn't match. I don't know if a two component version is even possible from an SVN client, but obviously they don't all follow the simple three component flavor like I previously thought, so I was trying to be defensive against this in my solution. My approach will work for one to three components. Do you think it is even worth worrying about the number of components in the version string?

brennie April 7, 2015, 8:40 p.m.

I was going to suggest (\d+)\.(\d+)\.(\d+) myself, I didn't consider the case where there are less than 3 version parts. I just assume that SVN will always have three. In that case, we can do (\d+)(?:\.(\d+))*,  which will capture all leading dot-separated numbers.

brennie April 7, 2015, 8:41 p.m.

Also make sure to use re.match and not re.find if you go with that idea. I'm not sure which is a more appropriate way to parse it out.

gmyers April 8, 2015, 6:39 p.m.

I couldn't get this to work. It seemed to always only capture the final dot-separated number, for example:

>>> re.match(r'(\d+)(?:\.(\d+))*', '1.6.10.5').groups()
('1', '5')

I tinkered with it for a while, but couldn't improve things. I checked the SVN version history (https://subversion.apache.org/docs/release-notes/release-history.html) and ever since 1.0.0 was released in 2004, the version numbers have been triples. I guess it is not possible to know what each individual client implementation would report (a la SlikSvn) but hopefully they would respect and report the full version triple.

I have a second approach which I'm going to post shortly which requires a version triple for parsing and logs a warning otherwise and sets the version to 0.0.0. See what you think about it, or perhaps you see something I was doing incorrectly above.

brennie April 8, 2015, 8:44 p.m.

So as it turns out, Python's re library doesn't support getting multiple values out of the same capture group so it can't be done in the generic way with just regex.

Description:

		A user of SlikSvn reported an error when parsing the svn client
		version string. For SlikSvn the version string is of the form:
		1.6.1-SlikSvn-tag-1.6.1
		which is more complicated than the version string produced by
		other svn clients where the format is:
		1.6.1

~		String parsing is updated to extract the first three decimal
~		components of the version string.
	~	String parsing is updated to extract exactly the first three
	~	numbers in the version string. If less than three numbers are
	+	present, which is an unlikely occurrence based on historical
	+	SVN version numbers (https://subversion.apache.org/docs/release-notes/release-history.html)
	+	then a warning is logged and a default version number of
	+	0.0.0 is assumed.

Commit:

29fe8218f11de2b44f65709b57e7296fe32431a2

ac34cc4792d1db99c6763eaa70adf197a54e76e5

Diff:

Revision 2 (+12 -2)

Show changes

rbtools/clients/svn.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/svn.py



Tool: Pyflakes
Processed Files:
    rbtools/clients/svn.py

rbtools/clients/svn.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 48
 E231 missing whitespace after ','
```
rbtools/clients/svn.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 50
 E231 missing whitespace after ','
```

Change Summary:

Fix Review Bot issues.

Commit:

ac34cc4792d1db99c6763eaa70adf197a54e76e5

ab73882d1fddda656c1a34e55bb810e4bf08662d

Diff:

Revision 3 (+12 -2)

Show changes

rbtools/clients/svn.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/svn.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/svn.py

rbtools/clients/svn.py (Diff revisions 1 - 3)

The issue has been dropped. Show all issues

This should have a leading underscore.

gmyers April 9, 2015, 5:42 p.m.

I can change this if you think it is important, but none of the other dozen or so class variables have a leading underscore so I'd suggest leaving if off for consistency.

rbtools/clients/svn.py (Diff revisions 1 - 3)

The issue has been resolved. Show all issues

Blank line between these. (I know there's stuff violating this right above, but that can be fixed later.)

rbtools/clients/svn.py (Diff revisions 1 - 3)
The issue has been resolved. Show all issues
```
Use double quotes here for the inner string.
```

Change Summary:

Address issues from Barret's review.

Commit:

ab73882d1fddda656c1a34e55bb810e4bf08662d

dbd0aacff2ebf20b75cd64f2efb5e21f49c25567

Diff:

Revision 4 (+13 -2)

Show changes

rbtools/clients/svn.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/svn.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/svn.py

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-0.7.x (4db748d)

Since this has been submitted, could someone please close issue 3834.  Thanks!