I've been trying to work out for some time now how to accomplish patch-series in Git with Review Board. This is a very important part of my project's workflow, and the lack of this support has been preventing us from deploying.
I think I came up with three ideas, each building on each other, to allow this support to come to fruition gradually.
Step 1) Modify post-review so that it will create an individual review on the Review Board server for each patch in a branch. Post-review should also amend the commit history of the patches in the branch to tag them with a link to the review in Review Board. Post-review would then be made aware of the presence of this tag, and in the case of review updates, it should use this tag to determine which review to update.
Furthermore, post-review should attach a complete list of the patches in review as part of the review description (this would probably require going back after creating the initial reviews and adding this afterwards as a comment). Ideally, it would read something like: http://reviews.example.com/r/1 http://reviews.example.com/r/2 <-- http://reviews.example.com/r/3
So you'd always have an easy way to identify which patch in the series you were looking at, as well as a view on which other reviews were related.
If the branch adds or removes patches, it should add a new comment with the new list of patches, ordered appropriately.
Pros: Changes are almost entirely in post-review. Cons: Could result in heavy amounts of email, since each patch would have its own email thread.
Step 2) Post-review should generate patches formatted with 'git format-patch -M -C --full-index' and attach them as raw files to the review(s). There are several ways this could be done; one patch per review, or all patches on the first review in the series*.
As I understand it, raw file upload support is already planned for 1.7, so this wouldn't be a major effort to implement. Mainly just extending the changes from Step 1) to include generating and uploading the files.
* This might have issues if the first patch in a series is ever removed. Might require some careful design.
Step 3) (Long-term ideas, some of which are taken from gerrit) Make the Review Board server into a public git repository. Post-review could then commit to this public repository in a private branch which would then be used to generate the reviews for Step 1) and Step 2). Users can then set up a remote repository with 'git remote add' to be able to easily retrieve the changes and perform local tests.
Doing things this way would require quite a bit more work than Step 1) or Step 2), but in the long-term, it would fit a lot more closely with common git workflows. We probably wouldn't need to have post-review generate the raw files at all, here. We could have Review Board itself generate them on-request rather than storing them separately (since it will have access to the repository directly).
I just thought I might mention this email again, since I saw no responses the first time. I noticed someone else on the list today asking about patch-sets (this time for Mercurial) and thought that maybe some of these ideas might work there as well.
On Mon, 2011-10-31 at 10:45 -0400, Stephen Gallagher wrote: > I've been trying to work out for some time now how to accomplish > patch-series in Git with Review Board. This is a very important part of > my project's workflow, and the lack of this support has been preventing > us from deploying.
> I think I came up with three ideas, each building on each other, to > allow this support to come to fruition gradually.
> Step 1) > Modify post-review so that it will create an individual review on the > Review Board server for each patch in a branch. Post-review should also > amend the commit history of the patches in the branch to tag them with a > link to the review in Review Board. Post-review would then be made aware > of the presence of this tag, and in the case of review updates, it > should use this tag to determine which review to update.
> Furthermore, post-review should attach a complete list of the patches in > review as part of the review description (this would probably require > going back after creating the initial reviews and adding this afterwards > as a comment). > Ideally, it would read something like: > http://reviews.example.com/r/1 > http://reviews.example.com/r/2 <-- > http://reviews.example.com/r/3
> So you'd always have an easy way to identify which patch in the series > you were looking at, as well as a view on which other reviews were > related.
> If the branch adds or removes patches, it should add a new comment with > the new list of patches, ordered appropriately.
> Pros: Changes are almost entirely in post-review. > Cons: Could result in heavy amounts of email, since each patch would > have its own email thread.
> Step 2) > Post-review should generate patches formatted with 'git format-patch -M > -C --full-index' and attach them as raw files to the review(s). There > are several ways this could be done; one patch per review, or all > patches on the first review in the series*.
> As I understand it, raw file upload support is already planned for 1.7, > so this wouldn't be a major effort to implement. Mainly just extending > the changes from Step 1) to include generating and uploading the files.
> * This might have issues if the first patch in a series is ever removed. > Might require some careful design.
> Step 3) (Long-term ideas, some of which are taken from gerrit) > Make the Review Board server into a public git repository. Post-review > could then commit to this public repository in a private branch which > would then be used to generate the reviews for Step 1) and Step 2). > Users can then set up a remote repository with 'git remote add' to be > able to easily retrieve the changes and perform local tests.
> Doing things this way would require quite a bit more work than Step 1) > or Step 2), but in the long-term, it would fit a lot more closely with > common git workflows. We probably wouldn't need to have post-review > generate the raw files at all, here. We could have Review Board itself > generate them on-request rather than storing them separately (since it > will have access to the repository directly).
I thought it might be worth bumping this thread again, given Christian's comments in the thread "Focus and Priorities". I would like to hear some thoughts on my suggested approach here.
On Thu, 2011-11-10 at 09:51 -0500, Stephen Gallagher wrote: > I just thought I might mention this email again, since I saw no > responses the first time. I noticed someone else on the list today > asking about patch-sets (this time for Mercurial) and thought that maybe > some of these ideas might work there as well.
> On Mon, 2011-10-31 at 10:45 -0400, Stephen Gallagher wrote: > > I've been trying to work out for some time now how to accomplish > > patch-series in Git with Review Board. This is a very important part of > > my project's workflow, and the lack of this support has been preventing > > us from deploying.
> > I think I came up with three ideas, each building on each other, to > > allow this support to come to fruition gradually.
> > Step 1) > > Modify post-review so that it will create an individual review on the > > Review Board server for each patch in a branch. Post-review should also > > amend the commit history of the patches in the branch to tag them with a > > link to the review in Review Board. Post-review would then be made aware > > of the presence of this tag, and in the case of review updates, it > > should use this tag to determine which review to update.
> > Furthermore, post-review should attach a complete list of the patches in > > review as part of the review description (this would probably require > > going back after creating the initial reviews and adding this afterwards > > as a comment). > > Ideally, it would read something like: > > http://reviews.example.com/r/1 > > http://reviews.example.com/r/2 <-- > > http://reviews.example.com/r/3
> > So you'd always have an easy way to identify which patch in the series > > you were looking at, as well as a view on which other reviews were > > related.
> > If the branch adds or removes patches, it should add a new comment with > > the new list of patches, ordered appropriately.
> > Pros: Changes are almost entirely in post-review. > > Cons: Could result in heavy amounts of email, since each patch would > > have its own email thread.
> > Step 2) > > Post-review should generate patches formatted with 'git format-patch -M > > -C --full-index' and attach them as raw files to the review(s). There > > are several ways this could be done; one patch per review, or all > > patches on the first review in the series*.
> > As I understand it, raw file upload support is already planned for 1.7, > > so this wouldn't be a major effort to implement. Mainly just extending > > the changes from Step 1) to include generating and uploading the files.
> > * This might have issues if the first patch in a series is ever removed. > > Might require some careful design.
> > Step 3) (Long-term ideas, some of which are taken from gerrit) > > Make the Review Board server into a public git repository. Post-review > > could then commit to this public repository in a private branch which > > would then be used to generate the reviews for Step 1) and Step 2). > > Users can then set up a remote repository with 'git remote add' to be > > able to easily retrieve the changes and perform local tests.
> > Doing things this way would require quite a bit more work than Step 1) > > or Step 2), but in the long-term, it would fit a lot more closely with > > common git workflows. We probably wouldn't need to have post-review > > generate the raw files at all, here. We could have Review Board itself > > generate them on-request rather than storing them separately (since it > > will have access to the repository directly).
Thanks for bumping this thread. I've been giving this functionality some thought lately, and here's what I'm currently leaning toward:
1) Give Review Board support for attaching multiple diffs (not just one) in any given update. It would work like today in that a user would run post-review and another user would see the latest changes, except it'd be broken up into several diffs.
2) Update post-review to support providing multiple diffs on upload automatically. There could be an option for squashing.
3) Store all the extra diff metadata and show it per-diff set in the diff viewer.
That's the first step and gets us patch series. The next would be deeper repo integration, which will benefit from all the deep hosting service work I'm doing right now for GitHub.
4) Provide an endpoint for WebHooks on different services (like GitHub) that will auto-post review requests when there are pull requests.
5) Tie into the APIs to accept/merge changes when it's possible (possibly easier for hosting services, but harder for standalone repos)
6) Add metadata for branches and make it possible for Review Board to tie in closer with a branch on an accessible repo, to make updating easier. This isn't fully thought out yet.
7) Eventually add some sort of tree browser. This gets very complicated on Git without a hosting service or without the Git repo being directly accessible.
Whatever we do, I think it's important that Review Board be itself aware of things like patchsets and DVCS workflows.
Christian
On Apr 23, 2012, at 7:46, Stephen Gallagher <step...@gallagherhome.com> wrote:
> I thought it might be worth bumping this thread again, given Christian's > comments in the thread "Focus and Priorities". I would like to hear some > thoughts on my suggested approach here.
> On Thu, 2011-11-10 at 09:51 -0500, Stephen Gallagher wrote: >> I just thought I might mention this email again, since I saw no >> responses the first time. I noticed someone else on the list today >> asking about patch-sets (this time for Mercurial) and thought that maybe >> some of these ideas might work there as well.
>> On Mon, 2011-10-31 at 10:45 -0400, Stephen Gallagher wrote: >>> I've been trying to work out for some time now how to accomplish >>> patch-series in Git with Review Board. This is a very important part of >>> my project's workflow, and the lack of this support has been preventing >>> us from deploying.
>>> I think I came up with three ideas, each building on each other, to >>> allow this support to come to fruition gradually.
>>> Step 1) >>> Modify post-review so that it will create an individual review on the >>> Review Board server for each patch in a branch. Post-review should also >>> amend the commit history of the patches in the branch to tag them with a >>> link to the review in Review Board. Post-review would then be made aware >>> of the presence of this tag, and in the case of review updates, it >>> should use this tag to determine which review to update.
>>> Furthermore, post-review should attach a complete list of the patches in >>> review as part of the review description (this would probably require >>> going back after creating the initial reviews and adding this afterwards >>> as a comment). >>> Ideally, it would read something like: >>> http://reviews.example.com/r/1 >>> http://reviews.example.com/r/2 <-- >>> http://reviews.example.com/r/3
>>> So you'd always have an easy way to identify which patch in the series >>> you were looking at, as well as a view on which other reviews were >>> related.
>>> If the branch adds or removes patches, it should add a new comment with >>> the new list of patches, ordered appropriately.
>>> Pros: Changes are almost entirely in post-review. >>> Cons: Could result in heavy amounts of email, since each patch would >>> have its own email thread.
>>> Step 2) >>> Post-review should generate patches formatted with 'git format-patch -M >>> -C --full-index' and attach them as raw files to the review(s). There >>> are several ways this could be done; one patch per review, or all >>> patches on the first review in the series*.
>>> As I understand it, raw file upload support is already planned for 1.7, >>> so this wouldn't be a major effort to implement. Mainly just extending >>> the changes from Step 1) to include generating and uploading the files.
>>> * This might have issues if the first patch in a series is ever removed. >>> Might require some careful design.
>>> Step 3) (Long-term ideas, some of which are taken from gerrit) >>> Make the Review Board server into a public git repository. Post-review >>> could then commit to this public repository in a private branch which >>> would then be used to generate the reviews for Step 1) and Step 2). >>> Users can then set up a remote repository with 'git remote add' to be >>> able to easily retrieve the changes and perform local tests.
>>> Doing things this way would require quite a bit more work than Step 1) >>> or Step 2), but in the long-term, it would fit a lot more closely with >>> common git workflows. We probably wouldn't need to have post-review >>> generate the raw files at all, here. We could have Review Board itself >>> generate them on-request rather than storing them separately (since it >>> will have access to the repository directly).
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.
Make the Review Board server into a public git repository.
I'd be careful with this... it's actually been one of the main PROBLEMS we have with gerrit. If you aren't careful, gerrit's repository gets out of sync with the actual repository, at which point both posting new requests and merging existing branches can quit working. It sounds like your thoughts for how to use this might not take it so far as to get into that trap, at least. (I think the main trick is to make it resilient against master having advanced beyond what RB considers 'latest'... or even to avoid RB having such a concept altogether, and just accept whatever SHA a request says is its upstream.)
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. This is a very important part of > my project's workflow, and the lack of this support has been preventing > us from deploying.
Patch series was the one feature missing from RB that my team really wanted. We ended up integrating topic branches into our workflow as a surrogate for patch series and use a custom script to create reviews (currently running RB 1.6.12). In our current workflow, there are two ways reviews are generated:
1) commits on "main" branches (e.g. master or maint) automatically generate a review via a post-receive hook 2) The topic-branch-review script, a wrapper around post-review, which a developer runs when s/he is ready to merge (essentially a pull request)
Internally the script calculates the merge base of topic and base. It then iterates over commits between the topic-branch and the merge-base, calculating the diff between each commit and the merge-base (not the commit's parent). Doing cumulative diffs allows us to use the interdiff functionality in RB to see one or more commits (many of our devs like seeing the topic branch as a single commit for review, I tend to break it down into a patch series). Each diff is used to update the original review, with a change description set to the oneline commit summary. The review request summary is simply the log of all the commits uploaded (the developer can use this as a starting point when adding a real description).
The optional arguments are to update an existing review (usually in response to review comments), and follows the same cumulative diff methodology.
The script is simple enough we try not to deal with rebases or merges. Common practice is to rebase the topic branch on top of the base branch just prior to posting the review, but then not rebase after that. I know we have dealt with merging the base branch into the topic branch, but I forget the details now (I think it shows up as a single commit with all the changes from the base).
From personal experience, I'd say this model works well for new features that require 10-30 commits to achieve maturity. We've reviewed some 70-100 commit topic branches, which I generally don't recommend but certainly happens. With that many commits remembering what RB diff number goes with which commit is almost impossible -- this could be improved with some UI in RB that helps translate diff number to commit ID (or change description). With the script we've found RB fits our development style very well and is very comfortable for our (relatively informal) review workflow.
> 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.)
> 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.)
> Thoughts?
I was thinking it might be a good time to start bringing this discussion
up again. I know things are probably pretty intense with the 1.8 release
plans, but I know there are a lot of people interested in this topic and
I'd like to get it back into the public view.
I had an offline conversation with Christian about patch-series a month
or so ago and I think I should probably try to summarize/transcribe the
thoughts we had back then as well.
Christian:
I absolutely want to introduce patchset support. My thought is that
post-review (well, rbt post, the successor coming in RBTools 0.5) would
allow you to optionally post a series of commits as one patchset,
instead of squashing them.
Review Board would be able to differentiate patchsets vs. standard
diffs. When a review request doesn't contain any patchsets, it'd be just
like it is today. However, when a patchset is involved, we change things
a little bit.
The revision selector is still there, but a patchset now counts as a
revision. The patchset will show each and every commit, along with their
metadata (descriptions and such). They could be viewed squashed (same
view as today), or per-commit.
We'd have some intelligence to try to see if a patchset was just amended
when updated next, or if part of it was replOn Sun 16 Dec 2012 01:28:46
PM EST, Matthew Woehlke wrote:
...
> 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.)
> Thoughts?
aced, or what. We'd still have a new patchset entry internally, but we'd
take advantage of some of our diff data sharing to see what the common
parts are. We can then indicate to the reviewer that commits were added,
or changed, or where. That'd make it easier to see what commits actually
need to be reviewed.
Along with this, there'd be some other improvements. We'd have a field
indicating where the development branch/repository is (so that people
can pull from you, if it makes sense in their setup), and the
branch/revision where the commit was made (as part of the submitted
form, and also available to set via new RBTools commands and options).
So that's the basic thought right now. There is a lot on our plates, so
I don't know when... Might not be 1.8 at this rate, but parts of it
might hit it. We're hoping to do more frequent 1.x releases. 1.8 at this
point is planned to be a rewrite of parts of the JavaScript codebase to
make it more extensible, and add some student projects and a few other
nice new bits of UI, and I'm hoping we can release by April.
Stephen:
> We'd have some intelligence to try to see if a patchset was just
> amended when updated next, or if part of it was replaced, or what.
> We'd still have a new patchset entry internally, but we'd take
> advantage of some of our diff data sharing to see what the common
> parts are. We can then indicate to the reviewer that commits were
> added, or changed, or where. That'd make it easier to see what commits
> actually need to be reviewed.
That will definitely be some interesting logic, especially if patches
are split or merged. We'd probably want some heuristics to determine
when we should treat a patch as a modification of an earlier patch vs. a
whole new patch. (e.g. what happens when as part of a review, someone
asks you to split a patch into two).
> Along with this, there'd be some other improvements. We'd have a field
> indicating where the development branch/repository is (so that people
> can pull from you, if it makes sense in their setup), and the
> branch/revision where the commit was made (as part of the submitted
> form, and also available to set via new RBTools commands and options).
On a related note, it would be a fantastic feature if we could also tie
this into github pull requests. It looks like the github API[1] allows a
client to register for notification of pull requests. It would be
excellent to be able to automatically-generate a ReviewBoard review for
the requested branch.
Similarly, it would be pretty excellent if we could tie in a button to
push the Merge Button™, though that would necessitate adding a
permission feature on ReviewBoard to set who was allowed to perform
merges. We'd also probably want to implement a policy such as "The
review must have at least N 'Ship It!' values before it can be merged."
Detailed comments below, but summarizing my personal short term priorities:
1. Custom field support.
2. Support DVCS revision in the changenum field.
3. Add a field for local branch name.
Of course, (1) means that (3) (and (2), sort-of) can be done with an extension... which is one reason (1) is first, but also because we have local need for custom fields that are probably not useful to a wide enough audience to justify being built-in. This lack is the biggest pain point for us right now.
I think pushing some things (especially patch set support) to 2.x sounds reasonable, but please, *please* at least have custom field support in 1.8 :-).
(Besides, a number of the other features can be implemented to at least some extent with better extension support. This would be a good path for a wider developer base to work on them, and/or to 'try them out' conceptually before bringing them into RB core... if that is even valuable in the long term. It may end up that some features work so well as extensions that there is no strong reason they can't just stay extensions.)
On 2013-03-12 08:52, Stephen Gallagher wrote:
> [paraphrasing Christian:]
> I absolutely want to introduce patchset support. My thought is that
> post-review (well, rbt post, the successor coming in RBTools 0.5) would
> allow you to optionally post a series of commits as one patchset,
> instead of squashing them.
> Review Board would be able to differentiate patchsets vs. standard
> diffs. When a review request doesn't contain any patchsets, it'd be just
> like it is today. However, when a patchset is involved, we change things
> a little bit.
> The revision selector is still there, but a patchset now counts as a
> revision. The patchset will show each and every commit, along with their
> metadata (descriptions and such). They could be viewed squashed (same
> view as today), or per-commit.
So... while I understand that this is technically more feasible, I'd be concerned with how it will handle e.g. rebases. Also, I think it will be much more logical for reviewers to keep commits within a request separate from revisions of the request, rather than mixing these concepts.
Basically, unless you're talking about a project with a draconian 'no rewriting history' policy, request revisions and commits are different (and incompatible) concepts. Trying to shoehorn the one into the other is likely to result in cases where the user experience is less than it would be if they were treated as being different.
> We'd have some intelligence to try to see if a patchset was just
> amended when updated next, or if part of it was replaced, or what.
> We'd still have a new patchset entry internally, but we'd take
> advantage of some of our diff data sharing to see what the common
> parts are. We can then indicate to the reviewer that commits were
> added, or changed, or where. That'd make it easier to see what
> commits actually need to be reviewed.
I still like my suggestion better; continue to upload a single patch (which would create a single request revision), but accept patches that are really 'git am' chains. Then detect these server-side and add a /second layer/ with the option to inspect individual patches.
This should still be able to take advantage of diff caching and similar. Detection of added/removed/reordered patches can then be done using the usual diff mechanisms on the complete patch files (some heuristics will be needed to interpret the results, but the actual comparison can use existing algorithms).
> Along with this, there'd be some other improvements. We'd have a field
> indicating where the development branch/repository is (so that people
> can pull from you, if it makes sense in their setup), and the
> branch/revision where the commit was made (as part of the submitted
> form, and also available to set via new RBTools commands and options).
I expect I will rehash the above paragraph, but... IMO, minimum requirements for decent DVCS support are two branch fields: one the merge target branch (i.e. 'master' most of the time), and the name of the head as pushed to the remote. Also a third field with the actual SHA of the request (I would prefer to have this in the existing changenum field; it seems to equate with what that field is intended to be).
I also wouldn't be worried about multiple repository support initially; even some public open source projects use single repositories with either branch-level commit access or just plain trusting people... and this is probably the more common case (in fact, I would expect not having a single canonical repository for code sharing to be rare) in corporate settings.
> On a related note, it would be a fantastic feature if we could also tie
> this into github pull requests. It looks like the github API[1] allows a
> client to register for notification of pull requests. It would be
> excellent to be able to automatically-generate a ReviewBoard review for
> the requested branch.
I wouldn't even try to do this in RB proper in the short term (if ever); it's too obviously excellent fodder for an extension or external tool... which means it can be developed in parallel without affecting RB's release schedule.
The only possible reason I can think of why it would ever need to be fully integrated is if it is not otherwise possible to provide reasonable security (i.e. if it would not be possible to secure access to the well-permissioned user that the tool would need to have to talk to RB).
> Similarly, it would be pretty excellent if we could tie in a button to
> push the Merge Button™, though that would necessitate adding a
> permission feature on ReviewBoard to set who was allowed to perform
> merges. We'd also probably want to implement a policy such as "The
> review must have at least N 'Ship It!' values before it can be merged."
For the record: this is one of the main reasons people give why RB is not an acceptable alternative to gerrit.
> The "at least N Ship Its!" is something that a lot of companies want,
> but not all want the same logic. Some want just a total, some want from
> certain groups, some from certain people, etc. I think we'd need
> something more general that could have an extension for defining such
> rules. I think it's separate work from the DVCS work.
This sounds like a good excuse to develop the extension system to allow an extension to implement the logic. Probably this just needs an internal field if a request can be merged that can be set by the extension.
#1 will absolutely happen for 1.8. It's in progress now.
One of our students is writing support for specifying a revision and branch a change was pushed as, after you close a review request. That should make 1.8.
Christian
On Mar 12, 2013, at 10:30, Matthew Woehlke <mwoehlke.fl...@gmail.com> wrote:
> Detailed comments below, but summarizing my personal short term priorities:
> 1. Custom field support.
> 2. Support DVCS revision in the changenum field.
> 3. Add a field for local branch name.
> Of course, (1) means that (3) (and (2), sort-of) can be done with an extension... which is one reason (1) is first, but also because we have local need for custom fields that are probably not useful to a wide enough audience to justify being built-in. This lack is the biggest pain point for us right now.
> I think pushing some things (especially patch set support) to 2.x sounds reasonable, but please, *please* at least have custom field support in 1.8 :-).
> (Besides, a number of the other features can be implemented to at least some extent with better extension support. This would be a good path for a wider developer base to work on them, and/or to 'try them out' conceptually before bringing them into RB core... if that is even valuable in the long term. It may end up that some features work so well as extensions that there is no strong reason they can't just stay extensions.)
> On 2013-03-12 08:52, Stephen Gallagher wrote:
>> [paraphrasing Christian:]
>> I absolutely want to introduce patchset support. My thought is that
>> post-review (well, rbt post, the successor coming in RBTools 0.5) would
>> allow you to optionally post a series of commits as one patchset,
>> instead of squashing them.
>> Review Board would be able to differentiate patchsets vs. standard
>> diffs. When a review request doesn't contain any patchsets, it'd be just
>> like it is today. However, when a patchset is involved, we change things
>> a little bit.
>> The revision selector is still there, but a patchset now counts as a
>> revision. The patchset will show each and every commit, along with their
>> metadata (descriptions and such). They could be viewed squashed (same
>> view as today), or per-commit.
> So... while I understand that this is technically more feasible, I'd be concerned with how it will handle e.g. rebases. Also, I think it will be much more logical for reviewers to keep commits within a request separate from revisions of the request, rather than mixing these concepts.
> Basically, unless you're talking about a project with a draconian 'no rewriting history' policy, request revisions and commits are different (and incompatible) concepts. Trying to shoehorn the one into the other is likely to result in cases where the user experience is less than it would be if they were treated as being different.
>> We'd have some intelligence to try to see if a patchset was just
>> amended when updated next, or if part of it was replaced, or what.
>> We'd still have a new patchset entry internally, but we'd take
>> advantage of some of our diff data sharing to see what the common
>> parts are. We can then indicate to the reviewer that commits were
>> added, or changed, or where. That'd make it easier to see what
>> commits actually need to be reviewed.
> I still like my suggestion better; continue to upload a single patch (which would create a single request revision), but accept patches that are really 'git am' chains. Then detect these server-side and add a /second layer/ with the option to inspect individual patches.
> This should still be able to take advantage of diff caching and similar. Detection of added/removed/reordered patches can then be done using the usual diff mechanisms on the complete patch files (some heuristics will be needed to interpret the results, but the actual comparison can use existing algorithms).
>> Along with this, there'd be some other improvements. We'd have a field
>> indicating where the development branch/repository is (so that people
>> can pull from you, if it makes sense in their setup), and the
>> branch/revision where the commit was made (as part of the submitted
>> form, and also available to set via new RBTools commands and options).
> I expect I will rehash the above paragraph, but... IMO, minimum requirements for decent DVCS support are two branch fields: one the merge target branch (i.e. 'master' most of the time), and the name of the head as pushed to the remote. Also a third field with the actual SHA of the request (I would prefer to have this in the existing changenum field; it seems to equate with what that field is intended to be).
> I also wouldn't be worried about multiple repository support initially; even some public open source projects use single repositories with either branch-level commit access or just plain trusting people... and this is probably the more common case (in fact, I would expect not having a single canonical repository for code sharing to be rare) in corporate settings.
>> On a related note, it would be a fantastic feature if we could also tie
>> this into github pull requests. It looks like the github API[1] allows a
>> client to register for notification of pull requests. It would be
>> excellent to be able to automatically-generate a ReviewBoard review for
>> the requested branch.
> I wouldn't even try to do this in RB proper in the short term (if ever); it's too obviously excellent fodder for an extension or external tool... which means it can be developed in parallel without affecting RB's release schedule.
> The only possible reason I can think of why it would ever need to be fully integrated is if it is not otherwise possible to provide reasonable security (i.e. if it would not be possible to secure access to the well-permissioned user that the tool would need to have to talk to RB).
>> Similarly, it would be pretty excellent if we could tie in a button to
>> push the Merge Button™, though that would necessitate adding a
>> permission feature on ReviewBoard to set who was allowed to perform
>> merges. We'd also probably want to implement a policy such as "The
>> review must have at least N 'Ship It!' values before it can be merged."
> For the record: this is one of the main reasons people give why RB is not an acceptable alternative to gerrit.
>> The "at least N Ship Its!" is something that a lot of companies want,
>> but not all want the same logic. Some want just a total, some want from
>> certain groups, some from certain people, etc. I think we'd need
>> something more general that could have an extension for defining such
>> rules. I think it's separate work from the DVCS work.
> This sounds like a good excuse to develop the extension system to allow an extension to implement the logic. Probably this just needs an internal field if a request can be merged that can be set by the extension.
> -- > Matthew
> -- > Want to help the Review Board project? Donate today at http://www.reviewboard.org/donate/ > Happy user? Let us know at http://www.reviewboard.org/users/ > -~----------~----~----~----~------~----~------~--~---
> To unsubscribe from this group, send email to reviewboard+unsubscribe@googlegroups.com
> For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en > --- You received this message because you are subscribed to the Google Groups "reviewboard" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to reviewboard+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
