Make the common rebase case easy

[jdm]

In my experience using Critic for Servo, the common case is that the rebased commits are just being shifted to a more recent base revision, and we could easily automate the process of rebasing the review by matching the commit messages against the old commits and finding the first one that is different. This would make the process a lot easier for our contributors - Critic is a constant pain point for any nontrivial first-time contributions from a newcomer.

[Manishearth]

The main problem with tracking rebases is that it's hard to match commits.

What we can do is allow rebases:

• which edit commits but not commit messages
• which introduce new commits
• which squash commits (not fixups -- in a squash, the commit messages are concatenated and can be detected, in a fixup, all commit messages).
• which fixup commits starting with fixup! with their previous commits.
• Rebases which fix merge conflicts

Within this subset of rebases, we can reliably track how the commits have changed since the commit messages are unchanged (or trackable in some form)

With this in mind, for cases where a commit is edited, instead of showing a merge view (which is confusing to review), we can instead show how the individual commit has changed (diff of a diff, effectively)

[mo]

When devs push directly to critic, the Fiddle&Tweak extension can be used to make pure "history rewrite" rebases (no actual changes, just changed commits msgs, squashes, squashed fixups etc) and conflict free "move rebases" (moving to a new upstream) super convenient. However, when critic reviews are tracking external branches (like in your case), it's more complicated as you point out. Internally at Opera we have both kinds of teams, but the folks that use reviews tracking external branches they never force push (overwrite) those branches instead they push a new version of the branch with a certain kind of suffix and (I think) we have some custom hooks that makes it work. However, that wouldn't work well with the standard github PR workflow. I'm personally not using the github integration, so fixing this is not at the top of my list atm, but having some kind of solution to this built into, not just the slightly customized critic instance that james runs, but to upstream critic would be great indeed.

[jgraham]

So FWIW I think that GH always supplies me with the base of a review. Using this information I think I can make the rebase situation better in the following steps:

1. Store the GH-supplied base with the pullrequests table in critic
2. When an update comes in, and the GH provided base doesn't match the current base, store the new base and use it as a suggestion in the "upstream" field in the rebase UI
3. When storing the new base, above, add a comment to the PR indicating how to manually rebase the review
4. Add logic in the branch tracker to match commits, as described here, so that if there is a non-ff update and we determine that doing a rebase is "safe" (i.e. the rebase is heuristically detected as pure-move or pure squash), it is performed automatically and branch tracking continues.

I am at step 2 currently. Step 3 is easy once I work out whether the current changes actually work. Step 4 is harder, particularly because it's unclear how to invoke the rebase logic from the branch tracker.

[jensl]

If the GH integration was implemented as an extension, you could've copied what the FiddleAndTweak extension does when it rebases a review, which is to disable the tracking, prepare the rebase, push (--force) directly to Critic's repository and then re-enable the tracking.

See https://github.com/jensl/FiddleAndTweak/blob/master/operationPerformRebase.js, lines 120-157.

But then again, that push can sometimes take a while to complete, so probably isn't something you want to do synchronously while handling the update notification from GH.

It shouldn't be too hard to make the branch tracker do this; you just need to inserting a row into the reviewrebases table and then push with --force. In theory, that row could be inserted somewhere else (i.e. the rebase could be prepared somewhere else) and then the branch tracker could just check if a rebase has been prepared, and if so push with --force.

You'd need to do something about https://github.com/jensl/critic/blob/master/src/index.py#L600 too, but you can just remove it. It's about completely refusing to rebase reviews via the branch tracker, and you want the opposite of that, after all.