From e4319562bc2834096fade432fd90c985b96476db Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Wed, 2 Nov 2016 10:29:16 -0700 Subject: trailer: be stricter in parsing separators Currently, a line is interpreted to be a trailer line if it contains a separator. Make parsing stricter by requiring the text on the left of the separator, if not the empty string, to be of the "" form. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano --- trailer.c | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/trailer.c b/trailer.c index f0ecde2d2f..eefe91d865 100644 --- a/trailer.c +++ b/trailer.c @@ -563,15 +563,32 @@ static int token_matches_item(const char *tok, struct arg_item *item, int tok_le } /* - * Return the location of the first separator in line, or -1 if there is no - * separator. + * If the given line is of the form + * "..." or "...", return the + * location of the separator. Otherwise, return -1. The optional whitespace + * is allowed there primarily to allow things like "Bug #43" where is + * "Bug" and is "#". + * + * The separator-starts-line case (in which this function returns 0) is + * distinguished from the non-well-formed-line case (in which this function + * returns -1) because some callers of this function need such a distinction. */ static int find_separator(const char *line, const char *separators) { - int loc = strcspn(line, separators); - if (!line[loc]) - return -1; - return loc; + int whitespace_found = 0; + const char *c; + for (c = line; *c; c++) { + if (strchr(separators, *c)) + return c - line; + if (!whitespace_found && (isalnum(*c) || *c == '-')) + continue; + if (c != line && (*c == ' ' || *c == '\t')) { + whitespace_found = 1; + continue; + } + break; + } + return -1; } /* -- cgit v1.2.3 From 710714aaa822acbad14f1b7100fa2776d2bc8ce6 Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Wed, 2 Nov 2016 10:29:17 -0700 Subject: commit: make ignore_non_trailer take buf/len Make ignore_non_trailer take a buf/len pair instead of struct strbuf. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano --- builtin/commit.c | 2 +- commit.c | 22 +++++++++++----------- commit.h | 2 +- trailer.c | 2 +- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 8976c3d29b..887ccc7577 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -790,7 +790,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, strbuf_stripspace(&sb, 0); if (signoff) - append_signoff(&sb, ignore_non_trailer(&sb), 0); + append_signoff(&sb, ignore_non_trailer(sb.buf, sb.len), 0); if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len) die_errno(_("could not write commit template")); diff --git a/commit.c b/commit.c index 856fd4aeef..2cf85158b4 100644 --- a/commit.c +++ b/commit.c @@ -1649,7 +1649,7 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len } /* - * Inspect sb and determine the true "end" of the log message, in + * Inspect the given string and determine the true "end" of the log message, in * order to find where to put a new Signed-off-by: line. Ignored are * trailing comment lines and blank lines, and also the traditional * "Conflicts:" block that is not commented out, so that we can use @@ -1659,37 +1659,37 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len * Returns the number of bytes from the tail to ignore, to be fed as * the second parameter to append_signoff(). */ -int ignore_non_trailer(struct strbuf *sb) +int ignore_non_trailer(const char *buf, size_t len) { int boc = 0; int bol = 0; int in_old_conflicts_block = 0; - while (bol < sb->len) { - char *next_line; + while (bol < len) { + const char *next_line = memchr(buf + bol, '\n', len - bol); - if (!(next_line = memchr(sb->buf + bol, '\n', sb->len - bol))) - next_line = sb->buf + sb->len; + if (!next_line) + next_line = buf + len; else next_line++; - if (sb->buf[bol] == comment_line_char || sb->buf[bol] == '\n') { + if (buf[bol] == comment_line_char || buf[bol] == '\n') { /* is this the first of the run of comments? */ if (!boc) boc = bol; /* otherwise, it is just continuing */ - } else if (starts_with(sb->buf + bol, "Conflicts:\n")) { + } else if (starts_with(buf + bol, "Conflicts:\n")) { in_old_conflicts_block = 1; if (!boc) boc = bol; - } else if (in_old_conflicts_block && sb->buf[bol] == '\t') { + } else if (in_old_conflicts_block && buf[bol] == '\t') { ; /* a pathname in the conflicts block */ } else if (boc) { /* the previous was not trailing comment */ boc = 0; in_old_conflicts_block = 0; } - bol = next_line - sb->buf; + bol = next_line - buf; } - return boc ? sb->len - boc : 0; + return boc ? len - boc : 0; } diff --git a/commit.h b/commit.h index afd14f318c..9c12abb911 100644 --- a/commit.h +++ b/commit.h @@ -355,7 +355,7 @@ extern const char *find_commit_header(const char *msg, const char *key, size_t *out_len); /* Find the end of the log message, the right place for a new trailer. */ -extern int ignore_non_trailer(struct strbuf *sb); +extern int ignore_non_trailer(const char *buf, size_t len); typedef void (*each_mergetag_fn)(struct commit *commit, struct commit_extra_header *extra, void *cb_data); diff --git a/trailer.c b/trailer.c index eefe91d865..9d101dbfbb 100644 --- a/trailer.c +++ b/trailer.c @@ -842,7 +842,7 @@ static int find_trailer_end(struct strbuf **lines, int patch_start) for (i = 0; i < patch_start; i++) strbuf_addbuf(&sb, lines[i]); - ignore_bytes = ignore_non_trailer(&sb); + ignore_bytes = ignore_non_trailer(sb.buf, sb.len); strbuf_release(&sb); for (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--) ignore_bytes -= lines[i]->len; -- cgit v1.2.3 From 022349c3b091f2aa047f1cd12b5409d564b25324 Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Wed, 2 Nov 2016 10:29:18 -0700 Subject: trailer: avoid unnecessary splitting on lines trailer.c currently splits lines while processing a buffer (and also rejoins lines when needing to invoke ignore_non_trailer). Avoid such line splitting, except when generating the strings corresponding to trailers (for ease of use by clients - a subsequent patch will allow other components to obtain the layout of a trailer block in a buffer, including the trailers themselves). The main purpose of this is to make it easy to return pointers into the original buffer (for a subsequent patch), but this also significantly reduces the number of memory allocations required. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano --- trailer.c | 194 ++++++++++++++++++++++++++++++++------------------------------ 1 file changed, 100 insertions(+), 94 deletions(-) diff --git a/trailer.c b/trailer.c index 9d101dbfbb..2faccd7e2f 100644 --- a/trailer.c +++ b/trailer.c @@ -102,12 +102,12 @@ static int same_trailer(struct trailer_item *a, struct arg_item *b) return same_token(a, b) && same_value(a, b); } -static inline int contains_only_spaces(const char *str) +static inline int is_blank_line(const char *str) { const char *s = str; - while (*s && isspace(*s)) + while (*s && *s != '\n' && isspace(*s)) s++; - return !*s; + return !*s || *s == '\n'; } static inline void strbuf_replace(struct strbuf *sb, const char *a, const char *b) @@ -702,51 +702,71 @@ static void process_command_line_args(struct list_head *arg_head, free(cl_separators); } -static struct strbuf **read_input_file(const char *file) +static void read_input_file(struct strbuf *sb, const char *file) { - struct strbuf **lines; - struct strbuf sb = STRBUF_INIT; - if (file) { - if (strbuf_read_file(&sb, file, 0) < 0) + 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) + if (strbuf_read(sb, fileno(stdin), 0) < 0) die_errno(_("could not read from stdin")); } +} - lines = strbuf_split(&sb, '\n'); +static const char *next_line(const char *str) +{ + const char *nl = strchrnul(str, '\n'); + return nl + !!*nl; +} - strbuf_release(&sb); +/* + * Return the position of the start of the last line. If len is 0, return -1. + */ +static int last_line(const char *buf, size_t len) +{ + int i; + if (len == 0) + return -1; + if (len == 1) + return 0; + /* + * Skip the last character (in addition to the null terminator), + * because if the last character is a newline, it is considered as part + * of the last line anyway. + */ + i = len - 2; - return lines; + for (; i >= 0; i--) { + if (buf[i] == '\n') + return i + 1; + } + return 0; } /* - * Return the (0 based) index of the start of the patch or the line - * count if there is no patch in the message. + * Return the position of the start of the patch or the length of str if there + * is no patch in the message. */ -static int find_patch_start(struct strbuf **lines, int count) +static int find_patch_start(const char *str) { - int i; + const char *s; - /* Get the start of the patch part if any */ - for (i = 0; i < count; i++) { - if (starts_with(lines[i]->buf, "---")) - return i; + for (s = str; *s; s = next_line(s)) { + if (starts_with(s, "---")) + return s - str; } - return count; + return s - str; } /* - * Return the (0 based) index of the first trailer line or count if - * there are no trailers. Trailers are searched only in the lines from - * index (count - 1) down to index 0. + * Return the position of the first trailer line or len if there are no + * trailers. */ -static int find_trailer_start(struct strbuf **lines, int count) +static int find_trailer_start(const char *buf, size_t len) { - int start, end_of_title, only_spaces = 1; + const char *s; + int end_of_title, l, only_spaces = 1; int recognized_prefix = 0, trailer_lines = 0, non_trailer_lines = 0; /* * Number of possible continuation lines encountered. This will be @@ -758,13 +778,13 @@ static int find_trailer_start(struct strbuf **lines, int count) int possible_continuation_lines = 0; /* The first paragraph is the title and cannot be trailers */ - for (start = 0; start < count; start++) { - if (lines[start]->buf[0] == comment_line_char) + for (s = buf; s < buf + len; s = next_line(s)) { + if (s[0] == comment_line_char) continue; - if (contains_only_spaces(lines[start]->buf)) + if (is_blank_line(s)) break; } - end_of_title = start; + end_of_title = s - buf; /* * Get the start of the trailers by looking starting from the end for a @@ -772,30 +792,33 @@ static int find_trailer_start(struct strbuf **lines, int count) * trailers, or (ii) contains at least one Git-generated trailer and * consists of at least 25% trailers. */ - for (start = count - 1; start >= end_of_title; start--) { + for (l = last_line(buf, len); + l >= end_of_title; + l = last_line(buf, l)) { + const char *bol = buf + l; const char **p; int separator_pos; - if (lines[start]->buf[0] == comment_line_char) { + if (bol[0] == comment_line_char) { non_trailer_lines += possible_continuation_lines; possible_continuation_lines = 0; continue; } - if (contains_only_spaces(lines[start]->buf)) { + if (is_blank_line(bol)) { if (only_spaces) continue; non_trailer_lines += possible_continuation_lines; if (recognized_prefix && trailer_lines * 3 >= non_trailer_lines) - return start + 1; - if (trailer_lines && !non_trailer_lines) - return start + 1; - return count; + return next_line(bol) - buf; + else if (trailer_lines && !non_trailer_lines) + return next_line(bol) - buf; + return len; } only_spaces = 0; for (p = git_generated_prefixes; *p; p++) { - if (starts_with(lines[start]->buf, *p)) { + if (starts_with(bol, *p)) { trailer_lines++; possible_continuation_lines = 0; recognized_prefix = 1; @@ -803,8 +826,8 @@ static int find_trailer_start(struct strbuf **lines, int count) } } - separator_pos = find_separator(lines[start]->buf, separators); - if (separator_pos >= 1 && !isspace(lines[start]->buf[0])) { + separator_pos = find_separator(bol, separators); + if (separator_pos >= 1 && !isspace(bol[0])) { struct list_head *pos; trailer_lines++; @@ -814,13 +837,13 @@ static int find_trailer_start(struct strbuf **lines, int count) list_for_each(pos, &conf_head) { struct arg_item *item; item = list_entry(pos, struct arg_item, list); - if (token_matches_item(lines[start]->buf, item, + if (token_matches_item(bol, item, separator_pos)) { recognized_prefix = 1; break; } } - } else if (isspace(lines[start]->buf[0])) + } else if (isspace(bol[0])) possible_continuation_lines++; else { non_trailer_lines++; @@ -831,88 +854,70 @@ continue_outer_loop: ; } - return count; -} - -/* Get the index of the end of the trailers */ -static int find_trailer_end(struct strbuf **lines, int patch_start) -{ - struct strbuf sb = STRBUF_INIT; - int i, ignore_bytes; - - for (i = 0; i < patch_start; i++) - strbuf_addbuf(&sb, lines[i]); - ignore_bytes = ignore_non_trailer(sb.buf, sb.len); - strbuf_release(&sb); - for (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--) - ignore_bytes -= lines[i]->len; - - return i + 1; + return len; } -static int has_blank_line_before(struct strbuf **lines, int start) +/* Return the position of the end of the trailers. */ +static int find_trailer_end(const char *buf, size_t len) { - for (;start >= 0; start--) { - if (lines[start]->buf[0] == comment_line_char) - continue; - return contains_only_spaces(lines[start]->buf); - } - return 0; + return len - ignore_non_trailer(buf, len); } -static void print_lines(FILE *outfile, struct strbuf **lines, int start, int end) +static int ends_with_blank_line(const char *buf, size_t len) { - int i; - for (i = start; lines[i] && i < end; i++) - fprintf(outfile, "%s", lines[i]->buf); + int ll = last_line(buf, len); + if (ll < 0) + return 0; + return is_blank_line(buf + ll); } static int process_input_file(FILE *outfile, - struct strbuf **lines, + const char *str, struct list_head *head) { - int count = 0; - int patch_start, trailer_start, trailer_end, i; + int patch_start, trailer_start, trailer_end; struct strbuf tok = STRBUF_INIT; struct strbuf val = STRBUF_INIT; struct trailer_item *last = NULL; + struct strbuf *trailer, **trailer_lines, **ptr; - /* Get the line count */ - while (lines[count]) - count++; - - patch_start = find_patch_start(lines, count); - trailer_end = find_trailer_end(lines, patch_start); - trailer_start = find_trailer_start(lines, trailer_end); + patch_start = find_patch_start(str); + trailer_end = find_trailer_end(str, patch_start); + trailer_start = find_trailer_start(str, trailer_end); /* Print lines before the trailers as is */ - print_lines(outfile, lines, 0, trailer_start); + fwrite(str, 1, trailer_start, outfile); - if (!has_blank_line_before(lines, trailer_start - 1)) + if (!ends_with_blank_line(str, trailer_start)) fprintf(outfile, "\n"); /* Parse trailer lines */ - for (i = trailer_start; i < trailer_end; i++) { + trailer_lines = strbuf_split_buf(str + trailer_start, + trailer_end - trailer_start, + '\n', + 0); + for (ptr = trailer_lines; *ptr; ptr++) { int separator_pos; - if (lines[i]->buf[0] == comment_line_char) + trailer = *ptr; + if (trailer->buf[0] == comment_line_char) continue; - if (last && isspace(lines[i]->buf[0])) { + if (last && isspace(trailer->buf[0])) { struct strbuf sb = STRBUF_INIT; - strbuf_addf(&sb, "%s\n%s", last->value, lines[i]->buf); + strbuf_addf(&sb, "%s\n%s", last->value, trailer->buf); strbuf_strip_suffix(&sb, "\n"); free(last->value); last->value = strbuf_detach(&sb, NULL); continue; } - separator_pos = find_separator(lines[i]->buf, separators); + separator_pos = find_separator(trailer->buf, separators); if (separator_pos >= 1) { - parse_trailer(&tok, &val, NULL, lines[i]->buf, + parse_trailer(&tok, &val, NULL, trailer->buf, separator_pos); last = add_trailer_item(head, strbuf_detach(&tok, NULL), strbuf_detach(&val, NULL)); } else { - strbuf_addbuf(&val, lines[i]); + strbuf_addbuf(&val, trailer); strbuf_strip_suffix(&val, "\n"); add_trailer_item(head, NULL, @@ -920,6 +925,7 @@ static int process_input_file(FILE *outfile, last = NULL; } } + strbuf_list_free(trailer_lines); return trailer_end; } @@ -968,7 +974,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str { LIST_HEAD(head); LIST_HEAD(arg_head); - struct strbuf **lines; + struct strbuf sb = STRBUF_INIT; int trailer_end; FILE *outfile = stdout; @@ -976,13 +982,13 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str git_config(git_trailer_default_config, NULL); git_config(git_trailer_config, NULL); - lines = read_input_file(file); + read_input_file(&sb, file); if (in_place) outfile = create_in_place_tempfile(file); /* Print the lines before the trailers */ - trailer_end = process_input_file(outfile, lines, &head); + trailer_end = process_input_file(outfile, sb.buf, &head); process_command_line_args(&arg_head, trailers); @@ -993,11 +999,11 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str free_all(&head); /* Print the lines after the trailers as is */ - print_lines(outfile, lines, trailer_end, INT_MAX); + fwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile); if (in_place) if (rename_tempfile(&trailers_tempfile, file)) die_errno(_("could not rename temporary file to %s"), file); - strbuf_list_free(lines); + strbuf_release(&sb); } -- cgit v1.2.3 From e8c352c316f31a3869f3ad1dae0e8b33042cbaf4 Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Wed, 2 Nov 2016 10:29:19 -0700 Subject: trailer: have function to describe trailer layout Create a function that, taking a string, describes the position of its trailer block (if available) and the contents thereof, and make trailer use it. This makes it easier for other Git components, in the future, to interpret trailer blocks in the same way as trailer. In a subsequent patch, another component will be made to use this. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano --- trailer.c | 118 +++++++++++++++++++++++++++++++++++++++++++------------------- trailer.h | 25 +++++++++++++ 2 files changed, 107 insertions(+), 36 deletions(-) diff --git a/trailer.c b/trailer.c index 2faccd7e2f..11f0b9fb40 100644 --- a/trailer.c +++ b/trailer.c @@ -46,6 +46,8 @@ static LIST_HEAD(conf_head); static char *separators = ":"; +static int configured; + #define TRAILER_ARG_STRING "$ARG" static const char *git_generated_prefixes[] = { @@ -546,6 +548,17 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb) return 0; } +static void ensure_configured(void) +{ + if (configured) + return; + + /* Default config must be setup first */ + git_config(git_trailer_default_config, NULL); + git_config(git_trailer_config, NULL); + configured = 1; +} + static const char *token_from_item(struct arg_item *item, char *tok) { if (item->conf.key) @@ -875,59 +888,43 @@ static int process_input_file(FILE *outfile, const char *str, struct list_head *head) { - int patch_start, trailer_start, trailer_end; + struct trailer_info info; struct strbuf tok = STRBUF_INIT; struct strbuf val = STRBUF_INIT; - struct trailer_item *last = NULL; - struct strbuf *trailer, **trailer_lines, **ptr; + int i; - patch_start = find_patch_start(str); - trailer_end = find_trailer_end(str, patch_start); - trailer_start = find_trailer_start(str, trailer_end); + trailer_info_get(&info, str); /* Print lines before the trailers as is */ - fwrite(str, 1, trailer_start, outfile); + fwrite(str, 1, info.trailer_start - str, outfile); - if (!ends_with_blank_line(str, trailer_start)) + if (!info.blank_line_before_trailer) fprintf(outfile, "\n"); - /* Parse trailer lines */ - trailer_lines = strbuf_split_buf(str + trailer_start, - trailer_end - trailer_start, - '\n', - 0); - for (ptr = trailer_lines; *ptr; ptr++) { + for (i = 0; i < info.trailer_nr; i++) { int separator_pos; - trailer = *ptr; - if (trailer->buf[0] == comment_line_char) - continue; - if (last && isspace(trailer->buf[0])) { - struct strbuf sb = STRBUF_INIT; - strbuf_addf(&sb, "%s\n%s", last->value, trailer->buf); - strbuf_strip_suffix(&sb, "\n"); - free(last->value); - last->value = strbuf_detach(&sb, NULL); + char *trailer = info.trailers[i]; + if (trailer[0] == comment_line_char) continue; - } - separator_pos = find_separator(trailer->buf, separators); + separator_pos = find_separator(trailer, separators); if (separator_pos >= 1) { - parse_trailer(&tok, &val, NULL, trailer->buf, + parse_trailer(&tok, &val, NULL, trailer, separator_pos); - last = add_trailer_item(head, - strbuf_detach(&tok, NULL), - strbuf_detach(&val, NULL)); + add_trailer_item(head, + strbuf_detach(&tok, NULL), + strbuf_detach(&val, NULL)); } else { - strbuf_addbuf(&val, trailer); + strbuf_addstr(&val, trailer); strbuf_strip_suffix(&val, "\n"); add_trailer_item(head, NULL, strbuf_detach(&val, NULL)); - last = NULL; } } - strbuf_list_free(trailer_lines); - return trailer_end; + trailer_info_release(&info); + + return info.trailer_end - str; } static void free_all(struct list_head *head) @@ -978,9 +975,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str int trailer_end; FILE *outfile = stdout; - /* Default config must be setup first */ - git_config(git_trailer_default_config, NULL); - git_config(git_trailer_config, NULL); + ensure_configured(); read_input_file(&sb, file); @@ -1007,3 +1002,54 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str strbuf_release(&sb); } + +void trailer_info_get(struct trailer_info *info, const char *str) +{ + int patch_start, trailer_end, trailer_start; + struct strbuf **trailer_lines, **ptr; + char **trailer_strings = NULL; + size_t nr = 0, alloc = 0; + char **last = NULL; + + ensure_configured(); + + patch_start = find_patch_start(str); + trailer_end = find_trailer_end(str, patch_start); + trailer_start = find_trailer_start(str, trailer_end); + + trailer_lines = strbuf_split_buf(str + trailer_start, + trailer_end - trailer_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_start); + info->trailer_start = str + trailer_start; + info->trailer_end = str + trailer_end; + info->trailers = trailer_strings; + info->trailer_nr = nr; +} + +void trailer_info_release(struct trailer_info *info) +{ + int i; + for (i = 0; i < info->trailer_nr; i++) + free(info->trailers[i]); + free(info->trailers); +} diff --git a/trailer.h b/trailer.h index 36b40b8176..65cc5d79c6 100644 --- a/trailer.h +++ b/trailer.h @@ -1,7 +1,32 @@ #ifndef TRAILER_H #define TRAILER_H +struct trailer_info { + /* + * True if there is a blank line before the location pointed to by + * trailer_start. + */ + 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. + */ + const char *trailer_start, *trailer_end; + + /* + * Array of trailers found. + */ + char **trailers; + size_t trailer_nr; +}; + void process_trailers(const char *file, int in_place, int trim_empty, struct string_list *trailers); +void trailer_info_get(struct trailer_info *info, const char *str); + +void trailer_info_release(struct trailer_info *info); + #endif /* TRAILER_H */ -- cgit v1.2.3 From 967dfd4d568c2b102281de8cc22ee35f7558358b Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Wed, 2 Nov 2016 10:29:20 -0700 Subject: sequencer: use trailer's trailer layout Make sequencer use trailer.c's trailer layout definition, as opposed to parsing the footer by itself. This makes "commit -s", "cherry-pick -x", and "format-patch --signoff" consistent with trailer, allowing non-trailer lines and multiple-line trailers in trailer blocks under certain conditions, and therefore suppressing the extra newline in those cases. Consistency with trailer extends to respecting trailer configs. Tests have been included to show that. Signed-off-by: Jonathan Tan Signed-off-by: Junio C Hamano --- sequencer.c | 75 +++++++++--------------------------------------- t/t3511-cherry-pick-x.sh | 16 +++++++++-- t/t4014-format-patch.sh | 37 ++++++++++++++++++++---- t/t7501-commit.sh | 36 +++++++++++++++++++++++ 4 files changed, 95 insertions(+), 69 deletions(-) diff --git a/sequencer.c b/sequencer.c index 5fd75f30dd..d64c97397b 100644 --- a/sequencer.c +++ b/sequencer.c @@ -16,6 +16,7 @@ #include "refs.h" #include "argv-array.h" #include "quote.h" +#include "trailer.h" #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION" @@ -56,30 +57,6 @@ static const char *get_todo_path(const struct replay_opts *opts) return git_path_todo_file(); } -static int is_rfc2822_line(const char *buf, int len) -{ - int i; - - for (i = 0; i < len; i++) { - int ch = buf[i]; - if (ch == ':') - return 1; - if (!isalnum(ch) && ch != '-') - break; - } - - return 0; -} - -static int is_cherry_picked_from_line(const char *buf, int len) -{ - /* - * We only care that it looks roughly like (cherry picked from ...) - */ - return len > strlen(cherry_picked_prefix) + 1 && - starts_with(buf, cherry_picked_prefix) && buf[len - 1] == ')'; -} - /* * Returns 0 for non-conforming footer * Returns 1 for conforming footer @@ -89,49 +66,25 @@ static int is_cherry_picked_from_line(const char *buf, int len) static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob, int ignore_footer) { - char prev; - int i, k; - int len = sb->len - ignore_footer; - const char *buf = sb->buf; - int found_sob = 0; - - /* footer must end with newline */ - if (!len || buf[len - 1] != '\n') - return 0; + struct trailer_info info; + int i; + int found_sob = 0, found_sob_last = 0; - prev = '\0'; - for (i = len - 1; i > 0; i--) { - char ch = buf[i]; - if (prev == '\n' && ch == '\n') /* paragraph break */ - break; - prev = ch; - } + trailer_info_get(&info, sb->buf); - /* require at least one blank line */ - if (prev != '\n' || buf[i] != '\n') + if (info.trailer_start == info.trailer_end) return 0; - /* advance to start of last paragraph */ - while (i < len - 1 && buf[i] == '\n') - i++; - - for (; i < len; i = k) { - int found_rfc2822; - - for (k = i; k < len && buf[k] != '\n'; k++) - ; /* do nothing */ - k++; + 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; + } - found_rfc2822 = is_rfc2822_line(buf + i, k - i - 1); - if (found_rfc2822 && sob && - !strncmp(buf + i, sob->buf, sob->len)) - found_sob = k; + trailer_info_release(&info); - if (!(found_rfc2822 || - is_cherry_picked_from_line(buf + i, k - i - 1))) - return 0; - } - if (found_sob == i) + if (found_sob_last) return 3; if (found_sob) return 2; diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh index 9cce5ae881..bf0a5c9887 100755 --- a/t/t3511-cherry-pick-x.sh +++ b/t/t3511-cherry-pick-x.sh @@ -25,9 +25,8 @@ Signed-off-by: B.U. Thor " mesg_broken_footer="$mesg_no_footer -The signed-off-by string should begin with the words Signed-off-by followed -by a colon and space, and then the signers name and email address. e.g. -Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" +This is not recognized as a footer because Myfooter is not a recognized token. +Myfooter: A.U. Thor " mesg_with_footer_sob="$mesg_with_footer Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" @@ -112,6 +111,17 @@ test_expect_success 'cherry-pick -s inserts blank line after non-conforming foot test_cmp expect actual ' +test_expect_success 'cherry-pick -s recognizes trailer config' ' + pristine_detach initial && + git -c "trailer.Myfooter.ifexists=add" cherry-pick -s mesg-broken-footer && + cat <<-EOF >expect && + $mesg_broken_footer + Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> + EOF + git log -1 --pretty=format:%B >actual && + test_cmp expect actual +' + test_expect_success 'cherry-pick -x inserts blank line when conforming footer not found' ' pristine_detach initial && sha1=$(git rev-parse mesg-no-footer^0) && diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh index ba4902df2b..482112ca33 100755 --- a/t/t4014-format-patch.sh +++ b/t/t4014-format-patch.sh @@ -1294,8 +1294,7 @@ EOF 4:Subject: [PATCH] subject 8: 10:Signed-off-by: example happens to be wrapped here. -11: -12:Signed-off-by: C O Mitter +11:Signed-off-by: C O Mitter EOF test_cmp expected actual ' @@ -1368,7 +1367,7 @@ EOF test_cmp expected actual ' -test_expect_success 'signoff: detect garbage in non-conforming footer' ' +test_expect_success 'signoff: tolerate garbage in conforming footer' ' append_signoff <<\EOF >actual && subject @@ -1383,8 +1382,36 @@ EOF 8: 10: 13:Signed-off-by: C O Mitter -14: -15:Signed-off-by: C O Mitter +EOF + test_cmp expected actual +' + +test_expect_success 'signoff: respect trailer config' ' + append_signoff <<\EOF >actual && +subject + +Myfooter: x +Some Trash +EOF + cat >expected <<\EOF && +4:Subject: [PATCH] subject +8: +11: +12:Signed-off-by: C O Mitter +EOF + test_cmp expected actual && + + test_config trailer.Myfooter.ifexists add && + append_signoff <<\EOF >actual && +subject + +Myfooter: x +Some Trash +EOF + cat >expected <<\EOF && +4:Subject: [PATCH] subject +8: +11:Signed-off-by: C O Mitter EOF test_cmp expected actual ' diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh index d84897a67a..4003a27e6a 100755 --- a/t/t7501-commit.sh +++ b/t/t7501-commit.sh @@ -460,6 +460,42 @@ $alt" && test_cmp expected actual ' +test_expect_success 'signoff respects trailer config' ' + + echo 5 >positive && + git add positive && + git commit -s -m "subject + +non-trailer line +Myfooter: x" && + git cat-file commit HEAD | sed -e "1,/^\$/d" > actual && + ( + echo subject + echo + echo non-trailer line + echo Myfooter: x + echo + echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" + ) >expected && + test_cmp expected actual && + + echo 6 >positive && + git add positive && + git -c "trailer.Myfooter.ifexists=add" commit -s -m "subject + +non-trailer line +Myfooter: x" && + git cat-file commit HEAD | sed -e "1,/^\$/d" > actual && + ( + echo subject + echo + echo non-trailer line + echo Myfooter: x + echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" + ) >expected && + test_cmp expected actual +' + test_expect_success 'multiple -m' ' >negative && -- cgit v1.2.3