post-review: yet another attempt to improve git support

Review Request #1144 — Created Oct. 4, 2009 and submitted

Information

djs
RBTools

Reviewers

This is refresh of review 985. I've refactored the change to be more intelligent, and I didn't touch git-svn behavior.

The general idea is to try to detect the merge base of your current head, or gracefully fall back to defaults. You can optionally specify a tracking branch if you need to.

* Try to detect the tracking branch for the current HEAD, if possible
* If a tracking branch doesn't exist, or is not remote, fall back to origin/master
* If --tracking-branch is specified, use this branch instead of origin/master
* Determine the merge base with the git merge-base command. This makes it okay to specify a tracking branch even when its head no longer points to the original branching point.
* No longer mucks with the current git-svn behavior.
* Add unit tests for pure git. The tests are limited to pure git GitClient right now and aren't comprehensive, but cover the scenarios I changed.

http://github.com/djs/rbtools/tree/review1144-r5
Tested with rbtools repo and local server with following scenarios:

* Remote tracking branch available
* Local tracking branch available (ignored)
* No tracking branch available (default to origin/master)
* No tracking branch available, with --tracking specified
* No tracking branch available with --parent specified
* Revision range specified

$ nosetests
..........
----------------------------------------------------------------------
Ran 10 tests in 12.420s
chipx86
  1. 
      
  2. rbtools/postreview.py (Diff revision 1)
     
     
     
    Blank line between these.
  3. rbtools/postreview.py (Diff revision 1)
     
     
    Make sure this wraps to 80 characters.
    
    Also, why not just rstrip() for all these?
  4. rbtools/postreview.py (Diff revision 1)
     
     
     
    Both rows of values inside the [...] should be aligned.
  5. rbtools/postreview.py (Diff revision 1)
     
     
    'is' shouldn't be used for string comparisons.
    
    This would be better as: if not self.upstream_branch:
  6. rbtools/postreview.py (Diff revision 1)
     
     
     
    Parameters should be aligned.
  7. rbtools/postreview.py (Diff revision 1)
     
     
     
    Blank line between these.
  8. rbtools/postreview.py (Diff revision 1)
     
     
     
     
    We seem to do this logic twice. I'd prefer an inline utility function like:
    
    
    def get_origin(default_upstream_branch=None, ignore_errors=False):
        self.upstream_branch = default_upstream_branch or options.tracking or 'origin/master'
        upstream_remote = ...
        origin = execute(..., ignore_errors=ignore_errors)
    
    
    Then you can just do:
    
    origin = get_origin(self.upstream_branch, True)
    
    if origin.startswith("fatal:"):
        origin = get_origin()
    
    
    Or something like that.
  9. rbtools/postreview.py (Diff revision 1)
     
     
     
    Alignment and rstrip.
  10. rbtools/postreview.py (Diff revision 1)
     
     
    Just do:
    
    if parent_branch:
  11. rbtools/postreview.py (Diff revision 1)
     
     
     
    Those two parameters can probably fit on the same line in under 80 chars.
  12. rbtools/postreview.py (Diff revision 1)
     
     
    Since we do the "%s..%s" % (ancestor, commit) twice, it'd be nice to just do this before the self.type checks and save it as revision_range or something.
  13. rbtools/postreview.py (Diff revision 1)
     
     
    I'd prefer --tracking-branch.
    
    Also, can you update the rbtools docs (reviewboard/docs/manual/users/tools/post-review.txt) with this parameter?
    1. I planned to do this, but there's no easy way to add it to the review since it's in a different repository
  14. 
      
DJ
chipx86
  1. I love that you're introducing unit tests for rbtools. We've needed this badly for a while :)
    
    Some small fixes (coding style) and a handful of docstring stuff.
  2. rbtools/postreview.py (Diff revision 2)
     
     
     
    No need for parens around returned tuples. Should do:
    
    self.upstream_branch, origin = \
        self.get_origin(....)
  3. rbtools/postreview.py (Diff revision 2)
     
     
    Same here with the parens.
  4. rbtools/postreview.py (Diff revision 2)
     
     
    Doc blocks in Python should describe what the function does first and foremost. This should then describe what it does with the parameters, and then what's returned. 
  5. rbtools/postreview.py (Diff revision 2)
     
     
     
    Blank line between these.
  6. rbtools/tests.py (Diff revision 2)
     
     
     
     
     
     
     
    Should be alphabetized.
  7. rbtools/tests.py (Diff revision 2)
     
     
     
    Should be one import with \ for multiple lines.
  8. rbtools/tests.py (Diff revision 2)
     
     
     
     
    Two blank lines between classes.
  9. rbtools/tests.py (Diff revision 2)
     
     
     
     
    Same here.
  10. rbtools/tests.py (Diff revision 2)
     
     
     
    Should have something, but if not, just remove the docstring.
  11. rbtools/tests.py (Diff revision 2)
     
     
     
    Make sure this wraps to 80 chars.
  12. rbtools/tests.py (Diff revision 2)
     
     
     
    Blank line between these.
  13. rbtools/tests.py (Diff revision 2)
     
     
     
    The alignment is off. Make sure all params align.
  14. rbtools/tests.py (Diff revision 2)
     
     
    Maybe call this self.client, so we can reuse that name in all tests?
  15. rbtools/tests.py (Diff revision 2)
     
     
     
     
    Really should have a docstring for the unit tests that says "Testing <some description here>"
    
    Same with all other test_* functions.
  16. rbtools/tests.py (Diff revision 2)
     
     
     
     
     
     
     
    We repeat this so much. Might be nice to just have a utility function that takes the filename, contents, and a commit message, and just does all this.
  17. rbtools/tests.py (Diff revision 2)
     
     
     
    Blank line between these. We're testing another thing, so it should be its own block.
  18. 
      
