-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Warn on shim overwrite #2033
Warn on shim overwrite #2033
Conversation
lib/core.ps1
Outdated
} | ||
$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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_ps1
This 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I removed the dead code. It should be good to go now. Thanks again.
4a607de
to
79c2c16
Compare
Tested and working for me