Enrich Trailer API

From: Linus Arver <[email protected]>

Previously the iterator did not iterate over non-trailer lines. This was somewhat unfortunate, because trailer blocks could have non-trailer lines in them since 146245063e (trailer: allow non-trailers in trailer block, 2016-10-21), which was before the iterator was created in f0939a0eb1 (trailer: add interface for iterating over commit trailers, 2020-09-27).

So if trailer API users wanted to iterate over all lines in a trailer block (including non-trailer lines), they could not use the iterator and were forced to use the lower-level trailer_info struct directly (which provides a raw string array that includes all lines in the trailer block).

Change the iterator's behavior so that we also iterate over non-trailer lines, instead of skipping over them. The new "raw" member of the iterator allows API users to access previously inaccessible non-trailer lines. Reword the variable "trailer" to just "line" because this variable can now hold both trailer lines _and_ non-trailer lines.

The new "raw" member is important because anyone currently not using the iterator is using trailer_info's raw string array directly to access lines to check what the combined key + value looks like. If we didn't provide a "raw" member here, iterator users would have to re-construct the unparsed line by concatenating the key and value back together again --- which places an undue burden for iterator users.

The next patch demonstrates the use of the iterator in sequencer.c as an example of where "raw" will be useful, so that it can start using the iterator.

For the existing use of the iterator in builtin/shortlog.c, we don't have to change the code there because that code does

    trailer_iterator_init(&iter, body);
    while (trailer_iterator_advance(&iter)) {
        const char *value = iter.val.buf;

        if (!string_list_has_string(&log->trailers, iter.key.buf))
            continue;

        ...

and the

        if (!string_list_has_string(&log->trailers, iter.key.buf))

condition already skips over non-trailer lines (iter.key.buf is empty for non-trailer lines, making the comparison still work unmodified even with this patch).

---
 trailer.c | 12 +++++-------
 trailer.h |  8 ++++++++
 2 files changed, 13 insertions(+), 7 deletions(-)

index 4f3318802d1..2cc4a910411 100644
@@ -1147,17 +1147,15 @@ 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 */
+	if (iter->internal.cur < iter->internal.info.trailer_nr) {
+		char *line = iter->internal.info.trailers[iter->internal.cur++];
+		int separator_pos = find_separator(line, separators);
 
+		iter->raw = line;
 		strbuf_reset(&iter->key);
 		strbuf_reset(&iter->val);
 		parse_trailer(&iter->key, &iter->val, NULL,
-			      trailer, separator_pos);
+			      line, separator_pos);
 		/* Always unfold values during iteration. */
 		unfold_value(&iter->val);
 		return 1;

index 9f42aa75994..ebafa3657e4 100644
@@ -125,6 +125,14 @@ void format_trailers_from_commit(const struct process_trailer_options *,
  *   trailer_iterator_release(&iter);
  */
 struct trailer_iterator {
+	/*
+	 * Raw line (e.g., "foo: bar baz") before being parsed as a trailer
+	 * key/val pair as part of a trailer block. A trailer block can be
+	 * either 100% trailer lines, or mixed in with non-trailer lines (in
+	 * which case at least 25% must be trailer lines).
+	 */
+	const char *raw;
+
 	struct strbuf key;
 	struct strbuf val;
 

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).

Note how we have to use "iter.raw" in order to get the same behavior as before when we iterated over the unparsed string array (trailers[]) in trailer_info.

---
 sequencer.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

index 8e199fc8a47..35462a6a9d9 100644
@@ -319,35 +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;
+	struct trailer_iterator iter;
+	size_t i = 0;
 	int found_sob = 0, found_sob_last = 0;
 	char saved_char;
 
-	opts.no_divider = 1;
-
 	if (ignore_footer) {
 		saved_char = sb->buf[sb->len - ignore_footer];
 		sb->buf[sb->len - ignore_footer] = '\0';
 	}
 
-	trailer_info_get(&opts, sb->buf, &info);
+	trailer_iterator_init(&iter, sb->buf);
 
 	if (ignore_footer)
 		sb->buf[sb->len - ignore_footer] = saved_char;
 
-	if (info.trailer_block_start == info.trailer_block_end)
-		return 0;
+	while (trailer_iterator_advance(&iter)) {
+		i++;
+		if (sob && !strncmp(iter.raw, sob->buf, sob->len))
+			found_sob = i;
+	}
+	trailer_iterator_release(&iter);
 
-	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;
-		}
+	if (!i)
+		return 0;
 
-	trailer_info_release(&info);
+	found_sob_last = (int)i == found_sob;
 
 	if (found_sob_last)
 		return 3;

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).

There are a couple disadvantages:

(A) every time the member of the struct is accessed an extra pointer dereference must be done, and

(B) for users of trailer_info outside trailer.c, this struct can no longer be allocated on the stack and may only be allocated on the heap (because its definition is hidden away in trailer.c) and appropriately deallocated by the user.

This patch believes that the benefits outweight the advantages for designing APIs, as explained below.

Making trailer_info private exposes existing deficiencies in the API. This is because users of this struct had full access to its internals, so there wasn't much need to actually design it to be "complete" in the sense that API users only needed to use what was provided by the API. For example, the location of the trailer block (start/end offsets relative to the start of the input text) was accessible by looking at these struct members directly. Now that the struct is private, we have to expose new API functions to allow clients to access this information (see 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 <trailer.h>. 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 | 12 +++---
 trailer.c                    | 81 ++++++++++++++++++++++++++++--------
 trailer.h                    | 40 +++++-------------
 3 files changed, 80 insertions(+), 53 deletions(-)

index 11f4ce9e4a2..6bf8cec005a 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");
 
 

