-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix #8106:fix open deleted file in git scm errors #8107
Conversation
@datou0412 Could you update the PR description please? |
@lmcbout could you try again please |
The 'Open File' action always opens the file as it exists in the working tree. IMO, the bug is that the 'Open File' action should not be there for a deleted file, since it does not exist and cannot be opened. So remove from both the context menu and the inline action. Double-clicking (or single-clicking depending on preferences) opens the diff. The diff for a deleted file should just show the contents that were deleted. This is technically a diff, even though one just sees one version of the file. The file contents should not be modifiable, and a diff URL should be used to open it, with (HEAD) or (Index) showing in the title. VS-Code, interestingly enough, has exactly the same bug. 'Open File' is there both in the context menu and inline action but it does nothing. Although we can't easily fix this when using the Git builtin (which I have not tested but I assume also does nothing), we can fix this in the Theia's git package. |
if (change.status === GitFileStatus.Deleted) { | ||
return changeUri.withScheme(GIT_RESOURCE_SCHEME).withQuery('HEAD'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this code is complete. Consider what happens if you modify a file, stage the file, then delete the file. You should see the file with 'M' status in the staged section and with a 'D' status in the unstaged section. (If one actually tries this out then you will see an incorrect 'M' in the unstaged section. That is because of this bug #7789). If you click anything in the unstaged section then you should see the diff between the staged and the unstaged. That means if you click on the deleted file in the unstaged section then you should see the contents of the file in the index, not the contents of the file in HEAD.
Until #7789 is fixed you won't be able to test all the cases here. So on that basis I would be happy with the changes currently in git-scm-provider with a TODO that more work is needed when #7789 is fixed. I think the suggestion will be correct in all cases, but I can't test it without fixing 7789.
if (change.status === GitFileStatus.Deleted) { | |
return changeUri.withScheme(GIT_RESOURCE_SCHEME).withQuery('HEAD'); | |
} | |
if (change.status === GitFileStatus.Deleted) { | |
if (change.staged) { | |
return changeUri.withScheme(GIT_RESOURCE_SCHEME).withQuery('HEAD'); | |
} else { | |
return changeUri.withScheme(GIT_RESOURCE_SCHEME); | |
} | |
} |
@lmcbout @akosyakov As I commented, I don't think we should be able to open a file that has been deleted. "Open File" opens what is in the file system. We should remove "Open File" for files that have been deleted but that is a different issue. Are you okay if we leave the code in git-contribution unchanged in this PR? So we just have the fix in git-scm-provider? @datou0412 Thanks for making the code changes and adding the TODO. There should be an explanation following the TODO, so others know that the TODO is to test the code. However, on further thought, it is probably better just to update #7789 to make sure this gets tested. So if you want to remove the TODO that is fine too. I just didn't want the testing of this code to get missed. |
@akosyakov it is fine with me |
I also noticed than the "Open File" does nothing in VSCODE. It is fine with me as well. We can always "Discard Changes" for a deleted file to bring it back to the workspace. So this PR solves what it is supposed to do |
@datou0412 so we can merge this if you 1) revert the changes in git-contribution and 2) either add a description after the TODO or perhaps better just remove the TODO. Sorry, I know this takes the commit almost back to where you started. |
Signed-off-by: 二凢 <[email protected]>
Signed-off-by: 二凢 [email protected]
What it does
Fixes: #8106
There is a bug when try to open deleted files in git scm, it causes errors and can not open anything.
For more detail, view the ISSUE #8106
How to test
Review checklist
Reminder for reviewers