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

Review Request #9221 — Created Sept. 24, 2017 and submitted

Information

RBTools
release-0.7.x
276ea82...

Reviewers

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.

Tests were run.

Description From Last Updated

E111 indentation is not a multiple of four

reviewbotreviewbot

Blank line between these.

brenniebrennie

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

brenniebrennie

no_renames=False over extra_args=[]

brenniebrennie

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

brenniebrennie

Update this to the next point release for RBTools: 0.7.11

brenniebrennie

Blank line between these.

brenniebrennie

Instead of checking the tool type, we should add a flag to SCMTool like supports_no_renames (defaulting to False) and set …

brenniebrennie

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

brenniebrennie

These can be combined into a single conditional.

daviddavid

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

daviddavid
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

bleblan2
brennie
  1. 
      
  2. rbtools/clients/git.py (Diff revision 2)
     
     
     
    Show all issues

    Blank line between these.

  3. rbtools/clients/git.py (Diff revision 2)
     
     
     
    Show all issues

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

  4. rbtools/clients/git.py (Diff revision 2)
     
     
    Show all issues

    no_renames=False over extra_args=[]

  5. rbtools/commands/__init__.py (Diff revision 2)
     
     
    Show all issues

    Update this to the next point release for RBTools: 0.7.11

  6. rbtools/commands/diff.py (Diff revision 2)
     
     
     
     
     
     
    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)
    
  7. rbtools/commands/diff.py (Diff revision 2)
     
     
    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.

  8. 
      
bleblan2
brennie
  1. 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.

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

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

  3. rbtools/commands/diff.py (Diff revisions 2 - 3)
     
     
     
    Show all issues

    Blank line between these.

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

  2. 
      
david
  1. 
      
  2. rbtools/commands/diff.py (Diff revision 4)
     
     
     
    Show all issues

    These can be combined into a single conditional.

  3. rbtools/commands/diff.py (Diff revision 4)
     
     
     
    Show all issues

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

  4. 
      
bleblan2
bleblan2
Review request changed
Status:
Completed
Change Summary:
Pushed to master (8a66d1c)