Make trailer_info struct private (plus sequencer cleanup)

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 ([email protected]) 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 [email protected] email address tomorrow (I'm switching jobs!) and so future emails from me will come from [email protected] [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/[email protected]/
[2]
https://lore.kernel.org/git/[email protected]/
[3]
https://lore.kernel.org/git/CAP8UFD08F0V13X0+CJ1uhMPzPWVMs2okGVMJch0DkQg5M3BWLA@mail.gmail.com/
[4]
https://lore.kernel.org/git/CAP8UFD1twELGKvvesxgCrZrypKZpgSt04ira3mvurG1UbpDfxQ@mail.gmail.com/
[5]
https://lore.kernel.org/git/[email protected]/

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 <[email protected]>
+Author: Linus Arver <[email protected]>

## Commit message ##
Makefile: sort UNIT_TEST_PROGRAMS

- Signed-off-by: Linus Arver <[email protected]>
+ Signed-off-by: Linus Arver <[email protected]>

## Makefile ##
@@ Makefile: THIRD_PARTY_SOURCES += sha1collisiondetection/%
2: 4ad0fbbb33c ! 2: b503b539c6f trailer: add unit tests for trailer iterator
@@
## Metadata ##
-Author: Linus Arver <[email protected]>
+Author: Linus Arver <[email protected]>

## 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 <[email protected]>
+ Signed-off-by: Linus Arver <[email protected]>

## Makefile ##
@@ Makefile: UNIT_TEST_PROGRAMS += t-ctype
3: 9077d5a315d ! 3: 4aeb48050b1 trailer: teach iterator about non-trailer lines

     @@
       ## Metadata ##
     -Author: Linus Arver <[email protected]>
     +Author: Linus Arver <[email protected]>
      
       ## 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 <[email protected]>
     +    Signed-off-by: Linus Arver <[email protected]>
      
       ## 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 <[email protected]>
+Author: Linus Arver <[email protected]>

## 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 <[email protected]>
+ Signed-off-by: Linus Arver <[email protected]>

## 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 <[email protected]>
+Author: Linus Arver <[email protected]>

## Commit message ##
interpret-trailers: access trailer_info with new helpers
@@ Commit message
implementation (and thus hidden from the API).

- Signed-off-by: Linus Arver <[email protected]>
+ Signed-off-by: Linus Arver <[email protected]>

## 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 <[email protected]>
+Author: Linus Arver <[email protected]>

## Commit message ##
trailer: make parse_trailers() return trailer_info pointer
@@ Commit message
format_trailers_from_commit() and trailer_iterator_init() accordingly.

- Signed-off-by: Linus Arver <[email protected]>
+ Signed-off-by: Linus Arver <[email protected]>

## 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 <[email protected]>
+Author: Linus Arver <[email protected]>

## Commit message ##
trailer: make trailer_info struct private
@@ Commit message

- Signed-off-by: Linus Arver <[email protected]>
+ Signed-off-by: Linus Arver <[email protected]>

## trailer.c ##
@@
8: 56e1cca4b7b ! 8: ca6f0c4208c trailer: retire trailer_info_get() from API
@@
## Metadata ##
-Author: Linus Arver <[email protected]>
+Author: Linus Arver <[email protected]>

## 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 <[email protected]>
+ Signed-off-by: Linus Arver <[email protected]>

## 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 <[email protected]>
+Author: Linus Arver <[email protected]>

## 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 <[email protected]>
+ Signed-off-by: Linus Arver <[email protected]>

## 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 <[email protected]>
     +Author: Linus Arver <[email protected]>
      
       ## Commit message ##
          trailer unit tests: inspect iterator contents
     @@ Commit message
          iteration.
      
          Helped-by: Junio C Hamano <[email protected]>
     -    Signed-off-by: Linus Arver <[email protected]>
     +    Signed-off-by: Linus Arver <[email protected]>
      
       ## 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