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

[Alban Gruin <alban.gruin@gmail.com>]

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