From: Junio C Hamano <gitster@xxxxxxxxx>
Subject: Re: [PATCH 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines
Date: Fri, 07 Nov 2014 12:27:03 -0800
> Christian Couder <chriscool@xxxxxxxxxxxxx> writes:
>
>> * Copyright (c) 2013, 2014 Christian Couder <chriscool@xxxxxxxxxxxxx>
>> @@ -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...
I hope it is better now, as I encapsulated the call to
ignore_non_trailer() into a new find_trailer_end() function. There
were already find_trailer_start() and find_patch_start(), and I think
this way we could have a nice high level line oriented API.
Yeah, it won't be as efficient as using only one strbuf and only byte
oriented functions, and it looks much less manly too :-) But over time in
Git we have developed a number of less efficient but quite clean
abstractions like strbuf, argv_array, sha1_array and so on, that we
are quite happy with.
So yeah, with old age we might have lost some manliness and lament the
time when men were really men, and ... Well, let's wait for the
hopefully big party^W conference next year to do that together :-)
Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html