Fixing a bug where the Git Status is written multiple times

[paulmarsy]

The bug occurs when you Remove-Module posh-git and Import-Module posh-git

As the variable VcsPromptStatuses already exists in the global scope from a previous import additional Write-VcsStatus calls are added to the variable.

The change removes the variable when the module is unloaded (which is good practice anyway) while still allowing the variable to exist in the global scope to enable the same extensibility hook

I've also placed the variable name used into another variable as it is being reused a few times so if it needs to change it only needs to be changed in one place - it may also be a candidate to be refactored into a module exported variable rather than a global

[paulmarsy]

I've created two separate pull requests as they solve different bugs despite being in the same area of code, so not sure if you would want to accept none/one/both or they have different feedback and changes needed

[dahlbyk]

I definitely hadn't considered Write-VcsStatus behavior when the module is loaded and unloaded. Is there a particular use case where this is a pain point?

My hestitation with removing the global variable is that it will kill any other integrations that have been initialized (specifically posh-hg). My preference on unload would be to remove the specific $VcsPromptStatuses member that we registered, and perhaps then remove the variable only if it's now empty.

For removing the specific member, it seems like a closure over the added ScriptBlock might work? Or we could tack on a NoteProperty or something. What do you think?

[paulmarsy]

The issue happens because I encapsulate PowerShell customisations and tweaks into a custom module. When I make a change it's quicker to unload the module and reload it for testing rather than restart the PowerShell shell.
I load posh-git within my module, it isn't a problem for my use case as I currently manually reset the global variable when unloading Load Default Modules/Posh Git.ps1

Looking back at the change it does seem much more sensible to remove the specific entry in the array that was added rather than just removing the entire variable.. I hadn't thought about other modules that may depend upon it existing.

Though uncommon it feels correct to remove the posh-git scriptblock when the module is unloaded similar to avoiding a memory leak.

I'll submit an updated version shortly 😄

[paulmarsy]

I've simplified the change just down to the specific problem.. let me know if you'd prefer the change to be done in a new pull request to make the history cleaner.

I did consider initialising the global variable using ({@()}.Invoke()) so it is a collection rather than array to use the Add() and Remove() functions but as posh-hg shares the variable it feels safer to reassign the variable using the where-object to remove our specific scriptblock from the array

[dahlbyk]

Much simpler, nicely done.

let me know if you'd prefer the change to be done in a new pull request to make the history cleaner.

Up to you - feel free to clean up history in-place and just push --force to this same PR branch.

[paulmarsy]

I've rewritten history to clean it up and to remove the noise.. I didn't know you could update the history once pushed to GitHub, a good learning experience 😄

[dahlbyk]

You'll find differing opinions if you should, but you can guess where I stand.

Thanks!