Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Older/Newer buttons to follow renames broken #2330

Closed
karlhorky opened this issue Aug 14, 2019 · 11 comments
Closed

Older/Newer buttons to follow renames broken #2330

karlhorky opened this issue Aug 14, 2019 · 11 comments

Comments

@karlhorky
Copy link
Contributor

karlhorky commented Aug 14, 2019

#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

@karlhorky karlhorky added the bug label Aug 14, 2019
@fregante
Copy link
Member

fregante commented Aug 14, 2019

It still works on the original test file:

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

@karlhorky
Copy link
Contributor Author

karlhorky commented Aug 14, 2019

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

@fregante
Copy link
Member

fregante commented Aug 14, 2019

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.

@karlhorky
Copy link
Contributor Author

karlhorky commented Aug 14, 2019

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?

@fregante
Copy link
Member

fregante commented Aug 14, 2019

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.

@karlhorky
Copy link
Contributor Author

karlhorky commented Aug 14, 2019

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! 馃槂

@karlhorky
Copy link
Contributor Author

karlhorky commented Aug 14, 2019

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...?

@fregante
Copy link
Member

fregante commented Aug 15, 2019

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

@karlhorky
Copy link
Contributor Author

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.

@fregante
Copy link
Member

fregante commented Dec 6, 2019

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

@fregante fregante closed this as completed Dec 6, 2019
@karlhorky
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants