[PATCH 0/2] pkt-line.[ch]: dead code removal

[PATCH 0/2] pkt-line.[ch]: dead code removal

[Ævar Arnfjörð Bjarmason]

Remove some dead code in pkt-line.[ch], perhaps someone has an
objection to 2/2 as we could keep that function variant around "just
in case", but it's trivial to use the underlying function (or re-add
this utility), so shedding the unused code seems better.

Ævar Arnfjörð Bjarmason (2):
  pkt-line.[ch]: remove unused packet_buf_write_len()
  pkt-line.[ch]: remove unused packet_read_line_buf()

 builtin/checkout--worker.c |  4 ++--
 daemon.c                   |  2 +-
 parallel-checkout.c        |  3 +--
 pkt-line.c                 | 45 ++++++--------------------------------
 pkt-line.h                 | 10 +--------
 remote-curl.c              |  2 +-
 6 files changed, 13 insertions(+), 53 deletions(-)

-- 
2.33.1.1338.g20da966911a

[PATCH 1/2] pkt-line.[ch]: remove unused packet_buf_write_len()

[Ævar Arnfjörð Bjarmason]

This function was added in f1f4d8acf40 (pkt-line: add
packet_buf_write_len function, 2018-03-15) for use in
0f1dc53f45d (remote-curl: implement stateless-connect command,
2018-03-15).

In a97d00799a1 (remote-curl: use post_rpc() for protocol v2 also,
2019-02-21) that only user of it went away, let's remove it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 pkt-line.c | 16 ----------------
 pkt-line.h |  1 -
 2 files changed, 17 deletions(-)

diff --git a/pkt-line.c b/pkt-line.c
index de4a94b437e..11e1adc872b 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -289,22 +289,6 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
 	va_end(args);
 }
 
-void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len)
-{
-	size_t orig_len, n;
-
-	orig_len = buf->len;
-	strbuf_addstr(buf, "0000");
-	strbuf_add(buf, data, len);
-	n = buf->len - orig_len;
-
-	if (n > LARGE_PACKET_MAX)
-		die(_("protocol error: impossibly long line"));
-
-	set_packet_header(&buf->buf[orig_len], n);
-	packet_trace(data, len, 1);
-}
-
 int write_packetized_from_fd_no_flush(int fd_in, int fd_out)
 {
 	char *buf = xmalloc(LARGE_PACKET_DATA_MAX);
diff --git a/pkt-line.h b/pkt-line.h
index 82b95e4bdd3..beb589a8593 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -29,7 +29,6 @@ void packet_buf_delim(struct strbuf *buf);
 void set_packet_header(char *buf, int size);
 void packet_write(int fd_out, const char *buf, size_t size);
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
-void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len);
 int packet_flush_gently(int fd);
 int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
 int write_packetized_from_fd_no_flush(int fd_in, int fd_out);
-- 
2.33.1.1338.g20da966911a

[PATCH 2/2] pkt-line.[ch]: remove unused packet_read_line_buf()

[Ævar Arnfjörð Bjarmason]

This function was added in 4981fe750b1 (pkt-line: share
buffer/descriptor reading implementation, 2013-02-23), but in
01f9ec64c8a (Use packet_reader instead of packet_read_line,
2018-12-29) the code that was using it was removed.

Since it's being removed we can in turn remove the "src" and "src_len"
arguments to packet_read(), all the remaining users just passed a
NULL/NULL pair to it.

That function is only a thin wrapper for packet_read_with_status()
which still needs those arguments, but for the thin packet_read()
convenience wrapper we can do away with it for now.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/checkout--worker.c |  4 ++--
 daemon.c                   |  2 +-
 parallel-checkout.c        |  3 +--
 pkt-line.c                 | 29 +++++++----------------------
 pkt-line.h                 |  9 +--------
 remote-curl.c              |  2 +-
 6 files changed, 13 insertions(+), 36 deletions(-)

diff --git a/builtin/checkout--worker.c b/builtin/checkout--worker.c
index fb9fd13b73c..ede7dc32a43 100644
--- a/builtin/checkout--worker.c
+++ b/builtin/checkout--worker.c
@@ -82,8 +82,8 @@ static void worker_loop(struct checkout *state)
 	size_t i, nr = 0, alloc = 0;
 
 	while (1) {
-		int len = packet_read(0, NULL, NULL, packet_buffer,
-				      sizeof(packet_buffer), 0);
+		int len = packet_read(0, packet_buffer, sizeof(packet_buffer),
+				      0);
 
 		if (len < 0)
 			BUG("packet_read() returned negative value");
diff --git a/daemon.c b/daemon.c
index 5c4cbad62d0..3b5f9a41ab2 100644
--- a/daemon.c
+++ b/daemon.c
@@ -770,7 +770,7 @@ static int execute(void)
 
 	set_keep_alive(0);
 	alarm(init_timeout ? init_timeout : timeout);
-	pktlen = packet_read(0, NULL, NULL, packet_buffer, sizeof(packet_buffer), 0);
+	pktlen = packet_read(0, packet_buffer, sizeof(packet_buffer), 0);
 	alarm(0);
 
 	len = strlen(line);
diff --git a/parallel-checkout.c b/parallel-checkout.c
index ddc0ff3c064..ed9c9995207 100644
--- a/parallel-checkout.c
+++ b/parallel-checkout.c
@@ -603,8 +603,7 @@ static void gather_results_from_workers(struct pc_worker *workers,
 				continue;
 
 			if (pfd->revents & POLLIN) {
-				int len = packet_read(pfd->fd, NULL, NULL,
-						      packet_buffer,
+				int len = packet_read(pfd->fd, packet_buffer,
 						      sizeof(packet_buffer), 0);
 
 				if (len < 0) {
diff --git a/pkt-line.c b/pkt-line.c
index 11e1adc872b..2dc8ac274bd 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -437,38 +437,28 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 	return PACKET_READ_NORMAL;
 }
 
-int packet_read(int fd, char **src_buffer, size_t *src_len,
-		char *buffer, unsigned size, int options)
+int packet_read(int fd, char *buffer, unsigned size, int options)
 {
 	int pktlen = -1;
 
-	packet_read_with_status(fd, src_buffer, src_len, buffer, size,
-				&pktlen, options);
+	packet_read_with_status(fd, NULL, NULL, buffer, size, &pktlen,
+				options);
 
 	return pktlen;
 }
 
-static char *packet_read_line_generic(int fd,
-				      char **src, size_t *src_len,
-				      int *dst_len)
+char *packet_read_line(int fd, int *dst_len)
 {
-	int len = packet_read(fd, src, src_len,
-			      packet_buffer, sizeof(packet_buffer),
+	int len = packet_read(fd, packet_buffer, sizeof(packet_buffer),
 			      PACKET_READ_CHOMP_NEWLINE);
 	if (dst_len)
 		*dst_len = len;
 	return (len > 0) ? packet_buffer : NULL;
 }
 
-char *packet_read_line(int fd, int *len_p)
-{
-	return packet_read_line_generic(fd, NULL, NULL, len_p);
-}
-
 int packet_read_line_gently(int fd, int *dst_len, char **dst_line)
 {
-	int len = packet_read(fd, NULL, NULL,
-			      packet_buffer, sizeof(packet_buffer),
+	int len = packet_read(fd, packet_buffer, sizeof(packet_buffer),
 			      PACKET_READ_CHOMP_NEWLINE|PACKET_READ_GENTLE_ON_EOF);
 	if (dst_len)
 		*dst_len = len;
@@ -477,11 +467,6 @@ int packet_read_line_gently(int fd, int *dst_len, char **dst_line)
 	return len;
 }
 
-char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)
-{
-	return packet_read_line_generic(-1, src, src_len, dst_len);
-}
-
 ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out, int options)
 {
 	int packet_len;
@@ -491,7 +476,7 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out, int options)
 
 	for (;;) {
 		strbuf_grow(sb_out, LARGE_PACKET_DATA_MAX);
-		packet_len = packet_read(fd_in, NULL, NULL,
+		packet_len = packet_read(fd_in,
 			/* strbuf_grow() above always allocates one extra byte to
 			 * store a '\0' at the end of the string. packet_read()
 			 * writes a '\0' extra byte at the end, too. Let it know
diff --git a/pkt-line.h b/pkt-line.h
index beb589a8593..467ae013573 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -87,8 +87,7 @@ void packet_fflush(FILE *f);
 #define PACKET_READ_CHOMP_NEWLINE        (1u<<1)
 #define PACKET_READ_DIE_ON_ERR_PACKET    (1u<<2)
 #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)
-int packet_read(int fd, char **src_buffer, size_t *src_len, char
-		*buffer, unsigned size, int options);
+int packet_read(int fd, char *buffer, unsigned size, int options);
 
 /*
  * Convert a four hex digit packet line length header into its numeric
@@ -137,12 +136,6 @@ char *packet_read_line(int fd, int *size);
  */
 int packet_read_line_gently(int fd, int *size, char **dst_line);
 
-/*
- * Same as packet_read_line, but read from a buf rather than a descriptor;
- * see packet_read for details on how src_* is used.
- */
-char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);
-
 /*
  * Reads a stream of variable sized packets until a flush packet is detected.
  */
diff --git a/remote-curl.c b/remote-curl.c
index 5975103b96a..d69156312bd 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1088,7 +1088,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads,
 		rpc->protocol_header = NULL;
 
 	while (!err) {
-		int n = packet_read(rpc->out, NULL, NULL, rpc->buf, rpc->alloc, 0);
+		int n = packet_read(rpc->out, rpc->buf, rpc->alloc, 0);
 		if (!n)
 			break;
 		rpc->pos = 0;
-- 
2.33.1.1338.g20da966911a

Re: [PATCH 0/2] pkt-line.[ch]: dead code removal

[Jeff King]

On Thu, Oct 14, 2021 at 10:15:10PM +0200, Ævar Arnfjörð Bjarmason wrote:

> Remove some dead code in pkt-line.[ch], perhaps someone has an
> objection to 2/2 as we could keep that function variant around "just
> in case", but it's trivial to use the underlying function (or re-add
> this utility), so shedding the unused code seems better.

These both look good to me.

It's perhaps a little weird to shed the now-always-NULL src arguments
from packet_read() in the second one, since the underlying function we
wrap still allows them.  But it does make the callers a little simpler,
and I think if we added any new callers that I'd much rather see them
use the struct-oriented packet_reader interface instead. Which is really
why it we can get rid of this function in the first place; it's old callers
are all using that interface.

In the long run, I'd be quite happy if could get rid of all of the
non-packet_reader calls entirely, but that's a much bigger topic. This
is a nice incremental step in that direction.

-Peff