Warn on shim overwrite#2033
Conversation
| } | ||
| $filename = [System.IO.Path]::GetFileName($path) | ||
| warn "Overwriting shim to $filename installed from $shim_app" | ||
| $warned_on_overwrite[$shim] = $shim_ps1 |
There was a problem hiding this comment.
Not sure what the warning flags were meant to do, but at the moment, since this function is only called once for any given shim name, it's not doing anything.
There was a problem hiding this comment.
I appreciate the feedback. The warnings are to inform the user when a program overwrites another program’s shim, as the two have the same name. My apologies if that wasn’t clear. I’m not sure what you mean by “it’s not doing anything,” I’ve tested it, and it’s reports when a shim is overwritten. Are you suggesting it report if the overwritten shim is then overwritten by a subsequent app?
There was a problem hiding this comment.
Sorry, I myself wasn't clear. I'm not talking about the warning feature itself (which works and is a good feature!)
I'm talking about where you set up a hashtable:
$warned_on_overwrite = @{}And later check if it already contains a shim:
if ($warned_on_overwrite.ContainsKey($shim)) {
return
}And later set a flag for this shim:
$warned_on_overwrite[$shim] = $shim_ps1This is the code that, as far as I can tell, isn't doing anything, since the function is only run once for any $shim. $warned_on_overwrite.ContainsKey($shim) will always be false.* What is this code supposed to be preventing?
*Technically a manifest could contain two shims with the same name; there's no code preventing it. But that's a different problem that should be handled elsewehere.
There was a problem hiding this comment.
I've just realized, that you might be thinking that the shim function gets called once for each individual shim file (.exe, .ps1, .cmd...)? If so then the code would make sense. But it isn't: it's only called once for each named shim (eg shim1) and all files (shim1.ps1, shim1.exe ...) are created in the one method call.
There was a problem hiding this comment.
You're right, that's leftover code that should be stripped out. Thanks for catching it. I will update the PR.
There was a problem hiding this comment.
OK, I removed the dead code. It should be good to go now. Thanks again.
4a607de to
79c2c16
Compare
Tested and working for me