sindresorhus/refined-github

Older/Newer buttons to follow renames broken #2330

karlhorky posted onGitHub

#1870 introduced the following of the file across renames (original issue #998), super awesome! šŸŽ‰

Unfortunately it's broken currently (link):

Screen Shot 2019-08-14 at 16 12 36

Screen Shot 2019-08-14 at 16 12 46

The problem appears to be that it is taking the commit from the current page instead of the commit before/after the rename.

cc @fregante


It still works on the original test file:

So I'm not sure of what's going on here

posted by fregante over 5 years ago

Oh weird! Yeah maybe because the path to the file changed...? And the old path no longer exists in master?

posted by karlhorky over 5 years ago

This works on the assumption that "History" pages for a file will always be available, whether the file still exists or not (as you can see in the original test link)

For some reason in this case it's not working, but if I replace master with the parent commit of the commit that renamed the file, it works:

https://github.com/microsoft/vscode/commits/df9b7778f262c2aaa0633d3545e9d46b045cfd31/extensions/markdown/syntaxes/markdown.tmLanguage.json

Paths or just filenames shouldn't make a difference because to git they're the same thing.

posted by fregante over 5 years ago

if I replace master with the parent commit of the commit that renamed the file, it works

šŸ‘ Yeah I noticed that too, that's what I meant in the description "instead of the commit before/after the rename".

I guess this is a potential solution for the problem? Link to the commits URL with the commit hash of the last / first commit in the list?

posted by karlhorky over 5 years ago

The problem is that if we change the link to point to the commit instead of master, the Newer button will never work.

If you visit https://github.com/microsoft/vscode/commits/df9b7778f262c2aaa0633d3545e9d46b045cfd31/extensions/markdown/syntaxes/markdown.tmLanguage.json you won't see any reference to whatever the "Older" page should point to.

This is not fixable unless we are able to follow the file's whole history and generate the navigation that way, which makes the feature rather complex.

posted by fregante over 5 years ago

Hm, right... I was thinking of getting the reference to the commit via the tilde operator at the end of the commit (https://github.com/microsoft/vscode/commits/df9b7778f262c2aaa0633d3545e9d46b045cfd31~/extensions/markdown-basics/syntaxes/markdown.tmLanguage.json), but that actually goes in the wrong direction - it gets the parent (and there's no way of knowing whether the "next commit" would involve this file either).

Wonder if there would be any easier way to grab the next URL from the API... Something like a "renames API" would be really useful right now! šŸ˜ƒ

posted by karlhorky over 5 years ago

If it cannot be done easily or right now, maybe the bug with just the Older button can be fixed for now, and a second bug opened for the Newer button...?

posted by karlhorky over 5 years ago

If it cannot be done easily or right now, maybe the bug with just the Older button can be fixed for now, and a second bug opened for the Newer button...?

That's the thing: the feature works correctly now in many cases, in both directions. If we fix the Older button in your case, the Newer button breaks for everyone.


A possible solution would be to preserve the branch information so that Older points to commits/$commit?branch=master and Newer can keep pointing to commits/$branch, even if in your case it might work.

This search parameter would have to be preserved across all navigation though

posted by fregante over 5 years ago

If we fix the Older button in your case, the Newer button breaks for everyone.

Ah I didn't understand that the fix would cause the other cases to break šŸ˜ž

The other solution sounds like it could be a start for an idea... is that ?branch=master parameter a GitHub feature already? šŸ˜® Never seen it before.

posted by karlhorky over 5 years ago

The original issue is no longer reproducible. If you find a bug, please open a new issue specific to it.

posted by fregante over 5 years ago

This looks good now, thanks for reporting back to this issue @fregante! If I notice anything else related, I'll open a new issue :)

posted by karlhorky over 5 years ago

Fund this Issue

$0.00
Funded

Pull requests