src: fix erroneous fallthrough in ParseEncoding()

[bnoordhuis]

A missing 'break' statement unintentionally allowed "linary"
and "luffer" as alternatives for "binary" and "buffer".

Regression introduced in commit 54cc721 ("buffer: introduce latin1
encoding term".)

R=@trevnorris

CI: https://ci.nodejs.org/job/node-test-pull-request/2972/

[addaleax]

tiny nit: the trailing \ here is superfluous/inconsistent with the way line continuations are ended in the rest of the Makefile

[bnoordhuis]

I tend to write it like that because it keeps the diff small when you add lines later on. But you're right, the rest of the Makefile doesn't use that style.

[addaleax]

LGTM

[thefourtheye]

Shouldn't the if..else if...else ladder below be fixed? Right now it returns BINARY if the case-insensitive comparison is truthy

 } elseif (StringEqualNoCase(encoding, "latin1")) {
returnLATIN1;
} elseif (StringEqualNoCase(encoding, "binary")) {
returnBINARY; // HERE
} elseif (StringEqualNoCase(encoding, "buffer")) {
returnBUFFER;

I think it should be

 } elseif (StringEqualNoCase(encoding, "latin1")) {
returnLATIN1;
} elseif (StringEqualNoCase(encoding, "binary")) {
returnLATIN1; // HERE
} elseif (StringEqualNoCase(encoding, "buffer")) {
returnBUFFER;

[jasnell]

LGTM
@thefourtheye ... it likely could be but not sure it has to be.

[bnoordhuis]

I think @thefourtheye is on to something. It wouldn't matter except that BINARY and LATIN1 are different values.

@trevnorris Can you comment on whether this patch makes sense?

diff --git a/src/node.h b/src/node.h
index d813b45..427f45d 100644
--- a/src/node.h+++ b/src/node.h@@ -278,7 +278,7 @@ inline void NODE_SET_PROTOTYPE_METHOD(v8::Local<v8::FunctionTemplate> recv,
}
#define NODE_SET_PROTOTYPE_METHOD node::NODE_SET_PROTOTYPE_METHOD
-enum encoding {ASCII, UTF8, BASE64, UCS2, LATIN1, BINARY, HEX, BUFFER};+enum encoding {ASCII, UTF8, BASE64, UCS2, LATIN1, HEX, BUFFER, BINARY = LATIN1};
NODE_EXTERN enum encoding ParseEncoding(
v8::Isolate* isolate,
v8::Local<v8::Value> encoding_v,

[trevnorris]

@bnoordhuis I think that makes better sense. The two are functionally equivalent. Rest of the changes LGTM. (thanks for adding the native API tests)

[bnoordhuis]

Landed in d06820c...6b48324, thanks. I'm going to follow up with a separate PR for the BINARY/LATIN1 thing.

[evanlucas]

This depends on #7111 which is semver minor, so will hold out on this until the next v6.x minor release

[MylesBorins]

added don't land label for v4.x

@evanlucas did you want to leave the v6.x label on there ?

[evanlucas]

It actually depends on a semver-major change, so I believe that leaving the dont-land-on-v6.x is appropriate for now (unless something changes). Thanks!