patchMake words boundary for --color-words configurable

> Previously --color-words only allow spaces as words boundary. However,
> just space is not enough. For example, when i rename a function from
> foo to bar, following example doesn't show as expected when using
> --color-words.
>
> ------------------
> - if (foo(arg))
> + if (bar(arg))
> ------------------
>
> It shows as "if <r>(foo(arg))</r><g>(foo(arg))</g>". Actually, it's the
> best to show as "if (<r>foo</r><g>bar</g>(arg))". Here "r" and "g"
> represent "red" and "green" separately.
>
> This patch introduces a configuration diff.wordsboundary to make
> --color-words treat both spaces and characters in diff.wordsboundary as
> boundary characters.

>  I suspect a more natural definition of word boundary is between a run of
>  word characters and a run of non-word characters.  IOW, instead of saying
>  " " and "(these other)" characters are boundary, you would say
>
>         if (foo(arg))
>
>  between f and space, open paren and f, second o and open paren, that open
>  paren and a, ... are boundaries.
>
>  If you go that route, you would make the definition of "what is the set of
>  word characters" configurable.

>>  In general, my opinion is that with international text it's better to
>>  define word boundary characters than trying to maintain a _huge_ list of
>>  characters used within words in different human languages.
>>
>
> Agreed, there is no easy way to designate such a huge list of
> non-ascii characters. Even if we figure out one way, the user may
> still be scared by such a huge list or the character class syntax.
> 
> I think doing this complex both the implementation and the representation.
> 
> Instead, if using non word characters, the user only needs specify a
> small list of characters (ascii or wide character). instead of the
> nearly whole set.

> Previously --color-words only allow spaces as boundary characters.
>  However, just space is not enough. For example, when i rename a function
>  from foo to bar, following example doesn't show as expected when using
>  --color-words.
>
>  ------------------
>  - if (foo(arg))
>  + if (bar(arg))
>  ------------------
>
>  It shows as "if <r>(foo(arg))</r><g>(foo(arg))</g>". Actually, it's the
>  best to show as "if (<r>foo</r><g>bar</g>(arg))". Here "r" and "g"
>  represent "red" and "green" separately.
>
>  This patch introduces a configuration diff.nonwordchars to make
>  --color-words treat both spaces and characters in diff.nonwordchars as
>  boundary characters.
>
>  If we set diff.nonwordchars to "()", the example above will show as
>  "if (<r>foo(</r><g>bar(</g>arg))". It's much better, athough not the best,
>

> Oh, there are some problems, assuming "{}" are set as diff.nonwordchars
> 
> 1. Trailing boundary character lost, for example
> ----------------------------
> $ git diff-
> - foo{
> + foo
> $ git diff --color-words
> foo
> ----------------------------
> With --color-words,  i can't know the trailing '{' is removed. This
> problem exists even without my patch. In that case, only trainling
> spaces are  lost.
> 
> 2. Trailing removed words shows at new line instead of the same line
> ----------------------------
> $ git diff
> - foo bar
> + foo
> (note: no space after foo)
> $ git diff --color-words
> foo
> <red>bar</red>
> --------------------------------
> bar should show in the same line with bar. This is not related to my patch.

> Has this series been ever tested?
> 
> $ git diff one two
> diff --git a/one b/two
> index f9facd3..10c6195 100644
> --- a/one
> +++ b/two
> @@ -1 +1 @@
> -A quick(brown) fox
> +A quick(yellow) fox
> 
> $ tail -n 2 .git/config
> [diff]
>         nonwordchars = "()"
> 
> $ git diff --color-words one two
> diff --git a/one b/two
> index f9facd3..10c6195 100644
> --- a/one
> +++ b/two
> @@ -1 +1 @@
> A quick(<red>brown)</red><green>yellow)</green> fox

> Has this series been ever tested?
>
>  $ git diff one two
>  diff --git a/one b/two
>  index f9facd3..10c6195 100644
>  --- a/one
>  +++ b/two
>  @@ -1 +1 @@
>  -A quick(brown) fox
>  +A quick(yellow) fox
>
>  $ tail -n 2 .git/config
>  [diff]
>         nonwordchars = "()"
>
>  $ git diff --color-words one two
>  diff --git a/one b/two
>  index f9facd3..10c6195 100644
>  --- a/one
>  +++ b/two
>  @@ -1 +1 @@
>  A quick(<red>brown)</red><green>yellow)</green> fox

