| Message ID | d3326021fb6a63707e4ce56f166447143f4e55a2.1704869487.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enrich Trailer API | expand |
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Linus Arver <linusa@google.com> > > Currently have two functions for formatting trailers exposed in > trailer.h: > > void format_trailers(FILE *outfile, struct list_head *head, > const struct process_trailer_options *opts); > > void format_trailers_from_commit(struct strbuf *out, const char *msg, > const struct process_trailer_options *opts); > > and previously these functions, although similar enough (even taking the > same process_trailer_options struct pointer), did not build on each > other. > > Make format_trailers_from_commit() rely on format_trailers(). Teach > format_trailers() to process trailers with the additional > process_trailer_options fields like opts->key_only which is only used by > format_trailers_from_commit() and not builtin/interpret-trailers.c. Yay. It feels a bit disappointing to see the diffstat and learn that we are not deleting substantial number of lines. > --- > builtin/interpret-trailers.c | 5 +- > pretty.c | 2 +- > ref-filter.c | 2 +- > trailer.c | 105 +++++++++++++++++++++++++++++------ > trailer.h | 21 +++---- > 5 files changed, 102 insertions(+), 33 deletions(-) > diff --git a/pretty.c b/pretty.c > index cf964b060cd..f0721a5214f 100644 > --- a/pretty.c > +++ b/pretty.c > @@ -1759,7 +1759,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ > goto trailer_out; > } > if (*arg == ')') { > - format_trailers_from_commit(sb, msg + c->subject_off, &opts); > + format_trailers_from_commit(msg + c->subject_off, &opts, sb); I am curious (read: no objection---merely wondering if there is a guiding principle behind the choice of the new order) why this new parameter ordering. I suspect it was originally written with a strbuf-centric worldview and having sb at the beginning may have made sense, but if we are moving them around, wouldn't it make more sense to have &opts as the first parameter, as this is primarily a "trailers" function? Unsure until I read through to the end, but that is my gut reaction. > static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok) > { > struct trailer_item *new_item = xcalloc(1, sizeof(*new_item)); > @@ -984,6 +971,78 @@ static void unfold_value(struct strbuf *val) > strbuf_release(&out); > } > > +void format_trailers(struct list_head *head, > + const struct process_trailer_options *opts, > + struct strbuf *out) > +{ > + struct list_head *pos; > + struct trailer_item *item; > + int need_separator = 0; > + > + list_for_each(pos, head) { > + item = list_entry(pos, struct trailer_item, list); > + if (item->token) { > + char c; > + ... > + if (!opts->filter || opts->filter(&tok, opts->filter_data)) { > + if (opts->unfold) > + unfold_value(&val); > + > + if (opts->separator && need_separator) > + strbuf_addbuf(out, opts->separator); > + if (!opts->value_only) > + strbuf_addbuf(out, &tok); > + if (!opts->key_only && !opts->value_only) { > + if (opts->key_value_separator) > + strbuf_addbuf(out, opts->key_value_separator); > + else { > + c = last_non_space_char(tok.buf); > + if (c) { > + if (!strchr(separators, c)) > + strbuf_addf(out, "%c ", separators[0]); > + } > + } That's an overly deep nesting. I wonder if a small file-scope helper function is in order? static add_separator(struct process_trailer_options *opts, const char *token struct strbuf *out) { if (opts->key_value_separator) strbuf_addbuf(out, opts->key_value_separator); else strbuf_addstr(out, ": "); } Or perhaps inside the context of the loop to go over the list of trailer items, one iteration of the list_for_each() loop can become one call to a small helper function format_one_trailer() and that may make the result easier to follow? In any case, I didn't see anything glaringly wrong so far in this series. Let me keep reading. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Linus Arver <linusa@google.com> >> >> Currently have two functions for formatting trailers exposed in >> trailer.h: >> >> void format_trailers(FILE *outfile, struct list_head *head, >> const struct process_trailer_options *opts); >> >> void format_trailers_from_commit(struct strbuf *out, const char *msg, >> const struct process_trailer_options *opts); >> >> and previously these functions, although similar enough (even taking the >> same process_trailer_options struct pointer), did not build on each >> other. >> >> Make format_trailers_from_commit() rely on format_trailers(). Teach >> format_trailers() to process trailers with the additional >> process_trailer_options fields like opts->key_only which is only used by >> format_trailers_from_commit() and not builtin/interpret-trailers.c. > > Yay. It feels a bit disappointing to see the diffstat and learn > that we are not deleting substantial number of lines. Indeed ;) >> --- >> builtin/interpret-trailers.c | 5 +- >> pretty.c | 2 +- >> ref-filter.c | 2 +- >> trailer.c | 105 +++++++++++++++++++++++++++++------ >> trailer.h | 21 +++---- >> 5 files changed, 102 insertions(+), 33 deletions(-) > >> diff --git a/pretty.c b/pretty.c >> index cf964b060cd..f0721a5214f 100644 >> --- a/pretty.c >> +++ b/pretty.c >> @@ -1759,7 +1759,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ >> goto trailer_out; >> } >> if (*arg == ')') { >> - format_trailers_from_commit(sb, msg + c->subject_off, &opts); >> + format_trailers_from_commit(msg + c->subject_off, &opts, sb); > > I am curious (read: no objection---merely wondering if there is a > guiding principle behind the choice of the new order) why this new > parameter ordering. Glen Choo told me a while ago [1] that we usually put "out parameters" at the end, and somehow that stuck with me. > I suspect it was originally written with a > strbuf-centric worldview and having sb at the beginning may have > made sense, Having gotten more familiar with the strbuf.h functions since this patch was originally written, I agree. > but if we are moving them around, wouldn't it make more > sense to have &opts as the first parameter, as this is primarily a > "trailers" function? Unsure until I read through to the end, but > that is my gut reaction. I simply (mechanically) moved "sb" from the beginning of the parameters to the end, and didn't think much beyond that adjustment. Your ordering seems fine to me. Noted for the next reroll, but I will wait until you're done reviewing the other patches before going with the new ordering (in case you change your mind). >> static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok) >> { >> struct trailer_item *new_item = xcalloc(1, sizeof(*new_item)); >> @@ -984,6 +971,78 @@ static void unfold_value(struct strbuf *val) >> strbuf_release(&out); >> } >> >> +void format_trailers(struct list_head *head, >> + const struct process_trailer_options *opts, >> + struct strbuf *out) >> +{ >> + struct list_head *pos; >> + struct trailer_item *item; >> + int need_separator = 0; >> + >> + list_for_each(pos, head) { >> + item = list_entry(pos, struct trailer_item, list); >> + if (item->token) { >> + char c; >> + ... >> + if (!opts->filter || opts->filter(&tok, opts->filter_data)) { >> + if (opts->unfold) >> + unfold_value(&val); >> + >> + if (opts->separator && need_separator) >> + strbuf_addbuf(out, opts->separator); >> + if (!opts->value_only) >> + strbuf_addbuf(out, &tok); >> + if (!opts->key_only && !opts->value_only) { >> + if (opts->key_value_separator) >> + strbuf_addbuf(out, opts->key_value_separator); >> + else { >> + c = last_non_space_char(tok.buf); >> + if (c) { >> + if (!strchr(separators, c)) >> + strbuf_addf(out, "%c ", separators[0]); >> + } >> + } > > That's an overly deep nesting. I wonder if a small file-scope > helper function is in order? > > static add_separator(struct process_trailer_options *opts, > const char *token > struct strbuf *out) > { > if (opts->key_value_separator) > strbuf_addbuf(out, opts->key_value_separator); > else > strbuf_addstr(out, ": "); > } > > Or perhaps inside the context of the loop to go over the list of > trailer items, one iteration of the list_for_each() loop can become > one call to a small helper function format_one_trailer() and that > may make the result easier to follow? This is actually what I've already done (introducing helper functions to make this more readable and reduce nesting), but in the larger series (not in this patch series). I think in this patch I tried to avoid introducing new functions in order to keep the original "shape" of the existing refactored functions, including all of the nesting that they originally had. I wanted to keep the original shape because I thought that would make review simpler. > In any case, I didn't see anything glaringly wrong so far in this > series. Let me keep reading. > > Thanks. Thank you for reviewing! [1] https://lore.kernel.org/git/kl6l5y5qa34v.fsf@chooglen-macbookpro.roam.corp.google.com/
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c index adb74276281..934833a4645 100644 --- a/builtin/interpret-trailers.c +++ b/builtin/interpret-trailers.c @@ -140,6 +140,7 @@ static void interpret_trailers(const char *file, { LIST_HEAD(head); struct strbuf sb = STRBUF_INIT; + struct strbuf trailer_block = STRBUF_INIT; struct trailer_info info; FILE *outfile = stdout; @@ -170,7 +171,9 @@ static void interpret_trailers(const char *file, } /* Print trailer block. */ - format_trailers(outfile, &head, opts); + format_trailers(&head, opts, &trailer_block); + fwrite(trailer_block.buf, 1, trailer_block.len, outfile); + strbuf_release(&trailer_block); free_trailers(&head); trailer_info_release(&info); diff --git a/pretty.c b/pretty.c index cf964b060cd..f0721a5214f 100644 --- a/pretty.c +++ b/pretty.c @@ -1759,7 +1759,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ goto trailer_out; } if (*arg == ')') { - format_trailers_from_commit(sb, msg + c->subject_off, &opts); + format_trailers_from_commit(msg + c->subject_off, &opts, sb); ret = arg - placeholder + 1; } trailer_out: diff --git a/ref-filter.c b/ref-filter.c index 35b989e1dfe..7fb13818686 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1985,7 +1985,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp struct strbuf s = STRBUF_INIT; /* Format the trailer info according to the trailer_opts given */ - format_trailers_from_commit(&s, subpos, &atom->u.contents.trailer_opts); + format_trailers_from_commit(subpos, &atom->u.contents.trailer_opts, &s); v->s = strbuf_detach(&s, NULL); } else if (atom->u.contents.option == C_BARE) diff --git a/trailer.c b/trailer.c index 0ce7e9079ca..315d90ee1ab 100644 --- a/trailer.c +++ b/trailer.c @@ -162,19 +162,6 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val) fprintf(outfile, "%s%c %s\n", tok, separators[0], val); } -void format_trailers(FILE *outfile, struct list_head *head, - const struct process_trailer_options *opts) -{ - struct list_head *pos; - struct trailer_item *item; - list_for_each(pos, head) { - item = list_entry(pos, struct trailer_item, list); - if ((!opts->trim_empty || strlen(item->value) > 0) && - (!opts->only_trailers || item->token)) - print_tok_val(outfile, item->token, item->value); - } -} - static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok) { struct trailer_item *new_item = xcalloc(1, sizeof(*new_item)); @@ -984,6 +971,78 @@ static void unfold_value(struct strbuf *val) strbuf_release(&out); } +void format_trailers(struct list_head *head, + const struct process_trailer_options *opts, + struct strbuf *out) +{ + struct list_head *pos; + struct trailer_item *item; + int need_separator = 0; + + list_for_each(pos, head) { + item = list_entry(pos, struct trailer_item, list); + if (item->token) { + char c; + + struct strbuf tok = STRBUF_INIT; + struct strbuf val = STRBUF_INIT; + strbuf_addstr(&tok, item->token); + strbuf_addstr(&val, item->value); + + /* + * Skip key/value pairs where the value was empty. This + * can happen from trailers specified without a + * separator, like `--trailer "Reviewed-by"` (no + * corresponding value). + */ + if (opts->trim_empty && !strlen(item->value)) + continue; + + if (!opts->filter || opts->filter(&tok, opts->filter_data)) { + if (opts->unfold) + unfold_value(&val); + + if (opts->separator && need_separator) + strbuf_addbuf(out, opts->separator); + if (!opts->value_only) + strbuf_addbuf(out, &tok); + if (!opts->key_only && !opts->value_only) { + if (opts->key_value_separator) + strbuf_addbuf(out, opts->key_value_separator); + else { + c = last_non_space_char(tok.buf); + if (c) { + if (!strchr(separators, c)) + strbuf_addf(out, "%c ", separators[0]); + } + } + } + if (!opts->key_only) + strbuf_addbuf(out, &val); + if (!opts->separator) + strbuf_addch(out, '\n'); + + need_separator = 1; + } + + strbuf_release(&tok); + strbuf_release(&val); + } else if (!opts->only_trailers) { + if (opts->separator && need_separator) { + strbuf_addbuf(out, opts->separator); + } + strbuf_addstr(out, item->value); + if (opts->separator) + strbuf_rtrim(out); + else + strbuf_addch(out, '\n'); + + need_separator = 1; + } + + } +} + /* * Parse trailers in "str", populating the trailer info and "head" * linked list structure. @@ -1144,13 +1203,25 @@ static void format_trailer_info(struct strbuf *out, } -void format_trailers_from_commit(struct strbuf *out, const char *msg, - const struct process_trailer_options *opts) +void format_trailers_from_commit(const char *msg, + const struct process_trailer_options *opts, + struct strbuf *out) { + LIST_HEAD(head); struct trailer_info info; - trailer_info_get(&info, msg, opts); - format_trailer_info(out, &info, msg, opts); + parse_trailers(&info, msg, &head, opts); + + /* If we want the whole block untouched, we can take the fast path. */ + if (!opts->only_trailers && !opts->unfold && !opts->filter && + !opts->separator && !opts->key_only && !opts->value_only && + !opts->key_value_separator) { + strbuf_add(out, msg + info.trailer_block_start, + info.trailer_block_end - info.trailer_block_start); + } else + format_trailers(&head, opts, out); + + free_trailers(&head); trailer_info_release(&info); } diff --git a/trailer.h b/trailer.h index 0e4f0ece9b3..50f70556302 100644 --- a/trailer.h +++ b/trailer.h @@ -102,21 +102,16 @@ void trailer_info_release(struct trailer_info *info); void trailer_config_init(void); void free_trailers(struct list_head *trailers); -void format_trailers(FILE *outfile, struct list_head *head, - const struct process_trailer_options *opts); +void format_trailers(struct list_head *head, + const struct process_trailer_options *opts, + struct strbuf *out); /* - * Format the trailers from the commit msg "msg" into the strbuf "out". - * Note two caveats about "opts": - * - * - this is primarily a helper for pretty.c, and not - * all of the flags are supported. - * - * - this differs from format_trailers slightly in that we always format - * only the trailer block itself, even if the "only_trailers" option is not - * set. + * Convenience function to format the trailers from the commit msg "msg" into + * the strbuf "out". Reuses format_trailers internally. */ -void format_trailers_from_commit(struct strbuf *out, const char *msg, - const struct process_trailer_options *opts); +void format_trailers_from_commit(const char *msg, + const struct process_trailer_options *opts, + struct strbuf *out); /* * An interface for iterating over the trailers found in a particular commit