If anyone know a better way to detect git-p4, I am all ears

--src-prefix and --dst-prefix should probably use base_path

should change p4 with --parent option, but default to p4

should use --parent option instead of hardcoded p4

is the timestamp data in the perforce diff actually used for anything?

its tempting to add a 

if self.type == "svn":
    parent_branch = options.parent_branch or "remotes/trunk"

base_path is actually important in diff generation, this should be solved before this patch is commited upstream

should check PerforceClients reviewboard settings after this line

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.

Should be elif

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

Blank line here.

Comments like this shouldn't be in a final diff.

These flags are all the same. It would be nice to condense this code.

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.

End with a period, and capitalize P on Perforce.

Space after #. Begin sentences with a capital letter, and end with a period.

Condense this into one elif.

Same comment about the comments.

Does this wrap to 80 columns?

This should use:
"%s%s@%s" % (base_path, filename, p4rev)

Should use: r'^%s%s#(\d+).*$" % (base_path, filename)

Space on both sides of =

Same here.

Blank line before this, and make sure it wraps to 80 columns.

This is indented the wrong number of spaces.

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.

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.

"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.

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.

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.

base_path doesn't mean anything for git or perforce, only for SVN. You can remove this comment.

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)

I'd love to see an updated version of this patch.