| Message ID | 31725832224e3d6b14066af8a87eaf4ab589179e.1707196348.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> > > Rename format_trailer_info() to format_trailers(). Finally, both > interpret-trailers and format_trailers_from_commit() can call > "format_trailers()"! > > Update the comment in <trailer.h> to remove the (now obsolete) caveats > about format_trailers_from_commit(). Nice.
On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget <gitgitgadget@gmail.com> wrote: > /* > - * 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 process_trailers slightly in that we always format > - * only the trailer block itself, even if the "only_trailers" option is not > - * set. This makes me wonder if there was actually a good reason why format_trailers() and format_trailer_info() were 2 different functions. Is there info about this in the commit message of the commit which introduced this comment?
Christian Couder <christian.couder@gmail.com> writes: > On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget > <gitgitgadget@gmail.com> wrote: > >> /* >> - * 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 process_trailers slightly in that we always format >> - * only the trailer block itself, even if the "only_trailers" option is not >> - * set. > > This makes me wonder if there was actually a good reason why > format_trailers() and format_trailer_info() were 2 different > functions. Is there info about this in the commit message of the > commit which introduced this comment? Good question. I see a388b10fc1 (pretty: move trailer formatting to trailer.c, 2017-08-15) and there it says: pretty: move trailer formatting to trailer.c The next commit will add many features to the %(trailer) placeholder in pretty.c. We'll need to access some internal functions of trailer.c for that, so our options are either: 1. expose those functions publicly or 2. make an entry point into trailer.c to do the formatting Doing (2) ends up exposing less surface area, though do note that caveats in the docstring of the new function. so it looks like this function started out from pretty.c and did not have access to all of the trailer implementation internals, and was never intended to replace (unify) the formatting machinery in trailer.c both before and after that commit. This seems like good information to include in the commit message, so I will do so in the next reroll. Aside: it is interesting that the current patch series is taking the direction of (1) instead of (2) as was done in the past (we had a choice to "libify" at that time but did not do so, in order to "expos[e] less surface area [in <trailer.h>]").
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c index f57af0db37b..11f4ce9e4a2 100644 --- a/builtin/interpret-trailers.c +++ b/builtin/interpret-trailers.c @@ -171,7 +171,7 @@ static void interpret_trailers(const struct process_trailer_options *opts, } /* Print trailer block. */ - format_trailer_info(opts, &head, &trailer_block); + format_trailers(opts, &head, &trailer_block); free_trailers(&head); fwrite(trailer_block.buf, 1, trailer_block.len, outfile); strbuf_release(&trailer_block); diff --git a/trailer.c b/trailer.c index 5c42a19943a..4f3318802d1 100644 --- a/trailer.c +++ b/trailer.c @@ -1053,9 +1053,9 @@ void trailer_info_release(struct trailer_info *info) free(info->trailers); } -void format_trailer_info(const struct process_trailer_options *opts, - struct list_head *trailers, - struct strbuf *out) +void format_trailers(const struct process_trailer_options *opts, + struct list_head *trailers, + struct strbuf *out) { size_t origlen = out->len; struct list_head *pos; @@ -1129,7 +1129,7 @@ void format_trailers_from_commit(const struct process_trailer_options *opts, strbuf_add(out, msg + info.trailer_block_start, info.trailer_block_end - info.trailer_block_start); } else - format_trailer_info(opts, &trailers, out); + format_trailers(opts, &trailers, out); free_trailers(&trailers); trailer_info_release(&info); diff --git a/trailer.h b/trailer.h index 3c13006a4c1..9f42aa75994 100644 --- a/trailer.h +++ b/trailer.h @@ -101,23 +101,16 @@ void trailer_info_get(const struct process_trailer_options *, void trailer_info_release(struct trailer_info *info); void trailer_config_init(void); -void format_trailer_info(const struct process_trailer_options *, +void format_trailers(const struct process_trailer_options *, struct list_head *trailers, struct strbuf *out); void free_trailers(struct list_head *); /* - * 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 process_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(const struct process_trailer_options *opts, +void format_trailers_from_commit(const struct process_trailer_options *, const char *msg, struct strbuf *out);