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