@@ -178,8 +178,8 @@ static void interpret_trailers(const struct process_trailer_options *opts,
 
 	/* 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);
-	trailer_info_release(&info);
+		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 ebafa3657e4..a63e97a2663 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,14 +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);
 
-void parse_trailers(const struct process_trailer_options *,
-		    struct trailer_info *,
-		    const char *str,
-		    struct list_head *head);
+struct trailer_info *parse_trailers(const struct process_trailer_options *,
+				    const char *str,
+				    struct list_head *head);
+struct trailer_info *trailer_info_get(const struct process_trailer_options *,
+				      const char *str);
 
-void trailer_info_get(const struct process_trailer_options *,
-		      const char *str,
-		      struct trailer_info *);
+size_t trailer_block_start(struct trailer_info *);
+size_t trailer_block_end(struct trailer_info *);
+int blank_line_before_trailer_block(struct trailer_info *);
 
 void trailer_info_release(struct trailer_info *info);
 

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

gitgitgadget

"outweigh the disadvantages"?

Will update, thanks.

Ack, will do.

Thanks, I will try to use this breakdown in the next reroll.

From: Linus Arver <[email protected]>

Make it "static" to be file-scoped to trailer.c, because no one outside of trailer.c uses it. Remove its declaration from <trailer.h>.

We have to also reposition it to be above parse_trailers(), which depends on it.

---
 trailer.c | 92 +++++++++++++++++++++++++++----------------------------
 trailer.h |  2 --
 2 files changed, 46 insertions(+), 48 deletions(-)

index cc211dfeeae..36e49ab7cf5 100644
@@ -980,6 +980,52 @@ static struct trailer_info *trailer_info_new(void)
 	return info;
 }
 
+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;
+	struct strbuf **trailer_lines, **ptr;
+	char **trailer_strings = NULL;
+	size_t nr = 0, alloc = 0;
+	char **last = NULL;
+
+	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);
+
+	trailer_lines = strbuf_split_buf(str + trailer_block_start,
+					 end_of_log_message - trailer_block_start,
+					 '\n',
+					 0);
+	for (ptr = trailer_lines; *ptr; ptr++) {
+		if (last && isspace((*ptr)->buf[0])) {
+			struct strbuf sb = STRBUF_INIT;
+			strbuf_attach(&sb, *last, strlen(*last), strlen(*last));
+			strbuf_addbuf(&sb, *ptr);
+			*last = strbuf_detach(&sb, NULL);
+			continue;
+		}
+		ALLOC_GROW(trailer_strings, nr + 1, alloc);
+		trailer_strings[nr] = strbuf_detach(*ptr, NULL);
+		last = find_separator(trailer_strings[nr], separators) >= 1
+			? &trailer_strings[nr]
+			: NULL;
+		nr++;
+	}
+	strbuf_list_free(trailer_lines);
+
+	info->blank_line_before_trailer = ends_with_blank_line(str,
+							       trailer_block_start);
+	info->trailer_block_start = trailer_block_start;
+	info->trailer_block_end = end_of_log_message;
+	info->trailers = trailer_strings;
+	info->trailer_nr = nr;
+
+	return info;
+}
+
 /*
  * Parse trailers in "str", populating the trailer info and "head"
  * linked list structure.

@@ -1045,52 +1091,6 @@ int blank_line_before_trailer_block(struct trailer_info *info)
 	return info->blank_line_before_trailer;
 }
 
-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;
-	struct strbuf **trailer_lines, **ptr;
-	char **trailer_strings = NULL;
-	size_t nr = 0, alloc = 0;
-	char **last = NULL;
-
-	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);
-
-	trailer_lines = strbuf_split_buf(str + trailer_block_start,
-					 end_of_log_message - trailer_block_start,
-					 '\n',
-					 0);
-	for (ptr = trailer_lines; *ptr; ptr++) {
-		if (last && isspace((*ptr)->buf[0])) {
-			struct strbuf sb = STRBUF_INIT;
-			strbuf_attach(&sb, *last, strlen(*last), strlen(*last));
-			strbuf_addbuf(&sb, *ptr);
-			*last = strbuf_detach(&sb, NULL);
-			continue;
-		}
-		ALLOC_GROW(trailer_strings, nr + 1, alloc);
-		trailer_strings[nr] = strbuf_detach(*ptr, NULL);
-		last = find_separator(trailer_strings[nr], separators) >= 1
-			? &trailer_strings[nr]
-			: NULL;
-		nr++;
-	}
-	strbuf_list_free(trailer_lines);
-
-	info->blank_line_before_trailer = ends_with_blank_line(str,
-							       trailer_block_start);
-	info->trailer_block_start = trailer_block_start;
-	info->trailer_block_end = end_of_log_message;
-	info->trailers = trailer_strings;
-	info->trailer_nr = nr;
-
-	return info;
-}
-
 void trailer_info_release(struct trailer_info *info)
 {
 	size_t i;

index a63e97a2663..1b7422fa2b0 100644
@@ -73,8 +73,6 @@ void process_trailers_lists(struct list_head *head,
 struct trailer_info *parse_trailers(const struct process_trailer_options *,
 				    const char *str,
 				    struct list_head *head);
-struct trailer_info *trailer_info_get(const struct process_trailer_options *,
-				      const char *str);
 
 size_t trailer_block_start(struct trailer_info *);
 size_t trailer_block_end(struct trailer_info *);

gitgitgadget

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 | 25 +++++-----
 trailer.c                    | 97 ++++++++++++++++++------------------
 trailer.h                    | 18 +++----
 3 files changed, 71 insertions(+), 69 deletions(-)

index 6bf8cec005a..f76841c5280 100644
@@ -140,8 +140,8 @@ 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 strbuf tb = STRBUF_INIT;
+	struct trailer_block *trailer_block;
 	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);
 
-	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 */
 	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,15 +171,16 @@ static void interpret_trailers(const struct process_trailer_options *opts,
 	}
 
 	/* Print trailer block. */
-	format_trailers(opts, &head, &trailer_block);
+	format_trailers(opts, &head, &tb);
 	free_trailers(&head);
-	fwrite(trailer_block.buf, 1, trailer_block.len, outfile);
-	strbuf_release(&trailer_block);
+	fwrite(tb.buf, 1, tb.len, outfile);
+	strbuf_release(&tb);
 
-	/* 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 1b7422fa2b0..76e6d941a07 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 struct process_trailer_options *,
-				    const char *str,
-				    struct list_head *head);
+struct trailer_block *parse_trailers(const struct process_trailer_options *,
+				     const char *str,
+				     struct list_head *head);
 
-size_t trailer_block_start(struct trailer_info *);
-size_t trailer_block_end(struct trailer_info *);
-int blank_line_before_trailer_block(struct trailer_info *);
+size_t trailer_block_start(struct trailer_block *);
+size_t trailer_block_end(struct trailer_block *);
+int blank_line_before_trailer_block(struct trailer_block *);
 
-void trailer_info_release(struct trailer_info *info);
+void trailer_block_release(struct trailer_block *);
 
 void trailer_config_init(void);
 void format_trailers(const struct process_trailer_options *,

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

gitgitgadget

I confess I did not like the name "tb" either. It does have "symmetry in naming with 'sb' above" going for it, but that symmetry is only on the surface because the "b" in "sb" stands for "buf" (buffer), not "block" as is the case for "tb".

Let me try to clean up the naming scheme a bit around here.

Will do, thanks.

From: Linus Arver <[email protected]>

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

parse_trailers_from_command_line_args()

from the trailers implementation, because it should not be concerned with command line arguments (as they have nothing to do with trailers themselves). Indeed, the interpret-trailers builtin is the only caller of this function inside Git.

Rename add_arg_item() to trailer_add_arg_item() to expose it as an API function. Rename new_trailers_clear() to free_new_trailers() because it will be promoted into an API function; the API already has free_trailers(), so using the "free_*" naming style will keep it consistent. 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".

---
 builtin/interpret-trailers.c |  6 +--
 trailer.c                    | 86 ++++++++++++++++++------------------
 trailer.h                    | 13 ++++++
 3 files changed, 58 insertions(+), 47 deletions(-)

index f76841c5280..f674b5f4b9e 100644
@@ -45,7 +45,7 @@ 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)
+static void free_new_trailers(struct list_head *trailers)
 {
 	struct list_head *pos, *tmp;
 	struct new_trailer_item *item;

@@ -64,7 +64,7 @@ static int option_parse_trailer(const struct option *opt,
 	struct new_trailer_item *item;
 
 	if (unset) {
-		new_trailers_clear(trailers);
+		free_new_trailers(trailers);
 		return 0;
 	}
 

@@ -237,7 +237,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 		interpret_trailers(&opts, &trailers, NULL);
 	}
 
-	new_trailers_clear(&trailers);
+	free_new_trailers(&trailers);
 
 	return 0;
 }

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

@@ -45,6 +46,12 @@ struct new_trailer_item {
 	enum trailer_if_missing if_missing;
 };
 
+void duplicate_trailer_conf(struct trailer_conf *dst,
+			    const struct trailer_conf *src);
+void trailer_add_arg_item(struct list_head *arg_head, char *tok, char *val,
+			  const struct trailer_conf *conf,
+			  const struct new_trailer_item *new_trailer_item);
+
 struct process_trailer_options {
 	int in_place;
 	int trim_empty;

@@ -70,6 +77,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 **);
+
 struct trailer_block *parse_trailers(const struct process_trailer_options *,
 				     const char *str,
 				     struct list_head *head);

gitgitgadget

That's a lot done in a single patch. I think splitting it into 3 or more patches would be nice for reviewers.

Thank you for pointing this out (and, for everyone else reading, please don't hesitate to ask for this from me); will do in the next reroll.

From: Linus Arver <[email protected]>

This is a preparatory refactor for deprecating "new_trailer_item" from the API (which will let us deprecate parse_trailers_from_command_line_args()).

Expose new helper functions from the API, because we'll be calling them from interpret-trailers.c soon when we move parse_trailers_from_command_line_args() there.

Move free_new_trailers() from the builtin to trailer.c because later on we will adjust it to free arg_item structs, which are private to trailer.c.

---
 builtin/interpret-trailers.c | 12 ---------
 trailer.c                    | 49 ++++++++++++++++++++++++++++++++++++
 trailer.h                    |  8 ++++++
 3 files changed, 57 insertions(+), 12 deletions(-)

index f674b5f4b9e..9169c320921 100644
@@ -45,18 +45,6 @@ static int option_parse_if_missing(const struct option *opt,
 	return trailer_set_if_missing(opt->value, arg);
 }
 
-static void free_new_trailers(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 int option_parse_trailer(const struct option *opt,
 				   const char *arg, int unset)
 {

index f80f8f7e63f..a2569c10451 100644
@@ -46,8 +46,14 @@ struct new_trailer_item {
 	enum trailer_if_missing if_missing;
 };
 
+void trailer_set_conf_where(enum trailer_where, struct trailer_conf *);
+void trailer_set_conf_if_exists(enum trailer_if_exists, struct trailer_conf *);
+void trailer_set_conf_if_missing(enum trailer_if_missing, struct trailer_conf *);
+
+struct trailer_conf *new_trailer_conf(void);
 void duplicate_trailer_conf(struct trailer_conf *dst,
 			    const struct trailer_conf *src);
+const char *trailer_default_separators(void);
 void trailer_add_arg_item(struct list_head *arg_head, char *tok, char *val,
 			  const struct trailer_conf *conf,
 			  const struct new_trailer_item *new_trailer_item);

@@ -98,6 +104,8 @@ void format_trailers(const struct process_trailer_options *,
 		     struct list_head *trailers,
 		     struct strbuf *out);
 void free_trailers(struct list_head *);
+void free_new_trailers(struct list_head *);
+void free_trailer_conf(struct trailer_conf *);
 
 /*
  * Convenience function to format the trailers from the commit msg "msg" into

gitgitgadget

This patch seems to be also doing too much.

I assume you mean that you'd like for the movement of free_new_trailers() to be separated into its own patch, separate from the introduction of new helper functions. I agree.

Will update.

From: Linus Arver <[email protected]>

This is a preparatory refactor for deprecating "new_trailer_item" from the API.

Instead of preserving the where/if_exists/if_missing information inside the new_trailer_item struct in parse_trailers_from_command_line_args() (only to look inside it again later on in trailer_add_arg_item()), pass this information directly as a trailer_conf which trailer_add_arg_item() already knows how to handle. This reduces the number of parameters we have to pass to trailer_add_arg_item() without any behavioral change.

In the next patch we'll be able to delete "new_trailer_item" altogether.

---
 trailer.c | 29 +++++++++++++++++------------
 trailer.h |  3 +--
 2 files changed, 18 insertions(+), 14 deletions(-)

index 9b8cb94c021..6ab5cf7e5d7 100644
@@ -729,21 +729,12 @@ static struct trailer_item *add_trailer_item(struct list_head *head, char *tok,
 }
 
 void trailer_add_arg_item(struct list_head *arg_head, char *tok, char *val,
-			  const struct trailer_conf *conf,
-			  const struct new_trailer_item *new_trailer_item)
+			  const struct trailer_conf *conf)
 {
 	struct arg_item *new_item = xcalloc(1, sizeof(*new_item));
 	new_item->token = tok;
 	new_item->value = val;
 	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;
-		if (new_trailer_item->if_exists != EXISTS_DEFAULT)
-			new_item->conf.if_exists = new_trailer_item->if_exists;
-		if (new_trailer_item->if_missing != MISSING_DEFAULT)
-			new_item->conf.if_missing = new_trailer_item->if_missing;
-	}
 	list_add_tail(&new_item->list, arg_head);
 }
 

@@ -759,7 +750,7 @@ void parse_trailers_from_config(struct list_head *config_head)
 			trailer_add_arg_item(config_head,
 					     xstrdup(token_from_item(item, NULL)),
 					     xstrdup(""),
-					     &item->conf, NULL);
+					     &item->conf);
 	}
 }
 

@@ -791,11 +782,25 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head,
 			      (int) sb.len, sb.buf);
 			strbuf_release(&sb);
 		} else {
+			struct trailer_conf *conf_current = new_trailer_conf();
 			parse_trailer(tr->text, separator_pos, &tok, &val, &conf);
+			duplicate_trailer_conf(conf_current, conf);
+
+			/*
+			 * Override conf_current with settings specified via CLI flags.
+			 */
+			if (tr->where != WHERE_DEFAULT)
+				trailer_set_conf_where(tr->where, conf_current);
+			if (tr->if_exists != EXISTS_DEFAULT)
+				trailer_set_conf_if_exists(tr->if_exists, conf_current);
+			if (tr->if_missing != MISSING_DEFAULT)
+				trailer_set_conf_if_missing(tr->if_missing, conf_current);
+
 			trailer_add_arg_item(arg_head,
 					     strbuf_detach(&tok, NULL),
 					     strbuf_detach(&val, NULL),
-					     conf, tr);
+					     conf_current);
+			free_trailer_conf(conf_current);
 		}
 	}
 

index a2569c10451..32fc93beb33 100644
@@ -55,8 +55,7 @@ void duplicate_trailer_conf(struct trailer_conf *dst,
 			    const struct trailer_conf *src);
 const char *trailer_default_separators(void);
 void trailer_add_arg_item(struct list_head *arg_head, char *tok, char *val,
-			  const struct trailer_conf *conf,
-			  const struct new_trailer_item *new_trailer_item);
+			  const struct trailer_conf *conf);
 
 struct process_trailer_options {
 	int in_place;

gitgitgadget

From: Linus Arver <[email protected]>

Previously, the "new_trailer_item" struct served only one purpose --- to capture the unparsed raw string <RAW> in "--trailer <RAW>", as well as the current state of the "where", "if_exists", and "if_missing" global variables at the time that the "--trailer <RAW>" CLI argument was encountered.

In addition, the previous CLI argument handling behavior was to capture the <RAW> string in all "--trailer <RAW>" arguments and to collect them (via option_parse_trailer()) into the "new_trailer_head" list. We would then iterate over this list again in parse_trailers_from_command_line_args() and convert these "new_trailer_item" objects into "arg_item" objects.

Skip this intermediate storage of "new_trailer_item" objects in favor of just storing "arg_item" objects. Remove the looping behavior of parse_trailers_from_command_line_args() so that it parses a single "--trailer ..." argument at a time. Rename it to parse_trailer_from_command_line_arg() to reflect this new behavior of only looking at one string (not multiple strings) at a time. Make option_parse_trailer() call parse_trailer_from_command_line_arg() so that the CLI arguments it sees are parsed immediately without the need for intermediate storage.

Delete "new_trailer_item", because we don't need it any more.

In the next patch we will retire parse_trailer_from_command_line_arg() as well, combining it with option_parse_trailer().

---
 builtin/interpret-trailers.c | 29 +++++--------
 trailer.c                    | 81 +++++++++++++++++-------------------
 trailer.h                    | 21 +++-------
 3 files changed, 54 insertions(+), 77 deletions(-)

index 9169c320921..943be5b360e 100644
@@ -49,7 +49,6 @@ 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;
 
 	if (unset) {
 		free_new_trailers(trailers);

@@ -59,12 +58,8 @@ 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);
+	parse_trailer_from_command_line_arg(arg, where, if_exists, if_missing, trailers);
+
 	return 0;
 }
 

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