> Hi,
>
>  The problem: given two chunks of text, where a word was changed, and a
>  non-word-character was moved to the next line.  Example:
>
>         The quick,
>         brown fox
>
>  vs
>
>         The fast
>         , brown fox
>
>  IMHO the layout of the new version should be retained, i.e.
>
>         The /quick/fast/
>         , brown fox
>
>  should be shown.

> Let's step back a bit and try to clarify the problem with a bit of
> illustration.
> 
> The motivation behind "word diff" is because line oriented diff is
> sometimes unwieldy.
> 
>     -Hello world.
>     +Hi, world.

> We instead can do this word by word (note that I am doing this as a
> thought experiment, to illustrate what the problem is and what should
> conceptually happen, not suggesting this particular implementation):
> 
>     preimage        postimage       word-diff
>     48656c6c6f                      -48656c6c6f Hello
>                     4869            +4869       Hi
>                     2c              +2c         ,
>     20              20               20         ' '
>     776f726c64      776f726c64       776f726c64 world      
>     2e              2e               2e         .
>     0a              0a               0a         '\n'
> 
> Which would give you "/Hello/Hi,/ world.\n".

> Would it be possible instead of in-line word diff, use word coloring
> to enhance traditional diff format?  Something like
> 
>      -/Hello/ world.
>      +/Hi,/ world.
> 
> (We could use bold, or reverse for marking changed fragment, or use
> color only for changed fragment).

> Junio C Hamano <junio@pobox.com> writes:
> 
> > Let's step back a bit and try to clarify the problem with a bit of
> > illustration.
> > 
> > The motivation behind "word diff" is because line oriented diff is
> > sometimes unwieldy.
> > 
> >     -Hello world.
> >     +Hi, world.
> [...]
> > We instead can do this word by word (note that I am doing this as a
> > thought experiment, to illustrate what the problem is and what should
> > conceptually happen, not suggesting this particular implementation):
> > 
> >     preimage        postimage       word-diff
> >     48656c6c6f                      -48656c6c6f Hello
> >                     4869            +4869       Hi
> >                     2c              +2c         ,
> >     20              20               20         ' '
> >     776f726c64      776f726c64       776f726c64 world      
> >     2e              2e               2e         .
> >     0a              0a               0a         '\n'
> > 
> > Which would give you "/Hello/Hi,/ world.\n".
> 
> Would it be possible instead of in-line word diff, use word coloring
> to enhance traditional diff format?  Something like
> 
>      -/Hello/ world.
>      +/Hi,/ world.
> 
> (We could use bold, or reverse for marking changed fragment, or use
> color only for changed fragment).
> 
> IMHO current output is nice, unless you have long lines and not very
> wide screen...

>
> So the overall algorithm I think should be is:
>
>  - make the input into stream of tokens, where a token is either a run of
>   word characters only, non-word punct characters only, or whitespaces
>   only;
>
>  - compute the diff over the stream of tokens;
>
>  - emit common tokens in white, deleted in red and added in green.
>
> Notice that you do not have to special case LF in any way if you go this
> route.
>
> You could do this with only two classes, and use a different tokenization
> rule: a token is either a run of word characters only, or each byte of non
> word character becomes individual token.  This however would yield a
> suboptimal result:
>
>    -if (i > 1)
>    +while (i >= 0)
>
>    preimage       postimage        word-diff
>    6966                            -6966       if
>                   7768696c65       +7768696c65 while
>    20             20                20         ' '
>    28             28                28         (
>    69             69                69         i
>    20             20                20         ' '
>    3e             3e                3e         >
>                   3d               +3d         =
>    20             20                20         ' '
>    31                              -31         1
>                   30               +30         0
>    29             29                29         )
>
> This would give "/if/while/ (i >//=/ /1/0/)".  A logical unit ">=" is
> chomped into two tokens, which is suboptimal for the same reason why the
> output "H/ello/i,/" from the original char-diff based one was suboptimal.
>

