git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Aug 1, 2022, 21:18 UTC
Message-ID
<xmqqr11zoe6i.fsf@gitster.g>
In-Reply-To
<86e6baba460f4d0fce353d1fb6a0e18b57ecadaa.1659291025.git.matheus.bernardino@usp.br>
Matheus Tavares <matheus.bernardino@usp.br> writes:
Show 8 quoted lines
> +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.
Show 8 quoted lines
> +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.

Show 15 quoted lines
> +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");
> +}
Show 21 quoted lines
> +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.
Show 14 quoted lines
> +		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.

Show 17 quoted lines
> +			} 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.
Show 14 quoted lines
> +		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.

Show 51 quoted lines
> +		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.
Previous: Ævar Arnfjörð BjarmasonNext: Matheus Tavares
Message 21 of 28 in “t0021: convert perl script to C test-tool helper”
  1. 0/2 t0021: convert perl script to C test-tool helperMatheus Tavares, Jul 22, 2022
  2. 2/2 t/t0021: replace old rot13-filter.pl uses with new test-tool cmdMatheus Tavares, Jul 22, 2022
  3. 1/2 t/t0021: convert the rot13-filter.pl script to CMatheus Tavares, Jul 22, 2022
  4. Ævar Arnfjörð BjarmasonJul 23, 2022
  5. Ævar Arnfjörð BjarmasonJul 23, 2022
  6. Matheus TavaresJul 23, 2022
  7. t/t0021: convert the rot13-filter.pl script to CMatheus Tavares, Jul 24, 2022
  8. Johannes SchindelinJul 28, 2022
  9. Junio C HamanoJul 28, 2022
  10. Ævar Arnfjörð BjarmasonJul 28, 2022
  11. Matheus TavaresJul 31, 2022
  12. Johannes SchindelinAug 9, 2022
  13. 0/3 t0021: convert perl script to C test-tool helperMatheus Tavares, Jul 31, 2022
  14. 1/3 t0021: avoid grepping for a Perl-specific string at filter outputMatheus Tavares, Jul 31, 2022
  15. Junio C HamanoAug 1, 2022
  16. 2/3 t0021: implementation the rot13-filter.pl script in CMatheus Tavares, Jul 31, 2022
  17. Ævar Arnfjörð BjarmasonAug 1, 2022
  18. Matheus TavaresAug 2, 2022
  19. Johannes SchindelinAug 9, 2022
  20. Ævar Arnfjörð BjarmasonAug 1, 2022
  21. Junio C HamanoAug 1, 2022
  22. Matheus TavaresAug 2, 2022
  23. Johannes SchindelinAug 9, 2022
  24. Junio C HamanoAug 10, 2022
  25. Junio C HamanoAug 10, 2022
  26. Johannes SchindelinAug 9, 2022
  27. Johannes SchindelinAug 9, 2022
  28. 3/3 tests: use the new C rot13-filter helper to avoid PERL prereqMatheus Tavares, Jul 31, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.