Fix staged file renamed with changes showing no diff #17364
Conversation
- Closes desktop#6014 - If a file is renamed with changes and is staged, both the rename and differences will be shown with this update. - If a file is renamed is staged, but not all changes, a warning message is shown that the file must be staged. Otherwise, I would have to choose between showing the diff for the staged or unstaged changes, but not both.
There was a problem hiding this comment.
I am a bit torn about this. The concept of "staged files" doesn't really exist on Desktop, but I also like that Desktop does a best effort trying to get the diff of changed files in the working directory, regardless of their state (staged or not).
I think Desktop should always show the diff between whatever is in the repository right now (staged or not) vs the last commit. If a file has staged changes and unstaged changes, the diff should be display with respect to the latter.
| if (file.status.hasUnstagedModifications) { | ||
| args.push('--', 'HEAD:' + file.status.oldPath, file.path) |
There was a problem hiding this comment.
Does this make sense given https://github.com/desktop/desktop/pull/17364/files?diff=split&w=1#diff-93a97971dea4186b7445f010cdada081f05aeefd49b7b8900109e403d7b3a8ddR225 ?
I mean, it looks like with your changes, the app will never display the diff for a renamed file when there are unstaged changes.
There was a problem hiding this comment.
I struggled with figuring out the exact right thing to use here. I could get it to work if the file was moved with no changes and if a file was moved with changes and they were all staged. What I couldn't figure out was how to show a diff of a moved file (staged) with unstaged modified. So that's why I added that message. I figured it is better to know that the file was moved and has changes (even if Desktop doesn't display them) than have Desktop say it was moved with no changes when it actually had changed.
Do you have any suggestions on how I could figure out the diff for a renamed file when there are unstaged changes? I didn't want to automatically stage the changes. That would work, but if you're in that situation, you probably have manually staged files outside of Desktop, so you probably don't want Desktop staging additional changes.
There was a problem hiding this comment.
Thanks for reviewing, @sergiou87.
am a bit torn about this. The concept of "staged files" doesn't really exist on Desktop, but I also like that Desktop does a best effort trying to get the diff of changed files in the working directory, regardless of their state (staged or not).
For some context, I don't really use the full features of Desktop. I prefer to do git operations (mv, add, rm, commit, etc.) from the command line. However, I like to view the diffs in Desktop because it's so much easier seeing the changes there.
I think Desktop should always show the diff between whatever is in the repository right now (staged or not) vs the last commit. If a file has staged changes and unstaged changes, the diff should be display with respect to the latter.
I agree with that with the exception of moved files. If I remember correctly, if you move a file outside of git, Desktop will show one file deleted and the other added. If you do a git mv instead, Desktop will show that it was moved. The one missing piece for me is that if you do a git mv, but also change it, I think it should show that the file was moved and that some changes were made. In a way, that's still showing the diff between the current state of the repo and the last commit.
| if (file.status.hasUnstagedModifications) { | ||
| args.push('--', 'HEAD:' + file.status.oldPath, file.path) |
There was a problem hiding this comment.
I struggled with figuring out the exact right thing to use here. I could get it to work if the file was moved with no changes and if a file was moved with changes and they were all staged. What I couldn't figure out was how to show a diff of a moved file (staged) with unstaged modified. So that's why I added that message. I figured it is better to know that the file was moved and has changes (even if Desktop doesn't display them) than have Desktop say it was moved with no changes when it actually had changed.
Do you have any suggestions on how I could figure out the diff for a renamed file when there are unstaged changes? I didn't want to automatically stage the changes. That would work, but if you're in that situation, you probably have manually staged files outside of Desktop, so you probably don't want Desktop staging additional changes.
|
Hey @ssigwart! After discussing it with the team, we’re slightly concerned that making this change could introduce unexpected regressions in other scenarios that, unlike the staging area, are well supported by Desktop. Given we haven’t received many reports or complaints around this, we have decided to not include your changes in the app. We’re very sorry about the time you’ve spent on this. |
|
Thanks, @sergiou87. Should #6014 be closed then? Also, if I write a change request would at least detect that the old and new files differ and update the message so it says there may be changes to the renamed file, but doesn't actually show them, do you think that might be merged? I feel like I can do a change like that without causing regressions. |
That's a very reasonable question. I thought about that, but decided not to close it in case someone came up with another way of fixing the problem. If you can think of another way of detecting renamed/moved files, that'd be great. However, I think even git uses heuristics to detect those cases in the staging area. If you have an idea, feel free to share it in #6014 before implementing it (unless you wanna test it out directly). We just don't want people to work hard on stuff before even knowing if it could be accepted 😓 Although we definitely appreciate the effort! ❤️ |
|
Thanks, @sergiou87. I'll see if I can find a way to detect that. |
Closes #6014
Description
Screenshots
Now, a renamed files with changes will show this if there are no other unstaged changes:
If there are other unstaged changes though, it will show this:
Release notes
Notes: Fixes issue where renamed files with changes are reported as having no changes.