RB Patch

Review Request #3440 - Created Oct. 21, 2012 and submitted

John Sintal
RBTools
master
rbtools
RB Patch will download a patch based off a specific review request ID and apply the patch on the local machine.

TODO:
- I keep getting an error when post-review'ing my mercurial changes (after a hg commit). http://pastie.org/5455551 which prevents me from fully testing Mercurial, though it should work.
- All testing performed on dev server for controlled experiments.

- Tested when user uses a specific revision id (--diff-revision)
- Tested when user specifices own pX argument (--px)

** GIT **
Steps performed:
1. Made a new review request (with commit A)
2. Made some changes to local files
3. Git commit those changes (commit B) and updated the request to create a diff (between B and A)
4. On local drive, reverted to commit A
5. Ran 'rb patch <r>' and viewed the results and affected files.

I did this for a positive and negative test.

** Mercurial **
Similar to Git. I am assuming there's no need for pX logic here, just like Git's apply patch.

** SVN **
Steps performed:
1. Made a new review request (with commit A)
2. Made some changes to local file(s)*
3. post-review a request to create a diff (between B and A)
4. On local drive, reverted to commit A
5. Ran 'rb patch <r>' and viewed the results and affected files.

One or more file changes:
- one file change (/trunk/file1.txt)
- multiple file changes (/trunk/file1.txt and /trunk/file2.txt) and files in different subdirectories (e.g. /trunk/file1.txt, /trunk/a/file2.txt, /trunk/a/b/file3.txt)

Sample directory cases #1:
server: /trunk/a/b/c/file.txt
local checked out: /../b (I just checked out from /trunk/a/b)
patch applied from /b/, /b/c/ <-- all succeeded


Sample directory cases #2:
server: /trunk/a/b/c/file.txt
local checked out: /trunk (I just checked out from /trunk)
patch applied from trunk, /trunk/a/, trunk/a/b/, trunk/a/b/c/ <-- all succeeded

-- 

Tested again with new changes made in rb-general branch (removal of extra options).
  • 0
  • 0
  • 44
  • 1
  • 45
Description From Last Updated
Steven MacLeod
  1. Looking over the plan you've laid out in your comments, no issues come to mind.
    
    This diff is including your changes for 'rb diff', which I assume means you've based your branch for 'rb patch' off of your other branch. To fix this, you should run post-review with '--parent=<name_of_rb_diff_branch>' when you post your next diff.
    1. Do I still include the --tracking-branch=api ?
  2. 
      
