"Linus Arver via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> From: Linus Arver <linusa@xxxxxxxxxx>
>
> This patch allows for the removal of "trailer_info_get()" from the
> trailer.h API, which will be in the next patch.
Hmph, do you mean "shortlog" and the sequencer were the only two
external callers and with this we can make it file-scope static to
trailer.c? Or do you mean the next step will be more than a removal
of a declaration from trailer.h plus adding "static" in front of its
definition in trailer.c, because there need other adjustments before
that happens?
> Instead of calling "trailer_info_get()", which is a low-level function
> in the trailers implementation (trailer.c), call
> trailer_iterator_advance(), which was specifically designed for public
> consumption in f0939a0eb1 (trailer: add interface for iterating over
> commit trailers, 2020-09-27).
;-).
> Also, teach the iterator about non-trailer lines, by adding a new field
> called "raw" to hold both trailer and non-trailer lines. This is
> necessary because a "trailer block" is a list of trailer lines of at
> least 25% trailers (see 146245063e (trailer: allow non-trailers in
> trailer block, 2016-10-21)), such that it may hold non-trailer lines.
That sounds like a task larger than something we would want in a
patch that focuses on another task (e.g. update sequencer not to
call trailer_info_get()) while at it. It seems from a casual glance
that the change to shortlog.c is to accomodate this change in the
semantics of what the iterator could return? It smells that this
patch does two more or less unrelated things at the same time?
> Signed-off-by: Linus Arver <linusa@xxxxxxxxxx>
> ---
> builtin/shortlog.c | 7 +++++--
> sequencer.c | 35 +++++++++++++++--------------------
> trailer.c | 17 +++++++++--------
> trailer.h | 13 +++++++++++++
> 4 files changed, 42 insertions(+), 30 deletions(-)
>
> diff --git a/builtin/shortlog.c b/builtin/shortlog.c
> index 1307ed2b88a..dc8fd5a5532 100644
> --- a/builtin/shortlog.c
> +++ b/builtin/shortlog.c
> @@ -172,7 +172,7 @@ static void insert_records_from_trailers(struct shortlog *log,
> const char *oneline)
> {
> struct trailer_iterator iter;
> - const char *commit_buffer, *body;
> + const char *commit_buffer, *body, *value;
> struct strbuf ident = STRBUF_INIT;
>
> if (!log->trailers.nr)
> @@ -190,7 +190,10 @@ static void insert_records_from_trailers(struct shortlog *log,
>
> trailer_iterator_init(&iter, body);
> while (trailer_iterator_advance(&iter)) {
> - const char *value = iter.val.buf;
> + if (!iter.is_trailer)
> + continue;
> +
> + value = iter.val.buf;
>
> if (!string_list_has_string(&log->trailers, iter.key.buf))
> continue;
> diff --git a/sequencer.c b/sequencer.c
> index 3cc88d8a800..bc7c82c5271 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -319,37 +319,32 @@ static const char *get_todo_path(const struct replay_opts *opts)
> static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
> size_t ignore_footer)
> {
> - struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
> - struct trailer_info info;
> - size_t i;
> - int found_sob = 0, found_sob_last = 0;
> - char saved_char;
> -
> - opts.no_divider = 1;
> + struct trailer_iterator iter;
> + size_t i = 0, found_sob = 0;
> + char saved_char = sb->buf[sb->len - ignore_footer];
>
> if (ignore_footer) {
> - saved_char = sb->buf[sb->len - ignore_footer];
> sb->buf[sb->len - ignore_footer] = '\0';
> }
>
> - trailer_info_get(&info, sb->buf, &opts);
> + trailer_iterator_init(&iter, sb->buf);
> + while (trailer_iterator_advance(&iter)) {
> + i++;
> + if (sob &&
> + iter.is_trailer &&
> + !strncmp(iter.raw, sob->buf, sob->len)) {
> + found_sob = i;
> + }
> + }
> + trailer_iterator_release(&iter);
>
> if (ignore_footer)
> sb->buf[sb->len - ignore_footer] = saved_char;
>
> - if (info.trailer_block_start == info.trailer_block_end)
> + if (!i)
> return 0;
>
> - for (i = 0; i < info.trailer_nr; i++)
> - if (sob && !strncmp(info.trailers[i], sob->buf, sob->len)) {
> - found_sob = 1;
> - if (i == info.trailer_nr - 1)
> - found_sob_last = 1;
> - }
> -
> - trailer_info_release(&info);
> -
> - if (found_sob_last)
> + if (found_sob == i)
> return 3;
> if (found_sob)
> return 2;
> diff --git a/trailer.c b/trailer.c
> index 71ea2bb67f8..5bcc9b0006c 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -1158,17 +1158,18 @@ 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 */
> -
> + char *line;
> + int separator_pos;
> + if (iter->internal.cur < iter->internal.info.trailer_nr) {
> + line = iter->internal.info.trailers[iter->internal.cur++];
> + separator_pos = find_separator(line, separators);
> + iter->is_trailer = (separator_pos > 0);
> +
> + iter->raw = line;
> strbuf_reset(&iter->key);
> strbuf_reset(&iter->val);
> parse_trailer(&iter->key, &iter->val, NULL,
> - trailer, separator_pos);
> + line, separator_pos);
> unfold_value(&iter->val);
> return 1;
> }
> diff --git a/trailer.h b/trailer.h
> index 244f29fc91f..a7599067acc 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -127,6 +127,19 @@ struct trailer_iterator {
> struct strbuf key;
> struct strbuf val;
>
> + /*
> + * 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).
> + */
> + const char *raw;
> +
> + /*
> + * 1 if the raw line was parsed as a trailer line (key/val pair).
> + */
> + int is_trailer;
> +
> /* private */
> struct {
> struct trailer_info info;
