search

facebook / sapling

A Scalable, User-Friendly Source Control System.
https://sapling-scm.com
GNU General Public License v2.0
6.13k stars 280 forks source link

ReviewStack showing incorrect diff when bottom PR is updated #650

Open alex-statsig opened 1 year ago

alex-statsig commented 1 year ago

Description of bug

Diffing breaks in reviewstack when only one PR in a stack is resubmitted

Steps to Reproduce

  1. Create a stack with "PR 1" and "PR 2" with any changes
  2. Rebase PR 1 onto a new base (updated main perhaps)
  3. sl pr submit on PR 1
  4. View PR 2 in review stack, observe that the changes from main are included (even though PR 1/2 don't touch those files).

Possible Solutions

  1. When any PR in a stack is updated, the whole stack should be resubmitted automatically (otherwise it leads to incorrect diff view)
  2. When showing the changes of PR 2 in ReviewStack, it should not be diffed against the most recent version of PR1, but rather the version it was submitted with

Fix 2 seems preferred given it doesn't require resubmitting the whole stack to change one piece, not sure on technical limitations.

quark-zju commented 1 year ago

@bolinfest Do you know if we can use the parent commit of PR 2 instead of PR 1 as "base" in this case?

alex-statsig commented 1 year ago

Another side effect of this that I think we ran into is that if PR 1/2 are accepted, and you merge PR 1 then merge PR 2 (or perhaps just merge PR 2?) you end up without the updates to PR 1. Maybe PR 2 should also not be allowed to merge if not updated with the changes below it?