rbtools support diff filename instead of calling scm to generate diff

Review Request #1197 — Created Nov. 5, 2009 and submitted

Information

RBTools

Reviewers

Added new flag "--diff-filename" that specifies the name of the file where diffs should be read from instead of using SCM client to generate diff.

Due to the way some (git) SCMClient subclasses change the current directory the new code has to take a backup of the starting directory if diff filename is not fully specified/qualified.

NOTE at this time only reading from a file is supported.
git diff --full-index HEAD >mydiff
./postreview.py --server=http://reviews.reviewboard.org/ --diff-filename mydiff -r 1197

I.e. this review was posted with the new code
DJ
  1. We could potentially change the GitClient to not chdir. I'm not sure what the comment about not doing it from the top level directory is referring to anyway. With pure git there's no reason we can't diff from a subdirectory. Perhaps there is an implication for git-svn though.
    1. I'm a complete newbie with git so I can't comment either way on potential changes to GitClient. If Git Client does change I think it should be done in a separate change, this change is only for specifying a diff filename.
      
      Is there any chance this can be accepted as-is (without "-" support for the filename)?
    2. I think that's fine. I still have to review the code (been on vacation, will get to it soon), but changing it to not chdir and adding "-" support can be done separately.
  2. 
      
chipx86
  1. 
      
  2. rbtools/postreview.py (Diff revision 1)
     
     
     
    I'd prefer a simpler comment: "upload an existing diff file, instead of generating a new diff"
  3. rbtools/postreview.py (Diff revision 1)
     
     
     
    Combine these.
    
    Blank line after these and before the if statement.
  4. rbtools/postreview.py (Diff revision 1)
     
     
     
     
     
     
     
    This can be simplified a little, and also needs to close the file handle. Should look more like:
    
    
    if options.diff_filename == '-':
        raise NotImplemented(...)
    
    parent_diff = None
    
    try:
        fp = open(os.path.join(origcwd, options.diff_filename), 'r')
        diff = fp.read()
        fp.close()
    except IOError, e:
        die("Unable to open diff filename: %s" % e)
  5. 
      
CL
LO
  1. 
      
  2. rbtools/postreview.py (Diff revision 2)
     
     
     
    You could combine these 2 lines into
    elif options.diff_filename:
    to be consistent with the previous elif.
    
    But then, the else: on line 2663 needs to be properly indented.
    
    1. Agreed merging line 2650 and 2651 into an elif and then de-indenting what is in the proposed diff is line 2663 (and the line below) would be cleaner.
      
      I'm probably not going to post another review for this though as it looks like interest for this may have dropped off. I've updated my local copy though! Thanks for the feedback.
      
      Chris
      
    2. I think it's still a useful change to get in, especially for automation.
  3. 
      
CL
CL
Review request changed
Change Summary:
Essentially the same code as the last posted diff, but this is after a "git pull/merge" to latest code as of 2009-01-12. I.e. fully merged code with headrevs.
LO
  1. Looks great.  I implemented locally a similar change, so it'll be useful if yours goes into the tree.  
    Many thanks.
  2. 
      
chipx86
  1. Thanks! Committed as r368f1f4.
    
    Also made a commit with support for --diff-filename=-, which prevents usage of raw_input and stuff.
  2.