Re: [PATCH v2] t/t0021: convert the rot13-filter.pl script to C

[Matheus Tavares <matheus.bernardino@usp.br>]

Hi, Dscho

On Thu, Jul 28, 2022 at 1:58 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> > On Sun, 24 Jul 2022, Matheus Tavares wrote:
> >
> > diff --git a/t/helper/test-rot13-filter.c b/t/helper/test-rot13-filter.c
> > +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;

Nice :) Thanks.

> > [...]
> > +static void packet_read_capabilities(struct string_list *caps)
> > +{
> > +     while (1) {
> > +             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`.

Ok, will do.

> > +
> > +/*
> > + * 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);
> }

Makes sense. We also use the list elsewhere (has_capability()), but we
can easily replace that with two global flags to indicate if we have
the "clean" and "smudge" caps.

> And then we would call it via
>
>         check_and_write_capabilities(remote_caps, argv + 1, argc - 1);
>
> [...]
> > +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.

I was somewhat intrigued as to why the flushes were needed in the Perl
script. But reading [1] and [2], now, it seems to have been an
oversight.

That is, Eric suggested splictily flushing stdout because it is a
pipe, but the author ended up erroneously disabling autoflush for
stdout too, so that's why we needed the flushes there. They later
acknowledged that and said that they would re-enabled it (see [2]),
but it seems to have been forgotten. So I think we can safely drop the
flush calls.

[1]: http://public-inbox.org/git/20160723072721.GA20875%40starla/
[2]: https://lore.kernel.org/git/7F1F1A0E-8FC3-4FBD-81AA-37786DE0EF50@gmail.com/

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

It actually prints them in sorted order:

        foreach my $pathname ( sort keys %DELAY )

That is required because some test cases will compare the output using
this order.

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

OK, will do.

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

Sure! I think we can take a step further and extract the if logic to a
separate function.

> > +             } 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))) {
>
> > +                             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.
>
> > +                                     }
> > +                             } 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!
>
> > +
> > +                     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);
>                                 }

I think, entry->output will always be NULL here, since we only get
inside this if block after entry->requested has been set to 1 at the
top of the function; and, at that point, we haven't run ro13 yet.
Nevertheless, it doesn't hurt to add the free call anyway :)

>
> > +                     } 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) ;-)
>
> > +                                     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).

Hmm, I'm not sure we need to emulate that. I do dislike the huge
function name as well, but I also don't quite like to repeat code
copying that loop here...

> > +                             fprintf(logfile, " [OK]\n");
> > +
> > +                             packet_flush(1);
> > +                             strbuf_release(&sb);
> > +                     }
> > +                     free(pathname);
> > +                     strbuf_release(&input);
> > +             }
> > +             free(command);
> > +     }
> > +}
> > [...]
> > +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))

Good idea! Thanks. Perhaps, can't we do:

        if (!pkt_buf || strncmp(pkt_buf, "git-filter-client", size))

to avoid the hard-coded and possibly error-prone 17?

> > +             die("bad initialize: '%s'", xstrndup(pkt_buf, size));
> > +
> > +     strbuf_reset(&sb);
> > +     strbuf_addf(&sb, "version=%d", version);

Thanks for a very detailed review and great suggestions!