[GSoC][PATCH] userdiff: add built-in pattern for golang

[GSoC][PATCH] userdiff: add built-in pattern for golang

[Alban Gruin]

This adds xfuncname and word_regex patterns for golang, a quite
popular programming language. It also includes test cases for the
xfuncname regex (t4018) and an updated documentation.

The xfuncname regex finds functions, structs and interfaces. The
word_regex pattern finds identifiers, integers, floats, complex
numbers and operators, according to the go specification.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 Documentation/gitattributes.txt | 2 ++
 t/t4018-diff-funcname.sh        | 1 +
 t/t4018/golang-complex-function | 8 ++++++++
 t/t4018/golang-func             | 4 ++++
 t/t4018/golang-interface        | 4 ++++
 t/t4018/golang-long-func        | 5 +++++
 t/t4018/golang-struct           | 4 ++++
 userdiff.c                      | 9 +++++++++
 8 files changed, 37 insertions(+)
 create mode 100644 t/t4018/golang-complex-function
 create mode 100644 t/t4018/golang-func
 create mode 100644 t/t4018/golang-interface
 create mode 100644 t/t4018/golang-long-func
 create mode 100644 t/t4018/golang-struct

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index c21f5ca10..d52b254a2 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -714,6 +714,8 @@ patterns are available:
 
 - `fountain` suitable for Fountain documents.
 
+- `golang` suitable for source code in the Go language.
+
 - `html` suitable for HTML/XHTML documents.
 
 - `java` suitable for source code in the Java language.
diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index 1795ffc3a..22f9f88f0 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh
@@ -33,6 +33,7 @@ diffpatterns="
 	css
 	fortran
 	fountain
+	golang
 	html
 	java
 	matlab
diff --git a/t/t4018/golang-complex-function b/t/t4018/golang-complex-function
new file mode 100644
index 000000000..a9211ed18
--- /dev/null
+++ b/t/t4018/golang-complex-function
@@ -0,0 +1,8 @@
+type Test struct {
+	a Type
+}
+
+func (t *Test) RIGHT(a Type) (Type, error) {
+	t.a = a
+        return ChangeMe, nil
+}
diff --git a/t/t4018/golang-func b/t/t4018/golang-func
new file mode 100644
index 000000000..b17698218
--- /dev/null
+++ b/t/t4018/golang-func
@@ -0,0 +1,4 @@
+func RIGHT() {
+	a := 5
+        b := ChangeMe
+}
diff --git a/t/t4018/golang-interface b/t/t4018/golang-interface
new file mode 100644
index 000000000..2e03d4fc0
--- /dev/null
+++ b/t/t4018/golang-interface
@@ -0,0 +1,4 @@
+type RIGHT interface {
+	a() Type
+        b() ChangeMe
+}
diff --git a/t/t4018/golang-long-func b/t/t4018/golang-long-func
new file mode 100644
index 000000000..ac3a77b5c
--- /dev/null
+++ b/t/t4018/golang-long-func
@@ -0,0 +1,5 @@
+func RIGHT(aVeryVeryVeryLongVariableName AVeryVeryVeryLongType,
+	anotherLongVariableName AnotherLongType) {
+	a := 5
+	b := ChangeMe
+}
diff --git a/t/t4018/golang-struct b/t/t4018/golang-struct
new file mode 100644
index 000000000..5cd70c8dc
--- /dev/null
+++ b/t/t4018/golang-struct
@@ -0,0 +1,4 @@
+type RIGHT struct {
+	a Type
+        b ChangeMe
+}
diff --git a/userdiff.c b/userdiff.c
index dbfb4e13c..8f5028f6b 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -38,6 +38,15 @@ IPATTERN("fortran",
 	 "|//|\\*\\*|::|[/<>=]="),
 IPATTERN("fountain", "^((\\.[^.]|(int|ext|est|int\\.?/ext|i/e)[. ]).*)$",
 	 "[^ \t-]+"),
