Re: [PATCH 3/4] trailer: have function to describe trailer layout
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 31, 2016, 22:53 UTC
- Message-ID
- <xmqqr36wqgep.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <d55ff281b88624cdbf5c21a56d817d8d17eff5c0.1477698917.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> static char *separators = ":"; > > +static int configured = 0;
Avoid initializing a static to 0 or NULL; instead let .bss take care of it.
Show 19 quoted lines
> static const char *token_from_item(struct arg_item *item, char *tok)
> {
> if (item->conf.key)
> @@ -872,59 +885,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);OK, it needs a reading of trailer_info_get() first to understand the remainder of this function. The function would grab these fields into info, among doing other things.
Show 7 quoted lines
> /* 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");
... and one of the "other things" include setting the ->blank_line_before_trailer field.
Show 13 quoted lines
> - /* 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)
> + char *trailer = info.trailers[i];
> + if (trailer[0] == comment_line_char)
> continue;And info.trailers[] is no longer an array of strbuf; it is an array of "char *", which I alluded to during my review of [2/4]. Looking good.
Show 10 quoted lines
> - 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);
> - continue;
> - }
> - separator_pos = find_separator(trailer->buf, separators);
> + separator_pos = find_separator(trailer, separators);... presumably, the line-folding is already handled in trailer_info_get(), so we do not need the "last" handling.
> if (separator_pos >= 1) {... and it it is a "mail-header: looking" one, then add it one way.
Show 13 quoted lines
> - parse_trailer(&tok, &val, NULL, trailer->buf,
> - separator_pos);
> - last = add_trailer_item(head,
> - strbuf_detach(&tok, NULL),
> - strbuf_detach(&val, NULL));
> + parse_trailer(&tok, &val, NULL, trailer,
> + separator_pos);
> + add_trailer_item(head,
> + strbuf_detach(&tok, NULL),
> + strbuf_detach(&val, NULL));
> } else {
> - strbuf_addbuf(&val, trailer);
> + strbuf_addstr(&val, trailer);... otherwise add it another way.
Show 14 quoted lines
> 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; > }
Nicely done.
Show 47 quoted lines
> @@ -1004,3 +999,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;
> +}OK, looking good.