From 0f3a461d4e0f8854181054d86681c0664ae8c0cc Mon Sep 17 00:00:00 2001 From: Linus Arver Date: Fri, 1 Mar 2024 00:14:38 +0000 Subject: [PATCH 1/9] trailer: free trailer_info _after_ all related usage 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. Reported-by: Junio C Hamano Signed-off-by: Linus Arver Signed-off-by: Junio C Hamano --- trailer.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/trailer.c b/trailer.c index 3a0710a458..e1d83390b6 100644 --- a/trailer.c +++ b/trailer.c @@ -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)) From a082e2893868be819b5486e9af3e390a7c948ee2 Mon Sep 17 00:00:00 2001 From: Linus Arver Date: Fri, 1 Mar 2024 00:14:39 +0000 Subject: [PATCH 2/9] shortlog: add test for de-duplicating folded trailers 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. Signed-off-by: Linus Arver Signed-off-by: Junio C Hamano --- t/t4201-shortlog.sh | 32 ++++++++++++++++++++++++++++++++ trailer.c | 1 + 2 files changed, 33 insertions(+) diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh index d7382709fc..f698d0c9ad 100755 --- a/t/t4201-shortlog.sh +++ b/t/t4201-shortlog.sh @@ -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 diff --git a/trailer.c b/trailer.c index e1d83390b6..f74915bd8c 100644 --- a/trailer.c +++ b/trailer.c @@ -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; } From 7b1c6aa541cc94796e9148e5e49c4d2f787e953d Mon Sep 17 00:00:00 2001 From: Linus Arver Date: Fri, 1 Mar 2024 00:14:40 +0000 Subject: [PATCH 3/9] trailer: rename functions to use 'trailer' 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. Helped-by: Junio C Hamano Helped-by: Christian Couder Signed-off-by: Linus Arver Signed-off-by: Junio C Hamano --- builtin/interpret-trailers.c | 4 ++-- trailer.c | 26 +++++++++++++------------- trailer.h | 6 +++--- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c index 033bd1556c..85a3413baf 100644 --- a/builtin/interpret-trailers.c +++ b/builtin/interpret-trailers.c @@ -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); diff --git a/trailer.c b/trailer.c index f74915bd8c..916175707d 100644 --- a/trailer.c +++ b/trailer.c @@ -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); diff --git a/trailer.h b/trailer.h index 1644cd05f6..37033e631a 100644 --- a/trailer.h +++ b/trailer.h @@ -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); From 0383dc5629dc4416b6564e5b458d174d770ef191 Mon Sep 17 00:00:00 2001 From: Linus Arver Date: Fri, 1 Mar 2024 00:14:42 +0000 Subject: [PATCH 4/9] trailer: reorder format_trailers_from_commit() parameters Currently there are two functions for formatting trailers in : 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). Similarly, reorder parameters for format_trailer_info(), because later on we will unify the two together. Signed-off-by: Linus Arver Signed-off-by: Junio C Hamano --- pretty.c | 2 +- ref-filter.c | 2 +- trailer.c | 11 ++++++----- trailer.h | 5 +++-- 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/pretty.c b/pretty.c index cf964b060c..bdbed4295a 100644 --- a/pretty.c +++ b/pretty.c @@ -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: diff --git a/ref-filter.c b/ref-filter.c index 35b989e1df..d358953b0c 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -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) diff --git a/trailer.c b/trailer.c index 916175707d..4a58005416 100644 --- a/trailer.c +++ b/trailer.c @@ -1176,10 +1176,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; @@ -1237,13 +1237,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); } diff --git a/trailer.h b/trailer.h index 37033e631a..027bf00282 100644 --- a/trailer.h +++ b/trailer.h @@ -101,8 +101,9 @@ void trailer_info_release(struct trailer_info *info); * 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 From ae0ec2e0e0bb26474f395504c6ed6fef3f59091f Mon Sep 17 00:00:00 2001 From: Linus Arver Date: Fri, 1 Mar 2024 00:14:41 +0000 Subject: [PATCH 5/9] trailer: move interpret_trailers() to interpret-trailers.c 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 . 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 so that interpret_trailers() (now in builtin/interpret-trailers.c) can continue calling them as a trailer API user. This enriches 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. Helped-by: Junio C Hamano Signed-off-by: Linus Arver Signed-off-by: Junio C Hamano --- builtin/interpret-trailers.c | 93 +++++++++++++++++++++++++++ trailer.c | 119 ++++------------------------------- trailer.h | 20 +++++- 3 files changed, 123 insertions(+), 109 deletions(-) diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c index 85a3413baf..d1cf0aa33a 100644 --- a/builtin/interpret-trailers.c +++ b/builtin/interpret-trailers.c @@ -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; diff --git a/trailer.c b/trailer.c index 4a58005416..5025be9789 100644 --- a/trailer.c +++ b/trailer.c @@ -5,7 +5,6 @@ #include "string-list.h" #include "run-command.h" #include "commit.h" -#include "tempfile.h" #include "trailer.h" #include "list.h" /* @@ -163,8 +162,8 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val) fprintf(outfile, "%s%c %s\n", tok, separators[0], val); } -static void format_trailers(const struct process_trailer_options *opts, - struct list_head *trailers, FILE *outfile) +void format_trailers(const struct process_trailer_options *opts, + struct list_head *trailers, FILE *outfile) { struct list_head *pos; struct trailer_item *item; @@ -366,8 +365,8 @@ static int find_same_and_apply_arg(struct list_head *head, return 0; } -static void process_trailers_lists(struct list_head *head, - struct list_head *arg_head) +void process_trailers_lists(struct list_head *head, + struct list_head *arg_head) { struct list_head *pos, *p; struct arg_item *arg_tok; @@ -589,7 +588,7 @@ static int git_trailer_config(const char *conf_key, const char *value, return 0; } -static void trailer_config_init(void) +void trailer_config_init(void) { if (configured) return; @@ -719,7 +718,7 @@ static void add_arg_item(struct list_head *arg_head, char *tok, char *val, list_add_tail(&new_item->list, arg_head); } -static void parse_trailers_from_config(struct list_head *config_head) +void parse_trailers_from_config(struct list_head *config_head) { struct arg_item *item; struct list_head *pos; @@ -735,8 +734,8 @@ static void parse_trailers_from_config(struct list_head *config_head) } } -static void parse_trailers_from_command_line_args(struct list_head *arg_head, - struct list_head *new_trailer_head) +void parse_trailers_from_command_line_args(struct list_head *arg_head, + struct list_head *new_trailer_head) { struct strbuf tok = STRBUF_INIT; struct strbuf val = STRBUF_INIT; @@ -775,17 +774,6 @@ static void parse_trailers_from_command_line_args(struct list_head *arg_head, free(cl_separators); } -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 const char *next_line(const char *str) { const char *nl = strchrnul(str, '\n'); @@ -1000,10 +988,10 @@ static void unfold_value(struct strbuf *val) * Parse trailers in "str", populating the trailer info and "head" * linked list structure. */ -static void parse_trailers(struct trailer_info *info, - const char *str, - struct list_head *head, - const struct process_trailer_options *opts) +void parse_trailers(const struct process_trailer_options *opts, + struct trailer_info *info, + const char *str, + struct list_head *head) { struct strbuf tok = STRBUF_INIT; struct strbuf val = STRBUF_INIT; @@ -1035,7 +1023,7 @@ static void parse_trailers(struct trailer_info *info, } } -static void free_trailers(struct list_head *trailers) +void free_trailers(struct list_head *trailers) { struct list_head *pos, *p; list_for_each_safe(pos, p, trailers) { @@ -1044,87 +1032,6 @@ static void free_trailers(struct list_head *trailers) } } -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; -} - -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(&info, sb.buf, &head, opts); - - /* Print the lines before the trailers */ - if (!opts->only_trailers) - fwrite(sb.buf, 1, info.trailer_block_start, outfile); - - if (!opts->only_trailers && !info.blank_line_before_trailer) - fprintf(outfile, "\n"); - - - if (!opts->only_input) { - LIST_HEAD(config_head); - LIST_HEAD(arg_head); - parse_trailers_from_config(&config_head); - parse_trailers_from_command_line_args(&arg_head, new_trailer_head); - list_splice(&config_head, &arg_head); - process_trailers_lists(&head, &arg_head); - } - - 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); -} - void trailer_info_get(struct trailer_info *info, const char *str, const struct process_trailer_options *opts) { diff --git a/trailer.h b/trailer.h index 027bf00282..c6d3ee49bb 100644 --- a/trailer.h +++ b/trailer.h @@ -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": From 9aa1b2bc890cea43f2b9aa3379ca88e98a17801f Mon Sep 17 00:00:00 2001 From: Linus Arver Date: Fri, 1 Mar 2024 00:14:43 +0000 Subject: [PATCH 6/9] trailer_info_get(): reorder parameters 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). Signed-off-by: Linus Arver Signed-off-by: Junio C Hamano --- sequencer.c | 2 +- trailer.c | 11 ++++++----- trailer.h | 5 +++-- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/sequencer.c b/sequencer.c index 3cc88d8a80..8e199fc8a4 100644 --- a/sequencer.c +++ b/sequencer.c @@ -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; diff --git a/trailer.c b/trailer.c index 5025be9789..f92d844361 100644 --- a/trailer.c +++ b/trailer.c @@ -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; } diff --git a/trailer.h b/trailer.h index c6d3ee49bb..410c61b62b 100644 --- a/trailer.h +++ b/trailer.h @@ -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); From bf35e0a018cf6d35834e762ac524754024800ad6 Mon Sep 17 00:00:00 2001 From: Linus Arver Date: Fri, 1 Mar 2024 00:14:44 +0000 Subject: [PATCH 7/9] format_trailers(): use strbuf instead of FILE 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). Signed-off-by: Linus Arver Signed-off-by: Junio C Hamano --- builtin/interpret-trailers.c | 6 +++++- trailer.c | 13 +++++++------ trailer.h | 3 ++- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c index d1cf0aa33a..11f4ce9e4a 100644 --- a/builtin/interpret-trailers.c +++ b/builtin/interpret-trailers.c @@ -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) diff --git a/trailer.c b/trailer.c index f92d844361..cbd643cd1f 100644 --- a/trailer.c +++ b/trailer.c @@ -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); } } diff --git a/trailer.h b/trailer.h index 410c61b62b..1d106b6dd4 100644 --- a/trailer.h +++ b/trailer.h @@ -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 *); /* From 2c948a78fd449a03c114802772acd4abfec25bba Mon Sep 17 00:00:00 2001 From: Linus Arver Date: Fri, 1 Mar 2024 00:14:45 +0000 Subject: [PATCH 8/9] format_trailer_info(): move "fast path" to caller 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(). Signed-off-by: Linus Arver Signed-off-by: Junio C Hamano --- trailer.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/trailer.c b/trailer.c index cbd643cd1f..e92d0154d9 100644 --- a/trailer.c +++ b/trailer.c @@ -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); } From 35ca4411a04a052e4365aa8a321491706156f7a0 Mon Sep 17 00:00:00 2001 From: Linus Arver Date: Fri, 1 Mar 2024 00:14:46 +0000 Subject: [PATCH 9/9] format_trailers_from_commit(): indirectly call trailer_info_get() This is another preparatory refactor to unify the trailer formatters. 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 (trailer_objects) instead of the `char **trailers` array. Signed-off-by: Linus Arver Signed-off-by: Junio C Hamano --- trailer.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/trailer.c b/trailer.c index e92d0154d9..798388cbf2 100644 --- a/trailer.c +++ b/trailer.c @@ -1140,9 +1140,11 @@ void format_trailers_from_commit(const struct process_trailer_options *opts, const char *msg, struct strbuf *out) { + LIST_HEAD(trailer_objects); struct trailer_info info; - trailer_info_get(opts, msg, &info); + 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 && !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(&trailer_objects); trailer_info_release(&info); }