Display two-dot diff variant in PR "Files changed"

[obfuscode]

Based off this discussion, a major shortcoming is that GitHub is showing the three-dot diff for PRs instead of the two-dot diff.

From Matt at GitHub Support:

We show what's called a "three-dot diff" which is the difference between the latest commit on the HEAD branch and the last common ancestor commit with the base branch. So if you forked off of a repo a while ago and things have changed in the parent repo since then, or if you opened a PR from the master branch and then the master branch has changed in the meantime, the diff wouldn't show that.

It sounds like you'd rather see a "two-dot diff", which would show the changes between the two most recent commits on each branch. If you'd like, you can merge the base branch back into the HEAD branch. That updates the last common ancestor and would be more likely to show you what you're expecting to see there.

Why this is a problem

• You cannot see the context of the change against the current base making it extremely difficult to judge if PR'd code should be included
• The merge itself uses a two-dot diff and will show merge conflicts even if everything in the PR looks good
• The only way to compare the actual current base vs. the incoming PR head is to pull down the PR's remote and manually compare it against your local head (even viewing it through any of the GUIs will show the ancestor commit)
• You cannot merge base back into a contributor's fork
• Even if you request them to update, subsequent PR merges throw it out of date again immediately
• This is a long-lived issue that GitHub most likely won't change

Proposed Solution
When viewing the "Files changed", if the PR references an ancestor commit, Refined GitHub adds a button to show the two-dot diff (current base diffed with PR head) and grabs the raw view of each file (within a reasonable limit) and replaces(?) the current view (or a tab/toggle) that shows the same lines as displayed in the PR's change.

Feature URL Location
https://github.com/YourOrg/SomeProject/pull/123/files

[fregante]

I’m not entirely sure I understand the difference yet, but I think:

• this isn’t doable unless GH has an API for this comparison. And even then we’d have to generate the DOM for each commit and remove/add them at the right place in the conversation, something that might not be that easy.
• the solution to this is to just click our existing “Update branch” button

Can you point to a real PR that shows the issue rather than a made-up one?

[obfuscode]

The biggest difference is that what you're comparing in the PR view is not the current code so basing the decision to merge code into the repo by comparing it here is inaccurate.

This was committed on Jan 2nd and adds the isset():
ExpressionEngine/ExpressionEngine@7df0f66

When reviewing a newer PR from the same person, I was confused because I recalled we had already fixed this:
ExpressionEngine/ExpressionEngine@e163df1

The PR shows the left panel from the point in time they forked the repo and is not accurate to what you're reviewing to merge, it just shows what they changed it from without regard to what you're about to merge it into.

This is a fairly innocuous example but it's happened a few times and extrapolated out for much larger commits on PRs with hundreds of files and it becomes a major issue (to me, perhaps I'm missing something most others seem to know?).

The "Update branch" button does not work in this situation because it's a contributor's fork that we don't have write access to. We can request them to update their branch, but as soon as you merge in other PRs (assuming they modify the same files as other PRs), then you're right back in the same situation, as we were with multiple contributors submitting PHP 7.4 updates.

I don't think the feature would need to actually do the diff itself, but just show the corresponding lines from the current file. The lines may not match up if the file has been drastically modified since the PR head, but if was scrollable or expandable like GH's is, that would be sufficient and/or it could be improved over time to be more accurate.

If the extension can fetch the raw versions of the files, as it's on the same domain, there shouldn't be a CORS issue, but off-hand I'm not sure what extension restrictions are for fetching content from same-domain urls.

I know this is a heavy feature and most likely violates your Rule 4 and I considered hacking something together to do this but figured it would benefit more people if it was in more suitable hands.

[fregante]

The "Update branch" button does not work in this situation because it's a contributor's fork that we don't have write access to

That's incorrect, or should be incorrect. By default, GitHub suggests giving write access to the branch — but they can also remove this access. That is bad practice and hostile to those you're sending a PR to. We have a feature that specifically suggests avoiding this.

just show the corresponding lines from the current file

That sounds pretty unsafe. If I removed 10 lines, the diff will completely off and will make even less sense.

I understand the problem, but this isn't something we can fix in a lightweight way, safely.

[fregante]

As they suggested:

If you'd like, you can merge the base branch back into the HEAD branch. That updates the last common ancestor and would be more likely to show you what you're expecting to see there.

This is what our "Update branch" does and that's why the requester should give give write access.

[obfuscode]

No problem, thank you for the discussion. I'll look for alternatives and/or tightening up our contributing guidelines. Closing this issue.