Enrich Trailer API

This patch series is the first 10 patches of a much larger series I've been working. The main goal of this series is to enrich the API in trailer.h. The larger series brings a number of additional code simplifications and cleanups (exposing and fixing some bugs along the way), and builds on top of this series. The goal of the larger series is to make the trailer interface ready for unit testing. By "trailer API" I mean those functions exposed in trailer.h.

Currently, we have "process_trailers()" in trailer.h which does many different things (parse command-line arguments, create temporary files, etc) that are independent of the concept of "trailers", whose interface trailer.h defines. This makes unit-testing this function difficult. While there is no technical reason why we couldn't write unit tests for the smaller functions that are called within process_trailers(), doing so would involve testing private ("static" in trailer.c) functions instead of the public functions exposed in trailer.h which authoritatively define the API.

As an alternative to this patch series, we could keep trailer.h intact and decide to unit-test the existing "trailer_info_get()" function which does most of the trailer parsing work. However this function wouldn't be easy to test either, because the resulting data structure merely contains the unparsed "trailers" lines. So the unit test (if it wants to inspect the result of parsing these lines) would have to invoke additional parsing functions itself. And at that point it would not longer be a unit test in the traditional sense, because it would be invoking multiple functions at once.

This series breaks up "process_trailers()" into smaller pieces, exposing many of the parts relevant to trailer-related processing in trailer.h. This forces us to start writing unit tests for these now public functions, but that is a good thing because those same unit tests should be easy to write (due to their small(er) sizes), but also, because those unit tests will now ensure some degree of stability across new versions of trailer.h (we will start noticing when the behavior of any of these API functions change).

Another benefit is that more clients (perhaps those outside of Git in the future) will be able to use the same trailer processing algorithms used by the interpret-trailers builtin. For example, a web server may want to parse trailers the same way that interpret-trailers would parse them, without having to shell out to interpret-trailers. Importing the new (richer) trailer.h file that this series promotes would help them achieve that goal. This use case was the original motivation behind my work in this area of Git.

Thanks to the aggressive refactoring in this series, I've been able to identify and fix a couple bugs in our existing implementation. Those fixes build on top of this series but were not included here, in order to keep this series small.

Linus Arver (10):
trailer: move process_trailers() to interpret-trailers.c
trailer: include "trailer" term in API functions
trailer: unify trailer formatting machinery
trailer: delete obsolete formatting functions
sequencer: use the trailer iterator
trailer: make trailer_info struct private
trailer: spread usage of "trailer_block" language
trailer: prepare to move parse_trailers_from_command_line_args() to
builtin
trailer: move arg handling to interpret-trailers.c
trailer: delete obsolete argument handling code from API

 builtin/interpret-trailers.c | 169 ++++++++--
 builtin/shortlog.c           |   7 +-
 pretty.c                     |   2 +-
 ref-filter.c                 |   2 +-
 sequencer.c                  |  35 +--
 trailer.c                    | 579 ++++++++++++++++-------------------
 trailer.h                    | 106 ++++---
 7 files changed, 482 insertions(+), 418 deletions(-)

base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1632%2Flistx%2Ftrailer-api-refactor-part-1-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1632/listx/trailer-api-refactor-part-1-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1632

gitgitgadget

From: Linus Arver <[email protected]>

The interpret-trailers.c builtin is the only place we need to call process_trailers(). As it stands, process_trailers() is inherently tied to how the builtin behaves, so move its definition there.

Delete the corresponding declaration from trailer.h, which then forces us to expose the working innards of that function. This enriches trailer.h to include a more granular API, which can then be unit-tested in the future (because process_trailers() by itself does too many things to be able to be easily unit-tested).

Take this opportunity to demote some file-handling functions out of the trailer API implementation, as these have nothing to do with trailers.

While we're at it, rename process_trailers() to interpret_trailers() in the builtin for consistency with the existing cmd_interpret_trailers(), which wraps around this function.

---
 builtin/interpret-trailers.c |  98 +++++++++++++++++++++++++++-
 trailer.c                    | 120 ++++-------------------------------
 trailer.h                    |  20 +++++-
 3 files changed, 126 insertions(+), 112 deletions(-)

index 033bd1556cf..444f8fb70c9 100644
@@ -9,6 +9,7 @@
 #include "gettext.h"
 #include "parse-options.h"
 #include "string-list.h"
+#include "tempfile.h"
 #include "trailer.h"
 #include "config.h"
 

@@ -91,6 +92,99 @@ static int parse_opt_parse(const struct option *opt, const char *arg,
 	return 0;
 }
 
