patch, 2 partst4034-diff-words: replace regex for diff driver

The next patch uses a non-whitespace regex, similar to the regex currently used by the 'testdriver' diff driver; replace the regex with a distinct one so that we can continue to conclude its effects.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>

---
Kept separate to keep the next patch clean.
---
 t/t4034-diff-words.sh |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)

Factor out the comprehensive non-whitespace regex in use by PATTERNS and IPATTERN and use it as the word-diff regex for the default diff driver.

As the default regex is no longer non-empty, update the word-regex selection logic (non-default driver from pre-image, then post-image, then the diff.wordRegex config) accordingly.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 diff.c                |   14 ++++++++------
 t/t4034-diff-words.sh |   31 +++++++++----------------------
 userdiff.c            |    8 +++++---
 3 files changed, 22 insertions(+), 31 deletions(-)

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

Why?

I seem to recall that the motivation for keeping the original code as-is instead of just emulating its behavior with a default regex was that it is faster. So disabling the default mode should at least have an advantage?

</devils-advocate>

Hi,

Thomas, first off, thanks for looking through this.

On Thu, Jan 12, 2012 at 4:05 AM, Thomas Rast <trast@student.ethz.ch> wrote:

If you're talking about speed, yeah, that's probably true.

But I think it's worthwhile to trade-off performance for a sensible default. Something like

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

(But I cheated. Foo and Bar have no common characters in common; if they did, the word diff would be messy.)

Both of which seem sensible. From a usability/effectiveness standpoint, I think it's more useful than what the current word-diff defaults to - the whole line is taken as a "word", with the pre-image shown as deleted and the post-image as added; we don't even try to run LCS on it.

Examples are lifted from: [1] http://article.gmane.org/gmane.comp.version-control.git/105896 [2] http://article.gmane.org/gmane.comp.version-control.git/105237

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

Sorry for distracting you with the performance argument; it was mostly the first thing that came to my mind that I could use to ask for the motivation, and evaluation of tradeoffs, that both were missing from the proposed commit message.

In that case (and I should have read the original patch), I am definitely against this change. It turns the default word-diff into character-diff, which is something entirely different, and frequently useless precisely for the reason you state:

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
  -k::
          Usually the program [-'cl-]{+remov+}e[-an-]s {+email cr+}u[-p'-]{+ft from+} the Subject:
          header line to extract the title line for the commit log
          message[-,-]
  [-      among which (1) remove 'Re:' or 're:', (2) leading-]
  [-      w-]{+.  T+}hi[-te-]s[-paces, (3) '[' up t-] o[-']', ty-]p[-ically '[PATCH]', and-]t[-he-]{+io+}n pre[-p-]{+v+}en[-ds "[PATCH] ".  This flag forbid-]{+t+}s this munging, and is most
          useful when used to read back 'git format-patch -k' output.
[snip]

Wouldn't you agree that

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

Regarding your examples

first please notice that both of them were written before (and actually discussing) the introduction of the wordRegex feature. At this point, we were trying to make up our minds w.r.t. how powerful the feature needs to be. Nowadays (or in fact, starting a few days after those emails) the user can easily achieve everything discussed here by setting the wordRegex to taste.

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

On Thu, Jan 12, 2012 at 5:22 PM, Thomas Rast <trast@student.ethz.ch> wrote:

You missed the '+' quantifier, as in

  [^[:space:]]+

Using that regex, that abomination of a word-diff that you mentioned disappears, like this:

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

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

Care to explain further? Not to sure what you mean here.

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

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

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.

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.

On Thu, Jan 19, 2012 at 11:53 PM, Thomas Rast <trast@student.ethz.ch> wrote:

My bad.

Thanks for the numbers. Well, that agrees with the intuition that regex is slower than isspace(), since you have run it through the regex engine.

Thanks.