NOTE: This series is based on the la/format-trailer-info topic branch (see its discussion at [1]). This series is based on the initial series [2], notably the v4 version of patches 17-20 as suggested by Christian [3]. This version addresses the review comments for those patches, namely the splitting up of Patch 19 there into 3 separate patches [4] (as Patches 05-07 here) . The central idea is to make the trailer_info struct private (that is, move its definition from trailer.h to trailer.c) --- aka the "pimpl" idiom. See the detailed commit message for Patch 07 for the motivation behind the change. Patch 04 makes sequencer.c a well-behaved trailer API consumer, by making use of the trailer iterator. Patch 03 prepares us for Patch 04. Patch 08 slightly reduces the weight of the API by removing (from the API surface) an unused function. Notable changes in v4 ===================== * Drop "25%" language in Patch 03 * Rename some variables * Update patch emails to personal (linus@xxxxxxxx) email Notable changes in v3 ===================== * (NEW Patch 10) Expand test coverage to check the contents of each iteration (raw, key, val fields), not just the total number of iterations * (NEW Patch 09) Add documentation in <trailer.h> for using parse_trailers() * (unrelated) I will lose access to my linusa@xxxxxxxxxx email address tomorrow (I'm switching jobs!) and so future emails from me will come from linus@xxxxxxxx [5]. I've added the latter email to the CC list here so things should just work. Cheers Notable changes in v2 ===================== * Add unit tests at the beginning of the series (Patches 01 and 02) and use it to verify that the other edge cases remain unchanged when we add the "raw" member (Patch 03) [1] https://lore.kernel.org/git/pull.1694.git.1710485706.gitgitgadget@xxxxxxxxx/ [2] https://lore.kernel.org/git/pull.1632.v4.git.1707196348.gitgitgadget@xxxxxxxxx/ [3] https://lore.kernel.org/git/CAP8UFD08F0V13X0+CJ1uhMPzPWVMs2okGVMJch0DkQg5M3BWLA@xxxxxxxxxxxxxx/ [4] https://lore.kernel.org/git/CAP8UFD1twELGKvvesxgCrZrypKZpgSt04ira3mvurG1UbpDfxQ@xxxxxxxxxxxxxx/ [5] https://lore.kernel.org/git/pull.1720.git.1713309711217.gitgitgadget@xxxxxxxxx/ Linus Arver (10): Makefile: sort UNIT_TEST_PROGRAMS trailer: add unit tests for trailer iterator trailer: teach iterator about non-trailer lines sequencer: use the trailer iterator interpret-trailers: access trailer_info with new helpers trailer: make parse_trailers() return trailer_info pointer trailer: make trailer_info struct private trailer: retire trailer_info_get() from API trailer: document parse_trailers() usage trailer unit tests: inspect iterator contents Makefile | 5 +- builtin/interpret-trailers.c | 12 +- sequencer.c | 27 ++- t/unit-tests/t-trailer.c | 315 +++++++++++++++++++++++++++++++++++ trailer.c | 167 ++++++++++++------- trailer.h | 94 +++++++---- 6 files changed, 506 insertions(+), 114 deletions(-) create mode 100644 t/unit-tests/t-trailer.c base-commit: 3452d173241c8b87ecdd67f91f594cb14327e394 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1696%2Flistx%2Ftrailer-api-part-3-v4 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1696/listx/trailer-api-part-3-v4 Pull-Request: https://github.com/gitgitgadget/git/pull/1696 Range-diff vs v3: 1: b6a1304f8ae ! 1: 8a9f71442d8 Makefile: sort UNIT_TEST_PROGRAMS @@ ## Metadata ## -Author: Linus Arver <linusa@xxxxxxxxxx> +Author: Linus Arver <linus@xxxxxxxx> ## Commit message ## Makefile: sort UNIT_TEST_PROGRAMS - Signed-off-by: Linus Arver <linusa@xxxxxxxxxx> + Signed-off-by: Linus Arver <linus@xxxxxxxx> ## Makefile ## @@ Makefile: THIRD_PARTY_SOURCES += sha1collisiondetection/% 2: 4ad0fbbb33c ! 2: b503b539c6f trailer: add unit tests for trailer iterator @@ ## Metadata ## -Author: Linus Arver <linusa@xxxxxxxxxx> +Author: Linus Arver <linus@xxxxxxxx> ## Commit message ## trailer: add unit tests for trailer iterator @@ Commit message implementation details which are, unlike the API, subject to drastic changes). - Signed-off-by: Linus Arver <linusa@xxxxxxxxxx> + Signed-off-by: Linus Arver <linus@xxxxxxxx> ## Makefile ## @@ Makefile: UNIT_TEST_PROGRAMS += t-ctype 3: 9077d5a315d ! 3: 4aeb48050b1 trailer: teach iterator about non-trailer lines @@ ## Metadata ## -Author: Linus Arver <linusa@xxxxxxxxxx> +Author: Linus Arver <linus@xxxxxxxx> ## Commit message ## trailer: teach iterator about non-trailer lines @@ Commit message for non-trailer lines, making the comparison still work even with this commit). - Rename "num_expected_trailers" to "num_expected_objects" in + Rename "num_expected_trailers" to "num_expected" in t/unit-tests/t-trailer.c because the items we iterate over now include non-trailer lines. - Signed-off-by: Linus Arver <linusa@xxxxxxxxxx> + Signed-off-by: Linus Arver <linus@xxxxxxxx> ## t/unit-tests/t-trailer.c ## @@ @@ t/unit-tests/t-trailer.c #include "trailer.h" -static void t_trailer_iterator(const char *msg, size_t num_expected_trailers) -+static void t_trailer_iterator(const char *msg, size_t num_expected_objects) ++static void t_trailer_iterator(const char *msg, size_t num_expected) { struct trailer_iterator iter; size_t i = 0; @@ t/unit-tests/t-trailer.c: static void t_trailer_iterator(const char *msg, size_t trailer_iterator_release(&iter); - check_uint(i, ==, num_expected_trailers); -+ check_uint(i, ==, num_expected_objects); ++ check_uint(i, ==, num_expected); } static void run_t_trailer_iterator(void) @@ t/unit-tests/t-trailer.c: static void run_t_trailer_iterator(void) const char *name; const char *msg; - size_t num_expected_trailers; -+ size_t num_expected_objects; ++ size_t num_expected; } tc[] = { { "empty input", @@ t/unit-tests/t-trailer.c: static void run_t_trailer_iterator(void) for (int i = 0; i < sizeof(tc) / sizeof(tc[0]); i++) { TEST(t_trailer_iterator(tc[i].msg, - tc[i].num_expected_trailers), -+ tc[i].num_expected_objects), ++ tc[i].num_expected), "%s", tc[i].name); } } @@ trailer.h: void format_trailers_from_commit(const struct process_trailer_options struct trailer_iterator { + /* + * Raw line (e.g., "foo: bar baz") before being parsed as a trailer -+ * key/val pair as part of a trailer block. A trailer block can be -+ * either 100% trailer lines, or mixed in with non-trailer lines (in -+ * which case at least 25% must be trailer lines). ++ * key/val pair as part of a trailer block (as the "key" and "val" ++ * fields below). If a line fails to parse as a trailer, then the "key" ++ * will be the entire line and "val" will be the empty string. + */ + const char *raw; -+ struct strbuf key; struct strbuf val; 4: 4a1d18da574 ! 4: a3d080d4d6c sequencer: use the trailer iterator @@ ## Metadata ## -Author: Linus Arver <linusa@xxxxxxxxxx> +Author: Linus Arver <linus@xxxxxxxx> ## Commit message ## sequencer: use the trailer iterator @@ Commit message before when we iterated over the unparsed string array (char **trailers) in trailer_info. - Signed-off-by: Linus Arver <linusa@xxxxxxxxxx> + Signed-off-by: Linus Arver <linus@xxxxxxxx> ## sequencer.c ## @@ sequencer.c: static const char *get_todo_path(const struct replay_opts *opts) 5: 460979ba964 ! 5: 44df42ca503 interpret-trailers: access trailer_info with new helpers @@ ## Metadata ## -Author: Linus Arver <linusa@xxxxxxxxxx> +Author: Linus Arver <linus@xxxxxxxx> ## Commit message ## interpret-trailers: access trailer_info with new helpers @@ Commit message implementation (and thus hidden from the API). Helped-by: Christian Couder <chriscool@xxxxxxxxxxxxx> - Signed-off-by: Linus Arver <linusa@xxxxxxxxxx> + Signed-off-by: Linus Arver <linus@xxxxxxxx> ## builtin/interpret-trailers.c ## @@ builtin/interpret-trailers.c: static void interpret_trailers(const struct process_trailer_options *opts, 6: d217858c637 ! 6: 9ed7cef9d29 trailer: make parse_trailers() return trailer_info pointer @@ ## Metadata ## -Author: Linus Arver <linusa@xxxxxxxxxx> +Author: Linus Arver <linus@xxxxxxxx> ## Commit message ## trailer: make parse_trailers() return trailer_info pointer @@ Commit message format_trailers_from_commit() and trailer_iterator_init() accordingly. Helped-by: Christian Couder <chriscool@xxxxxxxxxxxxx> - Signed-off-by: Linus Arver <linusa@xxxxxxxxxx> + Signed-off-by: Linus Arver <linus@xxxxxxxx> ## builtin/interpret-trailers.c ## @@ builtin/interpret-trailers.c: static void interpret_trailers(const struct process_trailer_options *opts, 7: 49c66c48cc1 ! 7: 246ac9a5d07 trailer: make trailer_info struct private @@ ## Metadata ## -Author: Linus Arver <linusa@xxxxxxxxxx> +Author: Linus Arver <linus@xxxxxxxx> ## Commit message ## trailer: make trailer_info struct private @@ Commit message Helped-by: Junio C Hamano <gitster@xxxxxxxxx> Helped-by: Christian Couder <chriscool@xxxxxxxxxxxxx> - Signed-off-by: Linus Arver <linusa@xxxxxxxxxx> + Signed-off-by: Linus Arver <linus@xxxxxxxx> ## trailer.c ## @@ 8: 56e1cca4b7b ! 8: ca6f0c4208c trailer: retire trailer_info_get() from API @@ ## Metadata ## -Author: Linus Arver <linusa@xxxxxxxxxx> +Author: Linus Arver <linus@xxxxxxxx> ## Commit message ## trailer: retire trailer_info_get() from API @@ Commit message We have to also reposition it to be above parse_trailers(), which depends on it. - Signed-off-by: Linus Arver <linusa@xxxxxxxxxx> + Signed-off-by: Linus Arver <linus@xxxxxxxx> ## trailer.c ## @@ trailer.c: static struct trailer_info *trailer_info_new(void) 9: 35304837e08 ! 9: c1a0f1bed04 trailer: document parse_trailers() usage @@ ## Metadata ## -Author: Linus Arver <linusa@xxxxxxxxxx> +Author: Linus Arver <linus@xxxxxxxx> ## Commit message ## trailer: document parse_trailers() usage @@ Commit message comments a bit easier to read (because "head" itself doesn't really have any domain-specific meaning here). - Signed-off-by: Linus Arver <linusa@xxxxxxxxxx> + Signed-off-by: Linus Arver <linus@xxxxxxxx> ## trailer.c ## @@ trailer.c: static struct trailer_info *trailer_info_get(const struct process_trailer_option 10: 4d53707f836 ! 10: 310b632ddfd trailer unit tests: inspect iterator contents @@ ## Metadata ## -Author: Linus Arver <linusa@xxxxxxxxxx> +Author: Linus Arver <linus@xxxxxxxx> ## Commit message ## trailer unit tests: inspect iterator contents @@ Commit message iteration. Helped-by: Junio C Hamano <gitster@xxxxxxxxx> - Signed-off-by: Linus Arver <linusa@xxxxxxxxxx> + Signed-off-by: Linus Arver <linus@xxxxxxxx> ## t/unit-tests/t-trailer.c ## @@ #include "test-lib.h" #include "trailer.h" --static void t_trailer_iterator(const char *msg, size_t num_expected_objects) -+struct trailer_assertions { +-static void t_trailer_iterator(const char *msg, size_t num_expected) ++struct contents { + const char *raw; + const char *key; + const char *val; +}; + -+static void t_trailer_iterator(const char *msg, size_t num_expected_objects, -+ struct trailer_assertions *trailer_assertions) ++static void t_trailer_iterator(const char *msg, size_t num_expected, ++ struct contents *contents) { struct trailer_iterator iter; size_t i = 0; @@ t/unit-tests/t-trailer.c trailer_iterator_init(&iter, msg); - while (trailer_iterator_advance(&iter)) + while (trailer_iterator_advance(&iter)) { -+ if (num_expected_objects) { -+ check_str(iter.raw, trailer_assertions[i].raw); -+ check_str(iter.key.buf, trailer_assertions[i].key); -+ check_str(iter.val.buf, trailer_assertions[i].val); ++ if (num_expected) { ++ check_str(iter.raw, contents[i].raw); ++ check_str(iter.key.buf, contents[i].key); ++ check_str(iter.val.buf, contents[i].val); + } i++; + } trailer_iterator_release(&iter); - check_uint(i, ==, num_expected_objects); -@@ t/unit-tests/t-trailer.c: static void t_trailer_iterator(const char *msg, size_t num_expected_objects) + check_uint(i, ==, num_expected); +@@ t/unit-tests/t-trailer.c: static void t_trailer_iterator(const char *msg, size_t num_expected) static void run_t_trailer_iterator(void) { @@ t/unit-tests/t-trailer.c: static void t_trailer_iterator(const char *msg, size_t static struct test_cases { const char *name; const char *msg; - size_t num_expected_objects; -+ struct trailer_assertions trailer_assertions[10]; + size_t num_expected; ++ struct contents contents[10]; } tc[] = { { "empty input", @@ t/unit-tests/t-trailer.c: static void run_t_trailer_iterator(void) for (int i = 0; i < sizeof(tc) / sizeof(tc[0]); i++) { TEST(t_trailer_iterator(tc[i].msg, -- tc[i].num_expected_objects), -+ tc[i].num_expected_objects, -+ tc[i].trailer_assertions), +- tc[i].num_expected), ++ tc[i].num_expected, ++ tc[i].contents), "%s", tc[i].name); } } -- gitgitgadget
