Re: [PATCH v2 4/5] Make boundary characters for --color-words configurable

Johannes Schindelin [off-list ref] writes:

I am rather interested in the semantics, i.e. if you can punch holes into 
this 3-class approach.

This is not the 3-class thing, but was done as a lunchtime hack.  It
removes more lines than it adds, with real comments ;-).

 * It removes the custom allocator used for minus/plus buffers and
   replaces it with the bog-standard strbuf;

 * The tokenization is done when diff_words_append() is called, i.e. when
   we read the original "added or deleted _lines_";

 * The tokenization function is separated out, and gets the emit_callback,
   so anybody can enhance it with customization using gitattributes and
   other heuristics.  More importantly, it is not byte oriented and would
   be easier to extend it to UTF-8 contents;

 * It does not have to play "suppressed_newline" games anymore.  A LF is
   just a token.

I haven't tested this at all (this is a lunchtime hack) and have a mild
suspicion that it may have corner case miscounting (e.g. I blindly
subtracts 3 from len when dealing with a line that represents a single
token from the internal diff output --- do I always have 3 there even when
the original file ends with an incomplete line?  I didn't check), but
other than that I think this is a lot easier to read and follow.

---

 diff.c |  216 +++++++++++++++++++++++++++++++--------------------------------
 1 files changed, 106 insertions(+), 110 deletions(-)

diff --git a/diff.c b/diff.c
index e35384b..344aaa6 100644
--- a/diff.c
+++ b/diff.c

@@ -351,87 +351,119 @@ static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)
 	return 0;
 }
 