@@ -148,15 +141,8 @@ static void interpret_trailers(const struct process_trailer_options *opts,
 	if (!opts->only_trailers && !blank_line_before_trailer_block(trailer_block))
 		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);
-	}
+	if (!opts->only_input)
+		process_trailers_lists(&head, new_trailer_head);
 
 	/* Print trailer block. */
 	format_trailers(opts, &head, &tb);

@@ -180,6 +166,7 @@ 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;
+	LIST_HEAD(configured_trailers);
 	LIST_HEAD(trailers);
 
 	struct option options[] = {

@@ -205,6 +192,10 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 	};
 
 	git_config(git_default_config, NULL);
+	trailer_config_init();
+
+	if (!opts.only_input)
+		parse_trailers_from_config(&configured_trailers);
 
 	argc = parse_options(argc, argv, prefix, options,
 			     git_interpret_trailers_usage, 0);

@@ -215,6 +206,8 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 			git_interpret_trailers_usage,
 			options);
 
+	list_splice(&configured_trailers, &trailers);
+
 	if (argc) {
 		int i;
 		for (i = 0; i < argc; i++)

index 32fc93beb33..2848a0d086c 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_set_conf_where(enum trailer_where, struct trailer_conf *);
 void trailer_set_conf_if_exists(enum trailer_if_exists, struct trailer_conf *);
 void trailer_set_conf_if_missing(enum trailer_if_missing, struct trailer_conf *);

@@ -76,8 +62,11 @@ 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 parse_trailer_from_command_line_arg(const char *line,
+					 enum trailer_where where,
+					 enum trailer_if_exists if_exists,
+					 enum trailer_if_missing if_missing,
+					 struct list_head *arg_head);
 
 void process_trailers_lists(struct list_head *head,
 			    struct list_head *arg_head);

gitgitgadget

From: Linus Arver <[email protected]>

Move the logic of parse_trailer_from_command_line_arg() into option_parse_trailer(), because that is the only caller and there's no benefit in keeping these two separate.

---
 builtin/interpret-trailers.c | 42 +++++++++++++++++++++++++++++-
 trailer.c                    | 50 ------------------------------------
 trailer.h                    |  6 -----
 3 files changed, 41 insertions(+), 57 deletions(-)

index 943be5b360e..9657b0d067c 100644
@@ -49,6 +49,11 @@ static int option_parse_trailer(const struct option *opt,
 				   const char *arg, int unset)
 {
 	struct list_head *trailers = opt->value;
+	struct strbuf tok = STRBUF_INIT;
+	struct strbuf val = STRBUF_INIT;
+	const struct trailer_conf *conf;
+	ssize_t separator_pos;
+	static char *cl_separators;
 
 	if (unset) {
 		free_new_trailers(trailers);

@@ -58,7 +63,42 @@ static int option_parse_trailer(const struct option *opt,
 	if (!arg)
 		return -1;
 
-	parse_trailer_from_command_line_arg(arg, where, if_exists, if_missing, trailers);
+	/*
+	 * In command-line arguments, '=' is accepted (in addition to the
+	 * separators that are defined).
+	 */
+	cl_separators = xstrfmt("=%s", trailer_default_separators());
+	separator_pos = find_separator(arg, cl_separators);
+	free(cl_separators);
+
+	if (separator_pos == 0) {
+		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);
+	} else {
+		struct trailer_conf *conf_current = new_trailer_conf();
+		parse_trailer(arg, separator_pos, &tok, &val, &conf);
+		duplicate_trailer_conf(conf_current, conf);
+
+		/*
+		 * Override conf_current with settings specified via CLI flags.
+		 */
+		if (where != WHERE_DEFAULT)
+			trailer_set_conf_where(where, conf_current);
+		if (if_exists != EXISTS_DEFAULT)
+			trailer_set_conf_if_exists(if_exists, conf_current);
+		if (if_missing != MISSING_DEFAULT)
+			trailer_set_conf_if_missing(if_missing, conf_current);
+
+		trailer_add_arg_item(trailers,
+				     strbuf_detach(&tok, NULL),
+				     strbuf_detach(&val, NULL),
+				     conf_current);
+		free_trailer_conf(conf_current);
+	}
 
 	return 0;
 }

index 2848a0d086c..af55032625d 100644
@@ -62,12 +62,6 @@ struct process_trailer_options {
 
 void parse_trailers_from_config(struct list_head *config_head);
 
-void parse_trailer_from_command_line_arg(const char *line,
-					 enum trailer_where where,
-					 enum trailer_if_exists if_exists,
-					 enum trailer_if_missing if_missing,
-					 struct list_head *arg_head);
-
 void process_trailers_lists(struct list_head *head,
 			    struct list_head *arg_head);
 

gitgitgadget

Well one benefit could be that 2 small functions might be easier to understand than a big one. So perhaps parse_trailer_from_command_line_arg() could just have been made static?

True.

In this case I don't think keeping these two functions separate would make sense because parse_trailer_from_command_line_arg() is much more heavyweight than option_parse_trailer() (one is just a thin wrapper around the other). And I didn't like the thought of having 2 function names that look very different:

parse_trailer_from_command_line_arg()
option_parse_trailer()

be so closely related in behavior.

And I already have some more patches (not in this series) that refactors this area a bit also, so I wanted to wait until the dust settled down a bit before deciding (esp. when unit tests are added) whether keep this function separate. It's not clear to me yet whether we do want to add unit tests for parse_trailer_from_command_line_arg() (if we do end up "resurrecting it" in this patch or later), so IDK. The main reason is because I think the first set of unit tests should be for the exposed functions in <trailer.h>, not so much the helper functions that only the builtin uses.

From: Linus Arver <[email protected]>

The new trailer_config_set_*() functions which were introduced a few patches ago put the out parameter (the variable being mutated) at the end of the parameter list.

Put the out parameter at the end for these functions for these existing trailer_set_*() functions for consistency. This also avoids confusion for API users because otherwise these two sets of functions look rather similar in <trailer.h> even though they have completely different out parameters.

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

index 9657b0d067c..d0c09d1d73b 100644
@@ -28,21 +28,21 @@ static int option_parse_where(const struct option *opt,
 			      const char *arg, int unset UNUSED)
 {
 	/* unset implies NULL arg, which is handled in our helper */
-	return trailer_set_where(opt->value, arg);
+	return trailer_set_where(arg, opt->value);
 }
 
 static int option_parse_if_exists(const struct option *opt,
 				  const char *arg, int unset UNUSED)
 {
 	/* unset implies NULL arg, which is handled in our helper */
-	return trailer_set_if_exists(opt->value, arg);
+	return trailer_set_if_exists(arg, opt->value);
 }
 
 static int option_parse_if_missing(const struct option *opt,
 				   const char *arg, int unset UNUSED)
 {
 	/* unset implies NULL arg, which is handled in our helper */
-	return trailer_set_if_missing(opt->value, arg);
+	return trailer_set_if_missing(arg, opt->value);
 }
 
 static int option_parse_trailer(const struct option *opt,

index b0b067ab12c..7b0bdfcb27e 100644
@@ -380,7 +380,7 @@ void process_trailers_lists(struct list_head *head,
 	}
 }
 
-int trailer_set_where(enum trailer_where *item, const char *value)
+int trailer_set_where(const char *value, enum trailer_where *item)
 {
 	if (!value)
 		*item = WHERE_DEFAULT;

@@ -397,7 +397,7 @@ int trailer_set_where(enum trailer_where *item, const char *value)
 	return 0;
 }
 
-int trailer_set_if_exists(enum trailer_if_exists *item, const char *value)
+int trailer_set_if_exists(const char *value, enum trailer_if_exists *item)
 {
 	if (!value)
 		*item = EXISTS_DEFAULT;

@@ -416,7 +416,7 @@ int trailer_set_if_exists(enum trailer_if_exists *item, const char *value)
 	return 0;
 }
 
-int trailer_set_if_missing(enum trailer_if_missing *item, const char *value)
+int trailer_set_if_missing(const char *value, enum trailer_if_missing *item)
 {
 	if (!value)
 		*item = MISSING_DEFAULT;

@@ -520,18 +520,18 @@ 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_trailer_conf.where,
-					      value) < 0)
+			if (trailer_set_where(value,
+					      &default_trailer_conf.where) < 0)
 				warning(_("unknown value '%s' for key '%s'"),
 					value, conf_key);
 		} else if (!strcmp(trailer_item, "ifexists")) {
-			if (trailer_set_if_exists(&default_trailer_conf.if_exists,
-						  value) < 0)
+			if (trailer_set_if_exists(value,
+						  &default_trailer_conf.if_exists) < 0)
 				warning(_("unknown value '%s' for key '%s'"),
 					value, conf_key);
 		} else if (!strcmp(trailer_item, "ifmissing")) {
-			if (trailer_set_if_missing(&default_trailer_conf.if_missing,
-						   value) < 0)
+			if (trailer_set_if_missing(value,
+						   &default_trailer_conf.if_missing) < 0)
 				warning(_("unknown value '%s' for key '%s'"),
 					value, conf_key);
 		} else if (!strcmp(trailer_item, "separators")) {

@@ -600,15 +600,15 @@ static int git_trailer_config(const char *conf_key, const char *value,
 		conf->cmd = xstrdup(value);
 		break;
 	case TRAILER_WHERE:
-		if (trailer_set_where(&conf->where, value))
+		if (trailer_set_where(value, &conf->where))
 			warning(_("unknown value '%s' for key '%s'"), value, conf_key);
 		break;
 	case TRAILER_IF_EXISTS:
-		if (trailer_set_if_exists(&conf->if_exists, value))
+		if (trailer_set_if_exists(value, &conf->if_exists))
 			warning(_("unknown value '%s' for key '%s'"), value, conf_key);
 		break;
 	case TRAILER_IF_MISSING:
-		if (trailer_set_if_missing(&conf->if_missing, value))
+		if (trailer_set_if_missing(value, &conf->if_missing))
 			warning(_("unknown value '%s' for key '%s'"), value, conf_key);
 		break;
 	default:

index af55032625d..4193bedbae4 100644
@@ -28,9 +28,9 @@ enum trailer_if_missing {
 	MISSING_DO_NOTHING
 };
 
-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);
+int trailer_set_where(const char *, enum trailer_where *);
+int trailer_set_if_exists(const char *, enum trailer_if_exists *);
+int trailer_set_if_missing(const char *, enum trailer_if_missing *);
 
 void trailer_set_conf_where(enum trailer_where, struct trailer_conf *);
 void trailer_set_conf_if_exists(enum trailer_if_exists, struct trailer_conf *);

gitgitgadget

s/for these functions for these/for the/

That wording is much better; will update.

From: Linus Arver <[email protected]>

The term "arg_item" is ambiguous because we use it to hold data for

(1) trailers specified as command line arguments (in builtin/interpret-trailers.c), and

(2) trailers specified in configuration,

and these are both used to ultimately insert new trailers (based on the contents of arg_item, acting as a kind of template) into some other set of existing trailers (such as those found in a trailer block inside a log message) that have already been parsed.

Rename "arg_item" to "trailer_template". This necessitates further renames to make the functions that act on these templates match the data structures (parameters) they act on:

  - [*] add_arg_to_input_list()      to apply_template_to_trailers()
  - [*] apply_arg_if_exists()        to maybe_add_if_exists()
  - [*] apply_arg_if_missing()       to maybe_add_if_missing()
  -     apply_command()              to run_command_from_template()
  - [*] apply_item_command()         to populate_template_value()
  -     free_arg_item()              to free_template() (non-API function)
  -     free_new_trailers()          to free_trailer_templates() (API function)
  -     get_conf_item()              to get_or_add_template_by()
  -     option_parse_trailer()       to option_parse_trailer_template()
  -     parse_trailers_from_config() to parse_trailer_templates_from_config()
  - [*] process_trailers_lists()     to apply_trailer_templates()
  -     token_from_item()            to token_from_template()
  -     token_matches_item           to token_matches_template
  - [*] trailer_add_arg_item()       to add_trailer_template()
  -     trailer_from_arg()           to trailer_from()
  - [*] check_if_different()         (reorder parameters only)
  - [*] find_same_and_apply_arg()    (reorder parameters only)

Reorder parameters to prefer input parameters toward the beginning and out parameters at the end; these functions have been marked with an asterisk ([*]).

This removes the "arg" terminology (standing for "CLI arguments") from the trailer implementation, which makes sense because trailers themselves have nothing to do with CLI argument handling.

Also note that these renames expose the previously liberal use of "trailer" to mean both trailers we read from the input text (trailer block) and trailer templates that are defined as CLI args or configurations. Some functions implied a single action when they could do two different things, so introduce words like "maybe" and "or" to make their behavior more explicit.

In summary this patch renames and reorders parameters for readability, without any behavioral change. We don't rename find_same_and_apply_arg(), because it will be refactored soon.

For parse_trailers_from_config() (renamed to parse_trailer_templates_from_config()), add a NEEDSWORK discussion about how the deprecated trailer.*.command configuration option is oddly more featureful than trailer.*.cmd (if we were to remove support for trailer.*.command, users would not be able to replicate the behavior with trailer.*.cmd and would lose out on functionality).

---
 builtin/interpret-trailers.c |  50 +++---
 trailer.c                    | 290 +++++++++++++++++++----------------
 trailer.h                    |  12 +-
 3 files changed, 193 insertions(+), 159 deletions(-)

index d0c09d1d73b..d3d2544c5f1 100644
@@ -45,10 +45,14 @@ static int option_parse_if_missing(const struct option *opt,
 	return trailer_set_if_missing(arg, opt->value);
 }
 
-static int option_parse_trailer(const struct option *opt,
-				   const char *arg, int unset)
+/*
+ * Interpret "--trailer ..." as trailer templates (trailers we want to add into
+ * the input text).
+ */
+static int option_parse_trailer_template(const struct option *opt,
+					 const char *arg, int unset)
 {
-	struct list_head *trailers = opt->value;
+	struct list_head *templates = opt->value;
 	struct strbuf tok = STRBUF_INIT;
 	struct strbuf val = STRBUF_INIT;
 	const struct trailer_conf *conf;

@@ -56,7 +60,7 @@ static int option_parse_trailer(const struct option *opt,
 	static char *cl_separators;
 
 	if (unset) {
-		free_new_trailers(trailers);
+		free_trailer_templates(templates);
 		return 0;
 	}
 

@@ -93,10 +97,10 @@ static int option_parse_trailer(const struct option *opt,
 		if (if_missing != MISSING_DEFAULT)
 			trailer_set_conf_if_missing(if_missing, conf_current);
 
-		trailer_add_arg_item(trailers,
-				     strbuf_detach(&tok, NULL),
+		add_trailer_template(strbuf_detach(&tok, NULL),
 				     strbuf_detach(&val, NULL),
-				     conf_current);
+				     conf_current,
+				     templates);
 		free_trailer_conf(conf_current);
 	}
 

@@ -158,10 +162,10 @@ static void read_input_file(struct strbuf *sb, const char *file)
 }
 
 static void interpret_trailers(const struct process_trailer_options *opts,
-			       struct list_head *new_trailer_head,
+			       struct list_head *templates,
 			       const char *file)
 {
-	LIST_HEAD(head);
+	LIST_HEAD(trailers_from_sb);
 	struct strbuf sb = STRBUF_INIT;
 	struct strbuf tb = STRBUF_INIT;
 	struct trailer_block *trailer_block;

@@ -172,7 +176,7 @@ static void interpret_trailers(const struct process_trailer_options *opts,
 	if (opts->in_place)
 		outfile = create_in_place_tempfile(file);
 
-	trailer_block = parse_trailers(opts, sb.buf, &head);
+	trailer_block = parse_trailers(opts, sb.buf, &trailers_from_sb);
 
 	/* Print the lines before the trailer block */
 	if (!opts->only_trailers)

@@ -182,11 +186,11 @@ static void interpret_trailers(const struct process_trailer_options *opts,
 		fprintf(outfile, "\n");
 
 	if (!opts->only_input)
-		process_trailers_lists(&head, new_trailer_head);
+		apply_trailer_templates(templates, &trailers_from_sb);
 
 	/* Print trailer block. */
-	format_trailers(opts, &head, &tb);
-	free_trailers(&head);
+	format_trailers(opts, &trailers_from_sb, &tb);
+	free_trailers(&trailers_from_sb);
 	fwrite(tb.buf, 1, tb.len, outfile);
 	strbuf_release(&tb);
 

@@ -206,8 +210,8 @@ 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;
-	LIST_HEAD(configured_trailers);
-	LIST_HEAD(trailers);
+	LIST_HEAD(configured_templates);
+	LIST_HEAD(templates);
 
 	struct option options[] = {
 		OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),

@@ -226,8 +230,8 @@ 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"),
-				N_("trailer(s) to add"), option_parse_trailer),
+		OPT_CALLBACK(0, "trailer", &templates, N_("trailer"),
+				N_("trailer(s) to add"), option_parse_trailer_template),
 		OPT_END()
 	};
 

@@ -235,30 +239,30 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 	trailer_config_init();
 
 	if (!opts.only_input)
-		parse_trailers_from_config(&configured_trailers);
+		parse_trailer_templates_from_config(&configured_templates);
 
 	argc = parse_options(argc, argv, prefix, options,
 			     git_interpret_trailers_usage, 0);
 
-	if (opts.only_input && !list_empty(&trailers))
+	if (opts.only_input && !list_empty(&templates))
 		usage_msg_opt(
 			_("--trailer with --only-input does not make sense"),
 			git_interpret_trailers_usage,
 			options);
 
-	list_splice(&configured_trailers, &trailers);
+	list_splice(&configured_templates, &templates);
 
 	if (argc) {
 		int i;
 		for (i = 0; i < argc; i++)
-			interpret_trailers(&opts, &trailers, argv[i]);
+			interpret_trailers(&opts, &templates, argv[i]);
 	} else {
 		if (opts.in_place)
 			die(_("no input file given for in-place editing"));
-		interpret_trailers(&opts, &trailers, NULL);
+		interpret_trailers(&opts, &templates, NULL);
 	}
 
-	free_new_trailers(&trailers);
+	free_trailer_templates(&templates);
 
 	return 0;
 }

index 4193bedbae4..2a21d74c263 100644
@@ -40,8 +40,8 @@ struct trailer_conf *new_trailer_conf(void);
 void duplicate_trailer_conf(struct trailer_conf *dst,
 			    const struct trailer_conf *src);
 const char *trailer_default_separators(void);
-void trailer_add_arg_item(struct list_head *arg_head, char *tok, char *val,
-			  const struct trailer_conf *conf);
+void add_trailer_template(char *tok, char *val, const struct trailer_conf *,
+			  struct list_head *templates);
 
 struct process_trailer_options {
 	int in_place;

@@ -60,10 +60,10 @@ struct process_trailer_options {
 
 #define PROCESS_TRAILER_OPTIONS_INIT {0}
 
-void parse_trailers_from_config(struct list_head *config_head);
+void parse_trailer_templates_from_config(struct list_head *config_head);
 
-void process_trailers_lists(struct list_head *head,
-			    struct list_head *arg_head);
+void apply_trailer_templates(struct list_head *templates,
+			     struct list_head *trailers_head);
 
 ssize_t find_separator(const char *line, const char *separators);
 

@@ -86,8 +86,8 @@ void format_trailers(const struct process_trailer_options *,
 		     struct list_head *trailers,
 		     struct strbuf *out);
 void free_trailers(struct list_head *);
-void free_new_trailers(struct list_head *);
 void free_trailer_conf(struct trailer_conf *);
+void free_trailer_templates(struct list_head *);
 
 /*
  * Convenience function to format the trailers from the commit msg "msg" into

gitgitgadget

That's a lot of changes in a single patch.

This change could be in a separate patch. Also there were discussions when trailer.*.command was deprecated and trailer.*.cmd introduced. I think it might be useful to talk about them in the commit message of the separate patch introducing the NEEDSWORK.

I confess I was not happy with the volume of the change either. I suppose I could break things down into renames vs parameter reorderings. Will update.

Will do (will reference the commit that introduced trailer.*.cmd also), thanks.

There are now 28 patches in this series.

I took a look at all of them, and I think that this series should be split into 4 or more series.

For example perhaps one series until the "trailer: move interpret_trailers() to interpret-trailers.c" patch, then another one until "trailer: finish formatting unification". etc.

Also I think it might be possible to avoid some test failures introduced by some patches. If it's not possible, I agree with Junio that it would be nice if the failing tests were changed to use 'test_expect_failure'.

Also it seems to me that many patches towards the end of this series should be split.

[...]

I like the goal of libifying Git the trailer API, and the way you want to do it seems reasonable to me.

[...]

I am a bit sad that this series doesn't introduce unit tests using the new test framework in C yet. I understand that this series is mostly a big refactoring and maybe it's better to introduce unit tests only when the refactoring is finished though. Anyway I hope the next series will introduce such tests.

Thanks for telling us about this use case.

I believe there was a reason why it was done this way. I don't remember what it was though.

It could be a feature to be able to normalize trailers in a certain way.

It could be a feature to be able to normalize this too.

I am not sure this is a bug fix either. It could perhaps be a normalization too.

It's possible that there was a reason why it was done this way.

I think you might want to take a look at the discussions on the mailing list when "interpret-trailers" was developed. There were a lot of discussions over a long time, and there were a lot of requests and suggestions about what it should do.

I presume that [01-09/28] would be the first part, nothing controversial and consisting of obvious clean-ups? I do not mind merging that part down to remove the future review load if everybody agrees.

Thanks.

Yeah, patches [01-09/28] look good to me. If we are merging them without the rest, we might want to change a bit the last sentence in 09/28 which says:

"In the next patch, we'll change format_trailer_info() to use the parsed trailer_item objects instead of the string array."

Maybe just: s/In the next patch/In a future patch/

I was hoping that you'll give us more details of what the other 3 or more you would envision the series to be, actually.

(Sorry for sending this previously to Junio only)

I think the next one could be [10-16/28], so until "trailer: finish formatting unification".

Then I am not sure about the next one, perhaps [17-20/28] or [17-21/28].

The rest would depend on the splitting of the big patches towards the end of the series.

Ack, I'll try to group them like this. Thanks.

This sounds fine to me. IIRC this means for the 2nd, 3rd+ series I have to remember to say "this series builds on top of the other topic branches '...'" in the cover letter. Now that I've written this out I will hopefully not forget to do this...

Or, I suppose I could just introduce the 1st sub-series, wait for that to get queued to next, then (re)introduce the 2nd sub-series, etc, in order. Hmm. I think this will be simpler.

As the breakages are deliberate, I will have to go with using "test_expect_failure".

In hindsight, I fully agree.

Aside: I am delighted with the quality of reviews on this project. It's not something I am used to, so please bear with me while I try to break old habits. Thanks.

This was my original goal as well, and still is.

I will see which API functions are stable enough, and add tests accordingly (in a patch series sooner than later).

Probably the "biggest" (?) thing that is coming from the larger series is the introduction of a complete separation between parsing (without any modification of the input) and formatting. The parser/formatter is a large chunk of the trailer implementation, so I would expect unit tests for those bits to have to wait until those improvements are merged into "next".

Noted. I'll see what I can find in our commit history.

True. But doing such normalization silently is undocumented behavior, and we should provide explicit flags for this sort of thing. Adding such flags might be the right thing to do (let's discuss more when this patch gets proposed). FWIW the behavior I observed is that this normalization only happens for *some* trailers that have configuration options, not all trailers in the input. So it's a special kind of (limited) normalization.

OK, but it would again be undocumented behavior.

See my comment above.

I confess I haven't looked too deeply into the original threads surrounding the introduction of "interpret-trailers". But all of the changes which I categorize as bugfixes above have a theme of undocumented modifications.

While working on this (and the larger) series around trailers, I only looked into some (not all) of the discussions on the mailing list in this area. Instead, I deferred to Documentation/git-interpret-trailers.txt as the official (authoritative) source of truth. This is partly why I first started out on this project last year by making improvements to that doc. And, this is why seeing so many edge cases where we perform such undocumented modifications smelled more like bugs to me than features.

That being said, I cannot disagree with your position that the so-called bugfixes I've reported above could be misguided (and should rather be just updates to missing documentation). Let's not try to decide that here, but instead later when I get to introduce those changes on the list, one at a time. Thanks.

FWIW, which was how the recent flurry of topics related to the reftable backend were done by Patrick, which was quite nice. People certainly need the feel of larger picture to get motivated to review an earlier part of the series, but now that they saw the projected end-game, splitting these into 4 series, each with materials in 7 or so patches in v4, and presenting in not-so-quick succession one by one, would make it less distracting and less daunting.

That's OK. If v4 were 40-patch series instead of 28, I am sure you would have had the unit-test part near the end. So a bite sized series of serieses may not show the unit-tests while the earlier batches are still cooking, but as long as we all are aiming for that same goal, we are fine. Let's help ourselves get there soon by reviewing each other's patches ;-).

Thanks, both.

I feel like I’m learning some things about how version control programs (and accompanying review software (here email)) can be vital—not just helpful, not just a nice-to-have—for structuring reviews of large changes from observing these conversations. Thanks to both/all.

Kristoffer Haugsbakk

Yeah, sure.

Sure no worries.

[...]

Ok.

Perhaps because we consider that having some configuration means that the user consistently expects things in a certain way.

Yeah, it might seem like undocumented features are bad, and I agree that reading original discussions can be tiring. But if the latter makes it possible to fix undocumented features by just properly documenting them, then I think it might just be the best thing to do. Ok not to decide about it now though.

Yes, this was one possibility I considered after sending my reply. If a user has gone out of their way to configure something, maybe they do want things (for those bits) to be normalized.

And adding a flag to disable normalization seems like a good feature to have also (while keeping the behavior of the interpret-trailers that has been relatively untouched since its introduction). But anyway I'm getting a little bit ahead of myself.

Thanks!

This patch series is the first 9 patches of a larger cleanup/bugfix series (henceforth "larger series") I've been working on. The main goal of this series is to begin the process of "libifying" the trailer API. By "API" I mean the interface exposed in trailer.h. The larger series brings a number of additional cleanups (exposing and fixing some bugs along the way), and builds on top of this series.

When the larger series is merged, we will be in a good state to additionally pursue the following goals:

1. "API reuse inside Git": make the API expressive enough to eliminate any need by other parts of Git to use the interpret-trailers builtin as a subprocess (instead they could just use the API directly);
2. "API stability": add unit tests to codify the expected behavior of API functions; and
3. "API documentation": create developer-focused documentation to explain how to use the API effectively, noting any API limitations or anti-patterns.

In the future after libification is "complete", users external to Git will be able to use the same trailer processing API used by the interpret-trailers builtin. For example, a web server may want to parse trailers the same way that Git would parse them, without having to call interpret-trailers as a subprocess. This use case was the original motivation behind my work in this area.

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

In summary this series breaks up "process_trailers()" into smaller pieces, exposing many of the parts relevant to trailer-related processing in trailer.h. This will force us to eventually introduce unit tests for these API functions, but that is a good thing for API stability. We also perform some preparatory refactors in order to help us unify the trailer formatting machinery toward the end of this series.

Notable changes in v5
=====================

* Removed patches 10+ from this series. Thanks to Christian for the suggestion.
* Reworded the log message of patch 09 to reflect the above arrangement, as suggested by Christian.

Notable changes in v4
=====================

* Patches 3, 4, 5, and 8 have been broken up into smaller steps. There are 28 instead of 10 patches now, but these 28 should be much easier to review than the (previously condensed) 10.
* NEW Patch 1: "trailer: free trailer_info after all related usage" fixes awkward use-after-free coding style
* NEW Patch 2: "shortlog: add test for de-duplicating folded trailers" increases test coverage related to trailer iterators and "unfold_value()"
* NEW Patch 27: "trailer_set_*(): put out parameter at the end" is a small refactor to reorder parameters.
* Patches 5-16: These smaller patches make up Patch 3 from v3.
* Patches 17-18: These smaller patches make up Patch 4 from v3.
* Patches 19-20: These smaller patches make up Patch 5 from v3.
* Patches 23-26: These smaller patches make up Patch 8 from v3.
* Anonymize unambiguous parameters in <trailer.h>.

Notable changes in v3
=====================

* Squashed Patch 4 into Patch 3 ("trailer: unify trailer formatting machinery"), to avoid breaking the build ("-Werror=unused-function" violations)
* NEW (Patch 10): Introduce "trailer template" terminology for readability (no behavioral change)
* (API function) Rename default_separators() to trailer_default_separators()
* (API function) Rename new_trailers_clear() to free_trailer_templates()
* trailer.h: for single-parameter functions, anonymize the parameter name to reduce verbosity

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

* (cover letter) Discuss goals of the larger series in more detail, especially the pimpl idiom
* (cover letter) List bug fixes pending in the larger series that depend on this series
* Reorder function parameters to have trailer options at the beginning (and out parameters toward the end)
* "sequencer: use the trailer iterator": prefer C string instead of strbuf for new "raw" field
* Patch 1 (was Patch 2) also renames ensure_configured() to trailer_config_init() (forgot to rename this one previously)

Linus Arver (9):
trailer: free trailer_info _after_ all related usage
shortlog: add test for de-duplicating folded trailers
trailer: prepare to expose functions as part of API
trailer: move interpret_trailers() to interpret-trailers.c
trailer: start preparing for formatting unification trailer_info_get(): reorder parameters format_trailers(): use strbuf instead of FILE format_trailer_info(): move "fast path" to caller format_trailers_from_commit(): indirectly call trailer_info_get()

 builtin/interpret-trailers.c | 101 ++++++++++++++++++-
 pretty.c                     |   2 +-
 ref-filter.c                 |   2 +-
 sequencer.c                  |   2 +-
 t/t4201-shortlog.sh          |  32 +++++++
 trailer.c                    | 181 +++++++++--------------------------
 trailer.h                    |  31 ++++--
 7 files changed, 204 insertions(+), 147 deletions(-)

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

Range-diff vs v4:

  1:  652df25f30e =  1:  652df25f30e trailer: free trailer_info _after_ all related usage
  2:  fdccaca2ba0 =  2:  fdccaca2ba0 shortlog: add test for de-duplicating folded trailers
  3:  4372af244f0 =  3:  4372af244f0 trailer: prepare to expose functions as part of API
  4:  4073b8eb510 =  4:  4073b8eb510 trailer: move interpret_trailers() to interpret-trailers.c
  5:  b2a0f7829a1 =  5:  b2a0f7829a1 trailer: start preparing for formatting unification
  6:  c1760f80356 =  6:  c1760f80356 trailer_info_get(): reorder parameters
  7:  9dc912b5bc5 =  7:  9dc912b5bc5 format_trailers(): use strbuf instead of FILE
  8:  b97c06d8bc3 =  8:  b97c06d8bc3 format_trailer_info(): move "fast path" to caller
  9:  6906910417a !  9:  7c656b3f775 format_trailers_from_commit(): indirectly call trailer_info_get()

@@ Commit message
format_trailer_info() only looks at the "trailers" string array, not the trailer_item objects which parse_trailers() populates.

- In the next patch, we'll change format_trailer_info() to use the parsed
+ In a future patch, we'll change format_trailer_info() to use the parsed
trailer_item objects instead of the string array.

 10:  f5b7ba08aa7 <  -:  ----------- format_trailer_info(): use trailer_item objects
 11:  457f2a839d5 <  -:  ----------- format_trailer_info(): drop redundant unfold_value()
 12:  a72eca301f7 <  -:  ----------- format_trailer_info(): append newline for non-trailer lines
 13:  ad77c33e457 <  -:  ----------- trailer: begin formatting unification
 14:  11f854399db <  -:  ----------- format_trailer_info(): teach it about opts->trim_empty
 15:  ba1f387747b <  -:  ----------- format_trailer_info(): avoid double-printing the separator
 16:  31725832224 <  -:  ----------- trailer: finish formatting unification
 17:  6f17c022b15 <  -:  ----------- trailer: teach iterator about non-trailer lines
 18:  cc92dfb0bda <  -:  ----------- sequencer: use the trailer iterator
 19:  f5f0d06613f <  -:  ----------- trailer: make trailer_info struct private
 20:  607ae7a90cd <  -:  ----------- trailer: retire trailer_info_get() from API
 21:  38f4b4c4135 <  -:  ----------- trailer: spread usage of "trailer_block" language
 22:  94bf182e3ff <  -:  ----------- trailer: prepare to delete "parse_trailers_from_command_line_args()"
 23:  3bfe4809ecb <  -:  ----------- trailer: add new helper functions to API
 24:  80e1958bb8d <  -:  ----------- trailer_add_arg_item(): drop new_trailer_item usage
 25:  a9080597a28 <  -:  ----------- trailer: deprecate "new_trailer_item" struct from API
 26:  9720526dd8a <  -:  ----------- trailer: unify "--trailer ..." arg handling
 27:  26df2514acb <  -:  ----------- trailer_set_*(): put out parameter at the end
 28:  14927038d85 <  -:  ----------- trailer: introduce "template" term for readability

gitgitgadget

From: Linus Arver <[email protected]>

In de7c27a186 (trailer: use offsets for trailer_start/trailer_end, 2023-10-20), we started using trailer block offsets in trailer_info. In particular, we dropped the use of a separate stack variable "size_t trailer_end", in favor of accessing the new "trailer_block_end" member of trailer_info (as "info.trailer_block_end").

At that time, we forgot to also move the

   trailer_info_release(&info);

line to be _after_ this new use of the trailer_info struct. Move it now.

Note that even without this patch, we didn't have leaks or any other problems because trailer_info_release() only frees memory allocated on the heap. The "trailer_block_end" member was allocated on the stack back then (as it is now) so it was still safe to use for all this time.

---
 trailer.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 3a0710a4583..e1d83390b66 100644
@@ -1111,13 +1111,12 @@ void process_trailers(const char *file,
 	}
 
 	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);
+	trailer_info_release(&info);
 
 	if (opts->in_place)
 		if (rename_tempfile(&trailers_tempfile, file))

gitgitgadget

From: Linus Arver <[email protected]>

The shortlog builtin was taught to use the trailer iterator interface in 47beb37bc6 (shortlog: match commit trailers with --group, 2020-09-27). The iterator always unfolds values and this has always been the case since the time the iterator was first introduced in f0939a0eb1 (trailer:
add interface for iterating over commit trailers, 2020-09-27). Add a comment line to remind readers of this behavior.

The fact that the iterator always unfolds values is important (at least for shortlog) because unfolding allows it to recognize both folded and unfolded versions of the same trailer for de-duplication.

Capture the existing behavior in a new test case to guard against regressions in this area. This test case is based off of the existing "shortlog de-duplicates trailers in a single commit" just above it. Now if we were to remove the call to

    unfold_value(&iter->val);

inside the iterator, this new test case will break.

---
 t/t4201-shortlog.sh | 32 ++++++++++++++++++++++++++++++++
 trailer.c           |  1 +
 2 files changed, 33 insertions(+)

index d7382709fc1..f698d0c9ad2 100755
@@ -312,6 +312,38 @@ test_expect_success 'shortlog de-duplicates trailers in a single commit' '
 	test_cmp expect actual
 '
 
+# Trailers that have unfolded (single line) and folded (multiline) values which
+# are otherwise identical are treated as the same trailer for de-duplication.
+test_expect_success 'shortlog de-duplicates trailers in a single commit (folded/unfolded values)' '
+	git commit --allow-empty -F - <<-\EOF &&
+	subject one
+
+	this message has two distinct values, plus a repeat (folded)
+
+	Repeated-trailer: Foo foo foo
+	Repeated-trailer: Bar
+	Repeated-trailer: Foo
+	  foo foo
+	EOF
+
+	git commit --allow-empty -F - <<-\EOF &&
+	subject two
+
+	similar to the previous, but without the second distinct value
+
+	Repeated-trailer: Foo foo foo
+	Repeated-trailer: Foo
+	  foo foo
+	EOF
+
+	cat >expect <<-\EOF &&
+	     2	Foo foo foo
+	     1	Bar
+	EOF
+	git shortlog -ns --group=trailer:repeated-trailer -2 HEAD >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'shortlog can match multiple groups' '
 	git commit --allow-empty -F - <<-\EOF &&
 	subject one

index e1d83390b66..f74915bd8cd 100644
@@ -1270,6 +1270,7 @@ int trailer_iterator_advance(struct trailer_iterator *iter)
 		strbuf_reset(&iter->val);
 		parse_trailer(&iter->key, &iter->val, NULL,
 			      trailer, separator_pos);
+		/* Always unfold values during iteration. */
 		unfold_value(&iter->val);
 		return 1;
 	}

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.

Take the opportunity to start putting trailer processing 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 f74915bd8cd..916175707d8 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,8 +1110,8 @@ void process_trailers(const char *file,
 		process_trailers_lists(&head, &arg_head);
 	}
 
-	print_all(outfile, &head, opts);
-	free_all(&head);
+	format_trailers(opts, &head, outfile);
+	free_trailers(&head);
 
 	/* Print the lines after the trailers as is */
 	if (!opts->only_trailers)

@@ -1134,7 +1134,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

Nit: actually this patch renames process_trailers() to interpret_trailers() so the function that will be moved will be interpret_trailers().

Nit: this patch and the next one will become commits, so perhaps:

s/In the next patch/In a following commit/

Except that "process_trailers()" already contains "trailer" but will still be renamed by this patch to "interpret_trailers()". So I think it might be nice to explain a bit why renaming process_trailers() to interpret_trailers() makes sense too.

Also I think the subject, "trailer: prepare to expose functions as part of API" could be more explicit about what the patch is actually doing, like perhaps "trailer: rename functions to use 'trailer'".

In general, when there is a patch called "prepare to do X", then we might expect a following patch called something like "actually do X". But there isn't any patch in the series named like "trailer: expose functions as part of API".

It's interesting to know that this will be the pattern going forward in the series, but that doesn't quite tell why it's a good idea to do it.

So I think it might be nice to repeat an explanation similar to the one you give in "trailer: start preparing for formatting unification" for format_trailers_from_commit():

"Reorder parameters for format_trailers_from_commit() to prefer

const struct process_trailer_options *opts

as the first parameter, because these options are intimately tied to formatting trailers."

And maybe also say that parameters like FILE *outfile should be last because they are some kind of 'out' parameters.

Oops, fixed locally.

TBH I've always wondered whether "patch" or "commit" matters --- I've seen examples of patch series that referred to "commits" instead of "patches", and vice versa. I was hoping to hear an opinion on this, so I'm happy to see (and apply) your suggestion. Thanks.

I will add something like:

Rename process_trailers() to interpret_trailers(), because it matches the name for the builtin command of the same name (git-interpret-trailers), which is the sole user of process_trailers().

Applied.

Sounds like a very sensible rule. IOW, leave the detailed explanation to the commit message and use the subject line only for the most obvious explanation of what's going on in the patch. +1

SGTM, will do.

Ah nice catch. Will do.

Really appreciate the quality of your reviews, thanks so much! :)

I think it is just fine to use either; sticking to one you pick consistently in the same series would have value. If you prefer commit, then fine. If you like patch, that's fine too.

Makes sense, thanks.

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 (together with a few helper functions called only by it) and remove its external declaration from <trailer.h>.

Several helper functions that are called by interpret_trailers() remain in trailer.c because other callers in the same file still call them. Declare them in <trailer.h> so that interpret_trailers() (now in builtin/interpret-trailers.c) can continue calling them as a trailer API user.

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 |  93 +++++++++++++++++++++++++++
 trailer.c                    | 119 ++++-------------------------------
 trailer.h                    |  20 +++++-
 3 files changed, 123 insertions(+), 109 deletions(-)

index 85a3413baf5..d1cf0aa33a2 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,98 @@ 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);
+
+	/* 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);
+	trailer_info_release(&info);
+
+	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..c292d44b62f 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 *,
+		    struct trailer_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 *,
+		     struct list_head *trailers, FILE *outfile);
+void free_trailers(struct list_head *);
+
 /*
  * Format the trailers from the commit msg "msg" into the strbuf "out".
  * Note two caveats about "opts":

gitgitgadget

From: Linus Arver <[email protected]>

Currently there are two functions for formatting trailers in <trailer.h>:

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

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

and although they are similar enough (even taking the same process_trailer_options struct pointer) they are used quite differently. One might intuitively think that format_trailers_from_commit() builds on top of format_trailers(), but this is not the case. Instead format_trailers_from_commit() calls format_trailer_info() and format_trailers() is never called in that codepath.

This is a preparatory refactor to help us deprecate format_trailers() in favor of format_trailer_info() (at which point we can rename the latter to the former). When the deprecation is complete, both format_trailers_from_commit(), and the interpret-trailers builtin will be able to call into the same helper function (instead of format_trailers() and format_trailer_info(), respectively). Unifying the formatters is desirable because it simplifies the API.

Reorder parameters for format_trailers_from_commit() to prefer

const struct process_trailer_options *opts

as the first parameter, because these options are intimately tied to formatting trailers. And take

struct strbuf *out

last, because it's an "out parameter" (something that the caller wants to use as the output of this function).

---
 pretty.c     |  2 +-
 ref-filter.c |  2 +-
 trailer.c    | 11 ++++++-----
 trailer.h    |  5 +++--
 4 files changed, 11 insertions(+), 9 deletions(-)

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 d23afa0a65c..5025be97899 100644
@@ -1083,10 +1083,10 @@ void trailer_info_release(struct trailer_info *info)
 	free(info->trailers);
 }
 
-static void format_trailer_info(struct strbuf *out,
+static void format_trailer_info(const struct process_trailer_options *opts,
 				const struct trailer_info *info,
 				const char *msg,
-				const struct process_trailer_options *opts)
+				struct strbuf *out)
 {
 	size_t origlen = out->len;
 	size_t i;

@@ -1144,13 +1144,14 @@ 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 struct process_trailer_options *opts,
+				 const char *msg,
+				 struct strbuf *out)
 {
 	struct trailer_info info;
 
 	trailer_info_get(&info, msg, opts);
-	format_trailer_info(out, &info, msg, opts);
+	format_trailer_info(opts, &info, msg, out);
 	trailer_info_release(&info);
 }
 

index c292d44b62f..c6d3ee49bbf 100644
@@ -115,8 +115,9 @@ void free_trailers(struct list_head *);
  *     only the trailer block itself, even if the "only_trailers" option is not
  *     set.
  */
-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

Here also I think the subject could be more specific like for example:

"trailer: reorder format_trailers_from_commit() parameters"

Applied, thanks.

Agreed. Will do.

From: Linus Arver <[email protected]>

This is another preparatory refactor to unify the trailer formatters.

Take

const struct process_trailer_options *opts

as the first parameter, because these options are required for parsing trailers (e.g., whether to treat "---" as the end of the log message). And take

struct trailer_info *info

last, because it's an "out parameter" (something that the caller wants to use as the output of this function).

---
 sequencer.c |  2 +-
 trailer.c   | 11 ++++++-----
 trailer.h   |  5 +++--
 3 files changed, 10 insertions(+), 8 deletions(-)

index 3cc88d8a800..8e199fc8a47 100644
@@ -332,7 +332,7 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
 		sb->buf[sb->len - ignore_footer] = '\0';
 	}
 
-	trailer_info_get(&info, sb->buf, &opts);
+	trailer_info_get(&opts, sb->buf, &info);
 
 	if (ignore_footer)
 		sb->buf[sb->len - ignore_footer] = saved_char;

index 5025be97899..f92d844361a 100644
@@ -997,7 +997,7 @@ void parse_trailers(const struct process_trailer_options *opts,
 	struct strbuf val = STRBUF_INIT;
 	size_t i;
 
-	trailer_info_get(info, str, opts);
+	trailer_info_get(opts, str, info);
 
 	for (i = 0; i < info->trailer_nr; i++) {
 		int separator_pos;

@@ -1032,8 +1032,9 @@ void free_trailers(struct list_head *trailers)
 	}
 }
 
-void trailer_info_get(struct trailer_info *info, const char *str,
-		      const struct process_trailer_options *opts)
+void trailer_info_get(const struct process_trailer_options *opts,
+		      const char *str,
+		      struct trailer_info *info)
 {
 	size_t end_of_log_message = 0, trailer_block_start = 0;
 	struct strbuf **trailer_lines, **ptr;

@@ -1150,7 +1151,7 @@ void format_trailers_from_commit(const struct process_trailer_options *opts,
 {
 	struct trailer_info info;
 
-	trailer_info_get(&info, msg, opts);
+	trailer_info_get(opts, msg, &info);
 	format_trailer_info(opts, &info, msg, out);
 	trailer_info_release(&info);
 }

@@ -1161,7 +1162,7 @@ void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
 	strbuf_init(&iter->key, 0);
 	strbuf_init(&iter->val, 0);
 	opts.no_divider = 1;
-	trailer_info_get(&iter->internal.info, msg, &opts);
+	trailer_info_get(&opts, msg, &iter->internal.info);
 	iter->internal.cur = 0;
 }
 

index c6d3ee49bbf..410c61b62be 100644
@@ -94,8 +94,9 @@ void parse_trailers(const struct process_trailer_options *,
 		    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_get(const struct process_trailer_options *,
+		      const char *str,
+		      struct trailer_info *);
 
 void trailer_info_release(struct trailer_info *info);
 

gitgitgadget

From: Linus Arver <[email protected]>

This is another preparatory refactor to unify the trailer formatters.

Make format_trailers() also write to a strbuf, to align with format_trailers_from_commit() which also does the same. Doing this makes format_trailers() behave similar to format_trailer_info() (which will soon help us replace one with the other).

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

index d1cf0aa33a2..11f4ce9e4a2 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,8 +170,11 @@ 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);
 	free_trailers(&head);
+	fwrite(trailer_block.buf, 1, trailer_block.len, outfile);
+	strbuf_release(&trailer_block);
 
 	/* Print the lines after the trailers as is */
 	if (!opts->only_trailers)

index f92d844361a..cbd643cd1fe 100644
@@ -144,12 +144,12 @@ static char last_non_space_char(const char *s)
 	return '\0';
 }
 
-static void print_tok_val(FILE *outfile, const char *tok, const char *val)
+static void print_tok_val(struct strbuf *out, const char *tok, const char *val)
 {
 	char c;
 
 	if (!tok) {
-		fprintf(outfile, "%s\n", val);
+		strbuf_addf(out, "%s\n", val);
 		return;
 	}
 

@@ -157,13 +157,14 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
 	if (!c)
 		return;
 	if (strchr(separators, c))
-		fprintf(outfile, "%s%s\n", tok, val);
+		strbuf_addf(out, "%s%s\n", tok, val);
 	else
-		fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
+		strbuf_addf(out, "%s%c %s\n", tok, separators[0], val);
 }
 
 void format_trailers(const struct process_trailer_options *opts,
-		     struct list_head *trailers, FILE *outfile)
+		     struct list_head *trailers,
+		     struct strbuf *out)
 {
 	struct list_head *pos;
 	struct trailer_item *item;

@@ -171,7 +172,7 @@ void format_trailers(const struct process_trailer_options *opts,
 		item = list_entry(pos, struct trailer_item, list);
 		if ((!opts->trim_empty || strlen(item->value) > 0) &&
 		    (!opts->only_trailers || item->token))
-			print_tok_val(outfile, item->token, item->value);
+			print_tok_val(out, item->token, item->value);
 	}
 }
 

index 410c61b62be..1d106b6dd40 100644
@@ -102,7 +102,8 @@ void trailer_info_release(struct trailer_info *info);
 
 void trailer_config_init(void);
 void format_trailers(const struct process_trailer_options *,
-		     struct list_head *trailers, FILE *outfile);
+		     struct list_head *trailers,
+		     struct strbuf *out);
 void free_trailers(struct list_head *);
 
 /*

gitgitgadget

From: Linus Arver <[email protected]>

This is another preparatory refactor to unify the trailer formatters.

This allows us to drop the "msg" parameter from format_trailer_info(), so that it take 3 parameters, similar to format_trailers() which also takes 3 parameters:

void format_trailers(const struct process_trailer_options *opts,
struct list_head *trailers, struct strbuf *out)

The short-term goal is to make format_trailer_info() be smart enough to deprecate format_trailers(). And then ultimately we will rename format_trailer_info() to format_trailers().

---
 trailer.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

index cbd643cd1fe..e92d0154d90 100644
@@ -1087,21 +1087,11 @@ void trailer_info_release(struct trailer_info *info)
 
 static void format_trailer_info(const struct process_trailer_options *opts,
 				const struct trailer_info *info,
-				const char *msg,
 				struct strbuf *out)
 {
 	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);

@@ -1153,7 +1143,15 @@ void format_trailers_from_commit(const struct process_trailer_options *opts,
 	struct trailer_info info;
 
 	trailer_info_get(opts, msg, &info);
-	format_trailer_info(opts, &info, msg, out);
+	/* 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);
+	} else
+		format_trailer_info(opts, &info, out);
+
 	trailer_info_release(&info);
 }
 

gitgitgadget

From: Linus Arver <[email protected]>

This is another preparatory refactor to unify the trailer formatters.

Instead of calling trailer_info_get() directly, call parse_trailers() which already calls trailer_info_get(). This change is a NOP because format_trailer_info() only looks at the "trailers" string array, not the trailer_item objects which parse_trailers() populates.

In a future patch, we'll change format_trailer_info() to use the parsed trailer_item objects instead of the string array.

---
 trailer.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

index e92d0154d90..e6665c99cc3 100644
@@ -1140,9 +1140,11 @@ void format_trailers_from_commit(const struct process_trailer_options *opts,
 				 const char *msg,
 				 struct strbuf *out)
 {
+	LIST_HEAD(trailers);
 	struct trailer_info info;
 
-	trailer_info_get(opts, msg, &info);
+	parse_trailers(opts, &info, msg, &trailers);
+
 	/* 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 &&

@@ -1152,6 +1154,7 @@ void format_trailers_from_commit(const struct process_trailer_options *opts,
 	} else
 		format_trailer_info(opts, &info, out);
 
+	free_trailers(&trailers);
 	trailer_info_release(&info);
 }
 

gitgitgadget

Is the extra processing done by parse_trailers() compared to trailer_info_get() impacting performance?

Also when looking only at the patch, it's a bit difficult to understand that the "trailers" string array is the char **trailers field in struct trailer_info and that the trailer_item objects are the elements of the struct list_head *head linked list. It could also be confusing because the patch is adding a new 'trailers' variable with LIST_HEAD(trailers);. So a few more details could help understand what's going on.

Ok, so I guess the possible performance issue would disappear then, as populating the trailer_item objects will be useful.

I was going to answer this now but I see that you've already reached the same conclusion as me further below. ;)

Makes sense. Admittedly, this is one thing that did bother me at some point but which I forgot about as I got more familiar with my own patch.

I will avoid shadowing the "trailers" word (and maybe at least add a suffix or prefix to disambiguate it).

Yep, exactly. In the larger series (some 20? 30?) commits down the road in my local tree, I have it so that we remove char **trailers entirely (because we should be using the (smarter) trailer_item objects, not raw strings, where possible).

I took another look and suggested some improvements to commit messages.

Thanks!

This patch series is the first 9 patches of a larger cleanup/bugfix series (henceforth "larger series") I've been working on. The main goal of this series is to begin the process of "libifying" the trailer API. By "API" I mean the interface exposed in trailer.h. The larger series brings a number of additional cleanups (exposing and fixing some bugs along the way), and builds on top of this series.

When the larger series is merged, we will be in a good state to additionally pursue the following goals:

1. "API reuse inside Git": make the API expressive enough to eliminate any need by other parts of Git to use the interpret-trailers builtin as a subprocess (instead they could just use the API directly);
2. "API stability": add unit tests to codify the expected behavior of API functions; and
3. "API documentation": create developer-focused documentation to explain how to use the API effectively, noting any API limitations or anti-patterns.

In the future after libification is "complete", users external to Git will be able to use the same trailer processing API used by the interpret-trailers builtin. For example, a web server may want to parse trailers the same way that Git would parse them, without having to call interpret-trailers as a subprocess. This use case was the original motivation behind my work in this area.

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

In summary this series breaks up "process_trailers()" into smaller pieces, exposing many of the parts relevant to trailer-related processing in trailer.h. This will force us to eventually introduce unit tests for these API functions, but that is a good thing for API stability. We also perform some preparatory refactors in order to help us unify the trailer formatting machinery toward the end of this series.

Notable changes in v6
=====================

* Mainly wording changes to commit messages. Thanks to Christian for the suggestions.

Notable changes in v5
=====================

* Removed patches 10+ from this series. Thanks to Christian for the suggestion.
* Reworded the log message of patch 09 to reflect the above arrangement, as suggested by Christian.

Notable changes in v4
=====================

* Patches 3, 4, 5, and 8 have been broken up into smaller steps. There are 28 instead of 10 patches now, but these 28 should be much easier to review than the (previously condensed) 10.
* NEW Patch 1: "trailer: free trailer_info after all related usage" fixes awkward use-after-free coding style
* NEW Patch 2: "shortlog: add test for de-duplicating folded trailers" increases test coverage related to trailer iterators and "unfold_value()"
* NEW Patch 27: "trailer_set_*(): put out parameter at the end" is a small refactor to reorder parameters.
* Patches 5-16: These smaller patches make up Patch 3 from v3.
* Patches 17-18: These smaller patches make up Patch 4 from v3.
* Patches 19-20: These smaller patches make up Patch 5 from v3.
* Patches 23-26: These smaller patches make up Patch 8 from v3.
* Anonymize unambiguous parameters in <trailer.h>.

Notable changes in v3
=====================

* Squashed Patch 4 into Patch 3 ("trailer: unify trailer formatting machinery"), to avoid breaking the build ("-Werror=unused-function" violations)
* NEW (Patch 10): Introduce "trailer template" terminology for readability (no behavioral change)
* (API function) Rename default_separators() to trailer_default_separators()
* (API function) Rename new_trailers_clear() to free_trailer_templates()
* trailer.h: for single-parameter functions, anonymize the parameter name to reduce verbosity

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

* (cover letter) Discuss goals of the larger series in more detail, especially the pimpl idiom
* (cover letter) List bug fixes pending in the larger series that depend on this series
* Reorder function parameters to have trailer options at the beginning (and out parameters toward the end)
* "sequencer: use the trailer iterator": prefer C string instead of strbuf for new "raw" field
* Patch 1 (was Patch 2) also renames ensure_configured() to trailer_config_init() (forgot to rename this one previously)

Linus Arver (9):
trailer: free trailer_info _after_ all related usage
shortlog: add test for de-duplicating folded trailers
trailer: rename functions to use 'trailer'
trailer: move interpret_trailers() to interpret-trailers.c
trailer: reorder format_trailers_from_commit() parameters trailer_info_get(): reorder parameters format_trailers(): use strbuf instead of FILE format_trailer_info(): move "fast path" to caller format_trailers_from_commit(): indirectly call trailer_info_get()

 builtin/interpret-trailers.c | 101 ++++++++++++++++++-
 pretty.c                     |   2 +-
 ref-filter.c                 |   2 +-
 sequencer.c                  |   2 +-
 t/t4201-shortlog.sh          |  32 +++++++
 trailer.c                    | 181 +++++++++--------------------------
 trailer.h                    |  31 ++++--
 7 files changed, 204 insertions(+), 147 deletions(-)

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

Range-diff vs v5:

  1:  652df25f30e =  1:  652df25f30e trailer: free trailer_info _after_ all related usage
  2:  fdccaca2ba0 =  2:  fdccaca2ba0 shortlog: add test for de-duplicating folded trailers
  3:  4372af244f0 !  3:  7b1d739cddb trailer: prepare to expose functions as part of API

@@ Metadata
Author: Linus Arver <[email protected]>

## Commit message ##
- trailer: prepare to expose functions as part of API
+ trailer: rename functions to use 'trailer'

- In the next patch, we will move "process_trailers" from trailer.c to
- builtin/interpret-trailers.c. That move will necessitate the growth of
- the trailer.h API, forcing us to expose some additional functions in
+ Rename process_trailers() to interpret_trailers(), because it matches
+ the name for the builtin command of the same name
+ (git-interpret-trailers), which is the sole user of process_trailers().
+
+ In a following commit, we will move "interpret_trailers" from trailer.c
+ to builtin/interpret-trailers.c. That move will necessitate the growth
+ of the trailer.h API, forcing us to expose some additional functions in
trailer.h.

Rename relevant functions so that they include the term "trailer" in
@@ Commit message
them by their "trailer" moniker, just like all the other functions already exposed by trailer.h.

- Take the opportunity to start putting trailer processing options (opts)
- as the first parameter. This will be the pattern going forward in this
- series.
+ Rename struct list_head *head to struct list_head *trailers because
+ "head" conveys no additional information beyond the "list_head" type.
+
+ Reorder parameters for format_trailers_from_commit() to prefer
+
+ const struct process_trailer_options *opts
+
+ as the first parameter, because these options are intimately tied to
+ formatting trailers. Parameters like FILE *outfile should be last
+ because they are a kind of 'out' parameter, so put such parameters at
+ the end. This will be the pattern going forward in this series.

+ Helped-by: Christian Couder <[email protected]>

## builtin/interpret-trailers.c ##

  4:  4073b8eb510 =  4:  7ac4da3019a trailer: move interpret_trailers() to interpret-trailers.c
  5:  b2a0f7829a1 !  5:  47c994ce025 trailer: start preparing for formatting unification

@@ Metadata
Author: Linus Arver <[email protected]>

## Commit message ##
- trailer: start preparing for formatting unification
+ trailer: reorder format_trailers_from_commit() parameters

Currently there are two functions for formatting trailers in <trailer.h>:
@@ Commit message
last, because it's an "out parameter" (something that the caller wants to use as the output of this function).

+ Similarly, reorder parameters for format_trailer_info(), because later
+ on we will unify the two together.
+

## pretty.c ##

  6:  c1760f80356 =  6:  7a565580167 trailer_info_get(): reorder parameters
  7:  9dc912b5bc5 =  7:  46c7f4c0e81 format_trailers(): use strbuf instead of FILE
  8:  b97c06d8bc3 =  8:  26b1f19d0e1 format_trailer_info(): move "fast path" to caller
  9:  7c656b3f775 !  9:  0e884d870c8 format_trailers_from_commit(): indirectly call trailer_info_get()

@@ Commit message

This is another preparatory refactor to unify the trailer formatters.

- Instead of calling trailer_info_get() directly, call parse_trailers()
- which already calls trailer_info_get(). This change is a NOP because
- format_trailer_info() only looks at the "trailers" string array, not the
- trailer_item objects which parse_trailers() populates.
+ For background, note that the "trailers" string array is the
+ char **trailers member in struct trailer_info and that the
+ trailer_item objects are the elements of the struct list_head *head
+ linked list.
+
+ Currently trailer_info_get() only populates char **trailers. And
+ parse_trailers() first calls trailer_info_get() so that it can use the
+ char **trailers to populate a list of struct trailer_item objects
+
+ Instead of calling trailer_info_get() directly from
+ format_trailers_from_commit(), make it call parse_trailers() instead
+ because parse_trailers() already calls trailer_info_get().
+
+ This change is a NOP because format_trailer_info() (which
+ format_trailers_from_commit() wraps around) only looks at the "trailers"
+ string array, not the trailer_item objects which parse_trailers()
+ populates. For now we do need to create a dummy
+
+ LIST_HEAD(trailer_objects);
+
+ because parse_trailers() expects it in its signature.

In a future patch, we'll change format_trailer_info() to use the parsed
- trailer_item objects instead of the string array.
+ trailer_item objects (trailer_objects) instead of the char **trailers
+ array.

@@ trailer.c: void format_trailers_from_commit(const struct process_trailer_options
const char *msg,
struct strbuf *out)
{
-+ LIST_HEAD(trailers);
++ LIST_HEAD(trailer_objects);
struct trailer_info info;

- trailer_info_get(opts, msg, &info);
-+ parse_trailers(opts, &info, msg, &trailers);
++ parse_trailers(opts, &info, msg, &trailer_objects);
+
/* If we want the whole block untouched, we can take the fast path. */ if (!opts->only_trailers && !opts->unfold && !opts->filter &&
@@ trailer.c: void format_trailers_from_commit(const struct process_trailer_options
} else
format_trailer_info(opts, &info, out);

-+ free_trailers(&trailers);
++ free_trailers(&trailer_objects);
trailer_info_release(&info);
}

gitgitgadget

From: Linus Arver <[email protected]>

In de7c27a186 (trailer: use offsets for trailer_start/trailer_end, 2023-10-20), we started using trailer block offsets in trailer_info. In particular, we dropped the use of a separate stack variable "size_t trailer_end", in favor of accessing the new "trailer_block_end" member of trailer_info (as "info.trailer_block_end").

At that time, we forgot to also move the

   trailer_info_release(&info);

line to be _after_ this new use of the trailer_info struct. Move it now.

Note that even without this patch, we didn't have leaks or any other problems because trailer_info_release() only frees memory allocated on the heap. The "trailer_block_end" member was allocated on the stack back then (as it is now) so it was still safe to use for all this time.

---
 trailer.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 3a0710a4583..e1d83390b66 100644
@@ -1111,13 +1111,12 @@ void process_trailers(const char *file,
 	}
 
 	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);
+	trailer_info_release(&info);
 
 	if (opts->in_place)
 		if (rename_tempfile(&trailers_tempfile, file))

gitgitgadget

From: Linus Arver <[email protected]>

The shortlog builtin was taught to use the trailer iterator interface in 47beb37bc6 (shortlog: match commit trailers with --group, 2020-09-27). The iterator always unfolds values and this has always been the case since the time the iterator was first introduced in f0939a0eb1 (trailer:
add interface for iterating over commit trailers, 2020-09-27). Add a comment line to remind readers of this behavior.

The fact that the iterator always unfolds values is important (at least for shortlog) because unfolding allows it to recognize both folded and unfolded versions of the same trailer for de-duplication.

Capture the existing behavior in a new test case to guard against regressions in this area. This test case is based off of the existing "shortlog de-duplicates trailers in a single commit" just above it. Now if we were to remove the call to

    unfold_value(&iter->val);

inside the iterator, this new test case will break.

---
 t/t4201-shortlog.sh | 32 ++++++++++++++++++++++++++++++++
 trailer.c           |  1 +
 2 files changed, 33 insertions(+)

index d7382709fc1..f698d0c9ad2 100755
@@ -312,6 +312,38 @@ test_expect_success 'shortlog de-duplicates trailers in a single commit' '
 	test_cmp expect actual
 '
 
+# Trailers that have unfolded (single line) and folded (multiline) values which
+# are otherwise identical are treated as the same trailer for de-duplication.
+test_expect_success 'shortlog de-duplicates trailers in a single commit (folded/unfolded values)' '
+	git commit --allow-empty -F - <<-\EOF &&
+	subject one
+
+	this message has two distinct values, plus a repeat (folded)
+
+	Repeated-trailer: Foo foo foo
+	Repeated-trailer: Bar
+	Repeated-trailer: Foo
+	  foo foo
+	EOF
+
+	git commit --allow-empty -F - <<-\EOF &&
+	subject two
+
+	similar to the previous, but without the second distinct value
+
+	Repeated-trailer: Foo foo foo
+	Repeated-trailer: Foo
+	  foo foo
+	EOF
+
+	cat >expect <<-\EOF &&
+	     2	Foo foo foo
+	     1	Bar
+	EOF
+	git shortlog -ns --group=trailer:repeated-trailer -2 HEAD >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'shortlog can match multiple groups' '
 	git commit --allow-empty -F - <<-\EOF &&
 	subject one

index e1d83390b66..f74915bd8cd 100644
@@ -1270,6 +1270,7 @@ int trailer_iterator_advance(struct trailer_iterator *iter)
 		strbuf_reset(&iter->val);
 		parse_trailer(&iter->key, &iter->val, NULL,
 			      trailer, separator_pos);
+		/* Always unfold values during iteration. */
 		unfold_value(&iter->val);
 		return 1;
 	}

gitgitgadget

From: Linus Arver <[email protected]>

Rename process_trailers() to interpret_trailers(), because it matches the name for the builtin command of the same name (git-interpret-trailers), which is the sole user of process_trailers().

In a following commit, we will move "interpret_trailers" from trailer.c to builtin/interpret-trailers.c. That move will necessitate the growth of the trailer.h API, forcing us to expose some additional functions in trailer.h.

Rename relevant functions so that they include the term "trailer" in 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.

Rename struct list_head *head to struct list_head *trailers because "head" conveys no additional information beyond the "list_head" type.

Reorder parameters for format_trailers_from_commit() to prefer

const struct process_trailer_options *opts

as the first parameter, because these options are intimately tied to formatting trailers. Parameters like FILE *outfile should be last because they are a kind of 'out' parameter, so put such parameters at the end. This will be the pattern going forward in this series.