From: Junio C Hamano <gitster@pobox.com>
To: Matheus Tavares <matheus.bernardino@usp.br>
Cc: git@vger.kernel.org, avarab@gmail.com, johannes.schindelin@gmx.de
Subject: Re: [PATCH v3 2/3] t0021: implementation the rot13-filter.pl script in C
Date: Mon, 01 Aug 2022 14:18:45 -0700 [thread overview]
Message-ID: <xmqqr11zoe6i.fsf@gitster.g> (raw)
In-Reply-To: <86e6baba460f4d0fce353d1fb6a0e18b57ecadaa.1659291025.git.matheus.bernardino@usp.br> (Matheus Tavares's message of "Sun, 31 Jul 2022 15:19:49 -0300")
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.
next prev parent reply other threads:[~2022-08-01 21:18 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-22 19:42 [PATCH 0/2] t0021: convert perl script to C test-tool helper Matheus Tavares
2022-07-22 19:42 ` [PATCH 1/2] t/t0021: convert the rot13-filter.pl script to C Matheus Tavares
2022-07-23 4:52 ` Ævar Arnfjörð Bjarmason
2022-07-23 4:59 ` Ævar Arnfjörð Bjarmason
2022-07-23 13:36 ` Matheus Tavares
2022-07-22 19:42 ` [PATCH 2/2] t/t0021: replace old rot13-filter.pl uses with new test-tool cmd Matheus Tavares
2022-07-24 15:09 ` [PATCH v2] t/t0021: convert the rot13-filter.pl script to C Matheus Tavares
2022-07-28 16:58 ` Johannes Schindelin
2022-07-28 17:54 ` Junio C Hamano
2022-07-28 19:50 ` Ævar Arnfjörð Bjarmason
2022-07-31 2:52 ` Matheus Tavares
2022-08-09 9:36 ` Johannes Schindelin
2022-07-31 18:19 ` [PATCH v3 0/3] t0021: convert perl script to C test-tool helper Matheus Tavares
2022-07-31 18:19 ` [PATCH v3 1/3] t0021: avoid grepping for a Perl-specific string at filter output Matheus Tavares
2022-08-01 20:41 ` Junio C Hamano
2022-07-31 18:19 ` [PATCH v3 2/3] t0021: implementation the rot13-filter.pl script in C Matheus Tavares
2022-08-01 11:33 ` Ævar Arnfjörð Bjarmason
2022-08-02 0:16 ` Matheus Tavares
2022-08-09 9:45 ` Johannes Schindelin
2022-08-01 11:39 ` Ævar Arnfjörð Bjarmason
2022-08-01 21:18 ` Junio C Hamano [this message]
2022-08-02 0:13 ` Matheus Tavares
2022-08-09 10:00 ` Johannes Schindelin
2022-08-10 18:37 ` Junio C Hamano
2022-08-10 19:58 ` Junio C Hamano
2022-08-09 10:37 ` Johannes Schindelin
2022-08-09 10:47 ` Johannes Schindelin
2022-07-31 18:19 ` [PATCH v3 3/3] tests: use the new C rot13-filter helper to avoid PERL prereq Matheus Tavares
2022-08-15 1:06 ` [PATCH v4 0/3] t0021: convert perl script to C test-tool helper Matheus Tavares
2022-08-15 1:06 ` [PATCH v4 1/3] t0021: avoid grepping for a Perl-specific string at filter output Matheus Tavares
2022-08-15 1:06 ` [PATCH v4 2/3] t0021: implementation the rot13-filter.pl script in C Matheus Tavares
2022-08-15 1:06 ` [PATCH v4 3/3] tests: use the new C rot13-filter helper to avoid PERL prereq Matheus Tavares
2022-08-15 13:01 ` [PATCH v4 0/3] t0021: convert perl script to C test-tool helper Johannes Schindelin
2022-08-19 22:17 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqr11zoe6i.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=johannes.schindelin@gmx.de \
--cc=matheus.bernardino@usp.br \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).