-
-
/trunk/rbtools/scripts/post-review (Diff revision 1) If anyone know a better way to detect git-p4, I am all ears
-
/trunk/rbtools/scripts/post-review (Diff revision 1) --src-prefix and --dst-prefix should probably use base_path
-
/trunk/rbtools/scripts/post-review (Diff revision 1) should change p4 with --parent option, but default to p4
-
/trunk/rbtools/scripts/post-review (Diff revision 1) should use --parent option instead of hardcoded p4
-
/trunk/rbtools/scripts/post-review (Diff revision 1) is the timestamp data in the perforce diff actually used for anything?
post-review: git-p4 and git-svn improvements
Review Request #889 — Created June 7, 2009 and discarded
git-p4 support for post-review: generates a perforce style diff for uploading to reviewboard server.
works on the perforce repo on acm.cs.uic.edu/reviews
HE
HE
Change Summary:
changed default parent from master to p4, which is the default git-p4 upstream branch
Diff: |
Revision 2 (+81 -7) |
---|
HE
-
-
/trunk/rbtools/scripts/post-review (Diff revision 2) its tempting to add a if self.type == "svn": parent_branch = options.parent_branch or "remotes/trunk"
HE
-
-
/trunk/rbtools/scripts/post-review (Diff revision 2) base_path is actually important in diff generation, this should be solved before this patch is commited upstream
HE
Change Summary:
cleaned up handling of --branch and --parent for git-p4 and git-svn
Summary: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+79 -8) |
HE
-
-
/trunk/rbtools/scripts/post-review (Diff revision 3) should check PerforceClients reviewboard settings after this line
HE
Change Summary:
whitespace cleanups fixed git-p4 diff to use correct paths in output
Diff: |
Revision 4 (+84 -9) |
---|
-
-
/trunk/rbtools/scripts/post-review (Diff revision 5) This should be removed before being committed. Base paths are used when filenames in a diff is relative to a directory, rather than absolute paths in a repository. In the case of git or Perforce, you shouldn't have to worry about this. It can be blank.
-
-
/trunk/rbtools/scripts/post-review (Diff revision 5) I think I'd prefer we tackle this a different way. Store a dictionary as part of the class definition, mapping types to default parent branches. Then this code becomes: parent_branch = options.parent_branch or \ self.DEFAULT_PARENT_BRANCHES.get(self.type, "origin")
-
-
/trunk/rbtools/scripts/post-review (Diff revision 5) Comments like this shouldn't be in a final diff.
-
/trunk/rbtools/scripts/post-review (Diff revision 5) These flags are all the same. It would be nice to condense this code.
-
/trunk/rbtools/scripts/post-review (Diff revision 5) Space after #, and please flesh out this comment so that someone not you down the road can know exactly what needs to be done here.
-
/trunk/rbtools/scripts/post-review (Diff revision 5) End with a period, and capitalize P on Perforce.
-
/trunk/rbtools/scripts/post-review (Diff revision 5) Space after #. Begin sentences with a capital letter, and end with a period.
-
-
-
/trunk/rbtools/scripts/post-review (Diff revision 5) Does this wrap to 80 columns? This should use: "%s%s@%s" % (base_path, filename, p4rev)
-
/trunk/rbtools/scripts/post-review (Diff revision 5) Should use: r'^%s%s#(\d+).*$" % (base_path, filename)
-
-
-
/trunk/rbtools/scripts/post-review (Diff revision 5) Blank line before this, and make sure it wraps to 80 columns.
-
MU
-
I have been planning to implement git-p4 support for post-review, and I was very glad to find a review already in progress for it. I would be very interested in getting this change finalized so I could add the remaining pieces on top of it. I made a modified version based on your diff that implements full support for parent diffs and multiple depot paths, but I'd prefer to get your change finished and submitted, so I could post my changes as a separate review.
-
/trunk/rbtools/scripts/post-review (Diff revision 5) I don't think options.branch currently means what you're using it for - it's posted to review board as the "branch" review field, and it would probably be pretty meaningless to show the local git branch name or a SHA1 hash. I suggest adding a new flag (maybe --source?) if you want to allow specifying a custom source git branch instead of HEAD.
-
/trunk/rbtools/scripts/post-review (Diff revision 5) "master" is hardcoded because it's used as the base branch to create the diff from. However, it would be nice to have it as a custom parameter with a default of "master". Using "parent" to mean the "base" branch is not quite accurate - review board allows posting a separate parent diff in addition to the normal diff. This is important when posting a series of reviews that depend on each other and modify the same files. For example, if you have these branches: p4 -> A -> B, you might want to post two reviews: (p4 -> A) and (A -> B). If we name branches as 'base', 'parent', and 'child', they correspond to the following: (p4 -> A): base=p4, parent=None (or p4), child=A (A -> B): base=p4, parent=A, child=B I'd suggest keeping the old behavior for now, and I can add support for parent diffs in my review.
-
/trunk/rbtools/scripts/post-review (Diff revision 5) This will get the entire history of the parent branch, which takes very long if there are many perforce commits. Assuming the branch argument is an imported p4 branch (base, not parent as explained above), only the last commit is necessary, so the arguments should include ("-n", "1") to limit the log output to one commit. It also needs to use split_lines=True so that the regex search can search individual lines instead of the whole output.
-
/trunk/rbtools/scripts/post-review (Diff revision 5) This should be using 'line' instead of 'log' (with the change above to split lines). This doesn't support git-p4 options in a different order, or git-p4 instances with multiple depot paths, but I can do that as part of my change.
-
-
/trunk/rbtools/scripts/post-review (Diff revision 5) base_path doesn't mean anything for git or perforce, only for SVN. You can remove this comment.
-
/trunk/rbtools/scripts/post-review (Diff revision 5) scan_for_server will return None, and you immediately do that if this falls through, so you can just do return PerforceClient().scan_for_server(repository_info)