davmac314 / dinit

Service monitoring / "init" system
Apache License 2.0
582 stars 45 forks source link

Add newline escaping support #345

Closed capezotte closed 2 months ago

capezotte commented 3 months ago

A few alternative options are possible (such appending the newline literally and handling them in read_setting_value , or translating it into a space) but this is likely what most people, coming from the shell, expect.

Closes: https://github.com/davmac314/dinit/issues/291

davmac314 commented 3 months ago

I understand what you're saying about people coming from the shell, but that "it works the same as the shell" is not an impression we want to give at all. Could you make it an error if first character of the next line is not whitespace, since that eliminates the otherwise ambiguous case? I.e.

setting1 = abc\
def   # error, new line following backslash does not begin with whitespace character
setting2 = abc\
 def   # ok, setting value is "abc def"

A case that isn't handled particularly well by this approach is comments. Eg:

setting1 = something # a comment\
 comment or setting value continues here maybe

I think the current patch sees it as one setting with "comment or setting value continues here" being considered an extension of the comment rather than of the value. I'd prefer it was either an error or an extension of the setting value, but I'm ok with letting this through as-is for now even though technically this is a breaking change - a comment with backslash at the end was previously just a single-line comment.

davmac314 commented 3 months ago

(Otherwise, this basically looks fine to me)

capezotte commented 2 months ago

Could you make it an error if first character of the next line is not whitespace, since that eliminates the otherwise ambiguous case?

Done.

I think the current patch sees it as one setting with "comment or setting value continues here" being considered an extension of the comment rather than of the value. I'd prefer it was either an error or an extension of the setting value

Now that you mentioned it, I believe it's better for it to be an extension of the setting value; in long command lines, some options (or chain-loading mini-utilities if the user goes that route) might be worth commenting on — the test file was updated accordingly.

Speaking of tests, I'd like to add tests for error conditions, but I'm not sure of the best approach; it seems each part of loadtests.cc is doing its own thing.

davmac314 commented 2 months ago

Looks good.

The "test_path_env_subst" test is probably a good template for new tests. To test for errors you can replace the exception handling there:

            catch (service_description_exc &exc) {
                //report_service_description_exc(exc);
            }

... with a version which sets a flag, that you can check at the end of the test, for example.

You can also add yourself to CONTRIBUTORS if you like.

capezotte commented 2 months ago

That should be it.

davmac314 commented 2 months ago

Merged, thanks!