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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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.