diff mbox series

[v4,10/28] format_trailer_info(): use trailer_item objects

Message ID f5b7ba08aa7c80a3bd5bcbf5563eac8896fe7054.1707196348.git.gitgitgadget@gmail.com (mailing list archive)
State New
Headers show
Series Enrich Trailer API | expand

Commit Message

Linus Arver Feb. 6, 2024, 5:12 a.m. UTC
From: Linus Arver <linusa@google.com>

This is another preparatory refactor to unify the trailer formatters.

Make format_trailer_info() operate on trailer_item objects, not the raw
string array.

This breaks t4205 and t6300. We will continue to make improvements until
the test suite passes again, ultimately renaming format_trailer_info()
to format_trailers(), at which point the unification of these formatters
will be complete.

Signed-off-by: Linus Arver <linusa@google.com>
---
 trailer.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

Comments

Junio C Hamano Feb. 9, 2024, 9:53 p.m. UTC | #1
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Linus Arver <linusa@google.com>
>
> This is another preparatory refactor to unify the trailer formatters.
>
> Make format_trailer_info() operate on trailer_item objects, not the raw
> string array.
>
> This breaks t4205 and t6300. We will continue to make improvements until
> the test suite passes again, ultimately renaming format_trailer_info()
> to format_trailers(), at which point the unification of these formatters
> will be complete.
>
> Signed-off-by: Linus Arver <linusa@google.com>
> ---
>  trailer.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)

I would have expected a (tentative) flip from test_expect_success to
test_expect_failure in the affected tests, to illustrate what behaviour
change this step introduces and why.

But the huge single step in the previous round broken out into
smaller steps like this round makes them much easier to follow.

Thanks.

> diff --git a/trailer.c b/trailer.c
> index e6665c99cc3..6333dfe1c11 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -1086,21 +1086,21 @@ void trailer_info_release(struct trailer_info *info)
>  }
>  
>  static void format_trailer_info(const struct process_trailer_options *opts,
> -				const struct trailer_info *info,
> +				struct list_head *trailers,
>  				struct strbuf *out)
>  {
>  	size_t origlen = out->len;
> -	size_t i;
> -
> -	for (i = 0; i < info->trailer_nr; i++) {
> -		char *trailer = info->trailers[i];
> -		ssize_t separator_pos = find_separator(trailer, separators);
> +	struct list_head *pos;
> +	struct trailer_item *item;
>  
> -		if (separator_pos >= 1) {
> +	list_for_each(pos, trailers) {
> +		item = list_entry(pos, struct trailer_item, list);
> +		if (item->token) {
>  			struct strbuf tok = STRBUF_INIT;
>  			struct strbuf val = STRBUF_INIT;
> +			strbuf_addstr(&tok, item->token);
> +			strbuf_addstr(&val, item->value);
>  
> -			parse_trailer(&tok, &val, NULL, trailer, separator_pos);
>  			if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
>  				if (opts->unfold)
>  					unfold_value(&val);
> @@ -1127,13 +1127,12 @@ static void format_trailer_info(const struct process_trailer_options *opts,
>  			if (opts->separator && out->len != origlen) {
>  				strbuf_addbuf(out, opts->separator);
>  			}
> -			strbuf_addstr(out, trailer);
> +			strbuf_addstr(out, item->value);
>  			if (opts->separator) {
>  				strbuf_rtrim(out);
>  			}
>  		}
>  	}
> -
>  }
>  
>  void format_trailers_from_commit(const struct process_trailer_options *opts,
> @@ -1152,7 +1151,7 @@ void format_trailers_from_commit(const struct process_trailer_options *opts,
>  		strbuf_add(out, msg + info.trailer_block_start,
>  			   info.trailer_block_end - info.trailer_block_start);
>  	} else
> -		format_trailer_info(opts, &info, out);
> +		format_trailer_info(opts, &trailers, out);
>  
>  	free_trailers(&trailers);
>  	trailer_info_release(&info);
Linus Arver Feb. 13, 2024, 4:35 p.m. UTC | #2
Junio C Hamano <gitster@pobox.com> writes:

> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Linus Arver <linusa@google.com>
>>
>> This is another preparatory refactor to unify the trailer formatters.
>>
>> Make format_trailer_info() operate on trailer_item objects, not the raw
>> string array.
>>
>> This breaks t4205 and t6300. We will continue to make improvements until
>> the test suite passes again, ultimately renaming format_trailer_info()
>> to format_trailers(), at which point the unification of these formatters
>> will be complete.
>>
>> Signed-off-by: Linus Arver <linusa@google.com>
>> ---
>>  trailer.c | 21 ++++++++++-----------
>>  1 file changed, 10 insertions(+), 11 deletions(-)
>
> I would have expected a (tentative) flip from test_expect_success to
> test_expect_failure in the affected tests, to illustrate what behaviour
> change this step introduces and why.

Somehow, such a simple idea did not cross my mind (even though I admit I
didn't like how I was breaking tests, albeit temporarily). Will update
on next reroll.

> But the huge single step in the previous round broken out into
> smaller steps like this round makes them much easier to follow.

Great, I'm doing something right for once! ;)
diff mbox series

Patch

diff --git a/trailer.c b/trailer.c
index e6665c99cc3..6333dfe1c11 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1086,21 +1086,21 @@  void trailer_info_release(struct trailer_info *info)
 }
 
 static void format_trailer_info(const struct process_trailer_options *opts,
-				const struct trailer_info *info,
+				struct list_head *trailers,
 				struct strbuf *out)
 {
 	size_t origlen = out->len;
-	size_t i;
-
-	for (i = 0; i < info->trailer_nr; i++) {
-		char *trailer = info->trailers[i];
-		ssize_t separator_pos = find_separator(trailer, separators);
+	struct list_head *pos;
+	struct trailer_item *item;
 
-		if (separator_pos >= 1) {
+	list_for_each(pos, trailers) {
+		item = list_entry(pos, struct trailer_item, list);
+		if (item->token) {
 			struct strbuf tok = STRBUF_INIT;
 			struct strbuf val = STRBUF_INIT;
+			strbuf_addstr(&tok, item->token);
+			strbuf_addstr(&val, item->value);
 
-			parse_trailer(&tok, &val, NULL, trailer, separator_pos);
 			if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
 				if (opts->unfold)
 					unfold_value(&val);
@@ -1127,13 +1127,12 @@  static void format_trailer_info(const struct process_trailer_options *opts,
 			if (opts->separator && out->len != origlen) {
 				strbuf_addbuf(out, opts->separator);
 			}
-			strbuf_addstr(out, trailer);
+			strbuf_addstr(out, item->value);
 			if (opts->separator) {
 				strbuf_rtrim(out);
 			}
 		}
 	}
-
 }
 
 void format_trailers_from_commit(const struct process_trailer_options *opts,
@@ -1152,7 +1151,7 @@  void format_trailers_from_commit(const struct process_trailer_options *opts,
 		strbuf_add(out, msg + info.trailer_block_start,
 			   info.trailer_block_end - info.trailer_block_start);
 	} else
-		format_trailer_info(opts, &info, out);
+		format_trailer_info(opts, &trailers, out);
 
 	free_trailers(&trailers);
 	trailer_info_release(&info);