John Sintal
John Sintal
John Sintal
Steven MacLeod
  1. It appears that the 'rb diff' changes were shown in the latest diff. Make sure to use '--parent=rbdiff' when updating the request so the changes are hidden.
  2. rbtools/commands/rbpatch.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Imports should be order like this:
    
    Standard Library imports (e.g. os, sys, etc.)
    ===BLANK LINE===
    Third Party imports
    ===BLANK LINE===
    Local Imports (e.g. 'rbtools...')
    
    Also, imports should be order alphabetically within the three categories.
  3. rbtools/commands/rbpatch.py (Diff revision 4)
     
     
    This is imported twice.
  4. rbtools/commands/rbpatch.py (Diff revision 4)
     
     
    This should be removed when not debugging.
  5. rbtools/commands/rbpatch.py (Diff revision 4)
     
     
     
     
    docstrings should contain a one line summary, followed by a blank line, and then further description.
  6. rbtools/commands/rbpatch.py (Diff revision 4)
     
     
     
     
     
     
     
    You'll want to get rid of these options. The user should be passing the review request id  and diff revision as arguments, not options.
    
    Right now the command would be called like this:
    rb patch -r <request_id> [--diff-revision=<diff_revision>]
    
    We want to call the command like this:
    rb patch <request_id> [<diff_revision>]
  7. rbtools/commands/rbpatch.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
    You shouldn't need these options for patch.
  8. rbtools/commands/rbpatch.py (Diff revision 4)
     
     
     
     
    This option isn't required. Although, maybe the command should give the option of creating a change list (A different, but similar option).
    1. Could you expound on 'change list' option? 
  9. rbtools/commands/rbpatch.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
    This option isn't needed.
    1. scan_usable_client -> get_repository_info complains about the lack of this option, so I'll include in the next revision for now.
  10. rbtools/commands/rbpatch.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Clear out the options which should be removed, as you remove them.
  11. rbtools/commands/rbpatch.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    This won't be needed for patch.
  12. rbtools/commands/rbpatch.py (Diff revision 4)
     
     
    This red block means there is trailing whitespace. You should view the diff before publishing and check for these.
  13. rbtools/commands/rbpatch.py (Diff revision 4)
     
     
    You never initialize self.tool, so this will fail.
  14. rbtools/commands/rbpatch.py (Diff revision 4)
     
     
     
     
    Yeah, I've been trying to decide how I want to handle this.
  15. rbtools/commands/rbpatch.py (Diff revision 4)
     
     
    ridorigcwd isn't an option. It looks like you've prepended 'self.options.rid' to the line.
    
    Also, this variable (or origcwd) are never referenced again.
  16. rbtools/commands/rbpatch.py (Diff revision 4)
     
     
    The comment explaining why this is set should probably be restored.
  17. rbtools/commands/rbpatch.py (Diff revision 4)
     
     
     
    These might be better off factored out, and have server_url passed into the method.
  18. rbtools/commands/rbpatch.py (Diff revision 4)
     
     
    You don't need this pass here.
  19. rbtools/commands/rbpatch.py (Diff revision 4)
     
     
     
     
    Same thing as my previous comment on docstrings.
  20. rbtools/commands/rbpatch.py (Diff revision 4)
     
     
     
    You should consolidate your calls to things like get_review_requests, and git_item. Each of these calls causes a HTTP request to the server, which is just returning the same data in this case.
  21. rbtools/commands/rbpatch.py (Diff revision 4)
     
     
     
    \ isn't needed to continue on the next line when you are inside an open parentheses. 
  22. rbtools/commands/rbpatch.py (Diff revision 4)
     
     
     
    This return statement is a little cluttered. Could you try and break this out and make it a little more readable.
  23. rbtools/commands/rbpatch.py (Diff revision 4)
     
     
    'with' isn't introduced until python 2.5, and we need to support 2.4, so you'll have to change this.
    
    Also, rbtools.utils.filesystem has functions for creating temporary files etc. You should use those instead of just writing in the working directory.
  24. rbtools/commands/rbpatch.py (Diff revision 4)
     
     
     
     
    You'll want to use rbtools.utils.process.execute here instead of executing the command yourself.
    
    for examples, take a look at the ./rbtools/clients/*.py files. 
  25. rbtools/commands/rbpatch.py (Diff revision 4)
     
     
     
    You'll need the tool since you'll be passing the patch to it. The actual code for applying your patch to the tree should live inside the clients.
    
    The base class will handle the default of calling out to 'patch', but sub classes can override this and use things such as 'git apply'
  26. rbtools/commands/rbpatch.py (Diff revision 4)
     
     
     
    The patch should be removed, but you'll want to use the temp files stuff I mentioned previously.
  27. rbtools/commands/rbpatch.py (Diff revision 4)
     
     
    Trailing whitespace.
  28. 
      
John Sintal
John Sintal
John Sintal
John Sintal
David Trowbridge
  1. 
      
  2. rbtools/clients/__init__.py (Diff revision 8)
     
     
     
     
     
    A more concise way to write this is:
    
    p_num = p or self._get_p_number(patch_file)
  3. rbtools/clients/__init__.py (Diff revision 8)
     
     
     
     
    Leftover debug output?
    1. I was thinking of letting the user see the command being executed, as well as the output of that command. Let me know if  that's okay.
  4. rbtools/clients/__init__.py (Diff revision 8)
     
     
    This should check the return value, no?
  5. rbtools/clients/git.py (Diff revision 8)
     
     
    If statements in python don't need parentheses. Also, a good way to check for falsiness is "if not p:"
  6. rbtools/clients/git.py (Diff revision 8)
     
     
    I think a much better way to do this would be to make execute() return the process return code, and check if it's non-zero.
    1. execute is returning 'data' which looks like just string data. How can I detect the return code? I'm currently looking at the method in process.py and I do not see the option.
    2. I've set ignore_errors=True in the execute() method, however, I still can't get the return int value. 
      
      Is that value actually 'rc' derived from p.wait() in the process.py:execute code? If that's the case, would I need to return rc as well? I don't want to have to modify the process.py:execute code.
  7. rbtools/commands/rbpatch.py (Diff revision 8)
     
     
     
     
     
     
     
     
    I don't think any of these options are necessary
    1. Indeed they are not needed, but I've had SCM related code complain about these options not being present. I will need to investigate further.
  8. rbtools/commands/rbpatch.py (Diff revision 8)
     
     
     
     
     
    Some trailing whitespace.
  9. rbtools/commands/rbpatch.py (Diff revision 8)
     
     
     
    I'm not sure if get_root should be in the base class (yet), but this should definitely be moved out into a helper function.
    1. I am guessing I should move:
      "os.umask(0077)
      cookie_file = os.path.join(get_home_path(), ".post-review-cookies.txt")"
      to function in the base command class?
  10. rbtools/commands/rbpatch.py (Diff revision 8)
     
     
    No parentheses needed, and can check using "not"
  11. rbtools/commands/rbpatch.py (Diff revision 8)
     
     
    Leftover debug stuff
  12. rbtools/commands/rbpatch.py (Diff revision 8)
     
     
    We should definitely delete it, especially in the success case.
  13. 
      
John Sintal
John Sintal
  1. 
      
  2. rbtools/clients/__init__.py (Diff revisions 8 - 9)
     
     
    Do I have all the ingredients/args to derive the pX number here?
    1. I think so - though I can't say I'm familiar with too many SCMs outside of SVN, Git and Mercurial.
  3. rbtools/clients/git.py (Diff revisions 8 - 9)
     
     
    Will fix in next submit
  4. rbtools/commands/rbpatch.py (Diff revisions 8 - 9)
     
     
    will fix excess space
  5. rbtools/commands/rbpatch.py (Diff revisions 8 - 9)
     
     
    Should I make this a helper function?!
    1. Naw - at least, IMO, I don't think another level of indirection buys us anything here.
  6. rbtools/commands/__init__.py (Diff revision 9)
     
     
    I made a helper in base class to get cookie, is this okay?
    1. Sounds like the right call for now. As David points out, we might end up tossing get_root in there as well down the line.
  7. 
      
John Sintal
Mike Conley
  1. When do you think you'll be ready to take this out of WIP so that we can do a deep review?
    1. Tomorrow by 5pm for sure, hopefully sooner. 
  2. 
      
John Sintal
John Sintal
John Sintal
John Sintal
John Sintal
John Sintal
Sampson Chen
  1. 
      
  2. rbtools/clients/__init__.py (Diff revision 16)
     
     
     
    Alphabetical:
    
    import re
    import sys
    
    
     (I had think about it for a sec to make sure, haha)
  3. rbtools/clients/__init__.py (Diff revision 16)
     
     
     
    Should this and other occurrences of this start on the 1st line:
    
    """Returns...
    
    ?
    1. It seems like the entire file was written like that. So, I'll stay consistent for now. Hope that's okay.
  4. 
      
John Sintal
John Sintal
John Sintal
Mike Conley
  1. This is looking really good - only two nits this time. One more pass, and I think this has my ship-it.
  2. rbtools/clients/__init__.py (Diff revision 19)
     
     
     
    r before s.
  3. rbtools/commands/rbpatch.py (Diff revision 19)
     
     
     
    This might look better:
    
    diffs = self.root_resource \
                .get_review_requests() \
                .get_item(request_id) \
                .get_diffs()
  4. 
      
John Sintal
Mike Conley
  1. I think I'm good with this. Thanks John!
  2. 
      
John Sintal
Steven MacLeod
  1. Ship It!
  2. 
      
John Sintal
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to api (0c55b87e540689f7c6e188e8796456b5ae169901).
Loading...