[v2,2/3] pkt-line: use string versions of functions

Commit Message

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

We have many cases where we are writing a control packet as a string
constant out and we need to specify the length of the string. Currently,
the length is specified as a magical `4` literal.

Change these instances to use a function that calls strlen() to
determine the length of the string removing the need to specify the
length at all. Since these functions are inline, the strlen()s should be
replaced with constants at compile-time so this should not result in any
performance penalty.

Signed-off-by: Denton Liu <[e-mail]>
---
 pkt-line.c | 46 ++++++++++++++++++++++++++++------------------
 1 file changed, 28 insertions(+), 18 deletions(-)

Comments

Đoàn Trần Công Danh June 14, 2020, 8:31 a.m. UTC | #1

On 2020-06-14 03:31:59-0400, Denton Liu <[e-mail]> wrote:
> We have many cases where we are writing a control packet as a string
> constant out and we need to specify the length of the string. Currently,
> the length is specified as a magical `4` literal.
> 
> Change these instances to use a function that calls strlen() to
> determine the length of the string removing the need to specify the
> length at all. Since these functions are inline, the strlen()s should be
> replaced with constants at compile-time so this should not result in any
> performance penalty.
> 
> Signed-off-by: Denton Liu <[e-mail]>
> ---
>  pkt-line.c | 46 ++++++++++++++++++++++++++++------------------
>  1 file changed, 28 insertions(+), 18 deletions(-)
> 
> diff --git a/pkt-line.c b/pkt-line.c
> index 8f9bc68ee2..72c6c29e03 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);

This will create i10n problems:
- Translators don't have enough context to know what does %s mean.
  In some languages, depend on value of %s, it will be translated to
  different phases by the order of words, word choices, gender.
- `type' won't be translated with this marker

I think it's better to pass full translated phase into this
function. Something like:

	static inline void control_packet_write(int fd, const char *s, const char *errstr)
	{
		...
		if (...)
			die_errno(errstr);
	}

and call the function with:

	control_packet_write(fd, "0000", _("unable to write flush packet"));

Other than that, I like the idea of using preprocessor to check
compile time constant string, but I'm not sure how to write it with
standard C

Patch

diff --git a/pkt-line.c b/pkt-line.c
index 8f9bc68ee2..72c6c29e03 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", 4, 1);
-	if (write_in_full(fd, "0000", 4) < 0)
-		die_errno(_("unable to write flush packet"));
+	control_packet_write(fd, "0000", "flush");
 }
 
 void packet_delim(int fd)
 {
-	packet_trace("0001", 4, 1);
-	if (write_in_full(fd, "0001", 4) < 0)
-		die_errno(_("unable to write delim packet"));
+	control_packet_write(fd, "0001", "delim");
 }
 
 void packet_response_end(int fd)
 {
-	packet_trace("0002", 4, 1);
-	if (write_in_full(fd, "0002", 4) < 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", 4, 1);
-	if (write_in_full(fd, "0000", 4) < 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", 4, 1);
-	strbuf_add(buf, "0000", 4);
+	control_packet_buf_write(buf, "0000");
 }
 
 void packet_buf_delim(struct strbuf *buf)
 {
-	packet_trace("0001", 4, 1);
-	strbuf_add(buf, "0001", 4);
+	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", 4, 0);
+		packet_trace_str("0000", 0);
 		*pktlen = 0;
 		return PACKET_READ_FLUSH;
 	} else if (len == 1) {
-		packet_trace("0001", 4, 0);
+		packet_trace_str("0001", 0);
 		*pktlen = 0;
 		return PACKET_READ_DELIM;
 	} else if (len == 2) {
-		packet_trace("0002", 4, 0);
+		packet_trace_str("0002", 0);
 		*pktlen = 0;
 		return PACKET_READ_RESPONSE_END;
 	} else if (len < 4) {