Summary

Add --no-renames flag for git diffs from rbt diff.

Review Request #9221 — Created Sept. 24, 2017 and submitted Nov. 23, 2017, 12:13 p.m.

Information

Owner

bleblan2

Repository

RBTools

Branch

release-0.7.x

Bugs

4551

Depends On

Commit

276ea82...

Reviewers

Groups

rbtools, students

People

chipx86, david

Description

When creating a git diff with rbt diff, the -M option is automatically
used if available and the user has no way of making rbt pass in
--no-renames to git diff.

This change adds a --no-renames flag to the rbt diff command which then
gets passed into git diff.

Testing Done

Tests were run.

Issues

Description	From	Last Updated
E111 indentation is not a multiple of four	reviewbot	Sept. 24, 2017, 9:13 a.m.
Blank line between these.	brennie	Sept. 25, 2017, 4:04 a.m.
Again, I don't like using extra_args this way. We should definitely be using keyword args for this.	brennie	Sept. 25, 2017, 4:46 a.m.
no_renames=False over extra_args=[]	brennie	Sept. 25, 2017, 4:46 a.m.
Can you split this up for one condition per line? It makes it easier to read.	brennie	Sept. 27, 2017, 5:25 a.m.
Update this to the next point release for RBTools: 0.7.11	brennie	Sept. 25, 2017, 4:46 a.m.
Blank line between these.	brennie	Sept. 27, 2017, 5:27 a.m.
Instead of checking the tool type, we should add a flag to SCMTool like supports_no_renames (defaulting to False) and set …	brennie	Sept. 25, 2017, 4:47 a.m.
I'm not sure that we should be using extra_args for this. This seems like a job for a keyword argument.	brennie	Sept. 25, 2017, 4:47 a.m.
These can be combined into a single conditional.	david	Nov. 3, 2017, 4:57 p.m.
I suspect we want to make this a fatal error (i.e. raise CommandError instead of just log something).	david	Nov. 3, 2017, 5 p.m.

flake8 failed.

JSHint passed.

flake8

rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E111 indentation is not a multiple of four
```

Change Summary:

Fix flake8 error with indent.

Commit:

b11dedb915f485914cfae2854fd9c3c028f0a052

d36c0824c706d22de5f8cb741f959c1158988198

Diff:

Revision 2 (+25 -4)

Show changes

	rbtools/clients/git.py
	rbtools/commands/__init__.py
	rbtools/commands/diff.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/clients/git.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line between these.
```

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

The issue has been resolved. Show all issues

Again, I don't like using extra_args this way. We should definitely be using keyword args for this.

rbtools/clients/git.py (Diff revision 2)
The issue has been resolved. Show all issues
```
no_renames=False over extra_args=[]
```
rbtools/commands/__init__.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Update this to the next point release for RBTools: 0.7.11
```

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

The issue has been resolved. Show all issues

Instead of checking the tool type, we should add a flag to SCMTool like supports_no_renames (defaulting to False) and set supports_no_renames = True in clients/git.py. Then we can check:

if self.options.no_renames:
    if tool.supports_no_renames:
        # ...
    else:
        logging.error('The %s SCM tool does not support diffs without renames',
                      tool.type)

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

The issue has been resolved. Show all issues

I'm not sure that we should be using extra_args for this. This seems like a job for a keyword argument.

Change Summary:

Update to use keyword arg for no_renames and address review comments.

Commit:

d36c0824c706d22de5f8cb741f959c1158988198

7f540d384586e9c63a4f7b0b373ff07d67a7ef76

Diff:

Revision 3 (+33 -14)

Show changes

	rbtools/clients/__init__.py
	rbtools/clients/bazaar.py
	rbtools/clients/clearcase.py
	rbtools/clients/cvs.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/clients/perforce.py
	rbtools/clients/plastic.py
	4 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Teeny tiny nitpicks :) You should probably get either David and/or Christian's feedback about this since we're adding new arguments to diff() and that could break third party SCM clients.

rbtools/clients/git.py (Diff revisions 2 - 3)

The issue has been resolved. Show all issues

Can you split this up for one condition per line? It makes it easier to read.

rbtools/commands/diff.py (Diff revisions 2 - 3)
The issue has been resolved. Show all issues
```
Blank line between these.
```

Change Summary:

Update code style.

Commit:

7f540d384586e9c63a4f7b0b373ff07d67a7ef76

6aa90e72a8aaea957553e3c8492fa8ca2103358c

People:

chipx86

david

Diff:

Revision 4 (+35 -14)

Show changes

	rbtools/clients/__init__.py
	rbtools/clients/bazaar.py
	rbtools/clients/clearcase.py
	rbtools/clients/cvs.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/clients/perforce.py
	rbtools/clients/plastic.py
	4 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

I'm happy with this as-is. Will wait for feedback from Chirstian and/or David before landing, however.

rbtools/commands/diff.py (Diff revision 4)
The issue has been resolved. Show all issues
```
These can be combined into a single conditional.
```

rbtools/commands/diff.py (Diff revision 4)

The issue has been resolved. Show all issues

I suspect we want to make this a fatal error (i.e. raise CommandError instead of just log something).

Change Summary:

Clean up structure around tool not supporting --no-renames flag based on feedback.

Commit:

6aa90e72a8aaea957553e3c8492fa8ca2103358c

276ea8299017aeeb582dd32a4d6e6b7e9200c257

Diff:

Revision 5 (+32 -14)

Show changes

	rbtools/clients/__init__.py
	rbtools/clients/bazaar.py
	rbtools/clients/clearcase.py
	rbtools/clients/cvs.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/clients/perforce.py
	rbtools/clients/plastic.py
	4 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Status:: Completed
Change Summary:: Pushed to master (8a66d1c)