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

2 messages, 2 authors, 2017-05-15 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2017-05-15 06:48:32

Stefan Beller [off-list ref] 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.
quoted hunk
Meanwhile simplify it by using a function that is designed for it.

Signed-off-by: Stefan Beller <redacted>
---
 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

From: Stefan Beller <hidden>
Date: 2017-05-15 16:13:35

On Sun, May 14, 2017 at 11:48 PM, Junio C Hamano [off-list ref] wrote:
Stefan Beller [off-list ref] writes:
quoted
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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help