-struct diff_words_buffer {
-	mmfile_t text;
-	long alloc;
-	long current; /* output pointer */
-	int suppressed_newline;
+typedef unsigned long (*sane_truncate_fn)(char *line, unsigned long len);
+
+struct emit_callback {
+	struct xdiff_emit_state xm;
+	int nparents, color_diff;
+	unsigned ws_rule;
+	sane_truncate_fn truncate;
+	const char **label_path;
+	struct diff_words_data *diff_words;
+	int *found_changesp;
+	FILE *file;
 };
 
-static void diff_words_append(char *line, unsigned long len,
-		struct diff_words_buffer *buffer)
+static size_t diff_words_tokenize(struct emit_callback *ecbdata,
+				  char *line, unsigned long len)
 {
-	if (buffer->text.size + len > buffer->alloc) {
-		buffer->alloc = (buffer->text.size + len) * 3 / 2;
-		buffer->text.ptr = xrealloc(buffer->text.ptr, buffer->alloc);
+	/*
+	 * This function currently is deliberately done very stupid,
+	 * but passing ecbdata here means that you can potentially
+	 * implement different tokenization rules depending on
+	 * the content (e.g. "gitattributes(5)").
+	 */
+	int is_space;
+	char *line0 = line;
+
+	if (!len)
+		return 0;
+
+	is_space = isspace(*line);
+	while (len && (isspace(*line) == is_space)) {
+		line++;
+		len--;
 	}
+	return line - line0;
+}
+
+static void diff_words_append(struct emit_callback *ecbdata,
+			      char *line, unsigned long len,
+			      struct strbuf *text)
+{
+	/* Skip leading +/- first. */
 	line++;
 	len--;
-	memcpy(buffer->text.ptr + buffer->text.size, line, len);
-	buffer->text.size += len;
+
+	/*
+	 * Tokenize and stuff the words in.
+	 */
+	while (len) {
+		size_t token_len = diff_words_tokenize(ecbdata, line, len);
+
+		if (line[0] != '\n') {
+			/*
+			 * A nonempty token has ' ' stuffed in front,
+			 * so that we can recover the original
+			 * end-of-line easily.  Stupid, but works.
+			 */
+			strbuf_add(text, " ", 1);
+			strbuf_add(text, line, token_len);
+			strbuf_add(text, "\n", 1);
+			len -= token_len;
+			line += token_len;
+		} else {
+			/* A real LF */
+			strbuf_add(text, "\n", 1);
+			break;
+		}
+	}
 }
 
 struct diff_words_data {
 	struct xdiff_emit_state xm;
-	struct diff_words_buffer minus, plus;
+	struct strbuf minus;
+	struct strbuf plus;
 	FILE *file;
 };
 
-static void print_word(FILE *file, struct diff_words_buffer *buffer, int len, int color,
-		int suppress_newline)
+static void emit_line(FILE *file, const char *set, const char *reset, const char *line, int len)
 {
-	const char *ptr;
-	int eol = 0;
-
-	if (len == 0)
-		return;
-
-	ptr  = buffer->text.ptr + buffer->current;
-	buffer->current += len;
-
-	if (ptr[len - 1] == '\n') {
-		eol = 1;
-		len--;
-	}
-
-	fputs(diff_get_color(1, color), file);
-	fwrite(ptr, len, 1, file);
-	fputs(diff_get_color(1, DIFF_RESET), file);
-
-	if (eol) {
-		if (suppress_newline)
-			buffer->suppressed_newline = 1;
-		else
-			putc('\n', file);
-	}
+	fputs(set, file);
+	fwrite(line, len, 1, file);
+	fputs(reset, file);
 }
 
 static void fn_out_diff_words_aux(void *priv, char *line, unsigned long len)
 {
 	struct diff_words_data *diff_words = priv;
+	const char *set;
+	const char *reset = diff_colors[DIFF_RESET];
 
-	if (diff_words->minus.suppressed_newline) {
-		if (line[0] != '+')
-			putc('\n', diff_words->file);
-		diff_words->minus.suppressed_newline = 0;
+	switch (line[0]) {
+	case '-':
+		set = diff_colors[DIFF_FILE_OLD];
+		break;
+	case '+':
+		set = diff_colors[DIFF_FILE_NEW];
+		break;
+	case ' ':
+		set = diff_colors[DIFF_PLAIN];
+		break;
+	default:
+		return; /* omit @@ -j,k +l,m @@ header */
 	}
 
-	len--;
-	switch (line[0]) {
-		case '-':
-			print_word(diff_words->file,
-				   &diff_words->minus, len, DIFF_FILE_OLD, 1);
-			break;
-		case '+':
-			print_word(diff_words->file,
-				   &diff_words->plus, len, DIFF_FILE_NEW, 0);
-			break;
-		case ' ':
-			print_word(diff_words->file,
-				   &diff_words->plus, len, DIFF_PLAIN, 0);
-			diff_words->minus.current += len;
-			break;
+	if (line[1] == ' ') {
+		/* A token */
+		line += 2;
+		len -= 3; /* drop the trailing LF */
+	} else {
+		/* A real LF */
+		line++;
+		len--;
 	}
+	emit_line(diff_words->file, set, reset, line, len);
 }
 
 /* this executes the word diff on the accumulated buffers */

@@ -441,27 +473,18 @@ static void diff_words_show(struct diff_words_data *diff_words)
 	xdemitconf_t xecfg;
 	xdemitcb_t ecb;
 	mmfile_t minus, plus;
-	int i;
+	unsigned long sz;
 
 	memset(&xecfg, 0, sizeof(xecfg));
-	minus.size = diff_words->minus.text.size;
-	minus.ptr = xmalloc(minus.size);
-	memcpy(minus.ptr, diff_words->minus.text.ptr, minus.size);
-	for (i = 0; i < minus.size; i++)
-		if (isspace(minus.ptr[i]))
-			minus.ptr[i] = '\n';
-	diff_words->minus.current = 0;
-
-	plus.size = diff_words->plus.text.size;
-	plus.ptr = xmalloc(plus.size);
-	memcpy(plus.ptr, diff_words->plus.text.ptr, plus.size);
-	for (i = 0; i < plus.size; i++)
-		if (isspace(plus.ptr[i]))
-			plus.ptr[i] = '\n';
-	diff_words->plus.current = 0;
+
+	minus.ptr = strbuf_detach(&diff_words->minus, &sz);
+	minus.size = sz;
+	plus.ptr = strbuf_detach(&diff_words->plus, &sz);
+	plus.size = sz;
 
 	xpp.flags = XDF_NEED_MINIMAL;
-	xecfg.ctxlen = diff_words->minus.alloc + diff_words->plus.alloc;
+	/* hack to make it a single hunk to show all */
+	xecfg.ctxlen = minus.size + plus.size;
 	ecb.outf = xdiff_outf;
 	ecb.priv = diff_words;
 	diff_words->xm.consume = fn_out_diff_words_aux;

@@ -469,37 +492,15 @@ static void diff_words_show(struct diff_words_data *diff_words)
 
 	free(minus.ptr);
 	free(plus.ptr);
-	diff_words->minus.text.size = diff_words->plus.text.size = 0;
-
-	if (diff_words->minus.suppressed_newline) {
-		putc('\n', diff_words->file);
-		diff_words->minus.suppressed_newline = 0;
-	}
 }
 
-typedef unsigned long (*sane_truncate_fn)(char *line, unsigned long len);
-
-struct emit_callback {
-	struct xdiff_emit_state xm;
-	int nparents, color_diff;
-	unsigned ws_rule;
-	sane_truncate_fn truncate;
-	const char **label_path;
-	struct diff_words_data *diff_words;
-	int *found_changesp;
-	FILE *file;
-};
-
 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)
+		if (ecbdata->diff_words->minus.len ||
+		    ecbdata->diff_words->plus.len)
 			diff_words_show(ecbdata->diff_words);
-
-		free (ecbdata->diff_words->minus.text.ptr);
-		free (ecbdata->diff_words->plus.text.ptr);
 		free(ecbdata->diff_words);
 		ecbdata->diff_words = NULL;
 	}

@@ -512,13 +513,6 @@ const char *diff_get_color(int diff_use_color, enum color_diff ix)
 	return "";
 }
 
