Re: [PATCH 1/2] t/t0021: convert the rot13-filter.pl script to C
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jul 23, 2022, 04:59 UTC
- Message-ID
- <220723.86pmhwquie.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <99823077be77bc621cfa8ccf3303bd612da343ad.1658518769.git.matheus.bernardino@usp.br>
On Fri, Jul 22 2022, Matheus Tavares wrote:
Looking a bit closer...
> however, that we still use the script as a wrapper at > this commit, in order to minimize the amount of changes it introduces > and help reviewers. At the next commit we will properly remove the > script and adjust the affected tests to use test-tool.
I'd prefer if we just squashed this, if you want to avoid some of the diff verbosity you could leave the PERL prereq on all the test_expect_success and remove it in a 2/2 (we just wouldn't run the test until then).
But I think it's all boilerplate, so just doing it in one step would be better, reasoning about the in-between steps is harder IMO (e.g. "exec" escaping or whatever)>
Show 11 quoted lines
> +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;
> + }
> + return str;
> +}Looks fine, but we should probably put in our CodingGuidelines at some point that we don't care about EBCDIC, as this isn't portable C (but probably portable enough, as we can probably assume ASCII) :)
> +static struct string_list *packet_read_capabilities(void)
> +{
> + struct string_list *caps = xmalloc(sizeof(*caps));malloc here...
Show 17 quoted lines
> + string_list_init_dup(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"));
> + }
> + return caps;
> +}
> +
> +/* Read remote capabilities and check them against capabilities we require */
> +static struct string_list *packet_read_and_check_capabilities(
> + struct string_list *required_caps)
> +{
> + struct string_list *remote_caps = packet_read_capabilities();...and here...
Show 8 quoted lines
> + struct string_list_item *item;
> + 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);
> + }
> + }
> + return remote_caps;...we'll return it...
Show 6 quoted lines
> + remote_caps = packet_read_and_check_capabilities(&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);
..and here you're missing a free(), but I wonder why not just declare this string_list in this function, and pass it down instead?
It's unfortunate that none of these tests seem to pass with SANITIZE=leak already, but the new command seems not to leak from a trivial glance except for in that one case.
Not knowing much about the filtering mechanism, I wonder if this code here wouldn't be better as a built-in some day. I.e. isn't this all shimmy we need to talk to some arbitrary conversion filter, except for the rot13 part?
So if we just invoked a "tr" with run_command() to do the actual rot13 filtering we could do any sort of arbitrary replacement, and present a variant of this this command as a "if you can't be bothered with packet-line" in gitattributes(5)...
...but maybe that's hopeless for some reason I'm missing, in any case, more #leftoverbits.