[PATCH] userdiff: Add a builtin pattern for dts files

[PATCH] userdiff: Add a builtin pattern for dts files

[Stephen Boyd]

The Linux kernel receives many patches to the devicetree files each
release. The hunk header for those patches typically show nothing,
making it difficult to figure out what node is being modified without
applying the patch or opening the file and seeking to the context. Let's
add a builtin 'dts' pattern to git so that users can get better diff
output on dts files when they use the diff=dts driver.

The regex has been constructed based on the spec at devicetree.org[1]

[1] https://github.com/devicetree-org/devicetree-specification/releases/latest

Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 Documentation/gitattributes.txt |  2 ++
 t/t4018-diff-funcname.sh        |  1 +
 t/t4018/dts-labels              |  8 +++++++
 t/t4018/dts-node-unitless       |  8 +++++++
 t/t4018/dts-nodes               |  8 +++++++
 t/t4018/dts-reference           |  8 +++++++
 t/t4034-diff-words.sh           |  1 +
 t/t4034/dts/expect              | 37 +++++++++++++++++++++++++++++++++
 t/t4034/dts/post                | 32 ++++++++++++++++++++++++++++
 t/t4034/dts/pre                 | 32 ++++++++++++++++++++++++++++
 userdiff.c                      |  9 ++++++++
 11 files changed, 146 insertions(+)
 create mode 100644 t/t4018/dts-labels
 create mode 100644 t/t4018/dts-node-unitless
 create mode 100644 t/t4018/dts-nodes
 create mode 100644 t/t4018/dts-reference
 create mode 100644 t/t4034/dts/expect
 create mode 100644 t/t4034/dts/post
 create mode 100644 t/t4034/dts/pre

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index b8392fc3300c..14e5784b962d 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -805,6 +805,8 @@ patterns are available:
 
 - `css` suitable for cascading style sheets.
 
+- `dts` suitable for devicetree (DTS) files.
+
 - `fortran` suitable for source code in the Fortran language.
 
 - `fountain` suitable for Fountain documents.
diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index 22f9f88f0afc..8acd04b206d4 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh
@@ -31,6 +31,7 @@ diffpatterns="
 	cpp
 	csharp
 	css
+	dts
 	fortran
 	fountain
 	golang
diff --git a/t/t4018/dts-labels b/t/t4018/dts-labels
new file mode 100644
index 000000000000..27cd4921cfb6
--- /dev/null
+++ b/t/t4018/dts-labels
@@ -0,0 +1,8 @@
+/ {
+	label_1: node1@ff00 {
+		label2: RIGHT {
+			vendor,some-property;
+			ChangeMe = <0x45-30>;
+		};
+	};
+};
diff --git a/t/t4018/dts-node-unitless b/t/t4018/dts-node-unitless
new file mode 100644
index 000000000000..c5287d91416e
--- /dev/null
+++ b/t/t4018/dts-node-unitless
@@ -0,0 +1,8 @@
+/ {
+	label_1: node1 {
+		RIGHT {
+			prop-array = <1>, <4>;
+			ChangeMe = <0xffeedd00>;
+		};
+	};
+};
diff --git a/t/t4018/dts-nodes b/t/t4018/dts-nodes
new file mode 100644
index 000000000000..5a4334bb1645
--- /dev/null
+++ b/t/t4018/dts-nodes
@@ -0,0 +1,8 @@
+/ {
+	label_1: node1@ff00 {
+		RIGHT@deadf00,4000 {
+			#size-cells = <1>;
+			ChangeMe = <0xffeedd00>;
+		};
+	};
+};
diff --git a/t/t4018/dts-reference b/t/t4018/dts-reference
new file mode 100644
index 000000000000..f115d4291d25
--- /dev/null
+++ b/t/t4018/dts-reference
@@ -0,0 +1,8 @@
+&label_1 {
+	TEST = <455>;
+};
+
+&RIGHT {
+	vendor,some-property;
+	ChangeMe = <0x45-30>;
+};
diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
index 912df91226f2..9a93c2a3e0dd 100755
--- a/t/t4034-diff-words.sh
+++ b/t/t4034-diff-words.sh
@@ -303,6 +303,7 @@ test_language_driver bibtex
 test_language_driver cpp
 test_language_driver csharp
 test_language_driver css
