Re: [PATCH 2/2] diff --word-diff: use non-whitespace regex by default

From: Thomas Rast <trast@student.ethz.ch>
Date: Jan 19, 2012, 15:53 UTC
Message-ID: <87bopzofir.fsf@thomas.inf.ethz.ch>
In-Reply-To: <CALUzUxqXTXZv4RE=4rBa79T3_1y7UdqZ6okjC1y-Ve+=NDbQ2g@mail.gmail.com>

Tay Ray Chuan <rctay89@gmail.com> writes:

Show 30 quoted lines

> On Thu, Jan 12, 2012 at 5:22 PM, Thomas Rast <trast@student.ethz.ch> wrote:
>> [snip]
>> Case in point, consider my patch sent out yesterday
>>
>>  http://article.gmane.org/gmane.comp.version-control.git/188391
>>
>> It consists of a one-hunk doc update.  word-diff is not brilliant:
>>
>>  -k::
>>          Usually the program [-'cleans up'-]{+removes email cruft from+} the Subject:
>>          header line to extract the title line for the commit log
>>          [-message,-]
>>  [-      among which (1) remove 'Re:' or 're:', (2) leading-]
>>  [-      whitespaces, (3) '[' up to ']', typically '[PATCH]', and-]
>>  [-      then prepends "[PATCH] ".-]{+message.+}  This [-flag forbids-]{+option prevents+} this munging, and is most
>>          useful when used to read back 'git format-patch -k' output.
>> [snip the rest as it's only {+}]
>>
>> But character-diff tries too hard to find common subsequences:
>>
>>  $ g show HEAD^^ --word-diff-regex='[^[:space:]]' | xsel
>>[snip]
>>  w-]{+.  T+}hi[-te-]s[-paces, (3) '[' up t-] o[-']', ty-]p[
>>
>> is just line noise?  The colors don't even help as most of it is removed
>> (red).
>
> You missed the '+' quantifier, as in
>
>   [^[:space:]]+

Did I?  I was working from the example you provided earlier

} matrix[a,b,c] } matrix[d,b,c] } gives } matrix[[-a-]{+d+},b,c] } } and when we have } } ImagineALanguageLikeFoo } ImagineALanguageLikeBar } we get } ImagineALanguageLike[-Foo-]{+Bar+}

Under [^[:space:]]+ neither of the examples would work. Actually, [^[:space:]]+ is the same as today's default, the [^[:space:]]* I mentioned later is (strictly speaking) broken as it allows for a 0-length match. (It doesn't really matter because IIRC the engine ignores 0-length words.)

Show 13 quoted lines

>> That being said, I can see some arguments for changing the default to
>> split punctuation into a separate word.  That is, whereas the current
>> default is semantically equivalent to a wordRegex of
>>
>>  [^[:space:]]*
>>
>> (but has a faster code path)
>
> Oh right, there *is* a sensible default implemented in. Somehow I was
> under the impression that there wasn't.
>
> I wonder which is faster, using the non-whitespace regex, or the
> isspace() calls...

I tried measuring it across a few commits, but it mostly gets drowned out by the diff effort. For a commit with stat

  exercises/cgal/cover/cover.cpp  |    5 +-
  exercises/cgal/cover/cover.in1  |27014 +++++++++++++++-----
  exercises/cgal/cover/cover.in2  |48996 +++++++++++++++++++++++------------
  exercises/cgal/cover/cover.in3  |55041 +++++++++++++++++++++++++--------------
  exercises/cgal/cover/cover.in4  |47600 ++++++++++++++++++++--------------
  exercises/cgal/cover/cover.int  |43491 ++++++++++++++++++++++---------
  exercises/cgal/cover/cover.out1 |   53 +-
  exercises/cgal/cover/cover.out2 |   24 +-
  exercises/cgal/cover/cover.out3 |   11 +-
  exercises/cgal/cover/cover.out4 |    2 +-
  exercises/cgal/cover/cover.outt |   23 +-
  exercises/cgal/cover/gen        |   39 +-
  exercises/cgal/cover/gen-1.cpp  |    4 +-
  exercises/cgal/cover/gen-2.cpp  |    6 +-
  exercises/cgal/cover/gen-3.cpp  |    6 +-

(sorry, can't share as those testcases are secret) I get best-of-5 timings

  --word-diff-regex='[^[:space:]]+'    0:07.50real 7.40user 0.07system
  --word-diff                          0:07.47real 7.41user 0.03system

In conclusion, "meh". I think ripping out the isspace() part would make for a nice code reduction.

Show 12 quoted lines

>> and your proposal is equivalent to
>>
>>  [^[:space:]]|UTF_8_GUARD
>>
>> I think there is a case to be made for a default of
>>
>>  [^[:space:]]|([[:alnum:]]|UTF_8_GUARD)+
>>
>> or some such.  There's a lot of bikeshedding lurking in the (non)extent
>> of the [[:alnum:]] here, however.
>
> Care to explain further? Not to sure what you mean here.

For natural language, it may or may not make sense to match numbers as part of a word.

For typical use in e.g. emails, a lot of punctuation has a double role; breaking words in

  http://article.gmane.org/gmane.comp.version-control.git/188391

may or may not make sense.

For some uses, especially source code, it would be better to match an underscore _ as part of a complete word, too.

For some programming languages, say lisp, a dash - would also belong in the same category.

There's no real reason other than ease of implementation why the pattern handles ASCII non-alphanumerics separately, but non-ASCII UTF-8 non-alnums (like, say, unicode NO-BREAK SPACE which would show as \xc2 \xa0) always goes into a word. But if you were to make UTF-8 sequences a single word, text in (say) many European languages would become chunked at accented letters.

I'm sure you can find more items for this list.  It's a grey area.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch