Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[Core] (Partially?) Fix data loss on dir rename (#4996)
* Fix lost filename in err msg In some circumstances, FileExceptions are constructed empty, then have a filename assigned to them, but the error message in these scenarios is left as the default "unknown" one, which is sometimes shown to users. This change fixes that case to be consistent with instances that are constructed with the filename. The exception can happen when trying to save the file in a location that does not exist, or when the user does not have permission to write there. If it comes when saving after closing the document, all previous changes can be lost. Partially fixes issue #4098. Co-authored-by: Heewa Barfchin <heewa.b@gmail.com>
- Loading branch information
1 parent
3ab5dad
commit 820e88f
Showing
4 changed files
with
104 additions
and
14 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
820e88f
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.
@chennes this commit should definitely have had all the messages from the squashed commits. Ideally, it should probably have stayed as separate commits for
App
,Gui
andBase
. I don't believe we can force onmaster
, right?820e88f
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.
My apologies --
I think it should be possible to edit the commit message, at any rate. The tree had gotten a little messy and I didn't want to pollute the trunk with "working" commits. I thought I'd gotten all the info necessary into the message,but if you can get me a better commit message I believe I can amend it.ETA - No, I see that I'm wrong here, I can't. I'm sorry for that, I'll try to be more careful going forward.
820e88f
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 believe the policy here should be to just dump in all the commit messages as they are, e.g. PR #4920. Too much information seems better than too little information.
820e88f
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 must acknowledge I'm also at fault, leaving caveats like "changes made in multiple modules" in the obscure task list instead of pro-actively making separate PRs.
Would it be worthwhile to push a revert and re-do the commits?
820e88f
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.
That seems reasonable -- for the foreseeable future I think I'll just work more closely with PR authors to get commits that I am comfortable committing as-is, rather than squashing them myself.
Regarding rolling it back and re-doing, I can't really make that call, only you can. Since what I was trying to avoid was a messy commit history, a rollback is counter to my goals, but if there's important information missing from the commit message, then we can do it.
820e88f
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 really don't know if it's worth the hassle redoing the whole thing just for cleanliness... That would be neither the first nor the last time this happens... I would let it be, and do our best next time :)