From: Christian Couder <christian.couder@gmail.com>
To: Linus Arver via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Christian Couder <chriscool@tuxfamily.org>,
Junio C Hamano <gitster@pobox.com>,
Emily Shaffer <nasamuffin@google.com>,
Josh Steadmon <steadmon@google.com>,
"Randall S. Becker" <rsbecker@nexbridge.com>,
Kristoffer Haugsbakk <code@khaugsbakk.name>,
Linus Arver <linus@ucla.edu>, Linus Arver <linusa@google.com>
Subject: Re: [PATCH v3 03/10] trailer: teach iterator about non-trailer lines
Date: Sat, 27 Apr 2024 14:50:21 +0200 [thread overview]
Message-ID: <CAP8UFD0OATdCaEN1SrvYMTZP3b1uWCZw57cXHhUNPW9eTj+x0Q@mail.gmail.com> (raw)
In-Reply-To: <9077d5a315d0d7272266856bf75a75b0a24df91d.1714091170.git.gitgitgadget@gmail.com>
(Sorry I just realized that I had sent this email to Linus only.)
On Fri, Apr 26, 2024 at 2:26 AM Linus Arver via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Linus Arver <linusa@google.com>
>
> Previously the iterator did not iterate over non-trailer lines. This was
> somewhat unfortunate, because trailer blocks could have non-trailer
> lines in them since 146245063e (trailer: allow non-trailers in trailer
> block, 2016-10-21), which was before the iterator was created in
> f0939a0eb1 (trailer: add interface for iterating over commit trailers,
> 2020-09-27).
>
> So if trailer API users wanted to iterate over all lines in a trailer
> block (including non-trailer lines), they could not use the iterator and
> were forced to use the lower-level trailer_info struct directly (which
> provides a raw string array that includes all lines in the trailer
> block).
>
> Change the iterator's behavior so that we also iterate over non-trailer
> lines, instead of skipping over them. The new "raw" member of the
> iterator allows API users to access previously inaccessible non-trailer
> lines. Reword the variable "trailer" to just "line" because this
> variable can now hold both trailer lines _and_ non-trailer lines.
>
> The new "raw" member is important because anyone currently not using the
> iterator is using trailer_info's raw string array directly to access
> lines to check what the combined key + value looks like. If we didn't
> provide a "raw" member here, iterator users would have to re-construct
> the unparsed line by concatenating the key and value back together again
> --- which places an undue burden for iterator users.
>
> The next commit demonstrates the use of the iterator in sequencer.c as an
> example of where "raw" will be useful, so that it can start using the
> iterator.
>
> For the existing use of the iterator in builtin/shortlog.c, we don't
> have to change the code there because that code does
>
> trailer_iterator_init(&iter, body);
> while (trailer_iterator_advance(&iter)) {
> const char *value = iter.val.buf;
>
> if (!string_list_has_string(&log->trailers, iter.key.buf))
> continue;
>
> ...
>
> and the
>
> if (!string_list_has_string(&log->trailers, iter.key.buf))
>
> condition already skips over non-trailer lines (iter.key.buf is empty
> for non-trailer lines, making the comparison still work even with this
> commit).
>
> Rename "num_expected_trailers" to "num_expected_objects" in
> t/unit-tests/t-trailer.c because the items we iterate over now include
> non-trailer lines.
I think it would be simpler if the previous patch used just
"num_expected" or "expected". It's not like the other fields in the
struct ("msg" and "name") are very explicit, so why this one only?
> Signed-off-by: Linus Arver <linusa@google.com>
> diff --git a/trailer.c b/trailer.c
> index 3e4dab9c065..4700c441442 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -1146,17 +1146,15 @@ void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
>
> int trailer_iterator_advance(struct trailer_iterator *iter)
> {
> - while (iter->internal.cur < iter->internal.info.trailer_nr) {
> - char *trailer = iter->internal.info.trailers[iter->internal.cur++];
> - int separator_pos = find_separator(trailer, separators);
> -
> - if (separator_pos < 1)
> - continue; /* not a real trailer */
> + if (iter->internal.cur < iter->internal.info.trailer_nr) {
> + char *line = iter->internal.info.trailers[iter->internal.cur++];
> + int separator_pos = find_separator(line, separators);
>
> + iter->raw = line;
> strbuf_reset(&iter->key);
> strbuf_reset(&iter->val);
> parse_trailer(&iter->key, &iter->val, NULL,
> - trailer, separator_pos);
> + line, separator_pos);
> /* Always unfold values during iteration. */
> unfold_value(&iter->val);
> return 1;
> diff --git a/trailer.h b/trailer.h
> index 9f42aa75994..ebafa3657e4 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -125,6 +125,14 @@ void format_trailers_from_commit(const struct process_trailer_options *,
> * trailer_iterator_release(&iter);
> */
> struct trailer_iterator {
> + /*
> + * Raw line (e.g., "foo: bar baz") before being parsed as a trailer
> + * key/val pair as part of a trailer block. A trailer block can be
> + * either 100% trailer lines, or mixed in with non-trailer lines (in
> + * which case at least 25% must be trailer lines).
I don't think 25% is important here. What is more important is to just
say that this field could not be an actual trailer, and to tell what
the 'key' and 'val' fields below will contain then.
> + */
> + const char *raw;
> +
> struct strbuf key;
> struct strbuf val;
next prev parent reply other threads:[~2024-04-27 12:50 UTC|newest]
Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-16 6:27 [PATCH 0/6] Make trailer_info struct private (plus sequencer cleanup) Linus Arver via GitGitGadget
2024-03-16 6:27 ` [PATCH 1/6] trailer: teach iterator about non-trailer lines Linus Arver via GitGitGadget
2024-03-16 6:27 ` [PATCH 2/6] sequencer: use the trailer iterator Linus Arver via GitGitGadget
2024-03-16 6:27 ` [PATCH 3/6] interpret-trailers: access trailer_info with new helpers Linus Arver via GitGitGadget
2024-03-16 6:27 ` [PATCH 4/6] trailer: make parse_trailers() return trailer_info pointer Linus Arver via GitGitGadget
2024-03-16 6:27 ` [PATCH 5/6] trailer: make trailer_info struct private Linus Arver via GitGitGadget
2024-03-16 6:27 ` [PATCH 6/6] trailer: retire trailer_info_get() from API Linus Arver via GitGitGadget
2024-03-16 17:06 ` [PATCH 0/6] Make trailer_info struct private (plus sequencer cleanup) Junio C Hamano
2024-03-26 22:00 ` Junio C Hamano
2024-04-19 5:36 ` Linus Arver
2024-04-19 5:22 ` [PATCH v2 0/8] " Linus Arver via GitGitGadget
2024-04-19 5:22 ` [PATCH v2 1/8] Makefile: sort UNIT_TEST_PROGRAMS Linus Arver via GitGitGadget
2024-04-19 5:22 ` [PATCH v2 2/8] trailer: add unit tests for trailer iterator Linus Arver via GitGitGadget
2024-04-19 5:33 ` Linus Arver
2024-04-19 18:46 ` Linus Arver
2024-04-19 21:52 ` Junio C Hamano
2024-04-20 0:14 ` Linus Arver
2024-04-19 5:22 ` [PATCH v2 3/8] trailer: teach iterator about non-trailer lines Linus Arver via GitGitGadget
2024-04-19 5:22 ` [PATCH v2 4/8] sequencer: use the trailer iterator Linus Arver via GitGitGadget
2024-04-23 21:19 ` Junio C Hamano
2024-04-19 5:22 ` [PATCH v2 5/8] interpret-trailers: access trailer_info with new helpers Linus Arver via GitGitGadget
2024-04-19 5:22 ` [PATCH v2 6/8] trailer: make parse_trailers() return trailer_info pointer Linus Arver via GitGitGadget
2024-04-23 23:17 ` Junio C Hamano
2024-04-19 5:22 ` [PATCH v2 7/8] trailer: make trailer_info struct private Linus Arver via GitGitGadget
2024-04-23 23:27 ` Junio C Hamano
2024-04-25 3:17 ` Linus Arver
2024-04-19 5:22 ` [PATCH v2 8/8] trailer: retire trailer_info_get() from API Linus Arver via GitGitGadget
2024-04-23 23:27 ` Junio C Hamano
2024-04-24 0:27 ` [PATCH v2 0/8] Make trailer_info struct private (plus sequencer cleanup) Junio C Hamano
2024-04-26 0:26 ` [PATCH v3 00/10] " Linus Arver via GitGitGadget
2024-04-26 0:26 ` [PATCH v3 01/10] Makefile: sort UNIT_TEST_PROGRAMS Linus Arver via GitGitGadget
2024-04-26 0:26 ` [PATCH v3 02/10] trailer: add unit tests for trailer iterator Linus Arver via GitGitGadget
2024-04-26 14:51 ` Christian Couder
2024-04-26 16:20 ` Junio C Hamano
2024-04-26 16:25 ` Linus Arver
2024-04-26 0:26 ` [PATCH v3 03/10] trailer: teach iterator about non-trailer lines Linus Arver via GitGitGadget
2024-04-27 12:50 ` Christian Couder [this message]
2024-04-30 4:42 ` Linus Arver
2024-04-30 4:55 ` Linus Arver
2024-04-26 0:26 ` [PATCH v3 04/10] sequencer: use the trailer iterator Linus Arver via GitGitGadget
2024-04-26 0:26 ` [PATCH v3 05/10] interpret-trailers: access trailer_info with new helpers Linus Arver via GitGitGadget
2024-04-26 0:26 ` [PATCH v3 06/10] trailer: make parse_trailers() return trailer_info pointer Linus Arver via GitGitGadget
2024-04-26 0:26 ` [PATCH v3 07/10] trailer: make trailer_info struct private Linus Arver via GitGitGadget
2024-04-26 0:26 ` [PATCH v3 08/10] trailer: retire trailer_info_get() from API Linus Arver via GitGitGadget
2024-04-26 0:26 ` [PATCH v3 09/10] trailer: document parse_trailers() usage Linus Arver via GitGitGadget
2024-04-26 0:26 ` [PATCH v3 10/10] trailer unit tests: inspect iterator contents Linus Arver via GitGitGadget
2024-04-27 12:51 ` [PATCH v3 00/10] Make trailer_info struct private (plus sequencer cleanup) Christian Couder
2024-05-02 4:54 ` [PATCH v4 " Linus Arver via GitGitGadget
2024-05-02 4:54 ` [PATCH v4 01/10] Makefile: sort UNIT_TEST_PROGRAMS Linus Arver via GitGitGadget
2024-05-02 4:54 ` [PATCH v4 02/10] trailer: add unit tests for trailer iterator Linus Arver via GitGitGadget
2024-05-02 16:54 ` Junio C Hamano
2024-05-02 4:54 ` [PATCH v4 03/10] trailer: teach iterator about non-trailer lines Linus Arver via GitGitGadget
2024-05-04 15:33 ` Phillip Wood
2024-05-05 1:37 ` Linus Arver
2024-05-05 14:09 ` Phillip Wood
2024-05-09 7:11 ` Linus Arver
2024-05-13 15:11 ` Phillip Wood
2024-05-13 15:13 ` Phillip Wood
2024-05-02 4:54 ` [PATCH v4 04/10] sequencer: use the trailer iterator Linus Arver via GitGitGadget
2024-05-02 4:54 ` [PATCH v4 05/10] interpret-trailers: access trailer_info with new helpers Linus Arver via GitGitGadget
2024-05-02 4:54 ` [PATCH v4 06/10] trailer: make parse_trailers() return trailer_info pointer Linus Arver via GitGitGadget
2024-05-02 4:54 ` [PATCH v4 07/10] trailer: make trailer_info struct private Linus Arver via GitGitGadget
2024-05-02 4:54 ` [PATCH v4 08/10] trailer: retire trailer_info_get() from API Linus Arver via GitGitGadget
2024-05-02 4:54 ` [PATCH v4 09/10] trailer: document parse_trailers() usage Linus Arver via GitGitGadget
2024-05-02 4:54 ` [PATCH v4 10/10] trailer unit tests: inspect iterator contents Linus Arver via GitGitGadget
2024-05-02 17:15 ` [PATCH v4 00/10] Make trailer_info struct private (plus sequencer cleanup) 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=CAP8UFD0OATdCaEN1SrvYMTZP3b1uWCZw57cXHhUNPW9eTj+x0Q@mail.gmail.com \
--to=christian.couder@gmail.com \
--cc=chriscool@tuxfamily.org \
--cc=code@khaugsbakk.name \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=linus@ucla.edu \
--cc=linusa@google.com \
--cc=nasamuffin@google.com \
--cc=rsbecker@nexbridge.com \
--cc=steadmon@google.com \
/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 read-only IMAP folder(s) and NNTP newsgroup(s).