Do not call sprintf with one argument

[hekike]

PR https://cr.joyent.us/#/c/5299

Safe user inputs in messages

Addresses the issue when someone builds message manually and it contains user-input with formatting characters instead of using sprintf.

For example:
In this case VError throws without this change:

var myInput = "encoded%stext"
new VError(myInput + " invalid input")

Related to #57

Performance Improvement

Calling sprintf is expensive, do not call when zero arguments are passed.
In some scenarios, an application needs to create hundreds of VError(s).

To test the change's effect:

for(var i = 0; i < 10000; i++) {
        new mod_verror.VError('Foo')
}

On my 2017 MBP 3.5Ghz, I see a 27% performance improvement after the change.

Before:

[davepacheco]

Thanks for submitting this!

I really don't like the change in behavior regarding format specifiers like '%s' in a single-argument string. Similar ideas have been discussed extensively in #57, #8, and a few others. In particular, this question and the reply under it. I get that this is a rough edge, and I'm sorry for the pain it causes, but I find the alternative painful, too (inconsistent behavior regarding cases that are otherwise the same from a user's perspective).

#57 proposes an alternate constructor that would avoid this particular rough edge for people who want that. I think that's the way to go here.

[davepacheco]

As an example, I think it's pretty confusing that VError('%s') would produces a valid Error with message '%s' while VError('%s %s', 'one') would blow up.

[hekike]

@davepacheco thanks for the feedback, I picked up the work from #57 with adding the modifications you asked in the CL: https://cr.joyent.us/#/c/4394

New CL: https://cr.joyent.us/#/c/5360/

Please review.

[gabegorelick]

Will this be done soon?

[davepacheco]

@gabegorelick I did provide feedback on the change back in January but I believe the author went a different route. I just tried to pick it up myself and apply the feedback but there's a little more work left than I realized. The new change is here:

https://cr.joyent.us/#/c/6608/

The main difference from review 5360 is that I've refactored the code as I suggested in 5360.
Unfortunately, the test suite doesn't pass. I'm not sure if it wasn't run before or if my changes made it worse, but it looks to me like more work is needed to separate out the tests that can be run with PError from those that can't (because they test format strings). Since I haven't fixed that, I don't know if there are other regressions lurking.

I think this is the best starting point if somebody wants to pick up the test suite work. I'm not sure when I'll get to that again.

[trentm]

My history of verror work that led to the skipPrintf option to VError and the PError class.
(@davepacheco has an earlier history here.)

• @gabegorelick opened #57 and PR #58 This introduced the idea of a options.skipPrintf to VError and a PError convenience class that used that option.
• The PR was moved to Joyent's Gerrit CR https://cr.joyent.us/#/c/4394/ at the time (archived at https://github.com/joyent/gerrit-migration/tree/master/archive/4394 now that Joyent moved from Gerrit back to GitHub PRs for code review).
• @hekike opened #63 to solve a similar issue, but with a different approach that was simpler but backwards incompatible: https://cr.joyent.us/#/c/5299 (archived at https://github.com/joyent/gerrit-migration/tree/master/archive/5299)
• The original approach from #63 was rejected, but discussion stayed on issue #63.
• That CR for #57 stalled and was eventually abandoned in favour of discussion (and new code) at on issue #63.
• A new CR by @davepacheco, based on @gabegorelick's original patch was started: https://cr.joyent.us/#/c/6608/ (archived at https://github.com/joyent/gerrit-migration/tree/master/archive/6608). This stalled.
• Netflix created the https://github.com/Netflix/nerror fork. ("We hope in the future after the initial development period we can seek convergence between the two projects.") This @netflix/nerror added options.skipPrintf and PError based on the above work. This package remains compatible with current verror. See #75 for my comparison of the two packages.
• Joyent's node-verror repo moved back to GitHub PRs for code review, and CR 6608 was moved to #73

My current intent:

• I'm going to try to finish #73 to add PError and skipPrintf to verror in a way compatible with @netflix/nerror.
• Talk to the netflix guys about whether and what the future can/should be for verror and nerror.