| Message ID | 84897cf5c83eb67c023603016b49fb7b56870aa3.1713504153.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | 5165f21817d2720e351a0c451f4029621897b2ba |
| Headers | show |
| Series | Make trailer_info struct private (plus sequencer cleanup) | expand |
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Linus Arver <linusa@google.com> > > 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). > > Avoiding "trailer_info_get()" means we don't have to worry about options > like "no_divider" (relevant for parsing trailers). We also don't have to > check for things like "info.trailer_start == info.trailer_end" to see > whether there were any trailers (instead we can just check to see > whether the iterator advanced at all). > > Note how we have to use "iter.raw" in order to get the same behavior as > before when we iterated over the unparsed string array (char **trailers) > in trailer_info. > Signed-off-by: Linus Arver <linusa@google.com> > --- > sequencer.c | 27 ++++++++++++--------------- > 1 file changed, 12 insertions(+), 15 deletions(-) OK. The code reduction primarily comes from the fact that the original was manually iterating over the trailer lines that you can get from the iterator API. > + while (trailer_iterator_advance(&iter)) { > + i++; > + if (sob && !strncmp(iter.raw, sob->buf, sob->len)) > + found_sob = i; > + } > + trailer_iterator_release(&iter); > > + if (!i) > + return 0; > > + found_sob_last = (int)i == found_sob; This is slightly harder to reason about, as we cannot directly say "the collection being iterated over has .nr members, and what we found was at the end" like the original could do in its loop. > - 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; > - } As 'i' is incremented before we set found_sob to it in the new loop, when it is assigned in the loop, the value of found_sob will never be zero. It used to be that found_sob takes only 0 or 1, but because we only care about found_sob and found_sob_last being zero/non-zero in the remainder of the code, this does not affect the correctness of the code. Looking good.
diff --git a/sequencer.c b/sequencer.c index ea1441e6174..4c1f6c675e7 100644 --- a/sequencer.c +++ b/sequencer.c @@ -319,35 +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; + struct trailer_iterator iter; + size_t i = 0; int found_sob = 0, found_sob_last = 0; char saved_char; - opts.no_divider = 1; - if (ignore_footer) { saved_char = sb->buf[sb->len - ignore_footer]; sb->buf[sb->len - ignore_footer] = '\0'; } - trailer_info_get(&opts, sb->buf, &info); + trailer_iterator_init(&iter, sb->buf); if (ignore_footer) sb->buf[sb->len - ignore_footer] = saved_char; - if (info.trailer_block_start == info.trailer_block_end) - return 0; + while (trailer_iterator_advance(&iter)) { + i++; + if (sob && !strncmp(iter.raw, sob->buf, sob->len)) + found_sob = i; + } + trailer_iterator_release(&iter); - 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; - } + if (!i) + return 0; - trailer_info_release(&info); + found_sob_last = (int)i == found_sob; if (found_sob_last) return 3;