Re: [PATCH v3 2/3] t0021: implementation the rot13-filter.pl script in C

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 9, 2022, 10:47 UTC
Message-ID: <9239s8np-69ss-n035-53s9-869s42p9srno@tzk.qr>
In-Reply-To: <86e6baba460f4d0fce353d1fb6a0e18b57ecadaa.1659291025.git.matheus.bernardino@usp.br>

Hi Matheus,

On Sun, 31 Jul 2022, Matheus Tavares wrote:

Show 20 quoted lines

> diff --git a/pkt-line.c b/pkt-line.c
> index 8e43c2def4..ce4e73b683 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -309,7 +309,8 @@ int write_packetized_from_fd_no_flush(int fd_in, int fd_out)
>  	return err;
>  }
>
> -int write_packetized_from_buf_no_flush(const char *src_in, size_t len, int fd_out)
> +int write_packetized_from_buf_no_flush_count(const char *src_in, size_t len,
> +					     int fd_out, int *packet_counter)
>  {
>  	int err = 0;
>  	size_t bytes_written = 0;
> @@ -324,6 +325,8 @@ int write_packetized_from_buf_no_flush(const char *src_in, size_t len, int fd_ou
>  			break;
>  		err = packet_write_gently(fd_out, src_in + bytes_written, bytes_to_write);
>  		bytes_written += bytes_to_write;
> +		if (packet_counter)
> +			(*packet_counter)++;

The only reason why we do this here is to try to imitate the Perl script that prints out a dot for every packet written, right?

But the Perl script wrote out those dots immediately and individually, not in one go after writing all the packets.

Unless the tests rely on the dots in the output, I would therefore recommend to simply scrap this functionality (and to write about it in the commit message, with the rationale that it does not fit into the current C code's paradigms and would require intrusive changes of questionable benefit) and avoid touching `pkt-line.[ch]` altogether.

Show 14 quoted lines

> [...]
> diff --git a/pkt-line.h b/pkt-line.h
> [...]
> +static void packet_initialize(void)
> +{
> +	int size;
> +	char *pkt_buf = packet_read_line(0, &size);
> +
> +	if (!pkt_buf || strncmp(pkt_buf, "git-filter-client", size))
> +		die("bad initialize: '%s'", xstrndup(pkt_buf, size));
> +
> +	pkt_buf = packet_read_line(0, &size);
> +	if (!pkt_buf || strncmp(pkt_buf, "version=2", size))
> +		die("bad version: '%.*s'", (int)size, pkt_buf);

This would mistake a packet `v` for being valid.

Junio pointed out in his review that `packet_read_line()` already NUL-terminates the buffer (except when it returns `NULL`), therefore we can write this instead:

	if (!pkt_buf || strcmp(pkt_buf, "version=2"))

Likewise with `"git-filter-client"`.

Show 22 quoted lines

> +
> +	pkt_buf = packet_read_line(0, &size);
> +	if (pkt_buf)
> +		die("bad version end: '%.*s'", (int)size, pkt_buf);
> +
> +	packet_write_fmt(1, "git-filter-server");
> +	packet_write_fmt(1, "version=2");
> +	packet_flush(1);
> +}
> +
> +static char *rot13_usage = "test-tool rot13-filter [--always-delay] <log path> <capabilities>";
> +
> +int cmd__rot13_filter(int argc, const char **argv)
> +{
> +	const char **caps;
> +	int cap_count, i = 1;
> +	struct strset remote_caps = STRSET_INIT;
> +
> +	if (argc > 1 && !strcmp(argv[1], "--always-delay")) {
> +		always_delay = 1;
> +		i++;
> +	}

This is so much simpler to read than if it used `parse_options()`, therefore I think that this is good as-is.

It is probably obvious that I did not spend as much time on reviewing this round as I did the previous time (after all, if one spends three hours here and three hours there, pretty soon one ends up having missed lunch before knowing it). However, it is equally obvious that you did a great job addressing my review of the previous round.

Thank you, Dscho