Summary

Improve robustness of --svn-show-copies-as-adds option and respect included files

Review Request #6960 — Created Feb. 18, 2015 and submitted Feb. 25, 2015, 1:15 p.m.

Information

Owner

gmyers

Repository

RBTools

Branch

master

Bugs

3649

Depends On

Commit

bd7b63e...

Reviewers

Groups

rbtools

People

Description

This fixes Issue #3649 where previously history_scheduled_with_commit() did not respect explicitly included files when determining if an SVN changeset contained an addition-with-history.  history_scheduled_with_commit() now adds included files to the call to svn status similar to how it is done for an SVN changelist.
In the process of fixing the issue I came across several other minor deficiencies related to the --svn-show-copies-as-adds option and I have also cleaned those up:

The SVN client version is checked before doing anything related to --svn-show-copies-as-adds, as the underlying SVN argument (--show-copies-as-adds) did not come into existence until version 1.7.0.
The existence of --svn-show-copies-as-adds is checked before calling history_scheduled_with_commit().  If the user has already specified this option then there is no point in doing additional work.
In history_scheduled_with_commit() add '-q' argument to svn status call to suppress unversioned items.  For working copies with unversioned items, the addition of this argument will result in fewer status lines to process.
svn status reports details about files contained in changelists at the end of its output, after detailing all non-changelist files.  In the case where we are not working with a changelist, return early when we get to the changelist "section" of the status output so as not to possibly incorrectly detect history from a changelist.
In history_scheduled_with_commit() respect exclude_patterns so as not to possibly incorrectly detect history from an excluded file.

To support checking the SVN client version, is_valid_version() is relocated from rbtools/clients/git.py to rbtools/utils/checks.py.  Moving to this common location allows the function to be used by both the git and SVN clients without duplicating code.  The unit test related to this function (test_is_valid_version()) is also relocated to rbtools/utils/tests.py.

Testing Done

I've added unit tests which should exercise all aspects of these changes.

Issues

Description	From	Last Updated
Col: 41 E231 missing whitespace after ','	reviewbot	Feb. 18, 2015, 6:03 p.m.
Col: 43 E231 missing whitespace after ','	reviewbot	Feb. 18, 2015, 6:03 p.m.
Col: 14 E713 test for membership should be 'not in'	reviewbot	Feb. 18, 2015, 6:03 p.m.
local variable 'result' is assigned to but never used	reviewbot	Feb. 18, 2015, 6:03 p.m.
This should indeed go into a common place. Maybe rbtools/utils/checks.py?	david	Feb. 20, 2015, 7:42 p.m.
Please remove the blank line between these two (nose and six are both third-party packages).	david	Feb. 20, 2015, 7:42 p.m.
I don't think we have anywhere else with newlines in the help text. How about: help='Treat copied or moved files …	david	Feb. 24, 2015, 1:38 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Feb. 20, 2015, 7:46 p.m.
Variable names should use underscores, not camelCase.	david	Feb. 24, 2015, 5:47 p.m.

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/clients/svn.py
    rbtools/commands/__init__.py



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

rbtools/clients/svn.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 41
 E231 missing whitespace after ','
```
rbtools/clients/svn.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 43
 E231 missing whitespace after ','
```
rbtools/clients/svn.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 14
 E713 test for membership should be 'not in'
```
rbtools/clients/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 local variable 'result' is assigned to but never used
```

Commit:

69d4e1e55391eb2ab0806a81729144d6414ea132

001f8c0adea70e81541c829ca39a8aef7e2b3251

Diff:

Revision 2 (+208 -12)

Show changes

	rbtools/clients/svn.py
	rbtools/clients/tests.py
	rbtools/commands/__init__.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/clients/svn.py
    rbtools/commands/__init__.py



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

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

The issue has been resolved. Show all issues

This should indeed go into a common place. Maybe rbtools/utils/checks.py?

rbtools/clients/tests.py (Diff revision 2)

The issue has been resolved. Show all issues

Please remove the blank line between these two (nose and six are both third-party packages).

rbtools/commands/__init__.py (Diff revision 2)

The issue has been dropped. Show all issues

I don't think we have anywhere else with newlines in the help text. How about:

help='Treat copied or moved files as new files '
     '(Subversion 1.7+ only).',

gmyers Feb. 19, 2015, 5:53 p.m.

I do see a few other instances where there are newlines in the help text.  In particular I modeled this after the text for --include and --exclude where a newline appears between the basic help description and a list of supported SCM clients.  That said, I'm happy to switch to your suggested text if that would be preferred.

david Feb. 24, 2015, 1:37 p.m.

