Re: [PATCH 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 7, 2014, 20:27 UTC
- Message-ID
- <xmqqzjc2eoyw.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <20141107185053.16854.5660.chriscool@tuxfamily.org>
Christian Couder <chriscool@tuxfamily.org> writes:
Show 23 quoted lines
> * 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...