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

From: "Đoàn Trần Công Danh" <congdanhqx@gmail.com>
To: Denton Liu <liu.denton@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>, Jeff King <peff@peff.net>
Subject: Re: [PATCH v2 2/3] pkt-line: use string versions of functions
Date: Sun, 14 Jun 2020 15:31:31 +0700	[thread overview]
Message-ID: <20200614083131.GD3405@danh.dev> (raw)
In-Reply-To: <d1b79c7734f0609fcac5e523644c3093f538bccf.1592119902.git.liu.denton@gmail.com>

On 2020-06-14 03:31:59-0400, Denton Liu <liu.denton@gmail.com> 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 <liu.denton@gmail.com>
> ---
>  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

-- 
Danh