On Mon, Jul 7, 2008 at 1:56 PM, Massimo Del Fedele max@veneto.com wrote:
Because AppSearchReg does search in reg, where paths are un-slashed. I could have fixed more low-level, of course, but then you'd have big probability to break unrelated stuffs.
Listen, I don't want to come off as mean, but like you say yourself, you are not knowledgeable about msi or the Win32 API for that matter. AppSearchReg queries the specified registry entry for a directory to check for its existence. You say, "in reg, where paths are un-slashed." There is no such definition of paths in the registry. A registry value contains whatever value the user set it to.
Again, AppSearchReg MUST return path with backslash. About the other AppSearch, I don't know anything.
Ok, you've just verified that you're not thinking about this correctly. You should care about the other parts of AppSearch.
Those will remain unfixed. Also, why are you making it the responsibility of AppSearchReg to add the backslash?
Just look at Package.c and you'll know why... SHGetFolderPathW(NULL,CSIDL_STARTMENU,NULL,0,pth); strcatW(pth,cszbs); MSI_SetPropertyW(package, SMF, pth); as an example.
Again, what does that have to do with anything? This is the special MSI properties code...not the AppSearch action. You are trying to relate all these disparate parts that are completely unrelated.
You're assuming
that ACTION_SearchDirectory never changes the value sent in and only copies it straight back if it finds the path.
That's a bad
assumption.
Yep, right, I checked it on my case and it didn't rip off the backslash, but that could change... I should have done after it. Plus, if you fix this at the lowest level, you get a fix
for all the AppSearch functionalities for free.
The "lower level", looking at code, would be change RegQueryValueExW() behaviour : rc = RegQueryValueExW(key, valueName, NULL, ®Type, value, &sz); which indeed IS correct like ii is, and for that one I DID a testcase. That was my first "hack" on this, it worked but WAS WRONG.
Now knowing what I just told you about AppSearchReg above, you cannot possibly think that I was suggesting you should change RegQueryValueEx...
Going back one step...
Ok, we've had tons of patches submitted, some committed, that fix one app and break tons of other apps. This is exactly why this has to be backed up by a test.
My patch solves just one case that should indeed be solved. I don't say it's perfect solution, but that's the best I've found and I know it's correct.
By definition you can't know if it's correct unless you add tests for it, and even then you can never be 100% sure (but it's pretty close).
Maybe it doesn't fix the universe, but it does fix a bug, and just THAT bug, without fiddling with other stuffs.
...and maybe it breaks some other apps. You act like I don't think your fix is correct. It probably is correct, but that's not the point. It's not tested, so you can't be certain that it's correct. I already said I'd add a test, so I don't know why you keep going on about this.
Making it "more low level" as you told, i.e. changing RegQueryValueExW() would be wrong and would probably break tons of stuffs that expect it (correctly) to return a path WITHOUT a trailing backslash.
See above.
But maybe you wanted to say that is ACTION_SearchDirectory() that must be patched ?
Yes. I thought that was obvious, but I'll try to not make those assumptions again.
Maybe that's right. BUT then, as it's not an exported function, you should test ALL usages of it and see if that's a correct behaviour. Well, It's used just in AppSearchDr, besides of AppSearchReg(). Maybe, knowing what is for AppSearchDr() I could know if it's correct to have its result backslashed too... But I don't. Again, my skills on MSI are *very* limited. I've chosen to fix just something I know must be fixed instead of try to fix the world without knowing anything about it. Wrong ?
Yes. You're verifying everything I've told you several times.
BTW, thanx for the review.
No problem.