DJ
chipx86
  1. You likely couldn't post due to the domain name change. I bet it doesn't handle redirects correctly. Try modifying the .reviewboardrc on the server to point to reviews.reviewboard.org. I'll push updates later to fix this.
    1. Looks like that works now
  2. 
      
chipx86
  1. Almost done, a few more things.
  2. rbtools/postreview.py (Diff revision 3)
     
     
    Out of curiosity, do you know what the minimum version of Git is that supports this? I mostly only care if it's recent, as we'll need to doc/work around it.
    1. Well, for-each-ref has been there forever. But you're right, it looks like upstream formatter is recent (v1.6.2.3):
      http://git.kernel.org/?p=git/git.git;a=commit;h=8cae19d987b1bbd43258558f591e39d9d216dcb3
      
      I guess we'll have to use another mechanism. Some ideas here, where I got the idea to use this: http://blog.woobling.org/2009/09/git-hate.html
  3. rbtools/postreview.py (Diff revision 3)
     
     
     
    Indentation issues.
  4. rbtools/postreview.py (Diff revision 3)
     
     
    Missing trailing period.
  5. rbtools/postreview.py (Diff revision 3)
     
     
     
    ignore_errors, should be on the same indentation level as [
  6. rbtools/postreview.py (Diff revision 3)
     
     
    Still would prefer we not repeat ourselves with this. Can just store it in a variable before the if statements.
    1. What? Are you talking about the "%s..%s" % (ancestor, commit) line fragment? That's nuts.
    2. I know it's a trivial amount of code, but it's an expression that we were creating twice (code-wise, not in execution). Creating it only once just helps clean up the code a little more, makes it easier to debug or change if we needed to.
  7. rbtools/tests.py (Diff revision 3)
     
     
     
    The second line should be on the same indentation level as the """, not as the contents.
  8. rbtools/tests.py (Diff revision 3)
     
     
    We reference this server many times in the file. Maybe store it as a class-devel variable?
  9. 
      
DJ
DJ
Review request changed
Change Summary:
Add a fugly hack to avoid using git for-each-ref. Unit tests pass
Description:
   

This is refresh of review 985. I've refactored the change to be more intelligent, and I didn't touch git-svn behavior.

   
   

The general idea is to try to detect the merge base of your current head, or gracefully fall back to defaults. You can optionally specify a tracking branch if you need to.

   
   
  • Try to detect the tracking branch for the current HEAD, if possible
   
  • If a tracking branch doesn't exist, or is not remote, fall back to origin/master
   
  • If --tracking-branch is specified, use this branch instead of origin/master
   
  • Determine the merge base with the git merge-base command. This makes it okay to specify a tracking branch even when its head no longer points to the original branching point.
   
  • No longer mucks with the current git-svn behavior.
   
  • Add unit tests for pure git. The tests are limited to pure git GitClient right now and aren't comprehensive, but cover the scenarios I changed.
   
~  

http://github.com/djs/rbtools/tree/review-1144r4

  ~

http://github.com/djs/rbtools/tree/review1144-r5

chipx86
  1. Thanks Dan. It's committed to r5985a31.
  2. 
      
XA
  1. 
      
  2. rbtools/postreview.py (Diff revision 5)
     
     
    lstrip isn't good here, for example it will change string
    refs/heads/dev
    to
    v
    insead of "dev"
    From doc of lstrip
    "The chars argument is a string specifying the set of characters to be removed"
    >>> 'www.example.com'.lstrip('cmowz.')
    'example.com'
    replace('refs/heads/','',1)
    or something similar would work better
    1. ouch. i knew that, don't know why I used that. I'll update it when I have a chance, or you can submit a patch. Would be good to add a new unit test :)
    2. well i didn't wrote single code of python in my life so maybe better not ;] i fixed it locally by using 
         merge = merge.replace('refs/heads/','',1)
      but i dunno if its 100% correct from git POV (i mean obscure cases like someone tracking  local branch named "refs/heads/testing" or smthing)
      
    3. That's what the unit tests are for :) If they pass, you didn't break it. As for the git correctness, I'm quite sure it's not perfect. This is all an ugly hack so we can support old versions of git.
    4. hmm maybe it's worth making it like "if git_version_is_very old {ask_for_upgrade;ugly hack} else {do it right}" ?
      
    5. I think we need to wait longer. This basic feature didn't go into git until 1.6.2.3. Debian stable is still at git 1.5.6. Though I have a feeling post-review won't work with that. It would be an interesting exercise to determine what minimum git version post-review requires for all its functionality.
    6. I used it (a bit, only simple post-review --parent=something) with debian-stable git and worked fine
  3. 
      
VI
  1. fasdfasfasd
  2.