| Message ID | cb8683837c9f583274780057621255a65a1c4c9f.1592934880.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pkt-line: war on magical `4` literal | expand |
On Tue, Jun 23, 2020 at 01:55:32PM -0400, Denton Liu wrote: > When we are dealing with a packet-line length header, we use the magic > literal `4`, representing the length of "0000" and "0001", the packet > line length headers. Use `strlen("000x")` so that we do not have to use > the magic literal. I'm not a huge fan of using strlen() for this, because it's _still_ magical (you cannot change "0000" in one place without changing it in another"). And while it helps with understanding that "4" matches the length of that string, IMHO it's harder to read because now I have to make sure that those much longer strings all match up. This refactoring also implies to me that if you changed all of "0000" on one line you'd be fine, but that's emphatically not true. The magic number "4" is used to size the buffer earlier in the function, and would have to match (and of course since this is a network protocol, it's not like you could even change those in isolation). So I dunno. I kind of think the raw "4" is the most readable. It's quite obvious to me in the context of a memcpy() what's going on. I don't mind memcpy_literal() or similar that hides the repetition, but I think it's hard to do here because of the arithmetic on the destination. -Peff
Jeff King <peff@peff.net> writes: > On Tue, Jun 23, 2020 at 01:55:32PM -0400, Denton Liu wrote: > >> When we are dealing with a packet-line length header, we use the magic >> literal `4`, representing the length of "0000" and "0001", the packet >> line length headers. Use `strlen("000x")` so that we do not have to use >> the magic literal. > > I'm not a huge fan of using strlen() for this, because it's _still_ > magical (you cannot change "0000" in one place without changing it in > another"). And while it helps with understanding that "4" matches the > length of that string, IMHO it's harder to read because now I have to > make sure that those much longer strings all match up. Yup. There are two instances of recurring pattern with two memcpy, where three copies of the same string must appear. Unless the whole thing is abstracted away so that these two instances are calls to a macro/function that takes a single "0000" (and "0001"), I do not think it is an improvement. > This refactoring also implies to me that if you changed all of "0000" on > one line you'd be fine, but that's emphatically not true. The magic > number "4" is used to size the buffer earlier in the function, and would > have to match (and of course since this is a network protocol, it's not > like you could even change those in isolation). Thanks. That's even more important point. > So I dunno. I kind of think the raw "4" is the most readable. It's quite > obvious to me in the context of a memcpy() what's going on. I don't mind > memcpy_literal() or similar that hides the repetition, but I think it's > hard to do here because of the arithmetic on the destination. > > -Peff
diff --git a/remote-curl.c b/remote-curl.c index 75532a8bae..a2cbef546b 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -596,10 +596,10 @@ static int rpc_read_from_out(struct rpc_state *rpc, int options, set_packet_header(buf - 4, *appended); break; case PACKET_READ_DELIM: - memcpy(buf - 4, "0001", 4); + memcpy(buf - strlen("0001"), "0001", strlen("0001")); break; case PACKET_READ_FLUSH: - memcpy(buf - 4, "0000", 4); + memcpy(buf - strlen("0000"), "0000", strlen("0000")); break; case PACKET_READ_RESPONSE_END: die(_("remote server sent stateless separator")); @@ -804,7 +804,7 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results) curl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url); curl_easy_setopt(slot->curl, CURLOPT_ENCODING, NULL); curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, "0000"); - curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, 4); + curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, strlen("0000")); curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers); curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer); curl_easy_setopt(slot->curl, CURLOPT_FILE, &buf);
When we are dealing with a packet-line length header, we use the magic literal `4`, representing the length of "0000" and "0001", the packet line length headers. Use `strlen("000x")` so that we do not have to use the magic literal. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- remote-curl.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)