Re: [PATCH 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines

[Junio C Hamano <gitster@pobox.com>]

Christian Couder <chriscool@tuxfamily.org> writes:

>   * Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>
> @@ -791,14 +792,24 @@ static int process_input_file(struct strbuf **lines,
>  			      struct trailer_item **in_tok_last)
>  {
>  	int count = 0;
> -	int patch_start, trailer_start, i;
> +	int trailer_start, trailer_end, patch_start, ignore_bytes, i;
> +	struct strbuf sb;
>  
>  	/* Get the line count */
>  	while (lines[count])
>  		count++;
>  
>  	patch_start = find_patch_start(lines, count);
> -	trailer_start = find_trailer_start(lines, patch_start);
> +
> +	/* Get the index of the end of the trailers */
> +	for (i = 0; i < patch_start; i++)
> +		strbuf_addbuf(&sb, lines[i]);
> +	ignore_bytes = ignore_non_trailer(&sb);
> +	for (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)
> +		ignore_bytes -= lines[i]->len;
> +	trailer_end = i + 1;

Looks like there is an impedance mismatch between the caller and the
callee here.  I can sort of understand why you might want to have an
array of trailer items, one element per line in the trailer block,
as that would make it easier on your brain when you have to reorder
them, insert a new one or a remove an existing one, but you seem to
be keeping _everything_ in that format, an array of strbuf with one
strbuf per line, which sounds really wasteful.  The data structure
might need to be rethoughtbefore this code becomes ready for
production.

I would have expected it to be more like (1) slurp everything in a
single strbuf, (2) find the trailer block inside that single strbuf,
splitting what you read in the previous step into one strbuf for
stuff before the trailer block, another strbuf for stuff after the
trailer block and an array of lines in the tailer block, (3) do
whatever your trailer flipping logic inside that array without
having to worry about stuff before or after the trailer block and
then finally (4) spit the whole thing out by concatenating the stuff
before the trailer block, the stuff in the trailer block and the
stuff after the trailer block.

Oh well...