+test_language_driver dts
 test_language_driver fortran
 test_language_driver html
 test_language_driver java
diff --git a/t/t4034/dts/expect b/t/t4034/dts/expect
new file mode 100644
index 000000000000..ed6ad9c65f8b
--- /dev/null
+++ b/t/t4034/dts/expect
@@ -0,0 +1,37 @@
+<BOLD>diff --git a/pre b/post<RESET>
+<BOLD>index ce95e99..7803aee 100644<RESET>
+<BOLD>--- a/pre<RESET>
+<BOLD>+++ b/post<RESET>
+<CYAN>@@ -1,32 +1,32 @@<RESET>
+/ {<RESET>
+	<RED>this_handle<RESET><GREEN>HANDLE_2<RESET>: <RED>node<RESET><GREEN>new-node<RESET>@<RED>f00<RESET><GREEN>eeda<RESET> {
+		compatible = "<RED>mydev<RESET><GREEN>vendor,compat<RESET>";
+		string-prop = <RED>start<RESET><GREEN>end<RESET>: "hello <RED>world!<RESET><GREEN>world?<RESET>" <RED>end<RESET><GREEN>start<RESET>: ;
+		<RED>#size-cells<RESET><GREEN>#address-cells<RESET> = <<RED>0+0<RESET><GREEN>0+40<RESET>>;
+		reg = <<RED>0xf00<RESET><GREEN>0xeeda<RESET>>;
+		prop = <<GREEN>(<RESET>1<GREEN>)<RESET>>;
+		prop = <<GREEN>(<RESET>-1e10<GREEN>)<RESET>>;
+		prop = <(!<RED>3<RESET><GREEN>1<RESET>)>;
+		prop = <(~<RED>3<RESET><GREEN>1<RESET>)>;
+		prop = <(<RED>3<RESET><GREEN>1<RESET>*<RED>4<RESET><GREEN>2<RESET>)>;
+		prop = <(<RED>3<RESET><GREEN>1<RESET>&<RED>4<RESET><GREEN>2<RESET>)>;
+		prop = <(<RED>3<RESET><GREEN>1<RESET>*<RED>4<RESET><GREEN>2<RESET>)>;
+		prop = <(<RED>3<RESET><GREEN>1<RESET>/<RED>4<RESET><GREEN>2<RESET>)>;
+		prop = <(<RED>3<RESET><GREEN>1<RESET>%<RED>4<RESET><GREEN>2<RESET>)>;
+		prop = <(<RED>3+4<RESET><GREEN>1+2<RESET>)>;
+		prop = <(<RED>3-4<RESET><GREEN>1-2<RESET>)>;
+		prop = /bits/ <RED>64<RESET><GREEN>32<RESET> <(<RED>3<RESET><GREEN>1<RESET><<<RED>4<RESET><GREEN>2<RESET>)>;
+		prop = <(<RED>3<RESET><GREEN>1<RESET>>><RED>4<RESET><GREEN>2<RESET>)>;
+		prop = <(<RED>3<RESET><GREEN>1<RESET>&<RED>4<RESET><GREEN>2<RESET>)>;
+		prop = <(<RED>3<RESET><GREEN>1<RESET>^<RED>4<RESET><GREEN>2<RESET>)>;
+		prop = <(<RED>3<RESET><GREEN>1<RESET>|<RED>4<RESET><GREEN>2<RESET>)>;
+		prop = <(<RED>3<RESET><GREEN>1<RESET>&&<RED>4<RESET><GREEN>2<RESET>)>;
+		prop = <(<RED>3<RESET><GREEN>1<RESET>||<RED>4<RESET><GREEN>2<RESET>)>;
+		prop = <(<RED>4?5<RESET><GREEN>1?2<RESET>:<RED>6<RESET><GREEN>3<RESET>)>;
+		list = <&<RED>this_handle<RESET><GREEN>HANDLE_2<RESET>>, <0 0 0 <RED>0<RESET><GREEN>1<RESET>>;
+	};<RESET>
+
+	&<RED>phandle<RESET><GREEN>phandle2<RESET> {
+		<RED>pre-phandle<RESET><GREEN>prop_handle<RESET> = <&<RED>this_handle<RESET><GREEN>HANDLE_2<RESET>>;
+	};<RESET>
+};<RESET>
diff --git a/t/t4034/dts/post b/t/t4034/dts/post
new file mode 100644
index 000000000000..7803aee28093
--- /dev/null
+++ b/t/t4034/dts/post
@@ -0,0 +1,32 @@
+/ {
+	HANDLE_2: new-node@eeda {
+		compatible = "vendor,compat";
+		string-prop = end: "hello world?" start: ;
+		#address-cells = <0+40>;
+		reg = <0xeeda>;
+		prop = <(1)>;
+		prop = <(-1e10)>;
+		prop = <(!1)>;
+		prop = <(~1)>;
+		prop = <(1*2)>;
+		prop = <(1&2)>;
+		prop = <(1*2)>;
+		prop = <(1/2)>;
+		prop = <(1%2)>;
+		prop = <(1+2)>;
+		prop = <(1-2)>;
+		prop = /bits/ 32 <(1<<2)>;
+		prop = <(1>>2)>;
+		prop = <(1&2)>;
+		prop = <(1^2)>;
+		prop = <(1|2)>;
+		prop = <(1&&2)>;
+		prop = <(1||2)>;
+		prop = <(1?2:3)>;
+		list = <&HANDLE_2>, <0 0 0 1>;
+	};
+
+	&phandle2 {
+		prop_handle = <&HANDLE_2>;
+	};
+};
diff --git a/t/t4034/dts/pre b/t/t4034/dts/pre
new file mode 100644
index 000000000000..ce95e993ec93
--- /dev/null
+++ b/t/t4034/dts/pre
@@ -0,0 +1,32 @@
+/ {
+	this_handle: node@f00 {
+		compatible = "mydev";
+		string-prop = start: "hello world!" end: ;
+		#size-cells = <0+0>;
+		reg = <0xf00>;
+		prop = <1>;
+		prop = <-1e10>;
+		prop = <(!3)>;
+		prop = <(~3)>;
+		prop = <(3*4)>;
+		prop = <(3&4)>;
+		prop = <(3*4)>;
+		prop = <(3/4)>;
+		prop = <(3%4)>;
+		prop = <(3+4)>;
+		prop = <(3-4)>;
+		prop = /bits/ 64 <(3<<4)>;
+		prop = <(3>>4)>;
+		prop = <(3&4)>;
+		prop = <(3^4)>;
+		prop = <(3|4)>;
+		prop = <(3&&4)>;
+		prop = <(3||4)>;
+		prop = <(4?5:6)>;
+		list = <&this_handle>, <0 0 0 0>;
+	};
+
+	&phandle {
+		pre-phandle = <&this_handle>;
+	};
+};
diff --git a/userdiff.c b/userdiff.c
index 97007abe5b16..2bc964e11089 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -23,6 +23,15 @@ IPATTERN("ada",
 	 "[a-zA-Z][a-zA-Z0-9_]*"
 	 "|[-+]?[0-9][0-9#_.aAbBcCdDeEfF]*([eE][+-]?[0-9_]+)?"
 	 "|=>|\\.\\.|\\*\\*|:=|/=|>=|<=|<<|>>|<>"),
