Re: [PATCH v3 2/3] t0021: implementation the rot13-filter.pl script in C

[Junio C Hamano <gitster@pobox.com>]

Matheus Tavares <matheus.bernardino@usp.br> writes:

> +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((const char *)buf, size, key, (const char **)&buf, &size) ||
> +	    !skip_prefix_mem((const char *)buf, size, "=", (const char **)&buf, &size) ||
> +	    !size)

So, skip_prefix_mem(), when successfully parses the prefix out,
advances buf[] to skip the prefix and shortens size by the same
amount, so buf[size] is pointing at the same byte.  The code wants
to make sure buf[] begins with the "<key>=", skip that part, so
presumably buf[] after the above part moves to the beginning of
<value> in the "<key>=<value>" string?  It also wants to reject
"<key>=", i.e. an empty string as the <value>?

> +		die("expected key '%s', got '%.*s'",
> +		    key, orig_size, orig_buf);
> +
> +	buf[size] = '\0';

I find this assignment somewhat strange, but primarily because it
uses the updated buf[size] that ought to be pointing at the same
byte as the original buf[size].  Is this necessary because buf[size]
upon the entry to this function does not necessarily have NUL there?

Reading ahead,

 * packet_key_val_read() feeds the buffer taken from
   packet_read_line_gently(), so buf[size] should be NUL terminated
   already.

 * read_capabilities() feeds the buffer taken from
   packet_read_line(), so buf[size] should be NUL terminated
   already.

> +	return buf;
> +}

And the caller gets the byte position that begins the <value> part.

> +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 xstrdup(get_value(buf, size, key));
> +}

The returned value from get_value() is pointing into
pkt-line.c::packet_buffer[], so we return a copy to the caller,
which takes the ownership.  OK.

> +static inline void assert_remote_capability(struct strset *caps, const char *cap)
> +{
> +	if (!strset_contains(caps, cap))
> +		die("required '%s' capability not available from remote", cap);
> +}
> +
> +static void read_capabilities(struct strset *remote_caps)
> +{
> +	for (;;) {
> +		int size;
> +		char *buf = packet_read_line(0, &size);
> +		if (!buf)
> +			break;
> +		strset_add(remote_caps, get_value(buf, size, "capability"));
> +	}

strset_add() creates a copy of what get_value() borrowed from
pkt-line.c::packet_buffer[] here, which is good.

> +	assert_remote_capability(remote_caps, "clean");
> +	assert_remote_capability(remote_caps, "smudge");
> +	assert_remote_capability(remote_caps, "delay");
> +}

> +static void command_loop(void)
> +{
> +	for (;;) {
> +		char *buf, *output;
> +		int size;
> +		char *pathname;
> +		struct delay_entry *entry;
> +		struct strbuf input = STRBUF_INIT;
> +		char *command = packet_key_val_read("command");
> +
> +		if (!command) {
> +			fprintf(logfile, "STOP\n");
> +			break;
> +		}
> +		fprintf(logfile, "IN: %s", command);
> +
> +		if (!strcmp(command, "list_available_blobs")) {
> +			reply_list_available_blobs_cmd();
> +			free(command);
> +			continue;
> +		}

OK.

> +		pathname = packet_key_val_read("pathname");
> +		if (!pathname)
> +			die("unexpected EOF while expecting pathname");
> +		fprintf(logfile, " %s", pathname);
> +
> +		/* Read until flush */
> +		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) {
> +					add_delay_entry(pathname, 1, 1);
> +				}

These are unnecessary {} around single statement blocks, but let's
let it pass in a test helper.

> +			} 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);
> +			}
> +		}
> +
> +
> +		read_packetized_to_strbuf(0, &input, 0);

I do not see a need for double blank lines above.

> +		fprintf(logfile, " %"PRIuMAX" [OK] -- ", (uintmax_t)input.len);
> +
> +		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_clean_cap) {
> +			output = rot13(input.buf);
> +		} else if (!strcmp(command, "smudge") && has_smudge_cap) {
> +			output = rot13(input.buf);
> +		} else {
> +			die("bad command '%s'", command);
> +		}

Good.  At this point, output all points into something and itself
does not own the memory it is pointing at.

> +		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;
> +			if (entry->output != output) {
> +				free(entry->output);
> +				entry->output = xstrdup(output);
> +			}
> +		} else {
> +			int i, nr_packets = 0;
> +			size_t output_len;
> +			const char *p;
> +			packet_write_fmt(1, "status=success");
> +			packet_flush(1);
> +
> +			if (skip_prefix(pathname, command, &p) &&
> +			    !strcmp(p, "-write-fail.r")) {
> +				fprintf(logfile, "[WRITE FAIL]\n");
> +				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, ".");
> +			fprintf(logfile, " [OK]\n");
> +
> +			packet_flush(1);
> +		}
> +		free(pathname);
> +		strbuf_release(&input);
> +		free(command);
> +	}
> +}

OK, at this point we are done with pathname and command so we can
free them for the next round.  input was used as a scratch buffer
and we are done with it, too.

Looking good.

Thanks.