-
-
Notifications
You must be signed in to change notification settings - Fork 247
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
Keybindings persistence #172
Conversation
0173006
to
3a72717
Compare
I squashed your commits from #169 because it makes the tree cleaner. Not sure how to resolve this right now...i never had to merge a PR that depended on on annother PR that got squashed 😅. Can you rebase to the current main branch? I also need to merge #159 first. Worst case you have to create a new PR for this. |
Hey! Ah yes, squashing. I remember this problem from work where we have the age-old discussion about "linear commit history" vs "safety while working with code". I had to work this one out completely when we migrated from SVN to GIT. Not sure if you are that well-versed with git, but the short version is: (please unfold if interested)... Not sure if you are that well-versed with git, but the short version is: Git really is not designed with linear commit histories in mind. It was built to implement workflows from kernel.org where patches fly left right and center and 3-5 different branches seem to get merged all the time and reintegrated into mainline. Since that workflow is all but linear, git does not implement linear workflows - and for a good reason: By encoding such complex merge trees it can keep track of changes in multiple branches in parallel and offer three-point merge features and other neat stuff without loosing any work, like we did in Subversion when someone would execute a faulty conflicting merge. Rebase and squash are "non-git" workflow additions that allow for linear workflows and - well squash gives a way to make more expressive, compressed histories. Both come with the cost of loosing work and with the cost of disrupting the complex merge tree by default. After a squash and after a rebase, branches will not directly integrate with each other anymore even if they originally might have a common ancestor in the commit-tree, because that ancestor might get deleted during a merge. At work, we decided to disable rebase&sqaush for one of our repos because of regular problems with this that were causing us to loose work... Anyway... Because of the squash, this branch lost its ancestor that linked it to the main-tree. Now there are a lot of conflicts because the three-point-merge algorithm fails to resolve the points where I resolved the conflicts before. I can fix that, I know how to do that, but it would be the easiest for me if I can do this once #159 is actually merged. Alternatively, we could decide now that we merge #159 through merging this PR. If you give me green-light for that solution, I will fix the commit tree doing a difficult rebase and then this is ready for merging. #159 would not be merged in that case, because it would be covered with the PR. |
Thanks for the explanation, very insightful. I got the squashing/rebase workflow from the Godot repo...normally i don't do this either, but it has it's advantages (cleaner tree, easier to revert changes etc). I can merge #159 first if it's easier. I assume squasing that PR would make things even more complicated, so should i just rebase and merge the 5 commits? Or a traditional merge commit? |
Your choice! One squash happened already, so I need to construct new patches anyway. Most likely, I will opt to rewrite the patches of this PR from scratch, which actually is quite easy because this PR mainly builds on top of the keybindings-PR (that one that is now squashed and merged) plus the config-file PR. So does not matter anymore :-)
Yeah, I know. From a management perspective it all becomes quite easy to follow when rebasing. And typically all works quite well as long as there is not too much parallel development like we had with the keybindings now. I will leave this open to you how you want to manage this project. The other way to manage this with rebasing is to negotiate with the contributors to a certain work-pattern that leads to compatible patches... 🤷 no idea what the better choice is wrt this 😆 |
Alright i merged #159 ;)
Yeah i have to think about that. For smaller self-contained PRs squashing should be fine. For bigger changes across multiple PRs like this one the traditional workflow might be better. We'll see |
3a72717
to
ab6ce2a
Compare
FYI: There only were two changesets that had to be rebased, so I did that manually using One problem though: The code comments got unlinked. Please check then in this thread and match them up with the appropriate locations in the actual code-base :-/ |
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.
Great work as always, thank you!
You deserve a special mention in the next release notes ;)
Thanks :-) Glad to help. I was planning on contributing some extra additions though, if you do not mind! :-) |
@MrApplejuice awesome, thank you so much. This is exactly what i wanted 🥇 |
Requires #169 and #159 to be merged first.
s.mp4
Changes:
store_shortcuts()
function to Settings which is called to persist keybindings