Page MenuHomePhabricator

Allow users to view changes in merge commits relative to any ancestor, or as conflict resolutions only
Open, Needs TriagePublic

Description

My team frequenty uses a feature branch workflow, where we develop features incrementally on a separate branch and then merge them into master. Individual changes are reviewed as they are committed to the feature branch. The problem happens when the changes are merged into master, generating a merge commit, which is then automatically audited, genenrating a (usually large) audit containing already-reviewed changes. Some more details below:

Given a package with automatic audit enabled it is fairly easy to get in a situation users are forced to re-audit changes during true merges.
Consider the following situation, involving a master branch and a feature branch. Assume all commits mentioned affect some package which has auto-audit enabled.

 feature: A - B - C 
                   \
master:   ... - D - E

Now, if commits A, B, C, all have audits, and commit E is a simple merge commit that merges those changes into master, then triggering an audit on E basically presents cumulative changes from A, B, C combined for audit again, even though they've already been audited individually. This is undesirable for a couple of reasons:

  • the merge commit is usually large since it's a sum changes of all merged commits
  • if a significant amount of time passes between individual changes being made and the merge, the auditor may spend a significant amount of time before even realizing they've already looked at the changes.
  • it may hide actual real changes introduced in the merge (such as to resolve merge conflicts) that may be very valuable to review.

I would propose that the audit of merge commits specifically (commits with multiple parents commits) is generated slightly differently from the normal commit. Phabricator already detects merged commits for every merge commit and lists them in the commit/audit UI. If all of the commits being merged already have open audits, then the content of the commit for auditing can be presented as just the diff between the last merged commit and the merge commit itself (which would amount to the changes made during the merge, like conflict resolution). If those changes are empty (likely a majority of cases), the audit can be skipped. If not all of the commits being merged have audits (for example because they come from an untracked branch), then the merge commit would reflect the entirety of the changes.

Event Timeline

As an adversarial actor, I can use the proposed rule to avoid audit of dangerous changes like this:

  • Make changes which are safe in one branch but unsafe in a different branch.
  • Have them audited in the safe branch.
  • Merge them silently into the unsafe branch.

Although this might sound a little far-fetched, I think it's probably quite easy to construct these kinds of changes.

This also seems a little hard to believe to me, at least in terms of being a general flaw with this workflow:

the auditor may spend a significant amount of time before even realizing they've already looked at the changes.

Surely a moderately careful reviewer can read the commit message and see that this is a merge?

I think we can find better attacks on this that don't require us to ignore the commit. For example:

  • Provide a way to toggle between "all merged changes" and "just the changes in the merge itself [conflict resolutions, etc]".
  • In the "all the merged changes" view show audit status next to the commits (T6024) as a hint that they've been audited.
    • If this proves insufficient, we could show an explicit cue like "THIS IS A MERGE COMMIT. YOU HAVE ALREADY AUDITED 9 OF THE 15 COMMITS IT MERGES!" but hopefully we don't need to go that far.

This also seems a little hard to believe to me, at least in terms of being a general flaw with this workflow:

the auditor may spend a significant amount of time before even realizing they've already looked at the changes.

You're not wrong ;). I'm mostly referring here to a scenario where a significant enough time has passed where the auditor might not remember the individual commits (months), so the list of merged commits doesn't provide too much value. The user then would have to click on each one to see if they reviewed them already, and then still try to figure out if htere are any additional changes.

I think this whole request can basically be distilled to this, especially if the audit status of each merged commit is available inline:

  • Provide a way to toggle between "all merged changes" and "just the changes in the merge itself [conflict resolutions, etc]"

I think the big trick on this is going to be making inline comments work somewhat-correctly in the extreme case of octopus merges.

epriestley renamed this task from Audits of true merge commits may present duplicate changes in potentially large audit requests to Allow users to view changes in merge commits relative to any ancestor, or as conflict resolutions only.Jan 19 2017, 4:52 PM
epriestley moved this task from Backlog to Future on the Audit board.