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



Christian:
> 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).

Yeah, we're going to have to play around with it, figure out what makes
sense.


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

We've discussed this a couple times. There's issues to work out here.
For instance, what happens if someone submits a pull request and doesn't
have a RB account? How does that work exactly? Maybe we'd need a "Log in
with GitHub" ability for it, but now we're adding new auth backend
infrastructure and possibly account linkage to the list of tasks.

There's a few integration things there we'd need to solve...

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

I have some notes in a notebook somewhere about such a button. I think
it'd be interesting to add. Again, lots of new bits to add to make that
happen.

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.


There's no way the DVCS work will happen for 1.8 anyway. It's going to
be a 2.0 feature. 1.8 has a narrower scope and we want to keep it that
way so it doesn't turn into a year-long release.

-- 
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
--- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.


Reply via email to