[v2,0/3] pkt-line: war on magical `4` literal

Commit Message

Denton Liu June 14, 2020, 7:31 a.m. UTC

There are many instances of the literal `4` in packet line-related code.
This series replaces these instances with either functions that can
generate the number or an appropriately-named constant.

Changes since v1:

* Introduce patch 1-2 so that the string length is used to generate the
  `4` where appropriate

Denton Liu (3):
  remote-curl: use strlen() instead of magic numbers
  pkt-line: use string versions of functions
  pkt-line: extract out PACKET_HEADER_SIZE

 pkt-line.c    | 66 +++++++++++++++++++++++++++++----------------------
 pkt-line.h    |  6 +++--
 remote-curl.c | 35 ++++++++++++++-------------
 3 files changed, 60 insertions(+), 47 deletions(-)

Interdiff against v1:

Comments

Junio C Hamano June 14, 2020, 9:32 p.m. UTC | #1

Denton Liu <[e-mail]> writes:

> +static inline void control_packet_write(int fd, const char *s, const char *type)
> +{
> +	packet_trace_str(s, 1);
> +	if (write_str_in_full(fd, s) < 0)
> +		die_errno(_("unable to write %s packet"), type);

This construct, whether "type" is a localized string _("...")  or
English literal "...", would not translate well, would it?  The
former causes sentence lego and the latter injects untranslated
adjective that modifies the word "packet" in the final sentence.

As write_str_in_full() MUST count how long the string is at runtime,
without any possibility to offload the work to compiler, when these
strings that are sent with the "magical 4" are all compile-time
constant literal strings, I am not sure if this is something we
should be happy with---we should aim better for a solution that does
not require the runtime to count if possible.

Thanks.

Patch

diff --git a/pkt-line.c b/pkt-line.c
index 245a56712f..f1fe0888c1 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -81,49 +81,59 @@  static void packet_trace(const char *buf, unsigned int len, int write)
 	strbuf_release(&out);
 }
 
+static inline void packet_trace_str(const char *buf, int write)
+{
+	packet_trace(buf, strlen(buf), write);
+}
+
+static inline void control_packet_write(int fd, const char *s, const char *type)
+{
+	packet_trace_str(s, 1);
+	if (write_str_in_full(fd, s) < 0)
+		die_errno(_("unable to write %s packet"), type);
+}
+
 /*
  * If we buffered things up above (we don't, but we should),
  * we'd flush it here
  */
 void packet_flush(int fd)
 {
-	packet_trace("0000", PACKET_HEADER_SIZE, 1);
-	if (write_in_full(fd, "0000", PACKET_HEADER_SIZE) < 0)
-		die_errno(_("unable to write flush packet"));
+	control_packet_write(fd, "0000", "flush");
 }
 
 void packet_delim(int fd)
 {
-	packet_trace("0001", PACKET_HEADER_SIZE, 1);
-	if (write_in_full(fd, "0001", PACKET_HEADER_SIZE) < 0)
-		die_errno(_("unable to write delim packet"));
+	control_packet_write(fd, "0001", "delim");
 }
 
 void packet_response_end(int fd)
 {
-	packet_trace("0002", PACKET_HEADER_SIZE, 1);
-	if (write_in_full(fd, "0002", PACKET_HEADER_SIZE) < 0)
-		die_errno(_("unable to write stateless separator packet"));
+	control_packet_write(fd, "0002", "stateless separator");
 }
 
 int packet_flush_gently(int fd)
 {
-	packet_trace("0000", PACKET_HEADER_SIZE, 1);
-	if (write_in_full(fd, "0000", PACKET_HEADER_SIZE) < 0)
+	packet_trace_str("0000", 1);
+	if (write_str_in_full(fd, "0000") < 0)
 		return error(_("flush packet write failed"));
 	return 0;
 }
 
+static inline void control_packet_buf_write(struct strbuf *buf, const char *s)
+{
+	packet_trace_str(s, 1);
+	strbuf_addstr(buf, s);
+}
+
 void packet_buf_flush(struct strbuf *buf)
 {
-	packet_trace("0000", PACKET_HEADER_SIZE, 1);
-	strbuf_add(buf, "0000", PACKET_HEADER_SIZE);
+	control_packet_buf_write(buf, "0000");
 }
 
 void packet_buf_delim(struct strbuf *buf)
 {
-	packet_trace("0001", PACKET_HEADER_SIZE, 1);
-	strbuf_add(buf, "0001", PACKET_HEADER_SIZE);
+	control_packet_buf_write(buf, "0001");
 }
 
 void set_packet_header(char *buf, int size)
@@ -337,15 +347,15 @@  enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 	if (len < 0) {
 		die(_("protocol error: bad line length character: %.4s"), linelen);
 	} else if (!len) {
-		packet_trace("0000", PACKET_HEADER_SIZE, 0);
+		packet_trace_str("0000", 0);
 		*pktlen = 0;
 		return PACKET_READ_FLUSH;
 	} else if (len == 1) {
-		packet_trace("0001", PACKET_HEADER_SIZE, 0);
+		packet_trace_str("0001", 0);
 		*pktlen = 0;
 		return PACKET_READ_DELIM;
 	} else if (len == 2) {
-		packet_trace("0002", PACKET_HEADER_SIZE, 0);
+		packet_trace_str("0002", 0);
 		*pktlen = 0;
 		return PACKET_READ_RESPONSE_END;
 	} else if (len < PACKET_HEADER_SIZE) {
diff --git a/remote-curl.c b/remote-curl.c
index bac295c5bc..7ba1280a41 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -536,8 +536,8 @@  struct rpc_state {
 	unsigned initial_buffer : 1;
 
 	/*
-	 * Whenever a pkt-line is read into buf, append the PACKET_HEADER_SIZE characters
-	 * denoting its length before appending the payload.
+	 * Whenever a pkt-line is read into buf, append the PACKET_HEADER_SIZE
+	 * characters denoting its length before appending the payload.
 	 */
 	unsigned write_line_lengths : 1;
 
@@ -556,8 +556,9 @@  struct rpc_state {
  * rpc->buf and rpc->len if there is enough space. Returns 1 if there was
  * enough space, 0 otherwise.
  *
- * If rpc->write_line_lengths is true, appends the line length as a PACKET_HEADER_SIZE-byte
- * hexadecimal string before appending the result described above.
+ * If rpc->write_line_lengths is true, appends the line length as a
+ * PACKET_HEADER_SIZE-byte hexadecimal string before appending the result
+ * described above.
  *
  * Writes the total number of bytes appended into appended.
  */
@@ -596,10 +597,10 @@  static int rpc_read_from_out(struct rpc_state *rpc, int options,
 			set_packet_header(buf - PACKET_HEADER_SIZE, *appended);
 			break;
 		case PACKET_READ_DELIM:
-			memcpy(buf - PACKET_HEADER_SIZE, "0001", PACKET_HEADER_SIZE);
+			memcpy(buf - strlen("0001"), "0001", strlen("0001"));
 			break;
 		case PACKET_READ_FLUSH:
-			memcpy(buf - PACKET_HEADER_SIZE, "0000", PACKET_HEADER_SIZE);
+			memcpy(buf - strlen("0000"), "0000", strlen("0000"));
 			break;
 		case PACKET_READ_RESPONSE_END:
 			die(_("remote server sent stateless separator"));