anordal / shellharden

The corrective bash syntax highlighter
Mozilla Public License 2.0
4.62k stars 129 forks source link

Just cats the script #34

Closed forthrin closed 4 years ago

forthrin commented 4 years ago

Running shellharden erratic-script on macOS (via Homebrew) simply outputs the script as-is. No syntax highlighting or no suggestions. For comparison, shellcheck erratic-script outputs a bunch of errors. What could be wrong?

anordal commented 4 years ago

No syntax highlighting could only mean that your terminal doesn't support 24-bit colors. I haven't bothered to add a fallback.

Try this to test 24-bit colors in your terminal:

printf '\e[48;2;0;0;255m\e[38;2;255;255;0myellow on blue background\e[m\n'

A great resource on 24-bit color termnials: https://gist.github.com/XVilka/8346728

forthrin commented 4 years ago
  1. Just using the standard macOS Terminal. So then I can't use this utility, then?

  2. Is the report purely color based? There is no textual change? I tried --transform and --syntax, then removed the ANSI codes and did a diff, but the output file is identical to the input file (which has errors found by shellcheck).

  3. And why 24 bit color? "256 colors should be enough for anyone." Maybe even 16 in this case.

anordal commented 4 years ago

The report uses background colors as a character level diff. If they don't show, you can still use --transform (and compare input and output), --replace (and compare before and after) or --check (if you just want the exit status).

For a little demo, try this ('' means stdin):

echo '${a}' | shellharden --transform ''

Here, you can see that shellharden would change ${a} to "$a". If it changes nothing to your script, then congratulations, your script is at least that decent. Shellharden only fixes some things that ShellCheck complains about, and ShellCheck only complains about some things that Shellharden fixes.

forthrin commented 4 years ago

Running with --transform did the trick. Don't know why it didn't work before.

When I read "something to say yes with" in the Readme, I thought this was an interactive tool asking Yes/No for each find. Evidently not. So how are you supposed to handle the report? Edit the output with an ANSI-compliant editor or something?

For the time being, something like the following would be my approach to reviewing and applying changes:

shellharden --transform script | diff -U 9999 script - > script-FIXME

anordal commented 4 years ago

There is no interactive mode, because I don't find it super-useful to accept only some changes if you want to hand-edit the rest – you might as well hand-edit those first and then accept everything (--replace). Thus why the report is optimized for human consumption.

If you review as a diff, I hope you have a character-level diff viewer! If you have the typical myriad of small changes anyway. Having to eye-scan every changed line (which appears twice) for subtle changes adds very little compared to reading the script once you see the pattern. So I don't think diff is the ideal format, but I suppose you can have a pretty decent experience with a good diff viewer as well.

I should consider making at least the background colors more backward compatible. In the meantime, I would get a 24-bit terminal if I were you. You have loads of options, also on Mac.

I have a suggestion if you want the interactive experience: git add -p.

forthrin commented 4 years ago

Could you describe step-by-step how you actually use this tool, eg.

  1. Run such-and-such command line
  2. Open the result in such-and-such editor
  3. Cursor around looking for red colors
  4. Remove everything marked in red, with possible exceptions
anordal commented 4 years ago

Remember, it's just a suggestive syntax highlighter that can also transform or replace scripts. Forget editing the color coded output. Those color codes are for terminals, not editors.

What I do:

  1. Review terminal output of shellharden yourscript.bash
  2. If a change must be made differently than suggested, edit the necessary part of the script itself, and go back to step 1.
  3. When all looks fine, you can shellharden --replace yourscript.bash.

I hope it's more obvious when you see the output as it's meant to be. I have made a change to use 3-bit background colors, which is what the diff uses, so you should at least see that.

I should perhaps also add a note in the help text about the 24-bit colors for the syntax. So a 24-bit terminal is still recommended. If I may say: Even Windows has that now.

forthrin commented 4 years ago

Aha! It works more manually than gleaned from the README. You should consider updating the documentation to reflect this. My personal suggestion from the top of my head would actually be to do a replace straight away (provided that experience goes to show that the script hardly, if ever, makes mistakes), but put the changed line as a comment on the line above. This way you have a record of the previous content, and you can review and approve it simply by deleting the comments.

...
# cp $a $b
cp "$a" "$b"
...

PS! You never said why you went for a whooping 24-bit color display, which seems excessive and excluding.

anordal commented 4 years ago

I wouldn't say Shellharden ever makes mistakes, but it rather predictably disables word splitting, and will rather happily break any code that is using that deliberately. The deal is that such code must be rewritten manually. Thus why it comes with a manual.

As an example, see Use arrays FTW.