| Message ID | b2a0f7829a1c5f2822e9a896ffe3744587ff1298.1708124951.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | 77d25a9c9dbb6bb73de8a894e45face745b5050e |
| Headers | show |
| Series | Enrich Trailer API | expand |
On Sat, Feb 17, 2024 at 12:09 AM Linus Arver via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Linus Arver <linusa@google.com> > > Currently there are two functions for formatting trailers in > <trailer.h>: > > void format_trailers(const struct process_trailer_options *, > struct list_head *trailers, FILE *outfile); > > void format_trailers_from_commit(struct strbuf *out, const char *msg, > const struct process_trailer_options *opts); > > and although they are similar enough (even taking the same > process_trailer_options struct pointer) they are used quite differently. > One might intuitively think that format_trailers_from_commit() builds on > top of format_trailers(), but this is not the case. Instead > format_trailers_from_commit() calls format_trailer_info() and > format_trailers() is never called in that codepath. > > This is a preparatory refactor to help us deprecate format_trailers() in > favor of format_trailer_info() (at which point we can rename the latter > to the former). When the deprecation is complete, both > format_trailers_from_commit(), and the interpret-trailers builtin will > be able to call into the same helper function (instead of > format_trailers() and format_trailer_info(), respectively). Unifying the > formatters is desirable because it simplifies the API. > > Reorder parameters for format_trailers_from_commit() to prefer > > const struct process_trailer_options *opts > > as the first parameter, because these options are intimately tied to > formatting trailers. And take > > struct strbuf *out > > last, because it's an "out parameter" (something that the caller wants > to use as the output of this function). Here also I think the subject could be more specific like for example: "trailer: reorder format_trailers_from_commit() parameters" > diff --git a/trailer.c b/trailer.c > index d23afa0a65c..5025be97899 100644 > --- a/trailer.c > +++ b/trailer.c > @@ -1083,10 +1083,10 @@ void trailer_info_release(struct trailer_info *info) > free(info->trailers); > } > > -static void format_trailer_info(struct strbuf *out, > +static void format_trailer_info(const struct process_trailer_options *opts, > const struct trailer_info *info, > const char *msg, > - const struct process_trailer_options *opts) > + struct strbuf *out) Ok, so it's not just format_trailers_from_commit() parameters that are reordered, but also format_trailer_info() parameters. It would be nice if the commit message mentioned it.
Christian Couder <christian.couder@gmail.com> writes: > On Sat, Feb 17, 2024 at 12:09 AM Linus Arver via GitGitGadget > <gitgitgadget@gmail.com> wrote: >> >> From: Linus Arver <linusa@google.com> >> >> Currently there are two functions for formatting trailers in >> <trailer.h>: >> >> void format_trailers(const struct process_trailer_options *, >> struct list_head *trailers, FILE *outfile); >> >> void format_trailers_from_commit(struct strbuf *out, const char *msg, >> const struct process_trailer_options *opts); >> >> and although they are similar enough (even taking the same >> process_trailer_options struct pointer) they are used quite differently. >> One might intuitively think that format_trailers_from_commit() builds on >> top of format_trailers(), but this is not the case. Instead >> format_trailers_from_commit() calls format_trailer_info() and >> format_trailers() is never called in that codepath. >> >> This is a preparatory refactor to help us deprecate format_trailers() in >> favor of format_trailer_info() (at which point we can rename the latter >> to the former). When the deprecation is complete, both >> format_trailers_from_commit(), and the interpret-trailers builtin will >> be able to call into the same helper function (instead of >> format_trailers() and format_trailer_info(), respectively). Unifying the >> formatters is desirable because it simplifies the API. >> >> Reorder parameters for format_trailers_from_commit() to prefer >> >> const struct process_trailer_options *opts >> >> as the first parameter, because these options are intimately tied to >> formatting trailers. And take >> >> struct strbuf *out >> >> last, because it's an "out parameter" (something that the caller wants >> to use as the output of this function). > > Here also I think the subject could be more specific like for example: > > "trailer: reorder format_trailers_from_commit() parameters" Applied, thanks. >> diff --git a/trailer.c b/trailer.c >> index d23afa0a65c..5025be97899 100644 >> --- a/trailer.c >> +++ b/trailer.c >> @@ -1083,10 +1083,10 @@ void trailer_info_release(struct trailer_info *info) >> free(info->trailers); >> } >> >> -static void format_trailer_info(struct strbuf *out, >> +static void format_trailer_info(const struct process_trailer_options *opts, >> const struct trailer_info *info, >> const char *msg, >> - const struct process_trailer_options *opts) >> + struct strbuf *out) > > Ok, so it's not just format_trailers_from_commit() parameters that are > reordered, but also format_trailer_info() parameters. It would be nice > if the commit message mentioned it. Agreed. Will do.
diff --git a/pretty.c b/pretty.c index cf964b060cd..bdbed4295aa 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(&opts, msg + c->subject_off, sb); ret = arg - placeholder + 1; } trailer_out: diff --git a/ref-filter.c b/ref-filter.c index 35b989e1dfe..d358953b0ce 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(&atom->u.contents.trailer_opts, subpos, &s); v->s = strbuf_detach(&s, NULL); } else if (atom->u.contents.option == C_BARE) diff --git a/trailer.c b/trailer.c index d23afa0a65c..5025be97899 100644 --- a/trailer.c +++ b/trailer.c @@ -1083,10 +1083,10 @@ void trailer_info_release(struct trailer_info *info) free(info->trailers); } -static void format_trailer_info(struct strbuf *out, +static void format_trailer_info(const struct process_trailer_options *opts, const struct trailer_info *info, const char *msg, - const struct process_trailer_options *opts) + struct strbuf *out) { size_t origlen = out->len; size_t i; @@ -1144,13 +1144,14 @@ 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 struct process_trailer_options *opts, + const char *msg, + struct strbuf *out) { struct trailer_info info; trailer_info_get(&info, msg, opts); - format_trailer_info(out, &info, msg, opts); + format_trailer_info(opts, &info, msg, out); trailer_info_release(&info); } diff --git a/trailer.h b/trailer.h index c292d44b62f..c6d3ee49bbf 100644 --- a/trailer.h +++ b/trailer.h @@ -115,8 +115,9 @@ void free_trailers(struct list_head *); * only the trailer block itself, even if the "only_trailers" option is not * set. */ -void format_trailers_from_commit(struct strbuf *out, const char *msg, - const struct process_trailer_options *opts); +void format_trailers_from_commit(const struct process_trailer_options *opts, + const char *msg, + struct strbuf *out); /* * An interface for iterating over the trailers found in a particular commit