Subject: [PATCH v4 00/10] Make trailer_info struct private (plus sequencer cleanup)

From: "Linus Arver via GitGitGadget" <[email-masked]>
Date: Thu, 02 May 2024 04:54:17 +0000

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

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-masked]>
     +Author: Linus Arver <[email-masked]>
      
       ## Commit message ##
          Makefile: sort UNIT_TEST_PROGRAMS
      
     -    Signed-off-by: Linus Arver <[email-masked]>
     +    Signed-off-by: Linus Arver <[email-masked]>
      
       ## Makefile ##
      @@ Makefile: THIRD_PARTY_SOURCES += sha1collisiondetection/%
  2:  4ad0fbbb33c !  2:  b503b539c6f trailer: add unit tests for trailer iterator
     @@
       ## Metadata ##
     -Author: Linus Arver <[email-masked]>
     +Author: Linus Arver <[email-masked]>
      
       ## 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-masked]>
     +    Signed-off-by: Linus Arver <[email-masked]>
      
       ## Makefile ##
      @@ Makefile: UNIT_TEST_PROGRAMS += t-ctype
  3:  9077d5a315d !  3:  4aeb48050b1 trailer: teach iterator about non-trailer lines
     @@
       ## Metadata ##
     -Author: Linus Arver <[email-masked]>
     +Author: Linus Arver <[email-masked]>
      
       ## 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-masked]>
     +    Signed-off-by: Linus Arver <[email-masked]>
      
       ## 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-masked]>
     +Author: Linus Arver <[email-masked]>
      
       ## 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-masked]>
     +    Signed-off-by: Linus Arver <[email-masked]>
      
       ## 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-masked]>
     +Author: Linus Arver <[email-masked]>
      
       ## Commit message ##
          interpret-trailers: access trailer_info with new helpers
     @@ Commit message
          implementation (and thus hidden from the API).
      
          Helped-by: Christian Couder <[email-masked]>
     -    Signed-off-by: Linus Arver <[email-masked]>
     +    Signed-off-by: Linus Arver <[email-masked]>
      
       ## 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-masked]>
     +Author: Linus Arver <[email-masked]>
      
       ## 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 <[email-masked]>
     -    Signed-off-by: Linus Arver <[email-masked]>
     +    Signed-off-by: Linus Arver <[email-masked]>
      
       ## 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-masked]>
     +Author: Linus Arver <[email-masked]>
      
       ## Commit message ##
          trailer: make trailer_info struct private
     @@ Commit message
      
          Helped-by: Junio C Hamano <[email-masked]>
          Helped-by: Christian Couder <[email-masked]>
     -    Signed-off-by: Linus Arver <[email-masked]>
     +    Signed-off-by: Linus Arver <[email-masked]>
      
       ## trailer.c ##
      @@
  8:  56e1cca4b7b !  8:  ca6f0c4208c trailer: retire trailer_info_get() from API
     @@
       ## Metadata ##
     -Author: Linus Arver <[email-masked]>
     +Author: Linus Arver <[email-masked]>
      
       ## 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-masked]>
     +    Signed-off-by: Linus Arver <[email-masked]>
      
       ## 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-masked]>
     +Author: Linus Arver <[email-masked]>
      
       ## 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-masked]>
     +    Signed-off-by: Linus Arver <[email-masked]>
      
       ## 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-masked]>
     +Author: Linus Arver <[email-masked]>
      
       ## Commit message ##
          trailer unit tests: inspect iterator contents
     @@ Commit message
          iteration.
      
          Helped-by: Junio C Hamano <[email-masked]>
     -    Signed-off-by: Linus Arver <[email-masked]>
     +    Signed-off-by: Linus Arver <[email-masked]>
      
       ## 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