| Message ID | pull.1632.v6.git.1709252086.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Enrich Trailer API | expand |
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes: > This patch series is the first 9 patches of a larger cleanup/bugfix series > (henceforth "larger series") I've been working on. The main goal of this > series is to begin the process of "libifying" the trailer API. By "API" I > mean the interface exposed in trailer.h. The larger series brings a number > of additional cleanups (exposing and fixing some bugs along the way), and > builds on top of this series. > > When the larger series is merged, we will be in a good state to additionally > pursue the following goals: > > 1. "API reuse inside Git": make the API expressive enough to eliminate any > need by other parts of Git to use the interpret-trailers builtin as a > subprocess (instead they could just use the API directly); > 2. "API stability": add unit tests to codify the expected behavior of API > functions; and > 3. "API documentation": create developer-focused documentation to explain > how to use the API effectively, noting any API limitations or > anti-patterns. > > In the future after libification is "complete", users external to Git will > be able to use the same trailer processing API used by the > interpret-trailers builtin. For example, a web server may want to parse > trailers the same way that Git would parse them, without having to call > interpret-trailers as a subprocess. This use case was the original > motivation behind my work in this area. > > With the libification-focused goals out of the way, let's turn to this patch > series in more detail. > > In summary this series breaks up "process_trailers()" into smaller pieces, > exposing many of the parts relevant to trailer-related processing in > trailer.h. This will force us to eventually introduce unit tests for these > API functions, but that is a good thing for API stability. We also perform > some preparatory refactors in order to help us unify the trailer formatting > machinery toward the end of this series. > > > Notable changes in v6 > ===================== > > * Mainly wording changes to commit messages. Thanks to Christian for the > suggestions. It's been nearly a week since this was posted. Any more comments, or is everybody happy with this iteration? Otherwise I am tempted to mark the topic for 'next' soon. Thanks.
On 2024.03.05 10:03, Junio C Hamano wrote: > > It's been nearly a week since this was posted. Any more comments, > or is everybody happy with this iteration? Otherwise I am tempted > to mark the topic for 'next' soon. > > Thanks. I scanned through v6 yesterday and have nothing new to add. LGTM.
Josh Steadmon <steadmon@google.com> writes: > On 2024.03.05 10:03, Junio C Hamano wrote: >> >> It's been nearly a week since this was posted. Any more comments, >> or is everybody happy with this iteration? Otherwise I am tempted >> to mark the topic for 'next' soon. >> >> Thanks. > > I scanned through v6 yesterday and have nothing new to add. LGTM. Thanks.
On Tue, Mar 5, 2024 at 8:07 PM Josh Steadmon <steadmon@google.com> wrote: > > On 2024.03.05 10:03, Junio C Hamano wrote: > > > > It's been nearly a week since this was posted. Any more comments, > > or is everybody happy with this iteration? Otherwise I am tempted > > to mark the topic for 'next' soon. > > > > Thanks. > > I scanned through v6 yesterday and have nothing new to add. LGTM. I took another look at it, and I am fine with it now too. Acked.
Christian Couder <christian.couder@gmail.com> writes: > On Tue, Mar 5, 2024 at 8:07 PM Josh Steadmon <steadmon@google.com> wrote: >> >> On 2024.03.05 10:03, Junio C Hamano wrote: >> > >> > It's been nearly a week since this was posted. Any more comments, >> > or is everybody happy with this iteration? Otherwise I am tempted >> > to mark the topic for 'next' soon. >> > >> > Thanks. >> >> I scanned through v6 yesterday and have nothing new to add. LGTM. > > I took another look at it, and I am fine with it now too. Acked. Thanks.
Christian Couder <christian.couder@gmail.com> writes: > On Tue, Mar 5, 2024 at 8:07 PM Josh Steadmon <steadmon@google.com> wrote: >> >> On 2024.03.05 10:03, Junio C Hamano wrote: >> > >> > It's been nearly a week since this was posted. Any more comments, >> > or is everybody happy with this iteration? Otherwise I am tempted >> > to mark the topic for 'next' soon. >> > >> > Thanks. >> >> I scanned through v6 yesterday and have nothing new to add. LGTM. > > I took another look at it, and I am fine with it now too. Acked. Thanks.
This patch series is the first 9 patches of a larger cleanup/bugfix series (henceforth "larger series") I've been working on. The main goal of this series is to begin the process of "libifying" the trailer API. By "API" I mean the interface exposed in trailer.h. The larger series brings a number of additional cleanups (exposing and fixing some bugs along the way), and builds on top of this series. When the larger series is merged, we will be in a good state to additionally pursue the following goals: 1. "API reuse inside Git": make the API expressive enough to eliminate any need by other parts of Git to use the interpret-trailers builtin as a subprocess (instead they could just use the API directly); 2. "API stability": add unit tests to codify the expected behavior of API functions; and 3. "API documentation": create developer-focused documentation to explain how to use the API effectively, noting any API limitations or anti-patterns. In the future after libification is "complete", users external to Git will be able to use the same trailer processing API used by the interpret-trailers builtin. For example, a web server may want to parse trailers the same way that Git would parse them, without having to call interpret-trailers as a subprocess. This use case was the original motivation behind my work in this area. With the libification-focused goals out of the way, let's turn to this patch series in more detail. In summary this series breaks up "process_trailers()" into smaller pieces, exposing many of the parts relevant to trailer-related processing in trailer.h. This will force us to eventually introduce unit tests for these API functions, but that is a good thing for API stability. We also perform some preparatory refactors in order to help us unify the trailer formatting machinery toward the end of this series. Notable changes in v6 ===================== * Mainly wording changes to commit messages. Thanks to Christian for the suggestions. Notable changes in v5 ===================== * Removed patches 10+ from this series. Thanks to Christian for the suggestion. * Reworded the log message of patch 09 to reflect the above arrangement, as suggested by Christian. Notable changes in v4 ===================== * Patches 3, 4, 5, and 8 have been broken up into smaller steps. There are 28 instead of 10 patches now, but these 28 should be much easier to review than the (previously condensed) 10. * NEW Patch 1: "trailer: free trailer_info after all related usage" fixes awkward use-after-free coding style * NEW Patch 2: "shortlog: add test for de-duplicating folded trailers" increases test coverage related to trailer iterators and "unfold_value()" * NEW Patch 27: "trailer_set_*(): put out parameter at the end" is a small refactor to reorder parameters. * Patches 5-16: These smaller patches make up Patch 3 from v3. * Patches 17-18: These smaller patches make up Patch 4 from v3. * Patches 19-20: These smaller patches make up Patch 5 from v3. * Patches 23-26: These smaller patches make up Patch 8 from v3. * Anonymize unambiguous parameters in <trailer.h>. Notable changes in v3 ===================== * Squashed Patch 4 into Patch 3 ("trailer: unify trailer formatting machinery"), to avoid breaking the build ("-Werror=unused-function" violations) * NEW (Patch 10): Introduce "trailer template" terminology for readability (no behavioral change) * (API function) Rename default_separators() to trailer_default_separators() * (API function) Rename new_trailers_clear() to free_trailer_templates() * trailer.h: for single-parameter functions, anonymize the parameter name to reduce verbosity Notable changes in v2 ===================== * (cover letter) Discuss goals of the larger series in more detail, especially the pimpl idiom * (cover letter) List bug fixes pending in the larger series that depend on this series * Reorder function parameters to have trailer options at the beginning (and out parameters toward the end) * "sequencer: use the trailer iterator": prefer C string instead of strbuf for new "raw" field * Patch 1 (was Patch 2) also renames ensure_configured() to trailer_config_init() (forgot to rename this one previously) Linus Arver (9): trailer: free trailer_info _after_ all related usage shortlog: add test for de-duplicating folded trailers trailer: rename functions to use 'trailer' trailer: move interpret_trailers() to interpret-trailers.c trailer: reorder format_trailers_from_commit() parameters trailer_info_get(): reorder parameters format_trailers(): use strbuf instead of FILE format_trailer_info(): move "fast path" to caller format_trailers_from_commit(): indirectly call trailer_info_get() builtin/interpret-trailers.c | 101 ++++++++++++++++++- pretty.c | 2 +- ref-filter.c | 2 +- sequencer.c | 2 +- t/t4201-shortlog.sh | 32 +++++++ trailer.c | 181 +++++++++-------------------------- trailer.h | 31 ++++-- 7 files changed, 204 insertions(+), 147 deletions(-) base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1632%2Flistx%2Ftrailer-api-refactor-part-1-v6 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1632/listx/trailer-api-refactor-part-1-v6 Pull-Request: https://github.com/gitgitgadget/git/pull/1632 Range-diff vs v5: 1: 652df25f30e = 1: 652df25f30e trailer: free trailer_info _after_ all related usage 2: fdccaca2ba0 = 2: fdccaca2ba0 shortlog: add test for de-duplicating folded trailers 3: 4372af244f0 ! 3: 7b1d739cddb trailer: prepare to expose functions as part of API @@ Metadata Author: Linus Arver <linusa@google.com> ## Commit message ## - trailer: prepare to expose functions as part of API + trailer: rename functions to use 'trailer' - In the next patch, we will move "process_trailers" from trailer.c to - builtin/interpret-trailers.c. That move will necessitate the growth of - the trailer.h API, forcing us to expose some additional functions in + Rename process_trailers() to interpret_trailers(), because it matches + the name for the builtin command of the same name + (git-interpret-trailers), which is the sole user of process_trailers(). + + In a following commit, we will move "interpret_trailers" from trailer.c + to builtin/interpret-trailers.c. That move will necessitate the growth + of the trailer.h API, forcing us to expose some additional functions in trailer.h. Rename relevant functions so that they include the term "trailer" in @@ Commit message them by their "trailer" moniker, just like all the other functions already exposed by trailer.h. - Take the opportunity to start putting trailer processing options (opts) - as the first parameter. This will be the pattern going forward in this - series. + Rename `struct list_head *head` to `struct list_head *trailers` because + "head" conveys no additional information beyond the "list_head" type. + + 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. Parameters like `FILE *outfile` should be last + because they are a kind of 'out' parameter, so put such parameters at + the end. This will be the pattern going forward in this series. Helped-by: Junio C Hamano <gitster@pobox.com> + Helped-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Linus Arver <linusa@google.com> ## builtin/interpret-trailers.c ## 4: 4073b8eb510 = 4: 7ac4da3019a trailer: move interpret_trailers() to interpret-trailers.c 5: b2a0f7829a1 ! 5: 47c994ce025 trailer: start preparing for formatting unification @@ Metadata Author: Linus Arver <linusa@google.com> ## Commit message ## - trailer: start preparing for formatting unification + trailer: reorder format_trailers_from_commit() parameters Currently there are two functions for formatting trailers in <trailer.h>: @@ Commit message last, because it's an "out parameter" (something that the caller wants to use as the output of this function). + Similarly, reorder parameters for format_trailer_info(), because later + on we will unify the two together. + Signed-off-by: Linus Arver <linusa@google.com> ## pretty.c ## 6: c1760f80356 = 6: 7a565580167 trailer_info_get(): reorder parameters 7: 9dc912b5bc5 = 7: 46c7f4c0e81 format_trailers(): use strbuf instead of FILE 8: b97c06d8bc3 = 8: 26b1f19d0e1 format_trailer_info(): move "fast path" to caller 9: 7c656b3f775 ! 9: 0e884d870c8 format_trailers_from_commit(): indirectly call trailer_info_get() @@ Commit message This is another preparatory refactor to unify the trailer formatters. - Instead of calling trailer_info_get() directly, call parse_trailers() - which already calls trailer_info_get(). This change is a NOP because - format_trailer_info() only looks at the "trailers" string array, not the - trailer_item objects which parse_trailers() populates. + For background, note that the "trailers" string array is the + `char **trailers` member in `struct trailer_info` and that the + trailer_item objects are the elements of the `struct list_head *head` + linked list. + + Currently trailer_info_get() only populates `char **trailers`. And + parse_trailers() first calls trailer_info_get() so that it can use the + `char **trailers` to populate a list of `struct trailer_item` objects + + Instead of calling trailer_info_get() directly from + format_trailers_from_commit(), make it call parse_trailers() instead + because parse_trailers() already calls trailer_info_get(). + + This change is a NOP because format_trailer_info() (which + format_trailers_from_commit() wraps around) only looks at the "trailers" + string array, not the trailer_item objects which parse_trailers() + populates. For now we do need to create a dummy + + LIST_HEAD(trailer_objects); + + because parse_trailers() expects it in its signature. In a future patch, we'll change format_trailer_info() to use the parsed - trailer_item objects instead of the string array. + trailer_item objects (trailer_objects) instead of the `char **trailers` + array. Signed-off-by: Linus Arver <linusa@google.com> @@ trailer.c: void format_trailers_from_commit(const struct process_trailer_options const char *msg, struct strbuf *out) { -+ LIST_HEAD(trailers); ++ LIST_HEAD(trailer_objects); struct trailer_info info; - trailer_info_get(opts, msg, &info); -+ parse_trailers(opts, &info, msg, &trailers); ++ parse_trailers(opts, &info, msg, &trailer_objects); + /* If we want the whole block untouched, we can take the fast path. */ if (!opts->only_trailers && !opts->unfold && !opts->filter && @@ trailer.c: void format_trailers_from_commit(const struct process_trailer_options } else format_trailer_info(opts, &info, out); -+ free_trailers(&trailers); ++ free_trailers(&trailer_objects); trailer_info_release(&info); }