Skip to content
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

Merged
merged 2 commits into from
May 4, 2018
Merged

Conversation

rasa
Copy link
Member

@rasa rasa commented Feb 20, 2018

Tested and working for me

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
Copy link
Contributor

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.

Copy link
Member Author

@rasa rasa Feb 26, 2018

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?

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

@rasa rasa added the review-needed Asks for review of these changes label Mar 12, 2018
@r15ch13 r15ch13 merged commit 0e2c253 into ScoopInstaller:master May 4, 2018
@rasa rasa deleted the rasa-warn-on-overwrite branch July 6, 2018 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-needed Asks for review of these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants