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

From

Denton Liu <liu.denton@gmail.com>

Date

Jun 14, 2020, 07:31 UTC

Message-ID

<cover.1592119902.git.liu.denton@gmail.com>

In-Reply-To

<7e803a2ba9458ce35c657e67323edfe4409205ec.1592055716.git.liu.denton@gmail.com>

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:

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"));

-- 
2.27.0.132.g321788e831