Hello!

On Fri, Mar 03, 2017 at 11:37:36PM +0800, OOO wrote:

> I made a sample and take screenshot[1].
> This one (remove current ngxBlock) is a blocker for some other changes.
>
> [1]:https://www.flickr.com/photos/othree/32843758310/

Ah, ok, I see. The problem only happens if there is an
indentation.

> And about the *ngxBlock* you talked about in the last paragraph.
> I think you are talking about curly bracket block:
>
> http {
> include conf/mime.types;
> index index.html index.htm index.php;
> }
>
> I think it's not necessary for nginx conf file syntax highlight definition.
> I can do more specific syntax highlight like ngxServerBlock for
> `server { ... }` or another for `http { ... }`.
> And made some constrain, ex: only allow *http* in *server*. Not allow
> *http* at root level.
> But it add lots of complexity. And I don't see the benefit worthy it.
> Also breaks highlight in partial conf file (which will be included by
> other conf file).

Not allowing "server" at root level will break highlighting in
include files, agree. But it should be still possible to check if
there are missing / unbalanced curly brackets, and warn a user if
tries to write "server { ..." instead of "server { ... }". And it
should be still possible to enforce http{} should be only at top level,
and there should be no server{} insider another server{}.

Moreover, I think it is the only way to properly parse
configuration for highliting. Without matching blocks you want be
able to properly highligh configuration directives. For example,
consider the following configuration:

map $foo $bar {
index foo;
}

index bar;

In this example, only "map" and "index bar" should be highlighted
as configuration directives. Highliting "index foo" would be an
error, because it is not a configuration directive, but a source
and resulting values of the map.

The same applies to matching directives. Not seeing a directive
ending will lead to

server_name some-name-but-no-semicolon
listen 80;

being misinterpreted as two configuration directives, making it
harder for people to find the bug. And this is actually a typical
problem nginx users often face, as per questions on the mailing
list. And it would be really good for syntax highlighting to help
them to find the bug instead of misleading them even further.

> I will fix the rewrite commit.
> But I have another question.
> Maybe you have an idea about it.
>
> rewrite "/foo bar/" /bazz/ last;
>
> For now, if I fix the bug. The "/foo bar/" part will be highlight as string.
> But the /bazz/ part will be normal (no highlight).
> I think they should both be highlight as string.
> Do you have any idea?

Current behaviour is to hightlight strings everywhere
unconditionally, and I don't think it's a problem. Your changes
won't introduce any regression here.

Moreover, this is something anyway will happen even if you
specifically define matching for a particular parameter. For
example, consider the following configuration snippet:

"rewrite" "/foo bar/" "/bazz/" "last";

It is identical to the snippet above from nginx point of view, but
all the arguments and the directive name will be highlited as
strings. I don't really think this is something would be easy to
resolve.

BTW, just noticed while testing the above example. ngxString now
doesn't match at the start of a line, so the following snippet
won't be correctly highlighted:

rewrite
"/foo/"
/bar
last;

I think the fix would be to explicitly check for whitespaces or
newline before ngxString, like this:

diff --git a/contrib/vim/syntax/nginx.vim b/contrib/vim/syntax/nginx.vim
--- a/contrib/vim/syntax/nginx.vim
+++ b/contrib/vim/syntax/nginx.vim
@@ -13,7 +13,7 @@ syn match ngxVariable '\$\(\w\+\|{\w\+}\
syn match ngxVariableBlock '\$\(\w\+\|{\w\+}\)' contained
syn match ngxVariableString '\$\(\w\+\|{\w\+}\)' contained
syn region ngxBlock start=+^+ end=+{+ skip=+\${+ contains=ngxComment,ngxDirectiveBlock,ngxVariableBlock,ngxString oneline
-syn region ngxString start=+[^:a-zA-Z>!\\@]\z(["']\)+lc=1 end=+\z1+ skip=+\\\\\|\\\z1+ contains=ngxVariableString
+syn region ngxString start=+\(^\|[ \t]\)\zs\z(["']\)+ end=+\z1+ skip=+\\\\\|\\\z1+ contains=ngxVariableString
syn match ngxComment ' *#.*$'

syn keyword ngxBoolean on

Please let me know if you have any objections to the above patch
and/or just include it into your patch series.

--
Maxim Dounin
http://nginx.org/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel