chore: prevent nil dereferences on cmd handlers #8319
Conversation
Prevent nil panic dereferences on cmd handlers. Add a unit test to prevent future mistakes
| if h == nil { | ||
| // We should never have a nil handler, but if we do, do not | ||
| // wrap it. Wrapping it just hides a nil pointer dereference. | ||
| return |
There was a problem hiding this comment.
This explains what, but not why? I'm guessing it's the call to h below, but why don't we just default to h being func(inv *clibase.Invocation) error { return inv.Command.HelpHandler(inv) } in this case?
There was a problem hiding this comment.
@mafredri I was wondering if defaulting to the help command would be a good solution.
Atm we manually invoke the help on all cmds except this one. So if I made that change it would be a new behavior that is probably fine, but being explicit is also pretty easy. Now that I have a unit test that will fail if you add a new cmd without a help, I figured I'd keep it being manual.
I'll update the comment with more explanation as to why. The unit test failure has that language near it
Prevent nil panic dereferences on cmd handlers. Add a unit test to prevent future mistakes
Fixes: #8315