Re: [PATCH v2] t/t0021: convert the rot13-filter.pl script to C
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Jul 28, 2022, 16:58 UTC
- Message-ID
- <4n20476q-6ssr-osp8-q5o3-p8ns726q4pn3@tzk.qr>
- In-Reply-To
- <f38f722de7c3323207eda5ea632b5acd3765c285.1658675222.git.matheus.bernardino@usp.br>
Hi Matheus,
On Sun, 24 Jul 2022, Matheus Tavares wrote:
Show 5 quoted lines
> This script is currently used by three test files: t0021-conversion.sh, > t2080-parallel-checkout-basics.sh, and > t2082-parallel-checkout-attributes.sh. To avoid the need for the PERL > dependency at these tests, let's convert the script to a C test-tool > command.
Great!
> - Squashed the two patches together.
I see why this might have been suggested, but it definitely made it more challenging for me to review. You see, it is easy to just fly over a patch that simply removes the `PERL` prereq, but it is much harder to jump back and forth over all of these removals when the `.c` version of the filter is added before them and the `.pl` version is removed after them. So I find that it was bad advice, but I do not fault you for following it (we all want reviews to just be over already and therefore sometimes pander to the reviewers, no matter how much or little sense their feedback makes).
It just would have been easier for me to review if the chaff was separated from the wheat, so to say.
To illustrate my point: it was a bit of a challenge to find the adjustment of the "smudge write error at" needle in all of that cruft. It would have made my life as a reviewer substantially easier had the patch series been organized this way (which I assume you had before the feedback you received demanded to squash everything in one hot pile):
1/3 adjust the needle for the error message 2/3 implement the rot13-filter in C 3/3 use the test-tool in the tests and remove the PERL prereq, and remove rot13-filter.pl
Show 26 quoted lines
> [...]
> diff --git a/pkt-line.c b/pkt-line.c
> index 8e43c2def4..4425bdae36 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -309,9 +309,10 @@ 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 *count_ptr)
> {
> - int err = 0;
> + int err = 0, count = 0;
> size_t bytes_written = 0;
> size_t bytes_to_write;
>
> @@ -324,10 +325,18 @@ 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;
> + count++;
> }
> + if (count_ptr)
> + *count_ptr = count;This is not just a counter, but a packet counter, right? In any case, it would probably make more sense to increment the value directly:
if (count_ptr) (*count_ptr)++;
More on that below, where you use it.
Show 7 quoted lines
> return err;
> }
>
> +int write_packetized_from_buf_no_flush(const char *src_in, size_t len, int fd_out)
> +{
> + return write_packetized_from_buf_no_flush_count(src_in, len, fd_out, NULL);
> +}Have you considered making this a `static inline` in `pkt-line.h`?
Show 68 quoted lines
> [...]
> diff --git a/t/helper/test-rot13-filter.c b/t/helper/test-rot13-filter.c
> new file mode 100644
> index 0000000000..536111f272
> --- /dev/null
> +++ b/t/helper/test-rot13-filter.c
> @@ -0,0 +1,393 @@
> +/*
> + * Example implementation for the Git filter protocol version 2
> + * See Documentation/gitattributes.txt, section "Filter Protocol"
> + *
> + * Usage: test-tool rot13-filter [--always-delay] <log path> <capabilities>
> + *
> + * Log path defines a debug log file that the script writes to. The
> + * subsequent arguments define a list of supported protocol capabilities
> + * ("clean", "smudge", etc).
> + *
> + * When --always-delay is given all pathnames with the "can-delay" flag
> + * that don't appear on the list bellow are delayed with a count of 1
> + * (see more below).
> + *
> + * This implementation supports special test cases:
> + * (1) If data with the pathname "clean-write-fail.r" is processed with
> + * a "clean" operation then the write operation will die.
> + * (2) If data with the pathname "smudge-write-fail.r" is processed with
> + * a "smudge" operation then the write operation will die.
> + * (3) If data with the pathname "error.r" is processed with any
> + * operation then the filter signals that it cannot or does not want
> + * to process the file.
> + * (4) If data with the pathname "abort.r" is processed with any
> + * operation then the filter signals that it cannot or does not want
> + * to process the file and any file after that is processed with the
> + * same command.
> + * (5) If data with a pathname that is a key in the delay hash is
> + * requested (e.g. "test-delay10.a") then the filter responds with
> + * a "delay" status and sets the "requested" field in the delay hash.
> + * The filter will signal the availability of this object after
> + * "count" (field in delay hash) "list_available_blobs" commands.
> + * (6) If data with the pathname "missing-delay.a" is processed that the
> + * filter will drop the path from the "list_available_blobs" response.
> + * (7) If data with the pathname "invalid-delay.a" is processed that the
> + * filter will add the path "unfiltered" which was not delayed before
> + * to the "list_available_blobs" response.
> + */
> +
> +#include "test-tool.h"
> +#include "pkt-line.h"
> +#include "string-list.h"
> +#include "strmap.h"
> +
> +static FILE *logfile;
> +static int always_delay;
> +static struct strmap delay = STRMAP_INIT;
> +static struct string_list requested_caps = STRING_LIST_INIT_NODUP;
> +
> +static int has_capability(const char *cap)
> +{
> + return unsorted_string_list_has_string(&requested_caps, cap);
> +}
> +
> +static char *rot13(char *str)
> +{
> + char *c;
> + for (c = str; *c; c++) {
> + if (*c >= 'a' && *c <= 'z')
> + *c = 'a' + (*c - 'a' + 13) % 26;
> + else if (*c >= 'A' && *c <= 'Z')
> + *c = 'A' + (*c - 'A' + 13) % 26;That's quite verbose, but it _is_ correct (if a bit harder than necessary to validate, I admit that I had to look up whether `%`'s precedence is higher than `+` in https://en.cppreference.com/w/c/language/operator_precedence).
A conciser way (also easier to reason about):
for (c = str; *c; c++) if (isalpha(*c)) *c += tolower(*c) < 'n' ? 13 : -13;
For fun, you could also look at https://hea-www.harvard.edu/~fine/Tech/rot13.html whether you want to use yet another approach.
Show 12 quoted lines
> + }
> + return str;
> +}
> +
> +static char *skip_key_dup(const char *buf, size_t size, const char *key)
> +{
> + struct strbuf keybuf = STRBUF_INIT;
> + strbuf_addf(&keybuf, "%s=", key);
> + if (!skip_prefix_mem(buf, size, keybuf.buf, &buf, &size) || !size)
> + die("bad %s: '%s'", key, xstrndup(buf, size));
> + strbuf_release(&keybuf);
> + return xstrndup(buf, size);This does what we want it to do, but it looks as if it was code translated from a language that does not care one bit about allocations to a language that cares a lot.
For example, instead of allocating a `strbuf` just to append `=` to the key, in idiomatic C this code would read like this:
static char *get_value(char *buf, size_t size, const char *key)
{
const char *orig_buf = buf;
int orig_size = (int)size; if (!skip_prefix_mem(buf, size, key, &buf, &size) ||
!skip_prefix_mem(buf, size, "=", &buf, &size) ||
!size)
die("expected key '%s', got '%.*s'",
key, orig_size, orig_buf);return xstrndup(buf, size); }
I was tempted, even, to suggest returning a `const char *` after NUL-terminating the line (via `buf[size] = '\0';`) instead of `xstrndup()`ing it, but `packet_read_line()` reads into the singleton `packet_buffer` and we use e.g. the `command` that is returned from this function after reading the next packet, so the command would most likely be overwritten.
Show 20 quoted lines
> +}
> +
> +/*
> + * Read a text packet, expecting that it is in the form "key=value" for
> + * the given key. An EOF does not trigger any error and is reported
> + * back to the caller with NULL. Die if the "key" part of "key=value" does
> + * not match the given key, or the value part is empty.
> + */
> +static char *packet_key_val_read(const char *key)
> +{
> + int size;
> + char *buf;
> + if (packet_read_line_gently(0, &size, &buf) < 0)
> + return NULL;
> + return skip_key_dup(buf, size, key);
> +}
> +
> +static void packet_read_capabilities(struct string_list *caps)
> +{
> + while (1) {In Git's source code, I think we prefer `for (;;)`. But not by much:
$ git grep 'while (1)' \*.c | wc
128 508 3745$ git grep 'for (;;)' \*.c | wc
156 614 4389Show 6 quoted lines
> + int size; > + char *buf = packet_read_line(0, &size); > + if (!buf) > + break; > + string_list_append_nodup(caps, > + skip_key_dup(buf, size, "capability"));
It is tempting to use unsorted string lists for everything because Perl makes that relatively easy.
However, in this instance I would strongly recommend using something more akin to Perl's "hash" data structure, in this instance a `strset`.
Show 16 quoted lines
> + }
> +}
> +
> +/* Read remote capabilities and check them against capabilities we require */
> +static void packet_read_and_check_capabilities(struct string_list *remote_caps,
> + struct string_list *required_caps)
> +{
> + struct string_list_item *item;
> + packet_read_capabilities(remote_caps);
> + for_each_string_list_item(item, required_caps) {
> + if (!unsorted_string_list_has_string(remote_caps, item->string)) {
> + die("required '%s' capability not available from remote",
> + item->string);
> + }
> + }
> +}This is a pretty literal translation from Perl to C, and a couple of years ago, I would have done the same.
However, these days I would recommend against it. In this instance, we are really only interested in three capabilities: clean, smudge and delay. It is much, much simpler to read in the capabilities and then manually verify that the three required ones were included:
static void read_capabilities(struct strset *remote_caps)
{
char *cap
while ((cap = packet_key_val_read("capability")))
strset_add(remote_caps, cap); if (!strset_contains(remote_caps, "clean"))
die("required 'clean' capability not available from remote");
if (!strset_contains(remote_caps, "smudge"))
die("required 'smudge' capability not available from remote");
if (!strset_contains(remote_caps, "delay"))
die("required 'delay' capability not available from remote");
}Show 7 quoted lines
> + > +/* > + * Check our capabilities we want to advertise against the remote ones > + * and then advertise our capabilities > + */ > +static void packet_check_and_write_capabilities(struct string_list *remote_caps, > + struct string_list *our_caps)
The list of "our caps" comes from the command-line. In C, this means we get a `const char **argv` and an `int argc`. So:
static void check_and_write_capabilities(struct strset *remote_caps,
const char **caps, int caps_count)
{
int i; for (i = 0; i < caps_count; i++) {
if (!strset_contains(remote_caps, caps[i]))
die("our capability '%s' is not available from remote",
caps[i]);packet_write_fmt(1, "capability=%s\n", caps[i]); } packet_flush(1); }
And then we would call it via
check_and_write_capabilities(remote_caps, argv + 1, argc - 1);
Show 5 quoted lines
> +
> +struct delay_entry {
> + int requested, count;
> + char *output;
> +};Since you declare this here, it makes most sense to define `free_delay_hash()` (which should really be named `free_delay_entries()`) and `add_delay_entry()` here.
Show 10 quoted lines
> +
> +static void command_loop(void)
> +{
> + while (1) {
> + char *command = packet_key_val_read("command");
> + if (!command) {
> + fprintf(logfile, "STOP\n");
> + break;
> + }
> + fprintf(logfile, "IN: %s", command);We will also need to `fflush(logfile)` here, to imitate the Perl script's behavior more precisely.
Show 29 quoted lines
> +
> + if (!strcmp(command, "list_available_blobs")) {
> + struct hashmap_iter iter;
> + struct strmap_entry *ent;
> + struct string_list_item *str_item;
> + struct string_list paths = STRING_LIST_INIT_NODUP;
> +
> + /* flush */
> + if (packet_read_line(0, NULL))
> + die("bad list_available_blobs end");
> +
> + strmap_for_each_entry(&delay, &iter, ent) {
> + struct delay_entry *delay_entry = ent->value;
> + if (!delay_entry->requested)
> + continue;
> + delay_entry->count--;
> + if (!strcmp(ent->key, "invalid-delay.a")) {
> + /* Send Git a pathname that was not delayed earlier */
> + packet_write_fmt(1, "pathname=unfiltered");
> + }
> + if (!strcmp(ent->key, "missing-delay.a")) {
> + /* Do not signal Git that this file is available */
> + } else if (!delay_entry->count) {
> + string_list_insert(&paths, ent->key);
> + packet_write_fmt(1, "pathname=%s", ent->key);
> + }
> + }
> +
> + /* Print paths in sorted order. */The Perl script does not order them specifically. Do we really have to do that here?
In any case, it is more performant to append the paths in an unsorted way and then sort them once in the end (that's O(N log(N)) instead of O(N^2)).
Show 9 quoted lines
> + for_each_string_list_item(str_item, &paths) > + fprintf(logfile, " %s", str_item->string); > + string_list_clear(&paths, 0); > + > + packet_flush(1); > + > + fprintf(logfile, " [OK]\n"); > + packet_write_fmt(1, "status=success"); > + packet_flush(1);
I know the Perl script uses an else here, but I'd much rather insert a `continue` at the end of the `list_available_blobs` clause and de-indent the remainder of the loop body.
Show 11 quoted lines
> + } else {
> + char *buf, *output;
> + int size;
> + char *pathname;
> + struct delay_entry *entry;
> + struct strbuf input = STRBUF_INIT;
> +
> + pathname = packet_key_val_read("pathname");
> + if (!pathname)
> + die("unexpected EOF while expecting pathname");
> + fprintf(logfile, " %s", pathname);Again, let's `fflush(logfile)` here.
> +
> + /* Read until flush */
> + buf = packet_read_line(0, &size);
> + while (buf) {Let's write this in more idiomatic C:
while ((buf = packet_read_line(0, &size))) {Show 9 quoted lines
> + if (!strcmp(buf, "can-delay=1")) {
> + entry = strmap_get(&delay, pathname);
> + if (entry && !entry->requested) {
> + entry->requested = 1;
> + } else if (!entry && always_delay) {
> + entry = xcalloc(1, sizeof(*entry));
> + entry->requested = 1;
> + entry->count = 1;
> + strmap_put(&delay, pathname, entry);I guess here is our chance to extend the signature of `add_delay_entry()` to accept a `requested` parameter, and to call that here.
Show 20 quoted lines
> + }
> + } else if (starts_with(buf, "ref=") ||
> + starts_with(buf, "treeish=") ||
> + starts_with(buf, "blob=")) {
> + fprintf(logfile, " %s", buf);
> + } else {
> + /*
> + * In general, filters need to be graceful about
> + * new metadata, since it's documented that we
> + * can pass any key-value pairs, but for tests,
> + * let's be a little stricter.
> + */
> + die("Unknown message '%s'", buf);
> + }
> + buf = packet_read_line(0, &size);
> + }
> +
> +
> + read_packetized_to_strbuf(0, &input, 0);
> + fprintf(logfile, " %"PRIuMAX" [OK] -- ", (uintmax_t)input.len);This reads _so much nicer_ than the Perl version!
Show 30 quoted lines
> +
> + entry = strmap_get(&delay, pathname);
> + if (entry && entry->output) {
> + output = entry->output;
> + } else if (!strcmp(pathname, "error.r") || !strcmp(pathname, "abort.r")) {
> + output = "";
> + } else if (!strcmp(command, "clean") && has_capability("clean")) {
> + output = rot13(input.buf);
> + } else if (!strcmp(command, "smudge") && has_capability("smudge")) {
> + output = rot13(input.buf);
> + } else {
> + die("bad command '%s'", command);
> + }
> +
> + if (!strcmp(pathname, "error.r")) {
> + fprintf(logfile, "[ERROR]\n");
> + packet_write_fmt(1, "status=error");
> + packet_flush(1);
> + } else if (!strcmp(pathname, "abort.r")) {
> + fprintf(logfile, "[ABORT]\n");
> + packet_write_fmt(1, "status=abort");
> + packet_flush(1);
> + } else if (!strcmp(command, "smudge") &&
> + (entry = strmap_get(&delay, pathname)) &&
> + entry->requested == 1) {
> + fprintf(logfile, "[DELAYED]\n");
> + packet_write_fmt(1, "status=delayed");
> + packet_flush(1);
> + entry->requested = 2;
> + entry->output = xstrdup(output);We need to call `free(entry->output)` before that lest we leak memory, but only if `output` is not identical anyway:
if (entry->output != output) {
free(entry->output);
entry->output = xstrdup(output);
}Show 9 quoted lines
> + } else {
> + int i, nr_packets;
> + size_t output_len;
> + struct strbuf sb = STRBUF_INIT;
> + packet_write_fmt(1, "status=success");
> + packet_flush(1);
> +
> + strbuf_addf(&sb, "%s-write-fail.r", command);
> + if (!strcmp(pathname, sb.buf)) {We can easily avoid allocating the string just for comparing it:
const char *p;
if (skip_prefix(pathname, command, &p) &&
!strcmp(p, "-write-fail.r")) {> + fprintf(logfile, "[WRITE FAIL]\n");
fflush(logfile) ;-)
Show 13 quoted lines
> + die("%s write error", command);
> + }
> +
> + output_len = strlen(output);
> + fprintf(logfile, "OUT: %"PRIuMAX" ", (uintmax_t)output_len);
> +
> + if (write_packetized_from_buf_no_flush_count(output,
> + output_len, 1, &nr_packets))
> + die("failed to write buffer to stdout");
> + packet_flush(1);
> +
> + for (i = 0; i < nr_packets; i++)
> + fprintf(logfile, ".");That's not quite the same as the Perl script does: it prints a '.' (without flushing, though) _every_ time it wrote a packet.
If you want to emulate that, you will have to copy/edit that loop (and in that case, the insanely long-named function `write_packetized_from_buf_no_flush_count()` is unnecessary, too).
Show 41 quoted lines
> + fprintf(logfile, " [OK]\n");
> +
> + packet_flush(1);
> + strbuf_release(&sb);
> + }
> + free(pathname);
> + strbuf_release(&input);
> + }
> + free(command);
> + }
> +}
> +
> +static void free_delay_hash(void)
> +{
> + struct hashmap_iter iter;
> + struct strmap_entry *ent;
> +
> + strmap_for_each_entry(&delay, &iter, ent) {
> + struct delay_entry *delay_entry = ent->value;
> + free(delay_entry->output);
> + free(delay_entry);
> + }
> + strmap_clear(&delay, 0);
> +}
> +
> +static void add_delay_entry(char *pathname, int count)
> +{
> + struct delay_entry *entry = xcalloc(1, sizeof(*entry));
> + entry->count = count;
> + if (strmap_put(&delay, pathname, entry))
> + BUG("adding the same path twice to delay hash?");
> +}
> +
> +static void packet_initialize(const char *name, int version)
> +{
> + struct strbuf sb = STRBUF_INIT;
> + int size;
> + char *pkt_buf = packet_read_line(0, &size);
> +
> + strbuf_addf(&sb, "%s-client", name);
> + if (!pkt_buf || strncmp(pkt_buf, sb.buf, size))We do not need the flexibility of the Perl package, where `name` is a parameter. We can hard-code `git-filter-client` here. I.e. something like this:
if (!pkt_buf || size != 17 || strncmp(pkt_buf, "git-filter-client", 17))
> + die("bad initialize: '%s'", xstrndup(pkt_buf, size));
> +
> + strbuf_reset(&sb);
> + strbuf_addf(&sb, "version=%d", version);Same here. We do not need to allocate a string just to compare it to the packet's payload.
Show 65 quoted lines
> + pkt_buf = packet_read_line(0, &size);
> + if (!pkt_buf || strncmp(pkt_buf, sb.buf, size))
> + die("bad version: '%s'", xstrndup(pkt_buf, size));
> +
> + pkt_buf = packet_read_line(0, &size);
> + if (pkt_buf)
> + die("bad version end: '%s'", xstrndup(pkt_buf, size));
> +
> + packet_write_fmt(1, "%s-server", name);
> + packet_write_fmt(1, "version=%d", version);
> + packet_flush(1);
> + strbuf_release(&sb);
> +}
> +
> +static char *rot13_usage = "test-tool rot13-filter [--always-delay] <log path> <capabilities>";
> +
> +int cmd__rot13_filter(int argc, const char **argv)
> +{
> + int i = 1;
> + struct string_list remote_caps = STRING_LIST_INIT_DUP,
> + supported_caps = STRING_LIST_INIT_NODUP;
> +
> + string_list_append(&supported_caps, "clean");
> + string_list_append(&supported_caps, "smudge");
> + string_list_append(&supported_caps, "delay");
> +
> + if (argc > 1 && !strcmp(argv[i], "--always-delay")) {
> + always_delay = 1;
> + i++;
> + }
> + if (argc - i < 2)
> + usage(rot13_usage);
> +
> + logfile = fopen(argv[i++], "a");
> + if (!logfile)
> + die_errno("failed to open log file");
> +
> + for ( ; i < argc; i++)
> + string_list_append(&requested_caps, argv[i]);
> +
> + add_delay_entry("test-delay10.a", 1);
> + add_delay_entry("test-delay11.a", 1);
> + add_delay_entry("test-delay20.a", 2);
> + add_delay_entry("test-delay10.b", 1);
> + add_delay_entry("missing-delay.a", 1);
> + add_delay_entry("invalid-delay.a", 1);
> +
> + fprintf(logfile, "START\n");
> +
> + packet_initialize("git-filter", 2);
> +
> + packet_read_and_check_capabilities(&remote_caps, &supported_caps);
> + packet_check_and_write_capabilities(&remote_caps, &requested_caps);
> + fprintf(logfile, "init handshake complete\n");
> +
> + string_list_clear(&supported_caps, 0);
> + string_list_clear(&remote_caps, 0);
> +
> + command_loop();
> +
> + fclose(logfile);
> + string_list_clear(&requested_caps, 0);
> + free_delay_hash();
> + return 0;
> +}Other than that, this looks great!
Thank you, Dscho