How are review diffs calculated?

I'm trying to evaluate Upsource and am noticing that diffs for reviews are often noisier than expected. Roughly speaking we follow the GitFlow workflow with master/develop/feature branches. It's pretty common for developers to back merge develop numerous times over the course of developing a feature to pro-actively avoid merge conflicts. I've noticed that for reviews of branches that have done this... files that were merged in from our develop branch, often but not always, show up in the Review diff. Previously we used Stash, which uses this approach to diffing: https://developer.atlassian.com/blog/2015/01/a-better-pull-request/. This was rock solid and the behavior I was expecting from Upsource. Can y'all explain to me why I would be seeing so many files unrelated to changes on a branch in a Review?

7 comments
Comment actions Permalink

Hi Cody,

It's an expected behavior and let me explain why Upsource thinks so:

Merge from the master or any other branch to a feature branch might have various consequences and in theory it's also a subject for review. While accepting some branch review you don't only accept changes that were made, but also current state of code base. If let say some changes were made to the same file in both branches (development branch and feature branch in our example) Upsource will try to identify if code changes in development branch are relevant to changes in feature branch and if not, changes from development branch wouldn't be shown in feature branch review. Complete removal of such changes from feature branch review might cause dramatic consequences.

Btw, what version are you using?

 

0
Comment actions Permalink

 

How does Upsource determine relevant changes from a merge? Initially I thought what you explained was the case. The first PR I noticed this happening on only showed files from the merge that had been modified on the PR branch as well. One of my other recent test cases was a VERY large PR for a refactor that had 100s of file changes. A number of the files that were showing up in the diff for the review were brand new files that had only been added on the develop branch. Why would these seemingly unrelated files show up in the review?

 

0
Comment actions Permalink

Cody,

There are some heuristics to determine if changes in the development branch are similar to the changes in the feature branch (e.g touch the same function, or touch nearest lines of the code and so on). How brand new files got to the the review is a good question. Perhaps there were some conflicts in the merge commit.. Do you see any conflicts in the merge commit regarding this brand new files?

0
Comment actions Permalink

Nope... no conflicts, these are new files. There are also changes showing up in files that were not modified by the feature branch. The best I can tell, all of the things... or almost all of the things, that were on develop are showing up in this PR diff.

0
Comment actions Permalink

 

0
Comment actions Permalink

Hey Guys,

"diff between null" is a definitely a bug. It is already fixed but corresponding product update not released yet..

Do you see this error on the same branch review we are speaking about?

0
Comment actions Permalink

Hi all,

just another question regarding the diffs in reviews. We often have large changesets that happens on the master. As @Artem Rokhin correctly said, these changes should also reviewed when a merge from a feature to a master branch is executed. But what's interesting in the first place on a merge commit are the resolved conflicts. To review resolved conflicts it's very cumversome when large changes are happened in the meantime. Is it possible to get a filtered view at the review page to get only these changes ?

Best 

Gerrit

0

Please sign in to leave a comment.