{"thread":{"id":"53670","subject":"[PATCH] pkt-line: extract out PACKET_HEADER_SIZE","startedAt":"2020-06-13T13:43:50Z","lastAt":"2020-06-23T19:39:46Z","messageCount":22,"participants":["Denton Liu","Đoàn Trần Công Danh","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"399651","messageId":"7e803a2ba9458ce35c657e67323edfe4409205ec.1592055716.git.liu.denton@gmail.com","threadId":"53670","inReplyTo":null,"subject":"[PATCH] pkt-line: extract out PACKET_HEADER_SIZE","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-06-13T13:43:22Z","receivedAt":"2020-06-13T13:43:50Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In pkt-line and remote-curl, we have many instances of magic `4`\nliterals floating around which represent the number of bytes in the\npacket line length header. Instead of using these magic numbers, replace\nthem with constant expressions.\n\nIn most cases, replace the `4` with `PACKET_HEADER_SIZE`. However, in\nthe case where there's a `char array[PACKET_HEADER_SIZE]` and we are\nreading data into it, replace the `4` with a `sizeof(array)` so that\nit's clear that the logic has something to do with that array.\n\nSigned-off-by: Denton Liu \n---\n pkt-line.c | 50 +++++++++++++++++++++++++-------------------------\n pkt-line.h | 6 ++++--\n remote-curl.c | 30 +++++++++++++++---------------\n 3 files changed, 44 insertions(+), 42 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 8f9bc68ee2..245a56712f 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -87,43 +87,43 @@ static void packet_trace(const char *buf, unsigned int len, int write)\n */\n void packet_flush(int fd)\n {\n-\tpacket_trace(\"0000\", 4, 1);\n-\tif (write_in_full(fd, \"0000\", 4) < 0)\n+\tpacket_trace(\"0000\", PACKET_HEADER_SIZE, 1);\n+\tif (write_in_full(fd, \"0000\", PACKET_HEADER_SIZE) < 0)\n \t\tdie_errno(_(\"unable to write flush packet\"));\n }\n \n void packet_delim(int fd)\n {\n-\tpacket_trace(\"0001\", 4, 1);\n-\tif (write_in_full(fd, \"0001\", 4) < 0)\n+\tpacket_trace(\"0001\", PACKET_HEADER_SIZE, 1);\n+\tif (write_in_full(fd, \"0001\", PACKET_HEADER_SIZE) < 0)\n \t\tdie_errno(_(\"unable to write delim packet\"));\n }\n \n void packet_response_end(int fd)\n {\n-\tpacket_trace(\"0002\", 4, 1);\n-\tif (write_in_full(fd, \"0002\", 4) < 0)\n+\tpacket_trace(\"0002\", PACKET_HEADER_SIZE, 1);\n+\tif (write_in_full(fd, \"0002\", PACKET_HEADER_SIZE) < 0)\n \t\tdie_errno(_(\"unable to write stateless separator packet\"));\n }\n \n int packet_flush_gently(int fd)\n {\n-\tpacket_trace(\"0000\", 4, 1);\n-\tif (write_in_full(fd, \"0000\", 4) < 0)\n+\tpacket_trace(\"0000\", PACKET_HEADER_SIZE, 1);\n+\tif (write_in_full(fd, \"0000\", PACKET_HEADER_SIZE) < 0)\n \t\treturn error(_(\"flush packet write failed\"));\n \treturn 0;\n }\n \n void packet_buf_flush(struct strbuf *buf)\n {\n-\tpacket_trace(\"0000\", 4, 1);\n-\tstrbuf_add(buf, \"0000\", 4);\n+\tpacket_trace(\"0000\", PACKET_HEADER_SIZE, 1);\n+\tstrbuf_add(buf, \"0000\", PACKET_HEADER_SIZE);\n }\n \n void packet_buf_delim(struct strbuf *buf)\n {\n-\tpacket_trace(\"0001\", 4, 1);\n-\tstrbuf_add(buf, \"0001\", 4);\n+\tpacket_trace(\"0001\", PACKET_HEADER_SIZE, 1);\n+\tstrbuf_add(buf, \"0001\", PACKET_HEADER_SIZE);\n }\n \n void set_packet_header(char *buf, int size)\n@@ -153,7 +153,7 @@ static void format_packet(struct strbuf *out, const char *prefix,\n \t\tdie(_(\"protocol error: impossibly long line\"));\n \n \tset_packet_header(&out->buf[orig_len], n);\n-\tpacket_trace(out->buf + orig_len + 4, n - 4, 1);\n+\tpacket_trace(out->buf + orig_len + PACKET_HEADER_SIZE, n - PACKET_HEADER_SIZE, 1);\n }\n \n static int packet_write_fmt_1(int fd, int gently, const char *prefix,\n@@ -199,13 +199,13 @@ static int packet_write_gently(const int fd_out, const char *buf, size_t size)\n \tstatic char packet_write_buffer[LARGE_PACKET_MAX];\n \tsize_t packet_size;\n \n-\tif (size > sizeof(packet_write_buffer) - 4)\n+\tif (size > sizeof(packet_write_buffer) - PACKET_HEADER_SIZE)\n \t\treturn error(_(\"packet write failed - data exceeds max packet size\"));\n \n \tpacket_trace(buf, size, 1);\n-\tpacket_size = size + 4;\n+\tpacket_size = size + PACKET_HEADER_SIZE;\n \tset_packet_header(packet_write_buffer, packet_size);\n-\tmemcpy(packet_write_buffer + 4, buf, size);\n+\tmemcpy(packet_write_buffer + PACKET_HEADER_SIZE, buf, size);\n \tif (write_in_full(fd_out, packet_write_buffer, packet_size) < 0)\n \t\treturn error(_(\"packet write failed\"));\n \treturn 0;\n@@ -313,7 +313,7 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size,\n \treturn ret;\n }\n \n-int packet_length(const char lenbuf_hex[4])\n+int packet_length(const char lenbuf_hex[PACKET_HEADER_SIZE])\n {\n \tint val = hex2chr(lenbuf_hex);\n \treturn (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);\n@@ -325,9 +325,9 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\t\t\t\t\tint options)\n {\n \tint len;\n-\tchar linelen[4];\n+\tchar linelen[PACKET_HEADER_SIZE];\n \n-\tif (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0) {\n+\tif (get_packet_data(fd, src_buffer, src_len, linelen, sizeof(linelen), options) < 0) {\n \t\t*pktlen = -1;\n \t\treturn PACKET_READ_EOF;\n \t}\n@@ -337,22 +337,22 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \tif (len < 0) {\n \t\tdie(_(\"protocol error: bad line length character: %.4s\"), linelen);\n \t} else if (!len) {\n-\t\tpacket_trace(\"0000\", 4, 0);\n+\t\tpacket_trace(\"0000\", PACKET_HEADER_SIZE, 0);\n \t\t*pktlen = 0;\n \t\treturn PACKET_READ_FLUSH;\n \t} else if (len == 1) {\n-\t\tpacket_trace(\"0001\", 4, 0);\n+\t\tpacket_trace(\"0001\", PACKET_HEADER_SIZE, 0);\n \t\t*pktlen = 0;\n \t\treturn PACKET_READ_DELIM;\n \t} else if (len == 2) {\n-\t\tpacket_trace(\"0002\", 4, 0);\n+\t\tpacket_trace(\"0002\", PACKET_HEADER_SIZE, 0);\n \t\t*pktlen = 0;\n \t\treturn PACKET_READ_RESPONSE_END;\n-\t} else if (len < 4) {\n+\t} else if (len < PACKET_HEADER_SIZE) {\n \t\tdie(_(\"protocol error: bad line length %d\"), len);\n \t}\n \n-\tlen -= 4;\n+\tlen -= sizeof(linelen);\n \tif ((unsigned)len >= size)\n \t\tdie(_(\"protocol error: bad line length %d\"), len);\n \n@@ -370,7 +370,7 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \n \tif ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&\n \t starts_with(buffer, \"ERR \"))\n-\t\tdie(_(\"remote error: %s\"), buffer + 4);\n+\t\tdie(_(\"remote error: %s\"), buffer + PACKET_HEADER_SIZE);\n \n \t*pktlen = len;\n \treturn PACKET_READ_NORMAL;\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 5b373fe4cd..d6121b8044 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -5,6 +5,8 @@\n #include \"strbuf.h\"\n #include \"sideband.h\"\n \n+#define PACKET_HEADER_SIZE 4\n+\n /*\n * Write a packetized stream, where each line is preceded by\n * its length (including the header) as a 4-byte hex number.\n@@ -82,7 +84,7 @@ int packet_read(int fd, char **src_buffer, size_t *src_len, char\n * If lenbuf_hex contains non-hex characters, return -1. Otherwise, return the\n * numeric value of the length header.\n */\n-int packet_length(const char lenbuf_hex[4]);\n+int packet_length(const char lenbuf_hex[PACKET_HEADER_SIZE]);\n \n /*\n * Read a packetized line into a buffer like the 'packet_read()' function but\n@@ -211,7 +213,7 @@ enum packet_read_status packet_reader_peek(struct packet_reader *reader);\n \n #define DEFAULT_PACKET_MAX 1000\n #define LARGE_PACKET_MAX 65520\n-#define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)\n+#define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - PACKET_HEADER_SIZE)\n extern char packet_buffer[LARGE_PACKET_MAX];\n \n struct packet_writer {\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 75532a8bae..bac295c5bc 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -536,7 +536,7 @@ struct rpc_state {\n \tunsigned initial_buffer : 1;\n \n \t/*\n-\t * Whenever a pkt-line is read into buf, append the 4 characters\n+\t * Whenever a pkt-line is read into buf, append the PACKET_HEADER_SIZE characters\n \t * denoting its length before appending the payload.\n \t */\n \tunsigned write_line_lengths : 1;\n@@ -556,7 +556,7 @@ struct rpc_state {\n * rpc->buf and rpc->len if there is enough space. Returns 1 if there was\n * enough space, 0 otherwise.\n *\n- * If rpc->write_line_lengths is true, appends the line length as a 4-byte\n+ * If rpc->write_line_lengths is true, appends the line length as a PACKET_HEADER_SIZE-byte\n * hexadecimal string before appending the result described above.\n *\n * Writes the total number of bytes appended into appended.\n@@ -569,8 +569,8 @@ static int rpc_read_from_out(struct rpc_state *rpc, int options,\n \tint pktlen_raw;\n \n \tif (rpc->write_line_lengths) {\n-\t\tleft = rpc->alloc - rpc->len - 4;\n-\t\tbuf = rpc->buf + rpc->len + 4;\n+\t\tleft = rpc->alloc - rpc->len - PACKET_HEADER_SIZE;\n+\t\tbuf = rpc->buf + rpc->len + PACKET_HEADER_SIZE;\n \t} else {\n \t\tleft = rpc->alloc - rpc->len;\n \t\tbuf = rpc->buf + rpc->len;\n@@ -582,7 +582,7 @@ static int rpc_read_from_out(struct rpc_state *rpc, int options,\n \t*status = packet_read_with_status(rpc->out, NULL, NULL, buf,\n \t\t\tleft, &pktlen_raw, options);\n \tif (*status != PACKET_READ_EOF) {\n-\t\t*appended = pktlen_raw + (rpc->write_line_lengths ? 4 : 0);\n+\t\t*appended = pktlen_raw + (rpc->write_line_lengths ? PACKET_HEADER_SIZE : 0);\n \t\trpc->len += *appended;\n \t}\n \n@@ -593,13 +593,13 @@ static int rpc_read_from_out(struct rpc_state *rpc, int options,\n \t\t\t\tdie(_(\"shouldn't have EOF when not gentle on EOF\"));\n \t\t\tbreak;\n \t\tcase PACKET_READ_NORMAL:\n-\t\t\tset_packet_header(buf - 4, *appended);\n+\t\t\tset_packet_header(buf - PACKET_HEADER_SIZE, *appended);\n \t\t\tbreak;\n \t\tcase PACKET_READ_DELIM:\n-\t\t\tmemcpy(buf - 4, \"0001\", 4);\n+\t\t\tmemcpy(buf - PACKET_HEADER_SIZE, \"0001\", PACKET_HEADER_SIZE);\n \t\t\tbreak;\n \t\tcase PACKET_READ_FLUSH:\n-\t\t\tmemcpy(buf - 4, \"0000\", 4);\n+\t\t\tmemcpy(buf - PACKET_HEADER_SIZE, \"0000\", PACKET_HEADER_SIZE);\n \t\t\tbreak;\n \t\tcase PACKET_READ_RESPONSE_END:\n \t\t\tdie(_(\"remote server sent stateless separator\"));\n@@ -682,7 +682,7 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)\n #endif\n \n struct check_pktline_state {\n-\tchar len_buf[4];\n+\tchar len_buf[PACKET_HEADER_SIZE];\n \tint len_filled;\n \tint remaining;\n };\n@@ -691,7 +691,7 @@ static void check_pktline(struct check_pktline_state *state, const char *ptr, si\n {\n \twhile (size) {\n \t\tif (!state->remaining) {\n-\t\t\tint digits_remaining = 4 - state->len_filled;\n+\t\t\tint digits_remaining = sizeof(state->len_buf) - state->len_filled;\n \t\t\tif (digits_remaining > size)\n \t\t\t\tdigits_remaining = size;\n \t\t\tmemcpy(&state->len_buf[state->len_filled], ptr, digits_remaining);\n@@ -699,16 +699,16 @@ static void check_pktline(struct check_pktline_state *state, const char *ptr, si\n \t\t\tptr += digits_remaining;\n \t\t\tsize -= digits_remaining;\n \n-\t\t\tif (state->len_filled == 4) {\n+\t\t\tif (state->len_filled == sizeof(state->len_buf)) {\n \t\t\t\tstate->remaining = packet_length(state->len_buf);\n \t\t\t\tif (state->remaining < 0) {\n \t\t\t\t\tdie(_(\"remote-curl: bad line length character: %.4s\"), state->len_buf);\n \t\t\t\t} else if (state->remaining == 2) {\n \t\t\t\t\tdie(_(\"remote-curl: unexpected response end packet\"));\n-\t\t\t\t} else if (state->remaining < 4) {\n+\t\t\t\t} else if (state->remaining < sizeof(state->len_buf)) {\n \t\t\t\t\tstate->remaining = 0;\n \t\t\t\t} else {\n-\t\t\t\t\tstate->remaining -= 4;\n+\t\t\t\t\tstate->remaining -= sizeof(state->len_buf);\n \t\t\t\t}\n \t\t\t\tstate->len_filled = 0;\n \t\t\t}\n@@ -804,7 +804,7 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)\n \tcurl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url);\n \tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, NULL);\n \tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, \"0000\");\n-\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, 4);\n+\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, PACKET_HEADER_SIZE);\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);\n \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);\n \tcurl_easy_setopt(slot->curl, CURLOPT_FILE, &buf);\n@@ -1469,7 +1469,7 @@ int cmd_main(int argc, const char **argv)\n \t\t\tparse_fetch(&buf);\n \n \t\t} else if (!strcmp(buf.buf, \"list\") || starts_with(buf.buf, \"list \")) {\n-\t\t\tint for_push = !!strstr(buf.buf + 4, \"for-push\");\n+\t\t\tint for_push = !!strstr(buf.buf + PACKET_HEADER_SIZE, \"for-push\");\n \t\t\toutput_refs(get_refs(for_push));\n \n \t\t} else if (starts_with(buf.buf, \"push \")) {\n-- \n2.27.0.132.g321788e831\n\n"},{"id":"399652","messageId":"20200613142306.GA4680@danh.dev","threadId":"53670","inReplyTo":"7e803a2ba9458ce35c657e67323edfe4409205ec.1592055716.git.liu.denton@gmail.com","subject":"Re: [PATCH] pkt-line: extract out PACKET_HEADER_SIZE","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-06-13T14:23:06Z","receivedAt":"2020-06-13T14:23:14Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-06-13 09:43:22-0400, Denton Liu wrote:\n> In pkt-line and remote-curl, we have many instances of magic `4`\n> literals floating around which represent the number of bytes in the\n> packet line length header. Instead of using these magic numbers, replace\n> them with constant expressions.\n> \n> In most cases, replace the `4` with `PACKET_HEADER_SIZE`. However, in\n> the case where there's a `char array[PACKET_HEADER_SIZE]` and we are\n> reading data into it, replace the `4` with a `sizeof(array)` so that\n> it's clear that the logic has something to do with that array.\n> \n> Signed-off-by: Denton Liu \n> ---\n> pkt-line.c | 50 +++++++++++++++++++++++++-------------------------\n> pkt-line.h | 6 ++++--\n> remote-curl.c | 30 +++++++++++++++---------------\n> 3 files changed, 44 insertions(+), 42 deletions(-)\n> \n> diff --git a/pkt-line.c b/pkt-line.c\n> index 8f9bc68ee2..245a56712f 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -87,43 +87,43 @@ static void packet_trace(const char *buf, unsigned int len, int write)\n> */\n> void packet_flush(int fd)\n> {\n> -\tpacket_trace(\"0000\", 4, 1);\n> -\tif (write_in_full(fd, \"0000\", 4) < 0)\n> +\tpacket_trace(\"0000\", PACKET_HEADER_SIZE, 1);\n> +\tif (write_in_full(fd, \"0000\", PACKET_HEADER_SIZE) < 0)\n\nI think the magic number 4 is easier to follow than some macro\nPACKET_HEADER_SIZE defined elsewhere. Because reader may judge that is\nthe size of \"0000\"\n\nHow about (this ugly code):\n\n\tpacket_trace(\"0000\", sizeof \"0000\" - 1, 1);\n\tif (write_in_full(fd, \"0000\", sizeof \"0000\" - 1) < 0)\n\n\n> \t\tdie_errno(_(\"unable to write flush packet\"));\n> }\n> \n> void packet_delim(int fd)\n> {\n> -\tpacket_trace(\"0001\", 4, 1);\n> -\tif (write_in_full(fd, \"0001\", 4) < 0)\n> +\tpacket_trace(\"0001\", PACKET_HEADER_SIZE, 1);\n> +\tif (write_in_full(fd, \"0001\", PACKET_HEADER_SIZE) < 0)\n> \t\tdie_errno(_(\"unable to write delim packet\"));\n> }\n> \n> void packet_response_end(int fd)\n> {\n> -\tpacket_trace(\"0002\", 4, 1);\n> -\tif (write_in_full(fd, \"0002\", 4) < 0)\n> +\tpacket_trace(\"0002\", PACKET_HEADER_SIZE, 1);\n> +\tif (write_in_full(fd, \"0002\", PACKET_HEADER_SIZE) < 0)\n> \t\tdie_errno(_(\"unable to write stateless separator packet\"));\n> }\n> \n> int packet_flush_gently(int fd)\n> {\n> -\tpacket_trace(\"0000\", 4, 1);\n> -\tif (write_in_full(fd, \"0000\", 4) < 0)\n> +\tpacket_trace(\"0000\", PACKET_HEADER_SIZE, 1);\n> +\tif (write_in_full(fd, \"0000\", PACKET_HEADER_SIZE) < 0)\n> \t\treturn error(_(\"flush packet write failed\"));\n> \treturn 0;\n> }\n> \n> void packet_buf_flush(struct strbuf *buf)\n> {\n> -\tpacket_trace(\"0000\", 4, 1);\n> -\tstrbuf_add(buf, \"0000\", 4);\n> +\tpacket_trace(\"0000\", PACKET_HEADER_SIZE, 1);\n> +\tstrbuf_add(buf, \"0000\", PACKET_HEADER_SIZE);\n> }\n> \n> void packet_buf_delim(struct strbuf *buf)\n> {\n> -\tpacket_trace(\"0001\", 4, 1);\n> -\tstrbuf_add(buf, \"0001\", 4);\n> +\tpacket_trace(\"0001\", PACKET_HEADER_SIZE, 1);\n> +\tstrbuf_add(buf, \"0001\", PACKET_HEADER_SIZE);\n> }\n> \n> void set_packet_header(char *buf, int size)\n> @@ -153,7 +153,7 @@ static void format_packet(struct strbuf *out, const char *prefix,\n> \t\tdie(_(\"protocol error: impossibly long line\"));\n> \n> \tset_packet_header(&out->buf[orig_len], n);\n> -\tpacket_trace(out->buf + orig_len + 4, n - 4, 1);\n> +\tpacket_trace(out->buf + orig_len + PACKET_HEADER_SIZE, n - PACKET_HEADER_SIZE, 1);\n> }\n> \n> static int packet_write_fmt_1(int fd, int gently, const char *prefix,\n> @@ -199,13 +199,13 @@ static int packet_write_gently(const int fd_out, const char *buf, size_t size)\n> \tstatic char packet_write_buffer[LARGE_PACKET_MAX];\n> \tsize_t packet_size;\n> \n> -\tif (size > sizeof(packet_write_buffer) - 4)\n> +\tif (size > sizeof(packet_write_buffer) - PACKET_HEADER_SIZE)\n> \t\treturn error(_(\"packet write failed - data exceeds max packet size\"));\n> \n> \tpacket_trace(buf, size, 1);\n> -\tpacket_size = size + 4;\n> +\tpacket_size = size + PACKET_HEADER_SIZE;\n> \tset_packet_header(packet_write_buffer, packet_size);\n> -\tmemcpy(packet_write_buffer + 4, buf, size);\n> +\tmemcpy(packet_write_buffer + PACKET_HEADER_SIZE, buf, size);\n> \tif (write_in_full(fd_out, packet_write_buffer, packet_size) < 0)\n> \t\treturn error(_(\"packet write failed\"));\n> \treturn 0;\n> @@ -313,7 +313,7 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size,\n> \treturn ret;\n> }\n> \n> -int packet_length(const char lenbuf_hex[4])\n> +int packet_length(const char lenbuf_hex[PACKET_HEADER_SIZE])\n> {\n> \tint val = hex2chr(lenbuf_hex);\n> \treturn (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);\n> @@ -325,9 +325,9 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n> \t\t\t\t\t\tint options)\n> {\n> \tint len;\n> -\tchar linelen[4];\n> +\tchar linelen[PACKET_HEADER_SIZE];\n> \n> -\tif (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0) {\n> +\tif (get_packet_data(fd, src_buffer, src_len, linelen, sizeof(linelen), options) < 0) {\n> \t\t*pktlen = -1;\n> \t\treturn PACKET_READ_EOF;\n> \t}\n> @@ -337,22 +337,22 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n> \tif (len < 0) {\n> \t\tdie(_(\"protocol error: bad line length character: %.4s\"), linelen);\n> \t} else if (!len) {\n> -\t\tpacket_trace(\"0000\", 4, 0);\n> +\t\tpacket_trace(\"0000\", PACKET_HEADER_SIZE, 0);\n> \t\t*pktlen = 0;\n> \t\treturn PACKET_READ_FLUSH;\n> \t} else if (len == 1) {\n> -\t\tpacket_trace(\"0001\", 4, 0);\n> +\t\tpacket_trace(\"0001\", PACKET_HEADER_SIZE, 0);\n> \t\t*pktlen = 0;\n> \t\treturn PACKET_READ_DELIM;\n> \t} else if (len == 2) {\n> -\t\tpacket_trace(\"0002\", 4, 0);\n> +\t\tpacket_trace(\"0002\", PACKET_HEADER_SIZE, 0);\n> \t\t*pktlen = 0;\n> \t\treturn PACKET_READ_RESPONSE_END;\n> -\t} else if (len < 4) {\n> +\t} else if (len < PACKET_HEADER_SIZE) {\n> \t\tdie(_(\"protocol error: bad line length %d\"), len);\n> \t}\n> \n> -\tlen -= 4;\n> +\tlen -= sizeof(linelen);\n> \tif ((unsigned)len >= size)\n> \t\tdie(_(\"protocol error: bad line length %d\"), len);\n> \n> @@ -370,7 +370,7 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n> \n> \tif ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&\n> \t starts_with(buffer, \"ERR \"))\n> -\t\tdie(_(\"remote error: %s\"), buffer + 4);\n> +\t\tdie(_(\"remote error: %s\"), buffer + PACKET_HEADER_SIZE);\n> \n> \t*pktlen = len;\n> \treturn PACKET_READ_NORMAL;\n> diff --git a/pkt-line.h b/pkt-line.h\n> index 5b373fe4cd..d6121b8044 100644\n> --- a/pkt-line.h\n> +++ b/pkt-line.h\n> @@ -5,6 +5,8 @@\n> #include \"strbuf.h\"\n> #include \"sideband.h\"\n> \n> +#define PACKET_HEADER_SIZE 4\n> +\n> /*\n> * Write a packetized stream, where each line is preceded by\n> * its length (including the header) as a 4-byte hex number.\n> @@ -82,7 +84,7 @@ int packet_read(int fd, char **src_buffer, size_t *src_len, char\n> * If lenbuf_hex contains non-hex characters, return -1. Otherwise, return the\n> * numeric value of the length header.\n> */\n> -int packet_length(const char lenbuf_hex[4]);\n> +int packet_length(const char lenbuf_hex[PACKET_HEADER_SIZE]);\n> \n> /*\n> * Read a packetized line into a buffer like the 'packet_read()' function but\n> @@ -211,7 +213,7 @@ enum packet_read_status packet_reader_peek(struct packet_reader *reader);\n> \n> #define DEFAULT_PACKET_MAX 1000\n> #define LARGE_PACKET_MAX 65520\n> -#define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)\n> +#define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - PACKET_HEADER_SIZE)\n> extern char packet_buffer[LARGE_PACKET_MAX];\n> \n> struct packet_writer {\n> diff --git a/remote-curl.c b/remote-curl.c\n> index 75532a8bae..bac295c5bc 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -536,7 +536,7 @@ struct rpc_state {\n> \tunsigned initial_buffer : 1;\n> \n> \t/*\n> -\t * Whenever a pkt-line is read into buf, append the 4 characters\n> +\t * Whenever a pkt-line is read into buf, append the PACKET_HEADER_SIZE characters\n> \t * denoting its length before appending the payload.\n> \t */\n> \tunsigned write_line_lengths : 1;\n> @@ -556,7 +556,7 @@ struct rpc_state {\n> * rpc->buf and rpc->len if there is enough space. Returns 1 if there was\n> * enough space, 0 otherwise.\n> *\n> - * If rpc->write_line_lengths is true, appends the line length as a 4-byte\n> + * If rpc->write_line_lengths is true, appends the line length as a PACKET_HEADER_SIZE-byte\n> * hexadecimal string before appending the result described above.\n> *\n> * Writes the total number of bytes appended into appended.\n> @@ -569,8 +569,8 @@ static int rpc_read_from_out(struct rpc_state *rpc, int options,\n> \tint pktlen_raw;\n> \n> \tif (rpc->write_line_lengths) {\n> -\t\tleft = rpc->alloc - rpc->len - 4;\n> -\t\tbuf = rpc->buf + rpc->len + 4;\n> +\t\tleft = rpc->alloc - rpc->len - PACKET_HEADER_SIZE;\n> +\t\tbuf = rpc->buf + rpc->len + PACKET_HEADER_SIZE;\n> \t} else {\n> \t\tleft = rpc->alloc - rpc->len;\n> \t\tbuf = rpc->buf + rpc->len;\n> @@ -582,7 +582,7 @@ static int rpc_read_from_out(struct rpc_state *rpc, int options,\n> \t*status = packet_read_with_status(rpc->out, NULL, NULL, buf,\n> \t\t\tleft, &pktlen_raw, options);\n> \tif (*status != PACKET_READ_EOF) {\n> -\t\t*appended = pktlen_raw + (rpc->write_line_lengths ? 4 : 0);\n> +\t\t*appended = pktlen_raw + (rpc->write_line_lengths ? PACKET_HEADER_SIZE : 0);\n> \t\trpc->len += *appended;\n> \t}\n> \n> @@ -593,13 +593,13 @@ static int rpc_read_from_out(struct rpc_state *rpc, int options,\n> \t\t\t\tdie(_(\"shouldn't have EOF when not gentle on EOF\"));\n> \t\t\tbreak;\n> \t\tcase PACKET_READ_NORMAL:\n> -\t\t\tset_packet_header(buf - 4, *appended);\n> +\t\t\tset_packet_header(buf - PACKET_HEADER_SIZE, *appended);\n> \t\t\tbreak;\n> \t\tcase PACKET_READ_DELIM:\n> -\t\t\tmemcpy(buf - 4, \"0001\", 4);\n> +\t\t\tmemcpy(buf - PACKET_HEADER_SIZE, \"0001\", PACKET_HEADER_SIZE);\n> \t\t\tbreak;\n> \t\tcase PACKET_READ_FLUSH:\n> -\t\t\tmemcpy(buf - 4, \"0000\", 4);\n> +\t\t\tmemcpy(buf - PACKET_HEADER_SIZE, \"0000\", PACKET_HEADER_SIZE);\n> \t\t\tbreak;\n> \t\tcase PACKET_READ_RESPONSE_END:\n> \t\t\tdie(_(\"remote server sent stateless separator\"));\n> @@ -682,7 +682,7 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)\n> #endif\n> \n> struct check_pktline_state {\n> -\tchar len_buf[4];\n> +\tchar len_buf[PACKET_HEADER_SIZE];\n> \tint len_filled;\n> \tint remaining;\n> };\n> @@ -691,7 +691,7 @@ static void check_pktline(struct check_pktline_state *state, const char *ptr, si\n> {\n> \twhile (size) {\n> \t\tif (!state->remaining) {\n> -\t\t\tint digits_remaining = 4 - state->len_filled;\n> +\t\t\tint digits_remaining = sizeof(state->len_buf) - state->len_filled;\n> \t\t\tif (digits_remaining > size)\n> \t\t\t\tdigits_remaining = size;\n> \t\t\tmemcpy(&state->len_buf[state->len_filled], ptr, digits_remaining);\n> @@ -699,16 +699,16 @@ static void check_pktline(struct check_pktline_state *state, const char *ptr, si\n> \t\t\tptr += digits_remaining;\n> \t\t\tsize -= digits_remaining;\n> \n> -\t\t\tif (state->len_filled == 4) {\n> +\t\t\tif (state->len_filled == sizeof(state->len_buf)) {\n> \t\t\t\tstate->remaining = packet_length(state->len_buf);\n> \t\t\t\tif (state->remaining < 0) {\n> \t\t\t\t\tdie(_(\"remote-curl: bad line length character: %.4s\"), state->len_buf);\n> \t\t\t\t} else if (state->remaining == 2) {\n> \t\t\t\t\tdie(_(\"remote-curl: unexpected response end packet\"));\n> -\t\t\t\t} else if (state->remaining < 4) {\n> +\t\t\t\t} else if (state->remaining < sizeof(state->len_buf)) {\n> \t\t\t\t\tstate->remaining = 0;\n> \t\t\t\t} else {\n> -\t\t\t\t\tstate->remaining -= 4;\n> +\t\t\t\t\tstate->remaining -= sizeof(state->len_buf);\n> \t\t\t\t}\n> \t\t\t\tstate->len_filled = 0;\n> \t\t\t}\n> @@ -804,7 +804,7 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)\n> \tcurl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url);\n> \tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, NULL);\n> \tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, \"0000\");\n> -\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, 4);\n> +\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, PACKET_HEADER_SIZE);\n> \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);\n> \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);\n> \tcurl_easy_setopt(slot->curl, CURLOPT_FILE, &buf);\n> @@ -1469,7 +1469,7 @@ int cmd_main(int argc, const char **argv)\n> \t\t\tparse_fetch(&buf);\n> \n> \t\t} else if (!strcmp(buf.buf, \"list\") || starts_with(buf.buf, \"list \")) {\n> -\t\t\tint for_push = !!strstr(buf.buf + 4, \"for-push\");\n> +\t\t\tint for_push = !!strstr(buf.buf + PACKET_HEADER_SIZE, \"for-push\");\n> \t\t\toutput_refs(get_refs(for_push));\n> \n> \t\t} else if (starts_with(buf.buf, \"push \")) {\n> -- \n> 2.27.0.132.g321788e831\n> \n\n-- \nDanh\n"},{"id":"399654","messageId":"20200613143931.GA730181@generichostname","threadId":"53670","inReplyTo":"20200613142306.GA4680@danh.dev","subject":"Re: [PATCH] pkt-line: extract out PACKET_HEADER_SIZE","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-06-13T14:39:31Z","receivedAt":"2020-06-13T14:39:37Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Sat, Jun 13, 2020 at 09:23:06PM +0700, Đoàn Trần Công Danh wrote:\n> > diff --git a/pkt-line.c b/pkt-line.c\n> > index 8f9bc68ee2..245a56712f 100644\n> > --- a/pkt-line.c\n> > +++ b/pkt-line.c\n> > @@ -87,43 +87,43 @@ static void packet_trace(const char *buf, unsigned int len, int write)\n> > */\n> > void packet_flush(int fd)\n> > {\n> > -\tpacket_trace(\"0000\", 4, 1);\n> > -\tif (write_in_full(fd, \"0000\", 4) < 0)\n> > +\tpacket_trace(\"0000\", PACKET_HEADER_SIZE, 1);\n> > +\tif (write_in_full(fd, \"0000\", PACKET_HEADER_SIZE) < 0)\n> \n> I think the magic number 4 is easier to follow than some macro\n> PACKET_HEADER_SIZE defined elsewhere. Because reader may judge that is\n> the size of \"0000\"\n> \n> How about (this ugly code):\n> \n> \tpacket_trace(\"0000\", sizeof \"0000\" - 1, 1);\n> \tif (write_in_full(fd, \"0000\", sizeof \"0000\" - 1) < 0)\n\nChris Torek mentioned something similar off-list:\n> This seems sensible, but at the same time, the string literals must\n> have the right size. For instance:\n> \n[...the diff above...]\n> \n> So if you're going to do this, it might be a good idea to replace\n> the string literals \"0000\", \"0001\", etc, with defined values as well.\n\nBetween the two suggested approaches, with your approach, it's\nimmediately obvious what the values of the packets are but, on the other\nhand, with Chris's approach, we can consolidate the \"magic\" string\nliterals together into a definition in a single-place.\n\nIn this case, the strings don't seem too magical and I think that the\npeople on this list are generally against over-abstracting and would\nconsider it more readable to leave the literals in place. (If this were\nmy own project, I'd take Chris's approach, though, since I try to avoid\nbare literals like the plague.)\n"},{"id":"399658","messageId":"xmqqimfuzz73.fsf@gitster.c.googlers.com","threadId":"53670","inReplyTo":"20200613142306.GA4680@danh.dev","subject":"Re: [PATCH] pkt-line: extract out PACKET_HEADER_SIZE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-13T16:51:28Z","receivedAt":"2020-06-13T16:51:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Đoàn Trần Công Danh writes:\n\n>> void packet_flush(int fd)\n>> {\n>> -\tpacket_trace(\"0000\", 4, 1);\n>> -\tif (write_in_full(fd, \"0000\", 4) < 0)\n>> +\tpacket_trace(\"0000\", PACKET_HEADER_SIZE, 1);\n>> +\tif (write_in_full(fd, \"0000\", PACKET_HEADER_SIZE) < 0)\n>\n> I think the magic number 4 is easier to follow than some macro\n> PACKET_HEADER_SIZE defined elsewhere. Because reader may judge that is\n> the size of \"0000\"\n\nI hate to admit it immensely, but I tend to agree with you. In the\ncontext of this single call,\n\n\twrite_in_full(fd, \"0000\", 4)\n\twrite_in_full(fd, \"0000\", PACKET_HEADER_SIZE)\n\nI find the former vastly easier.\n\n> How about (this ugly code):\n>\n> \tpacket_trace(\"0000\", sizeof \"0000\" - 1, 1);\n> \tif (write_in_full(fd, \"0000\", sizeof \"0000\" - 1) < 0)\n\nYeah, that is ugly. I was thinking more in the direction of\nreplacing these three-argument write_in_full with something like\n\n#define write_constant(fd, constant_string) \\\n\twrite_in_full((fd), (constant_string), strlen(constant_string))\n\nwith some preprocessor magic to make the compilation break when the\nsecond parameter to the macro is not a string constant.\n"},{"id":"399687","messageId":"cover.1592119902.git.liu.denton@gmail.com","threadId":"53670","inReplyTo":"7e803a2ba9458ce35c657e67323edfe4409205ec.1592055716.git.liu.denton@gmail.com","subject":"[PATCH v2 0/3] pkt-line: war on magical `4` literal","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-06-14T07:31:57Z","receivedAt":"2020-06-14T07:32:15Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"There are many instances of the literal `4` in packet line-related code.\nThis series replaces these instances with either functions that can\ngenerate the number or an appropriately-named constant.\n\nChanges since v1:\n\n* Introduce patch 1-2 so that the string length is used to generate the\n `4` where appropriate\n\nDenton Liu (3):\n remote-curl: use strlen() instead of magic numbers\n pkt-line: use string versions of functions\n pkt-line: extract out PACKET_HEADER_SIZE\n\n pkt-line.c | 66 +++++++++++++++++++++++++++++----------------------\n pkt-line.h | 6 +++--\n remote-curl.c | 35 ++++++++++++++-------------\n 3 files changed, 60 insertions(+), 47 deletions(-)\n\nInterdiff against v1:\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 245a56712f..f1fe0888c1 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -81,49 +81,59 @@ static void packet_trace(const char *buf, unsigned int len, int write)\n \tstrbuf_release(&out);\n }\n \n+static inline void packet_trace_str(const char *buf, int write)\n+{\n+\tpacket_trace(buf, strlen(buf), write);\n+}\n+\n+static inline void control_packet_write(int fd, const char *s, const char *type)\n+{\n+\tpacket_trace_str(s, 1);\n+\tif (write_str_in_full(fd, s) < 0)\n+\t\tdie_errno(_(\"unable to write %s packet\"), type);\n+}\n+\n /*\n * If we buffered things up above (we don't, but we should),\n * we'd flush it here\n */\n void packet_flush(int fd)\n {\n-\tpacket_trace(\"0000\", PACKET_HEADER_SIZE, 1);\n-\tif (write_in_full(fd, \"0000\", PACKET_HEADER_SIZE) < 0)\n-\t\tdie_errno(_(\"unable to write flush packet\"));\n+\tcontrol_packet_write(fd, \"0000\", \"flush\");\n }\n \n void packet_delim(int fd)\n {\n-\tpacket_trace(\"0001\", PACKET_HEADER_SIZE, 1);\n-\tif (write_in_full(fd, \"0001\", PACKET_HEADER_SIZE) < 0)\n-\t\tdie_errno(_(\"unable to write delim packet\"));\n+\tcontrol_packet_write(fd, \"0001\", \"delim\");\n }\n \n void packet_response_end(int fd)\n {\n-\tpacket_trace(\"0002\", PACKET_HEADER_SIZE, 1);\n-\tif (write_in_full(fd, \"0002\", PACKET_HEADER_SIZE) < 0)\n-\t\tdie_errno(_(\"unable to write stateless separator packet\"));\n+\tcontrol_packet_write(fd, \"0002\", \"stateless separator\");\n }\n \n int packet_flush_gently(int fd)\n {\n-\tpacket_trace(\"0000\", PACKET_HEADER_SIZE, 1);\n-\tif (write_in_full(fd, \"0000\", PACKET_HEADER_SIZE) < 0)\n+\tpacket_trace_str(\"0000\", 1);\n+\tif (write_str_in_full(fd, \"0000\") < 0)\n \t\treturn error(_(\"flush packet write failed\"));\n \treturn 0;\n }\n \n+static inline void control_packet_buf_write(struct strbuf *buf, const char *s)\n+{\n+\tpacket_trace_str(s, 1);\n+\tstrbuf_addstr(buf, s);\n+}\n+\n void packet_buf_flush(struct strbuf *buf)\n {\n-\tpacket_trace(\"0000\", PACKET_HEADER_SIZE, 1);\n-\tstrbuf_add(buf, \"0000\", PACKET_HEADER_SIZE);\n+\tcontrol_packet_buf_write(buf, \"0000\");\n }\n \n void packet_buf_delim(struct strbuf *buf)\n {\n-\tpacket_trace(\"0001\", PACKET_HEADER_SIZE, 1);\n-\tstrbuf_add(buf, \"0001\", PACKET_HEADER_SIZE);\n+\tcontrol_packet_buf_write(buf, \"0001\");\n }\n \n void set_packet_header(char *buf, int size)\n@@ -337,15 +347,15 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \tif (len < 0) {\n \t\tdie(_(\"protocol error: bad line length character: %.4s\"), linelen);\n \t} else if (!len) {\n-\t\tpacket_trace(\"0000\", PACKET_HEADER_SIZE, 0);\n+\t\tpacket_trace_str(\"0000\", 0);\n \t\t*pktlen = 0;\n \t\treturn PACKET_READ_FLUSH;\n \t} else if (len == 1) {\n-\t\tpacket_trace(\"0001\", PACKET_HEADER_SIZE, 0);\n+\t\tpacket_trace_str(\"0001\", 0);\n \t\t*pktlen = 0;\n \t\treturn PACKET_READ_DELIM;\n \t} else if (len == 2) {\n-\t\tpacket_trace(\"0002\", PACKET_HEADER_SIZE, 0);\n+\t\tpacket_trace_str(\"0002\", 0);\n \t\t*pktlen = 0;\n \t\treturn PACKET_READ_RESPONSE_END;\n \t} else if (len < PACKET_HEADER_SIZE) {\ndiff --git a/remote-curl.c b/remote-curl.c\nindex bac295c5bc..7ba1280a41 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -536,8 +536,8 @@ struct rpc_state {\n \tunsigned initial_buffer : 1;\n \n \t/*\n-\t * Whenever a pkt-line is read into buf, append the PACKET_HEADER_SIZE characters\n-\t * denoting its length before appending the payload.\n+\t * Whenever a pkt-line is read into buf, append the PACKET_HEADER_SIZE\n+\t * characters denoting its length before appending the payload.\n \t */\n \tunsigned write_line_lengths : 1;\n \n@@ -556,8 +556,9 @@ struct rpc_state {\n * rpc->buf and rpc->len if there is enough space. Returns 1 if there was\n * enough space, 0 otherwise.\n *\n- * If rpc->write_line_lengths is true, appends the line length as a PACKET_HEADER_SIZE-byte\n- * hexadecimal string before appending the result described above.\n+ * If rpc->write_line_lengths is true, appends the line length as a\n+ * PACKET_HEADER_SIZE-byte hexadecimal string before appending the result\n+ * described above.\n *\n * Writes the total number of bytes appended into appended.\n */\n@@ -596,10 +597,10 @@ static int rpc_read_from_out(struct rpc_state *rpc, int options,\n \t\t\tset_packet_header(buf - PACKET_HEADER_SIZE, *appended);\n \t\t\tbreak;\n \t\tcase PACKET_READ_DELIM:\n-\t\t\tmemcpy(buf - PACKET_HEADER_SIZE, \"0001\", PACKET_HEADER_SIZE);\n+\t\t\tmemcpy(buf - strlen(\"0001\"), \"0001\", strlen(\"0001\"));\n \t\t\tbreak;\n \t\tcase PACKET_READ_FLUSH:\n-\t\t\tmemcpy(buf - PACKET_HEADER_SIZE, \"0000\", PACKET_HEADER_SIZE);\n+\t\t\tmemcpy(buf - strlen(\"0000\"), \"0000\", strlen(\"0000\"));\n \t\t\tbreak;\n \t\tcase PACKET_READ_RESPONSE_END:\n \t\t\tdie(_(\"remote server sent stateless separator\"));\n-- \n2.27.0.132.g321788e831\n\n"},{"id":"399688","messageId":"d1b79c7734f0609fcac5e523644c3093f538bccf.1592119902.git.liu.denton@gmail.com","threadId":"53670","inReplyTo":"cover.1592119902.git.liu.denton@gmail.com","subject":"[PATCH v2 2/3] pkt-line: use string versions of functions","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-06-14T07:31:59Z","receivedAt":"2020-06-14T07:32:16Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"We have many cases where we are writing a control packet as a string\nconstant out and we need to specify the length of the string. Currently,\nthe length is specified as a magical `4` literal.\n\nChange these instances to use a function that calls strlen() to\ndetermine the length of the string removing the need to specify the\nlength at all. Since these functions are inline, the strlen()s should be\nreplaced with constants at compile-time so this should not result in any\nperformance penalty.\n\nSigned-off-by: Denton Liu \n---\n pkt-line.c | 46 ++++++++++++++++++++++++++++------------------\n 1 file changed, 28 insertions(+), 18 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 8f9bc68ee2..72c6c29e03 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -81,49 +81,59 @@ static void packet_trace(const char *buf, unsigned int len, int write)\n \tstrbuf_release(&out);\n }\n \n+static inline void packet_trace_str(const char *buf, int write)\n+{\n+\tpacket_trace(buf, strlen(buf), write);\n+}\n+\n+static inline void control_packet_write(int fd, const char *s, const char *type)\n+{\n+\tpacket_trace_str(s, 1);\n+\tif (write_str_in_full(fd, s) < 0)\n+\t\tdie_errno(_(\"unable to write %s packet\"), type);\n+}\n+\n /*\n * If we buffered things up above (we don't, but we should),\n * we'd flush it here\n */\n void packet_flush(int fd)\n {\n-\tpacket_trace(\"0000\", 4, 1);\n-\tif (write_in_full(fd, \"0000\", 4) < 0)\n-\t\tdie_errno(_(\"unable to write flush packet\"));\n+\tcontrol_packet_write(fd, \"0000\", \"flush\");\n }\n \n void packet_delim(int fd)\n {\n-\tpacket_trace(\"0001\", 4, 1);\n-\tif (write_in_full(fd, \"0001\", 4) < 0)\n-\t\tdie_errno(_(\"unable to write delim packet\"));\n+\tcontrol_packet_write(fd, \"0001\", \"delim\");\n }\n \n void packet_response_end(int fd)\n {\n-\tpacket_trace(\"0002\", 4, 1);\n-\tif (write_in_full(fd, \"0002\", 4) < 0)\n-\t\tdie_errno(_(\"unable to write stateless separator packet\"));\n+\tcontrol_packet_write(fd, \"0002\", \"stateless separator\");\n }\n \n int packet_flush_gently(int fd)\n {\n-\tpacket_trace(\"0000\", 4, 1);\n-\tif (write_in_full(fd, \"0000\", 4) < 0)\n+\tpacket_trace_str(\"0000\", 1);\n+\tif (write_str_in_full(fd, \"0000\") < 0)\n \t\treturn error(_(\"flush packet write failed\"));\n \treturn 0;\n }\n \n+static inline void control_packet_buf_write(struct strbuf *buf, const char *s)\n+{\n+\tpacket_trace_str(s, 1);\n+\tstrbuf_addstr(buf, s);\n+}\n+\n void packet_buf_flush(struct strbuf *buf)\n {\n-\tpacket_trace(\"0000\", 4, 1);\n-\tstrbuf_add(buf, \"0000\", 4);\n+\tcontrol_packet_buf_write(buf, \"0000\");\n }\n \n void packet_buf_delim(struct strbuf *buf)\n {\n-\tpacket_trace(\"0001\", 4, 1);\n-\tstrbuf_add(buf, \"0001\", 4);\n+\tcontrol_packet_buf_write(buf, \"0001\");\n }\n \n void set_packet_header(char *buf, int size)\n@@ -337,15 +347,15 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \tif (len < 0) {\n \t\tdie(_(\"protocol error: bad line length character: %.4s\"), linelen);\n \t} else if (!len) {\n-\t\tpacket_trace(\"0000\", 4, 0);\n+\t\tpacket_trace_str(\"0000\", 0);\n \t\t*pktlen = 0;\n \t\treturn PACKET_READ_FLUSH;\n \t} else if (len == 1) {\n-\t\tpacket_trace(\"0001\", 4, 0);\n+\t\tpacket_trace_str(\"0001\", 0);\n \t\t*pktlen = 0;\n \t\treturn PACKET_READ_DELIM;\n \t} else if (len == 2) {\n-\t\tpacket_trace(\"0002\", 4, 0);\n+\t\tpacket_trace_str(\"0002\", 0);\n \t\t*pktlen = 0;\n \t\treturn PACKET_READ_RESPONSE_END;\n \t} else if (len < 4) {\n-- \n2.27.0.132.g321788e831\n\n"},{"id":"399689","messageId":"aad3b9659ba36e372ddf213a5fcc10c30960bf07.1592119902.git.liu.denton@gmail.com","threadId":"53670","inReplyTo":"cover.1592119902.git.liu.denton@gmail.com","subject":"[PATCH v2 1/3] remote-curl: use strlen() instead of magic numbers","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-06-14T07:31:58Z","receivedAt":"2020-06-14T07:32:18Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"When we are memcpy()ing the length header, we use the magic literal `4`,\nrepresenting the length of \"0000\" and \"0001\", the packet line length\nheaders. Use `strlen(\"000x\")` so that we do not have to use the magic\nliteral.\n\nSigned-off-by: Denton Liu \n---\n remote-curl.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 75532a8bae..5536372b34 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -596,10 +596,10 @@ static int rpc_read_from_out(struct rpc_state *rpc, int options,\n \t\t\tset_packet_header(buf - 4, *appended);\n \t\t\tbreak;\n \t\tcase PACKET_READ_DELIM:\n-\t\t\tmemcpy(buf - 4, \"0001\", 4);\n+\t\t\tmemcpy(buf - strlen(\"0001\"), \"0001\", strlen(\"0001\"));\n \t\t\tbreak;\n \t\tcase PACKET_READ_FLUSH:\n-\t\t\tmemcpy(buf - 4, \"0000\", 4);\n+\t\t\tmemcpy(buf - strlen(\"0000\"), \"0000\", strlen(\"0000\"));\n \t\t\tbreak;\n \t\tcase PACKET_READ_RESPONSE_END:\n \t\t\tdie(_(\"remote server sent stateless separator\"));\n-- \n2.27.0.132.g321788e831\n\n"},{"id":"399690","messageId":"aff4f6401d0ee70f6871f6d75078ddd1e0ca6e47.1592119902.git.liu.denton@gmail.com","threadId":"53670","inReplyTo":"cover.1592119902.git.liu.denton@gmail.com","subject":"[PATCH v2 3/3] pkt-line: extract out PACKET_HEADER_SIZE","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-06-14T07:32:00Z","receivedAt":"2020-06-14T07:32:18Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In pkt-line and remote-curl, we have many instances of magic `4`\nliterals floating around which represent the number of bytes in the\npacket line length header. Instead of using these magic numbers, replace\nthem with constant expressions.\n\nIn most cases, replace the `4` with `PACKET_HEADER_SIZE`. However, in\nthe case where there's a `char array[PACKET_HEADER_SIZE]` and we are\nreading data into it, replace the `4` with a `sizeof(array)` so that\nit's clear that the logic has something to do with that array.\n\nSigned-off-by: Denton Liu \n---\n pkt-line.c | 20 ++++++++++----------\n pkt-line.h | 6 ++++--\n remote-curl.c | 31 ++++++++++++++++---------------\n 3 files changed, 30 insertions(+), 27 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 72c6c29e03..f1fe0888c1 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -163,7 +163,7 @@ static void format_packet(struct strbuf *out, const char *prefix,\n \t\tdie(_(\"protocol error: impossibly long line\"));\n \n \tset_packet_header(&out->buf[orig_len], n);\n-\tpacket_trace(out->buf + orig_len + 4, n - 4, 1);\n+\tpacket_trace(out->buf + orig_len + PACKET_HEADER_SIZE, n - PACKET_HEADER_SIZE, 1);\n }\n \n static int packet_write_fmt_1(int fd, int gently, const char *prefix,\n@@ -209,13 +209,13 @@ static int packet_write_gently(const int fd_out, const char *buf, size_t size)\n \tstatic char packet_write_buffer[LARGE_PACKET_MAX];\n \tsize_t packet_size;\n \n-\tif (size > sizeof(packet_write_buffer) - 4)\n+\tif (size > sizeof(packet_write_buffer) - PACKET_HEADER_SIZE)\n \t\treturn error(_(\"packet write failed - data exceeds max packet size\"));\n \n \tpacket_trace(buf, size, 1);\n-\tpacket_size = size + 4;\n+\tpacket_size = size + PACKET_HEADER_SIZE;\n \tset_packet_header(packet_write_buffer, packet_size);\n-\tmemcpy(packet_write_buffer + 4, buf, size);\n+\tmemcpy(packet_write_buffer + PACKET_HEADER_SIZE, buf, size);\n \tif (write_in_full(fd_out, packet_write_buffer, packet_size) < 0)\n \t\treturn error(_(\"packet write failed\"));\n \treturn 0;\n@@ -323,7 +323,7 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size,\n \treturn ret;\n }\n \n-int packet_length(const char lenbuf_hex[4])\n+int packet_length(const char lenbuf_hex[PACKET_HEADER_SIZE])\n {\n \tint val = hex2chr(lenbuf_hex);\n \treturn (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);\n@@ -335,9 +335,9 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\t\t\t\t\tint options)\n {\n \tint len;\n-\tchar linelen[4];\n+\tchar linelen[PACKET_HEADER_SIZE];\n \n-\tif (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0) {\n+\tif (get_packet_data(fd, src_buffer, src_len, linelen, sizeof(linelen), options) < 0) {\n \t\t*pktlen = -1;\n \t\treturn PACKET_READ_EOF;\n \t}\n@@ -358,11 +358,11 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\tpacket_trace_str(\"0002\", 0);\n \t\t*pktlen = 0;\n \t\treturn PACKET_READ_RESPONSE_END;\n-\t} else if (len < 4) {\n+\t} else if (len < PACKET_HEADER_SIZE) {\n \t\tdie(_(\"protocol error: bad line length %d\"), len);\n \t}\n \n-\tlen -= 4;\n+\tlen -= sizeof(linelen);\n \tif ((unsigned)len >= size)\n \t\tdie(_(\"protocol error: bad line length %d\"), len);\n \n@@ -380,7 +380,7 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \n \tif ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&\n \t starts_with(buffer, \"ERR \"))\n-\t\tdie(_(\"remote error: %s\"), buffer + 4);\n+\t\tdie(_(\"remote error: %s\"), buffer + PACKET_HEADER_SIZE);\n \n \t*pktlen = len;\n \treturn PACKET_READ_NORMAL;\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 5b373fe4cd..d6121b8044 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -5,6 +5,8 @@\n #include \"strbuf.h\"\n #include \"sideband.h\"\n \n+#define PACKET_HEADER_SIZE 4\n+\n /*\n * Write a packetized stream, where each line is preceded by\n * its length (including the header) as a 4-byte hex number.\n@@ -82,7 +84,7 @@ int packet_read(int fd, char **src_buffer, size_t *src_len, char\n * If lenbuf_hex contains non-hex characters, return -1. Otherwise, return the\n * numeric value of the length header.\n */\n-int packet_length(const char lenbuf_hex[4]);\n+int packet_length(const char lenbuf_hex[PACKET_HEADER_SIZE]);\n \n /*\n * Read a packetized line into a buffer like the 'packet_read()' function but\n@@ -211,7 +213,7 @@ enum packet_read_status packet_reader_peek(struct packet_reader *reader);\n \n #define DEFAULT_PACKET_MAX 1000\n #define LARGE_PACKET_MAX 65520\n-#define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)\n+#define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - PACKET_HEADER_SIZE)\n extern char packet_buffer[LARGE_PACKET_MAX];\n \n struct packet_writer {\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 5536372b34..7ba1280a41 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -536,8 +536,8 @@ struct rpc_state {\n \tunsigned initial_buffer : 1;\n \n \t/*\n-\t * Whenever a pkt-line is read into buf, append the 4 characters\n-\t * denoting its length before appending the payload.\n+\t * Whenever a pkt-line is read into buf, append the PACKET_HEADER_SIZE\n+\t * characters denoting its length before appending the payload.\n \t */\n \tunsigned write_line_lengths : 1;\n \n@@ -556,8 +556,9 @@ struct rpc_state {\n * rpc->buf and rpc->len if there is enough space. Returns 1 if there was\n * enough space, 0 otherwise.\n *\n- * If rpc->write_line_lengths is true, appends the line length as a 4-byte\n- * hexadecimal string before appending the result described above.\n+ * If rpc->write_line_lengths is true, appends the line length as a\n+ * PACKET_HEADER_SIZE-byte hexadecimal string before appending the result\n+ * described above.\n *\n * Writes the total number of bytes appended into appended.\n */\n@@ -569,8 +570,8 @@ static int rpc_read_from_out(struct rpc_state *rpc, int options,\n \tint pktlen_raw;\n \n \tif (rpc->write_line_lengths) {\n-\t\tleft = rpc->alloc - rpc->len - 4;\n-\t\tbuf = rpc->buf + rpc->len + 4;\n+\t\tleft = rpc->alloc - rpc->len - PACKET_HEADER_SIZE;\n+\t\tbuf = rpc->buf + rpc->len + PACKET_HEADER_SIZE;\n \t} else {\n \t\tleft = rpc->alloc - rpc->len;\n \t\tbuf = rpc->buf + rpc->len;\n@@ -582,7 +583,7 @@ static int rpc_read_from_out(struct rpc_state *rpc, int options,\n \t*status = packet_read_with_status(rpc->out, NULL, NULL, buf,\n \t\t\tleft, &pktlen_raw, options);\n \tif (*status != PACKET_READ_EOF) {\n-\t\t*appended = pktlen_raw + (rpc->write_line_lengths ? 4 : 0);\n+\t\t*appended = pktlen_raw + (rpc->write_line_lengths ? PACKET_HEADER_SIZE : 0);\n \t\trpc->len += *appended;\n \t}\n \n@@ -593,7 +594,7 @@ static int rpc_read_from_out(struct rpc_state *rpc, int options,\n \t\t\t\tdie(_(\"shouldn't have EOF when not gentle on EOF\"));\n \t\t\tbreak;\n \t\tcase PACKET_READ_NORMAL:\n-\t\t\tset_packet_header(buf - 4, *appended);\n+\t\t\tset_packet_header(buf - PACKET_HEADER_SIZE, *appended);\n \t\t\tbreak;\n \t\tcase PACKET_READ_DELIM:\n \t\t\tmemcpy(buf - strlen(\"0001\"), \"0001\", strlen(\"0001\"));\n@@ -682,7 +683,7 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)\n #endif\n \n struct check_pktline_state {\n-\tchar len_buf[4];\n+\tchar len_buf[PACKET_HEADER_SIZE];\n \tint len_filled;\n \tint remaining;\n };\n@@ -691,7 +692,7 @@ static void check_pktline(struct check_pktline_state *state, const char *ptr, si\n {\n \twhile (size) {\n \t\tif (!state->remaining) {\n-\t\t\tint digits_remaining = 4 - state->len_filled;\n+\t\t\tint digits_remaining = sizeof(state->len_buf) - state->len_filled;\n \t\t\tif (digits_remaining > size)\n \t\t\t\tdigits_remaining = size;\n \t\t\tmemcpy(&state->len_buf[state->len_filled], ptr, digits_remaining);\n@@ -699,16 +700,16 @@ static void check_pktline(struct check_pktline_state *state, const char *ptr, si\n \t\t\tptr += digits_remaining;\n \t\t\tsize -= digits_remaining;\n \n-\t\t\tif (state->len_filled == 4) {\n+\t\t\tif (state->len_filled == sizeof(state->len_buf)) {\n \t\t\t\tstate->remaining = packet_length(state->len_buf);\n \t\t\t\tif (state->remaining < 0) {\n \t\t\t\t\tdie(_(\"remote-curl: bad line length character: %.4s\"), state->len_buf);\n \t\t\t\t} else if (state->remaining == 2) {\n \t\t\t\t\tdie(_(\"remote-curl: unexpected response end packet\"));\n-\t\t\t\t} else if (state->remaining < 4) {\n+\t\t\t\t} else if (state->remaining < sizeof(state->len_buf)) {\n \t\t\t\t\tstate->remaining = 0;\n \t\t\t\t} else {\n-\t\t\t\t\tstate->remaining -= 4;\n+\t\t\t\t\tstate->remaining -= sizeof(state->len_buf);\n \t\t\t\t}\n \t\t\t\tstate->len_filled = 0;\n \t\t\t}\n@@ -804,7 +805,7 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)\n \tcurl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url);\n \tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, NULL);\n \tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, \"0000\");\n-\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, 4);\n+\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, PACKET_HEADER_SIZE);\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);\n \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);\n \tcurl_easy_setopt(slot->curl, CURLOPT_FILE, &buf);\n@@ -1469,7 +1470,7 @@ int cmd_main(int argc, const char **argv)\n \t\t\tparse_fetch(&buf);\n \n \t\t} else if (!strcmp(buf.buf, \"list\") || starts_with(buf.buf, \"list \")) {\n-\t\t\tint for_push = !!strstr(buf.buf + 4, \"for-push\");\n+\t\t\tint for_push = !!strstr(buf.buf + PACKET_HEADER_SIZE, \"for-push\");\n \t\t\toutput_refs(get_refs(for_push));\n \n \t\t} else if (starts_with(buf.buf, \"push \")) {\n-- \n2.27.0.132.g321788e831\n\n"},{"id":"399693","messageId":"20200614083131.GD3405@danh.dev","threadId":"53670","inReplyTo":"d1b79c7734f0609fcac5e523644c3093f538bccf.1592119902.git.liu.denton@gmail.com","subject":"Re: [PATCH v2 2/3] pkt-line: use string versions of functions","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-06-14T08:31:31Z","receivedAt":"2020-06-14T08:31:38Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-06-14 03:31:59-0400, Denton Liu wrote:\n> We have many cases where we are writing a control packet as a string\n> constant out and we need to specify the length of the string. Currently,\n> the length is specified as a magical `4` literal.\n> \n> Change these instances to use a function that calls strlen() to\n> determine the length of the string removing the need to specify the\n> length at all. Since these functions are inline, the strlen()s should be\n> replaced with constants at compile-time so this should not result in any\n> performance penalty.\n> \n> Signed-off-by: Denton Liu \n> ---\n> pkt-line.c | 46 ++++++++++++++++++++++++++++------------------\n> 1 file changed, 28 insertions(+), 18 deletions(-)\n> \n> diff --git a/pkt-line.c b/pkt-line.c\n> index 8f9bc68ee2..72c6c29e03 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -81,49 +81,59 @@ static void packet_trace(const char *buf, unsigned int len, int write)\n> \tstrbuf_release(&out);\n> }\n> \n> +static inline void packet_trace_str(const char *buf, int write)\n> +{\n> +\tpacket_trace(buf, strlen(buf), write);\n> +}\n> +\n> +static inline void control_packet_write(int fd, const char *s, const char *type)\n> +{\n> +\tpacket_trace_str(s, 1);\n> +\tif (write_str_in_full(fd, s) < 0)\n> +\t\tdie_errno(_(\"unable to write %s packet\"), type);\n\nThis will create i10n problems:\n- Translators don't have enough context to know what does %s mean.\n In some languages, depend on value of %s, it will be translated to\n different phases by the order of words, word choices, gender.\n- `type' won't be translated with this marker\n\nI think it's better to pass full translated phase into this\nfunction. Something like:\n\n\tstatic inline void control_packet_write(int fd, const char *s, const char *errstr)\n\t{\n\t\t...\n\t\tif (...)\n\t\t\tdie_errno(errstr);\n\t}\n\nand call the function with:\n\n\tcontrol_packet_write(fd, \"0000\", _(\"unable to write flush packet\"));\n\nOther than that, I like the idea of using preprocessor to check\ncompile time constant string, but I'm not sure how to write it with\nstandard C\n\n-- \nDanh\n"},{"id":"399697","messageId":"da9ba5fb2d0fb9481f81ce525cbabaedd722858d.1592125442.git.liu.denton@gmail.com","threadId":"53670","inReplyTo":"20200614083131.GD3405@danh.dev","subject":"[PATCH v2] pkt-line: use string versions of functions","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-06-14T09:07:54Z","receivedAt":"2020-06-14T09:08:21Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"We have many cases where we are writing a control packet as a string\nconstant out and we need to specify the length of the string. Currently,\nthe length is specified as a magical `4` literal.\n\nChange these instances to use a function that calls strlen() to\ndetermine the length of the string removing the need to specify the\nlength at all. Since these functions are inline, the strlen()s should be\nreplaced with constants at compile-time so this should not result in any\nperformance penalty.\n\nSigned-off-by: Denton Liu \n---\nHi Đoàn,\n\nPerhaps something like this?\n\n pkt-line.c | 48 ++++++++++++++++++++++++++++++------------------\n 1 file changed, 30 insertions(+), 18 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 8f9bc68ee2..022376f00f 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -81,49 +81,61 @@ static void packet_trace(const char *buf, unsigned int len, int write)\n \tstrbuf_release(&out);\n }\n \n+static inline void packet_trace_str(const char *buf, int write)\n+{\n+\tpacket_trace(buf, strlen(buf), write);\n+}\n+\n+#define control_packet_write(fd, s, errstr) \\\n+\tdo { \\\n+\t\t(void)s\"is a string constant\"; \\\n+\t\tpacket_trace_str((s), 1); \\\n+\t\tif (write_str_in_full((fd), (s)) < 0) \\\n+\t\t\tdie_errno((errstr)); \\\n+\t} while (0)\n+\n /*\n * If we buffered things up above (we don't, but we should),\n * we'd flush it here\n */\n void packet_flush(int fd)\n {\n-\tpacket_trace(\"0000\", 4, 1);\n-\tif (write_in_full(fd, \"0000\", 4) < 0)\n-\t\tdie_errno(_(\"unable to write flush packet\"));\n+\tcontrol_packet_write(fd, \"0000\", _(\"unable to write flush packet\"));\n }\n \n void packet_delim(int fd)\n {\n-\tpacket_trace(\"0001\", 4, 1);\n-\tif (write_in_full(fd, \"0001\", 4) < 0)\n-\t\tdie_errno(_(\"unable to write delim packet\"));\n+\tcontrol_packet_write(fd, \"0001\", _(\"unable to write delim packet\"));\n }\n \n void packet_response_end(int fd)\n {\n-\tpacket_trace(\"0002\", 4, 1);\n-\tif (write_in_full(fd, \"0002\", 4) < 0)\n-\t\tdie_errno(_(\"unable to write stateless separator packet\"));\n+\tcontrol_packet_write(fd, \"0002\", _(\"unable to write stateless separator packet\"));\n }\n \n int packet_flush_gently(int fd)\n {\n-\tpacket_trace(\"0000\", 4, 1);\n-\tif (write_in_full(fd, \"0000\", 4) < 0)\n+\tpacket_trace_str(\"0000\", 1);\n+\tif (write_str_in_full(fd, \"0000\") < 0)\n \t\treturn error(_(\"flush packet write failed\"));\n \treturn 0;\n }\n \n+#define control_packet_buf_write(buf, s) \\\n+\tdo { \\\n+\t\t(void)s\"is a string constant\"; \\\n+\t\tpacket_trace_str((s), 1); \\\n+\t\tstrbuf_addstr((buf), (s)); \\\n+\t} while (0)\n+\n void packet_buf_flush(struct strbuf *buf)\n {\n-\tpacket_trace(\"0000\", 4, 1);\n-\tstrbuf_add(buf, \"0000\", 4);\n+\tcontrol_packet_buf_write(buf, \"0000\");\n }\n \n void packet_buf_delim(struct strbuf *buf)\n {\n-\tpacket_trace(\"0001\", 4, 1);\n-\tstrbuf_add(buf, \"0001\", 4);\n+\tcontrol_packet_buf_write(buf, \"0001\");\n }\n \n void set_packet_header(char *buf, int size)\n@@ -337,15 +349,15 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \tif (len < 0) {\n \t\tdie(_(\"protocol error: bad line length character: %.4s\"), linelen);\n \t} else if (!len) {\n-\t\tpacket_trace(\"0000\", 4, 0);\n+\t\tpacket_trace_str(\"0000\", 0);\n \t\t*pktlen = 0;\n \t\treturn PACKET_READ_FLUSH;\n \t} else if (len == 1) {\n-\t\tpacket_trace(\"0001\", 4, 0);\n+\t\tpacket_trace_str(\"0001\", 0);\n \t\t*pktlen = 0;\n \t\treturn PACKET_READ_DELIM;\n \t} else if (len == 2) {\n-\t\tpacket_trace(\"0002\", 4, 0);\n+\t\tpacket_trace_str(\"0002\", 0);\n \t\t*pktlen = 0;\n \t\treturn PACKET_READ_RESPONSE_END;\n \t} else if (len < 4) {\n-- \n2.27.0.132.g321788e831\n\n"},{"id":"399716","messageId":"xmqqtuzdy08l.fsf@gitster.c.googlers.com","threadId":"53670","inReplyTo":"xmqqimfuzz73.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] pkt-line: extract out PACKET_HEADER_SIZE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-14T18:24:10Z","receivedAt":"2020-06-14T18:24:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> How about (this ugly code):\n>>\n>> \tpacket_trace(\"0000\", sizeof \"0000\" - 1, 1);\n>> \tif (write_in_full(fd, \"0000\", sizeof \"0000\" - 1) < 0)\n>\n> Yeah, that is ugly. I was thinking more in the direction of\n> replacing these three-argument write_in_full with something like\n>\n> #define write_constant(fd, constant_string) \\\n> \twrite_in_full((fd), (constant_string), strlen(constant_string))\n>\n> with some preprocessor magic to make the compilation break when the\n> second parameter to the macro is not a string constant.\n\nThere is a bit of subtlety but I did mean C preprocessor macro and\nnot a helper function with the above. With use of a macro defined\nlike above, the programmer can write\n\n\twrite_constant(fd, \"0000\");\n\nwhich would turn into\n\n\twrite_in_full((fd), \"0000\", strlen(\"0000\"));\n\t\nDescent compilers know to produce identical code as\n\n\twrite_in_full((fd), \"0000\", 4);\n\nwhen seeing a literal constant string given to strlen().\n\nBut a helper function like this:\n\n\tstatic int write_constant(int fd, const char *string) {\n\t\treturn write_in_full(fd, string, strlen(string));\n\t}\n\nhas less chance of getting the same kind of optimization (the helper\nneeds to be inlined before the compiler can realize that the\nparameter to strlen() is a literal constant whose length can be\ncomputed at the compile time).\n\n\n"},{"id":"399730","messageId":"xmqqimftxri4.fsf@gitster.c.googlers.com","threadId":"53670","inReplyTo":"cover.1592119902.git.liu.denton@gmail.com","subject":"Re: [PATCH v2 0/3] pkt-line: war on magical `4` literal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-14T21:32:51Z","receivedAt":"2020-06-14T21:32:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> +static inline void control_packet_write(int fd, const char *s, const char *type)\n> +{\n> +\tpacket_trace_str(s, 1);\n> +\tif (write_str_in_full(fd, s) < 0)\n> +\t\tdie_errno(_(\"unable to write %s packet\"), type);\n\nThis construct, whether \"type\" is a localized string _(\"...\") or\nEnglish literal \"...\", would not translate well, would it? The\nformer causes sentence lego and the latter injects untranslated\nadjective that modifies the word \"packet\" in the final sentence.\n\nAs write_str_in_full() MUST count how long the string is at runtime,\nwithout any possibility to offload the work to compiler, when these\nstrings that are sent with the \"magical 4\" are all compile-time\nconstant literal strings, I am not sure if this is something we\nshould be happy with---we should aim better for a solution that does\nnot require the runtime to count if possible.\n\nThanks.\n"},{"id":"399731","messageId":"xmqqeeqhxred.fsf@gitster.c.googlers.com","threadId":"53670","inReplyTo":"da9ba5fb2d0fb9481f81ce525cbabaedd722858d.1592125442.git.liu.denton@gmail.com","subject":"Re: [PATCH v2] pkt-line: use string versions of functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-14T21:35:06Z","receivedAt":"2020-06-14T21:35:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> +#define control_packet_write(fd, s, errstr) \\\n> +\tdo { \\\n> +\t\t(void)s\"is a string constant\"; \\\n> +\t\tpacket_trace_str((s), 1); \\\n> +\t\tif (write_str_in_full((fd), (s)) < 0) \\\n> +\t\t\tdie_errno((errstr)); \\\n> +\t} while (0)\n> +\n\nOh, that's much better. If you go this route, drop your use of\nwrite_str_in_full(), but count the length of s with strlen() here\nto give the chance to the compilers to count the constant strings\nat compile time.\n\n> /*\n> * If we buffered things up above (we don't, but we should),\n> * we'd flush it here\n> */\n> void packet_flush(int fd)\n> {\n> -\tpacket_trace(\"0000\", 4, 1);\n> -\tif (write_in_full(fd, \"0000\", 4) < 0)\n> -\t\tdie_errno(_(\"unable to write flush packet\"));\n> +\tcontrol_packet_write(fd, \"0000\", _(\"unable to write flush packet\"));\n\n> +#define control_packet_buf_write(buf, s) \\\n> +\tdo { \\\n> +\t\t(void)s\"is a string constant\"; \\\n> +\t\tpacket_trace_str((s), 1); \\\n> +\t\tstrbuf_addstr((buf), (s)); \\\n> +\t} while (0)\n> +\n\nLikewise for strbuf_addstr().\n\n> void packet_buf_flush(struct strbuf *buf)\n> {\n> -\tpacket_trace(\"0000\", 4, 1);\n> -\tstrbuf_add(buf, \"0000\", 4);\n> +\tcontrol_packet_buf_write(buf, \"0000\");\n> }\n"},{"id":"399734","messageId":"20200614222828.GA4846@generichostname","threadId":"53670","inReplyTo":"xmqqeeqhxred.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] pkt-line: use string versions of functions","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-06-14T22:28:28Z","receivedAt":"2020-06-14T22:28:36Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Sun, Jun 14, 2020 at 02:35:06PM -0700, Junio C Hamano wrote:\n> Denton Liu writes:\n> \n> > +#define control_packet_write(fd, s, errstr) \\\n> > +\tdo { \\\n> > +\t\t(void)s\"is a string constant\"; \\\n> > +\t\tpacket_trace_str((s), 1); \\\n> > +\t\tif (write_str_in_full((fd), (s)) < 0) \\\n> > +\t\t\tdie_errno((errstr)); \\\n> > +\t} while (0)\n> > +\n> \n> Oh, that's much better. If you go this route, drop your use of\n> write_str_in_full(), but count the length of s with strlen() here\n> to give the chance to the compilers to count the constant strings\n> at compile time.\n> \n> > /*\n> > * If we buffered things up above (we don't, but we should),\n> > * we'd flush it here\n> > */\n> > void packet_flush(int fd)\n> > {\n> > -\tpacket_trace(\"0000\", 4, 1);\n> > -\tif (write_in_full(fd, \"0000\", 4) < 0)\n> > -\t\tdie_errno(_(\"unable to write flush packet\"));\n> > +\tcontrol_packet_write(fd, \"0000\", _(\"unable to write flush packet\"));\n> \n> > +#define control_packet_buf_write(buf, s) \\\n> > +\tdo { \\\n> > +\t\t(void)s\"is a string constant\"; \\\n> > +\t\tpacket_trace_str((s), 1); \\\n> > +\t\tstrbuf_addstr((buf), (s)); \\\n> > +\t} while (0)\n> > +\n> \n> Likewise for strbuf_addstr().\n> \n> > void packet_buf_flush(struct strbuf *buf)\n> > {\n> > -\tpacket_trace(\"0000\", 4, 1);\n> > -\tstrbuf_add(buf, \"0000\", 4);\n> > +\tcontrol_packet_buf_write(buf, \"0000\");\n> > }\n\nChecking the code, it seems like write_str_in_full() and strbuf_addstr()\nare both inline defined. I was under the impression that the compiler\nwill optimise out these strlen()s to be compile-time constants. In fact,\nstrbuf_addstr() has this comment\n\n\tNOTE: This function will *always* be implemented as an inline or a macro\n\tusing strlen, meaning that this is efficient to write things like:\n\nso if my assumption isn't true, I suspect that we'd need to change the\ncomment.\n"},{"id":"399766","messageId":"20200615123257.GA20389@danh.dev","threadId":"53670","inReplyTo":"da9ba5fb2d0fb9481f81ce525cbabaedd722858d.1592125442.git.liu.denton@gmail.com","subject":"Re: [PATCH v2] pkt-line: use string versions of functions","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-06-15T12:32:57Z","receivedAt":"2020-06-15T12:33:02Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-06-14 05:07:54-0400, Denton Liu wrote:\n> Hi Đoàn,\n\nSorry for this late reply, I have another stuff to work on.\n\n> Perhaps something like this?\n\nThis looks better, but ...\n\n> \n> pkt-line.c | 48 ++++++++++++++++++++++++++++++------------------\n> 1 file changed, 30 insertions(+), 18 deletions(-)\n> \n> diff --git a/pkt-line.c b/pkt-line.c\n> index 8f9bc68ee2..022376f00f 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -81,49 +81,61 @@ static void packet_trace(const char *buf, unsigned int len, int write)\n> \tstrbuf_release(&out);\n> }\n> \n> +static inline void packet_trace_str(const char *buf, int write)\n> +{\n> +\tpacket_trace(buf, strlen(buf), write);\n> +}\n> +\n> +#define control_packet_write(fd, s, errstr) \\\n> +\tdo { \\\n> +\t\t(void)s\"is a string constant\"; \\\n\nIf we are going to not wrap s inside () here,\n\n> +\t\tpacket_trace_str((s), 1); \\\n\nI see no point in wrapping it here.\nAnd wrapping around \"string\" is not standard C,\nsome (or most) compilers support it as an extension.\n\nSee USE_PARENS_AROUND_GETTEXT_N\n\nCan we just put it straight?\n\n\tpacket_trace(s, strlen(s), 1);\n\n> +\t\tif (write_str_in_full((fd), (s)) < 0) \\\n> +\t\t\tdie_errno((errstr)); \\\n> +\t} while (0)\n> +\n\n-- \nDanh\n"},{"id":"400410","messageId":"cover.1592934880.git.liu.denton@gmail.com","threadId":"53670","inReplyTo":"cover.1592119902.git.liu.denton@gmail.com","subject":"[PATCH v3 0/3] pkt-line: war on magical `4` literal","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-06-23T17:55:31Z","receivedAt":"2020-06-23T17:55:51Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"There are many instances of the literal `4` in packet line-related code.\nThis series replaces these instances with either functions that can\ngenerate the number or an appropriately-named constant.\n\nChanges since v1:\n\n* Introduce patch 1-2 so that the string length is used to generate the\n `4` where appropriate\n\nChanges since v2:\n\n* Replace some a couple of inline functions with macros that ensure they\n are string constants\n\n* Use strlen() in one more instance over the PACKET_HEADER_SIZE\n\nDenton Liu (3):\n remote-curl: use strlen() instead of magic numbers\n pkt-line: use string versions of functions\n pkt-line: extract out PACKET_HEADER_SIZE\n\n pkt-line.c | 68 ++++++++++++++++++++++++++++++---------------------\n pkt-line.h | 6 +++--\n remote-curl.c | 35 +++++++++++++-------------\n 3 files changed, 62 insertions(+), 47 deletions(-)\n\nRange-diff against v2:\n1: d2aaf15aa8 ! 1: cb8683837c remote-curl: use strlen() instead of magic numbers\n @@ Metadata\n ## Commit message ##\n remote-curl: use strlen() instead of magic numbers\n \n - When we are memcpy()ing the length header, we use the magic literal `4`,\n - representing the length of \"0000\" and \"0001\", the packet line length\n - headers. Use `strlen(\"000x\")` so that we do not have to use the magic\n - literal.\n + When we are dealing with a packet-line length header, we use the magic\n + literal `4`, representing the length of \"0000\" and \"0001\", the packet\n + line length headers. Use `strlen(\"000x\")` so that we do not have to use\n + the magic literal.\n \n ## remote-curl.c ##\n @@ remote-curl.c: static int rpc_read_from_out(struct rpc_state *rpc, int options,\n @@ remote-curl.c: static int rpc_read_from_out(struct rpc_state *rpc, int options,\n \t\t\tbreak;\n \t\tcase PACKET_READ_RESPONSE_END:\n \t\t\tdie(_(\"remote server sent stateless separator\"));\n +@@ remote-curl.c: static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)\n + \tcurl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url);\n + \tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, NULL);\n + \tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, \"0000\");\n +-\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, 4);\n ++\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, strlen(\"0000\"));\n + \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);\n + \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);\n + \tcurl_easy_setopt(slot->curl, CURLOPT_FILE, &buf);\n2: 8cab5078b1 ! 2: d283a1b514 pkt-line: use string versions of functions\n @@ Commit message\n replaced with constants at compile-time so this should not result in any\n performance penalty.\n \n +\n + ## Notes ##\n + Junio, you mentioned in an earlier email[0] that write_str_in_full() and\n + strbuf_addstr() each count the string length at runtime. However, I\n + don't think that's true since they're both inline functions and\n + strbuf_addstr() has the following comment:\n +\n + \t/**\n + \t * Add a NUL-terminated string to the buffer.\n + \t *\n + \t * NOTE: This function will *always* be implemented as an inline or a macro\n + \t * using strlen, meaning that this is efficient to write things like:\n + \t *\n + \t * strbuf_addstr(sb, \"immediate string\");\n + \t *\n + \t */\n +\n + so I believe that the lengths should be determined at compile-time.\n +\n + [0]: https://lore.kernel.org/git/xmqqeeqhxred.fsf@gitster.c.googlers.com/\n +\n ## pkt-line.c ##\n @@ pkt-line.c: static void packet_trace(const char *buf, unsigned int len, int write)\n \tstrbuf_release(&out);\n @@ pkt-line.c: static void packet_trace(const char *buf, unsigned int len, int writ\n +\tpacket_trace(buf, strlen(buf), write);\n +}\n +\n -+static inline void control_packet_write(int fd, const char *s, const char *type)\n -+{\n -+\tpacket_trace_str(s, 1);\n -+\tif (write_str_in_full(fd, s) < 0)\n -+\t\tdie_errno(_(\"unable to write %s packet\"), type);\n -+}\n ++#define control_packet_write(fd, s, errstr) \\\n ++\tdo { \\\n ++\t\t(void)s\"is a string constant\"; \\\n ++\t\tpacket_trace_str(s, 1); \\\n ++\t\tif (write_str_in_full((fd), s) < 0) \\\n ++\t\t\tdie_errno((errstr)); \\\n ++\t} while (0)\n +\n /*\n * If we buffered things up above (we don't, but we should),\n @@ pkt-line.c: static void packet_trace(const char *buf, unsigned int len, int writ\n -\tpacket_trace(\"0000\", 4, 1);\n -\tif (write_in_full(fd, \"0000\", 4) < 0)\n -\t\tdie_errno(_(\"unable to write flush packet\"));\n -+\tcontrol_packet_write(fd, \"0000\", \"flush\");\n ++\tcontrol_packet_write(fd, \"0000\", _(\"unable to write flush packet\"));\n }\n \n void packet_delim(int fd)\n @@ pkt-line.c: static void packet_trace(const char *buf, unsigned int len, int writ\n -\tpacket_trace(\"0001\", 4, 1);\n -\tif (write_in_full(fd, \"0001\", 4) < 0)\n -\t\tdie_errno(_(\"unable to write delim packet\"));\n -+\tcontrol_packet_write(fd, \"0001\", \"delim\");\n ++\tcontrol_packet_write(fd, \"0001\", _(\"unable to write delim packet\"));\n }\n \n void packet_response_end(int fd)\n @@ pkt-line.c: static void packet_trace(const char *buf, unsigned int len, int writ\n -\tpacket_trace(\"0002\", 4, 1);\n -\tif (write_in_full(fd, \"0002\", 4) < 0)\n -\t\tdie_errno(_(\"unable to write stateless separator packet\"));\n -+\tcontrol_packet_write(fd, \"0002\", \"stateless separator\");\n ++\tcontrol_packet_write(fd, \"0002\", _(\"unable to write stateless separator packet\"));\n }\n \n int packet_flush_gently(int fd)\n @@ pkt-line.c: static void packet_trace(const char *buf, unsigned int len, int writ\n \treturn 0;\n }\n \n -+static inline void control_packet_buf_write(struct strbuf *buf, const char *s)\n -+{\n -+\tpacket_trace_str(s, 1);\n -+\tstrbuf_addstr(buf, s);\n -+}\n ++#define control_packet_buf_write(buf, s) \\\n ++\tdo { \\\n ++\t\t(void)s\"is a string constant\"; \\\n ++\t\tpacket_trace_str(s, 1); \\\n ++\t\tstrbuf_addstr((buf), s); \\\n ++\t} while (0)\n +\n void packet_buf_flush(struct strbuf *buf)\n {\n3: 585d8892c3 ! 3: 00c19983fd pkt-line: extract out PACKET_HEADER_SIZE\n @@ remote-curl.c: static void check_pktline(struct check_pktline_state *state, cons\n \t\t\t\t}\n \t\t\t\tstate->len_filled = 0;\n \t\t\t}\n -@@ remote-curl.c: static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)\n - \tcurl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url);\n - \tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, NULL);\n - \tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, \"0000\");\n --\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, 4);\n -+\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, PACKET_HEADER_SIZE);\n - \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);\n - \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);\n - \tcurl_easy_setopt(slot->curl, CURLOPT_FILE, &buf);\n @@ remote-curl.c: int cmd_main(int argc, const char **argv)\n \t\t\tparse_fetch(&buf);\n \n-- \n2.27.0.307.g7979e895e7\n\n"},{"id":"400411","messageId":"cb8683837c9f583274780057621255a65a1c4c9f.1592934880.git.liu.denton@gmail.com","threadId":"53670","inReplyTo":"cover.1592934880.git.liu.denton@gmail.com","subject":"[PATCH v3 1/3] remote-curl: use strlen() instead of magic numbers","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-06-23T17:55:32Z","receivedAt":"2020-06-23T17:55:52Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"When we are dealing with a packet-line length header, we use the magic\nliteral `4`, representing the length of \"0000\" and \"0001\", the packet\nline length headers. Use `strlen(\"000x\")` so that we do not have to use\nthe magic literal.\n\nSigned-off-by: Denton Liu \n---\n remote-curl.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 75532a8bae..a2cbef546b 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -596,10 +596,10 @@ static int rpc_read_from_out(struct rpc_state *rpc, int options,\n \t\t\tset_packet_header(buf - 4, *appended);\n \t\t\tbreak;\n \t\tcase PACKET_READ_DELIM:\n-\t\t\tmemcpy(buf - 4, \"0001\", 4);\n+\t\t\tmemcpy(buf - strlen(\"0001\"), \"0001\", strlen(\"0001\"));\n \t\t\tbreak;\n \t\tcase PACKET_READ_FLUSH:\n-\t\t\tmemcpy(buf - 4, \"0000\", 4);\n+\t\t\tmemcpy(buf - strlen(\"0000\"), \"0000\", strlen(\"0000\"));\n \t\t\tbreak;\n \t\tcase PACKET_READ_RESPONSE_END:\n \t\t\tdie(_(\"remote server sent stateless separator\"));\n@@ -804,7 +804,7 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)\n \tcurl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url);\n \tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, NULL);\n \tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, \"0000\");\n-\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, 4);\n+\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, strlen(\"0000\"));\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);\n \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);\n \tcurl_easy_setopt(slot->curl, CURLOPT_FILE, &buf);\n-- \n2.27.0.307.g7979e895e7\n\n"},{"id":"400412","messageId":"d283a1b514c46ed75d88918f136ca0a6f4b90adc.1592934880.git.liu.denton@gmail.com","threadId":"53670","inReplyTo":"cover.1592934880.git.liu.denton@gmail.com","subject":"[PATCH v3 2/3] pkt-line: use string versions of functions","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-06-23T17:55:33Z","receivedAt":"2020-06-23T17:55:53Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"We have many cases where we are writing a control packet as a string\nconstant out and we need to specify the length of the string. Currently,\nthe length is specified as a magical `4` literal.\n\nChange these instances to use a function that calls strlen() to\ndetermine the length of the string removing the need to specify the\nlength at all. Since these functions are inline, the strlen()s should be\nreplaced with constants at compile-time so this should not result in any\nperformance penalty.\n\nSigned-off-by: Denton Liu \n---\n\nNotes:\n Junio, you mentioned in an earlier email[0] that write_str_in_full() and\n strbuf_addstr() each count the string length at runtime. However, I\n don't think that's true since they're both inline functions and\n strbuf_addstr() has the following comment:\n \n \t/**\n \t * Add a NUL-terminated string to the buffer.\n \t *\n \t * NOTE: This function will *always* be implemented as an inline or a macro\n \t * using strlen, meaning that this is efficient to write things like:\n \t *\n \t * strbuf_addstr(sb, \"immediate string\");\n \t *\n \t */\n \n so I believe that the lengths should be determined at compile-time.\n \n [0]: https://lore.kernel.org/git/xmqqeeqhxred.fsf@gitster.c.googlers.com/\n\n pkt-line.c | 48 ++++++++++++++++++++++++++++++------------------\n 1 file changed, 30 insertions(+), 18 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 8f9bc68ee2..e29f427b19 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -81,49 +81,61 @@ static void packet_trace(const char *buf, unsigned int len, int write)\n \tstrbuf_release(&out);\n }\n \n+static inline void packet_trace_str(const char *buf, int write)\n+{\n+\tpacket_trace(buf, strlen(buf), write);\n+}\n+\n+#define control_packet_write(fd, s, errstr) \\\n+\tdo { \\\n+\t\t(void)s\"is a string constant\"; \\\n+\t\tpacket_trace_str(s, 1); \\\n+\t\tif (write_str_in_full((fd), s) < 0) \\\n+\t\t\tdie_errno((errstr)); \\\n+\t} while (0)\n+\n /*\n * If we buffered things up above (we don't, but we should),\n * we'd flush it here\n */\n void packet_flush(int fd)\n {\n-\tpacket_trace(\"0000\", 4, 1);\n-\tif (write_in_full(fd, \"0000\", 4) < 0)\n-\t\tdie_errno(_(\"unable to write flush packet\"));\n+\tcontrol_packet_write(fd, \"0000\", _(\"unable to write flush packet\"));\n }\n \n void packet_delim(int fd)\n {\n-\tpacket_trace(\"0001\", 4, 1);\n-\tif (write_in_full(fd, \"0001\", 4) < 0)\n-\t\tdie_errno(_(\"unable to write delim packet\"));\n+\tcontrol_packet_write(fd, \"0001\", _(\"unable to write delim packet\"));\n }\n \n void packet_response_end(int fd)\n {\n-\tpacket_trace(\"0002\", 4, 1);\n-\tif (write_in_full(fd, \"0002\", 4) < 0)\n-\t\tdie_errno(_(\"unable to write stateless separator packet\"));\n+\tcontrol_packet_write(fd, \"0002\", _(\"unable to write stateless separator packet\"));\n }\n \n int packet_flush_gently(int fd)\n {\n-\tpacket_trace(\"0000\", 4, 1);\n-\tif (write_in_full(fd, \"0000\", 4) < 0)\n+\tpacket_trace_str(\"0000\", 1);\n+\tif (write_str_in_full(fd, \"0000\") < 0)\n \t\treturn error(_(\"flush packet write failed\"));\n \treturn 0;\n }\n \n+#define control_packet_buf_write(buf, s) \\\n+\tdo { \\\n+\t\t(void)s\"is a string constant\"; \\\n+\t\tpacket_trace_str(s, 1); \\\n+\t\tstrbuf_addstr((buf), s); \\\n+\t} while (0)\n+\n void packet_buf_flush(struct strbuf *buf)\n {\n-\tpacket_trace(\"0000\", 4, 1);\n-\tstrbuf_add(buf, \"0000\", 4);\n+\tcontrol_packet_buf_write(buf, \"0000\");\n }\n \n void packet_buf_delim(struct strbuf *buf)\n {\n-\tpacket_trace(\"0001\", 4, 1);\n-\tstrbuf_add(buf, \"0001\", 4);\n+\tcontrol_packet_buf_write(buf, \"0001\");\n }\n \n void set_packet_header(char *buf, int size)\n@@ -337,15 +349,15 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \tif (len < 0) {\n \t\tdie(_(\"protocol error: bad line length character: %.4s\"), linelen);\n \t} else if (!len) {\n-\t\tpacket_trace(\"0000\", 4, 0);\n+\t\tpacket_trace_str(\"0000\", 0);\n \t\t*pktlen = 0;\n \t\treturn PACKET_READ_FLUSH;\n \t} else if (len == 1) {\n-\t\tpacket_trace(\"0001\", 4, 0);\n+\t\tpacket_trace_str(\"0001\", 0);\n \t\t*pktlen = 0;\n \t\treturn PACKET_READ_DELIM;\n \t} else if (len == 2) {\n-\t\tpacket_trace(\"0002\", 4, 0);\n+\t\tpacket_trace_str(\"0002\", 0);\n \t\t*pktlen = 0;\n \t\treturn PACKET_READ_RESPONSE_END;\n \t} else if (len < 4) {\n-- \n2.27.0.307.g7979e895e7\n\n"},{"id":"400413","messageId":"00c19983fd99d7c335238725011e32785cb2b2a5.1592934880.git.liu.denton@gmail.com","threadId":"53670","inReplyTo":"cover.1592934880.git.liu.denton@gmail.com","subject":"[PATCH v3 3/3] pkt-line: extract out PACKET_HEADER_SIZE","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-06-23T17:55:34Z","receivedAt":"2020-06-23T17:55:55Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In pkt-line and remote-curl, we have many instances of magic `4`\nliterals floating around which represent the number of bytes in the\npacket line length header. Instead of using these magic numbers, replace\nthem with constant expressions.\n\nIn most cases, replace the `4` with `PACKET_HEADER_SIZE`. However, in\nthe case where there's a `char array[PACKET_HEADER_SIZE]` and we are\nreading data into it, replace the `4` with a `sizeof(array)` so that\nit's clear that the logic has something to do with that array.\n\nSigned-off-by: Denton Liu \n---\n pkt-line.c | 20 ++++++++++----------\n pkt-line.h | 6 ++++--\n remote-curl.c | 29 +++++++++++++++--------------\n 3 files changed, 29 insertions(+), 26 deletions(-)\n\ndiff --git a/pkt-line.c b/pkt-line.c\nindex e29f427b19..d451f0b67c 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -165,7 +165,7 @@ static void format_packet(struct strbuf *out, const char *prefix,\n \t\tdie(_(\"protocol error: impossibly long line\"));\n \n \tset_packet_header(&out->buf[orig_len], n);\n-\tpacket_trace(out->buf + orig_len + 4, n - 4, 1);\n+\tpacket_trace(out->buf + orig_len + PACKET_HEADER_SIZE, n - PACKET_HEADER_SIZE, 1);\n }\n \n static int packet_write_fmt_1(int fd, int gently, const char *prefix,\n@@ -211,13 +211,13 @@ static int packet_write_gently(const int fd_out, const char *buf, size_t size)\n \tstatic char packet_write_buffer[LARGE_PACKET_MAX];\n \tsize_t packet_size;\n \n-\tif (size > sizeof(packet_write_buffer) - 4)\n+\tif (size > sizeof(packet_write_buffer) - PACKET_HEADER_SIZE)\n \t\treturn error(_(\"packet write failed - data exceeds max packet size\"));\n \n \tpacket_trace(buf, size, 1);\n-\tpacket_size = size + 4;\n+\tpacket_size = size + PACKET_HEADER_SIZE;\n \tset_packet_header(packet_write_buffer, packet_size);\n-\tmemcpy(packet_write_buffer + 4, buf, size);\n+\tmemcpy(packet_write_buffer + PACKET_HEADER_SIZE, buf, size);\n \tif (write_in_full(fd_out, packet_write_buffer, packet_size) < 0)\n \t\treturn error(_(\"packet write failed\"));\n \treturn 0;\n@@ -325,7 +325,7 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size,\n \treturn ret;\n }\n \n-int packet_length(const char lenbuf_hex[4])\n+int packet_length(const char lenbuf_hex[PACKET_HEADER_SIZE])\n {\n \tint val = hex2chr(lenbuf_hex);\n \treturn (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);\n@@ -337,9 +337,9 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\t\t\t\t\tint options)\n {\n \tint len;\n-\tchar linelen[4];\n+\tchar linelen[PACKET_HEADER_SIZE];\n \n-\tif (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0) {\n+\tif (get_packet_data(fd, src_buffer, src_len, linelen, sizeof(linelen), options) < 0) {\n \t\t*pktlen = -1;\n \t\treturn PACKET_READ_EOF;\n \t}\n@@ -360,11 +360,11 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\tpacket_trace_str(\"0002\", 0);\n \t\t*pktlen = 0;\n \t\treturn PACKET_READ_RESPONSE_END;\n-\t} else if (len < 4) {\n+\t} else if (len < PACKET_HEADER_SIZE) {\n \t\tdie(_(\"protocol error: bad line length %d\"), len);\n \t}\n \n-\tlen -= 4;\n+\tlen -= sizeof(linelen);\n \tif ((unsigned)len >= size)\n \t\tdie(_(\"protocol error: bad line length %d\"), len);\n \n@@ -382,7 +382,7 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \n \tif ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&\n \t starts_with(buffer, \"ERR \"))\n-\t\tdie(_(\"remote error: %s\"), buffer + 4);\n+\t\tdie(_(\"remote error: %s\"), buffer + PACKET_HEADER_SIZE);\n \n \t*pktlen = len;\n \treturn PACKET_READ_NORMAL;\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 5b373fe4cd..d6121b8044 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -5,6 +5,8 @@\n #include \"strbuf.h\"\n #include \"sideband.h\"\n \n+#define PACKET_HEADER_SIZE 4\n+\n /*\n * Write a packetized stream, where each line is preceded by\n * its length (including the header) as a 4-byte hex number.\n@@ -82,7 +84,7 @@ int packet_read(int fd, char **src_buffer, size_t *src_len, char\n * If lenbuf_hex contains non-hex characters, return -1. Otherwise, return the\n * numeric value of the length header.\n */\n-int packet_length(const char lenbuf_hex[4]);\n+int packet_length(const char lenbuf_hex[PACKET_HEADER_SIZE]);\n \n /*\n * Read a packetized line into a buffer like the 'packet_read()' function but\n@@ -211,7 +213,7 @@ enum packet_read_status packet_reader_peek(struct packet_reader *reader);\n \n #define DEFAULT_PACKET_MAX 1000\n #define LARGE_PACKET_MAX 65520\n-#define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)\n+#define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - PACKET_HEADER_SIZE)\n extern char packet_buffer[LARGE_PACKET_MAX];\n \n struct packet_writer {\ndiff --git a/remote-curl.c b/remote-curl.c\nindex a2cbef546b..c99a2a514a 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -536,8 +536,8 @@ struct rpc_state {\n \tunsigned initial_buffer : 1;\n \n \t/*\n-\t * Whenever a pkt-line is read into buf, append the 4 characters\n-\t * denoting its length before appending the payload.\n+\t * Whenever a pkt-line is read into buf, append the PACKET_HEADER_SIZE\n+\t * characters denoting its length before appending the payload.\n \t */\n \tunsigned write_line_lengths : 1;\n \n@@ -556,8 +556,9 @@ struct rpc_state {\n * rpc->buf and rpc->len if there is enough space. Returns 1 if there was\n * enough space, 0 otherwise.\n *\n- * If rpc->write_line_lengths is true, appends the line length as a 4-byte\n- * hexadecimal string before appending the result described above.\n+ * If rpc->write_line_lengths is true, appends the line length as a\n+ * PACKET_HEADER_SIZE-byte hexadecimal string before appending the result\n+ * described above.\n *\n * Writes the total number of bytes appended into appended.\n */\n@@ -569,8 +570,8 @@ static int rpc_read_from_out(struct rpc_state *rpc, int options,\n \tint pktlen_raw;\n \n \tif (rpc->write_line_lengths) {\n-\t\tleft = rpc->alloc - rpc->len - 4;\n-\t\tbuf = rpc->buf + rpc->len + 4;\n+\t\tleft = rpc->alloc - rpc->len - PACKET_HEADER_SIZE;\n+\t\tbuf = rpc->buf + rpc->len + PACKET_HEADER_SIZE;\n \t} else {\n \t\tleft = rpc->alloc - rpc->len;\n \t\tbuf = rpc->buf + rpc->len;\n@@ -582,7 +583,7 @@ static int rpc_read_from_out(struct rpc_state *rpc, int options,\n \t*status = packet_read_with_status(rpc->out, NULL, NULL, buf,\n \t\t\tleft, &pktlen_raw, options);\n \tif (*status != PACKET_READ_EOF) {\n-\t\t*appended = pktlen_raw + (rpc->write_line_lengths ? 4 : 0);\n+\t\t*appended = pktlen_raw + (rpc->write_line_lengths ? PACKET_HEADER_SIZE : 0);\n \t\trpc->len += *appended;\n \t}\n \n@@ -593,7 +594,7 @@ static int rpc_read_from_out(struct rpc_state *rpc, int options,\n \t\t\t\tdie(_(\"shouldn't have EOF when not gentle on EOF\"));\n \t\t\tbreak;\n \t\tcase PACKET_READ_NORMAL:\n-\t\t\tset_packet_header(buf - 4, *appended);\n+\t\t\tset_packet_header(buf - PACKET_HEADER_SIZE, *appended);\n \t\t\tbreak;\n \t\tcase PACKET_READ_DELIM:\n \t\t\tmemcpy(buf - strlen(\"0001\"), \"0001\", strlen(\"0001\"));\n@@ -682,7 +683,7 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)\n #endif\n \n struct check_pktline_state {\n-\tchar len_buf[4];\n+\tchar len_buf[PACKET_HEADER_SIZE];\n \tint len_filled;\n \tint remaining;\n };\n@@ -691,7 +692,7 @@ static void check_pktline(struct check_pktline_state *state, const char *ptr, si\n {\n \twhile (size) {\n \t\tif (!state->remaining) {\n-\t\t\tint digits_remaining = 4 - state->len_filled;\n+\t\t\tint digits_remaining = sizeof(state->len_buf) - state->len_filled;\n \t\t\tif (digits_remaining > size)\n \t\t\t\tdigits_remaining = size;\n \t\t\tmemcpy(&state->len_buf[state->len_filled], ptr, digits_remaining);\n@@ -699,16 +700,16 @@ static void check_pktline(struct check_pktline_state *state, const char *ptr, si\n \t\t\tptr += digits_remaining;\n \t\t\tsize -= digits_remaining;\n \n-\t\t\tif (state->len_filled == 4) {\n+\t\t\tif (state->len_filled == sizeof(state->len_buf)) {\n \t\t\t\tstate->remaining = packet_length(state->len_buf);\n \t\t\t\tif (state->remaining < 0) {\n \t\t\t\t\tdie(_(\"remote-curl: bad line length character: %.4s\"), state->len_buf);\n \t\t\t\t} else if (state->remaining == 2) {\n \t\t\t\t\tdie(_(\"remote-curl: unexpected response end packet\"));\n-\t\t\t\t} else if (state->remaining < 4) {\n+\t\t\t\t} else if (state->remaining < sizeof(state->len_buf)) {\n \t\t\t\t\tstate->remaining = 0;\n \t\t\t\t} else {\n-\t\t\t\t\tstate->remaining -= 4;\n+\t\t\t\t\tstate->remaining -= sizeof(state->len_buf);\n \t\t\t\t}\n \t\t\t\tstate->len_filled = 0;\n \t\t\t}\n@@ -1469,7 +1470,7 @@ int cmd_main(int argc, const char **argv)\n \t\t\tparse_fetch(&buf);\n \n \t\t} else if (!strcmp(buf.buf, \"list\") || starts_with(buf.buf, \"list \")) {\n-\t\t\tint for_push = !!strstr(buf.buf + 4, \"for-push\");\n+\t\t\tint for_push = !!strstr(buf.buf + PACKET_HEADER_SIZE, \"for-push\");\n \t\t\toutput_refs(get_refs(for_push));\n \n \t\t} else if (starts_with(buf.buf, \"push \")) {\n-- \n2.27.0.307.g7979e895e7\n\n"},{"id":"400422","messageId":"20200623185458.GD1444619@coredump.intra.peff.net","threadId":"53670","inReplyTo":"cb8683837c9f583274780057621255a65a1c4c9f.1592934880.git.liu.denton@gmail.com","subject":"Re: [PATCH v3 1/3] remote-curl: use strlen() instead of magic numbers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-06-23T18:54:58Z","receivedAt":"2020-06-23T18:55:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 23, 2020 at 01:55:32PM -0400, Denton Liu wrote:\n\n> When we are dealing with a packet-line length header, we use the magic\n> literal `4`, representing the length of \"0000\" and \"0001\", the packet\n> line length headers. Use `strlen(\"000x\")` so that we do not have to use\n> the magic literal.\n\nI'm not a huge fan of using strlen() for this, because it's _still_\nmagical (you cannot change \"0000\" in one place without changing it in\nanother\"). And while it helps with understanding that \"4\" matches the\nlength of that string, IMHO it's harder to read because now I have to\nmake sure that those much longer strings all match up.\n\nThis refactoring also implies to me that if you changed all of \"0000\" on\none line you'd be fine, but that's emphatically not true. The magic\nnumber \"4\" is used to size the buffer earlier in the function, and would\nhave to match (and of course since this is a network protocol, it's not\nlike you could even change those in isolation).\n\nSo I dunno. I kind of think the raw \"4\" is the most readable. It's quite\nobvious to me in the context of a memcpy() what's going on. I don't mind\nmemcpy_literal() or similar that hides the repetition, but I think it's\nhard to do here because of the arithmetic on the destination.\n\n-Peff\n"},{"id":"400423","messageId":"20200623191143.GE1444619@coredump.intra.peff.net","threadId":"53670","inReplyTo":"d283a1b514c46ed75d88918f136ca0a6f4b90adc.1592934880.git.liu.denton@gmail.com","subject":"Re: [PATCH v3 2/3] pkt-line: use string versions of functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-06-23T19:11:43Z","receivedAt":"2020-06-23T19:11:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 23, 2020 at 01:55:33PM -0400, Denton Liu wrote:\n\n> void packet_flush(int fd)\n> {\n> -\tpacket_trace(\"0000\", 4, 1);\n> -\tif (write_in_full(fd, \"0000\", 4) < 0)\n> -\t\tdie_errno(_(\"unable to write flush packet\"));\n> +\tcontrol_packet_write(fd, \"0000\", _(\"unable to write flush packet\"));\n> }\n\nI like this kind of abstraction much more than what was going on in the\nprevious patch.\n\n> +#define control_packet_write(fd, s, errstr) \\\n> +\tdo { \\\n> +\t\t(void)s\"is a string constant\"; \\\n\nThis is a neat trick. It might be nice to wrap in\nBUILD_ASSERT_IS_STRING_LITERAL() or similar. (Though I wonder a bit if\nwe even really need to assert that; wouldn't it be OK to use this\nfunction without it? We're using strlen(), after all, and not sizeof).\n\nWould it also be useful to assert that the length of the control packet\nis 4? And likewise that it's less than 4? That seems much more\ninteresting to me (as we'd be violating the protocol otherwise). And\nthat would be easy to do if the we passed the packet number as an\ninteger and formatted it ourselves.\n\nBut the result gets kind of ugly:\n\n void control_packet_write(int fd, unsigned packet_id, const char *errstr)\n {\n\tchar buf[4 + 1];\n\n\tassert(packet_id < 4); /* >= 4 are actual data packets */\n\tvsnprintf(buf, sizeof(buf), \"%04u\", packet_id);\n\tif (write_in_full(fd, buf, 4))\n\t\t...;\n }\n\nThere are only 3 of these, and their whole point is to hide not just\nthis magic \"4\", but the whole idea of \"this is what a control packet\nlooks like\". I kind of wonder if the abstractions we need to reduce the\n3 lines to 1 is really making anything more readable.\n\n-Peff\n"},{"id":"400431","messageId":"xmqqtuz1bmgz.fsf@gitster.c.googlers.com","threadId":"53670","inReplyTo":"20200623185458.GD1444619@coredump.intra.peff.net","subject":"Re: [PATCH v3 1/3] remote-curl: use strlen() instead of magic numbers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-23T19:39:40Z","receivedAt":"2020-06-23T19:39:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Jun 23, 2020 at 01:55:32PM -0400, Denton Liu wrote:\n>\n>> When we are dealing with a packet-line length header, we use the magic\n>> literal `4`, representing the length of \"0000\" and \"0001\", the packet\n>> line length headers. Use `strlen(\"000x\")` so that we do not have to use\n>> the magic literal.\n>\n> I'm not a huge fan of using strlen() for this, because it's _still_\n> magical (you cannot change \"0000\" in one place without changing it in\n> another\"). And while it helps with understanding that \"4\" matches the\n> length of that string, IMHO it's harder to read because now I have to\n> make sure that those much longer strings all match up.\n\nYup. There are two instances of recurring pattern with two memcpy,\nwhere three copies of the same string must appear. Unless the whole\nthing is abstracted away so that these two instances are calls to\na macro/function that takes a single \"0000\" (and \"0001\"), I do not\nthink it is an improvement.\n\n> This refactoring also implies to me that if you changed all of \"0000\" on\n> one line you'd be fine, but that's emphatically not true. The magic\n> number \"4\" is used to size the buffer earlier in the function, and would\n> have to match (and of course since this is a network protocol, it's not\n> like you could even change those in isolation).\n\nThanks. That's even more important point.\n\n> So I dunno. I kind of think the raw \"4\" is the most readable. It's quite\n> obvious to me in the context of a memcpy() what's going on. I don't mind\n> memcpy_literal() or similar that hides the repetition, but I think it's\n> hard to do here because of the arithmetic on the destination.\n>\n> -Peff\n"}]}