+PATTERNS("golang",
+	 /* Functions */
+	 "^[ \t]*(func[ \t]*.*(\\{[ \t]*)?)\n"
+	 /* Structs and interfaces */
+	 "^[ \t]*(type[ \t].*(struct|interface)[ \t]*(\\{[ \t]*)?)",
+	 /* -- */
+	 "[a-zA-Z_][a-zA-Z0-9_]*"
+	 "|[-+0-9.eE]+i?|0[xX]?[0-9a-fA-F]+i?"
+	 "|[-+*/<>%&^|=!:]=|--|\\+\\+|<<=?|>>=?|&\\^=?|&&|\\|\\||<-|\\.{3}"),
 PATTERNS("html", "^[ \t]*(<[Hh][1-6]([ \t].*)?>.*)$",
 	 "[^<>= \t]+"),
 PATTERNS("java",
-- 
2.16.1

Re: [GSoC][PATCH] userdiff: add built-in pattern for golang

[Eric Sunshine]

On Wed, Feb 28, 2018 at 12:29 PM, Alban Gruin <alban.gruin@gmail.com> wrote:
> This adds xfuncname and word_regex patterns for golang, a quite
> popular programming language. It also includes test cases for the
> xfuncname regex (t4018) and an updated documentation.

s/an //

> The xfuncname regex finds functions, structs and interfaces. The
> word_regex pattern finds identifiers, integers, floats, complex
> numbers and operators, according to the go specification.
>
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
> diff --git a/t/t4018/golang-complex-function b/t/t4018/golang-complex-function
> @@ -0,0 +1,8 @@
> +func (t *Test) RIGHT(a Type) (Type, error) {
> +       t.a = a
> +        return ChangeMe, nil
> +}

Most of these files use a mix of spaces and tabs for indentation.
Please use tabs only.

> diff --git a/userdiff.c b/userdiff.c
> @@ -38,6 +38,15 @@ IPATTERN("fortran",
> +PATTERNS("golang",
> +        /* Functions */
> +        "^[ \t]*(func[ \t]*.*(\\{[ \t]*)?)\n"

Why is the brace (and possible following whitespace) optional?
Considering that the language demands that the brace be on the same
line, I'd think the brace should be mandatory.

I suppose you made whitespace after 'func' optional to be able to
recognize a method (which hasn't been gofmt'd):

    func(x *X) name {

rather than the more typical:

    func (x *X) name {

I wonder if it would make sense to tighten the expression to recognize
functions and methods as distinct cases:

    function: mandatory whitespace following 'func'
    method: optional whitespace but mandatory '(' following 'func'

Your current expression could accidentally match:

    /*
      Fish like to have
      functors for lunch {
      just like eels}.
    */

but, even the suggested tighter expression could "accidentally" match
example code in a comment block anyhow, so I guess it probably doesn't
matter much in practice.

> +        /* Structs and interfaces */
> +        "^[ \t]*(type[ \t].*(struct|interface)[ \t]*(\\{[ \t]*)?)",

Whitespace is required after 'type'. Good. The language doesn't
require '{' to be on the same line as 'struct' or 'interface', and
this expression makes it optional. Okay.

> +        /* -- */
> +        "[a-zA-Z_][a-zA-Z0-9_]*"
> +        "|[-+0-9.eE]+i?|0[xX]?[0-9a-fA-F]+i?"
> +        "|[-+*/<>%&^|=!:]=|--|\\+\\+|<<=?|>>=?|&\\^=?|&&|\\|\\||<-|\\.{3}"),

Re: [GSoC][PATCH] userdiff: add built-in pattern for golang

[Alban Gruin]

>> diff --git a/userdiff.c b/userdiff.c
>> @@ -38,6 +38,15 @@ IPATTERN("fortran",
>> +PATTERNS("golang",
>> +        /* Functions */
>> +        "^[ \t]*(func[ \t]*.*(\\{[ \t]*)?)\n"
> 
> Why is the brace (and possible following whitespace) optional?
> Considering that the language demands that the brace be on the same
> line, I'd think the brace should be mandatory.
> 

I did this to support non-standard formatting. It's a niche case though,
maybe we could only support the standard formatting and modify the doc
to reflect this change.

> I suppose you made whitespace after 'func' optional to be able to
> recognize a method (which hasn't been gofmt'd):
> 
>     func(x *X) name {
> 
> rather than the more typical:
> 
>     func (x *X) name {
> 
> I wonder if it would make sense to tighten the expression to recognize
> functions and methods as distinct cases:
> 
>     function: mandatory whitespace following 'func'
>     method: optional whitespace but mandatory '(' following 'func'
> 
> Your current expression could accidentally match:
> 
>     /*
>       Fish like to have
>       functors for lunch {
>       just like eels}.
>     */
> 
> but, even the suggested tighter expression could "accidentally" match
> example code in a comment block anyhow, so I guess it probably doesn't
> matter much in practice.
> 

Same as before, I did this to support non-standard formatting.

Re: [GSoC][PATCH] userdiff: add built-in pattern for golang

[Eric Sunshine]

On Wed, Feb 28, 2018 at 4:31 PM, Alban Gruin <alban.gruin@gmail.com> wrote:
>>> diff --git a/userdiff.c b/userdiff.c
>>> @@ -38,6 +38,15 @@ IPATTERN("fortran",
>>> +PATTERNS("golang",
>>> +        /* Functions */
>>> +        "^[ \t]*(func[ \t]*.*(\\{[ \t]*)?)\n"
>>
>> Why is the brace (and possible following whitespace) optional?
>> Considering that the language demands that the brace be on the same
>> line, I'd think the brace should be mandatory.
>
> I did this to support non-standard formatting. It's a niche case though,
> maybe we could only support the standard formatting and modify the doc
> to reflect this change.

As noted, unlike 'struct' and 'interface', the brace for a 'func'
_must_ appear on the same line; that's a requirement of the language.
Placing it on a line is not an option.

    % cat >foo.go<<\EOF
    package foo
    func foo() {
    }
    EOF
    % go build foo.go

Versus:

    % cat >bar.go<<\EOF
    package bar
    func bar()
    {
    }
    EOF
    % go build bar.go
    ./bar.go:2:6: missing function body
    ./bar.go:3:1: syntax error: unexpected semicolon or newline before {

So, the regex probably ought to be strict about expecting the brace on
the same line as 'func'.

Re: [GSoC][PATCH] userdiff: add built-in pattern for golang

[Alban Gruin]

Le 28/02/2018 à 23:00, Eric Sunshine a écrit :
> On Wed, Feb 28, 2018 at 4:31 PM, Alban Gruin <alban.gruin@gmail.com> wrote:
>>>> diff --git a/userdiff.c b/userdiff.c
>>>> @@ -38,6 +38,15 @@ IPATTERN("fortran",
>>>> +PATTERNS("golang",
>>>> +        /* Functions */
>>>> +        "^[ \t]*(func[ \t]*.*(\\{[ \t]*)?)\n"
>>>
>>> Why is the brace (and possible following whitespace) optional?
>>> Considering that the language demands that the brace be on the same
>>> line, I'd think the brace should be mandatory.
>>
>> I did this to support non-standard formatting. It's a niche case though,
>> maybe we could only support the standard formatting and modify the doc
>> to reflect this change.
> 
> As noted, unlike 'struct' and 'interface', the brace for a 'func'
> _must_ appear on the same line; that's a requirement of the language.
> Placing it on a line is not an option.
> 
>     % cat >foo.go<<\EOF
>     package foo
>     func foo() {
>     }
>     EOF
>     % go build foo.go
> 
> Versus:
> 
>     % cat >bar.go<<\EOF
>     package bar
>     func bar()
>     {
>     }
>     EOF
>     % go build bar.go
>     ./bar.go:2:6: missing function body
>     ./bar.go:3:1: syntax error: unexpected semicolon or newline before {
> 
> So, the regex probably ought to be strict about expecting the brace on
> the same line as 'func'.
> 

Yes, but I can split the line like that:

    % cat >baz.go<<\EOF
    package baz
    func baz(arg1 int64,
    	arg2 int64) {
    }
    EOF
    % go build baz.go

This complies to the standard formatting (at least, gofmt doesn't change
it), but making the regex strict about the brace would cause it to
ignore those funcs, although I don't know how common they are.

Re: [GSoC][PATCH] userdiff: add built-in pattern for golang

[Eric Sunshine]

On Wed, Feb 28, 2018 at 5:17 PM, Alban Gruin <alban.gruin@gmail.com> wrote:
> Yes, but I can split the line like that:
>
>     % cat >baz.go<<\EOF
>     package baz
>     func baz(arg1 int64,
>         arg2 int64) {
>     }
>     EOF
>     % go build baz.go
>
> This complies to the standard formatting (at least, gofmt doesn't change
> it), but making the regex strict about the brace would cause it to
> ignore those funcs, although I don't know how common they are.

Makes sense. Thanks for the clarifying example. I wouldn't be at all
surprised it such formatting exists in the wild, so keeping the regex
as-is seems a good idea.

Re: [GSoC][PATCH] userdiff: add built-in pattern for golang

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Wed, Feb 28, 2018 at 5:17 PM, Alban Gruin <alban.gruin@gmail.com> wrote:
>> Yes, but I can split the line like that:
>>
>>     % cat >baz.go<<\EOF
>>     package baz
>>     func baz(arg1 int64,
>>         arg2 int64) {
>>     }
>>     EOF
>>     % go build baz.go
>>
>> This complies to the standard formatting (at least, gofmt doesn't change
>> it), but making the regex strict about the brace would cause it to
>> ignore those funcs, although I don't know how common they are.
>
> Makes sense. Thanks for the clarifying example. I wouldn't be at all
> surprised it such formatting exists in the wild, so keeping the regex
> as-is seems a good idea.

Does input like that appear in the tests the patch adds?  If not, it
probably is a good idea to have it somewhere in the commit (even if
there is no test addition, having it in the log message to explain
why the regex is done like so would be a good idea).

Thanks.

Re: [GSoC][PATCH] userdiff: add built-in pattern for golang

[Alban Gruin]

Le 28/02/2018 à 23:32, Junio C Hamano a écrit :
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
>> On Wed, Feb 28, 2018 at 5:17 PM, Alban Gruin <alban.gruin@gmail.com> wrote:
>>> Yes, but I can split the line like that:
>>>
>>>     % cat >baz.go<<\EOF
>>>     package baz
>>>     func baz(arg1 int64,
>>>         arg2 int64) {
>>>     }
>>>     EOF
>>>     % go build baz.go
>>>
>>> This complies to the standard formatting (at least, gofmt doesn't change
>>> it), but making the regex strict about the brace would cause it to
>>> ignore those funcs, although I don't know how common they are.
>>
>> Makes sense. Thanks for the clarifying example. I wouldn't be at all
>> surprised it such formatting exists in the wild, so keeping the regex
>> as-is seems a good idea.
> 
> Does input like that appear in the tests the patch adds?  If not, it
> probably is a good idea to have it somewhere in the commit (even if
> there is no test addition, having it in the log message to explain
> why the regex is done like so would be a good idea).
> 
> Thanks.
> 

Yes, it's in a file called "golang-long-func".  I'll send another patch
later to fix the typo in the commit message and the indentation in the
tests. I'll clarify the regex in the message, too.

[GSoC][PATCH v2] userdiff: add built-in pattern for golang

[Alban Gruin]

This adds xfuncname and word_regex patterns for golang, a quite
popular programming language. It also includes test cases for the
xfuncname regex (t4018) and updated documentation.

The xfuncname regex finds functions, structs and interfaces.  Although
the Go language prohibits the opening brace from being on its own
line, the regex does not makes it mandatory, to be able to match
`func` statements like this:

    func foo(bar int,
    	baz int) {
    }

This is covered by the test case t4018/golang-long-func.

The word_regex pattern finds identifiers, integers, floats, complex
numbers and operators, according to the go specification.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 Documentation/gitattributes.txt | 2 ++
 t/t4018-diff-funcname.sh        | 1 +
 t/t4018/golang-complex-function | 8 ++++++++
 t/t4018/golang-func             | 4 ++++
 t/t4018/golang-interface        | 4 ++++
 t/t4018/golang-long-func        | 5 +++++
 t/t4018/golang-struct           | 4 ++++
 userdiff.c                      | 9 +++++++++
 8 files changed, 37 insertions(+)
 create mode 100644 t/t4018/golang-complex-function
 create mode 100644 t/t4018/golang-func
 create mode 100644 t/t4018/golang-interface
 create mode 100644 t/t4018/golang-long-func
 create mode 100644 t/t4018/golang-struct

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index c21f5ca10..d52b254a2 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -714,6 +714,8 @@ patterns are available:
 
 - `fountain` suitable for Fountain documents.
 
+- `golang` suitable for source code in the Go language.
+
 - `html` suitable for HTML/XHTML documents.
 
 - `java` suitable for source code in the Java language.
diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index 1795ffc3a..22f9f88f0 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh
@@ -33,6 +33,7 @@ diffpatterns="
 	css
 	fortran
 	fountain
+	golang
 	html
 	java
 	matlab
diff --git a/t/t4018/golang-complex-function b/t/t4018/golang-complex-function
new file mode 100644
index 000000000..e057dcefe
--- /dev/null
+++ b/t/t4018/golang-complex-function
@@ -0,0 +1,8 @@
+type Test struct {
+	a Type
+}
+
+func (t *Test) RIGHT(a Type) (Type, error) {
+	t.a = a
+	return ChangeMe, nil
+}
diff --git a/t/t4018/golang-func b/t/t4018/golang-func
new file mode 100644
index 000000000..8e9c9ac7c
--- /dev/null
+++ b/t/t4018/golang-func
@@ -0,0 +1,4 @@
+func RIGHT() {
+	a := 5
+	b := ChangeMe
+}
diff --git a/t/t4018/golang-interface b/t/t4018/golang-interface
new file mode 100644
index 000000000..553bedec9
--- /dev/null
+++ b/t/t4018/golang-interface
@@ -0,0 +1,4 @@
+type RIGHT interface {
+	a() Type
+	b() ChangeMe
+}
diff --git a/t/t4018/golang-long-func b/t/t4018/golang-long-func
new file mode 100644
index 000000000..ac3a77b5c
--- /dev/null
+++ b/t/t4018/golang-long-func
@@ -0,0 +1,5 @@
+func RIGHT(aVeryVeryVeryLongVariableName AVeryVeryVeryLongType,
+	anotherLongVariableName AnotherLongType) {
+	a := 5
+	b := ChangeMe
+}
diff --git a/t/t4018/golang-struct b/t/t4018/golang-struct
new file mode 100644
index 000000000..5deda77fe
--- /dev/null
+++ b/t/t4018/golang-struct
@@ -0,0 +1,4 @@
+type RIGHT struct {
+	a Type
+	b ChangeMe
+}
diff --git a/userdiff.c b/userdiff.c
index dbfb4e13c..8f5028f6b 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -38,6 +38,15 @@ IPATTERN("fortran",
 	 "|//|\\*\\*|::|[/<>=]="),
 IPATTERN("fountain", "^((\\.[^.]|(int|ext|est|int\\.?/ext|i/e)[. ]).*)$",
 	 "[^ \t-]+"),
+PATTERNS("golang",
+	 /* Functions */
+	 "^[ \t]*(func[ \t]*.*(\\{[ \t]*)?)\n"
+	 /* Structs and interfaces */
+	 "^[ \t]*(type[ \t].*(struct|interface)[ \t]*(\\{[ \t]*)?)",
+	 /* -- */
+	 "[a-zA-Z_][a-zA-Z0-9_]*"
+	 "|[-+0-9.eE]+i?|0[xX]?[0-9a-fA-F]+i?"
+	 "|[-+*/<>%&^|=!:]=|--|\\+\\+|<<=?|>>=?|&\\^=?|&&|\\|\\||<-|\\.{3}"),
 PATTERNS("html", "^[ \t]*(<[Hh][1-6]([ \t].*)?>.*)$",
 	 "[^<>= \t]+"),
 PATTERNS("java",
-- 
2.16.1

Re: [GSoC][PATCH v2] userdiff: add built-in pattern for golang

[Eric Sunshine]

On Thu, Mar 1, 2018 at 6:19 AM, Alban Gruin <alban.gruin@gmail.com> wrote:
> This adds xfuncname and word_regex patterns for golang, a quite
> popular programming language. It also includes test cases for the
> xfuncname regex (t4018) and updated documentation.
>
> The xfuncname regex finds functions, structs and interfaces.  Although
> the Go language prohibits the opening brace from being on its own
> line, the regex does not makes it mandatory, to be able to match
> `func` statements like this:
>
>     func foo(bar int,
>         baz int) {
>     }
>
> This is covered by the test case t4018/golang-long-func.

A possible suggested rewrite to make it flow a bit better and to
mention the loose whitespace matching:

    The xfuncname regex finds functions, structs and interfaces.
    Although the Go language prohibits the opening brace of a 'func'
    from being on its own line, the regex makes the brace optional so
    it can match function declarations wrapped over multiple lines
    (covered by new test case t4018/golang-long-func):

        func foo(bar int,
            baz int) {
        }

    Whitespace matching is also a bit lax in order to handle
    non-standard formatting of method declarations. For instance:

        func(x *X) foo() {

    versus typical 'gofmt' formatted:

        func (x *x) foo() {

(Not necessarily worth a re-roll; perhaps Junio can pick it up when
queueing if he considers it an improvement.)

Thanks.

> The word_regex pattern finds identifiers, integers, floats, complex
> numbers and operators, according to the go specification.
>
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>