Code review per Branch compared with another Branch
Currently Plastic allows Code Review Per Branch. Problem is it shows differences between the original node from which the feature branch was created (in dev) and the head node of the feature branch.
However, this is a problem: when you update your feature branch merging updates other devs have merged in dev branch.
Why? Because all their changes also appear in the code review. So at this point is not possible to do a reasonable code review of the code in the feature branch.
The solution: Take the same approach that BitBucket takes. The code review should show only differences between the head of the dev branch and the head of the feature branch. In that way merges won't appear as differences for review.
So for create a new review for a branch Plastic Client should ask the user the branch it has to use compare to show differences for the review.
Thinking it twice maybe you don't need to ask, simply you can take the head of the branch from which the feature branch was created. Or maybe leave it open to choose wich branch you want to compare with and by default use the head of the branch from which the feature branch was created.
-
Anders Rostgaard commented
Just adding my agreement with Juan and Sergio here. It's cool that Plastic can show me the history of the branch and how things were merged into it and all that, but in almost every case, as a code review, I just don't care about it.
Just let me see the differences between the head of the branch I want to merge into and the current branch, thanks.
-
Juan commented
The feature request is quite old, but answering psantosl, I still think I would like to see just the changes introduced when merging the featured branch with the head of the target branch.
If the diff is displaying already all the history/merge/etc, I suggest we get a button to 'hide' all those changes, because when in the original branch there have been a lot of changes, you get all those changes within the specific branch changes. -
Sergio Navarro commented
Furthermore, plastic fails some times to detect merged changes and mark them as branch changes when indeed they are merged changes.
-
Sergio Navarro commented
In summary only because you can you shouldn't show all the info you have to the user. It is more helpful to the user you give the relevant info he needs for the task it is going to do and add options to show the detailed info only when he needs it because needs further investigation of a change inthe code.
-
Sergio Navarro commented
Sorry for the late answer I haven't found a moment till now to answer.
I didn't said I want to compare the branch head with the master head because as you said "Comparing the branch head to the master head will only work if all your merges come from 'main'",
What I want isto compare by default with the head of the parent branch (this is not the same that always compare with main). Futhermore, I would prefer leave it as a default option but leave it open to allow the user who asks the revision to set to which branch it has to be compared (it would be the one on which he wants to merge the his branch).I totally agree with Pablo Sanchez I don't need info of what changes come from a merge. At least I would like to have the option of hide this information. In order to don't see lines merged in a different color and to skip also this lines when moving to the next and previous change through the buttons in the IDE.
Our case are real circumstances, all the team is doing code review. In fact, their complains are not only related with the fact you have to see merged changes together with changes to review when they happen in the same file.
More complains start every time a code review has more than one iteration of question answer with changes in code between reviewer and author of code. While this is a piece of cake when you use Bitbucket (maybe because it cannot do it better it shows you just what you are more interested in). Plastic instead shows you the previous code. Yes I know you have a link to see the new code but it is not clean, it breaks the focus of the reviewer. If you need another iteration of answering-modification of code-review then review turns even more strange. You need to be thinking every time what code are you viewing (the old one, the first fix after first review?, the last one?)
You simply need the code in the heads of the two branches to merge in the screen, if you don't have both in screen most likely is you need extra clicks to continue with your review -
Pablo Sanchez commented
At a branch level diff, I like how plastic shows you de info about de commits or the entire branch, you have all the information about de files changed, merge ( only changes on source) merged with changes in your branch, you have all, that's great. But I think this is only useful on this scenario, when you want information about the branch.
I think a pull request ( code review) is a totally different scenario, As a code reviewer, I don't want to know details about the history of the branch, I just don't care, I want to know how the changes on this branch can affect to the branch from this one(the branch under review) began, info about merged files, that wasn't modified on the reviewed branch are just noise.
In addition, when you work on a modified code review, the code that's appears is the old one ( I think should be the new one, again I don't want the history, I want to see the actual state ( with an option to see the previous comments of this revision) -
psantosl commented
Wow, wow. Comparing the branch head to the master head will only work if all your merges come from "main", which I doubt will be the case under real circumstances.
BitBucket (the mercurial part or the git part??) probably does that because as a visual layer on top of a version control they can't do any better. But we do: have you checked this?? http://codicesoftware.blogspot.com/2015/02/using-history-to-better-explain-branch.html. This probably solves all your problems and is simply much more powerful than any other alternative available :-O
pablo