-static void emit_line(FILE *file, const char *set, const char *reset, const char *line, int len)
-{
-	fputs(set, file);
-	fwrite(line, len, 1, file);
-	fputs(reset, file);
-}
-
 static void emit_add_line(const char *reset, struct emit_callback *ecbdata, const char *line, int len)
 {
 	const char *ws = diff_get_color(ecbdata->color_diff, DIFF_WHITESPACE);

@@ -604,16 +598,16 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 		free_diff_words_data(ecbdata);
 	if (ecbdata->diff_words) {
 		if (line[0] == '-') {
-			diff_words_append(line, len,
+			diff_words_append(ecbdata, line, len,
 					  &ecbdata->diff_words->minus);
 			return;
 		} else if (line[0] == '+') {
-			diff_words_append(line, len,
+			diff_words_append(ecbdata, line, len,
 					  &ecbdata->diff_words->plus);
 			return;
 		}
-		if (ecbdata->diff_words->minus.text.size ||
-		    ecbdata->diff_words->plus.text.size)
+		if (ecbdata->diff_words->minus.len ||
+		    ecbdata->diff_words->plus.len)
 			diff_words_show(ecbdata->diff_words);
 		line++;
 		len--;

@@ -1470,6 +1464,8 @@ static void builtin_diff(const char *name_a,
 		if (DIFF_OPT_TST(o, COLOR_DIFF_WORDS)) {
 			ecbdata.diff_words =
 				xcalloc(1, sizeof(struct diff_words_data));
+			strbuf_init(&ecbdata.diff_words->minus, 0);
+			strbuf_init(&ecbdata.diff_words->plus, 0);
 			ecbdata.diff_words->file = o->file;
 		}
 		xdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);

On Wed, May 07, 2008 at 12:13:39PM -0700, Junio C Hamano wrote:

strbuf uses size_t; since we pass sz in as a pointer to strbuf_detach,
there can be a pointer type mismatch.

But more big-picture, comparing the output of the old color words and
this implementation, there is one thing I don't like: the new one
doesn't bring together runs of additions and deletions, which can make
parsing text much easier. For example:

  $ echo This is a complete sentence. >one
  $ echo Here is some totally different text. >two

  # with old implementation; /-.../ is red, /+.../ is green
  $ git diff --color-words one two
  ...
  /-This/ /+Here/ is /-a complete sentence./+some totally different text./

  # with this patch
  $ git diff --color-words one two
  ...
  /-This/+Here/ is /-a/+some/ /-complete/+totally/ /-sentence./+different text./

-Peff

Junio C Hamano wrote (2008-05-07 12:13 -0700):

Johannes Schindelin [off-list ref] writes:

quoted

I am rather interested in the semantics, i.e. if you can punch holes
into this 3-class approach.

This is not the 3-class thing, but was done as a lunchtime hack.  It
removes more lines than it adds, with real comments ;-).

I tested your lunchtime hack from the "pu" branch. I'm perfectly happy
with the colored output itself but I noticed some different line feed
behaviour that you might want to know. Look at the example below. The
first is normal line diff. The second is the same text with the old
--color-words behaviour and the last is with the lunchtime hack version.
There are only three words added to the text; additions are written as
{+word} in the --color-words output.


Normal line diff
----------------

-OpenOffice.org has user setting for defining the minimum length for
+OpenOffice.org has a user setting for defining the minimum length for
 words to be hyphenated. By default the word length is counted from the
-whole word - even for compound words. For example the word
-'elokuvalippu' is 12 characters long. The word will be hyphenated like
-'elo-ku-va-lip-pu' in all cases when the minimum word length is set to
-12 or less. If the minimum length is set to 13 or more the word is not
-hyphenated at all.
+whole word - even for compound words. For example the compound word
+'elokuvalippu' is considered 12 characters long. The word will be
+hyphenated like 'elo-ku-va-lip-pu' in all cases when the minimum word
+length is set to 12 or less. If the minimum length is set to 13 or more
+the word is not hyphenated at all.

With the old --color-words
--------------------------

OpenOffice.org has {+a }user setting for defining the minimum length for
words to be hyphenated. By default the word length is counted from the
whole word - even for compound words. For example the {+compound }word
'elokuvalippu' is {+considered }12 characters long. The word will be
hyphenated like 'elo-ku-va-lip-pu' in all cases when the minimum word
length is set to 12 or less. If the minimum length is set to 13 or more
the word is not hyphenated at all.

With the lunchtime hack --color-words
-------------------------------------

OpenOffice.org has {+a }user setting for defining the minimum length for
words to be hyphenated. By default the word length is counted from the
whole word - even for compound words. For example the {+compound }word
'elokuvalippu' is {+considered }12 characters long. The word will be 
hyphenated like
 'elo-ku-va-lip-pu' in all cases when the minimum word 
length is set to
 12 or less. If the minimum length is set to 13 or more 
the word is not
 hyphenated at all.

On Thu, May 8, 2008 at 3:13 AM, Junio C Hamano [off-list ref] wrote:

I still don't understand why a ' '  is prepended. See my comment for
the following part

+       if (line[1] == ' ') {
+               /* A token */
+               line += 2;
+               len -= 3; /* drop the trailing LF */
+       } else {
+               /* A real LF */
+               line++;
+               len--;
       }

I think we can recognize a real LF by that the diff line should be a
single '\n', i.e. line[1] == '\n'. So what's wrong by
s/line[1] == ' '/line[1] != '\n'/ ?

-- 
Ping Yin

On Thu, May 8, 2008 at 6:34 PM, Teemu Likonen [off-list ref] wrote:

Junio C Hamano wrote (2008-05-07 12:13 -0700):

quoted

Johannes Schindelin [off-list ref] writes:

quoted

I am rather interested in the semantics, i.e. if you can punch holes
into this 3-class approach.

This is not the 3-class thing, but was done as a lunchtime hack.  It
removes more lines than it adds, with real comments ;-).

I tested your lunchtime hack from the "pu" branch. I'm perfectly happy
with the colored output itself but I noticed some different line feed
behaviour that you might want to know. Look at the example below. The
first is normal line diff. The second is the same text with the old
--color-words behaviour and the last is with the lunchtime hack version.
There are only three words added to the text; additions are written as
{+word} in the --color-words output.

You not only added the three words, but also wrap line at different position.

Normal line diff
----------------

-OpenOffice.org has user setting for defining the minimum length for
+OpenOffice.org has a user setting for defining the minimum length for
 words to be hyphenated. By default the word length is counted from the
-whole word - even for compound words. For example the word
-'elokuvalippu' is 12 characters long. The word will be hyphenated like
-'elo-ku-va-lip-pu' in all cases when the minimum word length is set to
-12 or less. If the minimum length is set to 13 or more the word is not
-hyphenated at all.
+whole word - even for compound words. For example the compound word
+'elokuvalippu' is considered 12 characters long. The word will be
+hyphenated like 'elo-ku-va-lip-pu' in all cases when the minimum word
+length is set to 12 or less. If the minimum length is set to 13 or more
+the word is not hyphenated at all.

With the old --color-words
--------------------------

OpenOffice.org has {+a }user setting for defining the minimum length for
words to be hyphenated. By default the word length is counted from the
whole word - even for compound words. For example the {+compound }word
'elokuvalippu' is {+considered }12 characters long. The word will be
hyphenated like 'elo-ku-va-lip-pu' in all cases when the minimum word
length is set to 12 or less. If the minimum length is set to 13 or more
the word is not hyphenated at all.

With the lunchtime hack --color-words
-------------------------------------

OpenOffice.org has {+a }user setting for defining the minimum length for
words to be hyphenated. By default the word length is counted from the
whole word - even for compound words. For example the {+compound }word
'elokuvalippu' is {+considered }12 characters long. The word will be
hyphenated like
 'elo-ku-va-lip-pu' in all cases when the minimum word
length is set to
 12 or less. If the minimum length is set to 13 or more
the word is not
 hyphenated at all.

With junio's following code, the number of LF can be more than any of
the original two input. Because it will output a LF whenever we
encounter a real LF in the original input. So  some LFs are outputed
as {-LF}  or {+LF} . We can't differentiate them because we can't see
the colored output for added or removed LF. The same case is that we
can't differentiate added and removed spaces.

That's why i proposed colored background for added/removed space
characters in former reply of this thread.

+       if (line[1] == ' ') {
+               /* A token */
+               line += 2;
+               len -= 3; /* drop the trailing LF */
+       } else {
+               /* A real LF */
+               line++;
+               len--;
       }
+       emit_line(diff_words->file, set, reset, line, len);



-- 
Ping Yin

Ping Yin wrote (2008-05-10 17:02 +0800):

On Thu, May 8, 2008 at 6:34 PM, Teemu Likonen [off-list ref] wrote:

quoted

There are only three words added to the text; additions are written
as {+word} in the --color-words output.

You not only added the three words, but also wrap line at different
position.

Ah, you're right of course. I'm too focused on just words and language.
I don't mind these "added" LFs on Junio's version but I understand if
someone finds them surprising when color-word-diffing natural languages.

On Sat, May 10, 2008 at 5:02 PM, Ping Yin [off-list ref] wrote:

On Thu, May 8, 2008 at 6:34 PM, Teemu Likonen [off-list ref] wrote:

quoted

Junio C Hamano wrote (2008-05-07 12:13 -0700):

quoted

Johannes Schindelin [off-list ref] writes:

quoted

I am rather interested in the semantics, i.e. if you can punch holes
into this 3-class approach.

This is not the 3-class thing, but was done as a lunchtime hack.  It
removes more lines than it adds, with real comments ;-).

I tested your lunchtime hack from the "pu" branch. I'm perfectly happy
with the colored output itself but I noticed some different line feed
behaviour that you might want to know. Look at the example below. The
first is normal line diff. The second is the same text with the old
--color-words behaviour and the last is with the lunchtime hack version.
There are only three words added to the text; additions are written as
{+word} in the --color-words output.

You not only added the three words, but also wrap line at different position.

quoted

Normal line diff
----------------

-OpenOffice.org has user setting for defining the minimum length for
+OpenOffice.org has a user setting for defining the minimum length for
 words to be hyphenated. By default the word length is counted from the
-whole word - even for compound words. For example the word
-'elokuvalippu' is 12 characters long. The word will be hyphenated like
-'elo-ku-va-lip-pu' in all cases when the minimum word length is set to
-12 or less. If the minimum length is set to 13 or more the word is not
-hyphenated at all.
+whole word - even for compound words. For example the compound word
+'elokuvalippu' is considered 12 characters long. The word will be
+hyphenated like 'elo-ku-va-lip-pu' in all cases when the minimum word
+length is set to 12 or less. If the minimum length is set to 13 or more
+the word is not hyphenated at all.

With the old --color-words
--------------------------

OpenOffice.org has {+a }user setting for defining the minimum length for
words to be hyphenated. By default the word length is counted from the
whole word - even for compound words. For example the {+compound }word
'elokuvalippu' is {+considered }12 characters long. The word will be
hyphenated like 'elo-ku-va-lip-pu' in all cases when the minimum word
length is set to 12 or less. If the minimum length is set to 13 or more
the word is not hyphenated at all.

With the lunchtime hack --color-words
-------------------------------------

OpenOffice.org has {+a }user setting for defining the minimum length for
words to be hyphenated. By default the word length is counted from the
whole word - even for compound words. For example the {+compound }word
'elokuvalippu' is {+considered }12 characters long. The word will be
hyphenated like
 'elo-ku-va-lip-pu' in all cases when the minimum word
length is set to
 12 or less. If the minimum length is set to 13 or more
the word is not
 hyphenated at all.

With junio's following code, the number of LF can be more than any of
the original two input. Because it will output a LF whenever we
encounter a real LF in the original input. So  some LFs are outputed
as {-LF}  or {+LF} . We can't differentiate them because we can't see
the colored output for added or removed LF. The same case is that we
can't differentiate added and removed spaces.

That's why i proposed colored background for added/removed space
characters in former reply of this thread.

+       if (line[1] == ' ') {
+               /* A token */
+               line += 2;
+               len -= 3; /* drop the trailing LF */
+       } else {
+               /* A real LF */
+               line++;
+               len--;
      }
+       emit_line(diff_words->file, set, reset, line, len);

With following patch, the diff output becomes (i don't know which one is better)

OpenOffice.org has {+a }user setting for defining the minimum length for
words to be hyphenated. By default the word length is counted from the
whole word - even for compound words. For example the {compound +}word
'elokuvalippu' is {+considered }12 characters long. The word will be
hyphenated like
 'elo-ku-va-lip-pu' in all cases when the minimum word length is set to
 12 or less. If the minimum length is set to 13 or more the word is not
 hyphenated at all.

diff --git a/diff.c b/diff.c
index 51048c6..06fbace 100644
--- a/diff.c
+++ b/diff.c

@@ -446,6 +446,7 @@ struct diff_words_data {
 	struct xdiff_emit_state xm;
 	struct strbuf minus;
 	struct strbuf plus;
+	int suppressed_newline;
 	FILE *file;
 };

@@ -480,12 +481,16 @@ static void fn_out_diff_words_aux(
 		/* A token */
 		line += 2;
 		len -= 3; /* drop the trailing LF */
+		emit_line(diff_words->file, set, reset, line, len);
 	} else {
 		/* A real LF */
-		line++;
-		len--;
+		if (diff_words->suppressed_newline || line[0] == ' ') {
+			diff_words->suppressed_newline = 0;
+			emit_line(diff_words->file, set, reset, "\n", 1);
+		}
+		else
+			diff_words->suppressed_newline = 1;
 	}
-	emit_line(diff_words->file, set, reset, line, len);
 }

 /* this executes the word diff on the accumulated buffers */

@@ -510,8 +515,14 @@ static void diff_words_show(
 	ecb.outf = xdiff_outf;
 	ecb.priv = diff_words;
 	diff_words->xm.consume = fn_out_diff_words_aux;
+	diff_words->suppressed_newline = 0;
 	xdi_diff(&minus, &plus, &xpp, &xecfg, &ecb);

+	if (diff_words->suppressed_newline) {
+		putc('\n', diff_words->file);
+		diff_words->suppressed_newline = 0;
+	}
+
 	free(minus.ptr);
 	free(plus.ptr);
 }

-- 
Ping Yin

* Ping Yin [off-list ref] [2008-05-11 21:16:11 +0800]:

With following patch, the diff output becomes (i don't know which one is better)

OpenOffice.org has {+a }user setting for defining the minimum length for
words to be hyphenated. By default the word length is counted from the
whole word - even for compound words. For example the {compound +}word
'elokuvalippu' is {+considered }12 characters long. The word will be
hyphenated like

The above two lines are auto wrapped by gmail client, should be

'elokuvalippu' is {+considered }12 characters long. The word will be hyphenated like

Sorry for that.

-- 
Ping Yin