Summary

Add support for -X to git SCMClient

Review Request #6339 — Created Sept. 19, 2014 and submitted Sept. 20, 2014, 7:24 a.m.

Information

Owner

brennie

Repository

RBTools

Branch

master

Bugs

Depends On

Blocks

6345

Commit

191bc37...

Reviewers

Groups

rbtools, students

People

Description

Git (and the git-svn and git-p4 backends) now support the -X/--exclude
commandline option and the EXCLUDE_PATTERNS .reviewboardrc option.

Add unit tests for excluding files from Git diffs.

Rename exclude_files/EXCLUDE_FILES to exclude_patterns/EXCLUDE_PATTERNS
to reflect the actual behaviour of -X.

Testing Done

Unit tests pass.

Generated diffs with excluded files via rbt diff with git, git-svn,
and git-p4.

Issues

Description	From	Last Updated
Col: 59 E231 missing whitespace after ','	reviewbot	Sept. 19, 2014, 9:04 a.m.
Col: 39 E127 continuation line over-indented for visual indent	reviewbot	Sept. 19, 2014, 9:05 a.m.
Col: 39 E127 continuation line over-indented for visual indent	reviewbot	Sept. 19, 2014, 9:05 a.m.
Col: 39 E127 continuation line over-indented for visual indent	reviewbot	Sept. 19, 2014, 9:05 a.m.
Col: 39 E127 continuation line over-indented for visual indent	reviewbot	Sept. 19, 2014, 9:05 a.m.
Can you rename to filename? file is reserved in Python.	chipx86	Sept. 19, 2014, 9:42 a.m.
I assume this is an error condition? If so, we should probably log something.	chipx86	Sept. 19, 2014, 10:38 a.m.
No blank line here.	chipx86	Sept. 19, 2014, 9:41 a.m.
Any chance of supporting both include and exclude at the same time? (Did the Mercurial change support this?)	chipx86	Sept. 19, 2014, 10:38 a.m.
local variable 'include_Files' is assigned to but never used	reviewbot	Sept. 19, 2014, 11:15 a.m.
This should be updated to say "pattern" instead of "file".	david	Sept. 19, 2014, 1:33 p.m.
This should probably have a final 'else' that just asserts.	david	Sept. 19, 2014, 1:55 p.m.
Can we log this message at a higher level than 'debug'?	david	Sept. 19, 2014, 1:55 p.m.
Please replace the `' with "" and put this whole string in single-quotes instead of double.	david	Sept. 19, 2014, 1:55 p.m.
This line should be indented one more space (to match up with the '(').	david	Sept. 19, 2014, 1:57 p.m.
This should be indented one more space.	david	Sept. 19, 2014, 1:57 p.m.

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/clients/git.py

Ignored Files:
    docs/rbtools/rbt/commands/post.rst
    docs/rbtools/rbt/commands/diff.rst



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

Ignored Files:
    docs/rbtools/rbt/commands/post.rst
    docs/rbtools/rbt/commands/diff.rst

rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 59
 E231 missing whitespace after ','
```
rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 39
 E127 continuation line over-indented for visual indent
```
rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 39
 E127 continuation line over-indented for visual indent
```
rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 39
 E127 continuation line over-indented for visual indent
```
rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 39
 E127 continuation line over-indented for visual indent
```

Change Summary:

Add students group

Commit:

-	4d9dde7b4c0085cab622ef54aa4dc342f048037b
+	47fe9cdf2de3f7a914098c3135d7c4e4aa70d878

Groups:

+	students

Diff:

Revision 2 (+78 -16)

Show changes

	docs/rbtools/rbt/commands/diff.rst
	docs/rbtools/rbt/commands/post.rst
	rbtools/clients/git.py
	rbtools/clients/tests.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/clients/git.py

Ignored Files:
    docs/rbtools/rbt/commands/post.rst
    docs/rbtools/rbt/commands/diff.rst



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

Ignored Files:
    docs/rbtools/rbt/commands/post.rst
    docs/rbtools/rbt/commands/diff.rst

