Christian Couder [off-list ref] writes:
quoted hunk
* Copyright (c) 2013, 2014 Christian Couder [off-list ref]
@@ -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...
From: Junio C Hamano <redacted>
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 [off-list ref] writes:
quoted
* Copyright (c) 2013, 2014 Christian Couder [off-list ref]
@@ -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.