-
Notifications
You must be signed in to change notification settings - Fork 628
Add check over Windows Integration to already exists during upgrade #640
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
Conversation
It is my suggestion for fising Bug 5683 I opened months ago. git-for-windows/git#5683 It will enable preserving Windows Integration in any context of update - manually (it will check the option automatically if detected) or via silent update (winget or other path) by detecting if the Registry entries already exists and match the current installation at that time. Signed-off-by: Nicolas DENIS <[email protected]>
Confirmed success. Downloaded the artifact from the generated package by the Pull Request automatic built and apply to my laptop using the same command line used to test internally - silent update. (from version 2.50.0) |
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.
Add check over Windows Integration to already exists during upgrade
Even though I tried for several minutes to parse this message, and I am still failing. What is this supposed to tell the reader?
It is my suggestion for fising Bug 5683 I opened months ago. git-for-windows/git#5683
This might help you understand what this commit is about. Leaving everybody else puzzled. So please follow the guidance in https://github.blog/2022-06-30-write-better-commits-build-better-projects/ to improve it, in particular with a strong focus on this part:
What you’re doing | Why you’re doing it | |
---|---|---|
High-level (strategic) | Intent (what does this accomplish?) | Context (why does the code do what it does now?) |
Low-level (tactical) | Implementation (what did you do to accomplish your goal?) | Justification (why is this change being made?) |
It will enable preserving Windows Integration in any context of update - manually (it will check the option automatically if detected) or via silent update (winget or other path) by detecting if the Registry entries already exists and match the current installation at that time.
Again, I have a really tough time trying to understand what you are saying.
Signed-off-by: Nicolas DENIS [email protected]
If you don't want to provide your real email address, it is not a real Signed-off-by: footer. The purpose of including the email address in the DCO is so that you can be contacted in case there are questions about your changes.
Apart from the commit message, it strikes me as a work-around that addresses one tiny fraction of the problem that winget
updates fail to preserve the user's choices. Two of those choices, to be precise. All the other user choices are missed.
Maybe a more fruitful approach would be to find out why the registry setting Inno Setup: Selected Components
in Computer\HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall\Git_is1
is not respected.
// Git Bash Here | ||
if RegQueryStringValue(RootKey, | ||
'SOFTWARE\Classes\Directory\shell\git_shell\command', '', Cmd) then | ||
begin | ||
if Pos(AppPath, Cmd) > 0 then | ||
WizardSelectComponents('ext\shellhere'); | ||
end; | ||
// Git GUI Here | ||
if RegQueryStringValue(RootKey, | ||
'SOFTWARE\Classes\Directory\shell\git_gui\command', '', Cmd) then | ||
begin | ||
if Pos(AppPath, Cmd) > 0 then | ||
WizardSelectComponents('ext\guihere'); | ||
end; |
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.
While this might address your problem, it now opens up Git for Windows to more complaints along the lines "I upgraded via winget
, and the Explorer integration was kept but all my other custom configuration is gone!".
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 apologizes - I am not usually doing public code fixes like that. So I am surely not aware of every details of the process. Will try to do better next time.
That said - I believe you did get exactly my intention - to solve the issue of Windows Integration being lost as part of update - regardless of the path we elect to do it (winget is among them, but it in fact apply to any silent, automated update process at the end).
But I do agree also that my fix is - indeed - insufficient. I did focus on my own issue - but not the whole picture.
If I dig a bit, it could be linked to the missing UsePreviousSetupType directive in the [Setup] section to tell the setup to actually reuse the component. I will give it a try as soon as I can.
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 will give it a try as soon as I can.
That sounds great!
Cancelled and replace with Pull Request #641 |
It is my suggestion for fixing Bug#5683 I opened months ago. git-for-windows/git#5683
It will enable preserving Windows Integration in any context of update - manually (it will check the option automatically if detected) or via silent update (winget or other path) by detecting if the Registry entries already exists and match the current installation at that time.