+static struct tempfile *trailers_tempfile;
+
+static FILE *create_in_place_tempfile(const char *file)
+{
+	struct stat st;
+	struct strbuf filename_template = STRBUF_INIT;
+	const char *tail;
+	FILE *outfile;
+
+	if (stat(file, &st))
+		die_errno(_("could not stat %s"), file);
+	if (!S_ISREG(st.st_mode))
+		die(_("file %s is not a regular file"), file);
+	if (!(st.st_mode & S_IWUSR))
+		die(_("file %s is not writable by user"), file);
+
+	/* Create temporary file in the same directory as the original */
+	tail = strrchr(file, '/');
+	if (tail)
+		strbuf_add(&filename_template, file, tail - file + 1);
+	strbuf_addstr(&filename_template, "git-interpret-trailers-XXXXXX");
+
+	trailers_tempfile = xmks_tempfile_m(filename_template.buf, st.st_mode);
+	strbuf_release(&filename_template);
+	outfile = fdopen_tempfile(trailers_tempfile, "w");
+	if (!outfile)
+		die_errno(_("could not open temporary file"));
+
+	return outfile;
+}
+
+static void read_input_file(struct strbuf *sb, const char *file)
+{
+	if (file) {
+		if (strbuf_read_file(sb, file, 0) < 0)
+			die_errno(_("could not read input file '%s'"), file);
+	} else {
+		if (strbuf_read(sb, fileno(stdin), 0) < 0)
+			die_errno(_("could not read from stdin"));
+	}
+}
+
+static void interpret_trailers(const char *file,
+			       const struct process_trailer_options *opts,
+			       struct list_head *new_trailer_head)
+{
+	LIST_HEAD(head);
+	struct strbuf sb = STRBUF_INIT;
+	struct trailer_info info;
+	FILE *outfile = stdout;
+
+	ensure_configured();
+
+	read_input_file(&sb, file);
+
+	if (opts->in_place)
+		outfile = create_in_place_tempfile(file);
+
+	parse_trailers(&info, sb.buf, &head, opts);
+
+	/* Print the lines before the trailers */
+	if (!opts->only_trailers)
+		fwrite(sb.buf, 1, info.trailer_block_start, outfile);
+
+	if (!opts->only_trailers && !info.blank_line_before_trailer)
+		fprintf(outfile, "\n");
+
+
+	if (!opts->only_input) {
+		LIST_HEAD(config_head);
+		LIST_HEAD(arg_head);
+		parse_trailers_from_config(&config_head);
+		parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
+		list_splice(&config_head, &arg_head);
+		process_trailers_lists(&head, &arg_head);
+	}
+
+	print_all(outfile, &head, opts);
+
+	free_all(&head);
+	trailer_info_release(&info);
+
+	/* Print the lines after the trailers as is */
+	if (!opts->only_trailers)
+		fwrite(sb.buf + info.trailer_block_end, 1, sb.len - info.trailer_block_end, outfile);
+
+	if (opts->in_place)
+		if (rename_tempfile(&trailers_tempfile, file))
+			die_errno(_("could not rename temporary file to %s"), file);
+
+	strbuf_release(&sb);
+}
+
 int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 {
 	struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;

@@ -132,11 +226,11 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 	if (argc) {
 		int i;
 		for (i = 0; i < argc; i++)
-			process_trailers(argv[i], &opts, &trailers);
+			interpret_trailers(argv[i], &opts, &trailers);
 	} else {
 		if (opts.in_place)
 			die(_("no input file given for in-place editing"));
-		process_trailers(NULL, &opts, &trailers);
+		interpret_trailers(NULL, &opts, &trailers);
 	}
 
 	new_trailers_clear(&trailers);

index 1644cd05f60..b3e4a5e127d 100644
@@ -81,15 +81,29 @@ struct process_trailer_options {
 
 #define PROCESS_TRAILER_OPTIONS_INIT {0}
 
-void process_trailers(const char *file,
-		      const struct process_trailer_options *opts,
-		      struct list_head *new_trailer_head);
+void parse_trailers_from_config(struct list_head *config_head);
+
+void parse_trailers_from_command_line_args(struct list_head *arg_head,
+					   struct list_head *new_trailer_head);
+
+void process_trailers_lists(struct list_head *head,
+			    struct list_head *arg_head);
+
+void parse_trailers(struct trailer_info *info,
+		    const char *str,
+		    struct list_head *head,
+		    const struct process_trailer_options *opts);
 
 void trailer_info_get(struct trailer_info *info, const char *str,
 		      const struct process_trailer_options *opts);
 
 void trailer_info_release(struct trailer_info *info);
 
+void ensure_configured(void);
+void print_all(FILE *outfile, struct list_head *head,
+	       const struct process_trailer_options *opts);
+void free_all(struct list_head *head);
+
 /*
  * Format the trailers from the commit msg "msg" into the strbuf "out".
  * Note two caveats about "opts":

gitgitgadget

Yup.

From: Linus Arver <[email protected]>

These functions are exposed to clients and so they should include "trailer" in their names for easier identification, just like all the other functions already exposed by trailer.h.

---
 builtin/interpret-trailers.c |  7 ++++---
 trailer.c                    | 10 +++++-----
 trailer.h                    | 10 +++++-----
 3 files changed, 14 insertions(+), 13 deletions(-)

index 444f8fb70c9..adb74276281 100644
@@ -143,7 +143,7 @@ static void interpret_trailers(const char *file,
 	struct trailer_info info;
 	FILE *outfile = stdout;
 
-	ensure_configured();
+	trailer_config_init();
 
 	read_input_file(&sb, file);
 

@@ -169,9 +169,10 @@ static void interpret_trailers(const char *file,
 		process_trailers_lists(&head, &arg_head);
 	}
 
-	print_all(outfile, &head, opts);
+	/* Print trailer block. */
+	format_trailers(outfile, &head, opts);
 
-	free_all(&head);
+	free_trailers(&head);
 	trailer_info_release(&info);
 
 	/* Print the lines after the trailers as is */

index 9d70c9946bd..0ce7e9079ca 100644
@@ -162,8 +162,8 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
 		fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
 }
 
-void print_all(FILE *outfile, struct list_head *head,
-	       const struct process_trailer_options *opts)
+void format_trailers(FILE *outfile, struct list_head *head,
+		     const struct process_trailer_options *opts)
 {
 	struct list_head *pos;
 	struct trailer_item *item;

@@ -588,7 +588,7 @@ static int git_trailer_config(const char *conf_key, const char *value,
 	return 0;
 }
 
-void ensure_configured(void)
+void trailer_config_init(void)
 {
 	if (configured)
 		return;

@@ -1023,7 +1023,7 @@ void parse_trailers(struct trailer_info *info,
 	}
 }
 
-void free_all(struct list_head *head)
+void free_trailers(struct list_head *head)
 {
 	struct list_head *pos, *p;
 	list_for_each_safe(pos, p, head) {

@@ -1041,7 +1041,7 @@ void trailer_info_get(struct trailer_info *info, const char *str,
 	size_t nr = 0, alloc = 0;
 	char **last = NULL;
 
-	ensure_configured();
+	trailer_config_init();
 
 	end_of_log_message = find_end_of_log_message(str, opts->no_divider);
 	trailer_block_start = find_trailer_block_start(str, end_of_log_message);

index b3e4a5e127d..0e4f0ece9b3 100644
@@ -99,11 +99,11 @@ void trailer_info_get(struct trailer_info *info, const char *str,
 
 void trailer_info_release(struct trailer_info *info);
 
-void ensure_configured(void);
-void print_all(FILE *outfile, struct list_head *head,
-	       const struct process_trailer_options *opts);
-void free_all(struct list_head *head);
+void trailer_config_init(void);
+void free_trailers(struct list_head *trailers);
 
+void format_trailers(FILE *outfile, struct list_head *head,
+		     const struct process_trailer_options *opts);
 /*
  * Format the trailers from the commit msg "msg" into the strbuf "out".
  * Note two caveats about "opts":

@@ -111,7 +111,7 @@ void free_all(struct list_head *head);
  *   - this is primarily a helper for pretty.c, and not
  *     all of the flags are supported.
  *
- *   - this differs from process_trailers slightly in that we always format
+ *   - this differs from format_trailers slightly in that we always format
  *     only the trailer block itself, even if the "only_trailers" option is not
  *     set.
  */

gitgitgadget

Oh, if you were to do this and code movement in two separate patches, doing the rename before the move in the previous step would have made much more sense. If we stopped after 01/10, the tree would have been a very sorry state. If this came first, even if we stop after these renaming of internal functions that are not extern, nobody wil be hurt by these new and improved names.

Ah very true. I think I was thinking of the series as one atomic thing (all 10 patches or nothing), which is a bad habit I need to break. I'll reorder these two on the next reroll, because principles matter (and it's an easy mechanical change). Thanks.

Thanks. I wouldn't have minded if these renames and moves were done in a single step, but because you already have them as separate two steps, let's keep them separate.

From: Linus Arver <[email protected]>

Currently have two functions for formatting trailers exposed in trailer.h:

void format_trailers(FILE *outfile, struct list_head *head,
const struct process_trailer_options *opts);

void format_trailers_from_commit(struct strbuf *out, const char *msg,
const struct process_trailer_options *opts);

and previously these functions, although similar enough (even taking the same process_trailer_options struct pointer), did not build on each other.

Make format_trailers_from_commit() rely on format_trailers(). Teach format_trailers() to process trailers with the additional process_trailer_options fields like opts->key_only which is only used by format_trailers_from_commit() and not builtin/interpret-trailers.c.

This will allow us to delete the format_trailer_info() and print_tok_val() functions in the next patch. They are not deleted here in order to keep the diff small.

---
 builtin/interpret-trailers.c |   5 +-
 pretty.c                     |   2 +-
 ref-filter.c                 |   2 +-
 trailer.c                    | 105 +++++++++++++++++++++++++++++------
 trailer.h                    |  21 +++----
 5 files changed, 102 insertions(+), 33 deletions(-)

index adb74276281..934833a4645 100644
@@ -140,6 +140,7 @@ static void interpret_trailers(const char *file,
 {
 	LIST_HEAD(head);
 	struct strbuf sb = STRBUF_INIT;
+	struct strbuf trailer_block = STRBUF_INIT;
 	struct trailer_info info;
 	FILE *outfile = stdout;
 

@@ -170,7 +171,9 @@ static void interpret_trailers(const char *file,
 	}
 
 	/* Print trailer block. */
-	format_trailers(outfile, &head, opts);
+	format_trailers(&head, opts, &trailer_block);
+	fwrite(trailer_block.buf, 1, trailer_block.len, outfile);
+	strbuf_release(&trailer_block);
 
 	free_trailers(&head);
 	trailer_info_release(&info);

index cf964b060cd..f0721a5214f 100644
@@ -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(msg + c->subject_off, &opts, sb);
 			ret = arg - placeholder + 1;
 		}
 	trailer_out:

index 35b989e1dfe..7fb13818686 100644
@@ -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(subpos, &atom->u.contents.trailer_opts, &s);
 
 			v->s = strbuf_detach(&s, NULL);
 		} else if (atom->u.contents.option == C_BARE)

index 0e4f0ece9b3..50f70556302 100644
@@ -102,21 +102,16 @@ void trailer_info_release(struct trailer_info *info);
 void trailer_config_init(void);
 void free_trailers(struct list_head *trailers);
 
-void format_trailers(FILE *outfile, struct list_head *head,
-		     const struct process_trailer_options *opts);
+void format_trailers(struct list_head *head,
+		     const struct process_trailer_options *opts,
+		     struct strbuf *out);
 /*
- * Format the trailers from the commit msg "msg" into the strbuf "out".
- * Note two caveats about "opts":
- *
- *   - this is primarily a helper for pretty.c, and not
- *     all of the flags are supported.
- *
- *   - this differs from format_trailers slightly in that we always format
- *     only the trailer block itself, even if the "only_trailers" option is not
- *     set.
+ * Convenience function to format the trailers from the commit msg "msg" into
+ * the strbuf "out". Reuses format_trailers internally.
  */
-void format_trailers_from_commit(struct strbuf *out, const char *msg,
-				 const struct process_trailer_options *opts);
+void format_trailers_from_commit(const char *msg,
+				 const struct process_trailer_options *opts,
+				 struct strbuf *out);
 
 /*
  * An interface for iterating over the trailers found in a particular commit

gitgitgadget

Yay. It feels a bit disappointing to see the diffstat and learn that we are not deleting substantial number of lines.

	static add_separator(struct process_trailer_options *opts,
        		     const char *token
			     struct strbuf *out)
	{
		if (opts->key_value_separator)
			strbuf_addbuf(out, opts->key_value_separator);
		else
			strbuf_addstr(out, ": ");
	}

Indeed ;)

Glen Choo told me a while ago [1] that we usually put "out parameters" at the end, and somehow that stuck with me.

Having gotten more familiar with the strbuf.h functions since this patch was originally written, I agree.

I simply (mechanically) moved "sb" from the beginning of the parameters to the end, and didn't think much beyond that adjustment.

Your ordering seems fine to me. Noted for the next reroll, but I will wait until you're done reviewing the other patches before going with the new ordering (in case you change your mind).

This is actually what I've already done (introducing helper functions to make this more readable and reduce nesting), but in the larger series (not in this patch series). I think in this patch I tried to avoid introducing new functions in order to keep the original "shape" of the existing refactored functions, including all of the nesting that they originally had. I wanted to keep the original shape because I thought that would make review simpler.

Thank you for reviewing!

[1] https://lore.kernel.org/git/[email protected]/

From: Linus Arver <[email protected]>

---
 trailer.c | 79 -------------------------------------------------------
 1 file changed, 79 deletions(-)

index 315d90ee1ab..132f22b3dd7 100644
@@ -144,24 +144,6 @@ static char last_non_space_char(const char *s)
 	return '\0';
 }
 
-static void print_tok_val(FILE *outfile, const char *tok, const char *val)
-{
-	char c;
-
-	if (!tok) {
-		fprintf(outfile, "%s\n", val);
-		return;
-	}
-
-	c = last_non_space_char(tok);
-	if (!c)
-		return;
-	if (strchr(separators, c))
-		fprintf(outfile, "%s%s\n", tok, val);
-	else
-		fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
-}
-
 static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
 {
 	struct trailer_item *new_item = xcalloc(1, sizeof(*new_item));

@@ -1142,67 +1124,6 @@ void trailer_info_release(struct trailer_info *info)
 	free(info->trailers);
 }
 
-static void format_trailer_info(struct strbuf *out,
-				const struct trailer_info *info,
-				const char *msg,
-				const struct process_trailer_options *opts)
-{
-	size_t origlen = out->len;
-	size_t i;
-
-	/* If we want the whole block untouched, we can take the fast path. */
-	if (!opts->only_trailers && !opts->unfold && !opts->filter &&
-	    !opts->separator && !opts->key_only && !opts->value_only &&
-	    !opts->key_value_separator) {
-		strbuf_add(out, msg + info->trailer_block_start,
-			   info->trailer_block_end - info->trailer_block_start);
-		return;
-	}
-
-	for (i = 0; i < info->trailer_nr; i++) {
-		char *trailer = info->trailers[i];
-		ssize_t separator_pos = find_separator(trailer, separators);
-
-		if (separator_pos >= 1) {
-			struct strbuf tok = STRBUF_INIT;
-			struct strbuf val = STRBUF_INIT;
-
-			parse_trailer(&tok, &val, NULL, trailer, separator_pos);
-			if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
-				if (opts->unfold)
-					unfold_value(&val);
-
-				if (opts->separator && out->len != origlen)
-					strbuf_addbuf(out, opts->separator);
-				if (!opts->value_only)
-					strbuf_addbuf(out, &tok);
-				if (!opts->key_only && !opts->value_only) {
-					if (opts->key_value_separator)
-						strbuf_addbuf(out, opts->key_value_separator);
-					else
-						strbuf_addstr(out, ": ");
-				}
-				if (!opts->key_only)
-					strbuf_addbuf(out, &val);
-				if (!opts->separator)
-					strbuf_addch(out, '\n');
-			}
-			strbuf_release(&tok);
-			strbuf_release(&val);
-
-		} else if (!opts->only_trailers) {
-			if (opts->separator && out->len != origlen) {
-				strbuf_addbuf(out, opts->separator);
-			}
-			strbuf_addstr(out, trailer);
-			if (opts->separator) {
-				strbuf_rtrim(out);
-			}
-		}
-	}
-
-}
-
 void format_trailers_from_commit(const char *msg,
 				 const struct process_trailer_options *opts,
 				 struct strbuf *out)

gitgitgadget

OK, these unused functions are the reason why 03/10 weren't removing as many lines as I would have expected from the refactoring.

Looking good.

From: Linus Arver <[email protected]>

This patch allows for the removal of "trailer_info_get()" from the trailer.h API, which will be in the next patch.

Instead of calling "trailer_info_get()", which is a low-level function in the trailers implementation (trailer.c), call trailer_iterator_advance(), which was specifically designed for public consumption in f0939a0eb1 (trailer: add interface for iterating over commit trailers, 2020-09-27).

Avoiding "trailer_info_get()" means we don't have to worry about options like "no_divider" (relevant for parsing trailers). We also don't have to check for things like "info.trailer_start == info.trailer_end" to see whether there were any trailers (instead we can just check to see whether the iterator advanced at all).

Also, teach the iterator about non-trailer lines, by adding a new field called "raw" to hold both trailer and non-trailer lines. This is necessary because a "trailer block" is a list of trailer lines of at least 25% trailers (see 146245063e (trailer: allow non-trailers in trailer block, 2016-10-21)), such that it may hold non-trailer lines.

---
 builtin/shortlog.c |  7 +++++--
 sequencer.c        | 35 +++++++++++++++--------------------
 trailer.c          | 20 ++++++++++++--------
 trailer.h          | 13 +++++++++++++
 4 files changed, 45 insertions(+), 30 deletions(-)

index 1307ed2b88a..dc8fd5a5532 100644
@@ -172,7 +172,7 @@ static void insert_records_from_trailers(struct shortlog *log,
 					 const char *oneline)
 {
 	struct trailer_iterator iter;
-	const char *commit_buffer, *body;
+	const char *commit_buffer, *body, *value;
 	struct strbuf ident = STRBUF_INIT;
 
 	if (!log->trailers.nr)

@@ -190,7 +190,10 @@ static void insert_records_from_trailers(struct shortlog *log,
 
 	trailer_iterator_init(&iter, body);
 	while (trailer_iterator_advance(&iter)) {
-		const char *value = iter.val.buf;
+		if (!iter.is_trailer)
+			continue;
+
+		value = iter.val.buf;
 
 		if (!string_list_has_string(&log->trailers, iter.key.buf))
 			continue;

index 3cc88d8a800..d199869cda9 100644
@@ -319,37 +319,32 @@ static const char *get_todo_path(const struct replay_opts *opts)
 static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
 	size_t ignore_footer)
 {
-	struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
-	struct trailer_info info;
-	size_t i;
-	int found_sob = 0, found_sob_last = 0;
-	char saved_char;
-
-	opts.no_divider = 1;
+	struct trailer_iterator iter;
+	size_t i = 0, found_sob = 0;
+	char saved_char = sb->buf[sb->len - ignore_footer];
 
 	if (ignore_footer) {
-		saved_char = sb->buf[sb->len - ignore_footer];
 		sb->buf[sb->len - ignore_footer] = '\0';
 	}
 
-	trailer_info_get(&info, sb->buf, &opts);
+	trailer_iterator_init(&iter, sb->buf);
+	while (trailer_iterator_advance(&iter)) {
+		i++;
+		if (sob &&
+		    iter.is_trailer &&
+		    !strncmp(iter.raw.buf, sob->buf, sob->len)) {
+			found_sob = i;
+		}
+	}
+	trailer_iterator_release(&iter);
 
 	if (ignore_footer)
 		sb->buf[sb->len - ignore_footer] = saved_char;
 
-	if (info.trailer_block_start == info.trailer_block_end)
+	if (!i)
 		return 0;
 
-	for (i = 0; i < info.trailer_nr; i++)
-		if (sob && !strncmp(info.trailers[i], sob->buf, sob->len)) {
-			found_sob = 1;
-			if (i == info.trailer_nr - 1)
-				found_sob_last = 1;
-		}
-
-	trailer_info_release(&info);
-
-	if (found_sob_last)
+	if (found_sob == i)
 		return 3;
 	if (found_sob)
 		return 2;

index 132f22b3dd7..593717fd56c 100644
@@ -1151,6 +1151,7 @@ void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
 	struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
 	strbuf_init(&iter->key, 0);
 	strbuf_init(&iter->val, 0);
+	strbuf_init(&iter->raw, 0);
 	opts.no_divider = 1;
 	trailer_info_get(&iter->internal.info, msg, &opts);
 	iter->internal.cur = 0;

@@ -1158,17 +1159,19 @@ void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
 
 int trailer_iterator_advance(struct trailer_iterator *iter)
 {
-	while (iter->internal.cur < iter->internal.info.trailer_nr) {
-		char *trailer = iter->internal.info.trailers[iter->internal.cur++];
-		int separator_pos = find_separator(trailer, separators);
-
-		if (separator_pos < 1)
-			continue; /* not a real trailer */
-
+	char *line;
+	int separator_pos;
+	if (iter->internal.cur < iter->internal.info.trailer_nr) {
+		line = iter->internal.info.trailers[iter->internal.cur++];
+		separator_pos = find_separator(line, separators);
+		iter->is_trailer = (separator_pos > 0);
+
+		strbuf_reset(&iter->raw);
+		strbuf_addstr(&iter->raw, line);
 		strbuf_reset(&iter->key);
 		strbuf_reset(&iter->val);
 		parse_trailer(&iter->key, &iter->val, NULL,
-			      trailer, separator_pos);
+			      line, separator_pos);
 		unfold_value(&iter->val);
 		return 1;
 	}

@@ -1180,4 +1183,5 @@ void trailer_iterator_release(struct trailer_iterator *iter)
 	trailer_info_release(&iter->internal.info);
 	strbuf_release(&iter->val);
 	strbuf_release(&iter->key);
+	strbuf_release(&iter->raw);
 }

index 50f70556302..d50c9fd79b2 100644
@@ -127,6 +127,19 @@ struct trailer_iterator {
 	struct strbuf key;
 	struct strbuf val;
 
+	/*
+	 * Raw line (e.g., "foo: bar baz") before being parsed as a trailer
+	 * key/val pair. This field can contain non-trailer lines because it's
+	 * valid for a trailer block to contain such lines (i.e., we only
+	 * require 25% of the lines in a trailer block to be trailer lines).
+	 */
+	struct strbuf raw;
+
+	/*
+	 * 1 if the raw line was parsed as a separate key/val pair.
+	 */
+	int is_trailer;
+
 	/* private */
 	struct {
 		struct trailer_info info;

gitgitgadget

Nice.

OK. This would change behaviour, wouldn't it, in the sense that we used to yield a non-trailer line from the old iterator but the new one skips them? Is that something we can demonstrate and protect in tests (presumably once we conclude these refactoring, we would be able to call into this machinery from unit-testing framework)?

Thanks.

I think it's the other way; the old iterator only iterated over trailer lines, skipping over non-trailer lines (see the "not a real trailer" deleted bit for trailer_iterator_advance()). The new one iterates over all lines found in the trailer block, whether they are trailer or non-trailer lines.

The function insert_records_from_trailers() from shortlog.c uses the new iterator, but has to be careful because the new iterator goes over non-trailer lines too. That's why it now does

    if (!iter.is_trailer)
            continue;

to do the skipping itself.

Yup, that is exactly what I want to do once the dust around the trailer API settles down after this series (and also the larger series). :)

From: Linus Arver <[email protected]>

In 13211ae23f (trailer: separate public from internal portion of trailer_iterator, 2023-09-09) we moved trailer_info behind an anonymous struct to discourage use by trailer.h API users. However it still left open the possibility of external use of trailer_info itself. Now that there are no external users of trailer_info, we can make this struct private.

Make this struct private by putting its definition inside trailer.c. This has two benefits:

(1) it makes the surface area of the public facing interface (trailer.h) smaller, and

(2) external API users are unable to peer inside this struct (because it is only ever exposed as an opaque pointer).

This change exposes some deficiencies in the API, mainly with regard to information about the location of the trailer block that was parsed. Expose new API functions to access this information (needed by builtin/interpret-trailers.c).

The idea in this patch to hide implementation details behind an "opaque pointer" is also known as the "pimpl" (pointer to implementation) idiom in C++ and is a common pattern in that language (where, for example, abstract classes only have pointers to concrete classes).

However, the original inspiration to use this idiom does not come from C++, but instead the book "C Interfaces and Implementations: Techniques for Creating Reusable Software" [1]. This book recommends opaque pointers as a good design principle for designing C libraries, using the term "interface" as the functions defined in *.h (header) files and "implementation" as the corresponding *.c file which define the interfaces.

The book says this about opaque pointers:

... clients can manipulate such pointers freely, but they can’t dereference them; that is, they can’t look at the innards of the structure pointed to by them. Only the implementation has that privilege. Opaque pointers hide representation details and help catch errors.

In our case, "struct trailer_info" is now hidden from clients, and the ways in which this opaque pointer can be used is limited to the richness of the trailer.h file. In other words, trailer.h exclusively controls exactly how "trailer_info" pointers are to be used.

[1] Hanson, David R. "C Interfaces and Implementations: Techniques for Creating Reusable Software". Addison Wesley, 1997. p. 22

---
 builtin/interpret-trailers.c |  13 +--
 trailer.c                    | 154 +++++++++++++++++++++++------------
 trailer.h                    |  37 ++-------
 3 files changed, 117 insertions(+), 87 deletions(-)

index 934833a4645..0838a57e157 100644
@@ -141,7 +141,7 @@ static void interpret_trailers(const char *file,
 	LIST_HEAD(head);
 	struct strbuf sb = STRBUF_INIT;
 	struct strbuf trailer_block = STRBUF_INIT;
-	struct trailer_info info;
+	struct trailer_info *info;
 	FILE *outfile = stdout;
 
 	trailer_config_init();

@@ -151,13 +151,13 @@ static void interpret_trailers(const char *file,
 	if (opts->in_place)
 		outfile = create_in_place_tempfile(file);
 
-	parse_trailers(&info, sb.buf, &head, opts);
+	info = parse_trailers(sb.buf, &head, opts);
 
 	/* Print the lines before the trailers */
 	if (!opts->only_trailers)
-		fwrite(sb.buf, 1, info.trailer_block_start, outfile);
+		fwrite(sb.buf, 1, trailer_block_start(info), outfile);
 
-	if (!opts->only_trailers && !info.blank_line_before_trailer)
+	if (!opts->only_trailers && !blank_line_before_trailer_block(info))
 		fprintf(outfile, "\n");
 
 

@@ -176,11 +176,12 @@ static void interpret_trailers(const char *file,
 	strbuf_release(&trailer_block);
 
 	free_trailers(&head);
-	trailer_info_release(&info);
 
 	/* Print the lines after the trailers as is */
 	if (!opts->only_trailers)
-		fwrite(sb.buf + info.trailer_block_end, 1, sb.len - info.trailer_block_end, outfile);
+		fwrite(sb.buf + trailer_block_end(info), 1, sb.len - trailer_block_end(info), outfile);
+
+	trailer_info_release(info);
 
 	if (opts->in_place)
 		if (rename_tempfile(&trailers_tempfile, file))

index d50c9fd79b2..b06da1a7d3a 100644
@@ -4,6 +4,8 @@
 #include "list.h"
 #include "strbuf.h"
 
+struct trailer_info;
+
 enum trailer_where {
 	WHERE_DEFAULT,
 	WHERE_END,

@@ -29,27 +31,6 @@ int trailer_set_where(enum trailer_where *item, const char *value);
 int trailer_set_if_exists(enum trailer_if_exists *item, const char *value);
 int trailer_set_if_missing(enum trailer_if_missing *item, const char *value);
 
-struct trailer_info {
-	/*
-	 * True if there is a blank line before the location pointed to by
-	 * trailer_block_start.
-	 */
-	int blank_line_before_trailer;
-
-	/*
-	 * Offsets to the trailer block start and end positions in the input
-	 * string. If no trailer block is found, these are both set to the
-	 * "true" end of the input (find_end_of_log_message()).
-	 */
-	size_t trailer_block_start, trailer_block_end;
-
-	/*
-	 * Array of trailers found.
-	 */
-	char **trailers;
-	size_t trailer_nr;
-};
-
 /*
  * A list that represents newly-added trailers, such as those provided
  * with the --trailer command line option of git-interpret-trailers.

@@ -89,13 +70,13 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head,
 void process_trailers_lists(struct list_head *head,
 			    struct list_head *arg_head);
 
-void parse_trailers(struct trailer_info *info,
-		    const char *str,
-		    struct list_head *head,
-		    const struct process_trailer_options *opts);
+struct trailer_info *parse_trailers(const char *str,
+				    struct list_head *head,
+				    const struct process_trailer_options *opts);
 
-void trailer_info_get(struct trailer_info *info, const char *str,
-		      const struct process_trailer_options *opts);
+size_t trailer_block_start(struct trailer_info *info);
+size_t trailer_block_end(struct trailer_info *info);
+int blank_line_before_trailer_block(struct trailer_info *info);
 
 void trailer_info_release(struct trailer_info *info);
 

@@ -142,7 +123,7 @@ struct trailer_iterator {
 
 	/* private */
 	struct {
-		struct trailer_info info;
+		struct trailer_info *info;
 		size_t cur;
 	} internal;
 };

gitgitgadget

Surely, and looking good from a cursory glance. I'll defer to Christian to comment on this step as an area expert, though.

Thanks.

Looks like a pretty straightforward change. I think the only point worth discussing is using the "pimpl" idiom. I think it's harder to see the value because the trailer_info struct is fairly simple, but I can definitely see this pattern being useful as we libify more complex parts of Git where the struct internals have more complicated logic involved. The pimpl pattern also seems like it will force us to think harder about providing a useful interface, so I am in favor of using it here as practice for future libification.

Sounds good.

From: Linus Arver <[email protected]>

Deprecate the "trailer_info" struct name and replace it with "trailer_block". The main reason is to help readability, because "trailer_info" on the surface sounds like it's about a single trailer when in reality it is a collection of contiguous lines, at least 25% of which are trailers.

---
 builtin/interpret-trailers.c | 26 +++++-----
 trailer.c                    | 99 ++++++++++++++++++------------------
 trailer.h                    | 18 +++----
 3 files changed, 71 insertions(+), 72 deletions(-)

index 0838a57e157..42d9ca07a56 100644
@@ -140,8 +140,8 @@ static void interpret_trailers(const char *file,
 {
 	LIST_HEAD(head);
 	struct strbuf sb = STRBUF_INIT;
-	struct strbuf trailer_block = STRBUF_INIT;
-	struct trailer_info *info;
+	struct strbuf tb = STRBUF_INIT;
+	struct trailer_block *trailer_block;
 	FILE *outfile = stdout;
 
 	trailer_config_init();

@@ -151,13 +151,13 @@ static void interpret_trailers(const char *file,
 	if (opts->in_place)
 		outfile = create_in_place_tempfile(file);
 
-	info = parse_trailers(sb.buf, &head, opts);
+	trailer_block = parse_trailers(sb.buf, opts, &head);
 
-	/* Print the lines before the trailers */
+	/* Print the lines before the trailer block */
 	if (!opts->only_trailers)
-		fwrite(sb.buf, 1, trailer_block_start(info), outfile);
+		fwrite(sb.buf, 1, trailer_block_start(trailer_block), outfile);
 
-	if (!opts->only_trailers && !blank_line_before_trailer_block(info))
+	if (!opts->only_trailers && !blank_line_before_trailer_block(trailer_block))
 		fprintf(outfile, "\n");
 
 

@@ -171,17 +171,17 @@ static void interpret_trailers(const char *file,
 	}
 
 	/* Print trailer block. */
-	format_trailers(&head, opts, &trailer_block);
-	fwrite(trailer_block.buf, 1, trailer_block.len, outfile);
-	strbuf_release(&trailer_block);
+	format_trailers(&head, opts, &tb);
+	fwrite(tb.buf, 1, tb.len, outfile);
+	strbuf_release(&tb);
 
 	free_trailers(&head);
 
-	/* Print the lines after the trailers as is */
+	/* Print the lines after the trailer block as is */
 	if (!opts->only_trailers)
-		fwrite(sb.buf + trailer_block_end(info), 1, sb.len - trailer_block_end(info), outfile);
-
-	trailer_info_release(info);
+		fwrite(sb.buf + trailer_block_end(trailer_block),
+		       1, sb.len - trailer_block_end(trailer_block), outfile);
+	trailer_block_release(trailer_block);
 
 	if (opts->in_place)
 		if (rename_tempfile(&trailers_tempfile, file))

index b06da1a7d3a..5c8503ade78 100644
@@ -4,7 +4,7 @@
 #include "list.h"
 #include "strbuf.h"
 
-struct trailer_info;
+struct trailer_block;
 
 enum trailer_where {
 	WHERE_DEFAULT,

@@ -70,15 +70,15 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head,
 void process_trailers_lists(struct list_head *head,
 			    struct list_head *arg_head);
 
-struct trailer_info *parse_trailers(const char *str,
-				    struct list_head *head,
-				    const struct process_trailer_options *opts);
+struct trailer_block *parse_trailers(const char *str,
+				     const struct process_trailer_options *opts,
+				     struct list_head *head);
 
-size_t trailer_block_start(struct trailer_info *info);
-size_t trailer_block_end(struct trailer_info *info);
-int blank_line_before_trailer_block(struct trailer_info *info);
+size_t trailer_block_start(struct trailer_block *trailer_block);
+size_t trailer_block_end(struct trailer_block *trailer_block);
+int blank_line_before_trailer_block(struct trailer_block *trailer_block);
 
-void trailer_info_release(struct trailer_info *info);
+void trailer_block_release(struct trailer_block *trailer_block);
 
 void trailer_config_init(void);
 void free_trailers(struct list_head *trailers);

@@ -123,7 +123,7 @@ struct trailer_iterator {
 
 	/* private */
 	struct {
-		struct trailer_info *info;
+		struct trailer_block *trailer_block;
 		size_t cur;
 	} internal;
 };

gitgitgadget

Together with the introduction of trailer_block_{start,end}() accessor functions in the previous step, this sounds like the right thing to do.

"info" is a word with so low information contents. At least "block" would hint that we are talking about a block of text that makes up the trailer attached to a single commit.

There is also "conf_info" and the whole "key"/"key alias" vs "token" lingo which (if we are going to rename things for consistency) needs to be updated to match documentation. In the larger series I've cleaned this up as well.

From: Linus Arver <[email protected]>

Expose more functions in the trailer.h API, in preparation for moving out

parse_trailers_from_command_line_args()

to interpret-trailer.c, because the trailer API should not be concerned with command line arguments (as it has nothing to do with trailers themselves). The interpret-trailers builtin is the only user of the above function.

---
 trailer.c | 66 +++++++++++++++++++++++++++----------------------------
 trailer.h | 10 +++++++++
 2 files changed, 42 insertions(+), 34 deletions(-)

index 360e76376b8..e2d541372a3 100644
@@ -33,7 +33,7 @@ struct trailer_block {
 	size_t trailer_nr;
 };
 
-struct conf_info {
+struct trailer_conf {
 	char *name;
 	char *key;
 	char *command;

@@ -43,7 +43,7 @@ struct conf_info {
 	enum trailer_if_missing if_missing;
 };
 
-static struct conf_info default_conf_info;
+static struct trailer_conf default_trailer_conf;
 
 struct trailer_item {
 	struct list_head list;

@@ -59,7 +59,7 @@ struct arg_item {
 	struct list_head list;
 	char *token;
 	char *value;
-	struct conf_info conf;
+	struct trailer_conf conf;
 };
 
 static LIST_HEAD(conf_head);

@@ -210,7 +210,7 @@ static int check_if_different(struct trailer_item *in_tok,
 	return 1;
 }
 
-static char *apply_command(struct conf_info *conf, const char *arg)
+static char *apply_command(struct trailer_conf *conf, const char *arg)
 {
 	struct strbuf cmd = STRBUF_INIT;
 	struct strbuf buf = STRBUF_INIT;

@@ -424,7 +424,8 @@ int trailer_set_if_missing(enum trailer_if_missing *item, const char *value)
 	return 0;
 }
 
-static void duplicate_conf(struct conf_info *dst, const struct conf_info *src)
+void duplicate_trailer_conf(struct trailer_conf *dst,
+			    const struct trailer_conf *src)
 {
 	*dst = *src;
 	dst->name = xstrdup_or_null(src->name);

@@ -447,7 +448,7 @@ static struct arg_item *get_conf_item(const char *name)
 
 	/* Item does not already exists, create it */
 	CALLOC_ARRAY(item, 1);
-	duplicate_conf(&item->conf, &default_conf_info);
+	duplicate_trailer_conf(&item->conf, &default_trailer_conf);
 	item->conf.name = xstrdup(name);
 
 	list_add_tail(&item->list, &conf_head);

@@ -482,17 +483,17 @@ static int git_trailer_default_config(const char *conf_key, const char *value,
 	variable_name = strrchr(trailer_item, '.');
 	if (!variable_name) {
 		if (!strcmp(trailer_item, "where")) {
-			if (trailer_set_where(&default_conf_info.where,
+			if (trailer_set_where(&default_trailer_conf.where,
 					      value) < 0)
 				warning(_("unknown value '%s' for key '%s'"),
 					value, conf_key);
 		} else if (!strcmp(trailer_item, "ifexists")) {
-			if (trailer_set_if_exists(&default_conf_info.if_exists,
+			if (trailer_set_if_exists(&default_trailer_conf.if_exists,
 						  value) < 0)
 				warning(_("unknown value '%s' for key '%s'"),
 					value, conf_key);
 		} else if (!strcmp(trailer_item, "ifmissing")) {
-			if (trailer_set_if_missing(&default_conf_info.if_missing,
+			if (trailer_set_if_missing(&default_trailer_conf.if_missing,
 						   value) < 0)
 				warning(_("unknown value '%s' for key '%s'"),
 					value, conf_key);

@@ -511,7 +512,7 @@ static int git_trailer_config(const char *conf_key, const char *value,
 {
 	const char *trailer_item, *variable_name;
 	struct arg_item *item;
-	struct conf_info *conf;
+	struct trailer_conf *conf;
 	char *name = NULL;
 	enum trailer_info_type type;
 	int i;

@@ -585,9 +586,9 @@ void trailer_config_init(void)
 		return;
 
 	/* Default config must be setup first */
-	default_conf_info.where = WHERE_END;
-	default_conf_info.if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;
-	default_conf_info.if_missing = MISSING_ADD;
+	default_trailer_conf.where = WHERE_END;
+	default_trailer_conf.if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;
+	default_trailer_conf.if_missing = MISSING_ADD;
 	git_config(git_trailer_default_config, NULL);
 	git_config(git_trailer_config, NULL);
 	configured = 1;

@@ -620,7 +621,7 @@ static int token_matches_item(const char *tok, struct arg_item *item, size_t tok
  * distinguished from the non-well-formed-line case (in which this function
  * returns -1) because some callers of this function need such a distinction.
  */
-static ssize_t find_separator(const char *line, const char *separators)
+ssize_t find_separator(const char *line, const char *separators)
 {
 	int whitespace_found = 0;
 	const char *c;

@@ -645,28 +646,28 @@ static ssize_t find_separator(const char *line, const char *separators)
  *
  * If separator_pos is -1, interpret the whole trailer as a token.
  */
-static void parse_trailer(struct strbuf *tok, struct strbuf *val,
-			 const struct conf_info **conf, const char *trailer,
-			 ssize_t separator_pos)
+void parse_trailer(const char *line, ssize_t separator_pos,
+		   struct strbuf *tok, struct strbuf *val,
+		   const struct trailer_conf **conf)
 {
 	struct arg_item *item;
 	size_t tok_len;
 	struct list_head *pos;
 
 	if (separator_pos != -1) {
-		strbuf_add(tok, trailer, separator_pos);
+		strbuf_add(tok, line, separator_pos);
 		strbuf_trim(tok);
-		strbuf_addstr(val, trailer + separator_pos + 1);
+		strbuf_addstr(val, line + separator_pos + 1);
 		strbuf_trim(val);
 	} else {
-		strbuf_addstr(tok, trailer);
+		strbuf_addstr(tok, line);
 		strbuf_trim(tok);
 	}
 
 	/* Lookup if the token matches something in the config */
 	tok_len = token_len_without_separator(tok->buf, tok->len);
 	if (conf)
-		*conf = &default_conf_info;
+		*conf = &default_trailer_conf;
 	list_for_each(pos, &conf_head) {
 		item = list_entry(pos, struct arg_item, list);
 		if (token_matches_item(tok->buf, item, tok_len)) {

@@ -691,13 +692,13 @@ static struct trailer_item *add_trailer_item(struct list_head *head, char *tok,
 }
 
 static void add_arg_item(struct list_head *arg_head, char *tok, char *val,
-			 const struct conf_info *conf,
+			 const struct trailer_conf *conf,
 			 const struct new_trailer_item *new_trailer_item)
 {
 	struct arg_item *new_item = xcalloc(1, sizeof(*new_item));
 	new_item->token = tok;
 	new_item->value = val;
-	duplicate_conf(&new_item->conf, conf);
+	duplicate_trailer_conf(&new_item->conf, conf);
 	if (new_trailer_item) {
 		if (new_trailer_item->where != WHERE_DEFAULT)
 			new_item->conf.where = new_trailer_item->where;

@@ -730,7 +731,7 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head,
 {
 	struct strbuf tok = STRBUF_INIT;
 	struct strbuf val = STRBUF_INIT;
-	const struct conf_info *conf;
+	const struct trailer_conf *conf;
 	struct list_head *pos;
 
 	/*

@@ -753,8 +754,7 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head,
 			      (int) sb.len, sb.buf);
 			strbuf_release(&sb);
 		} else {
-			parse_trailer(&tok, &val, &conf, tr->text,
-				      separator_pos);
+			parse_trailer(tr->text, separator_pos, &tok, &val, &conf);
 			add_arg_item(arg_head,
 				     strbuf_detach(&tok, NULL),
 				     strbuf_detach(&val, NULL),

@@ -1116,20 +1116,19 @@ struct trailer_block *parse_trailers(const char *str,
 
 	for (i = 0; i < trailer_block->trailer_nr; i++) {
 		int separator_pos;
-		char *trailer = trailer_block->trailers[i];
-		if (trailer[0] == comment_line_char)
+		char *line = trailer_block->trailers[i];
+		if (line[0] == comment_line_char)
 			continue;
-		separator_pos = find_separator(trailer, separators);
+		separator_pos = find_separator(line, separators);
 		if (separator_pos >= 1) {
-			parse_trailer(&tok, &val, NULL, trailer,
-				      separator_pos);
+			parse_trailer(line, separator_pos, &tok, &val, NULL);
 			if (opts->unfold)
 				unfold_value(&val);
 			add_trailer_item(head,
 					 strbuf_detach(&tok, NULL),
 					 strbuf_detach(&val, NULL));
 		} else if (!opts->only_trailers) {
-			strbuf_addstr(&val, trailer);
+			strbuf_addstr(&val, line);
 			strbuf_strip_suffix(&val, "\n");
 			add_trailer_item(head,
 					 NULL,

@@ -1217,8 +1216,7 @@ int trailer_iterator_advance(struct trailer_iterator *iter)
 		strbuf_addstr(&iter->raw, line);
 		strbuf_reset(&iter->key);
 		strbuf_reset(&iter->val);
-		parse_trailer(&iter->key, &iter->val, NULL,
-			      line, separator_pos);
+		parse_trailer(line, separator_pos, &iter->key, &iter->val, NULL);
 		unfold_value(&iter->val);
 		return 1;
 	}

index 5c8503ade78..fe49a9bad52 100644
@@ -5,6 +5,7 @@
 #include "strbuf.h"
 
 struct trailer_block;
+struct trailer_conf;
 
 enum trailer_where {
 	WHERE_DEFAULT,

@@ -45,6 +46,9 @@ struct new_trailer_item {
 	enum trailer_if_missing if_missing;
 };
 
+void duplicate_trailer_conf(struct trailer_conf *dst,
+			    const struct trailer_conf *src);
+
 struct process_trailer_options {
 	int in_place;
 	int trim_empty;

@@ -70,6 +74,12 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head,
 void process_trailers_lists(struct list_head *head,
 			    struct list_head *arg_head);
 
+ssize_t find_separator(const char *line, const char *separators);
+
+void parse_trailer(const char *line, ssize_t separator_pos,
+		   struct strbuf *tok, struct strbuf *val,
+		   const struct trailer_conf **conf);
+
 struct trailer_block *parse_trailers(const char *str,
 				     const struct process_trailer_options *opts,
 				     struct list_head *head);

gitgitgadget

Will do.

From: Linus Arver <[email protected]>

We don't move the "arg_item" struct to interpret-trailers.c, because it is now a struct that contains information about trailers that should be injected into the input text's own trailers. We will rename this language as such in a follow-up patch to keep the diff here small.

---
 builtin/interpret-trailers.c | 88 ++++++++++++++++++++++--------------
 trailer.c                    | 63 +++++++++++++++++++-------
 trailer.h                    | 12 +++++
 3 files changed, 113 insertions(+), 50 deletions(-)

index 42d9ca07a56..4da4eac3b46 100644
@@ -45,23 +45,17 @@ static int option_parse_if_missing(const struct option *opt,
 	return trailer_set_if_missing(opt->value, arg);
 }
 
-static void new_trailers_clear(struct list_head *trailers)
-{
-	struct list_head *pos, *tmp;
-	struct new_trailer_item *item;
-
-	list_for_each_safe(pos, tmp, trailers) {
-		item = list_entry(pos, struct new_trailer_item, list);
-		list_del(pos);
-		free(item);
-	}
-}
+static char *cl_separators;
 
 static int option_parse_trailer(const struct option *opt,
 				   const char *arg, int unset)
 {
 	struct list_head *trailers = opt->value;
-	struct new_trailer_item *item;
+	struct strbuf tok = STRBUF_INIT;
+	struct strbuf val = STRBUF_INIT;
+	const struct trailer_conf *conf;
+	struct trailer_conf *conf_current = new_trailer_conf();
+	ssize_t separator_pos;
 
 	if (unset) {
 		new_trailers_clear(trailers);

@@ -71,12 +65,31 @@ static int option_parse_trailer(const struct option *opt,
 	if (!arg)
 		return -1;
 
-	item = xmalloc(sizeof(*item));
-	item->text = arg;
-	item->where = where;
-	item->if_exists = if_exists;
-	item->if_missing = if_missing;
-	list_add_tail(&item->list, trailers);
+	separator_pos = find_separator(arg, cl_separators);
+	if (separator_pos) {
+		parse_trailer(arg, separator_pos, &tok, &val, &conf);
+		duplicate_trailer_conf(conf_current, conf);
+
+		/*
+		 * Override conf_current with settings specified via CLI flags.
+		 */
+		trailer_conf_set(where, if_exists, if_missing, conf_current);
+
+		add_arg_item(strbuf_detach(&tok, NULL),
+			     strbuf_detach(&val, NULL),
+			     conf_current,
+			     trailers);
+	} else {
+		struct strbuf sb = STRBUF_INIT;
+		strbuf_addstr(&sb, arg);
+		strbuf_trim(&sb);
+		error(_("empty trailer token in trailer '%.*s'"),
+			(int) sb.len, sb.buf);
+		strbuf_release(&sb);
+	}
+
+	free(conf_current);
+
 	return 0;
 }
 

@@ -136,7 +149,7 @@ static void read_input_file(struct strbuf *sb, const char *file)
 
 static void interpret_trailers(const char *file,
 			       const struct process_trailer_options *opts,
-			       struct list_head *new_trailer_head)
+			       struct list_head *arg_trailers)
 {
 	LIST_HEAD(head);
 	struct strbuf sb = STRBUF_INIT;

@@ -144,8 +157,6 @@ static void interpret_trailers(const char *file,
 	struct trailer_block *trailer_block;
 	FILE *outfile = stdout;
 
-	trailer_config_init();
-
 	read_input_file(&sb, file);
 
 	if (opts->in_place)

@@ -162,12 +173,7 @@ static void interpret_trailers(const char *file,
 
 
 	if (!opts->only_input) {
-		LIST_HEAD(config_head);
-		LIST_HEAD(arg_head);
-		parse_trailers_from_config(&config_head);
-		parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
-		list_splice(&config_head, &arg_head);
-		process_trailers_lists(&head, &arg_head);
+		process_trailers_lists(&head, arg_trailers);
 	}
 
 	/* Print trailer block. */

@@ -193,7 +199,8 @@ static void interpret_trailers(const char *file,
 int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 {
 	struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
-	LIST_HEAD(trailers);
+	LIST_HEAD(configured_trailers);
+	LIST_HEAD(arg_trailers);
 
 	struct option options[] = {
 		OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),

@@ -212,33 +219,48 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK_F(0, "parse", &opts, NULL, N_("alias for --only-trailers --only-input --unfold"),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_parse),
 		OPT_BOOL(0, "no-divider", &opts.no_divider, N_("do not treat \"---\" as the end of input")),
-		OPT_CALLBACK(0, "trailer", &trailers, N_("trailer"),
+		OPT_CALLBACK(0, "trailer", &arg_trailers, N_("trailer"),
 				N_("trailer(s) to add"), option_parse_trailer),
 		OPT_END()
 	};
 
 	git_config(git_default_config, NULL);
+	trailer_config_init();
+
+	if (!opts.only_input) {
+		parse_trailers_from_config(&configured_trailers);
+	}
+
+	/*
+	* In command-line arguments, '=' is accepted (in addition to the
+	* separators that are defined).
+	*/
+	cl_separators = xstrfmt("=%s", default_separators());
 
 	argc = parse_options(argc, argv, prefix, options,
 			     git_interpret_trailers_usage, 0);
 
-	if (opts.only_input && !list_empty(&trailers))
+	free(cl_separators);
+
+	if (opts.only_input && !list_empty(&arg_trailers))
 		usage_msg_opt(
 			_("--trailer with --only-input does not make sense"),
 			git_interpret_trailers_usage,
 			options);
 
+	list_splice(&configured_trailers, &arg_trailers);
+
 	if (argc) {
 		int i;
 		for (i = 0; i < argc; i++)
-			interpret_trailers(argv[i], &opts, &trailers);
+			interpret_trailers(argv[i], &opts, &arg_trailers);
 	} else {
 		if (opts.in_place)
 			die(_("no input file given for in-place editing"));
-		interpret_trailers(NULL, &opts, &trailers);
+		interpret_trailers(NULL, &opts, &arg_trailers);
 	}
 
-	new_trailers_clear(&trailers);
+	new_trailers_clear(&arg_trailers);
 
 	return 0;
 }

index fe49a9bad52..9b86acfe2d4 100644
@@ -46,9 +46,20 @@ struct new_trailer_item {
 	enum trailer_if_missing if_missing;
 };
 
+void trailer_conf_set(enum trailer_where where,
+		      enum trailer_if_exists if_exists,
+		      enum trailer_if_missing if_missing,
+		      struct trailer_conf *conf);
+
+struct trailer_conf *new_trailer_conf(void);
 void duplicate_trailer_conf(struct trailer_conf *dst,
 			    const struct trailer_conf *src);
 
+const char *default_separators(void);
+
+void add_arg_item(char *tok, char *val, const struct trailer_conf *conf,
+		  struct list_head *arg_head);
+
 struct process_trailer_options {
 	int in_place;
 	int trim_empty;

@@ -92,6 +103,7 @@ void trailer_block_release(struct trailer_block *trailer_block);
 
 void trailer_config_init(void);
 void free_trailers(struct list_head *trailers);
+void new_trailers_clear(struct list_head *trailers);
 
 void format_trailers(struct list_head *head,
 		     const struct process_trailer_options *opts,

gitgitgadget

Yes, and I've implemented a "trailer_subsystem_conf" struct to hold all configuration-related bits (and hardcoded defaults) relating to trailers in one place, in the larger series.

Makes sense. Thanks.

From: Linus Arver <[email protected]>

This commit was not squashed with its parent in order to keep the diff separate (to make the additive changes in the parent easier to read).

---
 trailer.c | 39 ---------------------------------------
 trailer.h | 17 -----------------
 2 files changed, 56 deletions(-)

index 0a86e0d5afa..27bb2195f53 100644
@@ -745,45 +745,6 @@ void parse_trailers_from_config(struct list_head *config_head)
 	}
 }
 
-void parse_trailers_from_command_line_args(struct list_head *arg_head,
-					   struct list_head *new_trailer_head)
-{
-	struct strbuf tok = STRBUF_INIT;
-	struct strbuf val = STRBUF_INIT;
-	const struct trailer_conf *conf;
-	struct list_head *pos;
-
-	/*
-	 * In command-line arguments, '=' is accepted (in addition to the
-	 * separators that are defined).
-	 */
-	char *cl_separators = xstrfmt("=%s", separators);
-
-	/* Add an arg item for each trailer on the command line */
-	list_for_each(pos, new_trailer_head) {
-		struct new_trailer_item *tr =
-			list_entry(pos, struct new_trailer_item, list);
-		ssize_t separator_pos = find_separator(tr->text, cl_separators);
-
-		if (separator_pos == 0) {
-			struct strbuf sb = STRBUF_INIT;
-			strbuf_addstr(&sb, tr->text);
-			strbuf_trim(&sb);
-			error(_("empty trailer token in trailer '%.*s'"),
-			      (int) sb.len, sb.buf);
-			strbuf_release(&sb);
-		} else {
-			parse_trailer(tr->text, separator_pos, &tok, &val, &conf);
-			add_arg_item(strbuf_detach(&tok, NULL),
-				     strbuf_detach(&val, NULL),
-				     conf,
-				     arg_head);
-		}
-	}
-
-	free(cl_separators);
-}
-
 static const char *next_line(const char *str)
 {
 	const char *nl = strchrnul(str, '\n');

index 9b86acfe2d4..8a89e95c171 100644
@@ -32,20 +32,6 @@ int trailer_set_where(enum trailer_where *item, const char *value);
 int trailer_set_if_exists(enum trailer_if_exists *item, const char *value);
 int trailer_set_if_missing(enum trailer_if_missing *item, const char *value);
 
-/*
- * A list that represents newly-added trailers, such as those provided
- * with the --trailer command line option of git-interpret-trailers.
- */
-struct new_trailer_item {
-	struct list_head list;
-
-	const char *text;
-
-	enum trailer_where where;
-	enum trailer_if_exists if_exists;
-	enum trailer_if_missing if_missing;
-};
-
 void trailer_conf_set(enum trailer_where where,
 		      enum trailer_if_exists if_exists,
 		      enum trailer_if_missing if_missing,

@@ -79,9 +65,6 @@ struct process_trailer_options {
 
 void parse_trailers_from_config(struct list_head *config_head);
 
-void parse_trailers_from_command_line_args(struct list_head *arg_head,
-					   struct list_head *new_trailer_head);
-
 void process_trailers_lists(struct list_head *head,
 			    struct list_head *arg_head);
 

gitgitgadget

Are there places in the current code that deals with trailers but does not use the trailer API (e.g., manually parse and/or insert the trailer in an in-core buffer)? Is it part of the larger goal to update these places so that we will always use the trailer API to touch trailers, and if so, have these places been identified?

Obviously the reason why I ask is that testing cannot be the goal by itself. The "alternative" ...

... may allow you to "test", but it would make it more difficult in the future to revamp the trailer API, if it is needed, in order to cover code paths that ought to be using but currently bypassing the trailer API.

And helper functions, each of which does one small thing well, may be more applicable to other code paths that are currently bypassing the API.

It would be nicer to have a concise list of these fixes (in the form of "git shortlog") as a teaser here ;-). That would hopefully entice others into reviewing this part that forms the foundation.

Thanks.

While working on this series I unfortunately did not search for such use cases (I limited the scope of my work to mainly the interpret-trailers builtin). But, a quick search just now on master branch turned up append_signoff() in sequencer.c which constructs a Signed-off-by trailer manually with a raw strbuf [1].

This is somewhat understandable though, because AFAICS the current trailer API does not support creating and formatting single trailer objects.

That sounds like The Right Thing to do.

Not yet, but, it should be easy to check any remaining cases other than the one I identified up above.

I now seem to recall an offline discussion where I said I wanted to enrich the API to make other parts of Git also use this new API. Somehow I've left that motivation out of the cover letter (will include in the next reroll).

Agreed. I should include this rationale as well in the next cover letter, thanks.

Yep.

Ah, good idea. Here's a shortlog (with a short summary of each one) of bug fixes that are forthcoming:

trailer: trailer replacement should not change its position
(Summary: If we found a trailer we'd like to replace, preserve its position relative to the other trailers found in the trailer block, instead of always moving it to the beginning or end of the entire trailer block.)

interpret-trailers: preserve trailers coming from the input
(Summary: Sometimes, the parsed trailers from the input will be formatted differently depending on whether we provide --only-trailers or not. Make the trailers that were not modified and are coming directly from the input get formatted the same way, regardless of this flag.)

interpret-trailers: fail if given unrecognized arguments
(Summary: E.g., for "--where", only accept recognized WHERE_* enum values. If we get something unrecognized, fail with an error instead of silently doing nothing. Ditto for "--if-exists" and "--if-missing".)

The last one is a different class of bug than the first two, and perhaps less interesting.

Thanks.

[1] https://github.com/git/git/blob/d4dbce1db5cd227a57074bcfc7ec9f0655961bba/sequencer.c#L5299-L5301

Actually, upon closer inspection I realize that we already fail if given unrecognized arguments to --where, --if-exists, and --if-missing. But we don't explain why to the user because no error message is printed. This commit has been retitled to "interpret-trailers: print error if given unrecognized arguments".

Thanks.

I agree with Junio's points, and I added a small nitpick about patch 8's commit message. But apart from that, this all looks good to me and I'm interested in seeing the followup series as well. Thanks!

This patch series is the first 10 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.

The reason why the larger series itself doesn't tackle these goals directly is because I believe that API code should be thought from the ground up with a libification-focused perspective. Some structs and functions exposed in the API today should probably not be libified (read: kept in trailer.h) as is. For example, the "trailer_iterator" struct has a "private" member and it feels wrong to allow API users to peek inside here (and take at face value our future API users' pinky promise that they won't depend on those private internals not meant for public consumption).

One pattern we could use here to cleanly separate "what is the API" (publicly exposed) and "what is the implementation" (private) is the pointer-to-implementation ("pimpl") idiom. There may be other appropriate patterns, but I've chosen this one because it's a simple, low-level concept (put structs in foo.c instead of foo.h), which has far-reaching high-level consequences (API users must rely exclusively on the API to make use of such private structs, via opaque pointers). The pimpl idiom for C comes from the book "C Interfaces and Implementations" (see patch "trailer: make trailer_info struct private").

The idea of turning a public struct into a private one is a fundamental question of libification because it forces us to reconsider all of the data structures we have and how they're actually used by already existing users. For the trailer API, those existing users are the "interpret-trailers" builtin command, and anything else that includes the "trailer.h" header file (e.g., sequencer.c). One advantage of this idiom is that even the compiler understands it --- the compiler will loudly complain if you try to access the innards of a private struct through an opaque pointer.

Another advantage of this idiom is that it helps to reduce the probability of breaking changes in the API. Because a private struct's members are out of view from our users (they only know about opaque pointers to the private struct, not its members), we are free to modify the members of the struct at any time, as much as we like, as long as we don't break the semantics of the exposed API functions (which is why unit-testing these API functions will be crucial long-term).

If this pimpl idiom turns out to be a mistake, undoing it is easy --- just move the relevant struct definition from foo.c to the header file. So it's a great way to try things out without digging ourselves into a pit of despair that will be difficult to get out of.

With the libification-focused goals out of the way, let's turn to this patch series in more detail.

Currently, we have "process_trailers()" in trailer.h which does many different things (parse command-line arguments, create temporary files, etc) that are independent of the concept of "trailers". Keeping this function as an API function would make unit-testing it difficult. While there is no technical reason why we couldn't write unit tests for the smaller functions that are called within process_trailers(), doing so would involve testing private ("static" in trailer.c) functions instead of API functions, which defeats the goal of "API stability" mentioned earlier above.

As an alternative to how things are done in this patch series, we could keep trailer.h intact and decide to unit-test the existing "trailer_info_get()" function which does most of the trailer parsing work (and is used by sequencer.c). However this function wouldn't be easy to test either, because the resulting "trailer_info" struct merely contains the unparsed "trailers" lines. So the unit test (if it wants to inspect the result of parsing these lines) would have to invoke additional parsing functions itself. And at that point it would no longer be a unit test in the traditional sense, because it would be invoking multiple functions at once.

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.

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.

Thanks to the aggressive refactoring in this series, I've been able to identify and fix several bugs in our existing implementation. Those fixes build on top of this series but were not included here, in order to keep this series small. Below is a "shortlog" of those fixes I have locally:

* "trailer: trailer replacement should not change its position" (If we found a trailer we'd like to replace, preserve its position relative to the other trailers found in the trailer block, instead of always moving it to the beginning or end of the entire trailer block.)
* "interpret-trailers: preserve trailers coming from the input" (Sometimes, the parsed trailers from the input will be formatted differently depending on whether we provide --only-trailers or not. Make the trailers that were not modified and which are coming directly from the input get formatted the same way, regardless of this flag.)
* "interpret-trailers: do not modify the input if NOP" (Refrain from subtracting or adding a newline around the patch divider "---" if we are not adding new trailers.)
* "trailer formatter: split up format_trailer() monolith" (Fix a bug in git-log where we still printed a blank newline even if we didn't want to format anything.)
* "interpret-trailers: fail if given unrecognized arguments" (E.g., for "--where", only accept recognized WHERE_* enum values. If we get something unrecognized, fail with an error instead of silently doing nothing. Ditto for "--if-exists" and "--if-missing".)

Notable changes in v2
=====================

* 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 (10):
trailer: prepare to expose functions as part of API
trailer: move interpret_trailers() to interpret-trailers.c
trailer: unify trailer formatting machinery
trailer: delete obsolete formatting functions
sequencer: use the trailer iterator
trailer: make trailer_info struct private
trailer: spread usage of "trailer_block" language
trailer: prepare to move parse_trailers_from_command_line_args() to
builtin
trailer: move arg handling to interpret-trailers.c
trailer: delete obsolete argument handling code from API

 builtin/interpret-trailers.c | 169 ++++++++--
 builtin/shortlog.c           |   7 +-
 pretty.c                     |   2 +-
 ref-filter.c                 |   2 +-
 sequencer.c                  |  35 +--
 trailer.c                    | 576 ++++++++++++++++-------------------
 trailer.h                    | 105 ++++---
 7 files changed, 478 insertions(+), 418 deletions(-)

base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1632%2Flistx%2Ftrailer-api-refactor-part-1-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1632/listx/trailer-api-refactor-part-1-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1632

Range-diff vs v1:

2: 5f64718abfc ! 1: e2d3ed9b5b6 trailer: include "trailer" term in API functions
@@ Metadata
Author: Linus Arver <[email protected]>

## Commit message ##
- trailer: include "trailer" term in API functions
+ trailer: prepare to expose functions as part of API

- These functions are exposed to clients and so they should include
- "trailer" in their names for easier identification, just like all the
- other functions already exposed by trailer.h.
+ 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
+ trailer.h.

+ Rename relevant functions so that they include the term "trailer" in
+ their name, so that clients of the API will be able to easily identify
+ them by their "trailer" moniker, just like all the other functions
+ already exposed by trailer.h.
+
+ The the opportunity to start putting trailer processions options (opts)
+ as the first parameter. This will be the pattern going forward in this
+ series.
+
+ Helped-by: Junio C Hamano <[email protected]>

## builtin/interpret-trailers.c ##
-@@ builtin/interpret-trailers.c: static void interpret_trailers(const char *file,
- struct trailer_info info;
- FILE *outfile = stdout;
-
-- ensure_configured();
-+ trailer_config_init();
-
- read_input_file(&sb, file);
-
-@@ builtin/interpret-trailers.c: static void interpret_trailers(const char *file,
- process_trailers_lists(&head, &arg_head);
+@@ builtin/interpret-trailers.c: int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
+ if (argc) {
+ int i;
+ for (i = 0; i < argc; i++)
+- process_trailers(argv[i], &opts, &trailers);
++ interpret_trailers(&opts, &trailers, argv[i]);
+ } else {
+ if (opts.in_place)
+ die(_("no input file given for in-place editing"));
+- process_trailers(NULL, &opts, &trailers);
++ interpret_trailers(&opts, &trailers, NULL);
}

-- print_all(outfile, &head, opts);
-+ /* Print trailer block. */
-+ format_trailers(outfile, &head, opts);
-
-- free_all(&head);
-+ free_trailers(&head);
- trailer_info_release(&info);
-
- /* Print the lines after the trailers as is */
+ new_trailers_clear(&trailers);

## trailer.c ##
@@ trailer.c: static void print_tok_val(FILE *outfile, const char *tok, const char *val)
fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
}

--void print_all(FILE *outfile, struct list_head *head,
-- const struct process_trailer_options *opts)
-+void format_trailers(FILE *outfile, struct list_head *head,
-+ const struct process_trailer_options *opts)
+-static void print_all(FILE *outfile, struct list_head *head,
+- const struct process_trailer_options *opts)
++static void format_trailers(const struct process_trailer_options *opts,
++ struct list_head *trailers, FILE *outfile)
{
struct list_head *pos;
struct trailer_item *item;
+- list_for_each(pos, head) {
++ list_for_each(pos, trailers) {
+ item = list_entry(pos, struct trailer_item, list);
+ if ((!opts->trim_empty || strlen(item->value) > 0) &&
+ (!opts->only_trailers || item->token))
@@ trailer.c: static int git_trailer_config(const char *conf_key, const char *value,
return 0;
}

--void ensure_configured(void)
-+void trailer_config_init(void)
+-static void ensure_configured(void)
++static void trailer_config_init(void)
{
if (configured)
return;
-@@ trailer.c: void parse_trailers(struct trailer_info *info, +@@ trailer.c: static void parse_trailers(struct trailer_info *info,
}
}

--void free_all(struct list_head *head)
-+void free_trailers(struct list_head *head)
+-static void free_all(struct list_head *head)
++static void free_trailers(struct list_head *trailers)
{
struct list_head *pos, *p;
- list_for_each_safe(pos, p, head) {
+- list_for_each_safe(pos, p, head) {
++ list_for_each_safe(pos, p, trailers) {
+ list_del(pos);
+ free_trailer_item(list_entry(pos, struct trailer_item, list));
+ }
+@@ trailer.c: static FILE *create_in_place_tempfile(const char *file)
+ return outfile;
+ }
+
+-void process_trailers(const char *file,
+- const struct process_trailer_options *opts,
+- struct list_head *new_trailer_head)
++void interpret_trailers(const struct process_trailer_options *opts,
++ struct list_head *new_trailer_head,
++ const char *file)
+ {
+ LIST_HEAD(head);
+ struct strbuf sb = STRBUF_INIT;
+ struct trailer_info info;
+ FILE *outfile = stdout;
+
+- ensure_configured();
++ trailer_config_init();
+
+ read_input_file(&sb, file);
+
+@@ trailer.c: void process_trailers(const char *file,
+ process_trailers_lists(&head, &arg_head);
+ }
+
+- print_all(outfile, &head, opts);
++ format_trailers(opts, &head, outfile);
+
+- free_all(&head);
++ free_trailers(&head);
+ trailer_info_release(&info);
+
+ /* Print the lines after the trailers as is */
@@ trailer.c: void trailer_info_get(struct trailer_info *info, const char *str,
size_t nr = 0, alloc = 0;
char **last = NULL;
@@ trailer.c: void trailer_info_get(struct trailer_info *info, const char *str,
trailer_block_start = find_trailer_block_start(str, end_of_log_message);

## trailer.h ##
-@@ trailer.h: void trailer_info_get(struct trailer_info *info, const char *str, -
- void trailer_info_release(struct trailer_info *info);
-
--void ensure_configured(void);
--void print_all(FILE *outfile, struct list_head *head,
-- const struct process_trailer_options *opts);
--void free_all(struct list_head *head);
-+void trailer_config_init(void);
-+void free_trailers(struct list_head *trailers);
-
-+void format_trailers(FILE *outfile, struct list_head *head,
-+ const struct process_trailer_options *opts);
- /*
- * Format the trailers from the commit msg "msg" into the strbuf "out".
- * Note two caveats about "opts":
-@@ trailer.h: void free_all(struct list_head *head);
- * - this is primarily a helper for pretty.c, and not
- * all of the flags are supported.
- *
-- * - this differs from process_trailers slightly in that we always format
-+ * - this differs from format_trailers slightly in that we always format
- * only the trailer block itself, even if the "only_trailers" option is not
- * set.
- */
+@@ trailer.h: struct process_trailer_options {
+
+ #define PROCESS_TRAILER_OPTIONS_INIT {0}
+
+-void process_trailers(const char *file,
+- const struct process_trailer_options *opts,
+- struct list_head *new_trailer_head);
++void interpret_trailers(const struct process_trailer_options *opts,
++ struct list_head *new_trailer_head,
++ const char *file);
+
+ void trailer_info_get(struct trailer_info *info, const char *str,
+ const struct process_trailer_options *opts);
1: 2dc3279b37f ! 2: eaca39fd7ea trailer: move process_trailers() to interpret-trailers.c

     @@ Metadata
      Author: Linus Arver <[email protected]>
      
       ## Commit message ##
     -    trailer: move process_trailers() to interpret-trailers.c
     +    trailer: move interpret_trailers() to interpret-trailers.c
      
          The interpret-trailers.c builtin is the only place we need to call
     -    process_trailers(). As it stands, process_trailers() is inherently tied
     -    to how the builtin behaves, so move its definition there.
     +    interpret_trailers(), so move its definition there.
      
          Delete the corresponding declaration from trailer.h, which then forces
          us to expose the working innards of that function. This enriches
     -    trailer.h to include a more granular API, which can then be unit-tested
     -    in the future (because process_trailers() by itself does too many things
     -    to be able to be easily unit-tested).
     +    trailer.h with a more granular API, which can then be unit-tested in the
     +    future (because interpret_trailers() by itself does too many things to
     +    be able to be easily unit-tested).
      
          Take this opportunity to demote some file-handling functions out of the
          trailer API implementation, as these have nothing to do with trailers.
      
     -    While we're at it, rename process_trailers() to interpret_trailers() in
     -    the builtin for consistency with the existing cmd_interpret_trailers(),
     -    which wraps around this function.
     -
          Signed-off-by: Linus Arver <[email protected]>
      
       ## builtin/interpret-trailers.c ##
     @@ builtin/interpret-trailers.c: static int parse_opt_parse(const struct option *op
      +	}
      +}
      +
     -+static void interpret_trailers(const char *file,
     -+			       const struct process_trailer_options *opts,
     -+			       struct list_head *new_trailer_head)
     ++static void interpret_trailers(const struct process_trailer_options *opts,
     ++			       struct list_head *new_trailer_head,
     ++			       const char *file)
      +{
      +	LIST_HEAD(head);
      +	struct strbuf sb = STRBUF_INIT;
      +	struct trailer_info info;
      +	FILE *outfile = stdout;
      +
     -+	ensure_configured();
     ++	trailer_config_init();
      +
      +	read_input_file(&sb, file);
      +
      +	if (opts->in_place)
      +		outfile = create_in_place_tempfile(file);
      +
     -+	parse_trailers(&info, sb.buf, &head, opts);
     ++	parse_trailers(opts, &info, sb.buf, &head);
      +
      +	/* Print the lines before the trailers */
      +	if (!opts->only_trailers)
     @@ builtin/interpret-trailers.c: static int parse_opt_parse(const struct option *op
      +		process_trailers_lists(&head, &arg_head);
      +	}
      +
     -+	print_all(outfile, &head, opts);
     ++	format_trailers(opts, &head, outfile);
      +
     -+	free_all(&head);
     ++	free_trailers(&head);
      +	trailer_info_release(&info);
      +
      +	/* Print the lines after the trailers as is */
     @@ builtin/interpret-trailers.c: static int parse_opt_parse(const struct option *op
       int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
       {
       	struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
     -@@ builtin/interpret-trailers.c: int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
     - 	if (argc) {
     - 		int i;
     - 		for (i = 0; i < argc; i++)
     --			process_trailers(argv[i], &opts, &trailers);
     -+			interpret_trailers(argv[i], &opts, &trailers);
     - 	} else {
     - 		if (opts.in_place)
     - 			die(_("no input file given for in-place editing"));
     --		process_trailers(NULL, &opts, &trailers);
     -+		interpret_trailers(NULL, &opts, &trailers);
     - 	}
     - 
     - 	new_trailers_clear(&trailers);
      
       ## trailer.c ##
      @@
     @@ trailer.c: static void print_tok_val(FILE *outfile, const char *tok, const char
       		fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
       }
       
     --static void print_all(FILE *outfile, struct list_head *head,
     --		      const struct process_trailer_options *opts)
     -+void print_all(FILE *outfile, struct list_head *head,
     -+	       const struct process_trailer_options *opts)
     +-static void format_trailers(const struct process_trailer_options *opts,
     +-			    struct list_head *trailers, FILE *outfile)
     ++void format_trailers(const struct process_trailer_options *opts,
     ++		     struct list_head *trailers, FILE *outfile)
       {
       	struct list_head *pos;
       	struct trailer_item *item;
     @@ trailer.c: static int git_trailer_config(const char *conf_key, const char *value
       	return 0;
       }
       
     --static void ensure_configured(void)
     -+void ensure_configured(void)
     +-static void trailer_config_init(void)
     ++void trailer_config_init(void)
       {
       	if (configured)
       		return;
     @@ trailer.c: static void unfold_value(struct strbuf *val)
      -			     const char *str,
      -			     struct list_head *head,
      -			     const struct process_trailer_options *opts)
     -+void parse_trailers(struct trailer_info *info,
     ++void parse_trailers(const struct process_trailer_options *opts,
     ++		    struct trailer_info *info,
      +		    const char *str,
     -+		    struct list_head *head,
     -+		    const struct process_trailer_options *opts)
     ++		    struct list_head *head)
       {
       	struct strbuf tok = STRBUF_INIT;
       	struct strbuf val = STRBUF_INIT;
     @@ trailer.c: static void parse_trailers(struct trailer_info *info,
       	}
       }
       
     --static void free_all(struct list_head *head)
     -+void free_all(struct list_head *head)
     +-static void free_trailers(struct list_head *trailers)
     ++void free_trailers(struct list_head *trailers)
       {
       	struct list_head *pos, *p;
     - 	list_for_each_safe(pos, p, head) {
     -@@ trailer.c: static void free_all(struct list_head *head)
     + 	list_for_each_safe(pos, p, trailers) {
     +@@ trailer.c: static void free_trailers(struct list_head *trailers)
       	}
       }
       
     @@ trailer.c: static void free_all(struct list_head *head)
      -	return outfile;
      -}
      -
     --void process_trailers(const char *file,
     --		      const struct process_trailer_options *opts,
     --		      struct list_head *new_trailer_head)
     +-void interpret_trailers(const struct process_trailer_options *opts,
     +-			struct list_head *new_trailer_head,
     +-			const char *file)
      -{
      -	LIST_HEAD(head);
      -	struct strbuf sb = STRBUF_INIT;
      -	struct trailer_info info;
      -	FILE *outfile = stdout;
      -
     --	ensure_configured();
     +-	trailer_config_init();
      -
      -	read_input_file(&sb, file);
      -
     @@ trailer.c: static void free_all(struct list_head *head)
      -		process_trailers_lists(&head, &arg_head);
      -	}
      -
     --	print_all(outfile, &head, opts);
     +-	format_trailers(opts, &head, outfile);
      -
     --	free_all(&head);
     +-	free_trailers(&head);
      -	trailer_info_release(&info);
      -
      -	/* Print the lines after the trailers as is */
     @@ trailer.h: struct process_trailer_options {
       
       #define PROCESS_TRAILER_OPTIONS_INIT {0}
       
     --void process_trailers(const char *file,
     --		      const struct process_trailer_options *opts,
     --		      struct list_head *new_trailer_head);
     +-void interpret_trailers(const struct process_trailer_options *opts,
     +-			struct list_head *new_trailer_head,
     +-			const char *file);
      +void parse_trailers_from_config(struct list_head *config_head);
      +
      +void parse_trailers_from_command_line_args(struct list_head *arg_head,
     @@ trailer.h: struct process_trailer_options {
      +void process_trailers_lists(struct list_head *head,
      +			    struct list_head *arg_head);
      +
     -+void parse_trailers(struct trailer_info *info,
     ++void parse_trailers(const struct process_trailer_options *opts,
     ++		    struct trailer_info *info,
      +		    const char *str,
     -+		    struct list_head *head,
     -+		    const struct process_trailer_options *opts);
     ++		    struct list_head *head);
       
       void trailer_info_get(struct trailer_info *info, const char *str,
       		      const struct process_trailer_options *opts);
       
       void trailer_info_release(struct trailer_info *info);
       
     -+void ensure_configured(void);
     -+void print_all(FILE *outfile, struct list_head *head,
     -+	       const struct process_trailer_options *opts);
     -+void free_all(struct list_head *head);
     ++void trailer_config_init(void);
     ++void format_trailers(const struct process_trailer_options *opts,
     ++		     struct list_head *trailers, FILE *outfile);
     ++void free_trailers(struct list_head *trailers);
      +
       /*
        * Format the trailers from the commit msg "msg" into the strbuf "out".
  3:  d3326021fb6 !  3:  9b7747d550e trailer: unify trailer formatting machinery
     @@ Commit message
          format_trailers() to process trailers with the additional
          process_trailer_options fields like opts->key_only which is only used by
          format_trailers_from_commit() and not builtin/interpret-trailers.c.
     +    While we're at it, reorder parameters to put the trailer processing
     +    options first, and the out parameter (strbuf we write into) at the end.
      
     -    This will allow us to delete the format_trailer_info() and
     +    This unification will allow us to delete the format_trailer_info() and
          print_tok_val() functions in the next patch. They are not deleted here
          in order to keep the diff small.
      
     +    Helped-by: Junio C Hamano <[email protected]>
          Signed-off-by: Linus Arver <[email protected]>
      
       ## builtin/interpret-trailers.c ##
     -@@ builtin/interpret-trailers.c: static void interpret_trailers(const char *file,
     +@@ builtin/interpret-trailers.c: static void interpret_trailers(const struct process_trailer_options *opts,
       {
       	LIST_HEAD(head);
       	struct strbuf sb = STRBUF_INIT;
     @@ builtin/interpret-trailers.c: static void interpret_trailers(const char *file,
       	struct trailer_info info;
       	FILE *outfile = stdout;
       
     -@@ builtin/interpret-trailers.c: static void interpret_trailers(const char *file,
     +@@ builtin/interpret-trailers.c: static void interpret_trailers(const struct process_trailer_options *opts,
     + 		process_trailers_lists(&head, &arg_head);
       	}
       
     - 	/* Print trailer block. */
     --	format_trailers(outfile, &head, opts);
     -+	format_trailers(&head, opts, &trailer_block);
     +-	format_trailers(opts, &head, outfile);
     ++	/* Print trailer block. */
     ++	format_trailers(opts, &head, &trailer_block);
      +	fwrite(trailer_block.buf, 1, trailer_block.len, outfile);
      +	strbuf_release(&trailer_block);
       
     @@ pretty.c: static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
       		}
       		if (*arg == ')') {
      -			format_trailers_from_commit(sb, msg + c->subject_off, &opts);
     -+			format_trailers_from_commit(msg + c->subject_off, &opts, sb);
     ++			format_trailers_from_commit(&opts, msg + c->subject_off, sb);
       			ret = arg - placeholder + 1;
       		}
       	trailer_out:
     @@ ref-filter.c: static void grab_sub_body_contents(struct atom_value *val, int der
       
       			/* 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(subpos, &atom->u.contents.trailer_opts, &s);
     ++			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)
     @@ trailer.c: static void print_tok_val(FILE *outfile, const char *tok, const char
       		fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
       }
       
     --void format_trailers(FILE *outfile, struct list_head *head,
     --		     const struct process_trailer_options *opts)
     +-void format_trailers(const struct process_trailer_options *opts,
     +-		     struct list_head *trailers, FILE *outfile)
      -{
      -	struct list_head *pos;
      -	struct trailer_item *item;
     --	list_for_each(pos, head) {
     +-	list_for_each(pos, trailers) {
      -		item = list_entry(pos, struct trailer_item, list);
      -		if ((!opts->trim_empty || strlen(item->value) > 0) &&
      -		    (!opts->only_trailers || item->token))
     @@ trailer.c: static void unfold_value(struct strbuf *val)
       	strbuf_release(&out);
       }
       
     -+void format_trailers(struct list_head *head,
     -+		     const struct process_trailer_options *opts,
     ++void format_trailers(const struct process_trailer_options *opts,
     ++		     struct list_head *trailers,
      +		     struct strbuf *out)
      +{
      +	struct list_head *pos;
      +	struct trailer_item *item;
      +	int need_separator = 0;
      +
     -+	list_for_each(pos, head) {
     ++	list_for_each(pos, trailers) {
      +		item = list_entry(pos, struct trailer_item, list);
      +		if (item->token) {
      +			char c;
     @@ trailer.c: 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 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)
       {
      +	LIST_HEAD(head);
     @@ trailer.c: static void format_trailer_info(struct strbuf *out,
       
      -	trailer_info_get(&info, msg, opts);
      -	format_trailer_info(out, &info, msg, opts);
     -+	parse_trailers(&info, msg, &head, opts);
     ++	parse_trailers(opts, &info, msg, &head);
      +
      +	/* If we want the whole block untouched, we can take the fast path. */
      +	if (!opts->only_trailers && !opts->unfold && !opts->filter &&
     @@ trailer.c: static void format_trailer_info(struct strbuf *out,
      +		strbuf_add(out, msg + info.trailer_block_start,
      +			   info.trailer_block_end - info.trailer_block_start);
      +	} else
     -+		format_trailers(&head, opts, out);
     ++		format_trailers(opts, &head, out);
      +
      +	free_trailers(&head);
       	trailer_info_release(&info);
     @@ trailer.c: static void format_trailer_info(struct strbuf *out,
      
       ## trailer.h ##
      @@ trailer.h: void trailer_info_release(struct trailer_info *info);
     + 
       void trailer_config_init(void);
     + void format_trailers(const struct process_trailer_options *opts,
     +-		     struct list_head *trailers, FILE *outfile);
     ++		     struct list_head *trailers,
     ++		     struct strbuf *out);
       void free_trailers(struct list_head *trailers);
       
     --void format_trailers(FILE *outfile, struct list_head *head,
     --		     const struct process_trailer_options *opts);
     -+void format_trailers(struct list_head *head,
     -+		     const struct process_trailer_options *opts,
     -+		     struct strbuf *out);
       /*
      - * Format the trailers from the commit msg "msg" into the strbuf "out".
      - * Note two caveats about "opts":
     @@ trailer.h: void trailer_info_release(struct trailer_info *info);
      - *   - this is primarily a helper for pretty.c, and not
      - *     all of the flags are supported.
      - *
     -- *   - this differs from format_trailers slightly in that we always format
     +- *   - this differs from process_trailers slightly in that we always format
      - *     only the trailer block itself, even if the "only_trailers" option is not
      - *     set.
      + * Convenience function to format the trailers from the commit msg "msg" into
     @@ trailer.h: void trailer_info_release(struct trailer_info *info);
        */
      -void format_trailers_from_commit(struct strbuf *out, const char *msg,
      -				 const struct process_trailer_options *opts);
     -+void format_trailers_from_commit(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);
       
       /*
  4:  8d864614757 !  4:  f1171f5202f trailer: delete obsolete formatting functions
     @@ trailer.c: void trailer_info_release(struct trailer_info *info)
      -
      -}
      -
     - void format_trailers_from_commit(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)
  5:  fd4a9d54d95 !  5:  5ba842b5005 sequencer: use the trailer iterator
     @@ sequencer.c: static const char *get_todo_path(const struct replay_opts *opts)
      +		i++;
      +		if (sob &&
      +		    iter.is_trailer &&
     -+		    !strncmp(iter.raw.buf, sob->buf, sob->len)) {
     ++		    !strncmp(iter.raw, sob->buf, sob->len)) {
      +			found_sob = i;
      +		}
      +	}
     @@ sequencer.c: static const char *get_todo_path(const struct replay_opts *opts)
      
       ## trailer.c ##
      @@ trailer.c: void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
     - 	struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
     - 	strbuf_init(&iter->key, 0);
     - 	strbuf_init(&iter->val, 0);
     -+	strbuf_init(&iter->raw, 0);
     - 	opts.no_divider = 1;
     - 	trailer_info_get(&iter->internal.info, msg, &opts);
     - 	iter->internal.cur = 0;
     -@@ trailer.c: void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
       
       int trailer_iterator_advance(struct trailer_iterator *iter)
       {
     @@ trailer.c: void trailer_iterator_init(struct trailer_iterator *iter, const char
      +		separator_pos = find_separator(line, separators);
      +		iter->is_trailer = (separator_pos > 0);
      +
     -+		strbuf_reset(&iter->raw);
     -+		strbuf_addstr(&iter->raw, line);
     ++		iter->raw = line;
       		strbuf_reset(&iter->key);
       		strbuf_reset(&iter->val);
       		parse_trailer(&iter->key, &iter->val, NULL,
     @@ trailer.c: void trailer_iterator_init(struct trailer_iterator *iter, const char
       		unfold_value(&iter->val);
       		return 1;
       	}
     -@@ trailer.c: void trailer_iterator_release(struct trailer_iterator *iter)
     - 	trailer_info_release(&iter->internal.info);
     - 	strbuf_release(&iter->val);
     - 	strbuf_release(&iter->key);
     -+	strbuf_release(&iter->raw);
     - }
      
       ## trailer.h ##
      @@ trailer.h: struct trailer_iterator {
     @@ trailer.h: struct trailer_iterator {
       
      +	/*
      +	 * Raw line (e.g., "foo: bar baz") before being parsed as a trailer
     -+	 * key/val pair. This field can contain non-trailer lines because it's
     -+	 * valid for a trailer block to contain such lines (i.e., we only
     -+	 * require 25% of the lines in a trailer block to be trailer lines).
     ++	 * 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).
      +	 */
     -+	struct strbuf raw;
     ++	const char *raw;
      +
      +	/*
     -+	 * 1 if the raw line was parsed as a separate key/val pair.
     ++	 * 1 if the raw line was parsed as a trailer line (key/val pair).
      +	 */
      +	int is_trailer;
      +
  6:  0cbe96421c7 !  6:  f0ac2f6c4b9 trailer: make trailer_info struct private
     @@ Commit message
          Make this struct private by putting its definition inside trailer.c.
          This has two benefits:
      
     -    (1) it makes the surface area of the public facing interface (trailer.h)
     -        smaller, and
     +      (1) it makes the surface area of the public facing
     +          interface (trailer.h) smaller, and
      
     -    (2) external API users are unable to peer inside this struct (because it
     -        is only ever exposed as an opaque pointer).
     +      (2) external API users are unable to peer inside this struct (because
     +          it is only ever exposed as an opaque pointer).
      
          This change exposes some deficiencies in the API, mainly with regard to
          information about the location of the trailer block that was parsed.
     @@ Commit message
          Signed-off-by: Linus Arver <[email protected]>
      
       ## builtin/interpret-trailers.c ##
     -@@ builtin/interpret-trailers.c: static void interpret_trailers(const char *file,
     +@@ builtin/interpret-trailers.c: static void interpret_trailers(const struct process_trailer_options *opts,
       	LIST_HEAD(head);
       	struct strbuf sb = STRBUF_INIT;
       	struct strbuf trailer_block = STRBUF_INIT;
     @@ builtin/interpret-trailers.c: static void interpret_trailers(const char *file,
       	FILE *outfile = stdout;
       
       	trailer_config_init();
     -@@ builtin/interpret-trailers.c: static void interpret_trailers(const char *file,
     +@@ builtin/interpret-trailers.c: static void interpret_trailers(const struct process_trailer_options *opts,
       	if (opts->in_place)
       		outfile = create_in_place_tempfile(file);
       
     --	parse_trailers(&info, sb.buf, &head, opts);
     -+	info = parse_trailers(sb.buf, &head, opts);
     +-	parse_trailers(opts, &info, sb.buf, &head);
     ++	info = parse_trailers(opts, sb.buf, &head);
       
       	/* Print the lines before the trailers */
       	if (!opts->only_trailers)
     @@ builtin/interpret-trailers.c: static void interpret_trailers(const char *file,
       		fprintf(outfile, "\n");
       
       
     -@@ builtin/interpret-trailers.c: static void interpret_trailers(const char *file,
     +@@ builtin/interpret-trailers.c: static void interpret_trailers(const struct process_trailer_options *opts,
       	strbuf_release(&trailer_block);
       
       	free_trailers(&head);
     @@ trailer.c
       struct conf_info {
       	char *name;
       	char *key;
     -@@ trailer.c: void format_trailers(struct list_head *head,
     +@@ trailer.c: void format_trailers(const struct process_trailer_options *opts,
       	}
       }
       
     @@ trailer.c: void format_trailers(struct list_head *head,
      +	return info;
      +}
      +
     -+static struct trailer_info *trailer_info_get(const char *str,
     -+					     const struct process_trailer_options *opts)
     ++static struct trailer_info *trailer_info_get(const struct process_trailer_options *opts,
     ++					     const char *str)
      +{
      +	struct trailer_info *info = trailer_info_new();
      +	size_t end_of_log_message = 0, trailer_block_start = 0;
     @@ trailer.c: void format_trailers(struct list_head *head,
        * Parse trailers in "str", populating the trailer info and "head"
        * linked list structure.
        */
     --void parse_trailers(struct trailer_info *info,
     +-void parse_trailers(const struct process_trailer_options *opts,
     +-		    struct trailer_info *info,
      -		    const char *str,
     --		    struct list_head *head,
     --		    const struct process_trailer_options *opts)
     -+struct trailer_info *parse_trailers(const char *str,
     -+				    struct list_head *head,
     -+				    const struct process_trailer_options *opts)
     +-		    struct list_head *head)
     ++struct trailer_info *parse_trailers(const struct process_trailer_options *opts,
     ++				    const char *str,
     ++				    struct list_head *head)
       {
      +	struct trailer_info *info;
       	struct strbuf tok = STRBUF_INIT;
     @@ trailer.c: void format_trailers(struct list_head *head,
       	size_t i;
       
      -	trailer_info_get(info, str, opts);
     -+	info = trailer_info_get(str, opts);
     ++	info = trailer_info_get(opts, str);
       
       	for (i = 0; i < info->trailer_nr; i++) {
       		int separator_pos;
     -@@ trailer.c: void parse_trailers(struct trailer_info *info,
     +@@ trailer.c: void parse_trailers(const struct process_trailer_options *opts,
       					 strbuf_detach(&val, NULL));
       		}
       	}
     @@ trailer.c: void parse_trailers(struct trailer_info *info,
      +	return info;
       }
       
     - void free_trailers(struct list_head *head)
     -@@ trailer.c: void free_trailers(struct list_head *head)
     + void free_trailers(struct list_head *trailers)
     +@@ trailer.c: void free_trailers(struct list_head *trailers)
       	}
       }
       
     @@ trailer.c: void trailer_info_release(struct trailer_info *info)
      +	free(info);
       }
       
     - void format_trailers_from_commit(const char *msg,
     -@@ trailer.c: void format_trailers_from_commit(const char *msg,
     + void format_trailers_from_commit(const struct process_trailer_options *opts,
     +@@ trailer.c: void format_trailers_from_commit(const struct process_trailer_options *opts,
       				 struct strbuf *out)
       {
       	LIST_HEAD(head);
      -	struct trailer_info info;
      -
     --	parse_trailers(&info, msg, &head, opts);
     -+	struct trailer_info *info = parse_trailers(msg, &head, opts);
     +-	parse_trailers(opts, &info, msg, &head);
     ++	struct trailer_info *info = parse_trailers(opts, msg, &head);
       
       	/* 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 char *msg,
      +		strbuf_add(out, msg + info->trailer_block_start,
      +			   info->trailer_block_end - info->trailer_block_start);
       	} else
     - 		format_trailers(&head, opts, out);
     + 		format_trailers(opts, &head, out);
       
       	free_trailers(&head);
      -	trailer_info_release(&info);
     @@ trailer.c: void format_trailers_from_commit(const char *msg,
      +	struct trailer_info *internal = trailer_info_new();
       	strbuf_init(&iter->key, 0);
       	strbuf_init(&iter->val, 0);
     - 	strbuf_init(&iter->raw, 0);
       	opts.no_divider = 1;
      -	trailer_info_get(&iter->internal.info, msg, &opts);
      +	iter->internal.info = internal;
     -+	iter->internal.info = trailer_info_get(msg, &opts);
     ++	iter->internal.info = trailer_info_get(&opts, msg);
       	iter->internal.cur = 0;
       }
       
     @@ trailer.c: int trailer_iterator_advance(struct trailer_iterator *iter)
      +	trailer_info_release(iter->internal.info);
       	strbuf_release(&iter->val);
       	strbuf_release(&iter->key);
     - 	strbuf_release(&iter->raw);
     + }
      
       ## trailer.h ##
      @@
     @@ trailer.h: void parse_trailers_from_command_line_args(struct list_head *arg_head
       void process_trailers_lists(struct list_head *head,
       			    struct list_head *arg_head);
       
     --void parse_trailers(struct trailer_info *info,
     +-void parse_trailers(const struct process_trailer_options *opts,
     +-		    struct trailer_info *info,
      -		    const char *str,
     --		    struct list_head *head,
     --		    const struct process_trailer_options *opts);
     -+struct trailer_info *parse_trailers(const char *str,
     -+				    struct list_head *head,
     -+				    const struct process_trailer_options *opts);
     +-		    struct list_head *head);
     ++struct trailer_info *parse_trailers(const struct process_trailer_options *opts,
     ++				    const char *str,
     ++				    struct list_head *head);
       
      -void trailer_info_get(struct trailer_info *info, const char *str,
      -		      const struct process_trailer_options *opts);
  7:  9183990583f !  7:  291aa83af55 trailer: spread usage of "trailer_block" language
     @@ Commit message
          Signed-off-by: Linus Arver <[email protected]>
      
       ## builtin/interpret-trailers.c ##
     -@@ builtin/interpret-trailers.c: static void interpret_trailers(const char *file,
     +@@ builtin/interpret-trailers.c: static void interpret_trailers(const struct process_trailer_options *opts,
       {
       	LIST_HEAD(head);
       	struct strbuf sb = STRBUF_INIT;
     @@ builtin/interpret-trailers.c: static void interpret_trailers(const char *file,
       	FILE *outfile = stdout;
       
       	trailer_config_init();
     -@@ builtin/interpret-trailers.c: static void interpret_trailers(const char *file,
     +@@ builtin/interpret-trailers.c: static void interpret_trailers(const struct process_trailer_options *opts,
       	if (opts->in_place)
       		outfile = create_in_place_tempfile(file);
       
     --	info = parse_trailers(sb.buf, &head, opts);
     -+	trailer_block = parse_trailers(sb.buf, opts, &head);
     +-	info = parse_trailers(opts, sb.buf, &head);
     ++	trailer_block = parse_trailers(opts, sb.buf, &head);
       
      -	/* Print the lines before the trailers */
      +	/* Print the lines before the trailer block */
     @@ builtin/interpret-trailers.c: static void interpret_trailers(const char *file,
       		fprintf(outfile, "\n");
       
       
     -@@ builtin/interpret-trailers.c: static void interpret_trailers(const char *file,
     +@@ builtin/interpret-trailers.c: static void interpret_trailers(const struct process_trailer_options *opts,
       	}
       
       	/* Print trailer block. */
     --	format_trailers(&head, opts, &trailer_block);
     +-	format_trailers(opts, &head, &trailer_block);
      -	fwrite(trailer_block.buf, 1, trailer_block.len, outfile);
      -	strbuf_release(&trailer_block);
     -+	format_trailers(&head, opts, &tb);
     ++	format_trailers(opts, &head, &tb);
      +	fwrite(tb.buf, 1, tb.len, outfile);
      +	strbuf_release(&tb);
       
     @@ trailer.c
       
       	/*
       	 * Array of trailers found.
     -@@ trailer.c: void format_trailers(struct list_head *head,
     +@@ trailer.c: void format_trailers(const struct process_trailer_options *opts,
       	}
       }
       
     @@ trailer.c: void format_trailers(struct list_head *head,
      +	return trailer_block;
       }
       
     --static struct trailer_info *trailer_info_get(const char *str,
     --					     const struct process_trailer_options *opts)
     -+static struct trailer_block *trailer_block_get(const char *str,
     -+					       const struct process_trailer_options *opts)
     +-static struct trailer_info *trailer_info_get(const struct process_trailer_options *opts,
     +-					     const char *str)
     ++static struct trailer_block *trailer_block_get(const struct process_trailer_options *opts,
     ++					       const char *str)
       {
      -	struct trailer_info *info = trailer_info_new();
      +	struct trailer_block *trailer_block = trailer_block_new();
       	size_t end_of_log_message = 0, trailer_block_start = 0;
       	struct strbuf **trailer_lines, **ptr;
       	char **trailer_strings = NULL;
     -@@ trailer.c: static struct trailer_info *trailer_info_get(const char *str,
     +@@ trailer.c: static struct trailer_info *trailer_info_get(const struct process_trailer_option
       	}
       	strbuf_list_free(trailer_lines);
       
     @@ trailer.c: static struct trailer_info *trailer_info_get(const char *str,
      + * Parse trailers in "str", populating the trailer_block info and "head" linked
      + * list structure.
        */
     --struct trailer_info *parse_trailers(const char *str,
     --				    struct list_head *head,
     --				    const struct process_trailer_options *opts)
     -+struct trailer_block *parse_trailers(const char *str,
     -+				     const struct process_trailer_options *opts,
     +-struct trailer_info *parse_trailers(const struct process_trailer_options *opts,
     +-				    const char *str,
     +-				    struct list_head *head)
     ++struct trailer_block *parse_trailers(const struct process_trailer_options *opts,
     ++				     const char *str,
      +				     struct list_head *head)
       {
      -	struct trailer_info *info;
     @@ trailer.c: static struct trailer_info *trailer_info_get(const char *str,
       	struct strbuf val = STRBUF_INIT;
       	size_t i;
       
     --	info = trailer_info_get(str, opts);
     -+	trailer_block = trailer_block_get(str, opts);
     +-	info = trailer_info_get(opts, str);
     ++	trailer_block = trailer_block_get(opts, str);
       
      -	for (i = 0; i < info->trailer_nr; i++) {
      +	for (i = 0; i < trailer_block->trailer_nr; i++) {
     @@ trailer.c: static struct trailer_info *trailer_info_get(const char *str,
       		if (trailer[0] == comment_line_char)
       			continue;
       		separator_pos = find_separator(trailer, separators);
     -@@ trailer.c: struct trailer_info *parse_trailers(const char *str,
     +@@ trailer.c: struct trailer_info *parse_trailers(const struct process_trailer_options *opts,
       		}
       	}
       
     @@ trailer.c: struct trailer_info *parse_trailers(const char *str,
      +	return trailer_block;
       }
       
     - void free_trailers(struct list_head *head)
     -@@ trailer.c: void free_trailers(struct list_head *head)
     + void free_trailers(struct list_head *trailers)
     +@@ trailer.c: void free_trailers(struct list_head *trailers)
       	}
       }
       
     @@ trailer.c: void free_trailers(struct list_head *head)
      +	free(trailer_block);
       }
       
     - void format_trailers_from_commit(const char *msg,
     -@@ trailer.c: void format_trailers_from_commit(const char *msg,
     + void format_trailers_from_commit(const struct process_trailer_options *opts,
     +@@ trailer.c: void format_trailers_from_commit(const struct process_trailer_options *opts,
       				 struct strbuf *out)
       {
       	LIST_HEAD(head);
     --	struct trailer_info *info = parse_trailers(msg, &head, opts);
     -+	struct trailer_block *trailer_block = parse_trailers(msg, opts, &head);
     +-	struct trailer_info *info = parse_trailers(opts, msg, &head);
     ++	struct trailer_block *trailer_block = parse_trailers(opts, msg, &head);
       
       	/* 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 char *msg,
      +		strbuf_add(out, msg + trailer_block->start,
      +			   trailer_block->end - trailer_block->start);
       	} else
     - 		format_trailers(&head, opts, out);
     + 		format_trailers(opts, &head, out);
       
       	free_trailers(&head);
      -	trailer_info_release(info);
     @@ trailer.c: void format_trailers_from_commit(const char *msg,
      -	struct trailer_info *internal = trailer_info_new();
       	strbuf_init(&iter->key, 0);
       	strbuf_init(&iter->val, 0);
     - 	strbuf_init(&iter->raw, 0);
       	opts.no_divider = 1;
      -	iter->internal.info = internal;
     --	iter->internal.info = trailer_info_get(msg, &opts);
     -+	iter->internal.trailer_block = trailer_block_get(msg, &opts);
     +-	iter->internal.info = trailer_info_get(&opts, msg);
     ++	iter->internal.trailer_block = trailer_block_get(&opts, msg);
       	iter->internal.cur = 0;
       }
       
     @@ trailer.c: int trailer_iterator_advance(struct trailer_iterator *iter)
      +	trailer_block_release(iter->internal.trailer_block);
       	strbuf_release(&iter->val);
       	strbuf_release(&iter->key);
     - 	strbuf_release(&iter->raw);
     + }
      
       ## trailer.h ##
      @@
     @@ trailer.h: void parse_trailers_from_command_line_args(struct list_head *arg_head
       void process_trailers_lists(struct list_head *head,
       			    struct list_head *arg_head);
       
     --struct trailer_info *parse_trailers(const char *str,
     --				    struct list_head *head,
     --				    const struct process_trailer_options *opts);
     -+struct trailer_block *parse_trailers(const char *str,
     -+				     const struct process_trailer_options *opts,
     +-struct trailer_info *parse_trailers(const struct process_trailer_options *opts,
     +-				    const char *str,
     +-				    struct list_head *head);
     ++struct trailer_block *parse_trailers(const struct process_trailer_options *opts,
     ++				     const char *str,
      +				     struct list_head *head);
       
      -size_t trailer_block_start(struct trailer_info *info);
     @@ trailer.h: void parse_trailers_from_command_line_args(struct list_head *arg_head
      +void trailer_block_release(struct trailer_block *trailer_block);
       
       void trailer_config_init(void);
     - void free_trailers(struct list_head *trailers);
     + void format_trailers(const struct process_trailer_options *opts,
      @@ trailer.h: struct trailer_iterator {
       
       	/* private */

8: 406725df46a ! 8: 64ee07d0b53 trailer: prepare to move parse_trailers_from_command_line_args() to builtin
@@ Commit message
parse_trailers_from_command_line_args()

to interpret-trailer.c, because the trailer API should not be concerned
- with command line arguments (as it has nothing to do with trailers
- themselves). The interpret-trailers builtin is the only user of the
- above function.
+ with command line arguments (as they have nothing to do with trailers
+ themselves). The interpret-trailers builtin is the only caller of this
+ function.

+ Also rename "conf_info" to "trailer_conf" for readability, dropping the
+ low-value "_info" suffix as we did earlier in this series for
+ "trailer_info" to "trailer_block".
+
+ Helped-by: Josh Steadmon <[email protected]>

## trailer.c ##
@@ trailer.c: void parse_trailers_from_command_line_args(struct list_head *arg_head
add_arg_item(arg_head,
strbuf_detach(&tok, NULL),
strbuf_detach(&val, NULL),
-@@ trailer.c: struct trailer_block *parse_trailers(const char *str,
+@@ trailer.c: struct trailer_block *parse_trailers(const struct process_trailer_options *opts,

for (i = 0; i < trailer_block->trailer_nr; i++) {
int separator_pos;
@@ trailer.c: struct trailer_block *parse_trailers(const char *str,
add_trailer_item(head,
NULL,
@@ trailer.c: int trailer_iterator_advance(struct trailer_iterator *iter)
- strbuf_addstr(&iter->raw, line);
+ iter->raw = line;
strbuf_reset(&iter->key);
strbuf_reset(&iter->val);
- parse_trailer(&iter->key, &iter->val, NULL,
@@ trailer.h: void parse_trailers_from_command_line_args(struct list_head *arg_head
+ struct strbuf *tok, struct strbuf *val,
+ const struct trailer_conf **conf);
+
- struct trailer_block *parse_trailers(const char *str,
- const struct process_trailer_options *opts,
+ struct trailer_block *parse_trailers(const struct process_trailer_options *opts,
+ const char *str,
struct list_head *head);
9: 8a99d0fca21 ! 9: 1b4bdde65bc trailer: move arg handling to interpret-trailers.c

     @@ builtin/interpret-trailers.c: static int option_parse_trailer(const struct optio
       }
       
      @@ builtin/interpret-trailers.c: static void read_input_file(struct strbuf *sb, const char *file)
     + }
       
     - static void interpret_trailers(const char *file,
     - 			       const struct process_trailer_options *opts,
     --			       struct list_head *new_trailer_head)
     -+			       struct list_head *arg_trailers)
     + static void interpret_trailers(const struct process_trailer_options *opts,
     +-			       struct list_head *new_trailer_head,
     ++			       struct list_head *arg_trailers,
     + 			       const char *file)
       {
       	LIST_HEAD(head);
     - 	struct strbuf sb = STRBUF_INIT;
     -@@ builtin/interpret-trailers.c: static void interpret_trailers(const char *file,
     +@@ builtin/interpret-trailers.c: static void interpret_trailers(const struct process_trailer_options *opts,
       	struct trailer_block *trailer_block;
       	FILE *outfile = stdout;
       
     @@ builtin/interpret-trailers.c: static void interpret_trailers(const char *file,
       	read_input_file(&sb, file);
       
       	if (opts->in_place)
     -@@ builtin/interpret-trailers.c: static void interpret_trailers(const char *file,
     +@@ builtin/interpret-trailers.c: static void interpret_trailers(const struct process_trailer_options *opts,
       
       
       	if (!opts->only_input) {
     @@ builtin/interpret-trailers.c: static void interpret_trailers(const char *file,
       	}
       
       	/* Print trailer block. */
     -@@ builtin/interpret-trailers.c: static void interpret_trailers(const char *file,
     +@@ builtin/interpret-trailers.c: static void interpret_trailers(const struct process_trailer_options *opts,
       int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
       {
       	struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
     @@ builtin/interpret-trailers.c: int cmd_interpret_trailers(int argc, const char **
       	if (argc) {
       		int i;
       		for (i = 0; i < argc; i++)
     --			interpret_trailers(argv[i], &opts, &trailers);
     -+			interpret_trailers(argv[i], &opts, &arg_trailers);
     +-			interpret_trailers(&opts, &trailers, argv[i]);
     ++			interpret_trailers(&opts, &arg_trailers, argv[i]);
       	} else {
       		if (opts.in_place)
       			die(_("no input file given for in-place editing"));
     --		interpret_trailers(NULL, &opts, &trailers);
     -+		interpret_trailers(NULL, &opts, &arg_trailers);
     +-		interpret_trailers(&opts, &trailers, NULL);
     ++		interpret_trailers(&opts, &arg_trailers, NULL);
       	}
       
      -	new_trailers_clear(&trailers);
     @@ trailer.c: void parse_trailers_from_command_line_args(struct list_head *arg_head
       		}
       	}
       
     -@@ trailer.c: void free_trailers(struct list_head *head)
     +@@ trailer.c: void free_trailers(struct list_head *trailers)
       	}
       }
       
     @@ trailer.h: struct new_trailer_item {
       struct process_trailer_options {
       	int in_place;
       	int trim_empty;
     -@@ trailer.h: void trailer_block_release(struct trailer_block *trailer_block);
     - 
     - void trailer_config_init(void);
     +@@ trailer.h: void format_trailers(const struct process_trailer_options *opts,
     + 		     struct list_head *trailers,
     + 		     struct strbuf *out);
       void free_trailers(struct list_head *trailers);
     -+void new_trailers_clear(struct list_head *trailers);
     - 
     - void format_trailers(struct list_head *head,
     - 		     const struct process_trailer_options *opts,
     +-
     ++void new_trailers_clear(struct list_head *new_trailers);
     + /*
     +  * Convenience function to format the trailers from the commit msg "msg" into
     +  * the strbuf "out". Reuses format_trailers internally.

10: 243eee730e4 = 10: ed67ebf8647 trailer: delete obsolete argument handling code from API

gitgitgadget

From: Linus Arver <[email protected]>

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 trailer.h.

Rename relevant functions so that they include the term "trailer" in their name, so that clients of the API will be able to easily identify them by their "trailer" moniker, just like all the other functions already exposed by trailer.h.

The the opportunity to start putting trailer processions options (opts) as the first parameter. This will be the pattern going forward in this series.

---
 builtin/interpret-trailers.c |  4 ++--
 trailer.c                    | 26 +++++++++++++-------------
 trailer.h                    |  6 +++---
 3 files changed, 18 insertions(+), 18 deletions(-)

index 033bd1556cf..85a3413baf5 100644
@@ -132,11 +132,11 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 	if (argc) {
 		int i;
 		for (i = 0; i < argc; i++)
-			process_trailers(argv[i], &opts, &trailers);
+			interpret_trailers(&opts, &trailers, argv[i]);
 	} else {
 		if (opts.in_place)
 			die(_("no input file given for in-place editing"));
-		process_trailers(NULL, &opts, &trailers);
+		interpret_trailers(&opts, &trailers, NULL);
 	}
 
 	new_trailers_clear(&trailers);

index 3a0710a4583..66b6660f5a4 100644
@@ -163,12 +163,12 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
 		fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
 }
 
-static void print_all(FILE *outfile, struct list_head *head,
-		      const struct process_trailer_options *opts)
+static void format_trailers(const struct process_trailer_options *opts,
+			    struct list_head *trailers, FILE *outfile)
 {
 	struct list_head *pos;
 	struct trailer_item *item;
-	list_for_each(pos, head) {
+	list_for_each(pos, trailers) {
 		item = list_entry(pos, struct trailer_item, list);
 		if ((!opts->trim_empty || strlen(item->value) > 0) &&
 		    (!opts->only_trailers || item->token))

@@ -589,7 +589,7 @@ static int git_trailer_config(const char *conf_key, const char *value,
 	return 0;
 }
 
-static void ensure_configured(void)
+static void trailer_config_init(void)
 {
 	if (configured)
 		return;

@@ -1035,10 +1035,10 @@ static void parse_trailers(struct trailer_info *info,
 	}
 }
 
-static void free_all(struct list_head *head)
+static void free_trailers(struct list_head *trailers)
 {
 	struct list_head *pos, *p;
-	list_for_each_safe(pos, p, head) {
+	list_for_each_safe(pos, p, trailers) {
 		list_del(pos);
 		free_trailer_item(list_entry(pos, struct trailer_item, list));
 	}

@@ -1075,16 +1075,16 @@ static FILE *create_in_place_tempfile(const char *file)
 	return outfile;
 }
 
-void process_trailers(const char *file,
-		      const struct process_trailer_options *opts,
-		      struct list_head *new_trailer_head)
+void interpret_trailers(const struct process_trailer_options *opts,
+			struct list_head *new_trailer_head,
+			const char *file)
 {
 	LIST_HEAD(head);
 	struct strbuf sb = STRBUF_INIT;
 	struct trailer_info info;
 	FILE *outfile = stdout;
 
-	ensure_configured();
+	trailer_config_init();
 
 	read_input_file(&sb, file);
 

@@ -1110,9 +1110,9 @@ void process_trailers(const char *file,
 		process_trailers_lists(&head, &arg_head);
 	}
 
-	print_all(outfile, &head, opts);
+	format_trailers(opts, &head, outfile);
 
-	free_all(&head);
+	free_trailers(&head);
 	trailer_info_release(&info);
 
 	/* Print the lines after the trailers as is */

@@ -1135,7 +1135,7 @@ void trailer_info_get(struct trailer_info *info, const char *str,
 	size_t nr = 0, alloc = 0;
 	char **last = NULL;
 
-	ensure_configured();
+	trailer_config_init();
 
 	end_of_log_message = find_end_of_log_message(str, opts->no_divider);
 	trailer_block_start = find_trailer_block_start(str, end_of_log_message);

index 1644cd05f60..37033e631a1 100644
@@ -81,9 +81,9 @@ struct process_trailer_options {
 
 #define PROCESS_TRAILER_OPTIONS_INIT {0}
 
-void process_trailers(const char *file,
-		      const struct process_trailer_options *opts,
-		      struct list_head *new_trailer_head);
+void interpret_trailers(const struct process_trailer_options *opts,
+			struct list_head *new_trailer_head,
+			const char *file);
 
 void trailer_info_get(struct trailer_info *info, const char *str,
 		      const struct process_trailer_options *opts);

gitgitgadget

Nitpick: typo in the commit message.
s/The the opportunity/Take the opportunity/ ?

Ack, will update in reroll.

From: Linus Arver <[email protected]>

The interpret-trailers.c builtin is the only place we need to call interpret_trailers(), so move its definition there.

Delete the corresponding declaration from trailer.h, which then forces us to expose the working innards of that function. This enriches trailer.h with a more granular API, which can then be unit-tested in the future (because interpret_trailers() by itself does too many things to be able to be easily unit-tested).

Take this opportunity to demote some file-handling functions out of the trailer API implementation, as these have nothing to do with trailers.

---
 builtin/interpret-trailers.c |  94 +++++++++++++++++++++++++++
 trailer.c                    | 120 ++++-------------------------------
 trailer.h                    |  20 +++++-
 3 files changed, 124 insertions(+), 110 deletions(-)

index 85a3413baf5..8556acde4aa 100644
@@ -9,6 +9,7 @@
 #include "gettext.h"
 #include "parse-options.h"
 #include "string-list.h"
+#include "tempfile.h"
 #include "trailer.h"
 #include "config.h"
 

@@ -91,6 +92,99 @@ static int parse_opt_parse(const struct option *opt, const char *arg,
 	return 0;
 }
 
+static struct tempfile *trailers_tempfile;
+
+static FILE *create_in_place_tempfile(const char *file)
+{
+	struct stat st;
+	struct strbuf filename_template = STRBUF_INIT;
+	const char *tail;
+	FILE *outfile;
+
+	if (stat(file, &st))
+		die_errno(_("could not stat %s"), file);
+	if (!S_ISREG(st.st_mode))
+		die(_("file %s is not a regular file"), file);
+	if (!(st.st_mode & S_IWUSR))
+		die(_("file %s is not writable by user"), file);
+
+	/* Create temporary file in the same directory as the original */
+	tail = strrchr(file, '/');
+	if (tail)
+		strbuf_add(&filename_template, file, tail - file + 1);
+	strbuf_addstr(&filename_template, "git-interpret-trailers-XXXXXX");
+
+	trailers_tempfile = xmks_tempfile_m(filename_template.buf, st.st_mode);
+	strbuf_release(&filename_template);
+	outfile = fdopen_tempfile(trailers_tempfile, "w");
+	if (!outfile)
+		die_errno(_("could not open temporary file"));
+
+	return outfile;
+}
+
+static void read_input_file(struct strbuf *sb, const char *file)
+{
+	if (file) {
+		if (strbuf_read_file(sb, file, 0) < 0)
+			die_errno(_("could not read input file '%s'"), file);
+	} else {
+		if (strbuf_read(sb, fileno(stdin), 0) < 0)
+			die_errno(_("could not read from stdin"));
+	}
+}
+
+static void interpret_trailers(const struct process_trailer_options *opts,
+			       struct list_head *new_trailer_head,
+			       const char *file)
+{
+	LIST_HEAD(head);
+	struct strbuf sb = STRBUF_INIT;
+	struct trailer_info info;
+	FILE *outfile = stdout;
+
+	trailer_config_init();
+
+	read_input_file(&sb, file);
+
+	if (opts->in_place)
+		outfile = create_in_place_tempfile(file);
+
+	parse_trailers(opts, &info, sb.buf, &head);
+
+	/* Print the lines before the trailers */
+	if (!opts->only_trailers)
+		fwrite(sb.buf, 1, info.trailer_block_start, outfile);
+
+	if (!opts->only_trailers && !info.blank_line_before_trailer)
+		fprintf(outfile, "\n");
+
+
+	if (!opts->only_input) {
+		LIST_HEAD(config_head);
+		LIST_HEAD(arg_head);
+		parse_trailers_from_config(&config_head);
+		parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
+		list_splice(&config_head, &arg_head);
+		process_trailers_lists(&head, &arg_head);
+	}
+
+	format_trailers(opts, &head, outfile);
+
+	free_trailers(&head);
+	trailer_info_release(&info);
+
+	/* Print the lines after the trailers as is */
+	if (!opts->only_trailers)
+		fwrite(sb.buf + info.trailer_block_end, 1, sb.len - info.trailer_block_end, outfile);
+
+	if (opts->in_place)
+		if (rename_tempfile(&trailers_tempfile, file))
+			die_errno(_("could not rename temporary file to %s"), file);
+
+	strbuf_release(&sb);
+}
+
 int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 {
 	struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;

index 37033e631a1..4f603e03ceb 100644
@@ -81,15 +81,29 @@ struct process_trailer_options {
 
 #define PROCESS_TRAILER_OPTIONS_INIT {0}
 
-void interpret_trailers(const struct process_trailer_options *opts,
-			struct list_head *new_trailer_head,
-			const char *file);
+void parse_trailers_from_config(struct list_head *config_head);
+
+void parse_trailers_from_command_line_args(struct list_head *arg_head,
+					   struct list_head *new_trailer_head);
+
+void process_trailers_lists(struct list_head *head,
+			    struct list_head *arg_head);
+
+void parse_trailers(const struct process_trailer_options *opts,
+		    struct trailer_info *info,
+		    const char *str,
+		    struct list_head *head);
 
 void trailer_info_get(struct trailer_info *info, const char *str,
 		      const struct process_trailer_options *opts);
 
 void trailer_info_release(struct trailer_info *info);
 
+void trailer_config_init(void);
+void format_trailers(const struct process_trailer_options *opts,
+		     struct list_head *trailers, FILE *outfile);
+void free_trailers(struct list_head *trailers);
+
 /*
  * Format the trailers from the commit msg "msg" into the strbuf "out".
  * Note two caveats about "opts":

gitgitgadget

From: Linus Arver <[email protected]>

Currently have two functions for formatting trailers exposed in trailer.h:

void format_trailers(FILE *outfile, struct list_head *head,
const struct process_trailer_options *opts);

void format_trailers_from_commit(struct strbuf *out, const char *msg,
const struct process_trailer_options *opts);

and previously these functions, although similar enough (even taking the same process_trailer_options struct pointer), did not build on each other.

Make format_trailers_from_commit() rely on format_trailers(). Teach format_trailers() to process trailers with the additional process_trailer_options fields like opts->key_only which is only used by format_trailers_from_commit() and not builtin/interpret-trailers.c. While we're at it, reorder parameters to put the trailer processing options first, and the out parameter (strbuf we write into) at the end.

This unification will allow us to delete the format_trailer_info() and print_tok_val() functions in the next patch. They are not deleted here in order to keep the diff small.

---
 builtin/interpret-trailers.c |   6 +-
 pretty.c                     |   2 +-
 ref-filter.c                 |   2 +-
 trailer.c                    | 105 +++++++++++++++++++++++++++++------
 trailer.h                    |  19 +++----
 5 files changed, 102 insertions(+), 32 deletions(-)

index 8556acde4aa..5352ee65bd1 100644
@@ -140,6 +140,7 @@ static void interpret_trailers(const struct process_trailer_options *opts,
 {
 	LIST_HEAD(head);
 	struct strbuf sb = STRBUF_INIT;
+	struct strbuf trailer_block = STRBUF_INIT;
 	struct trailer_info info;
 	FILE *outfile = stdout;
 

@@ -169,7 +170,10 @@ static void interpret_trailers(const struct process_trailer_options *opts,
 		process_trailers_lists(&head, &arg_head);
 	}
 
-	format_trailers(opts, &head, outfile);
+	/* Print trailer block. */
+	format_trailers(opts, &head, &trailer_block);
+	fwrite(trailer_block.buf, 1, trailer_block.len, outfile);
+	strbuf_release(&trailer_block);
 
 	free_trailers(&head);
 	trailer_info_release(&info);

index cf964b060cd..bdbed4295aa 100644
@@ -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:

index 35b989e1dfe..d358953b0ce 100644
@@ -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)

index 4f603e03ceb..c309b01323d 100644
@@ -101,22 +101,17 @@ void trailer_info_release(struct trailer_info *info);
 
 void trailer_config_init(void);
 void format_trailers(const struct process_trailer_options *opts,
-		     struct list_head *trailers, FILE *outfile);
+		     struct list_head *trailers,
+		     struct strbuf *out);
 void free_trailers(struct list_head *trailers);
 
 /*
- * Format the trailers from the commit msg "msg" into the strbuf "out".
- * Note two caveats about "opts":
- *
- *   - this is primarily a helper for pretty.c, and not
- *     all of the flags are supported.
- *
- *   - this differs from process_trailers slightly in that we always format
- *     only the trailer block itself, even if the "only_trailers" option is not
- *     set.
+ * Convenience function to format the trailers from the commit msg "msg" into
+ * the strbuf "out". Reuses format_trailers internally.
  */
-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

gitgitgadget

Unfortunately this breaks the build:

trailer.c:1145:13: error: ‘format_trailer_info’ defined but not used [-Werror=unused-function]

and

trailer.c:147:13: error: ‘print_tok_val’ defined but not used [-Werror=unused-function]

While separating this patch from the deletion does make it easier to review, it may make bisection more difficult.

FTR I've tried a preview version of squashing the deletion into this patch with "/preview" in GitGitGadget, and it generated a clean-enough diff where the deletions weren't intermixed with additions (maybe it uses the patience diff algorithm). But I didn't squash them for v2 because I was concerned about the range diff becoming even more difficult to read for reviewers.

I'm OK with squashing them for v3, but I'm also not sure that's necessary. For example, during bisection you could use DEVOPTS=no-error (or similar) in config.mak to skip over harmless errors like "unused-function". Personally I'd prefer to keep the patches separate because they started separately on the list.

Ultimately, I don't have a strong opinion on this. Maybe Junio or someone else can cast the tie-breaking vote? To squash or not to squash? I will take lazy consensus to mean "squash" for v3 if no one has objections. Thanks.

From: Linus Arver <[email protected]>

---
 trailer.c | 79 -------------------------------------------------------
 1 file changed, 79 deletions(-)

index 7692bf9eb40..71ea2bb67f8 100644
@@ -144,24 +144,6 @@ static char last_non_space_char(const char *s)
 	return '\0';
 }
 
-static void print_tok_val(FILE *outfile, const char *tok, const char *val)
-{
-	char c;
-
-	if (!tok) {
-		fprintf(outfile, "%s\n", val);
-		return;
-	}
-
-	c = last_non_space_char(tok);
-	if (!c)
-		return;
-	if (strchr(separators, c))
-		fprintf(outfile, "%s%s\n", tok, val);
-	else
-		fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
-}
-
 static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
 {
 	struct trailer_item *new_item = xcalloc(1, sizeof(*new_item));

@@ -1142,67 +1124,6 @@ void trailer_info_release(struct trailer_info *info)
 	free(info->trailers);
 }
 
-static void format_trailer_info(struct strbuf *out,
-				const struct trailer_info *info,
-				const char *msg,
-				const struct process_trailer_options *opts)
-{
-	size_t origlen = out->len;
-	size_t i;
-
-	/* If we want the whole block untouched, we can take the fast path. */
-	if (!opts->only_trailers && !opts->unfold && !opts->filter &&
-	    !opts->separator && !opts->key_only && !opts->value_only &&
-	    !opts->key_value_separator) {
-		strbuf_add(out, msg + info->trailer_block_start,
-			   info->trailer_block_end - info->trailer_block_start);
-		return;
-	}
-
-	for (i = 0; i < info->trailer_nr; i++) {
-		char *trailer = info->trailers[i];
-		ssize_t separator_pos = find_separator(trailer, separators);
-
-		if (separator_pos >= 1) {
-			struct strbuf tok = STRBUF_INIT;
-			struct strbuf val = STRBUF_INIT;
-
-			parse_trailer(&tok, &val, NULL, trailer, separator_pos);
-			if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
-				if (opts->unfold)
-					unfold_value(&val);
-
-				if (opts->separator && out->len != origlen)
-					strbuf_addbuf(out, opts->separator);
-				if (!opts->value_only)
-					strbuf_addbuf(out, &tok);
-				if (!opts->key_only && !opts->value_only) {
-					if (opts->key_value_separator)
-						strbuf_addbuf(out, opts->key_value_separator);
-					else
-						strbuf_addstr(out, ": ");
-				}
-				if (!opts->key_only)
-					strbuf_addbuf(out, &val);
-				if (!opts->separator)
-					strbuf_addch(out, '\n');
-			}
-			strbuf_release(&tok);
-			strbuf_release(&val);
-
-		} else if (!opts->only_trailers) {
-			if (opts->separator && out->len != origlen) {
-				strbuf_addbuf(out, opts->separator);
-			}
-			strbuf_addstr(out, trailer);
-			if (opts->separator) {
-				strbuf_rtrim(out);
-			}
-		}
-	}
-
-}
-
 void format_trailers_from_commit(const struct process_trailer_options *opts,
 				 const char *msg,
 				 struct strbuf *out)

gitgitgadget

From: Linus Arver <[email protected]>

This patch allows for the removal of "trailer_info_get()" from the trailer.h API, which will be in the next patch.

Instead of calling "trailer_info_get()", which is a low-level function in the trailers implementation (trailer.c), call trailer_iterator_advance(), which was specifically designed for public consumption in f0939a0eb1 (trailer: add interface for iterating over commit trailers, 2020-09-27).

Avoiding "trailer_info_get()" means we don't have to worry about options like "no_divider" (relevant for parsing trailers). We also don't have to check for things like "info.trailer_start == info.trailer_end" to see whether there were any trailers (instead we can just check to see whether the iterator advanced at all).

Also, teach the iterator about non-trailer lines, by adding a new field called "raw" to hold both trailer and non-trailer lines. This is necessary because a "trailer block" is a list of trailer lines of at least 25% trailers (see 146245063e (trailer: allow non-trailers in trailer block, 2016-10-21)), such that it may hold non-trailer lines.

---
 builtin/shortlog.c |  7 +++++--
 sequencer.c        | 35 +++++++++++++++--------------------
 trailer.c          | 17 +++++++++--------
 trailer.h          | 13 +++++++++++++
 4 files changed, 42 insertions(+), 30 deletions(-)

index 1307ed2b88a..dc8fd5a5532 100644
@@ -172,7 +172,7 @@ static void insert_records_from_trailers(struct shortlog *log,
 					 const char *oneline)
 {
 	struct trailer_iterator iter;
-	const char *commit_buffer, *body;
+	const char *commit_buffer, *body, *value;
 	struct strbuf ident = STRBUF_INIT;
 
 	if (!log->trailers.nr)

@@ -190,7 +190,10 @@ static void insert_records_from_trailers(struct shortlog *log,
 
 	trailer_iterator_init(&iter, body);
 	while (trailer_iterator_advance(&iter)) {
-		const char *value = iter.val.buf;
+		if (!iter.is_trailer)
+			continue;
+
+		value = iter.val.buf;
 
 		if (!string_list_has_string(&log->trailers, iter.key.buf))
 			continue;

index 3cc88d8a800..bc7c82c5271 100644
@@ -319,37 +319,32 @@ static const char *get_todo_path(const struct replay_opts *opts)
 static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
 	size_t ignore_footer)
 {
-	struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
-	struct trailer_info info;
-	size_t i;
-	int found_sob = 0, found_sob_last = 0;
-	char saved_char;
-
-	opts.no_divider = 1;
+	struct trailer_iterator iter;
+	size_t i = 0, found_sob = 0;
+	char saved_char = sb->buf[sb->len - ignore_footer];
 
 	if (ignore_footer) {
-		saved_char = sb->buf[sb->len - ignore_footer];
 		sb->buf[sb->len - ignore_footer] = '\0';
 	}
 
-	trailer_info_get(&info, sb->buf, &opts);
+	trailer_iterator_init(&iter, sb->buf);
+	while (trailer_iterator_advance(&iter)) {
+		i++;
+		if (sob &&
+		    iter.is_trailer &&
+		    !strncmp(iter.raw, sob->buf, sob->len)) {
+			found_sob = i;
+		}
+	}
+	trailer_iterator_release(&iter);
 
 	if (ignore_footer)
 		sb->buf[sb->len - ignore_footer] = saved_char;
 
-	if (info.trailer_block_start == info.trailer_block_end)
+	if (!i)
 		return 0;
 
-	for (i = 0; i < info.trailer_nr; i++)
-		if (sob && !strncmp(info.trailers[i], sob->buf, sob->len)) {
-			found_sob = 1;
-			if (i == info.trailer_nr - 1)
-				found_sob_last = 1;
-		}
-
-	trailer_info_release(&info);
-
-	if (found_sob_last)
+	if (found_sob == i)
 		return 3;
 	if (found_sob)
 		return 2;

index 71ea2bb67f8..5bcc9b0006c 100644
@@ -1158,17 +1158,18 @@ void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
 
 int trailer_iterator_advance(struct trailer_iterator *iter)
 {
-	while (iter->internal.cur < iter->internal.info.trailer_nr) {
-		char *trailer = iter->internal.info.trailers[iter->internal.cur++];
-		int separator_pos = find_separator(trailer, separators);
-
-		if (separator_pos < 1)
-			continue; /* not a real trailer */
-
+	char *line;
+	int separator_pos;
+	if (iter->internal.cur < iter->internal.info.trailer_nr) {
+		line = iter->internal.info.trailers[iter->internal.cur++];
+		separator_pos = find_separator(line, separators);
+		iter->is_trailer = (separator_pos > 0);
+
+		iter->raw = line;
 		strbuf_reset(&iter->key);
 		strbuf_reset(&iter->val);
 		parse_trailer(&iter->key, &iter->val, NULL,
-			      trailer, separator_pos);
+			      line, separator_pos);
 		unfold_value(&iter->val);
 		return 1;
 	}

index c309b01323d..a47f18b1525 100644
@@ -127,6 +127,19 @@ struct trailer_iterator {
 	struct strbuf key;
 	struct strbuf val;
 
+	/*
+	 * 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).
+	 */
+	const char *raw;
+
+	/*
+	 * 1 if the raw line was parsed as a trailer line (key/val pair).
+	 */
+	int is_trailer;
+
 	/* private */
 	struct {
 		struct trailer_info info;

gitgitgadget

From: Linus Arver <[email protected]>

In 13211ae23f (trailer: separate public from internal portion of trailer_iterator, 2023-09-09) we moved trailer_info behind an anonymous struct to discourage use by trailer.h API users. However it still left open the possibility of external use of trailer_info itself. Now that there are no external users of trailer_info, we can make this struct private.

Make this struct private by putting its definition inside trailer.c. This has two benefits:

(1) it makes the surface area of the public facing interface (trailer.h) smaller, and

(2) external API users are unable to peer inside this struct (because it is only ever exposed as an opaque pointer).

This change exposes some deficiencies in the API, mainly with regard to information about the location of the trailer block that was parsed. Expose new API functions to access this information (needed by builtin/interpret-trailers.c).

The idea in this patch to hide implementation details behind an "opaque pointer" is also known as the "pimpl" (pointer to implementation) idiom in C++ and is a common pattern in that language (where, for example, abstract classes only have pointers to concrete classes).

However, the original inspiration to use this idiom does not come from C++, but instead the book "C Interfaces and Implementations: Techniques for Creating Reusable Software" [1]. This book recommends opaque pointers as a good design principle for designing C libraries, using the term "interface" as the functions defined in *.h (header) files and "implementation" as the corresponding *.c file which define the interfaces.

The book says this about opaque pointers:

... clients can manipulate such pointers freely, but they can’t dereference them; that is, they can’t look at the innards of the structure pointed to by them. Only the implementation has that privilege. Opaque pointers hide representation details and help catch errors.

In our case, "struct trailer_info" is now hidden from clients, and the ways in which this opaque pointer can be used is limited to the richness of the trailer.h file. In other words, trailer.h exclusively controls exactly how "trailer_info" pointers are to be used.

[1] Hanson, David R. "C Interfaces and Implementations: Techniques for Creating Reusable Software". Addison Wesley, 1997. p. 22

---
 builtin/interpret-trailers.c |  13 +--
 trailer.c                    | 154 +++++++++++++++++++++++------------
 trailer.h                    |  37 ++-------
 3 files changed, 117 insertions(+), 87 deletions(-)

index 5352ee65bd1..9e6ed6b65e2 100644
@@ -141,7 +141,7 @@ static void interpret_trailers(const struct process_trailer_options *opts,
 	LIST_HEAD(head);
 	struct strbuf sb = STRBUF_INIT;
 	struct strbuf trailer_block = STRBUF_INIT;
-	struct trailer_info info;
+	struct trailer_info *info;
 	FILE *outfile = stdout;
 
 	trailer_config_init();

@@ -151,13 +151,13 @@ static void interpret_trailers(const struct process_trailer_options *opts,
 	if (opts->in_place)
 		outfile = create_in_place_tempfile(file);
 
-	parse_trailers(opts, &info, sb.buf, &head);
+	info = parse_trailers(opts, sb.buf, &head);
 
 	/* Print the lines before the trailers */
 	if (!opts->only_trailers)
-		fwrite(sb.buf, 1, info.trailer_block_start, outfile);
+		fwrite(sb.buf, 1, trailer_block_start(info), outfile);
 
-	if (!opts->only_trailers && !info.blank_line_before_trailer)
+	if (!opts->only_trailers && !blank_line_before_trailer_block(info))
 		fprintf(outfile, "\n");
 
 

@@ -176,11 +176,12 @@ static void interpret_trailers(const struct process_trailer_options *opts,
 	strbuf_release(&trailer_block);
 
 	free_trailers(&head);
-	trailer_info_release(&info);
 
 	/* Print the lines after the trailers as is */
 	if (!opts->only_trailers)
-		fwrite(sb.buf + info.trailer_block_end, 1, sb.len - info.trailer_block_end, outfile);
+		fwrite(sb.buf + trailer_block_end(info), 1, sb.len - trailer_block_end(info), outfile);
+
+	trailer_info_release(info);
 
 	if (opts->in_place)
 		if (rename_tempfile(&trailers_tempfile, file))

index a47f18b1525..fff16a29a55 100644
@@ -4,6 +4,8 @@
 #include "list.h"
 #include "strbuf.h"
 
+struct trailer_info;
+
 enum trailer_where {
 	WHERE_DEFAULT,
 	WHERE_END,

@@ -29,27 +31,6 @@ int trailer_set_where(enum trailer_where *item, const char *value);
 int trailer_set_if_exists(enum trailer_if_exists *item, const char *value);
 int trailer_set_if_missing(enum trailer_if_missing *item, const char *value);
 
-struct trailer_info {
-	/*
-	 * True if there is a blank line before the location pointed to by
-	 * trailer_block_start.
-	 */
-	int blank_line_before_trailer;
-
-	/*
-	 * Offsets to the trailer block start and end positions in the input
-	 * string. If no trailer block is found, these are both set to the
-	 * "true" end of the input (find_end_of_log_message()).
-	 */
-	size_t trailer_block_start, trailer_block_end;
-
-	/*
-	 * Array of trailers found.
-	 */
-	char **trailers;
-	size_t trailer_nr;
-};
-
 /*
  * A list that represents newly-added trailers, such as those provided
  * with the --trailer command line option of git-interpret-trailers.

@@ -89,13 +70,13 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head,
 void process_trailers_lists(struct list_head *head,
 			    struct list_head *arg_head);
 
-void parse_trailers(const struct process_trailer_options *opts,
-		    struct trailer_info *info,
-		    const char *str,
-		    struct list_head *head);
+struct trailer_info *parse_trailers(const struct process_trailer_options *opts,
+				    const char *str,
+				    struct list_head *head);
 
-void trailer_info_get(struct trailer_info *info, const char *str,
-		      const struct process_trailer_options *opts);
+size_t trailer_block_start(struct trailer_info *info);
+size_t trailer_block_end(struct trailer_info *info);
+int blank_line_before_trailer_block(struct trailer_info *info);
 
 void trailer_info_release(struct trailer_info *info);