Re: [PATCH 02/19] diff: move line ending check into emit_hunk_header

2017-05-14 Thread Junio C Hamano
Stefan Beller  writes:

> In a later patch, I want to propose an option to detect&color
> moved lines in a diff, which cannot be done in a one-pass over
> the diff. Instead we need to go over the whole diff twice,
> because we cannot detect the first line of the two corresponding
> lines (+ and -) that got moved.
>
> So to prepare the diff machinery for two pass algorithms
> (i.e. buffer it all up and then operate on the result),
> move all emissions to places, such that the only emitting
> function is emit_line_0.
>
> This patch moves code that is conceptually part of
> emit_hunk_header, but was using output in fn_out_consume,
> back to emit_hunk_header.

Makes sort-of sense.  If I were selling this patch, I'd remove the
first two paragraph and stress on how completing the line inside
emit_hunk_header() is conceptually cleaner than doing it outside.

emit_hunk_header() function is responsible for assembling a
hunk header and calling emit_line() to send the hunk header
to the output file.  Its only caller fn_out_consume() needs
to prepare for a case where the function emits an incomplete
line and add the terminating LF.  

Instead make sure emit_hunk_header() to always send a
completed line to emit_line().

or something like that.

Note that I am not saying "buffering the entire diff in-core?  why
should we support such a use case?".  I am saying that this change
is a clean-up that is justifiable, without having to answer such a
question.

>
> Meanwhile simplify it by using a function that is designed for it.
>
> Signed-off-by: Stefan Beller 
> ---
>  diff.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 3f5bf8b5a4..c2ed605cd0 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -677,6 +677,8 @@ static void emit_hunk_header(struct emit_callback 
> *ecbdata,
>   }
>  
>   strbuf_add(&msgbuf, line + len, org_len - len);
> + strbuf_complete_line(&msgbuf);
> +
>   emit_line(ecbdata->opt, "", "", msgbuf.buf, msgbuf.len);
>   strbuf_release(&msgbuf);
>  }
> @@ -1315,8 +1317,6 @@ static void fn_out_consume(void *priv, char *line, 
> unsigned long len)
>   len = sane_truncate_line(ecbdata, line, len);
>   find_lno(line, ecbdata);
>   emit_hunk_header(ecbdata, line, len);
> - if (line[len-1] != '\n')
> - putc('\n', o->file);
>   return;
>   }


Re: [PATCH 02/19] diff: move line ending check into emit_hunk_header

2017-05-15 Thread Stefan Beller
On Sun, May 14, 2017 at 11:48 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> In a later patch, I want to propose an option to detect&color
>> moved lines in a diff, which cannot be done in a one-pass over
>> the diff. Instead we need to go over the whole diff twice,
>> because we cannot detect the first line of the two corresponding
>> lines (+ and -) that got moved.
>>
>> So to prepare the diff machinery for two pass algorithms
>> (i.e. buffer it all up and then operate on the result),
>> move all emissions to places, such that the only emitting
>> function is emit_line_0.
>>
>> This patch moves code that is conceptually part of
>> emit_hunk_header, but was using output in fn_out_consume,
>> back to emit_hunk_header.
>
> Makes sort-of sense.  If I were selling this patch, I'd remove the
> first two paragraph and stress on how completing the line inside
> emit_hunk_header() is conceptually cleaner than doing it outside.
>
> emit_hunk_header() function is responsible for assembling a
> hunk header and calling emit_line() to send the hunk header
> to the output file.  Its only caller fn_out_consume() needs
> to prepare for a case where the function emits an incomplete
> line and add the terminating LF.
>
> Instead make sure emit_hunk_header() to always send a
> completed line to emit_line().
>
> or something like that.
>
> Note that I am not saying "buffering the entire diff in-core?  why
> should we support such a use case?".  I am saying that this change
> is a clean-up that is justifiable, without having to answer such a
> question.

Right, the first couple patches are more cleanup than preparation.
I considered sending them on their own, but then decided to rather
include it in this series.

I'll reword the commit message for a resend.

Thanks,
Stefan