First of all, of course I am not Alexandre.
But I have to say I think I agree with James on this, with regards to submitting just one patch with the correct spacing. I can see how seeing one patch without any spacing changes keeps the diff very simple (I am assuming this is why you submitted your patches in this way), but at the same time I would assume that if Alexandre really wants to see what changed in this way after applying the one patch you will send with correct spacing, he can just type:
git-diff -b
and see something very similar to your patch #1 (ignores whitespace, same effect).
Other than that, I did not see anything obviously wrong with the patch upon a quick look. If you don't hear from anyone else on wine-devel, I would suggest resubmitting as one patch with correct indentation as James suggested. If you don't see it committed after a while (maybe a week or so, maybe longer) I would either get in touch with the list again or try to get in touch with Alexandre to see if he has any specific comments.
Hope that helps.
Misha
Also, there are no changes to tests that show that the uninstaller changes are adding/correcting behavior so it matches windows. Unit tests would make the change more compelling.
Chris
On 6/14/07, Misha Koshelev mk144210@bcm.edu wrote:
First of all, of course I am not Alexandre.
But I have to say I think I agree with James on this, with regards to submitting just one patch with the correct spacing. I can see how seeing one patch without any spacing changes keeps the diff very simple (I am assuming this is why you submitted your patches in this way), but at the same time I would assume that if Alexandre really wants to see what changed in this way after applying the one patch you will send with correct spacing, he can just type:
git-diff -b
and see something very similar to your patch #1 (ignores whitespace, same effect).
Other than that, I did not see anything obviously wrong with the patch upon a quick look. If you don't hear from anyone else on wine-devel, I would suggest resubmitting as one patch with correct indentation as James suggested. If you don't see it committed after a while (maybe a week or so, maybe longer) I would either get in touch with the list again or try to get in touch with Alexandre to see if he has any specific comments.
Hope that helps.
Misha
On Thu, 2007-06-14 at 10:50 -0400, Chris Morgan wrote:
On 6/14/07, Misha Koshelev mk144210@bcm.edu wrote:
First of all, of course I am not Alexandre.
But I have to say I think I agree with James on this, with regards to submitting just one patch with the correct spacing. I can see how seeing one patch without any spacing changes keeps the diff very simple (I am assuming this is why you submitted your patches in this way), but at the same time I would assume that if Alexandre really wants to see what changed in this way after applying the one patch you will send with correct spacing, he can just type:
git-diff -b
and see something very similar to your patch #1 (ignores whitespace, same effect).
Other than that, I did not see anything obviously wrong with the patch upon a quick look. If you don't hear from anyone else on wine-devel, I would suggest resubmitting as one patch with correct indentation as James suggested. If you don't see it committed after a while (maybe a week or so, maybe longer) I would either get in touch with the list again or try to get in touch with Alexandre to see if he has any specific comments.
Hope that helps.
Misha
Also, there are no changes to tests that show that the uninstaller changes are adding/correcting behavior so it matches windows. Unit tests would make the change more compelling.
Chris
Actually that brings up a good point, does anything ever get written to Software\Microsoft\Windows\CurrentVersion in HKEY_CURRENT_USER in windows? I have no such key on my (rather plain) XP install, and from what I can tell wine MSI never writes to this key (see dlls/msi/registry.c, function MSIREG_OpenUninstallKey line 378) in HKEY_CURRENT_USER.
Is this done by some specific application, or if not what was the original motivation for the patches? From what I can tell this was not in the emails you linked. Btw, I think it would be a good idea to include this kind of info when you submit the patches.
Misha
On 6/14/07, Misha Koshelev mk144210@bcm.edu wrote:
Actually that brings up a good point, does anything ever get written to Software\Microsoft\Windows\CurrentVersion in HKEY_CURRENT_USER in windows? I have no such key on my (rather plain) XP install, and from what I can tell wine MSI never writes to this key (see dlls/msi/registry.c, function MSIREG_OpenUninstallKey line 378) in HKEY_CURRENT_USER.
Is this done by some specific application, or if not what was the original motivation for the patches? From what I can tell this was not in the emails you linked. Btw, I think it would be a good idea to include this kind of info when you submit the patches.
The original try of the emails (I think towards the end of May), had it. There is a bug filed for it on bugzilla (search for IMVU). Some applications (IMVU instant messenger, for one) do use current user by default, and some installers ask where you want start menu entries and application settings stored, all users, or current user, where all users as far as the registry is concerned would be HKLM.. It was brought up in private after I sent the linked emails, that I should include that info in every try, so I will do so in the future.
As for the currentversion key not being there in your xp install, it may be just because it's a plain xp install. I use XP on a daily basis and frequently access the registry, and I've never noticed a time when it was not there with something under it, although I never checked immediately after a clean install.
On 6/14/07, Misha Koshelev <mk144210 at bcm.edu> wrote:
Actually that brings up a good point, does anything ever get written to Software\Microsoft\Windows\CurrentVersion in HKEY_CURRENT_USER in windows? I have no such key on my (rather plain) XP install, and from what I can tell wine MSI never writes to this key (see dlls/msi/registry.c, function MSIREG_OpenUninstallKey line 378) in HKEY_CURRENT_USER.
Is this done by some specific application, or if not what was the original motivation for the patches? From what I can tell this was not in the emails you linked. Btw, I think it would be a good idea to include this kind of info when you submit the patches.
The original try of the emails (I think towards the end of May), had it. There is a bug filed for it on bugzilla (search for IMVU). Some applications (IMVU instant messenger, for one) do use current user by default, and some installers ask where you want start menu entries and application settings stored, all users, or current user, where all users as far as the registry is concerned would be HKLM.. It was brought up in private after I sent the linked emails, that I should include that info in every try, so I will do so in the future.
As for the currentversion key not being there in your xp install, it may be just because it's a plain xp install. I use XP on a daily basis and frequently access the registry, and I've never noticed a time when it was not there with something under it, although I never checked immediately after a clean install.
Well in that case, my recommendation would be to include the bug # in your patch email with each try, and just to submit one patch with the proper spacing. And if you still don't see it committed, I would try to get in touch with Alexandre and see if he has specific comments.
As far as I can tell, there aren't any unit/conformance tests for the uninstaller program (or even anything in the programs folder period???) so I wouldn't think that should hold up its adoption...
Misha
-
Chris Morgan -
Misha Koshelev -
Tom Spear