diff mbox series

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

Message ID 0832f7d324da643d7a480111d693ff5559c2b7a7.1612208747.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Simple IPC Mechanism | expand

Commit Message

Johannes Schindelin Feb. 1, 2021, 7:45 p.m. UTC 
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.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 convert.c  | 2 +-
 pkt-line.c | 9 ++++++---
 pkt-line.h | 4 +++-
 3 files changed, 10 insertions(+), 5 deletions(-)

Comments

Jeff King Feb. 2, 2021, 9:48 a.m. UTC | #1 
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
Johannes Schindelin Feb. 2, 2021, 10:56 p.m. UTC | #2 
Hi Peff,


On Tue, 2 Feb 2021, Jeff King wrote:

> 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):

Fine by me.

Thanks,
Dscho

>
>  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
>
Jeff Hostetler Feb. 5, 2021, 6:30 p.m. UTC | #3 
On 2/2/21 4:48 AM, Jeff King wrote:
> 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!
> 

I agree.  I'll move flush to the caller and rename the write packetized
function slightly to guard against new callers assuming the old behavior
during the transition.

Jeff


> 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
>
diff mbox series

Patch

diff --git a/convert.c b/convert.c
index ee360c2f07c..3f396a9b288 100644
--- a/convert.c
+++ b/convert.c
@@ -886,7 +886,7 @@  static int apply_multi_file_filter(const char *path, const char *src, size_t len
 	if (fd >= 0)
 		err = write_packetized_from_fd(fd, process->in);
 	else
-		err = write_packetized_from_buf(src, len, process->in);
+		err = write_packetized_from_buf(src, len, process->in, 1);
 	if (err)
 		goto done;
 
diff --git a/pkt-line.c b/pkt-line.c
index 5d86354cbeb..d91a1deda95 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -275,14 +275,17 @@  int write_packetized_from_fd(int fd_in, int fd_out)
 	return err;
 }
 
-int write_packetized_from_buf(const char *src_in, size_t len, int fd_out)
+int write_packetized_from_buf(const char *src_in, size_t len, int fd_out,
+			      int flush_at_end)
 {
 	static struct packet_scratch_space scratch;
 
-	return write_packetized_from_buf2(src_in, len, fd_out, &scratch);
+	return write_packetized_from_buf2(src_in, len, fd_out,
+					  flush_at_end, &scratch);
 }
 
 int write_packetized_from_buf2(const char *src_in, size_t len, int fd_out,
+			       int flush_at_end,
 			       struct packet_scratch_space *scratch)
 {
 	int err = 0;
@@ -299,7 +302,7 @@  int write_packetized_from_buf2(const char *src_in, size_t len, int fd_out,
 		err = packet_write_gently(fd_out, src_in + bytes_written, bytes_to_write, scratch);
 		bytes_written += bytes_to_write;
 	}
-	if (!err)
+	if (!err && flush_at_end)
 		err = packet_flush_gently(fd_out);
 	return err;
 }
diff --git a/pkt-line.h b/pkt-line.h
index f1d5625e91f..ccf27549227 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -40,8 +40,10 @@  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(int fd_in, int fd_out);
-int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
+int write_packetized_from_buf(const char *src_in, size_t len, int fd_out,
+			      int flush_at_end);
 int write_packetized_from_buf2(const char *src_in, size_t len, int fd_out,
+			       int flush_at_end,
 			       struct packet_scratch_space *scratch);
 
 /*