Summary

Support SVN 1.9 (nonexistent) diffs with relocation info.

Review Request #8146 — Created May 6, 2016 and submitted June 2, 2016, 2:29 p.m.

Information

Owner

hopfeld

Repository

Review Board

Branch

Bugs

4374, 4392

Depends On

Reviewers

Groups

reviewboard

People

Description

Subversion 1.9 introduced the '(nonexistent)' revision as part of the revision string for newly created files in patches, where earlier versions create '(revision 0)' instead.

Revision string handling must cover some variants and shall extract relocation information -if any- (when creating svn diffs between branches), handle a date string (created by svnlook diff) and extract the revision number or the '(nonexistent)' expression.

The current support in reviewboad for '(nonexistent)' revision specifier does not handle diffs created between branches and does not handle diffs created by svnlook with additional date information.

This patch merges the code paths for subversion 1.9 ('nonexistent') revision specifiers and the already existing revision parser regex.

Patches created with earlier subversion clients are properly handled, as well.

A new unit test is included for the '(nonexistent)' revision string as seen in diffs between branches.

Testing Done

Uploading a SVN 1.9 diff with relocation information is successful.
Ran unittests.

Issues

Description	From	Last Updated
Retain comments to indicate the languages.	gmyers	May 12, 2016, 6:18 a.m.
What happened to group 3? Can we use (?: like we did before so we don't lose a group number?	chipx86	May 12, 2016, 6:15 a.m.
Why the None? We just didn't have a group matching? Shouldn't there have been something there at least? Should also …	chipx86	May 12, 2016, 6:16 a.m.
We should have a new unit test for this, rather than reusing this one. This is really about the "nonexistent" …	chipx86	May 12, 2016, 6:16 a.m.
Col: 1 W191 indentation contains tabs	reviewbot	May 12, 2016, 1:17 p.m.
Col: 1 E101 indentation contains mixed spaces and tabs	reviewbot	May 12, 2016, 1:18 p.m.
Col: 1 E101 indentation contains mixed spaces and tabs	reviewbot	May 12, 2016, 1:17 p.m.
Col: 1 E101 indentation contains mixed spaces and tabs	reviewbot	May 12, 2016, 1:17 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	May 12, 2016, 1:17 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	May 12, 2016, 1:17 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	May 12, 2016, 1:17 p.m.
Col: 1 E101 indentation contains mixed spaces and tabs	reviewbot	May 12, 2016, 1:17 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	May 12, 2016, 1:17 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	May 12, 2016, 1:17 p.m.
Col: 1 W191 indentation contains tabs	reviewbot	May 12, 2016, 1:17 p.m.
Col: 1 E101 indentation contains mixed spaces and tabs	reviewbot	May 12, 2016, 1:17 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	May 12, 2016, 1:17 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	May 12, 2016, 1:17 p.m.
Col: 80 E501 line too long (99 > 79 characters)	reviewbot	May 12, 2016, 1:17 p.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	May 12, 2016, 1:17 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	May 12, 2016, 11:40 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	May 12, 2016, 11:40 p.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	May 12, 2016, 11:40 p.m.
information, not informationen.	gmyers	May 23, 2016, 2:40 a.m.

Tool: Pyflakes
Processed Files:
    reviewboard/scmtools/tests.py
    reviewboard/scmtools/svn/__init__.py

My regex-fu is not that strong, but this seems reasonable to me.  Thanks for the new unit test.

I believe this also resolves #4374 (https://hellosplat.com/s/beanbag/tickets/4374/)

hopfeld May 9, 2016, 1 a.m.

Indeet, #4374 has the same root cause.
The regex-fu is not that dramatic.

just imagine the revision string to be composed of up to three parts
"(.../trunk/foo.c)  SOME_DATE_HERE   (revision indicator)"

^^^^^^^^^^^^^^^^^^  _~_~_~_~~~   ^^^^^^^^^^^^^^^^^^^^

    (1)                 (2)                  (3)

1. is optional and matches the relative location to the SVN root url.

2. SOME_DATE_HERE is self describing, but optional

3. the revision indicator placed between braces () is mandatory. 

   - This can be a (localizable) string 'revision' followed by a number or 

   - a (localizable) string equivalent for 'nonexistent'.

The regex matches the the relative location (if any) and matches the revision number or the 'nonexistent'. Reviewboard needs to know if this is a new file (PRE_CREATION), which is true  whenever the revision number '0' is detected or the 'nonexistent'-part of the regex matches.
This is what the patch does.

I'm going to fix the comments soon.

reviewboard/scmtools/svn/__init__.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Retain comments to indicate the languages.
```

Does this retain all support for all prior versions?

The description in the review request (which we'll use for the commit message) should ideally contain more information on what was broken, how it's been fixed, and note that all prior versions are supported (assuming they are). We have a write-up on what we like to see at https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/.

Some good information to include would be the older format for these types of revision indicators in diffs, and the newer format, for comparison purposes.

The info you have on the regex in your other reply would be good to have in the source code, so it's more clear when reviewing and down the road when further changes are made. (Really, we should have been documenting all the formats this whole time..)

hopfeld May 9, 2016, 1:51 a.m.

I'll adjust the source and the review description including your suggestions. :-)

It's a good point to update format specifications of what the svn diffs may look like and what is handled by reviewboard. So, I'm looking into this...

HO

hopfeld May 12, 2016, 6:38 a.m.
```
Review description updated.
```

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

The issue has been dropped. Show all issues

What happened to group 3? Can we use (?: like we did before so we don't lose a group number?

hopfeld May 9, 2016, 1:51 a.m.

I'm looking into this. Maybe there is another regex trick waiting out there for this particular problem ...

hopfeld May 12, 2016, 6:38 a.m.

I would prefer to use the updated regex, and have it properly commented. So we will have group(4).

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

The issue has been resolved. Show all issues

Why the None? We just didn't have a group matching? Shouldn't there have been something there at least?

Should also use a tuple instead of a list here, as it's lighter-weight.

hopfeld May 9, 2016, 1:51 a.m.

None it is as group(4) only matches the revision number. In case of the '(nonexistent)' string, this part of the regex does not match, so the result will be 'None'. But group(3) will hold the '(nonextistent)' string. group(3) will contain the localized string, so the only useful check would be to check whether group(3) is not None.
After all this seemed logical to me, as there is literally no revision for a file in the state 'PRE_CREATION'.

chipx86 May 9, 2016, 2:25 p.m.

Ah, I see. Okay, this would be a great comment to have in there. Basically, breaking down the groups of the regex and the expectations (since that regex is pretty massive).

Thanks for the contribution, btw, and being so responsive and open to the feedback! :)

reviewboard/scmtools/tests.py (Diff revision 1)

The issue has been resolved. Show all issues

We should have a new unit test for this, rather than reusing this one. This is really about the "nonexistent" string alone, but a new test would be about that + branch names. It could also include the localized versions, to verify.

Can you think of any other tests that would help prove this change? Other cases we're not already covering?

HO

hopfeld May 9, 2016, 1:51 a.m.
```
I will create a new unittest for this.
```

Change Summary:


added proper comments to the complex regex
created new unittest case for (nonexistent) and relocation information.

Description:

~		Merge the code paths for subversion 1.9 "nonexistent" revision markers and the already existing revision parser.
	~	Subversion 1.9 introduced the '(nonexistent)' revision as part of the revision string for newly created files in patches, where earlier versions create '(revision 0)' instead.
-		This will handle the already known cases where subversion prepends relocation information, inserts timestamps.
-		Additionally handles the 'nonexistent' revision specifier.

~		Extend Unit test to include a testcase in the with relocation information and (nonexistent) revision string.
	~	Revision string handling must cover some variants and shall extract relocation information -if any- (when creating svn diffs between branches), handle a date string (created by svnlook diff) and extract the revision number or the '(nonexistent)' expression.
	+
	+	The current support in reviewboad for '(nonexistent)' revision specifier does not handle diffs created between branches and does not handle diffs created by svnlook with additional date information.
	+
	+	This patch merges the code paths for subversion 1.9 ('nonexistent') revision specifiers and the already existing revision parser regex.
	+
	+
	+
	+	Patches created with earlier subversion clients are properly handled, as well.
	+
	+	A new unit test is included for the '(nonexistent)' revision string as seen in diffs between branches.

Bugs:

4374

Diff:

Revision 2 (+44 -16)

Show changes

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

Tool: Pyflakes
Processed Files:
    reviewboard/scmtools/tests.py
    reviewboard/scmtools/svn/__init__.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/scmtools/tests.py
    reviewboard/scmtools/svn/__init__.py

reviewboard/scmtools/svn/__init__.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
reviewboard/scmtools/svn/__init__.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 E101 indentation contains mixed spaces and tabs
```
reviewboard/scmtools/svn/__init__.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 E101 indentation contains mixed spaces and tabs
```
reviewboard/scmtools/svn/__init__.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 E101 indentation contains mixed spaces and tabs
```
reviewboard/scmtools/svn/__init__.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
reviewboard/scmtools/svn/__init__.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
reviewboard/scmtools/svn/__init__.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
reviewboard/scmtools/svn/__init__.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 E101 indentation contains mixed spaces and tabs
```
reviewboard/scmtools/svn/__init__.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
reviewboard/scmtools/svn/__init__.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
reviewboard/scmtools/svn/__init__.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
reviewboard/scmtools/svn/__init__.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 1
 E101 indentation contains mixed spaces and tabs
```
reviewboard/scmtools/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
reviewboard/scmtools/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```
reviewboard/scmtools/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (99 > 79 characters)
```
reviewboard/scmtools/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (83 > 79 characters)
```

Change Summary:


follow PEP8 style guide

Diff:

Revision 3 (+48 -16)

Show changes

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

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/scmtools/tests.py
    reviewboard/scmtools/svn/__init__.py

reviewboard/scmtools/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 13
 E128 continuation line under-indented for visual indent
```
reviewboard/scmtools/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 13
 E128 continuation line under-indented for visual indent
```
reviewboard/scmtools/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 13
 E128 continuation line under-indented for visual indent
```

Change Summary:


PEP8 Style fixes (again, sorry)

Diff:

Revision 4 (+47 -17)

Show changes

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

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/scmtools/tests.py
    reviewboard/scmtools/svn/__init__.py

reviewboard/scmtools/tests.py (Diff revision 4)

The issue has been resolved. Show all issues

information, not informationen.

HO

hopfeld May 23, 2016, 2:41 a.m.
```
"Informationen" is German ...
```

chipx86 May 25, 2016, 7:45 p.m.

It doesn't appear this test is specific to German, though. Docstrings should be in English, and if they are about a word in another language (or about any string at all), the string in question should be in quotes.

Change Summary:


Fixed typo in test description
Updated to apply against current master

Diff:

Revision 5 (+47 -17)

Show changes

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

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/scmtools/tests.py
    reviewboard/scmtools/svn/__init__.py

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.0.x (33081fb)