From 65b4ad82b81e1a1f4afbb7f4974384d7db479c0a Mon Sep 17 00:00:00 2001 From: Linus Arver Date: Fri, 15 Mar 2024 06:55:01 +0000 Subject: format_trailer_info(): use trailer_item objects This is another preparatory refactor to unify the trailer formatters. Make format_trailer_info() operate on trailer_item objects, not the raw string array. We will continue to make improvements, culminating in the renaming of format_trailer_info() to format_trailers(), at which point the unification of these formatters will be complete. In order to avoid breaking t4205 and t6300, flip *_success to *_failure in the affected test cases. Add a temporary "test_trailer_option_expect_failure" wrapper which we will use along with "test_expect_failure" in the next commit to avoid breaking tests. When the dust settles with the refactors a few more commits later, we will drop the use of *_failure to make the tests truly pass again. When the preparatory refactors are complete, we'll be able to drop the use of *_failure that we introduce here. Signed-off-by: Linus Arver Signed-off-by: Junio C Hamano --- t/t4205-log-pretty-formats.sh | 12 ++++++------ t/t6300-for-each-ref.sh | 16 ++++++++++++++-- trailer.c | 21 ++++++++++----------- 3 files changed, 30 insertions(+), 19 deletions(-) diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index e3d655e6b8..339e0c892e 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -675,7 +675,7 @@ test_expect_success '%(trailers:only=no,only=true) shows only "key: value" trail test_cmp expect actual ' -test_expect_success '%(trailers:unfold) unfolds trailers' ' +test_expect_failure '%(trailers:unfold) unfolds trailers' ' git log --no-walk --pretty="%(trailers:unfold)" >actual && { unfold actual && { echo "Acked-by: A U Thor " && @@ -752,7 +752,7 @@ test_expect_success '%(trailers:key) without value is error' ' test_cmp expect actual ' -test_expect_success '%(trailers:keyonly) shows only keys' ' +test_expect_failure '%(trailers:keyonly) shows only keys' ' git log --no-walk --pretty="format:%(trailers:keyonly)" >actual && test_write_lines \ "Signed-off-by" \ @@ -774,7 +774,7 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' ' test_cmp expect actual ' -test_expect_success '%(trailers:valueonly) shows only values' ' +test_expect_failure '%(trailers:valueonly) shows only values' ' git log --no-walk --pretty="format:%(trailers:valueonly)" >actual && test_write_lines \ "A U Thor " \ @@ -813,7 +813,7 @@ test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separa test_cmp expect actual ' -test_expect_success 'pretty format %(trailers:key_value_separator) changes key-value separator' ' +test_expect_failure 'pretty format %(trailers:key_value_separator) changes key-value separator' ' git log --no-walk --pretty=format:"X%(trailers:key_value_separator=%x00)X" >actual && ( printf "XSigned-off-by\0A U Thor \n" && @@ -824,7 +824,7 @@ test_expect_success 'pretty format %(trailers:key_value_separator) changes key-v test_cmp expect actual ' -test_expect_success 'pretty format %(trailers:key_value_separator,unfold) changes key-value separator' ' +test_expect_failure 'pretty format %(trailers:key_value_separator,unfold) changes key-value separator' ' git log --no-walk --pretty=format:"X%(trailers:key_value_separator=%x00,unfold)X" >actual && ( printf "XSigned-off-by\0A U Thor \n" && diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index eb6c8204e8..2688dcc7b9 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -1446,7 +1446,19 @@ test_trailer_option () { ' } -test_trailer_option '%(trailers:unfold) unfolds trailers' \ +# Just like test_trailer_option, but expect failure instead of success. +test_trailer_option_expect_failure () { + title=$1 option=$2 + cat >expect + test_expect_failure "$title" ' + git for-each-ref --format="%($option)" refs/heads/main >actual && + test_cmp expect actual && + git for-each-ref --format="%(contents:$option)" refs/heads/main >actual && + test_cmp expect actual + ' +} + +test_trailer_option_expect_failure '%(trailers:unfold) unfolds trailers' \ 'trailers:unfold' <<-EOF $(unfold $(grep patch.description 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); @@ -1126,13 +1126,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, @@ -1151,7 +1150,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, &trailer_objects, out); free_trailers(&trailer_objects); trailer_info_release(&info); -- cgit 1.2.3-korg From 41ea0a900221897c5ba36afb9f0b31bf543cea7e Mon Sep 17 00:00:00 2001 From: Linus Arver Date: Fri, 15 Mar 2024 06:55:02 +0000 Subject: format_trailer_info(): drop redundant unfold_value() This is another preparatory refactor to unify the trailer formatters. In the last patch we made format_trailer_info() use trailer_item objects instead of the "trailers" string array. This means that the call to unfold_value() here is redundant because the trailer_item objects are already unfolded in parse_trailers() which is a dependency of our caller, format_trailers_from_commit(). Remove the redundant call. Signed-off-by: Linus Arver Signed-off-by: Junio C Hamano --- trailer.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/trailer.c b/trailer.c index a74f05db55..2c0dd8ac82 100644 --- a/trailer.c +++ b/trailer.c @@ -1101,9 +1101,6 @@ static void format_trailer_info(const struct process_trailer_options *opts, strbuf_addstr(&val, item->value); if (!opts->filter || opts->filter(&tok, opts->filter_data)) { - if (opts->unfold) - unfold_value(&val); - if (opts->separator && out->len != origlen) strbuf_addbuf(out, opts->separator); if (!opts->value_only) -- cgit 1.2.3-korg From 9f0c9702de9c87aa30697d88c4dc9ad0610f4201 Mon Sep 17 00:00:00 2001 From: Linus Arver Date: Fri, 15 Mar 2024 06:55:03 +0000 Subject: format_trailer_info(): append newline for non-trailer lines This wraps up the preparatory refactors to unify the trailer formatters. Two patches ago we made format_trailer_info() use trailer_item objects instead of the "trailers" string array. The strings in the array include trailing newlines, because the string array is split up with trailer_lines = strbuf_split_buf(str + trailer_block_start, end_of_log_message - trailer_block_start, '\n', 0); in trailer_info_get() and strbuf_split_buf() includes the terminator (in this case the newline character '\n') for each split-up substring. And before we made the transition to use trailer_item objects for it, format_trailer_info() called parse_trailer() (which trims newlines) for trailer lines but did _not_ call parse_trailer() for non-trailer lines. So for trailer lines it had to add back the trimmed newline like this if (!opts->separator) strbuf_addch(out, '\n'); But for non-trailer lines it didn't have to add back the newline because it could just reuse same string in the "trailers" string array (which again, already included the trailing newline). Now that format_trailer_info() uses trailer_item objects for all cases, it can't rely on "trailers" string array anymore. And so it must be taught to add a newline back when printing non-trailer lines, just like it already does for trailer lines. Do so now. The test suite can pass again without the need to hide failures with *_failure, so flip the affected test cases back to *_success. Now, format_trailer_info() is in better shape to supersede format_trailers(), which we'll do in the next commit. Signed-off-by: Linus Arver Signed-off-by: Junio C Hamano --- t/t4205-log-pretty-formats.sh | 12 ++++++------ t/t6300-for-each-ref.sh | 16 ++-------------- trailer.c | 5 +++-- 3 files changed, 11 insertions(+), 22 deletions(-) diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 339e0c892e..e3d655e6b8 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -675,7 +675,7 @@ test_expect_success '%(trailers:only=no,only=true) shows only "key: value" trail test_cmp expect actual ' -test_expect_failure '%(trailers:unfold) unfolds trailers' ' +test_expect_success '%(trailers:unfold) unfolds trailers' ' git log --no-walk --pretty="%(trailers:unfold)" >actual && { unfold actual && { echo "Acked-by: A U Thor " && @@ -752,7 +752,7 @@ test_expect_success '%(trailers:key) without value is error' ' test_cmp expect actual ' -test_expect_failure '%(trailers:keyonly) shows only keys' ' +test_expect_success '%(trailers:keyonly) shows only keys' ' git log --no-walk --pretty="format:%(trailers:keyonly)" >actual && test_write_lines \ "Signed-off-by" \ @@ -774,7 +774,7 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' ' test_cmp expect actual ' -test_expect_failure '%(trailers:valueonly) shows only values' ' +test_expect_success '%(trailers:valueonly) shows only values' ' git log --no-walk --pretty="format:%(trailers:valueonly)" >actual && test_write_lines \ "A U Thor " \ @@ -813,7 +813,7 @@ test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separa test_cmp expect actual ' -test_expect_failure 'pretty format %(trailers:key_value_separator) changes key-value separator' ' +test_expect_success 'pretty format %(trailers:key_value_separator) changes key-value separator' ' git log --no-walk --pretty=format:"X%(trailers:key_value_separator=%x00)X" >actual && ( printf "XSigned-off-by\0A U Thor \n" && @@ -824,7 +824,7 @@ test_expect_failure 'pretty format %(trailers:key_value_separator) changes key-v test_cmp expect actual ' -test_expect_failure 'pretty format %(trailers:key_value_separator,unfold) changes key-value separator' ' +test_expect_success 'pretty format %(trailers:key_value_separator,unfold) changes key-value separator' ' git log --no-walk --pretty=format:"X%(trailers:key_value_separator=%x00,unfold)X" >actual && ( printf "XSigned-off-by\0A U Thor \n" && diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 2688dcc7b9..eb6c8204e8 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -1446,19 +1446,7 @@ test_trailer_option () { ' } -# Just like test_trailer_option, but expect failure instead of success. -test_trailer_option_expect_failure () { - title=$1 option=$2 - cat >expect - test_expect_failure "$title" ' - git for-each-ref --format="%($option)" refs/heads/main >actual && - test_cmp expect actual && - git for-each-ref --format="%(contents:$option)" refs/heads/main >actual && - test_cmp expect actual - ' -} - -test_trailer_option_expect_failure '%(trailers:unfold) unfolds trailers' \ +test_trailer_option '%(trailers:unfold) unfolds trailers' \ 'trailers:unfold' <<-EOF $(unfold $(grep patch.description separator); } strbuf_addstr(out, item->value); - if (opts->separator) { + if (opts->separator) strbuf_rtrim(out); - } + else + strbuf_addch(out, '\n'); } } } -- cgit 1.2.3-korg From 676c1db76e310c400b602890ac6853fdf8fdfa98 Mon Sep 17 00:00:00 2001 From: Linus Arver Date: Fri, 15 Mar 2024 06:55:04 +0000 Subject: trailer: begin formatting unification Now that the preparatory refactors are over, we can replace the call to format_trailers() in interpret-trailers with format_trailer_info(). This unifies the trailer formatting machinery In order to avoid breakages in t7502 and t7513, we have to steal the features present in format_trailers(). Namely, we have to teach format_trailer_info() as follows: (1) make it aware of opts->trim_empty, and (2) make it avoid hardcoding ": " as the separator and space (which can result in double-printing these characters). For (2), make it only print the separator and space if we cannot find any recognized separator somewhere in the key (yes, keys may have a trailing separator in it --- we will eventually fix this design but not now). Do so by copying the code out of print_tok_val(), and deleting the same function. Helped-by: Junio C Hamano Helped-by: Christian Couder Signed-off-by: Linus Arver Signed-off-by: Junio C Hamano --- builtin/interpret-trailers.c | 2 +- trailer.c | 54 ++++++++++++++------------------------------ trailer.h | 2 +- 3 files changed, 19 insertions(+), 39 deletions(-) diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c index 11f4ce9e4a..f57af0db37 100644 --- a/builtin/interpret-trailers.c +++ b/builtin/interpret-trailers.c @@ -171,7 +171,7 @@ static void interpret_trailers(const struct process_trailer_options *opts, } /* Print trailer block. */ - format_trailers(opts, &head, &trailer_block); + format_trailer_info(opts, &head, &trailer_block); free_trailers(&head); fwrite(trailer_block.buf, 1, trailer_block.len, outfile); strbuf_release(&trailer_block); diff --git a/trailer.c b/trailer.c index fe8b0819d5..43d5baef9c 100644 --- a/trailer.c +++ b/trailer.c @@ -144,38 +144,6 @@ static char last_non_space_char(const char *s) return '\0'; } -static void print_tok_val(struct strbuf *out, const char *tok, const char *val) -{ - char c; - - if (!tok) { - strbuf_addf(out, "%s\n", val); - return; - } - - c = last_non_space_char(tok); - if (!c) - return; - if (strchr(separators, c)) - strbuf_addf(out, "%s%s\n", tok, val); - else - strbuf_addf(out, "%s%c %s\n", tok, separators[0], val); -} - -void format_trailers(const struct process_trailer_options *opts, - struct list_head *trailers, - struct strbuf *out) -{ - struct list_head *pos; - struct trailer_item *item; - 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)) - print_tok_val(out, item->token, item->value); - } -} - static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok) { struct trailer_item *new_item = xcalloc(1, sizeof(*new_item)); @@ -1084,9 +1052,9 @@ void trailer_info_release(struct trailer_info *info) free(info->trailers); } -static void format_trailer_info(const struct process_trailer_options *opts, - struct list_head *trailers, - struct strbuf *out) +void format_trailer_info(const struct process_trailer_options *opts, + struct list_head *trailers, + struct strbuf *out) { size_t origlen = out->len; struct list_head *pos; @@ -1100,6 +1068,15 @@ static void format_trailer_info(const struct process_trailer_options *opts, strbuf_addstr(&tok, item->token); strbuf_addstr(&val, item->value); + /* + * Skip key/value pairs where the value was empty. This + * can happen from trailers specified without a + * separator, like `--trailer "Reviewed-by"` (no + * corresponding value). + */ + if (opts->trim_empty && !strlen(item->value)) + continue; + if (!opts->filter || opts->filter(&tok, opts->filter_data)) { if (opts->separator && out->len != origlen) strbuf_addbuf(out, opts->separator); @@ -1108,8 +1085,11 @@ static void format_trailer_info(const struct process_trailer_options *opts, if (!opts->key_only && !opts->value_only) { if (opts->key_value_separator) strbuf_addbuf(out, opts->key_value_separator); - else - strbuf_addstr(out, ": "); + else { + char c = last_non_space_char(tok.buf); + if (c && !strchr(separators, c)) + strbuf_addf(out, "%c ", separators[0]); + } } if (!opts->key_only) strbuf_addbuf(out, &val); diff --git a/trailer.h b/trailer.h index 1d106b6dd4..3c13006a4c 100644 --- a/trailer.h +++ b/trailer.h @@ -101,7 +101,7 @@ void trailer_info_get(const struct process_trailer_options *, void trailer_info_release(struct trailer_info *info); void trailer_config_init(void); -void format_trailers(const struct process_trailer_options *, +void format_trailer_info(const struct process_trailer_options *, struct list_head *trailers, struct strbuf *out); void free_trailers(struct list_head *); -- cgit 1.2.3-korg From 3452d173241c8b87ecdd67f91f594cb14327e394 Mon Sep 17 00:00:00 2001 From: Linus Arver Date: Fri, 15 Mar 2024 06:55:05 +0000 Subject: trailer: finish formatting unification Rename format_trailer_info() to format_trailers(). Finally, both interpret-trailers and format_trailers_from_commit() can call "format_trailers()"! Update the comment in to remove the (now obsolete) caveats about format_trailers_from_commit(). Those caveats come from a388b10fc1 (pretty: move trailer formatting to trailer.c, 2017-08-15) where it says: pretty: move trailer formatting to trailer.c The next commit will add many features to the %(trailer) placeholder in pretty.c. We'll need to access some internal functions of trailer.c for that, so our options are either: 1. expose those functions publicly or 2. make an entry point into trailer.c to do the formatting Doing (2) ends up exposing less surface area, though do note that caveats in the docstring of the new function. which suggests format_trailers_from_commit() started out from pretty.c and did not have access to all of the trailer implementation internals, and was never intended to replace (unify) the formatting machinery in trailer.c. The refactors leading up to this commit (as well as additional refactors that will follow) expose additional functions publicly, and is therefore choosing option (1) as described in a388b10fc1. Signed-off-by: Linus Arver Signed-off-by: Junio C Hamano --- builtin/interpret-trailers.c | 2 +- trailer.c | 8 ++++---- trailer.h | 15 ++++----------- 3 files changed, 9 insertions(+), 16 deletions(-) diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c index f57af0db37..11f4ce9e4a 100644 --- a/builtin/interpret-trailers.c +++ b/builtin/interpret-trailers.c @@ -171,7 +171,7 @@ static void interpret_trailers(const struct process_trailer_options *opts, } /* Print trailer block. */ - format_trailer_info(opts, &head, &trailer_block); + format_trailers(opts, &head, &trailer_block); free_trailers(&head); fwrite(trailer_block.buf, 1, trailer_block.len, outfile); strbuf_release(&trailer_block); diff --git a/trailer.c b/trailer.c index 43d5baef9c..3e4dab9c06 100644 --- a/trailer.c +++ b/trailer.c @@ -1052,9 +1052,9 @@ void trailer_info_release(struct trailer_info *info) free(info->trailers); } -void format_trailer_info(const struct process_trailer_options *opts, - struct list_head *trailers, - struct strbuf *out) +void format_trailers(const struct process_trailer_options *opts, + struct list_head *trailers, + struct strbuf *out) { size_t origlen = out->len; struct list_head *pos; @@ -1128,7 +1128,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, &trailer_objects, out); + format_trailers(opts, &trailer_objects, out); free_trailers(&trailer_objects); trailer_info_release(&info); diff --git a/trailer.h b/trailer.h index 3c13006a4c..9f42aa7599 100644 --- a/trailer.h +++ b/trailer.h @@ -101,23 +101,16 @@ void trailer_info_get(const struct process_trailer_options *, void trailer_info_release(struct trailer_info *info); void trailer_config_init(void); -void format_trailer_info(const struct process_trailer_options *, +void format_trailers(const struct process_trailer_options *, struct list_head *trailers, struct strbuf *out); void free_trailers(struct list_head *); /* - * Format the trailers from the commit msg "msg" into the strbuf "out". - * Note two caveats about "opts": - * - * - this is primarily a helper for pretty.c, and not - * all of the flags are supported. - * - * - this differs from process_trailers slightly in that we always format - * only the trailer block itself, even if the "only_trailers" option is not - * set. + * Convenience function to format the trailers from the commit msg "msg" into + * the strbuf "out". Reuses format_trailers() internally. */ -void format_trailers_from_commit(const struct process_trailer_options *opts, +void format_trailers_from_commit(const struct process_trailer_options *, const char *msg, struct strbuf *out); -- cgit 1.2.3-korg