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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Matheus,

On Sun, 31 Jul 2022, Matheus Tavares wrote:

> 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.

> [...]
> 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"`.

> +
> +	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