From 13211ae23f9126be81b3b483163bf963df4826aa Mon Sep 17 00:00:00 2001 From: Linus Arver Date: Sat, 9 Sep 2023 06:16:12 +0000 Subject: [PATCH 1/6] trailer: separate public from internal portion of trailer_iterator The fields here are not meant to be used by downstream callers, so put them behind an anonymous struct named as "internal" to warn against their use. This follows the pattern in 576de3d956 (unpack_trees: start splitting internal fields from public API, 2023-02-27). Signed-off-by: Linus Arver Signed-off-by: Junio C Hamano --- trailer.c | 10 +++++----- trailer.h | 6 ++++-- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/trailer.c b/trailer.c index f408f9b058..de4bdece84 100644 --- a/trailer.c +++ b/trailer.c @@ -1220,14 +1220,14 @@ 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->info, msg, &opts); - iter->cur = 0; + trailer_info_get(&iter->internal.info, msg, &opts); + iter->internal.cur = 0; } int trailer_iterator_advance(struct trailer_iterator *iter) { - while (iter->cur < iter->info.trailer_nr) { - char *trailer = iter->info.trailers[iter->cur++]; + 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) @@ -1245,7 +1245,7 @@ int trailer_iterator_advance(struct trailer_iterator *iter) void trailer_iterator_release(struct trailer_iterator *iter) { - trailer_info_release(&iter->info); + trailer_info_release(&iter->internal.info); strbuf_release(&iter->val); strbuf_release(&iter->key); } diff --git a/trailer.h b/trailer.h index 795d2fccfd..ab2cd01756 100644 --- a/trailer.h +++ b/trailer.h @@ -119,8 +119,10 @@ struct trailer_iterator { struct strbuf val; /* private */ - struct trailer_info info; - size_t cur; + struct { + struct trailer_info info; + size_t cur; + } internal; }; /* From c2a8edf997a8d9f3f005666f63105172a23bd3a0 Mon Sep 17 00:00:00 2001 From: Linus Arver Date: Sat, 9 Sep 2023 06:16:13 +0000 Subject: [PATCH 2/6] trailer: split process_input_file into separate pieces Currently, process_input_file does three things: (1) parse the input string for trailers, (2) print text before the trailers, and (3) calculate the position of the input where the trailers end. Rename this function to parse_trailers(), and make it only do (1). The caller of this function, process_trailers, becomes responsible for (2) and (3). These items belong inside process_trailers because they are both concerned with printing the surrounding text around trailers (which is already one of the immediate concerns of process_trailers). Signed-off-by: Linus Arver Signed-off-by: Junio C Hamano --- trailer.c | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/trailer.c b/trailer.c index de4bdece84..2c56cbc4a2 100644 --- a/trailer.c +++ b/trailer.c @@ -961,28 +961,24 @@ static void unfold_value(struct strbuf *val) strbuf_release(&out); } -static size_t process_input_file(FILE *outfile, - const char *str, - struct list_head *head, - const struct process_trailer_options *opts) +/* + * 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) { - struct trailer_info info; struct strbuf tok = STRBUF_INIT; struct strbuf val = STRBUF_INIT; size_t i; - trailer_info_get(&info, str, opts); + trailer_info_get(info, str, opts); - /* Print lines before the trailers as is */ - if (!opts->only_trailers) - fwrite(str, 1, info.trailer_start - str, outfile); - - if (!opts->only_trailers && !info.blank_line_before_trailer) - fprintf(outfile, "\n"); - - for (i = 0; i < info.trailer_nr; i++) { + for (i = 0; i < info->trailer_nr; i++) { int separator_pos; - char *trailer = info.trailers[i]; + char *trailer = info->trailers[i]; if (trailer[0] == comment_line_char) continue; separator_pos = find_separator(trailer, separators); @@ -1002,10 +998,6 @@ static size_t process_input_file(FILE *outfile, strbuf_detach(&val, NULL)); } } - - trailer_info_release(&info); - - return info.trailer_end - str; } static void free_all(struct list_head *head) @@ -1054,6 +1046,7 @@ void process_trailers(const char *file, { LIST_HEAD(head); struct strbuf sb = STRBUF_INIT; + struct trailer_info info; size_t trailer_end; FILE *outfile = stdout; @@ -1064,8 +1057,16 @@ void process_trailers(const char *file, if (opts->in_place) outfile = create_in_place_tempfile(file); + parse_trailers(&info, sb.buf, &head, opts); + trailer_end = info.trailer_end - sb.buf; + /* Print the lines before the trailers */ - trailer_end = process_input_file(outfile, sb.buf, &head, opts); + if (!opts->only_trailers) + fwrite(sb.buf, 1, info.trailer_start - sb.buf, outfile); + + if (!opts->only_trailers && !info.blank_line_before_trailer) + fprintf(outfile, "\n"); + if (!opts->only_input) { LIST_HEAD(arg_head); @@ -1076,6 +1077,7 @@ 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) From 94430d03df639ef49f178f1339ed48a31d84061f Mon Sep 17 00:00:00 2001 From: Linus Arver Date: Sat, 9 Sep 2023 06:16:14 +0000 Subject: [PATCH 3/6] trailer: split process_command_line_args into separate functions Previously, process_command_line_args did two things: (1) parse trailers from the configuration, and (2) parse trailers defined on the command line. Separate (1) outside to a new function, parse_trailers_from_config. Rename the remaining logic to parse_trailers_from_command_line_args. Signed-off-by: Linus Arver Signed-off-by: Junio C Hamano --- trailer.c | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/trailer.c b/trailer.c index 2c56cbc4a2..b6de5d9cb2 100644 --- a/trailer.c +++ b/trailer.c @@ -711,10 +711,25 @@ static void add_arg_item(struct list_head *arg_head, char *tok, char *val, list_add_tail(&new_item->list, arg_head); } -static void process_command_line_args(struct list_head *arg_head, - struct list_head *new_trailer_head) +static void parse_trailers_from_config(struct list_head *config_head) { struct arg_item *item; + struct list_head *pos; + + /* Add an arg item for each configured trailer with a command */ + list_for_each(pos, &conf_head) { + item = list_entry(pos, struct arg_item, list); + if (item->conf.command) + add_arg_item(config_head, + xstrdup(token_from_item(item, NULL)), + xstrdup(""), + &item->conf, NULL); + } +} + +static void parse_trailers_from_command_line_args(struct list_head *arg_head, + struct list_head *new_trailer_head) +{ struct strbuf tok = STRBUF_INIT; struct strbuf val = STRBUF_INIT; const struct conf_info *conf; @@ -726,16 +741,6 @@ static void process_command_line_args(struct list_head *arg_head, */ char *cl_separators = xstrfmt("=%s", separators); - /* Add an arg item for each configured trailer with a command */ - list_for_each(pos, &conf_head) { - item = list_entry(pos, struct arg_item, list); - if (item->conf.command) - add_arg_item(arg_head, - xstrdup(token_from_item(item, NULL)), - xstrdup(""), - &item->conf, NULL); - } - /* Add an arg item for each trailer on the command line */ list_for_each(pos, new_trailer_head) { struct new_trailer_item *tr = @@ -1069,8 +1074,11 @@ void process_trailers(const char *file, if (!opts->only_input) { + LIST_HEAD(config_head); LIST_HEAD(arg_head); - process_command_line_args(&arg_head, new_trailer_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); } From ee8c5ee08cee9aee6732ab315e94bc88631c7431 Mon Sep 17 00:00:00 2001 From: Linus Arver Date: Sat, 9 Sep 2023 06:16:15 +0000 Subject: [PATCH 4/6] trailer: teach find_patch_start about --no-divider Currently, find_patch_start only finds the start of the patch part of the input (by looking at the "---" divider) for cases where the "--no-divider" flag has not been provided. If the user provides this flag, we do not rely on find_patch_start at all and just call strlen() directly on the input. Instead, make find_patch_start aware of "--no-divider" and make it handle that case as well. This means we no longer need to call strlen at all and can just rely on the existing code in find_patch_start. By forcing callers to consider this important option, we avoid the kind of mistake described in be3d654343 (commit: pass --no-divider to interpret-trailers, 2023-06-17). This patch will make unit testing a bit more pleasant in this area in the future when we adopt a unit testing framework, because we would not have to test multiple functions to check how finding the start of a patch part works (we would only need to test find_patch_start). Signed-off-by: Linus Arver Signed-off-by: Junio C Hamano --- trailer.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/trailer.c b/trailer.c index b6de5d9cb2..f646e484a2 100644 --- a/trailer.c +++ b/trailer.c @@ -812,14 +812,14 @@ static ssize_t last_line(const char *buf, size_t len) * Return the position of the start of the patch or the length of str if there * is no patch in the message. */ -static size_t find_patch_start(const char *str) +static size_t find_patch_start(const char *str, int no_divider) { const char *s; for (s = str; *s; s = next_line(s)) { const char *v; - if (skip_prefix(s, "---", &v) && isspace(*v)) + if (!no_divider && skip_prefix(s, "---", &v) && isspace(*v)) return s - str; } @@ -1109,11 +1109,7 @@ void trailer_info_get(struct trailer_info *info, const char *str, ensure_configured(); - if (opts->no_divider) - patch_start = strlen(str); - else - patch_start = find_patch_start(str); - + patch_start = find_patch_start(str, opts->no_divider); trailer_end = find_trailer_end(str, patch_start); trailer_start = find_trailer_start(str, trailer_end); From d2be104085b24867e3dd9cb061c75b456071b351 Mon Sep 17 00:00:00 2001 From: Linus Arver Date: Sat, 9 Sep 2023 06:16:16 +0000 Subject: [PATCH 5/6] trailer: rename *_DEFAULT enums to *_UNSPECIFIED Do not use *_DEFAULT as a suffix to the enums, because the word "default" is overloaded. The following are two examples of the ambiguity of the word "default": (1) "Default" can mean using the "default" values that are hardcoded in trailer.c as default_conf_info.where = WHERE_END; default_conf_info.if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR; default_conf_info.if_missing = MISSING_ADD; in ensure_configured(). These values are referred to as "the default" in the docs for interpret-trailers. These defaults are used if no "trailer.*" configurations are defined. (2) "Default" can also mean the "trailer.*" configurations themselves, because these configurations are used by "default" (ahead of the hardcoded defaults in (1)) if no command line arguments are provided. This concept of defaulting back to the configurations was introduced in 0ea5292e6b (interpret-trailers: add options for actions, 2017-08-01). In addition, the corresponding *_DEFAULT values are chosen when the user provides the "--no-where", "--no-if-exists", or "--no-if-missing" flags on the command line. These "--no-*" flags are used to clear previously provided flags of the form "--where", "--if-exists", and "--if-missing". Using these "--no-*" flags undoes the specifying of these flags (if any), so using the word "UNSPECIFIED" is more natural here. So instead of using "*_DEFAULT", use "*_UNSPECIFIED" because this signals to the reader that the *_UNSPECIFIED value by itself carries no meaning (it's a zero value and by itself does not "default" to anything, necessitating the need to have some other way of getting to a useful value). Signed-off-by: Linus Arver Signed-off-by: Junio C Hamano --- trailer.c | 17 ++++++++++------- trailer.h | 6 +++--- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/trailer.c b/trailer.c index f646e484a2..6ad2fbca94 100644 --- a/trailer.c +++ b/trailer.c @@ -388,7 +388,7 @@ static void process_trailers_lists(struct list_head *head, int trailer_set_where(enum trailer_where *item, const char *value) { if (!value) - *item = WHERE_DEFAULT; + *item = WHERE_UNSPECIFIED; else if (!strcasecmp("after", value)) *item = WHERE_AFTER; else if (!strcasecmp("before", value)) @@ -405,7 +405,7 @@ int trailer_set_where(enum trailer_where *item, const char *value) int trailer_set_if_exists(enum trailer_if_exists *item, const char *value) { if (!value) - *item = EXISTS_DEFAULT; + *item = EXISTS_UNSPECIFIED; else if (!strcasecmp("addIfDifferent", value)) *item = EXISTS_ADD_IF_DIFFERENT; else if (!strcasecmp("addIfDifferentNeighbor", value)) @@ -424,7 +424,7 @@ 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) { if (!value) - *item = MISSING_DEFAULT; + *item = MISSING_UNSPECIFIED; else if (!strcasecmp("doNothing", value)) *item = MISSING_DO_NOTHING; else if (!strcasecmp("add", value)) @@ -586,7 +586,10 @@ static void ensure_configured(void) if (configured) return; - /* Default config must be setup first */ + /* + * Default config must be setup first. These defaults are used if there + * are no "trailer.*" or "trailer..*" options configured. + */ default_conf_info.where = WHERE_END; default_conf_info.if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR; default_conf_info.if_missing = MISSING_ADD; @@ -701,11 +704,11 @@ static void add_arg_item(struct list_head *arg_head, char *tok, char *val, new_item->value = val; duplicate_conf(&new_item->conf, conf); if (new_trailer_item) { - if (new_trailer_item->where != WHERE_DEFAULT) + if (new_trailer_item->where != WHERE_UNSPECIFIED) new_item->conf.where = new_trailer_item->where; - if (new_trailer_item->if_exists != EXISTS_DEFAULT) + if (new_trailer_item->if_exists != EXISTS_UNSPECIFIED) new_item->conf.if_exists = new_trailer_item->if_exists; - if (new_trailer_item->if_missing != MISSING_DEFAULT) + if (new_trailer_item->if_missing != MISSING_UNSPECIFIED) new_item->conf.if_missing = new_trailer_item->if_missing; } list_add_tail(&new_item->list, arg_head); diff --git a/trailer.h b/trailer.h index ab2cd01756..a689d768c7 100644 --- a/trailer.h +++ b/trailer.h @@ -5,14 +5,14 @@ #include "strbuf.h" enum trailer_where { - WHERE_DEFAULT, + WHERE_UNSPECIFIED, WHERE_END, WHERE_AFTER, WHERE_BEFORE, WHERE_START }; enum trailer_if_exists { - EXISTS_DEFAULT, + EXISTS_UNSPECIFIED, EXISTS_ADD_IF_DIFFERENT_NEIGHBOR, EXISTS_ADD_IF_DIFFERENT, EXISTS_ADD, @@ -20,7 +20,7 @@ enum trailer_if_exists { EXISTS_DO_NOTHING }; enum trailer_if_missing { - MISSING_DEFAULT, + MISSING_UNSPECIFIED, MISSING_ADD, MISSING_DO_NOTHING }; From b5e75f87b5881811a7eee8ea6aa10cd43f1fe430 Mon Sep 17 00:00:00 2001 From: Linus Arver Date: Sat, 9 Sep 2023 06:16:17 +0000 Subject: [PATCH 6/6] trailer: use offsets for trailer_start/trailer_end Previously these fields in the trailer_info struct were of type "const char *" and pointed to positions in the input string directly (to the start and end positions of the trailer block). Use offsets to make the intended usage less ambiguous. We only need to reference the input string in format_trailer_info(), so update that function to take a pointer to the input. Signed-off-by: Linus Arver Signed-off-by: Junio C Hamano --- trailer.c | 17 ++++++++--------- trailer.h | 7 +++---- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/trailer.c b/trailer.c index 6ad2fbca94..00326720e8 100644 --- a/trailer.c +++ b/trailer.c @@ -1055,7 +1055,6 @@ void process_trailers(const char *file, LIST_HEAD(head); struct strbuf sb = STRBUF_INIT; struct trailer_info info; - size_t trailer_end; FILE *outfile = stdout; ensure_configured(); @@ -1066,11 +1065,10 @@ void process_trailers(const char *file, outfile = create_in_place_tempfile(file); parse_trailers(&info, sb.buf, &head, opts); - trailer_end = info.trailer_end - sb.buf; /* Print the lines before the trailers */ if (!opts->only_trailers) - fwrite(sb.buf, 1, info.trailer_start - sb.buf, outfile); + fwrite(sb.buf, 1, info.trailer_start, outfile); if (!opts->only_trailers && !info.blank_line_before_trailer) fprintf(outfile, "\n"); @@ -1092,7 +1090,7 @@ void process_trailers(const char *file, /* Print the lines after the trailers as is */ if (!opts->only_trailers) - fwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile); + fwrite(sb.buf + info.trailer_end, 1, sb.len - info.trailer_end, outfile); if (opts->in_place) if (rename_tempfile(&trailers_tempfile, file)) @@ -1104,7 +1102,7 @@ void process_trailers(const char *file, void trailer_info_get(struct trailer_info *info, const char *str, const struct process_trailer_options *opts) { - int patch_start, trailer_end, trailer_start; + size_t patch_start, trailer_end = 0, trailer_start = 0; struct strbuf **trailer_lines, **ptr; char **trailer_strings = NULL; size_t nr = 0, alloc = 0; @@ -1139,8 +1137,8 @@ void trailer_info_get(struct trailer_info *info, const char *str, info->blank_line_before_trailer = ends_with_blank_line(str, trailer_start); - info->trailer_start = str + trailer_start; - info->trailer_end = str + trailer_end; + info->trailer_start = trailer_start; + info->trailer_end = trailer_end; info->trailers = trailer_strings; info->trailer_nr = nr; } @@ -1155,6 +1153,7 @@ void trailer_info_release(struct trailer_info *info) static void format_trailer_info(struct strbuf *out, const struct trailer_info *info, + const char *msg, const struct process_trailer_options *opts) { size_t origlen = out->len; @@ -1164,7 +1163,7 @@ static void format_trailer_info(struct strbuf *out, if (!opts->only_trailers && !opts->unfold && !opts->filter && !opts->separator && !opts->key_only && !opts->value_only && !opts->key_value_separator) { - strbuf_add(out, info->trailer_start, + strbuf_add(out, msg + info->trailer_start, info->trailer_end - info->trailer_start); return; } @@ -1219,7 +1218,7 @@ void format_trailers_from_commit(struct strbuf *out, const char *msg, struct trailer_info info; trailer_info_get(&info, msg, opts); - format_trailer_info(out, &info, opts); + format_trailer_info(out, &info, msg, opts); trailer_info_release(&info); } diff --git a/trailer.h b/trailer.h index a689d768c7..13fbf0dcd1 100644 --- a/trailer.h +++ b/trailer.h @@ -37,11 +37,10 @@ struct trailer_info { int blank_line_before_trailer; /* - * Pointers to the start and end of the trailer block found. If there - * is no trailer block found, these 2 pointers point to the end of the - * input string. + * Offsets to the trailer block start and end positions in the input + * string. If no trailer block is found, these are set to 0. */ - const char *trailer_start, *trailer_end; + size_t trailer_start, trailer_end; /* * Array of trailers found.