[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH 3/4] trailer: have function to describe trailer layout
From:       Junio C Hamano <gitster () pobox ! com>
Date:       2016-10-31 22:53:34
Message-ID: xmqqr36wqgep.fsf () gitster ! mtv ! corp ! google ! com
[Download RAW message or body]

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.

>  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.

>  	/* 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.  

> -	/* 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.

> -		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.

> -			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.

>  			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.

> @@ -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.

[prev in list] [next in list] [prev in thread] [next in thread]