rbtools/clients/git.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can you rename to filename? file is reserved in Python.
```

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

The issue has been resolved. Show all issues

I assume this is an error condition? If so, we should probably log something.

rbtools/clients/git.py (Diff revision 2)
The issue has been resolved. Show all issues
```
No blank line here.
```

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

The issue has been resolved. Show all issues

Any chance of supporting both include and exclude at the same time? (Did the Mercurial change support this?)

brennie Sept. 19, 2014, 9:40 a.m.

By this do you mean removing files in exclude_files from include_files?

The mercurial change just adds -X f1 -X f2 to the commandline because it natively supports it.

chipx86 Sept. 19, 2014, 9:47 a.m.

If you have excluded files set up, then no included files passed on the command line will be used, since we never reach this block.

What does Mercurial do if you specify both specific files and use -X?

brennie Sept. 19, 2014, 10:37 a.m.

As it turns out, hg diff -r r1 -r r2 -X f -- f returns nothing. I /think/ this is a good behaviour and will replicate this in the git tool.

Change Summary:

Support -I and -X at the same time in git.

Rename exclude_files -> exclude_patterns.

Description:

		Git (and the git-svn and git-p4 backends) now support the -X/--exclude
~		commandline option and the EXCLUDE_FILES `.reviewboardrc` option.
	~	commandline option and the EXCLUDE_PATTERNS `.reviewboardrc` option.

		Add unit tests for excluding files from Git diffs.
	+
	+	Rename exclude_files/EXCLUDE_FILES to exclude_patterns/EXCLUDE_PATTERNS
	+	to reflect the actual behaviour of -X.

Commit:

-	47fe9cdf2de3f7a914098c3135d7c4e4aa70d878
+	dca41bcdc7a191a10b445d6d8b0e07c3fb3224ad

Diff:

Revision 3 (+127 -50)

Show changes

	docs/rbtools/rbt/commands/diff.rst
	docs/rbtools/rbt/commands/post.rst
	rbtools/clients/__init__.py
	rbtools/clients/bazaar.py
	rbtools/clients/clearcase.py
	rbtools/clients/cvs.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	7 more

Tool: Pyflakes
Processed Files:
    rbtools/clients/cvs.py
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/commands/__init__.py
    rbtools/commands/diff.py
    rbtools/clients/clearcase.py
    rbtools/clients/plastic.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/perforce.py
    rbtools/clients/bazaar.py
    rbtools/clients/svn.py

Ignored Files:
    docs/rbtools/rbt/commands/post.rst
    docs/rbtools/rbt/commands/diff.rst

rbtools/clients/git.py (Diff revision 3)
The issue has been resolved. Show all issues
```
 local variable 'include_Files' is assigned to but never used
```

Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/cvs.py
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/commands/__init__.py
    rbtools/commands/diff.py
    rbtools/clients/clearcase.py
    rbtools/clients/plastic.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/perforce.py
    rbtools/clients/bazaar.py
    rbtools/clients/svn.py

Ignored Files:
    docs/rbtools/rbt/commands/post.rst
    docs/rbtools/rbt/commands/diff.rst

Change Summary:

Fix typo

Commit:

-	dca41bcdc7a191a10b445d6d8b0e07c3fb3224ad
+	f23dd8c5603d38ac7ed624aa1d6e27d00d1c1d46

Diff:

Revision 4 (+127 -50)

Show changes

	docs/rbtools/rbt/commands/diff.rst
	docs/rbtools/rbt/commands/post.rst
	rbtools/clients/__init__.py
	rbtools/clients/bazaar.py
	rbtools/clients/clearcase.py
	rbtools/clients/cvs.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	7 more

Tool: Pyflakes
Processed Files:
    rbtools/clients/cvs.py
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/commands/__init__.py
    rbtools/commands/diff.py
    rbtools/clients/clearcase.py
    rbtools/clients/plastic.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/perforce.py
    rbtools/clients/bazaar.py
    rbtools/clients/svn.py

Ignored Files:
    docs/rbtools/rbt/commands/post.rst
    docs/rbtools/rbt/commands/diff.rst



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/cvs.py
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/commands/__init__.py
    rbtools/commands/diff.py
    rbtools/clients/clearcase.py
    rbtools/clients/plastic.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/perforce.py
    rbtools/clients/bazaar.py
    rbtools/clients/svn.py

Ignored Files:
    docs/rbtools/rbt/commands/post.rst
    docs/rbtools/rbt/commands/diff.rst

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

Can you tell me which assert statements actually test your changes? I'm having trouble understainding the test case

brennie Sept. 19, 2014, 1:14 p.m.

The test compares the result of running the git diff after including two files and excluding one of them to what the result would be of just including one file.

Ship it!

```
Ship It!
```

docs/rbtools/rbt/commands/post.rst (Diff revision 4)
The issue has been resolved. Show all issues
```
This should be updated to say "pattern" instead of "file".
```

rbtools/clients/git.py (Diff revision 4)

The issue has been resolved. Show all issues

This should probably have a final 'else' that just asserts.

rbtools/clients/git.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Can we log this message at a higher level than 'debug'?
```

