post-review: git-p4 and git-svn improvements

Review Request #889 — Created June 7, 2009 and discarded


Review Board SVN (deprecated)


git-p4 support for post-review:

generates a perforce style diff for uploading to reviewboard server.

works on the perforce repo on
  2. /trunk/rbtools/scripts/post-review (Diff revision 1)
    If anyone know a better way to detect git-p4, I am all ears
  3. /trunk/rbtools/scripts/post-review (Diff revision 1)
    --src-prefix and --dst-prefix should probably use base_path
  4. /trunk/rbtools/scripts/post-review (Diff revision 1)
    should change p4 with --parent option, but default to p4
  5. /trunk/rbtools/scripts/post-review (Diff revision 1)
    should use --parent option instead of hardcoded p4
  6. /trunk/rbtools/scripts/post-review (Diff revision 1)
    is the timestamp data in the perforce diff actually used for anything?
  2. /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"
  2. /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
  2. /trunk/rbtools/scripts/post-review (Diff revision 3)
    should check PerforceClients reviewboard settings after this line
  2. /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.
  3. /trunk/rbtools/scripts/post-review (Diff revision 5)
    Should be elif
  4. /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")
  5. /trunk/rbtools/scripts/post-review (Diff revision 5)
    Blank line here.
  6. /trunk/rbtools/scripts/post-review (Diff revision 5)
    Comments like this shouldn't be in a final diff.
  7. /trunk/rbtools/scripts/post-review (Diff revision 5)
    These flags are all the same. It would be nice to condense this code.
  8. /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.
  9. /trunk/rbtools/scripts/post-review (Diff revision 5)
    End with a period, and capitalize P on Perforce.
  10. /trunk/rbtools/scripts/post-review (Diff revision 5)
    Space after #. Begin sentences with a capital letter, and end with a period.
  11. /trunk/rbtools/scripts/post-review (Diff revision 5)
    Condense this into one elif.
  12. /trunk/rbtools/scripts/post-review (Diff revision 5)
    Same comment about the comments.
  13. /trunk/rbtools/scripts/post-review (Diff revision 5)
    Does this wrap to 80 columns?
    This should use:
    "%s%s@%s" % (base_path, filename, p4rev)
  14. /trunk/rbtools/scripts/post-review (Diff revision 5)
    Should use: r'^%s%s#(\d+).*$" % (base_path, filename)
  15. /trunk/rbtools/scripts/post-review (Diff revision 5)
    Space on both sides of =
  16. /trunk/rbtools/scripts/post-review (Diff revision 5)
    Same here.
  17. /trunk/rbtools/scripts/post-review (Diff revision 5)
    Blank line before this, and make sure it wraps to 80 columns.
  18. /trunk/rbtools/scripts/post-review (Diff revision 5)
    This is indented the wrong number of spaces.
  1. 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.
  2. /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.
  3. /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.
    1. I tried to address this in my patch which you might want to look at:
      On that note, dvcs would really help with this right now :)
    2. I see, but I don't fully understand this feature.
      In your scenario, is A a diff against the "trunk" equivalent branch? and B your changes to that branch?
      I am kind of struggling with the fact that if A is based on p4 and B is based on A, then, in git, this means that B is based on p4 as well.
      Are you looking to have review board silently apply patchset A to the repo, then only show the diff created with B?
    3. Yes, review board will apply diff (p4->A) and then show (A->B) as the diff in the review. If you ever update the diff through the web interface, the dialog has a "diff" field and a "parent diff" field. The parent diff is applied first, and the regular diff is applied on top of the result.
      This way it's possible to post a series of separate but related reviews that modify the same files (which is one of the benefits of using git instead of p4)
      In terms of which base/trunk branch to pick, I suppose "p4 is probably more correct than "master". But I think it should also be a separte argument because you might want to post a diff based on an older change. For example, you might sync the remote p4 branch, but keep your existing changes where they are instead of rebasing them.
    4. My solution to specifying an older p4 revision was to specify a sha1 of an older git-p4 revision as parent.  all of the git-diff commands should accept valid commitish.  This is also the reason I originally checked more that just the latest revision, and I looked in the current branch, as the remote git-p4 branch might be more up to date than the the current branch if the current branch hasn't merged the latest p4 revision.
      just to make sure I got this,
      --base defaults to p4, (trunk and origin, for git-svn and regular git)
      --parent defaults to base, and diffs against base
      --branch defaults to current branch, and diffs against parent.
      does this sound valid, and are people okay with me adding a --base option?
      I would like a way to intelligently guess the correct p4 revision, just in case the remote p4 branch is more up to date than the diff I am submitting.
    5. Defaulting to 'trunk' is not correct for git-svn. There's no reason for anything to be named trunk in a git-svn repository. I understand the argument, though, that the remote may be more up-to-date and so you'll pull in unwanted changes. If you choose origin for the git default, git-svn should be the default for git-svn.
    6. The flags seem good to me, except for the --branch - see my comment above this one (it's not supposed to be a local git branch name as far as I can tell). I we want to have an override for the current source branch, maybe it should be called something like --source? Also, if parent is not specified, or parent is the same as base, it's probably a good idea to skip the diff and leave parent_diff_lines = None.
      It seems that the SVN remote name is different depending on what you specify for --trunk when doing the initial clone. I think the right way to do this be to check the config option "svn-remote.svn.fetch" and use whatever it reports.
      As far as choosing the correct base branch when the remote branch more up to date, "git merge-base" should be useful. Give it the most recent remote branch and the local branch that the diff is being created for (or HEAD if blank), and it will print the SHA1 of the common ancestor branch, which should be a remote branch that can be used as the base.
  4. /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.
  5. /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.
Review request changed
Testing Done:

works on the perforce repo on


works on the perforce repo on

  2. /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.
  3. /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)
  1. I'd love to see an updated version of this patch.