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

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?

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(): https://github.com/ExpressionEngine/ExpressionEngine/commit/7df0f663ad40c463651b96fed2ba737594144701

When reviewing a newer PR from the same person, I was confused because I recalled we had already fixed this: https://github.com/ExpressionEngine/ExpressionEngine/pull/397/commits/e163df1a08343c2d54aa8337aca3535afc118dbc

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.

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.

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.

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