+PATTERNS("dts",
+	 /* Node name (with optional label and unit address) */
+	 "^[ \t]*((([a-zA-Z_][a-zA-Z0-9_]*: )?[a-zA-Z][a-zA-Z0-9,._+-]*(@[a-zA-Z0-9,._+-]+)?"
+	 /* Reference */
+	 "|&[a-zA-Z_][a-zA-Z0-9_]*[ \t]*)[ \t]*\\{)[ \t]*$",
+	 /* -- */
+	 /* Property names and math operators */
+	 "[a-zA-Z0-9,._+?#-]+"
+	 "|[-+*/%&^|!~]"),
 IPATTERN("fortran",
 	 "!^([C*]|[ \t]*!)\n"
 	 "!^[ \t]*MODULE[ \t]+PROCEDURE[ \t]\n"
-- 
Sent by a computer through tubes

Re: [PATCH] userdiff: Add a builtin pattern for dts files

[Alban Gruin]

Hi Stephen,

thank you for your patch.  I left a few comments below.

Le 11/01/2019 à 22:51, Stephen Boyd a écrit :
> The Linux kernel receives many patches to the devicetree files each
> release. The hunk header for those patches typically show nothing,
> making it difficult to figure out what node is being modified without
> applying the patch or opening the file and seeking to the context. Let's
> add a builtin 'dts' pattern to git so that users can get better diff
> output on dts files when they use the diff=dts driver.
> 
> The regex has been constructed based on the spec at devicetree.org[1]
> 
> [1] https://github.com/devicetree-org/devicetree-specification/releases/latest
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> ---
>  Documentation/gitattributes.txt |  2 ++
>  t/t4018-diff-funcname.sh        |  1 +
>  t/t4018/dts-labels              |  8 +++++++
>  t/t4018/dts-node-unitless       |  8 +++++++
>  t/t4018/dts-nodes               |  8 +++++++
>  t/t4018/dts-reference           |  8 +++++++
>  t/t4034-diff-words.sh           |  1 +
>  t/t4034/dts/expect              | 37 +++++++++++++++++++++++++++++++++
>  t/t4034/dts/post                | 32 ++++++++++++++++++++++++++++
>  t/t4034/dts/pre                 | 32 ++++++++++++++++++++++++++++
>  userdiff.c                      |  9 ++++++++
>  11 files changed, 146 insertions(+)
>  create mode 100644 t/t4018/dts-labels
>  create mode 100644 t/t4018/dts-node-unitless
>  create mode 100644 t/t4018/dts-nodes
>  create mode 100644 t/t4018/dts-reference
>  create mode 100644 t/t4034/dts/expect
>  create mode 100644 t/t4034/dts/post
>  create mode 100644 t/t4034/dts/pre
> 
> -%<-
> diff --git a/userdiff.c b/userdiff.c
> index 97007abe5b16..2bc964e11089 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -23,6 +23,15 @@ IPATTERN("ada",
>  	 "[a-zA-Z][a-zA-Z0-9_]*"
>  	 "|[-+]?[0-9][0-9#_.aAbBcCdDeEfF]*([eE][+-]?[0-9_]+)?"
>  	 "|=>|\\.\\.|\\*\\*|:=|/=|>=|<=|<<|>>|<>"),
> +PATTERNS("dts",
> +	 /* Node name (with optional label and unit address) */
> +	 "^[ \t]*((([a-zA-Z_][a-zA-Z0-9_]*: )?[a-zA-Z][a-zA-Z0-9,._+-]*(@[a-zA-Z0-9,._+-]+)?"

From the spec, label and node names “shall be [between] 1 to 31
characters in length”.  It’s not enforced here, and I guess it’s not
really git’s job to check for this kind of rule.  Others may disagree
with me, though.

Should labels end with exactly one space after the colon, or can there
be more, or none at all?

> +	 /* Reference */
> +	 "|&[a-zA-Z_][a-zA-Z0-9_]*[ \t]*)[ \t]*\\{)[ \t]*$",

It’s not specified in the spec, but these lines must end with a curly
brace?  What if there is a comment after the curly brace?

This pattern does not match the root node, but I guess it’s not
important as most of the interesting stuff in a dts is not directly in it.

> +	 /* -- */
> +	 /* Property names and math operators */
> +	 "[a-zA-Z0-9,._+?#-]+"
> +	 "|[-+*/%&^|!~]"),

There is a `%' operator here and in your tests, but it’s not mentioned
in the spec if I’m not mistaken.  Does it actually exists?

>  IPATTERN("fortran",
>  	 "!^([C*]|[ \t]*!)\n"
>  	 "!^[ \t]*MODULE[ \t]+PROCEDURE[ \t]\n"
> 
Cheers,
Alban

Re: [PATCH] userdiff: Add a builtin pattern for dts files

[Junio C Hamano]

Alban Gruin <alban.gruin@gmail.com> writes:

> thank you for your patch.  I left a few comments below.
>
> Le 11/01/2019 à 22:51, Stephen Boyd a écrit:
>> The Linux kernel receives many patches to the devicetree files each
>> release. The hunk header for those patches typically show nothing,
>> making it difficult to figure out what node is being modified without
>> applying the patch or opening the file and seeking to the context. Let's
>> add a builtin 'dts' pattern to git so that users can get better diff
>> output on dts files when they use the diff=dts driver.

A sort of meta-question.

What is missing in the current git that prevents the folks involved
in device-tree project from achieving what this patch tries to
accomplish without having to wait the Git project to act on it?  To
put it another way, is it a symptom of a bad design that from time
to time the Git project has to add built-in patterns?

Ability to ship arbitrary piece of text that you would normally
place in .git/config is not exactly an answer to the above question,
and will not happen as that has grave security implications.

But perhaps we can start accepting an in-tree config-like file whose
contents are limited to verified-safe settings
(e.g. "diff.*.xfuncname" and nothing else), so that projects can
ship two files in-tree:

 - ".gitattributes" that says "*.dts diff=dts"

 - ".gitpreferences" that says "[diff "dts"] xfuncname=..." to
   define the pattern the patch under review adds.

without waiting for the next release of Git to add one more built-in
pattern?

Anything that defines executable (e.g. "diff.*.command") should
never be accepted as part of the in-tree config-like file (for two
reasons: security and portability), but there should be some
"obviously safe" subset of config settings that we can allow project
to impose on its users, I hope.

Re: [PATCH] userdiff: Add a builtin pattern for dts files

[Stephen Boyd]

Quoting Alban Gruin (2019-01-13 13:26:21)
> Hi Stephen,
> 
> thank you for your patch.  I left a few comments below.
> 
> Le 11/01/2019 à 22:51, Stephen Boyd a écrit :
> > diff --git a/userdiff.c b/userdiff.c
> > index 97007abe5b16..2bc964e11089 100644
> > --- a/userdiff.c
> > +++ b/userdiff.c
> > @@ -23,6 +23,15 @@ IPATTERN("ada",
> >        "[a-zA-Z][a-zA-Z0-9_]*"
> >        "|[-+]?[0-9][0-9#_.aAbBcCdDeEfF]*([eE][+-]?[0-9_]+)?"
> >        "|=>|\\.\\.|\\*\\*|:=|/=|>=|<=|<<|>>|<>"),
> > +PATTERNS("dts",
> > +      /* Node name (with optional label and unit address) */
> > +      "^[ \t]*((([a-zA-Z_][a-zA-Z0-9_]*: )?[a-zA-Z][a-zA-Z0-9,._+-]*(@[a-zA-Z0-9,._+-]+)?"
> 
> From the spec, label and node names “shall be [between] 1 to 31
> characters in length”.  It’s not enforced here, and I guess it’s not
> really git’s job to check for this kind of rule.  Others may disagree
> with me, though.
> 
> Should labels end with exactly one space after the colon, or can there
> be more, or none at all?

There can be any number of spaces after the colon. I can fix the regex
here to accept any amount of whitespace after the colon.

> 
> > +      /* Reference */
> > +      "|&[a-zA-Z_][a-zA-Z0-9_]*[ \t]*)[ \t]*\\{)[ \t]*$",
> 
> It’s not specified in the spec, but these lines must end with a curly
> brace?  

That isn't common but it is supported. I can change the regex to look
for a line that ends in '{' or something that isn't ';' with anything
after the node name?

> What if there is a comment after the curly brace?

There can be a comment after the curly brace or before the curly brace.
The spec allows C style /* */ type comments, in addition to C++ style //
comments. I've never really seen that happen in practice though so it's
not very common. Grepping the linux sources shows two hits:

arch/arm/boot/dts/lpc3250-ea3250.dts:&ohci /* &usbd */ {
arch/arm/boot/dts/lpc3250-phy3250.dts:&ohci /* &usbd */ {

> 
> This pattern does not match the root node, but I guess it’s not
> important as most of the interesting stuff in a dts is not directly in it.

Agreed.

> 
> > +      /* -- */
> > +      /* Property names and math operators */
> > +      "[a-zA-Z0-9,._+?#-]+"
> > +      "|[-+*/%&^|!~]"),
> 
> There is a `%' operator here and in your tests, but it’s not mentioned
> in the spec if I’m not mistaken.  Does it actually exists?

The compiler doesn't seem to complain when it's used. I can send a patch
to update the spec for this rather esoteric feature. I can also include
more tests and support for the boolean relational operators which also
seem to be supported but probably never used.

Re: [PATCH] userdiff: Add a builtin pattern for dts files

[Alban Gruin]

Hi Junio,

Le 14/01/2019 à 19:34, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@gmail.com> writes:
> 
>> thank you for your patch.  I left a few comments below.
>>
>> Le 11/01/2019 à 22:51, Stephen Boyd a écrit:
>>> The Linux kernel receives many patches to the devicetree files each
>>> release. The hunk header for those patches typically show nothing,
>>> making it difficult to figure out what node is being modified without
>>> applying the patch or opening the file and seeking to the context. Let's
>>> add a builtin 'dts' pattern to git so that users can get better diff
>>> output on dts files when they use the diff=dts driver.
> 
> A sort of meta-question.
> 
> What is missing in the current git that prevents the folks involved
> in device-tree project from achieving what this patch tries to
> accomplish without having to wait the Git project to act on it?  To
> put it another way, is it a symptom of a bad design that from time
> to time the Git project has to add built-in patterns?
> 
> Ability to ship arbitrary piece of text that you would normally
> place in .git/config is not exactly an answer to the above question,
> and will not happen as that has grave security implications.
> 
> But perhaps we can start accepting an in-tree config-like file whose
> contents are limited to verified-safe settings
> (e.g. "diff.*.xfuncname" and nothing else), so that projects can
> ship two files in-tree:
> 
>  - ".gitattributes" that says "*.dts diff=dts"
> 
>  - ".gitpreferences" that says "[diff "dts"] xfuncname=..." to
>    define the pattern the patch under review adds.
> 
> without waiting for the next release of Git to add one more built-in
> pattern?
> 
> Anything that defines executable (e.g. "diff.*.command") should
> never be accepted as part of the in-tree config-like file (for two
> reasons: security and portability), but there should be some
> "obviously safe" subset of config settings that we can allow project
> to impose on its users, I hope.
> 

I really don’t know what to think about this.  I like your proposal, but
it will take some time to write such a feature, while there is a patch
almost ready to support the dts syntax.  But I guess that if it is
merged, it will be nearly-impossible to remove from the source if a
feature like you proposed is eventually implemented.

Re: [PATCH] userdiff: Add a builtin pattern for dts files

[Alban Gruin]

Hi, sorry for the late answer.

Le 14/01/2019 à 19:34, Stephen Boyd a écrit :
> Quoting Alban Gruin (2019-01-13 13:26:21)
>> Hi Stephen,
>>
>> thank you for your patch.  I left a few comments below.
>>
>> Le 11/01/2019 à 22:51, Stephen Boyd a écrit :
>>> diff --git a/userdiff.c b/userdiff.c
>>> index 97007abe5b16..2bc964e11089 100644
>>> --- a/userdiff.c
>>> +++ b/userdiff.c
>>> @@ -23,6 +23,15 @@ IPATTERN("ada",
>>>        "[a-zA-Z][a-zA-Z0-9_]*"
>>>        "|[-+]?[0-9][0-9#_.aAbBcCdDeEfF]*([eE][+-]?[0-9_]+)?"
>>>        "|=>|\\.\\.|\\*\\*|:=|/=|>=|<=|<<|>>|<>"),
>>> +PATTERNS("dts",
>>> +      /* Node name (with optional label and unit address) */
>>> +      "^[ \t]*((([a-zA-Z_][a-zA-Z0-9_]*: )?[a-zA-Z][a-zA-Z0-9,._+-]*(@[a-zA-Z0-9,._+-]+)?"
>>
>> From the spec, label and node names “shall be [between] 1 to 31
>> characters in length”.  It’s not enforced here, and I guess it’s not
>> really git’s job to check for this kind of rule.  Others may disagree
>> with me, though.
>>
>> Should labels end with exactly one space after the colon, or can there
>> be more, or none at all?
> 
> There can be any number of spaces after the colon. I can fix the regex
> here to accept any amount of whitespace after the colon.
> 
>>
>>> +      /* Reference */
>>> +      "|&[a-zA-Z_][a-zA-Z0-9_]*[ \t]*)[ \t]*\\{)[ \t]*$",
>>
>> It’s not specified in the spec, but these lines must end with a curly
>> brace?  
> 
> That isn't common but it is supported. I can change the regex to look
> for a line that ends in '{' or something that isn't ';' with anything
> after the node name?
> 
>> What if there is a comment after the curly brace?
> 
> There can be a comment after the curly brace or before the curly brace.
> The spec allows C style /* */ type comments, in addition to C++ style //
> comments. I've never really seen that happen in practice though so it's
> not very common. Grepping the linux sources shows two hits:
> 
> arch/arm/boot/dts/lpc3250-ea3250.dts:&ohci /* &usbd */ {
> arch/arm/boot/dts/lpc3250-phy3250.dts:&ohci /* &usbd */ {
> 

I grepped through Linux and uboot’s sources and it seems that “it is not
common” is actually “it does not exists in the wild”.  Perhaps it’s not
worth to support them.

>>
>>> +      /* -- */
>>> +      /* Property names and math operators */
>>> +      "[a-zA-Z0-9,._+?#-]+"
>>> +      "|[-+*/%&^|!~]"),
>>
>> There is a `%' operator here and in your tests, but it’s not mentioned
>> in the spec if I’m not mistaken.  Does it actually exists?
> 
> The compiler doesn't seem to complain when it's used. I can send a patch
> to update the spec for this rather esoteric feature. I can also include
> more tests and support for the boolean relational operators which also
> seem to be supported but probably never used.
> 

I’d like you to do this, yes.

To be fair, I don’t know what to think about this patch after Junio’s
message.

Re: [PATCH] userdiff: Add a builtin pattern for dts files

[Rob Herring]

On Sun, Jan 13, 2019 at 3:26 PM Alban Gruin <alban.gruin@gmail.com> wrote:
>
> Hi Stephen,
>
> thank you for your patch.  I left a few comments below.
>
> Le 11/01/2019 à 22:51, Stephen Boyd a écrit :
> > The Linux kernel receives many patches to the devicetree files each
> > release. The hunk header for those patches typically show nothing,
> > making it difficult to figure out what node is being modified without
> > applying the patch or opening the file and seeking to the context. Let's
> > add a builtin 'dts' pattern to git so that users can get better diff
> > output on dts files when they use the diff=dts driver.
> >
> > The regex has been constructed based on the spec at devicetree.org[1]
> >
> > [1] https://github.com/devicetree-org/devicetree-specification/releases/latest
> >
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> > ---
> >  Documentation/gitattributes.txt |  2 ++
> >  t/t4018-diff-funcname.sh        |  1 +
> >  t/t4018/dts-labels              |  8 +++++++
> >  t/t4018/dts-node-unitless       |  8 +++++++
> >  t/t4018/dts-nodes               |  8 +++++++
> >  t/t4018/dts-reference           |  8 +++++++
> >  t/t4034-diff-words.sh           |  1 +
> >  t/t4034/dts/expect              | 37 +++++++++++++++++++++++++++++++++
> >  t/t4034/dts/post                | 32 ++++++++++++++++++++++++++++
> >  t/t4034/dts/pre                 | 32 ++++++++++++++++++++++++++++
> >  userdiff.c                      |  9 ++++++++
> >  11 files changed, 146 insertions(+)
> >  create mode 100644 t/t4018/dts-labels
> >  create mode 100644 t/t4018/dts-node-unitless
> >  create mode 100644 t/t4018/dts-nodes
> >  create mode 100644 t/t4018/dts-reference
> >  create mode 100644 t/t4034/dts/expect
> >  create mode 100644 t/t4034/dts/post
> >  create mode 100644 t/t4034/dts/pre
> >
> > -%<-
> > diff --git a/userdiff.c b/userdiff.c
> > index 97007abe5b16..2bc964e11089 100644
> > --- a/userdiff.c
> > +++ b/userdiff.c
> > @@ -23,6 +23,15 @@ IPATTERN("ada",
> >        "[a-zA-Z][a-zA-Z0-9_]*"
> >        "|[-+]?[0-9][0-9#_.aAbBcCdDeEfF]*([eE][+-]?[0-9_]+)?"
> >        "|=>|\\.\\.|\\*\\*|:=|/=|>=|<=|<<|>>|<>"),
> > +PATTERNS("dts",
> > +      /* Node name (with optional label and unit address) */
> > +      "^[ \t]*((([a-zA-Z_][a-zA-Z0-9_]*: )?[a-zA-Z][a-zA-Z0-9,._+-]*(@[a-zA-Z0-9,._+-]+)?"
>
> From the spec, label and node names “shall be [between] 1 to 31
> characters in length”.  It’s not enforced here, and I guess it’s not
> really git’s job to check for this kind of rule.  Others may disagree
> with me, though.

The spec does say 31 characters, but that's never been enforced. So,
of course, there are occurrences in the wild (though maybe they were
just property names, not node names). In any case, we plan to change
the spec to increase the size. To what, I don't know.

Rob