Merge request diff sets

This came up when talking with a user at OSCON. I think it's worthy of a discussion.

Problem

In larger merge requests it can become difficult to keep track of requested changes and potential new changes sneaking in between commits/pushes. For example, suppose I make a bunch of comments on a merge request asking the developer to change things. They come along and fix all of these things and then ask me to review the merge request again. It's not easy to see if they 'snuck' in other changes, whether maliciously or just because they saw something else that needed fixed.

Solution?

The user described a workflow: Once the reviewer makes comments during the first round they can 'mark' the set somehow. This indicates that any changes now should be shown as a diff between the reviewed code and the new changes. This enables the reviewer to easily step between review sessions and have no doubt as to what code changed.

I don't think we should change the default diff view in merge requests but maybe add this feature in another way that is useful and has a good UX?

• This is almost like looking at a diff between individual commits, which can be done today. However, this feature should also account for scenarios where a user rebases/squashes commits between pushes. In this case the diff should be between the original commit (no longer in this MR) and the new commit. It also allows easy view of changes in the case that multiple/many commits are pushed between reviews.

Added feature proposal label

cc/ @JobV @dzaporozhets

We see all the commits that are added in the activity timeline.

@Haydn Had a potential customer with 2000 seats ask for this.

You can see new changes in MR timeline as @sytses mentioned:

This is almost like looking at a diff between individual commits, which can be done today. However, this feature should also account for scenarios where a user rebases/squashes commits between pushes. In this case the diff should be between the original commit (no longer in this MR) and the new commit. It also allows easy view of changes in the case that multiple/many commits are pushed between reviews.

This would require us to persist commits that are pushed away / squashed by someone. That could be complex.

@dzaporozhets It think the essential line is: "a user rebases/squashes commits between pushes. In this case the diff should be between the original commit (no longer in this MR) and the new commit." (I propose to forget about the second request of review sets for now)

This is the rebase workflow that people with a linear history (many hardware organizations) use. It is supported by Gerrit.

Can we store the original commit of an MR? Is that still in the repo after a rebase and compact? If so we can add a link to 'view how this MR changed' which shows a diff between the original commit and the latest commit.

This would require us to persist commits that are pushed away / squashed by someone. That could be complex.

If a commit is squashed and force pushed the commit is gone entirely from the repo? In this case, we can skip that for now and come back to it later if it's necessary.

Maybe there is a way to show a diff between the latest push and the previous one. We can already do diffs between commits so this is really just a 'shortcut' making it easier, more intuitive. Gerrit requires each commit to be a separate review set, which is overkill.

@dblessing Good suggestion. This is also what the customer needs: "Diff between latest push and previous push is what is really needed."

@dzaporozhets what do you think about showing the diff between the latest and previous push if the latest push rebases the previous one?

User reports Diff between latest push and previous push is what is really needed.

@dzaporozhets Basically we can link to the commit as we normally do and add something at the end (after the description): rebases SHA1

@sytses @dblessing Ok lets go through technical basics first. There are 2 scenarios:

1. user push new commits
2. user does rebase

For scenario 1 you can just look at added commits in discussion timeline. Maybe it would be useful to put compare link if user pushed multiple commits.

For scenario 2 rebase is rewriting history since certain point. Usually it means that all commits in merge requests are rewritten based on new master. Diff look for common ancestor which is master branch for both old(before rebase) and new(after rebase) commit. That means that diff between MR commit before rebase and MR commit after rebase is SAME as MR diff except changes introduced in new master (which you would not care because they are already in master and you don't need to review it). So it makes no sense to implement something for rebase cases because it would almost identical to MR diff except you get extra confused by reviewing code that is already in master.

I will try to explain it with symbols:

master: A, A1
feature-before-rebase: A, B1
feature-after-rebase: A, A1, B2

MR diff before rebase: B1
MR diff after rebase: B2
MR diff between B1 and B2: A1 + B2.

I dont see practical use of diff between B1 and B2 because:

• A1 already in master. Makes no sense to review it.
• B2 diff is identical to what you see on MR diff tab.

Maybe it would be useful to put compare link if user pushed multiple commits.

I think this could be helpful.

Another customer asked about Gerrit integration in https://gitlab.zendesk.com/agent/tickets/12013

@Haydn please confirm with the customer if this is what they mean.

@JobV This sounds similar to https://gitlab.com/gitlab-org/gitlab-ce/issues/13570

Thanks @dblessing. We can close this one in favor of #13570 (closed).

Status changed to closed

mentioned in merge request !14785