Re: Re: [PATCH] userdiff: add build-in pattern for shell

[Pickfire]

Junio C Hamano  wrote:

> Ivan Tham  writes:
> 
> > Shell are widely used but comes with lots of different patterns. The
> > build-in pattern aim for POSIX-compatible shells with some additions:
> >
> > - Notably ${g//re/s} and ${g#cut}
> > - "function" from bash
> >
> > Signed-off-by: Ivan Tham 
> > ---
> >  Documentation/gitattributes.txt |  2 ++
> >  t/t4034-diff-words.sh   |  1 +
> >  t/t4034/sh/expect   | 14 ++
> >  t/t4034/sh/post |  7 +++
> >  t/t4034/sh/pre  |  7 +++
> >  userdiff.c  |  5 +
> >  6 files changed, 36 insertions(+)
> >  create mode 100644 t/t4034/sh/expect
> >  create mode 100644 t/t4034/sh/post
> >  create mode 100644 t/t4034/sh/pre
> >
> > diff --git a/Documentation/gitattributes.txt 
> > b/Documentation/gitattributes.txt
> > index a53d093ca..1bad72df2 100644
> > --- a/Documentation/gitattributes.txt
> > +++ b/Documentation/gitattributes.txt
> > @@ -706,6 +706,8 @@ patterns are available:
> >  
> >  - `ruby` suitable for source code in the Ruby language.
> >  
> > +- `sh` suitable for source code in POSIX-compatible shells.
> 
> The new test you added seems to show that this is not limited to
> POSIX shells but also understands bashisms like ${x//x/x}.  Perhaps
> drop "POSIX-compatible" from here

Those shells are still POSIX-compatible so I think it is true to put
that or otherwise, something like fish shell will break since it is
as well a shell but the syntax is totally different.

> > diff --git a/userdiff.c b/userdiff.c
> > index 8b732e40b..8d5127fb6 100644
> > --- a/userdiff.c
> > +++ b/userdiff.c
> > @@ -148,6 +148,11 @@ PATTERNS("csharp",
> >  "[a-zA-Z_][a-zA-Z0-9_]*"
> >  "|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lL]?"
> >  "|[-+*/<>%&^|=!]=|--|\\+\\+|<<=?|>>=?|&&|\\|\\||::|->"),
> > +PATTERNS("sh",
> > +"^[ \t]*(function )?[A-Za-z_][A-Za-z_0-9]*[ \t]*()[\t]*\\{?$",
> 
> There is something funky going on around parentheses on this line.
> The ones around "function " is meant to be syntactic metacharacters
> to produce a group in the regexp so that you can apply '?'
> (i.e. zero or one occurrence) to it.  But I think the second pair of
> parentheses that appears later on the line, which enclose nothing,
> are meant to be literal?  E.g. "hello (){\n\techo world;\n}\n"  They
> would need some quoting, perhaps like
> 
>   ...[ \t]*\\(\\)[\t]*

Ah, I think I forgot to escape the quoting of ( and ). I will send in another
patch for that.

> > +/* -- */
> > +"(\\$|--?)?([a-zA-Z_][a-zA-Z0-9._]*|[0-9]+|#)|--" /* command/param */
> 
> TBH, I have no idea what this line-noise is doing.

That breaks word into "a", "$a" and "-a" as well as "$1" and "$#". I tried
supporting $? by adding +|#|\\?)--" but it doesn't seemed like it is working.

> $foobar, $4, --foobar, foobar, 123 and -- can be seen easily out of
> these patterns.  I am not sure what --# would be (perhaps you meant
> to only catch $# and --# is included by accident, in which case it
> is understandable).  It feels a bit strange to see that $# is
> supported but not $?; --foo but not --foo=bar; foobar but not "foo
> bar" inside a dq-pair.

Yes, getting --# will be very rare in shell. I think it is better to seperate
the --foo=bar into --foo and bar. I don't get what you man by the dq-pair.

> > +"|\\$[({]|[)}]|[-+*/=!]=?|[\\]&%#/|]{1,2}|[<>]{1,3}|[ \t]#.*"),
> 
> And this one is even more dense.

Yes, that takes care of the operators, special symbols and stuff.

Re: Re: Re: Microproject | Add more builtin patterns for userdiff

[Pickfire]

Jacob Keller <jacob.kel...@gmail.com> wrote:

> On Tue, Mar 28, 2017 at 10:53 AM, Pickfire <pickf...@riseup.net> wrote:
> >
> > Yes, I can't reproduce it outside the test suite. I have added the builtin
> > and yet the test fails. test_decode_color gets same output as expect but
> > still it fails, should I send in the patch?
> 
> You also need to ensure you have the exact same color settings as used
> by the test scripts.
> 
> Thanks,
> Jake

Yes, I used the same color, looks like the block that are failing is:

test_must_fail git diff --no-index "$@" pre post >output

Re: Re: Microproject | Add more builtin patterns for userdiff

[Pickfire]

Stefan Beller <sbel...@google.com> wrote:

> On Tue, Mar 28, 2017 at 3:46 AM, Pickfire <pickf...@riseup.net> wrote:
> 
> > EOF
> >
> > echo '* diff="cpp"' > .gitmodules
> 
> Did you mean .gitattributes?

Yeah, that's a spelling error.

Re: Re: Microproject | Add more builtin patterns for userdiff

[Pickfire]

Jacob Keller <jacob.kel...@gmail.com> wrote:

> On Tue, Mar 28, 2017 at 3:46 AM, Pickfire <pickf...@riseup.net> wrote:
> > While I was working buildins shell patterns for user diffs. I noticed that
> > the tests t4034 passes but I can reproduce it manually with:
> >
> > mkdir cpp/ && cd cpp/ && git init
> >
> > cat > pre < > Foo():x(0&&1){}
> > cout<<"Hello World!\n"<<endl;
> > 1 -1e10 0xabcdef 'x'
> > [a] a->b a.b
> > !a ~a a++ a-- a*b a
> > a*b a/b a%b
> > a+b a-b
> > a<>b
> > ab a>=b
> > a==b a!=b
> > a
> > a^b
> > a|b
> > a&
> > a||b
> > a?b:z
> > a=b a+=b a-=b a*=b a/=b a%=b a<<=b a>>=b a&=b a^=b a|=b
> > a,y
> > a::b
> > EOF
> >
> > cat > post < > Foo() : x(0&42) { bar(x); }
> > cout<<"Hello World?\n"<<endl;
> > (1) (-1e10) (0xabcdef) 'y'
> > [x] x->y x.y
> > !x ~x x++ x-- x*y x
> > x*y x/y x%y
> > x+y x-y
> > x<>y
> > xy x>=y
> > x==y x!=y
> > x
> > x^y
> > x|y
> > x&
> > x||y
> > x?y:z
> > x=y x+=y x-=y x*=y x/=y x%=y x<<=y x>>=y x&=y x^=y x|=y
> > x,y
> > x::y
> > EOF
> >
> > echo '* diff="cpp"' > .gitmodules
> > git diff --no-index --color-words pre post > output
> >
> > Surprisingly, it shows (which is very different from the expected output):
> >
> 
> The diff test code uses "test_decode_color" function which decodes the
> color commands into human readable text. From the looks of it, you're
> trying to reproduce the test outside the test suite. However, you're
> not decoding the colors using the test library function, so it doesn't
> look right.

Yes, I can't reproduce it outside the test suite. I have added the builtin
and yet the test fails. test_decode_color gets same output as expect but
still it fails, should I send in the patch?

Microproject | Add more builtin patterns for userdiff

[Pickfire]

While I was working buildins shell patterns for user diffs. I noticed that
the tests t4034 passes but I can reproduce it manually with:

mkdir cpp/ && cd cpp/ && git init

cat > pre b
ab a>=b
a==b a!=b
a
a^b
a|b
a&
a||b
a?b:z
a=b a+=b a-=b a*=b a/=b a%=b a<<=b a>>=b a&=b a^=b a|=b
a,y
a::b
EOF

cat > post y
xy x>=y
x==y x!=y
x
x^y
x|y
x&
x||y
x?y:z
x=y x+=y x-=y x*=y x/=y x%=y x<<=y x>>=y x&=y x^=y x|=y
x,y
x::y
EOF

echo '* diff="cpp"' > .gitmodules
git diff --no-index --color-words pre post > output

Surprisingly, it shows (which is very different from the expected output):

^[[1mdiff --git a/pre b/post^[[m
^[[1mindex 23d5c8a..7e8c026 100644^[[m
^[[1m--- a/pre^[[m
^[[1m+++ b/post^[[m
^[[36m@@ -1,19 +1,19 @@^[[m
^[[31mFoo():x(0&&1){}^[[m^[[32mFoo() : x(0&42) { bar(x); }^[[m
cout<<"Hello ^[[31mWorld!\n"<b a.b^[[m
^[[31m!a ~a a++ a-- a*b a^[[m
^[[31ma*b a/b a%b^[[m
^[[31ma+b a-b^[[m
^[[31ma<>b^[[m
^[[31mab a>=b^[[m
^[[31ma==b a!=b^[[m
^[[31ma^[[m
^[[31ma^b^[[m
^[[31ma|b^[[m
^[[31ma&^[[m
^[[31ma||b^[[m
^[[31ma?b:z^[[m
^[[31ma=b a+=b a-=b a*=b a/=b a%=b a<<=b a>>=b a&=b a^=b a|=b^[[m
^[[31ma,y^[[m
^[[31ma::b^[[m^[[32mWorld?\n"<y x.y^[[m
^[[32m!x ~x x++ x-- x*y x^[[m
^[[32mx*y x/y x%y^[[m
^[[32mx+y x-y^[[m
^[[32mx<>y^[[m
^[[32mxy x>=y^[[m
^[[32mx==y x!=y^[[m
^[[32mx^[[m
^[[32mx^y^[[m
^[[32mx|y^[[m
^[[32mx&^[[m
^[[32mx||y^[[m
^[[32mx?y:z^[[m
^[[32mx=y x+=y x-=y x*=y x/=y x%=y x<<=y x>>=y x&=y x^=y x|=y^[[m
^[[32mx,y^[[m
^[[32mx::y^[[m

Instead of:

diff --git a/pre b/post
index 23d5c8a..7e8c026 100644
--- a/pre
+++ b/post
@@ -1,19 +1,19 @@
Foo() : x(0&&1&42) { bar(x); }
cout<<"Hello World!?\n"<b 
ay x.by
!ax ~a ax x++ 
ax-- ax*b 
ay x&b
ay
x*b ay x/b ay 
x%b
ay
x+b ay x-b
ay
x<>b
ay
xb ay x>=b
ay
x==b ay x!=b
ay
x&b
ay
x^b
ay
x|b
ay
x&&b
ay
x||b
ay
x?by:z
ax=b ay x+=b 
ay x-=b ay x*=b 
ay x/=b ay x%=b 
ay x<<=b ay x>>=b 
ay x&=b ay x^=b 
ay x|=b
ay
x,y
ax::by

That's does not just happens to cpp builtins, it happens to bibtex as well.
Is it that I had missed some configuration since I have tested this on a
few machines?

I have a workaround for that, which is to run log with --color-words=
with regex from the userdiff.c, does it automatically use the builtins diff?