rbtools/clients/git.py (Diff revision 4)

The issue has been resolved. Show all issues

Please replace the `' with "" and put this whole string in single-quotes instead of double.

rbtools/commands/diff.py (Diff revision 4)
The issue has been resolved. Show all issues
```
This line should be indented one more space (to match up with the '(').
```
rbtools/commands/post.py (Diff revision 4)
The issue has been resolved. Show all issues
```
This should be indented one more space.
```

Change Summary:

Refactor generation of parameters in the git SCMClient's make_diff method.
The parameters are only generated once and saved so that they may be used
in both git diff and git diff-tree (in case -X is supplied).

Add an assert for an unreachable code location.

Log at error level and use appropriate quotes.

Fix documentation.

Fix indentation.

Commit:

-	f23dd8c5603d38ac7ed624aa1d6e27d00d1c1d46
+	199d446063ebccbf17008d70e30ea7637311950d

Diff:

Revision 5 (+136 -60)

Show changes

	docs/rbtools/rbt/commands/diff.rst
	docs/rbtools/rbt/commands/post.rst
	rbtools/clients/__init__.py
	rbtools/clients/bazaar.py
	rbtools/clients/clearcase.py
	rbtools/clients/cvs.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	7 more

Tool: Pyflakes
Processed Files:
    rbtools/clients/cvs.py
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/commands/__init__.py
    rbtools/commands/diff.py
    rbtools/clients/clearcase.py
    rbtools/clients/plastic.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/perforce.py
    rbtools/clients/bazaar.py
    rbtools/clients/svn.py

Ignored Files:
    docs/rbtools/rbt/commands/post.rst
    docs/rbtools/rbt/commands/diff.rst

Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/cvs.py
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/commands/__init__.py
    rbtools/commands/diff.py
    rbtools/clients/clearcase.py
    rbtools/clients/plastic.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/perforce.py
    rbtools/clients/bazaar.py
    rbtools/clients/svn.py

Ignored Files:
    docs/rbtools/rbt/commands/post.rst
    docs/rbtools/rbt/commands/diff.rst

rbtools/clients/git.py (Diff revision 4)

You can use fnmatch.filter function to get a subset of the filenames that match each pattern. It will simplify the code a bit but not necessary.

brennie Sept. 19, 2014, 2:11 p.m.
```
Implemented in latest diff.
```

Change Summary:

Use fnmatch.filter

Commit:

-	199d446063ebccbf17008d70e30ea7637311950d
+	191bc37ed54229b0b44c98db10b4f6cc85f85e83

Diff:

Revision 6 (+133 -59)

Show changes

	docs/rbtools/rbt/commands/diff.rst
	docs/rbtools/rbt/commands/post.rst
	rbtools/clients/__init__.py
	rbtools/clients/bazaar.py
	rbtools/clients/clearcase.py
	rbtools/clients/cvs.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	7 more

Tool: Pyflakes
Processed Files:
    rbtools/clients/cvs.py
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/commands/__init__.py
    rbtools/commands/diff.py
    rbtools/clients/clearcase.py
    rbtools/clients/plastic.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/perforce.py
    rbtools/clients/bazaar.py
    rbtools/clients/svn.py

Ignored Files:
    docs/rbtools/rbt/commands/post.rst
    docs/rbtools/rbt/commands/diff.rst



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/cvs.py
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/commands/__init__.py
    rbtools/commands/diff.py
    rbtools/clients/clearcase.py
    rbtools/clients/plastic.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/perforce.py
    rbtools/clients/bazaar.py
    rbtools/clients/svn.py

Ignored Files:
    docs/rbtools/rbt/commands/post.rst
    docs/rbtools/rbt/commands/diff.rst

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-0.6.x (f7a6d7d)