Re: Review Board, Git and patch-series
#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 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 comm
Re: Review Board, Git and patch-series
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 t
Re: Review Board, Git and patch-series
On 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? > 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 te
Re: Review Board, Git and patch-series
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.) Thoughts? -- 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+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en
Re: Review Board, Git and patch-series
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) topic-branch-review is pretty easy to use: topic-branch-review topic-branch base-branch [-r=reviewID --last-reviewed=first_diff] 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. I hope this information is useful. Thanks, Stephen -- 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+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en
Re: Review Board, Git and patch-series
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.) -- 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+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en
Re: Review Board, Git and patch-series
Hi Stephen, 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 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 requ
Re: Review Board, Git and patch-series
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). > > > > > -- 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+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en
Re: Review Board, Git and patch-series
+1 vote for patch-series feature. -- 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+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en
Re: Review Board, Git and patch-series
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). > -- 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+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en
Review Board, Git and patch-series
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). -- 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+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en
