Bug 311233 - Version 2.15 of bash syntax highlighting is incorrect
Summary: Version 2.15 of bash syntax highlighting is incorrect
Status: RESOLVED FIXED
Alias: None
Product: kate
Classification: Unclassified
Component: syntax (show other bugs)
Version: unspecified
Platform: unspecified Linux
: NOR minor (vote)
Target Milestone: ---
Assignee: KWrite Developers
URL:
Keywords:
Depends on: 140511
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-06 00:12 UTC by necrodust44
Modified: 2012-12-29 03:47 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In: 4.10


Attachments
This patch fixes the regression introduced in version 2.14: backquote and parenthesis on command arguments (9.74 KB, patch)
2012-12-08 13:17 UTC, Luiz Angelo De Luca
Details
This is the bash.xml after patch applied (41.58 KB, text/plain)
2012-12-08 13:19 UTC, Luiz Angelo De Luca
Details

Note You need to log in before you can comment on or make changes to this bug.
Description necrodust44 2012-12-06 00:12:32 UTC
Version 2.15 of bash syntax produce completely incorrect syntax highlighting. It should be removed and version 2.14 should be brought back to the repositories. 

But the primary question is who adopted these changes? Now I can take any syntax script, change it so that it will produce incorrect syntax highlighting, upload it to the repositories, and nobody will even care?

Reproducible: Always

Steps to Reproduce:
If you have GRUB installed, try to open /etc/grub.d/ scripts with bash syntax highlighting of versions 2.14 and 2.15.
Comment 1 Dominik Haumann 2012-12-06 09:39:41 UTC
Luiz, the patch you provided in bug #140511 seems to have regressions.
Can you have a look? Otherwise, we'll have to revert your changes for KDE 4.10 again.

@necrodust44:
> But the primary question is who adopted these changes? Now I can take any syntax script,
> change it so that it will produce incorrect syntax highlighting, upload it to the repositories,
> and nobody will even care?

We have > 200 highlightings, it is impossible to test all corner cases, especially since we cannot know /all/ the languages out there. So instead of flaming here, provide a patch to fix it. This helps much more. Thanks for you bug report, though, since this is the correct way to get things fixed :)
Comment 2 Luiz Angelo De Luca 2012-12-06 16:22:32 UTC
I'll take a look
Comment 3 Luiz Angelo De Luca 2012-12-07 22:04:28 UTC
In the last patch, in order to avoid highlighting special names as arguments like in:

echo do the job

I splitted the command processing into "FindCommand" and "CommandArgs". However, I missed the backquote case (that was reported) and the parenthesis case (still not reported). As I use a subcontext (CommandArgs), I must create CommandArgs context for each of the special situations (backquote and parenthesis), just like comment already does.

I would be much easier if kate could pass someway information to lower context, so I could use only one context for all three situations.

I'm finishing the parenthesis part and I'll post the patch soon
Comment 4 necrodust44 2012-12-08 01:52:36 UTC
I think that you should test you version more thoroughly before you add it as stable. And the backquotes and parentheses seem to be not the only cases. For example, I noticed that long arguments like --argument=value were treated as variables and the left half was being marked.
Comment 5 Luiz Angelo De Luca 2012-12-08 12:54:28 UTC
necrodust44,

Syntax highlight is heuristic. The current katepart does not allow or it is too much complicated to match every single detail of languages syntax. If you check my bugs about kate syntax highlight, so would see a bunch of cases where it fails. For example:

echo $((echo xxx) )

The last time I checked, even inside bash own parser, this case is a hack. You will only know when it is "$((xxx))" of "$( (xxx) )" after the final parenthesis is matched. However, at that time, "$((" and all its contents were already consumed.

Now back to the real problem. I improved the command parsing and now I detect not known commands (i.e.: not builtin and unixcommands) and I added a new color called OtherCommands. This way, CommandArgs can be selected correctly for all my test cases. I also pop the context, without consuming the text, when it finds a "}" or ")". This will fix the parenthesis problem.

For the backquote, it is a different beast. I need to know if backquote was already open and the found backquote closes it or if it opens a new one. The solution is to create a parallel context for each case, sharing most of its rules but differing when they find the backquote. This seems to do the trick, at least for your grub scripts.

Personally, I never ever use backquote. The syntax "$(cmd)" does just the same and it also allow to place it inside another one easily "$(cmd $(cmd2))".

The next time you face a syntax problem with bash, do not insult the contributors via personal emails. This does not helps. Generally it is a very bad idea to insult the one that you need help from. Remember that this is volunteer work. Also, try to isolate the problem, place more information in the bug report as a small script with the problem. You may be new to this "world" but this is how things work here.
Comment 6 Luiz Angelo De Luca 2012-12-08 13:17:52 UTC
Created attachment 75713 [details]
This patch fixes the regression introduced in version 2.14: backquote and parenthesis on command arguments

This patch fixes the regression when "`", "}", ")" closes when inside the command argument context. Also, it now highlight unknown command when it is at the command position.

The patch also cleans some xml ident.

Can you guys test it? I had no problem with the "--param=xxx" situation.
Comment 7 Luiz Angelo De Luca 2012-12-08 13:19:12 UTC
Created attachment 75714 [details]
This is the bash.xml after patch applied

In order to test, just place this file at:

~/.kde4/share/apps/katepart/syntax/bash.xml
Comment 8 Dominik Haumann 2012-12-09 13:55:14 UTC
@necrodust44: can you quickly test this, please? If there are no regressions to version 2.14, it's fine to commit.
Comment 9 Milian Wolff 2012-12-13 14:30:03 UTC
the file attached here fixes a serious regression for me (i.e.: foo="bla $(pwd) asdf" would never exit the string highlighting context).

can't see any other regressions from a quick glance at some of my scripts.
Comment 10 Dominik Haumann 2012-12-28 10:34:15 UTC
Git commit 5c905a445476032d816e4e1db070c3e52a523bda by Dominik Haumann.
Committed on 28/12/2012 at 12:33.
Pushed by dhaumann into branch 'master'.

fix regression of bash version 2.15

Thanks to Luiz Angelo De Luca for the fix!
Does this also fix 312121 and 312274 ?
Related: bug 312121, bug 312274
FIXED-IN: 4.10

M  +62   -29   part/syntax/data/bash.xml

http://commits.kde.org/kate/5c905a445476032d816e4e1db070c3e52a523bda
Comment 11 Dominik Haumann 2012-12-28 10:34:57 UTC
Git commit b9947e0454dbb3b9d11be5a6ffb8331569d8df31 by Dominik Haumann.
Committed on 28/12/2012 at 12:33.
Pushed by dhaumann into branch 'KDE/4.10'.

fix regression of bash version 2.15

Thanks to Luiz Angelo De Luca for the fix!
Does this also fix 312121 and 312274 ?
Related: bug 312121, bug 312274
FIXED-IN: 4.10

M  +62   -29   part/syntax/data/bash.xml

http://commits.kde.org/kate/b9947e0454dbb3b9d11be5a6ffb8331569d8df31
Comment 12 Luiz Angelo De Luca 2012-12-29 03:47:54 UTC
Thanks Dominik