Skip to content

Warn on shim overwrite#2033

Merged
r15ch13 merged 2 commits intoScoopInstaller:masterfrom
rasa:rasa-warn-on-overwrite
May 4, 2018
Merged

Warn on shim overwrite#2033
r15ch13 merged 2 commits intoScoopInstaller:masterfrom
rasa:rasa-warn-on-overwrite

Conversation

@rasa
Copy link
Copy Markdown
Member

@rasa rasa commented Feb 20, 2018

Tested and working for me

Comment thread 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 force-pushed the rasa-warn-on-overwrite branch from 4a607de to 79c2c16 Compare February 27, 2018 19:42
@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