Markus Heidelberg <markus.heidelberg@xxxxxx> writes:
> I try to reword:
> With 2/3 and 3/3 I wanted to keep --color-words specific code in the
> block starting with
>
> if (ecbdata->diff_words) {
>
> and didn't want to contaminate the block starting with
>
> if (line[0] == '@') {
>
> with non-hunk-header content.
The contamination was already done long time ago. The way "color words"
mode piggybacks into the rest of the code is to let the line oriented diff
codepath run, capture the lines to its buffer so that it can split them
into words and compare, and hijack the control flow not to let the line
oriented diff to be output, and in the context of the original design,
Dscho's patch makes sense.
I do think that the way the "color words" logic has to touch line-oriented
codepaths unnecessarily makes it look intrusive; one reason is because it
exposes the state that is only interesting to the "color words" mode to
these places too much.
With a small helper function on top of Dscho's patch, I think the code can
be made a lot more readable. This way, "consume" codepath is made more
about "here is what we do when we get a line from the line-oriented diff
engine", and we can keep the knowledge of "color words" mode to the
minimum (no more than "here is where we may need to flush the buffer").
The helper hides the implementation details such as how we decide if we
have accumulated words or what we do when we do need to flush.
There is another large-ish "if (ecbdata->diff_words)" codeblock in
fn_out_consume() that peeks into *(ecbdata->diff_words); I think it should
be ejected from the main codepath for the same reason (i.e. readability).
.
Probably we can make a helper function that has only a single caller, like
this:
static void diff_words_use_line(char *line, unsigned long len,
struct emit_callback *ecbdata,
const char *plain, const char *reset)
{
if (line[0] == '-') {
diff_words_append(line, len,
&ecbdata->diff_words->minus);
return;
} else if (line[0] == '+') {
diff_words_append(line, len,
&ecbdata->diff_words->plus);
return;
}
diff_words_flush(ecbdata);
line++;
len--;
emit_line(ecbdata->file, plain, reset, line, len);
}
It would even be cleaner to change "diff_words_append()" function to do
all of the above.
But that is outside the scope of this comment.
diff.c | 26 ++++++++++++--------------
1 files changed, 12 insertions(+), 14 deletions(-)
diff --git a/diff.c b/diff.c
index b7ecfe3..8c66e4a 100644
--- a/diff.c
+++ b/diff.c
@@ -541,14 +541,18 @@ struct emit_callback {
FILE *file;
};
+/* In "color-words" mode, show word-diff of words accumulated in the buffer */
+static void diff_words_flush(struct emit_callback *ecbdata)
+{
+ if (ecbdata->diff_words->minus.text.size ||
+ ecbdata->diff_words->plus.text.size)
+ diff_words_show(ecbdata->diff_words);
+}
+
static void free_diff_words_data(struct emit_callback *ecbdata)
{
if (ecbdata->diff_words) {
- /* flush buffers */
- if (ecbdata->diff_words->minus.text.size ||
- ecbdata->diff_words->plus.text.size)
- diff_words_show(ecbdata->diff_words);
-
+ diff_words_flush(ecbdata);
free (ecbdata->diff_words->minus.text.ptr);
free (ecbdata->diff_words->minus.orig);
free (ecbdata->diff_words->plus.text.ptr);
@@ -656,12 +660,8 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
for (i = 0; i < len && line[i] == '@'; i++)
;
if (2 <= i && i < len && line[i] == ' ') {
- /* flush --color-words even for --unified=0 */
- if (ecbdata->diff_words &&
- (ecbdata->diff_words->minus.text.size ||
- ecbdata->diff_words->plus.text.size))
- diff_words_show(ecbdata->diff_words);
-
+ if (ecbdata->diff_words)
+ diff_words_flush(ecbdata);
ecbdata->nparents = i - 1;
len = sane_truncate_line(ecbdata, line, len);
emit_line(ecbdata->file,
@@ -691,9 +691,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
&ecbdata->diff_words->plus);
return;
}
- if (ecbdata->diff_words->minus.text.size ||
- ecbdata->diff_words->plus.text.size)
- diff_words_show(ecbdata->diff_words);
+ diff_words_flush(ecbdata);
line++;
len--;
emit_line(ecbdata->file, plain, reset, line, len);
--
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