Summary

Adding git-p4 support

Review Request #3122 — Created May 30, 2012 and submitted Aug. 15, 2013, 1:30 p.m.

Information

Owner

cvanes

Repository

RBTools

Branch

master

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

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

Testing Done

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.

Issues

Description	From	Last Updated
Can we make use of the --tracking-branch parameter here and other places where master is hard-coded?	chipx86	June 29, 2012, 2:36 p.m.
Should wrap to 80 chars.	chipx86	June 29, 2012, 2:41 p.m.
80 chars.	chipx86	June 29, 2012, 2:42 p.m.
Remove this line.	chipx86	June 29, 2012, 2:43 p.m.
Trailing whitespace.	chipx86	June 29, 2012, 2:43 p.m.
Trailing whitespace.	chipx86	June 29, 2012, 2:45 p.m.
Trailing whitespace.	chipx86	June 29, 2012, 2:46 p.m.
Same.	chipx86	June 29, 2012, 2:47 p.m.
Should be on one line.	chipx86	June 29, 2012, 2:47 p.m.
Blank line between these.	chipx86	June 29, 2012, 2:49 p.m.
Blank line between these.	chipx86	June 29, 2012, 3:04 p.m.
You could do: elif line.startswith("index ") or line.startswith("new file mode "):	chipx86	June 29, 2012, 3:04 p.m.
Wrap to 80 chars.	chipx86	June 29, 2012, 3:04 p.m.
Space on both sides of the =	chipx86	June 29, 2012, 3:04 p.m.
Blank line between these. Also, space on both sides of the =	chipx86	June 29, 2012, 3:04 p.m.
Indentation is wrong.	chipx86	June 29, 2012, 3:04 p.m.
Col: 72 E203 whitespace before ','	reviewbot	March 1, 2013, 12:45 p.m.
Col: 26 E225 missing whitespace around operator	reviewbot	March 1, 2013, 12:45 p.m.
Col: 22 E225 missing whitespace around operator	reviewbot	March 1, 2013, 12:45 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 1, 2013, 12:57 p.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	March 1, 2013, 12:58 p.m.
Col: 80 E501 line too long (84 > 79 characters)	reviewbot	March 1, 2013, 1 p.m.
Col: 80 E501 line too long (108 > 79 characters)	reviewbot	March 1, 2013, 1:01 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	March 1, 2013, 1:02 p.m.
Col: 44 W291 trailing whitespace	reviewbot	March 1, 2013, 1:09 p.m.
Col: 47 W291 trailing whitespace	reviewbot	March 1, 2013, 1:09 p.m.
Col: 60 W291 trailing whitespace	reviewbot	March 1, 2013, 1:09 p.m.
Col: 65 W291 trailing whitespace	reviewbot	March 1, 2013, 1:09 p.m.
Col: 64 W291 trailing whitespace	reviewbot	March 1, 2013, 1:09 p.m.
Col: 65 W291 trailing whitespace	reviewbot	March 1, 2013, 1:09 p.m.
Col: 64 W291 trailing whitespace	reviewbot	March 1, 2013, 1:09 p.m.
Col: 60 W291 trailing whitespace	reviewbot	March 1, 2013, 1:09 p.m.
Col: 59 W291 trailing whitespace	reviewbot	March 1, 2013, 1:09 p.m.

Change Summary:: Adding pull request url
Description:: This was taken from a previous patch request (http://reviews.review-board.org/r/889/) and modified for the current 0.4.1 release. The obvious limitation is that reviews can only be sent for the master branch but I've used this within my workplace for quite a while so the patch has been thoroughly tested. Git-p4 only supports pushing to perforce from the master branch but I've plans to make further changes to support sending reviews from any branch.
+
+
github pull request: https://github.com/reviewboard/rbtools/pull/16

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?

cvanes June 29, 2012, 3:49 p.m.

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.

cvanes July 24, 2012, 4:38 p.m.

Any update on this? Was this patch applied?

chipx86 March 3, 2013, 12:32 p.m.

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.

rbtools/clients/git.py (Diff revision 1)

The issue has been dropped. Show all issues

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

cvanes June 29, 2012, 3:05 p.m.

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.

cvanes June 29, 2012, 3:12 p.m.

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):

rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Should wrap to 80 chars.
```
rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
80 chars.
```
rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Remove this line.
```
rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Trailing whitespace.
```
rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Trailing whitespace.
```
rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Trailing whitespace.
```
rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same.
```
rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Should be on one line.
```
rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Blank line between these.
```
rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Blank line between these.
```

rbtools/clients/git.py (Diff revision 1)

The issue has been resolved. Show all issues

You could do:

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

rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Wrap to 80 chars.
```
rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Space on both sides of the =
```
rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Blank line between these.

Also, space on both sides of the =
```
rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Indentation is wrong.
```

Change Summary:

Updates following review.

Diff:

Revision 2 (+20 -19)

Show changes

rbtools/clients/git.py

Change Summary:

Tested against latest master. It would be good if this could be part of 0.5 as we need to keep patching the script for use in our organisation.

Diff:

Revision 3 (+75 -2)

Show changes

	.gitignore
	rbtools/clients/git.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    rbtools/clients/git.py
  Ignored Files:
    .gitignore

rbtools/clients/git.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 72
 E203 whitespace before ','
```
rbtools/clients/git.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 26
 E225 missing whitespace around operator
```
rbtools/clients/git.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 22
 E225 missing whitespace around operator
```
rbtools/clients/git.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
rbtools/clients/git.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (83 > 79 characters)
```
rbtools/clients/git.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (84 > 79 characters)
```
rbtools/clients/git.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (108 > 79 characters)
```
rbtools/clients/git.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (82 > 79 characters)
```

Description:: ~
This was taken from a previous patch request (http://reviews.review-board.org/r/889/) and modified for the current 0.4.1 release. The obvious limitation is that reviews can only be sent for the master branch but I've used this within my workplace for quite a while so the patch has been thoroughly tested. Git-p4 only supports pushing to perforce from the master branch but I've plans to make further changes to support sending reviews from any branch.
~
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.

~
github pull request: https://github.com/reviewboard/rbtools/pull/16
~
original github pull request: https://github.com/reviewboard/rbtools/pull/16
Testing Done:: ~
We've been testing this for quite a while on 0.2 and recently patched 0.4.1, so extensive testing has been completed by a number of users.
~
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.

Change Summary:

Implementing coding style suggestions from reviewbot.

Diff:

Revision 4 (+84 -2)

Show changes

	.gitignore
	rbtools/clients/git.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    rbtools/clients/git.py
  Ignored Files:
    .gitignore

rbtools/clients/git.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 44
 W291 trailing whitespace
```
rbtools/clients/git.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 47
 W291 trailing whitespace
```
rbtools/clients/git.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 60
 W291 trailing whitespace
```
rbtools/clients/git.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 65
 W291 trailing whitespace
```
rbtools/clients/git.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 64
 W291 trailing whitespace
```
rbtools/clients/git.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 65
 W291 trailing whitespace
```
rbtools/clients/git.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 64
 W291 trailing whitespace
```
rbtools/clients/git.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 60
 W291 trailing whitespace
```
rbtools/clients/git.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 59
 W291 trailing whitespace
```

Change Summary:

Removing trailing whitespace.

Diff:

Revision 5 (+84 -2)

Show changes

	.gitignore
	rbtools/clients/git.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    rbtools/clients/git.py
  Ignored Files:
    .gitignore

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to master (cd92600). Thanks!

		This was taken from a previous patch request (http://reviews.review-board.org/r/889/) and modified for the current 0.4.1 release. The obvious limitation is that reviews can only be sent for the master branch but I've used this within my workplace for quite a while so the patch has been thoroughly tested. Git-p4 only supports pushing to perforce from the master branch but I've plans to make further changes to support sending reviews from any branch.
	+
	+	github pull request: https://github.com/reviewboard/rbtools/pull/16

~		We've been testing this for quite a while on 0.2 and recently patched 0.4.1, so extensive testing has been completed by a number of users.
	~	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.