use a shell parser to format commands

[mvdan]

At the moment, the code simply treats shell code as a string, splitting by && and doing string replaces.

This will work most of the time, but will break fairly easily, with cases like:

• bash -c 'foo && bar' - we don't want this to be split
• apk update && apk add x - won't be replaced with apk add --no-cache due to the double space (markdown gets rid of the double space before &&, though)

I use this tool and suffer from these from time to time, so I'd be happy to work on a small patch to use mvdan.cc/sh/syntax. Using its printer you'd also format the shell code, such as replacing `foo` with $(foo).

I see no tests, though - so I'm a bit wary to touch the code. Chances are I'm going to introduce a couple of regressions :)

[jessfraz]

please do :)

[jeffxf]

Instead of just ignoring &&'s that are inside quotes, I started implementing mvdan/sh/syntax last night for the full shell parser. I got it working but then had some concerns:

• Just because something is quoted doesn't mean it's going to be shelled out (could try to address this by looking for 'bash', 'sh', etc. before the quotes but that could be a mess)
• Should we provide a flag to enable the shell parser?
• Should we provide flag(s) for shell parser options or just insist on formatting a certain way?

I'm leaning towards a shell parser being out of scope of what dockfmt is for, but I may be wrong and don't have a problem solving those ^ issues if you disagree and would like to have it.

Just let me know if you'd like to have the shell parser and if I should include anything to address those ^ . On the other hand, if you just want to stick with ignoring quoted &&'s, let me know if you have an issue with the regex solution; I can handle criticism :)

[mvdan]

I personally think that using regular expressions is not a great solution for this - it's just a slightly smarter version of the old code :) Reminds me of https://stackoverflow.com/questions/1732348/regex-match-open-tags-except-xhtml-self-contained-tags

To name a few possible edge cases that aren't handled by the regex:

• echo $'&&'
• echo ${x//&&/||} # replace all '&&' with '||'
• [[ x && y ]] # this is a bash test AND, not a command AND

You also don't need to enforce the formatting that the sh/syntax package produces - you could simply use its parser to find the location of the command && operators, and then manually split the lines like the existing code does.

Just because something is quoted doesn't mean it's going to be shelled out

I'm not sure I understand - my point is that anything that is quoted should never be split in multiple lines. For example, this can result in the string having extra whitespace due to the indentation. And as you say, one can never know if the string is really a shell program.

Ultimately this won't matter for most people as this issue is about edge cases, but I tend to prefer correctness when it is possible :)

[jeffxf]

@mvdan I agree that regex is rarely the perfect answer and needs to be used cautiously. The first example you gave parses correctly for me with the regex though, but you're right about the last two since quotes aren't involved. Using the parser by itself might be a good idea then. I got carried away implementing full command parsing which is what I started to question being in scope. I'll try to take another look at your syntax parser sometime this week.

[tianon]

I've just played with @mvdan's shell parser / shfmt in the context of moby/moby#38055, and I can say it's definitely got me impressed. I was stylistically happier with the output after turning on several of the optional configuration knobs (-bn -ci -sr), but the basics were definitely all correct (including weird features like <<-EOF getting indented properly, which isn't relevant for Dockerfile syntax but is still useful to gauge how well @mvdan understands the space of "shell code parsing" 👍 ❤️).

[jeffxf]

@tianon Oh without a doubt. My hacked up PR a few months ago wasn't meant to discredit @mvdan's work. I agree that his syntax parser needs to be used here. I dropped the ball on "taking a look this week" since its well... been much longer than that. I started a new job the week after that and got way too busy. I won't specify a time frame this time but it's definitely something I hope to look into again soon.