On 2012-12-15 14:13, Matthew Woehlke wrote:
On Monday, October 31, 2011 10:45:15 AM UTC-4, Stephen Gallagher wrote:
I've been trying to work out for some time now how to accomplish
patch-series in Git with Review Board. [...]

Modify post-review so that it will create an individual review on the
Review Board server for each patch in a branch.

I actually wouldn't like this. First off, I don't see it doing anything
even remotely reasonable if a change to a request is anything other than
new commits at the end (e.g. rewriting a commit early in the series, or
injecting commits into the middle of the branch history). I'd also be
worried about how it handles rebases. Gerrit, for example, does very badly
with rebases. RB currently, by treating requests as a single diff, does
hugely better, since a straight rebase doesn't cause a lot of change in the
overall diff.

I think the ideal solution is to slurp in both the overall diff (so that
inter-revision diffs are sane), and also the per-commit diffs, with a way
to drill down into a request to view the individual commits. Then to diff
requests at that level, RB should be clever enough to match up commits
(even if their SHA's are different, probably by some degree-of-similarity
heuristic), show differences between corresponding commits, new commits in
the chain, commits removed from the chain, etc... basically, diff patch
sets like file trees.

Yes, it's more work (and mostly server side), but the end result is much
better, and without the regressions versus the current 'squash the branch
into one diff' that your approach would introduce.

So I was thinking more about this, and came up with a vision how I think the UI would work best.

The basic idea is to add a new option to diff display, 'view as patch series'. The important parts are a) not available if a request is a single commit, or is pre-commit (or e.g. repositories for which patch series aren't available), and b) with the option OFF, things stay as they are now. That is, the diff shows the whole request, diffs between request revisions work as now (including good results when a rebase occurs), etc.

With it ON, with a single request revision (i.e. versus base, not versus a different revision of the request), the only difference is that the file list is now a file TREE. The top-level nodes are commits, ordered as in the commit history. The child nodes are files, as usual. There would be some SMALL (e.g. a line of text is okay, a big block is too much) divider between commits, which are presented in continuous pages the way files are now. (The pagination doesn't really change, except probably to add a bias to break pages between commits.)

When comparing revisions, commit matching is done as previously mentioned, then each commit is compared as revision requests are currently. Deleted or added commits would thus show the same as a diff hunk in one request revision and not in the other. Rearranged commits should be displayed like rearranged code.

(On that note, it would be nice if rearranged code could be improved to show the original code opposite, for comparison, but clearly marked that it came from elsewhere. Likewise then for rearranged commits.)

Note I haven't said anything about commit logs. I would actually ignore those server-side in the first pass, and have post-review fake up diff hunks as if a file (e.g.) ".gitlog" was created. For each individual commit, this would be the log for just that commit, always as if the file didn't previously exist (even though trying to apply such a patch series would of course fail; the reason is so that RB always presents the log as a strict creation), and likewise for the unified patch, but with the entire log. This allows the log to be reviewed the same as code, diffs of the full log against request revisions work neatly, etc.

In the long term, this could be automated server-side if there is enough need/benefit, but the only thing that should change UI-wise would be for the file header to be slightly different to indicate a commit log versus an actual file in the repository. (I'm not convinced of the necessity, however, and I can imagine it would be non-trivial to do server-side.)



Want to help the Review Board project? Donate today at 
Happy user? Let us know at http://www.reviewboard.org/users/
To unsubscribe from this group, send email to 
For more options, visit this group at 

Reply via email to