I think I would prefer that (I'll go through and clean up the existing ones).

david Feb. 24, 2015, 1:38 p.m.

Actually, nevermind, it's more prevalent than I had thought. As-is is fine.

Description:

		This fixes Issue #3649 where previously `history_scheduled_with_commit()` did not respect explicitly included files when determining if an SVN changeset contained an addition-with-history. `history_scheduled_with_commit()` now adds included files to the call to `svn status` similar to how it is done for an SVN changelist.

		In the process of fixing the issue I came across several other minor deficiencies related to the --svn-show-copies-as-adds option and I have also cleaned those up:

		The SVN client version is checked before doing anything related to --svn-show-copies-as-adds, as the underlying SVN argument (--show-copies-as-adds) did not come into existence until version 1.7.0.
		The existence of --svn-show-copies-as-adds is checked before calling `history_scheduled_with_commit()`. If the user has already specified this option then there is no point in doing additional work.
		In `history_scheduled_with_commit()` add '-q' argument to `svn status` call to suppress unversioned items. For working copies with unversioned items, the addition of this argument will result in fewer status lines to process.
		`svn status` reports details about files contained in changelists at the end of its output, after detailing all non-changelist files. In the case where we are not working with a changelist, return early when we get to the changelist "section" of the status output so as not to possibly incorrectly detect history from a changelist.
		In `history_scheduled_with_commit()` respect exclude_patterns so as not to possibly incorrectly detect history from an excluded file.

~
	~	To support checking the SVN client version, `is_valid_version()` is relocated from rbtools/clients/git.py to rbtools/utils/checks.py. Moving to this common location allows the function to be used by both the git and SVN clients without duplicating code. The unit test related to this function (test_is_valid_version()) is also relocated to rbtools/utils/tests.py.
-
-		One question: I borrowed `is_valid_version()` from clients/git.py. Is it preferable to move this function to a common location to avoid duplicated code? If so, what location do you recommend?

Commit:

001f8c0adea70e81541c829ca39a8aef7e2b3251

d10cab0ca363e5041e7a1ac54b307a3f54a6a864

Diff:

Revision 3 (+224 -43)

Show changes

	rbtools/clients/git.py
	rbtools/clients/svn.py
	rbtools/clients/tests.py
	rbtools/commands/__init__.py
	rbtools/utils/checks.py
	rbtools/utils/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/utils/tests.py
    rbtools/clients/tests.py
    rbtools/commands/__init__.py
    rbtools/clients/git.py
    rbtools/utils/checks.py
    rbtools/clients/svn.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/utils/tests.py
    rbtools/clients/tests.py
    rbtools/commands/__init__.py
    rbtools/clients/git.py
    rbtools/utils/checks.py
    rbtools/clients/svn.py

rbtools/utils/checks.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```

Commit:

d10cab0ca363e5041e7a1ac54b307a3f54a6a864

9d8e836937d1c09ec510a2b1949b1d29221c4a94

Diff:

Revision 4 (+225 -43)

Show changes

	rbtools/clients/git.py
	rbtools/clients/svn.py
	rbtools/clients/tests.py
	rbtools/commands/__init__.py
	rbtools/utils/checks.py
	rbtools/utils/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/utils/tests.py
    rbtools/clients/tests.py
    rbtools/commands/__init__.py
    rbtools/clients/git.py
    rbtools/utils/checks.py
    rbtools/clients/svn.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/utils/tests.py
    rbtools/clients/tests.py
    rbtools/commands/__init__.py
    rbtools/clients/git.py
    rbtools/utils/checks.py
    rbtools/clients/svn.py

Change Summary:

In check_update() need to grab svn_show_copies_as_adds option via getattr() to properly handle commands, such as 'rbt status', which do not recognize --svn-show-copies as adds as a command line option.

Commit:

9d8e836937d1c09ec510a2b1949b1d29221c4a94

52dc85c91eaa511311e1807fded30b07d35b6157

Diff:

Revision 5 (+225 -43)

Show changes

	rbtools/clients/git.py
	rbtools/clients/svn.py
	rbtools/clients/tests.py
	rbtools/commands/__init__.py
	rbtools/utils/checks.py
	rbtools/utils/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/utils/tests.py
    rbtools/clients/tests.py
    rbtools/commands/__init__.py
    rbtools/clients/git.py
    rbtools/utils/checks.py
    rbtools/clients/svn.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/utils/tests.py
    rbtools/clients/tests.py
    rbtools/commands/__init__.py
    rbtools/clients/git.py
    rbtools/utils/checks.py
    rbtools/clients/svn.py

rbtools/clients/svn.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Variable names should use underscores, not camelCase.
```

Commit:

52dc85c91eaa511311e1807fded30b07d35b6157

bd7b63e371459952f65c45798428caf5718d91e6

Diff:

Revision 6 (+225 -43)

Show changes

	rbtools/clients/git.py
	rbtools/clients/svn.py
	rbtools/clients/tests.py
	rbtools/commands/__init__.py
	rbtools/utils/checks.py
	rbtools/utils/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/utils/tests.py
    rbtools/clients/tests.py
    rbtools/commands/__init__.py
    rbtools/clients/git.py
    rbtools/utils/checks.py
    rbtools/clients/svn.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/utils/tests.py
    rbtools/clients/tests.py
    rbtools/commands/__init__.py
    rbtools/clients/git.py
    rbtools/utils/checks.py
    rbtools/clients/svn.py

Ship it!

```
Ship It!
```

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