Re: [PATCH v2 04/14] pkt-line: optionally skip the flush packet in write_packetized_from_buf()

[Jeff King <peff@peff.net>]

On Mon, Feb 01, 2021 at 07:45:37PM +0000, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> This function currently has only one caller: `apply_multi_file_filter()`
> in `convert.c`. That caller wants a flush packet to be written after
> writing the payload.
> 
> However, we are about to introduce a user that wants to write many
> packets before a final flush packet, so let's extend this function to
> prepare for that scenario.

I think this is a sign that the function is not very well-designed in
the first place. It seems like the code would be easier to understand
overall if that caller just explicitly did the flush itself. It even
already does so in other cases!

Something like (untested):

 convert.c  | 4 ++++
 pkt-line.c | 4 ----
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/convert.c b/convert.c
index ee360c2f07..3968ac37b9 100644
--- a/convert.c
+++ b/convert.c
@@ -890,6 +890,10 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
 	if (err)
 		goto done;
 
+	err = packet_flush_gently(process->in);
+	if (err)
+		goto done;
+
 	err = subprocess_read_status(process->out, &filter_status);
 	if (err)
 		goto done;
diff --git a/pkt-line.c b/pkt-line.c
index d633005ef7..014520a9c2 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -256,8 +256,6 @@ int write_packetized_from_fd(int fd_in, int fd_out)
 			break;
 		err = packet_write_gently(fd_out, buf, bytes_to_write);
 	}
-	if (!err)
-		err = packet_flush_gently(fd_out);
 	return err;
 }
 
@@ -277,8 +275,6 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out)
 		err = packet_write_gently(fd_out, src_in + bytes_written, bytes_to_write);
 		bytes_written += bytes_to_write;
 	}
-	if (!err)
-		err = packet_flush_gently(fd_out);
 	return err;
 }
 

-Peff