> "Ping Yin" <pkufranky@gmail.com> writes:
>
>  > For this example,both "/if/while/ (i />/>=/ /1/0/)" and  "/if/while/
>  > (i >//=/ /1/0/)" are fine to me.
>
>  For the particular example, both are Ok, but for this other example:
>
>         -if (i > 1...
>         +if ((i > 1...
>
>  it probably is better to treat each non-word character as a separate
>  token, that is, it would be easier to read if we said "( stayed intact,
>  and another ( was added", instead of saying "( is changed to ((".
>
>  So "a run of punct chars" rule only sometimes produces better output but
>  otherwise worse output, and to make it produce better output consistently,
>  we would need to know the syntax of the target language for tokenization,
>  i.e. ">=" and ">" are comparison operators, while "(" is a token and "(("
>  is better split into two open-paren tokens.
>
>  So as a very longer term subproject, we may want to teach the mechanism
>  language specific tokenization rules, just like we can specify the hunk
>  header pattern via gitattributes(5) to the diff output layer.
>
>  Of course, I do not expect you to do that during this round --- and if we
>  choose to keep the rule simple, I think it is probably better to use
>  one-char-one-token rule for now.
>
>
>  > And when designing, i think it's better to take multi-byte characters
>  > into account. For multi-byte characters (especially CJK), every
>  > character should be considered as a token.
>
>  If we take an idealistic view for the longer term, we should be tokenizing
>  even CJK sensibly, but unlike Occidental scripts, we cannot even use
>  inter-word spacing for tokenizing hint, so unless we are willing to learn
>  morphological analysis (which we are not for now), the best we can do is
>  to use one-char-one-token rule.
>
>         Side Note.  For Japanese we could cheat and often do a slightly
>         better job than simple one-char-one-token without having full
>         morphological analysis by splicing between Kanji and Kana
>         boundaries, but I'd prefer not to go there and keep the rules we
>         would use to the minimum.
>
>  I should stress that I said "character" in the above "punct" and "CJK"
>  discussions, not "byte".
>

>
>  The one-char-one-token and multi-char-one-token rules may have
>  different implementation issues. I think multi-char-one-token rule may
>  be more representative. So for the current time, i prefer considering
>  both run of word characters and single non-word character as a token.
>

> > I will come up with an implementation still using diff.nonwordchars few
>  > days later.
>
>  If I did not like the unnecessary negative approach "nonwordchars" (as
>  opposed to "wordchars"), it seems even less appropriate now, when you
>  actually want to discern between "spaceCharacters",
>  "punctuationCharacters" and "wordCharacters".
>

> On Tue, May 6, 2008 at 4:55 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > > I will come up with an implementation still using diff.nonwordchars few
> >  > days later.
> >
> >  If I did not like the unnecessary negative approach "nonwordchars" (as
> >  opposed to "wordchars"), it seems even less appropriate now, when you
> >  actually want to discern between "spaceCharacters",
> >  "punctuationCharacters" and "wordCharacters".
> >
> 
> Hmm, punctchars should be a better word than nonwordchars.
> 
> So how about this
> 
> --color-words={char,punct,word}
> 
>   - char: one char one token
>   - punct/word: a token can be either a run of word characters or a
> single punct character.  diff.punctchars is used for punct, and
> diff.wordchars is used for word.

>  > So how about this
>  >
>  > --color-words={char,punct,word}
>  >
>  >   - char: one char one token
>  >   - punct/word: a token can be either a run of word characters or a
>  > single punct character.  diff.punctchars is used for punct, and
>  > diff.wordchars is used for word.
>
>  I am rather interested in the semantics, i.e. if you can punch holes into
>  this 3-class approach.
>
>  Bikeshedding comes later ;-)

> On Wed, May 7, 2008 at 7:24 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >  > So how about this
> >  >
> >  > --color-words={char,punct,word}
> >  >
> >  >   - char: one char one token
> >  >   - punct/word: a token can be either a run of word characters or a
> >  > single punct character.  diff.punctchars is used for punct, and
> >  > diff.wordchars is used for word.
> >
> >  I am rather interested in the semantics, i.e. if you can punch holes into
> >  this 3-class approach.
> >
> >  Bikeshedding comes later ;-)
> 
> Sorry, but i can't parse both sentences, especially Bikeshedding and
> "punch holes into".

>  > Sorry, but i can't parse both sentences, especially Bikeshedding and
>  > "punch holes into".
>
>  "punch holes into": find cases where Junio's proposed algorithm breaks
>  down.

>
>  "bikeshedding": discussing minor implementation details that are not
>  really interesting at this stage.  See also
>  http://en.wikipedia.org/wiki/Bikeshedding.
>

> 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.

>  /* 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;

> 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./

> I suspect that heavily depends on the input text.  If you drop "different"
> in the example, the output becomes:
> 
>     {-This|+Here} is {-a|+some} {-complete|+totally} {-sentence.|+text.}
> 
> which is totally sensible.
>
> [...]
> 
> which would yield on the output:
> 
>     {-This |+Here }is {-a complete sentence.|+some totally different text.}

> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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 ;-).

> Junio C Hamano wrote (2008-05-07 12:13 -0700):
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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 ;-).
>
> 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 6:34 PM, Teemu Likonen <tlikonen@iki.fi> wrote:
> > 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.

> On Thu, May 8, 2008 at 6:34 PM, Teemu Likonen <tlikonen@iki.fi> wrote:
>> Junio C Hamano wrote (2008-05-07 12:13 -0700):
>>
>>> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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 ;-).
>>
>> 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);
>

> 
> 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

> 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.

> "Ping Yin" <pkufranky@gmail.com> writes:
>
>  > 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.
>
>  Yeah, after playing with it a bit, I realize that my original stated goal
>  of not playing games with "newline suppression" goes very against what
>  color-words, which is a word oriented diff, tries to achieve.  It appears
>  that it is necessary to reintroduce suppressed_newline.
>

> On Mon, May 12, 2008 at 12:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> "Ping Yin" <pkufranky@gmail.com> writes:
>>
>>> 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.
>>
>>  Yeah, after playing with it a bit, I realize that my original
>>  stated goal of not playing games with "newline suppression" goes
>>  very against what color-words, which is a word oriented diff,
>>  tries to achieve.  It appears that it is necessary to reintroduce
>>  suppressed_newline.
>>
> 
> No matter how well we play with suppressed_newline, we still can't
> achieve the best result by doing word diff between multiple minus
> lines and multiple plus lines.
> 
>  ( i think the result of vimdiff can be considered as the best).

>> To achieve the best, we have to find the pairs of lines (one minus and
>> one plus for each pair) which most match each other, and then do the
>> word diff for each pair.
>
> Wouldn't be enough to treat run of plus/minus lines as a single block,
> tokenize, do token-based (as opposed to line-based) diff, then show it
> using linebreaks of the destination file (pluses line)?

> Jakub Narebski <jnareb@gmail.com> writes:
>> "Ping Yin" <pkufranky@gmail.com> writes:
>>>
>>> To achieve the best, we have to find the pairs of lines (one minus and
>>> one plus for each pair) which most match each other, and then do the
>>> word diff for each pair.
>>
>> Wouldn't be enough to treat run of plus/minus lines as a single block,
>> tokenize, do token-based (as opposed to line-based) diff, then show it
>> using linebreaks of the destination file (pluses line)?
> 
> I tried the "using linebreaks" but I discarded it because I did not think
> it would work.  If we rewrite the last three lines above with this single
> line:
> 
>> Wouldn't be enough to use magic?
> 
> and apply that algorithm between the two, then we would get a long single
> line that has words painted in red, two lines worth, followed by green "to
> use magic?"  and finally an end-of-line.

> Jakub Narebski <jnareb@gmail.com> writes:
> 
> >> To achieve the best, we have to find the pairs of lines (one minus and
> >> one plus for each pair) which most match each other, and then do the
> >> word diff for each pair.
> >
> > Wouldn't be enough to treat run of plus/minus lines as a single block,
> > tokenize, do token-based (as opposed to line-based) diff, then show it
> > using linebreaks of the destination file (pluses line)?
> 
> I tried the "using linebreaks" but I discarded it because I did not think
> it would work.  If we rewrite the last three lines above with this single
> line:
> 
> > Wouldn't be enough to use magic?
> 
> and apply that algorithm between the two, then we would get a long single
> line that has words painted in red, two lines worth, followed by green "to
> use magic?"  and finally an end-of-line.

> 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;

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

> On Sun, May 4, 2008 at 5:44 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Hi,
> >
> >  The problem: given two chunks of text, where a word was changed, and a
> >  non-word-character was moved to the next line.  Example:
> >
> >         The quick,
> >         brown fox
> >
> >  vs
> >
> >         The fast
> >         , brown fox
> >
> >  IMHO the layout of the new version should be retained, i.e.
> >
> >         The /quick/fast/
> >         , brown fox
> >
> >  should be shown.
> 
> Why not
> 
>   The <r>quick</r><g>fast</g><r>,</r>
>   <g>,</g>brown fox

>  > >
>  > >         The quick,
>  > >         brown fox
>  > >
>  > >  vs
>  > >
>  > >         The fast
>  > >         , brown fox
>  > >
>  > >  IMHO the layout of the new version should be retained, i.e.
>  > >
>  > >         The /quick/fast/
>  > >         , brown fox
>  > >
>  > >  should be shown.
>  >
>  > Why not
>  >
>  >   The <r>quick</r><g>fast</g><r>,</r>
>  >   <g>,</g>brown fox
>
>  I might well be a complete idiot, but your <r></r><something> example is
>  way harder for me to read than my example.
>
>  And of course your example would still be wrong: the "quick" and the comma
>  would not be separated at all.

> With --color-words, following example will show deleted word "bar" at
> another line. (<r> represents red)
> ------------------
> $ git diff
> - foo bar
> + foo
> $ git diff --color-words
> foo
> <r>bar</r>
> ------------------

> Instead, we always supress the newline when using print_word, and in
> fn_out_diff_words_aux, a newline is shown only in following cases:
>
>   - true minus.suppressed_newline followd by a line beginning with
>     '-', ' ' or '@' (i.e. not '+')
>   - true plus.suppressed_newline followd by a line beginning with
>     '+', ' ' or '@' (i.e. not '-')

> diff --git a/diff.c b/diff.c
> index b5f7141..11316fe 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -409,6 +409,7 @@ static void print_word(FILE *file, struct diff_words_buffer *buffer, int len, in
>  static void fn_out_diff_words_aux(void *priv, char *line, unsigned long len)
>  {
>  	struct diff_words_data *diff_words;
> +	char cm;
>  	struct diff_words_buffer *dm, *dp;
>  	FILE *df;

> +	int i;
> +
> +	dest->size = src->size;
> +	dest->ptr = xmalloc(dest->size);
> +	memcpy(dest->ptr, src->ptr, dest->size);
> +	for (i = 0; i < dest->size; i++)
> +		if (isspace(dest->ptr[i]))
> +			dest->ptr[i] = '\n';
> +}
> +
>  /* this executes the word diff on the accumulated buffers */
>  static void diff_words_show(struct diff_words_data *diff_words)
>  {
> @@ -444,20 +455,9 @@ static void diff_words_show(struct diff_words_data *diff_words)
>  	int i;

> Before feeding minus and plus lines into xdi_diff, we replace non word
> characters with '\n'. So we need recover the replaced character (always
> the last character) in the callback fn_out_diff_words_aux.
> 
> Therefore, a common diff line beginning with ' ' is not always a real
> common line.

> Hi,
>
>
>  On Sun, 4 May 2008, Ping Yin wrote:
>
>  > Before feeding minus and plus lines into xdi_diff, we replace non word
>  > characters with '\n'. So we need recover the replaced character (always
>  > the last character) in the callback fn_out_diff_words_aux.
>  >
>  > Therefore, a common diff line beginning with ' ' is not always a real
>  > common line.
>
>  Umm, why?

>  > And we should check the last characters of the common diff line. If they
>  > are different, we should output the first len-1 characters as the common
>  > part and then the last characters in minus and plus separately.
>
>  Umm, why?

> On Sun, May 4, 2008 at 5:54 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >  On Sun, 4 May 2008, Ping Yin wrote:
> >
> >  > Before feeding minus and plus lines into xdi_diff, we replace non 
> >  > word characters with '\n'. So we need recover the replaced 
> >  > character (always the last character) in the callback 
> >  > fn_out_diff_words_aux.
> >  >
> >  > Therefore, a common diff line beginning with ' ' is not always a 
> >  > real common line.
> >
> >  Umm, why?
> 
> Because we need recover the replaced character.
> 
> Say, for a common diff line " foo", after restoring the replaced 
> character, the corresponding line in minus and plus may be different. 
> For example, "foo(" and "foo)".

>  >
>  > Because we need recover the replaced character.
>  >
>  > Say, for a common diff line " foo", after restoring the replaced
>  > character, the corresponding line in minus and plus may be different.
>  > For example, "foo(" and "foo)".
>
>  Why do I have to spend time trying to figure out what you meant, write an
>  email, and get the explanation only in a response (i.e. not the commit
>  message, where it belongs)?
>

> A more sensible implementation is to use 'insert' instead of 'replace'.
> Say, to insert a line break between runs of word characters and non-word
> characters or between non-word characters.
>
> That is, "foo>=bar" will be rewritten as "foo\n>\n=\nbar" instead of
> "foo\n\nbar".

> Ping Yin <pkufranky@gmail.com> writes:
>
>  > A more sensible implementation is to use 'insert' instead of 'replace'.
>  > Say, to insert a line break between runs of word characters and non-word
>  > characters or between non-word characters.
>  >
>  > That is, "foo>=bar" will be rewritten as "foo\n>\n=\nbar" instead of
>  > "foo\n\nbar".
>
>  Hmmmm.
>
>  I am not sure if/why you would want a separator between '>' and '=' in
>  that example.  Shouldn't that be "foo <sep> >= <sep> bar"?

> @@ -417,10 +418,11 @@ static void fn_out_diff_words_aux(void *priv, char *line, unsigned long len)
>  	dwb_plus = &(diff_words->plus);
>  	outfile = diff_words->file;
>  
> -	if (dwp_minus->suppressed_newline) {
> -		if (line[0] != '+')
> -			putc('\n', outfile);
> -		dwp_minus->suppressed_newline = 0;
> +	if ((dwb_minus->suppressed_newline && line[0] != '+') ||

> Hi,
>
>
>  On Sun, 4 May 2008, Ping Yin wrote:
>
>  > With --color-words, following example will show deleted word "bar" at
>  > another line.
>
>  "will", or "used to"?

>  > This is caused by the unsymmetrical handling of LF in the plus and minus
>  > buffer in fn_out_diff_words_aux.
>
>  Is it not rather caused by the need to replace non-word-characters with
>  LF?

>
>  > Following is original unsymmetrical handling rules where LF represents
>  > a LF will be shown there.
>
>  I cannot parse this sentence.

>
>  > The second rule causes any word following the trailing plus word will
>  > be shown in a different line.
>
>  I cannot parse this sentence.
>
>

>
>  > @@ -417,10 +418,11 @@ static void fn_out_diff_words_aux(void *priv, char *line, unsigned long len)
>  >       dwb_plus = &(diff_words->plus);
>  >       outfile = diff_words->file;
>  >
>  > -     if (dwp_minus->suppressed_newline) {
>  > -             if (line[0] != '+')
>  > -                     putc('\n', outfile);
>  > -             dwp_minus->suppressed_newline = 0;
>  > +     if ((dwb_minus->suppressed_newline && line[0] != '+') ||
>
>  If the previous version had dwp_minus, and the new version has dwb_minus,
>  I wonder if both compile and pass the test suite.
>

> On Sun, May 4, 2008 at 5:52 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> >  On Sun, 4 May 2008, Ping Yin wrote:
> >
> >  > This is caused by the unsymmetrical handling of LF in the plus and 
> >  > minus buffer in fn_out_diff_words_aux.
> >
> >  Is it not rather caused by the need to replace non-word-characters 
> >  with LF?
> 
> No, i think this has nothing to do with the replacing 
> non-word-characters with LF.

> >  > Following is original unsymmetrical handling rules where LF 
> >  > represents a LF will be shown there.
> >
> >  I cannot parse this sentence.
> 
> Following is the original unsymmetrical handling rules

> >  > The second rule causes any word following the trailing plus word 
> >  > will be shown in a different line.
> >
> >  I cannot parse this sentence.
> 
> The second rule causes any word following the trailing plus word to show 
> in a different line with the trailing plus word.

> Hi,
>
>  the use of this patch evades me.  Care to defend why it is necessary?
>  Preferably in the commit message?
>

> On Sun, May 4, 2008 at 5:47 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Hi,
> >
> >  the use of this patch evades me.  Care to defend why it is necessary?
> >  Preferably in the commit message?
> >
> 
> In fn_out_diff_words_aux, we use diff_words->plus in so many places.
> So just use shorter name to save typing and avoid typo.

> Signed-off-by: Ping Yin <pkufranky@gmail.com>
> ---
>   
>> I think config variables should be in alphabetical order in config.txt.
>> Hence your diff.nonwordchars is not in the right place
>>     
>
> THX, this is fixing patch
>
>  Documentation/config.txt |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index eb05592..812ec2c 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -537,6 +537,10 @@ diff.external::
>  	program only on a subset of your files, you might want to
>  	use linkgit:gitattributes[5] instead.
>  
> +diff.nonwordchars::
> +	Specify additional boundary characters other than spaces for
> +	--color-words.
> +
>  diff.renameLimit::
>  	The number of files to consider when performing the copy/rename
>  	detection; equivalent to the git diff option '-l'.
> @@ -546,10 +550,6 @@ diff.renames::
>  	will enable basic rename detection.  If set to "copies" or
>  	"copy", it will detect copies, as well.
>  
> -diff.nonwordchars::
> -    Specify additional boundary characters other than spaces for
> -    --color-words.
> -
>  fetch.unpackLimit::
>  	If the number of objects fetched over the git native
>  	transfer is below this
>   

>
>  Now, you can specify which characters are to be interpreted as word
>  characters with "--color-words=A-Za-z", or by setting the config variable
>  diff.wordCharacters.
>
>  Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>  ---
>
>         I would have preferred an approach like this.
>
>   Documentation/config.txt       |    6 ++++
>   Documentation/diff-options.txt |    8 ++++-
>   README                         |    2 +-
>   diff.c                         |   64 +++++++++++++++++++++++++++++++++++++++-
>   4 files changed, 77 insertions(+), 3 deletions(-)
>
>  diff --git a/Documentation/config.txt b/Documentation/config.txt
>  index 05bf2df..663d82b 100644
>  --- a/Documentation/config.txt
>  +++ b/Documentation/config.txt
>  @@ -546,6 +546,12 @@ diff.renames::
>         will enable basic rename detection.  If set to "copies" or
>         "copy", it will detect copies, as well.
>
>  +diff.wordcharacters::
>  +       This config setting overrides which characters are interpreted as
>  +       word characters by the --color-words option of linkgit:git-diff[1].

> On Sat, May 3, 2008 at 10:03 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> >  Now, you can specify which characters are to be interpreted as word
> >  characters with "--color-words=A-Za-z", or by setting the config variable
> >  diff.wordCharacters.
> >
> >  Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> >  ---
> >
> >         I would have preferred an approach like this.
> >
> >   Documentation/config.txt       |    6 ++++
> >   Documentation/diff-options.txt |    8 ++++-
> >   README                         |    2 +-
> >   diff.c                         |   64 +++++++++++++++++++++++++++++++++++++++-
> >   4 files changed, 77 insertions(+), 3 deletions(-)
> >
> >  diff --git a/Documentation/config.txt b/Documentation/config.txt
> >  index 05bf2df..663d82b 100644
> >  --- a/Documentation/config.txt
> >  +++ b/Documentation/config.txt
> >  @@ -546,6 +546,12 @@ diff.renames::
> >         will enable basic rename detection.  If set to "copies" or
> >         "copy", it will detect copies, as well.
> >
> >  +diff.wordcharacters::
> >  +       This config setting overrides which characters are interpreted as
> >  +       word characters by the --color-words option of linkgit:git-diff[1].
> 
> I think a-zA-Z0-9_ should always be word characters. We can't override 
> them, instead, we just extend them.

> Now, you can specify which characters are to be interpreted as word
> characters with "--color-words=A-Za-z", or by setting the config
> variable diff.wordCharacters.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> 
> 	I would have preferred an approach like this.

> Johannes Schindelin wrote (2008-05-03 15:03 +0100):
> 
> > Now, you can specify which characters are to be interpreted as word
> > characters with "--color-words=A-Za-z", or by setting the config
> > variable diff.wordCharacters.
> > 
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > 
> > 	I would have preferred an approach like this.
> 
> Unfortunately this does not work at all with other than Ascii 
> characters. It makes --color-words completely unusable for anything 
> other than Ascii text. Sorry.

> Ping Yin's version has also the problem that UTF-8 multibyte characters
> U+0080..U+10FFFF don't work in diff.nonwordchars. Fortunately the most
> important word delimiters are in U+0000..U+007F (=Ascii) area so Ping's
> version is perfectly usable with Unicode text.
>
> (Even the old --color-words behaviour with only SPACE as non-word char 
> was perfectly usable with Unicode text.)

> Now, you can specify which characters are to be interpreted as word 
> characters with "--color-words=A-Za-z", or by setting the config variable 
> diff.wordCharacters.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>
> 	I would have preferred an approach like this.

> diff --git a/README b/README
> index 548142c..0e325e2 100644
> --- a/README
> +++ b/README
> @@ -4,7 +4,7 @@
>  
>  ////////////////////////////////////////////////////////////////
>  
> -"git" can mean anything, depending on your mood.
> +"git" cann mean anything, depending on your mood.

> @@ -456,7 +514,7 @@ static void diff_words_show(struct diff_words_data *diff_words)
>  	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]))
> +		if (!word_character[(unsigned char)plus.ptr[i]])
>  			plus.ptr[i] = '\n';
>  	diff_words->plus.current = 0;

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Now, you can specify which characters are to be interpreted as word 
> > characters with "--color-words=A-Za-z", or by setting the config variable 
> > diff.wordCharacters.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >
> > 	I would have preferred an approach like this.
> 
> Hmmm...

> > diff --git a/README b/README
> > index 548142c..0e325e2 100644
> > --- a/README
> > +++ b/README
> > @@ -4,7 +4,7 @@
> >  
> >  ////////////////////////////////////////////////////////////////
> >  
> > -"git" can mean anything, depending on your mood.
> > +"git" cann mean anything, depending on your mood.
> 
> Heh.

> > @@ -456,7 +514,7 @@ static void diff_words_show(struct diff_words_data *diff_words)
> >  	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]))
> > +		if (!word_character[(unsigned char)plus.ptr[i]])
> >  			plus.ptr[i] = '\n';
> >  	diff_words->plus.current = 0;
> 
> I do not think there is much difference between specifying the set of 
> word characters and the set of non-word characters, especially as long 
> as your definition of "character" is limited to 8-bit bytes.  By 
> enumerating word characters, your patch is letting the user specify non 
> word characters that are remainder from the 256-element set.  By the 
> way, I think you meant to do the same for the "minus" side a few lines 
> above this hunk.

> I commented on the patch from Ping earier about a quite different issue. 
> I was wondering if we can avoid losing the non-word character 
> information. The original code replaces any isspace byte with LF, but a 
> whitespace is a whitespace is a whitespace so there won't be much loss 
> of information, but making the above isspace() configurable means that 
> now you are going to drop non-space non-word characters from the output 
> set.
> 
> Instead of dropping the original character and replacing it with LF, I 
> thought a more sensible approach would be to _insert_ a line break 
> between runs of word characters and non-word characters (while probably 
> dropping a LF in the original).  That is, instead of what the current 
> implementation of the above loop does to "ab c d" (i.e. rewrite it to 
> "ab\n\nc\nd"), rewrite it to "ab\n \nc\n \nd".  Which feels more 
> consistent with the way how \b should work.

> On Fri, 2 May 2008, Ping Yin wrote:
> 
> > Previously --color-words only allow spaces as words boundary.
> > However, just space is not enough. For example, when i rename
> > a function from foo to bar, following example doesn't show as
> > expected when using --color-words.
> 
> Thanks for starting this.
> 
> However, as Junio pointed out, it is easier to specify
> word-characters, rather than non-word characters (think TAB), and...

> > On Fri, 2 May 2008, Ping Yin wrote:
> > 
> > > Previously --color-words only allow spaces as words boundary.
> > > However, just space is not enough. For example, when i rename
> > > a function from foo to bar, following example doesn't show as
> > > expected when using --color-words.