Adding git-p4 support

Review Request #3122 — Created May 30, 2012 and submitted

Information

RBTools
master

Reviewers

This was taken from a previous patch request (http://reviews.review-board.org/r/889/) and modified for the current 0.5 release. Git-p4 only supports pushing to perforce from the master branch but the rbtools modifications allow sending reviews from and git branch.

original github pull request: https://github.com/reviewboard/rbtools/pull/16
The patch has been thoroughly tested within our organisation from version 0.2 right through to 0.4.1 and with the latest 0.5 development branch. We have roughly 20 users using git-p4 on a daily basis.
Description From Last Updated

Can we make use of the --tracking-branch parameter here and other places where master is hard-coded?

chipx86chipx86

Should wrap to 80 chars.

chipx86chipx86

80 chars.

chipx86chipx86

Remove this line.

chipx86chipx86

Trailing whitespace.

chipx86chipx86

Trailing whitespace.

chipx86chipx86

Trailing whitespace.

chipx86chipx86

Same.

chipx86chipx86

Should be on one line.

chipx86chipx86

Blank line between these.

chipx86chipx86

Blank line between these.

chipx86chipx86

You could do: elif line.startswith("index ") or line.startswith("new file mode "):

chipx86chipx86

Wrap to 80 chars.

chipx86chipx86

Space on both sides of the =

chipx86chipx86

Blank line between these. Also, space on both sides of the =

chipx86chipx86

Indentation is wrong.

chipx86chipx86

Col: 72 E203 whitespace before ','

reviewbotreviewbot

Col: 26 E225 missing whitespace around operator

reviewbotreviewbot

Col: 22 E225 missing whitespace around operator

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (84 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (108 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

Col: 44 W291 trailing whitespace

reviewbotreviewbot

Col: 47 W291 trailing whitespace

reviewbotreviewbot

Col: 60 W291 trailing whitespace

reviewbotreviewbot

Col: 65 W291 trailing whitespace

reviewbotreviewbot

Col: 64 W291 trailing whitespace

reviewbotreviewbot

Col: 65 W291 trailing whitespace

reviewbotreviewbot

Col: 64 W291 trailing whitespace

reviewbotreviewbot

Col: 60 W291 trailing whitespace

reviewbotreviewbot

Col: 59 W291 trailing whitespace

reviewbotreviewbot
CV
chipx86
  1. I'm curious, how are the plans to support branches other than master? I'd prefer not to hard-code here (though we could allow usage of --tracking-branch to override easily enough).
    
    What other restrictions are there? Can you give an overview of basic usage?
    1. After a bit more research it seems that review requests are supported from feature branches, we just use remotes/p4/master for generating diffs. I think I had previously confused the fact the git-p4 only supports pushing to perforce from master and the capabilities of the rbtools patch.
      
      Usage/flow:
      1. User executes "git p4 clone //some/depot/path" which pulls down the repo and creates local tracking branch "master" for "remotes/p4/master" which is used fro syncing changes from perforce. Each commit from perforce contains metadata "[git-p4: depot-paths = "//some/depot/path/": change = xxxxxx]" which is how git-p4 determines what to push to perforce later, "git p4 commit". The absence of this metadata is used to determine what commit(s) should be pulled into the review request.
      2. Commit changes to some feature branch.
      3. Execute post-review which uses the metadata stored in the commit object to execute a combination of perforce and git commands to generate a diff in a format which reviewboard can use to generate diffs on the UI.
      
      I'll publish an updated review with the following issues resolved. Let me know if you need anything else.
    2. Any update on this? Was this patch applied?
    3. Hi Chris,
      
      Sorry, this has basically not been on my radar at all for a while.
      
      I went to go commit it, but realized I'm a bit unsure about the diff output. The Perforce output it provides does not match at all what we use for Perforce diffs. It's missing the "===" lines, and doesn't include any moved file support. I'm not entirely convinced our Perforce support will handle this.
      
      There's also TODOs in there for the timestamp, and that seems like something that needs to be fixed.
      
      These things will need to be taken care of for this patch to go in.
  2. rbtools/clients/git.py (Diff revision 1)
     
     
    Show all issues
    Can we make use of the --tracking-branch parameter here and other places where master is hard-coded?
    1. The git-p4 script only supports pushing to perforce from the master branch so this is the only remote branch it tracks. git_p4_ref here is only used to determine that we are attempting to generate a diff from a repository cloned using git-p4 and not for generating the actual diff.
    2. We use the following just to figure out if it's a git-p4 repo or not.
      
      git_p4_ref = os.path.join(git_dir, 'refs', 'remotes', 'p4', 'master')
      data = execute([self.git, 'config', '--get', 'git-p4.port'], 
                     ignore_errors=True)
      m = re.search(r'(.+)', data)
      if m and os.path.exists(git_p4_ref):
  3. rbtools/clients/git.py (Diff revision 1)
     
     
    Show all issues
    Should wrap to 80 chars.
  4. rbtools/clients/git.py (Diff revision 1)
     
     
    Show all issues
    80 chars.
  5. rbtools/clients/git.py (Diff revision 1)
     
     
    Show all issues
    Remove this line.
  6. rbtools/clients/git.py (Diff revision 1)
     
     
    Show all issues
    Trailing whitespace.
  7. rbtools/clients/git.py (Diff revision 1)
     
     
    Show all issues
    Trailing whitespace.
  8. rbtools/clients/git.py (Diff revision 1)
     
     
    Show all issues
    Trailing whitespace.
  9. rbtools/clients/git.py (Diff revision 1)
     
     
    Show all issues
    Same.
  10. rbtools/clients/git.py (Diff revision 1)
     
     
     
     
    Show all issues
    Should be on one line.
  11. rbtools/clients/git.py (Diff revision 1)
     
     
     
    Show all issues
    Blank line between these.
  12. rbtools/clients/git.py (Diff revision 1)
     
     
     
    Show all issues
    Blank line between these.
  13. rbtools/clients/git.py (Diff revision 1)
     
     
     
     
     
     
     
    Show all issues
    You could do:
    
    elif line.startswith("index ") or line.startswith("new file mode "):
  14. rbtools/clients/git.py (Diff revision 1)
     
     
    Show all issues
    Wrap to 80 chars.
  15. rbtools/clients/git.py (Diff revision 1)
     
     
    Show all issues
    Space on both sides of the =
  16. rbtools/clients/git.py (Diff revision 1)
     
     
     
    Show all issues
    Blank line between these.
    
    Also, space on both sides of the =
  17. rbtools/clients/git.py (Diff revision 1)
     
     
     
    Show all issues
    Indentation is wrong.
  18. 
      
CV
CV
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        rbtools/clients/git.py
      Ignored Files:
        .gitignore
    
    
  2. rbtools/clients/git.py (Diff revision 3)
     
     
    Show all issues
    Col: 72
     E203 whitespace before ','
    
  3. rbtools/clients/git.py (Diff revision 3)
     
     
    Show all issues
    Col: 26
     E225 missing whitespace around operator
    
  4. rbtools/clients/git.py (Diff revision 3)
     
     
    Show all issues
    Col: 22
     E225 missing whitespace around operator
    
  5. rbtools/clients/git.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  6. rbtools/clients/git.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (83 > 79 characters)
    
  7. rbtools/clients/git.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (84 > 79 characters)
    
  8. rbtools/clients/git.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (108 > 79 characters)
    
  9. rbtools/clients/git.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (82 > 79 characters)
    
  10. 
      
CV
CV
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        rbtools/clients/git.py
      Ignored Files:
        .gitignore
    
    
  2. rbtools/clients/git.py (Diff revision 4)
     
     
    Show all issues
    Col: 44
     W291 trailing whitespace
    
  3. rbtools/clients/git.py (Diff revision 4)
     
     
    Show all issues
    Col: 47
     W291 trailing whitespace
    
  4. rbtools/clients/git.py (Diff revision 4)
     
     
    Show all issues
    Col: 60
     W291 trailing whitespace
    
  5. rbtools/clients/git.py (Diff revision 4)
     
     
    Show all issues
    Col: 65
     W291 trailing whitespace
    
  6. rbtools/clients/git.py (Diff revision 4)
     
     
    Show all issues
    Col: 64
     W291 trailing whitespace
    
  7. rbtools/clients/git.py (Diff revision 4)
     
     
    Show all issues
    Col: 65
     W291 trailing whitespace
    
  8. rbtools/clients/git.py (Diff revision 4)
     
     
    Show all issues
    Col: 64
     W291 trailing whitespace
    
  9. rbtools/clients/git.py (Diff revision 4)
     
     
    Show all issues
    Col: 60
     W291 trailing whitespace
    
  10. rbtools/clients/git.py (Diff revision 4)
     
     
    Show all issues
    Col: 59
     W291 trailing whitespace
    
  11. 
      
CV
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        rbtools/clients/git.py
      Ignored Files:
        .gitignore
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
CV
Review request changed
Status:
Completed
Change Summary:
Pushed to master (cd92600). Thanks!