Better design about command formatting

[JakkuSakura]

I'm working on a few PRs, and found current design of command formatting a bit hard to grasp

Take this for example

if min_size is not None:
    args.append("-size +{0}c".format(parse_size(min_size)))
if max_size is not None:
    args.append("-size -{0}c".format(parse_size(max_size)))
if size is not None:
    args.append("-size {0}c".format(parse_size(size)))
if maxdepth is not None:
    args.append("-maxdepth {0}".format(maxdepth))

def maybe_quote(value):
    return shlex.quote(value) if quote_path else value

if fname is not None:
    args.append("-name {0}".format(maybe_quote(fname)))
if iname is not None:
    args.append("-iname {0}".format(maybe_quote(iname)))
if regex is not None:
    args.append("-regex {0}".format(maybe_quote(regex)))

return make_formatted_string_command(
    "find {path} -type {type_flag} {args} || true",
    path=maybe_quote(path),
    type_flag=self.type_flag,
    args=" ".join(args),
)

Before, path was written as QuoteString, and it's a bit hard to compose args(and understand)
When I track down, QuoteString was handled as a special case in StringCommand
make_formatted_string_command makes the command more complicated. it splits the command then in QuoteString it will be composed again. I don't get this design.

Until now, with direct use of shlex.quote(wrapped directly in QuoteString.str), formatting becomes more controllable.

I'd prefer using vanilla format and QuoteString so it becomes

'command {}'.format(QuoteString(arg))
args = ['command', 'path', QuoteString(path)]
' '.join(args)

I'm I losing something?

[JakkuSakura]

If you approve this approach, I'd like to write a PR

[jankatins]

No comments on the approach, but PLEASE make it ensure_quoted or quote_if_needed or whatever... maybe is really hard to reason when just reading the calling code without knowing what the functions really does under the hood.

[JakkuSakura]

As simple as

def ensure_quoted(s):
    return shlex.quote(s)

I used maybe because the original code uses a variable to control the escaping

[JakkuSakura]

I kinda understand the design of make_formatted_string_command. Only when the path is associated with MaskString, it makes sense to use make_formatted_string_command. All other cases, format and ensure_quoted are easier to understand and faster.

Cases like below can be an overuse of it. below case is doable with just python's format and ensure_quoted. It took me quite a while to figure out make_formatted_string_command is not necessary

class FindFilesBase(FactBase):
    abstract = True
    default = list
    type_flag: str

    def process(self, output):
        return output

    def command(self, path, quote_path=True):
        return make_formatted_string_command(
            "find {0} -type {type_flag} || true",
            QuoteString(path) if quote_path else path,
            type_flag=self.type_flag,
        )

[JakkuSakura]

Maybe the more appreciate use of StringCommand is to pass args, instead of relying on make_formatted_string_command. this way ensure_quoted is no longer required

[Fizzadar]

This seems much nicer, IIRC make_formatted_string_command is basically a hack to support more complicated shell scripts. I think the best approach would be just to migrate everything to using StringCommand(...) directly which is far more explicit.

[JakkuSakura]

I did some experiments with the find files PR. it's suitable to just use StringCommand for complex command with lots of variables. However, some other commands have "not too few" and "not too many" spaces between, making the migration tricky. I'd prefer not to rewrite them

make_formatted_string_command(
    (
        "(( sha1sum {0} 2> /dev/null || shasum {0} || sha1 {0} ) | grep {1} ) "
        "|| ( echo {2} && exit 1 )"
    ),
    QuoteString(dest),
    sha1sum,
    QuoteString("SHA1 did not match!"),
)