Re: [PATCHv5 16/17] diff: buffer all output if asked to

From: Stefan Beller <sbeller@google.com>
Date: May 25, 2017, 05:34 UTC
Message-ID: <CAGZ79kZM5KPP7iDo7rT5gxsooRrftJrA=+3-WBXW3nqfXeTy4Q@mail.gmail.com>
In-Reply-To: <xmqqo9uhfz8g.fsf@gitster.mtv.corp.google.com>

On Wed, May 24, 2017 at 7:26 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 16 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> Introduce a new option 'use_buffer' in the struct diff_options which
>> controls whether all output is buffered up until all output is available.
>> ...
>> Unconditionally enable output via buffer in this patch as it yields
>> a great opportunity for testing, i.e. all the diff tests from the
>> test suite pass without having reordering issues (i.e. only parts
>> of the output got buffered, and we forgot to buffer other parts).
>> The test suite passes, which gives confidence that we converted all
>> functions to use emit_line for output.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> Oh, did I?

Yes, this is essentially the v4 with small indentation fixes, so I assumed your signoff is still valid.

Which leads to explaining my workflow (which I think we discussed that half a year ago with Dscho in a longer thread).

As soon as you apply a series I take that series and work off that series, because you explained you would do smaller fixups occasionally.

Patches that change drastically, I strip off your signoff and pretend like I created them from scratch, but for those which I barely touch (if at all), I do not remove your signoff, as I'd be just passing them along again.

Maybe I have to rethink that strategy.

Show 29 quoted lines

>
>> ---
>>  diff.c | 155 ++++++++++++++++++++++++++++++++++++++++++++++++++---------------
>>  diff.h |  41 +++++++++++++++++
>>  2 files changed, 161 insertions(+), 35 deletions(-)
>>
>> diff --git a/diff.c b/diff.c
>> index 514c5facd7..8e06206881 100644
>> --- a/diff.c
>> +++ b/diff.c
>> ...
>> @@ -2579,6 +2628,13 @@ static void builtin_diff(const char *name_a,
>>                       xecfg.ctxlen = strtoul(v, NULL, 10);
>>               if (o->word_diff)
>>                       init_diff_words_data(&ecbdata, o, one, two);
>> +             if (o->use_buffer) {
>> +                     struct diff_line e = diff_line_INIT;
>
> This ...
>
>> +                     e.state = DIFF_LINE_RELOAD_WS_RULE;
>> ...
>> +#define diff_line_INIT {NULL, NULL, NULL, 0, 0, 0}
>
> ... and this should be in all caps.   We do not say
>
>         struct strbuf buf = strbuf_INIT;
>
> and we should do the same for this new thing.

Yes. Totally agree. That is fallout from a mechanical replace all s/buffered_patch_line/diff_line/ and the case sensitivity got lost.

Will fix again.

Stefan