Curly braces cannot be given in a child process jvm arg if you also need to supply an escaped comma

[Vampire]

Besides that the escaping of commas for child process jvm args using curly braces is not documented anywhere but in the source code, there is one case that currently does not work.

If you for example need to supply -DsomeValue=,{, this is currently not possible due to the current comma-handling code.
If you supply it like that, you end up with the two arguments -DsomeValue=, and {.
If you supply it like -DsomeValue={,}{, you end up with the one argument -DsomeValue=,, because if one escaped comma was found, all curly braces are stripped away.

Curly braces should only be stripped away if they actually did escape a comma and other curly braces should be preserved, also if they are supplied as pair like -DsomeValue={,}{} or -DsomeValue={,}{foo} which should be interpreted as -DsomeValue=,{} and -DsomeValue=,{foo}.

The only questionable case is, what happens if you have curly braces within curly braces like -DsomeValue={{,}} or {-DsomeValue={,}}.
I'm not sure whether it would be better to interpret this as

• -DsomeValue=, and -DsomeValue=,, or
• -DsomeValue={,} and {-DsomeValue=,}, or maybe
• -DsomeValue={,} in both cases

[szpak]

Related: jopt-simple/jopt-simple#147

[Vampire]

Not sure that is really related, what I mentioned is PIT-own code here: https://github.com/hcoles/pitest/blob/master/pitest-command-line/src/main/java/org/pitest/mutationtest/commandline/CommaAwareArgsProcessor.java

[rossdrew]

Wrote & submitted a fix and some test cases. Please confirm the test is actually testing what you stated @Vampire? That is

  // Issue #1441: braces that did not escape a comma should be preserved
  {  Arrays.asList("--jvmArgs", "-DsomeValue={,}{"), Arrays.asList("-DsomeValue=,{") },
  {  Arrays.asList("--jvmArgs", "-DsomeValue={,}{}"), Arrays.asList("-DsomeValue=,{}") },
  {  Arrays.asList("--jvmArgs", "-DsomeValue={,}{foo}"), Arrays.asList("-DsomeValue=,{foo}") },

or

Input	Expected Output
DsomeValue={,}{	-DsomeValue=,{")
-DsomeValue={,}{}	-DsomeValue=,{}
-DsomeValue={,}{foo}	-DsomeValue=,{foo}

Of course a decision also needs to be made if this is the way forward by @hcoles.

[Vampire]

The text in the table says "after escaped comma", so just to make sure, same if in front and same if text in-between, not that some AI did some very focused change right after the escaped comma.

Also the questionable cases mentioned should be tested after it was decided how they should be handled, and probably the whole escaping then also documented.

[rossdrew]

I've removed the text. I generated the table using AI from my test examples so that I didn't need to format a markdown table. Maybe didn't curate the text well enough...or at all. My bad.

Are you comfortable with how my approach deals with the suggested test cases? Should I add some more? perhaps (disclaimer, I used AI to generate some more test cases):

Input	Expected Output
-DsomeValue={{,}	-DsomeValue={,
-DsomeValue={}{,}	-DsomeValue={},
-DsomeValue={foo}{,}	-DsomeValue={foo},
-DsomeValue={,}x{	-DsomeValue=,x{
-DsomeValue={,}x{}	-DsomeValue=,x{}
-DsomeValue={,}x{foo}	-DsomeValue=,x{foo}

I guess I'm asking if it's worth iterating on this if the test cases are wrong or incomplete or when a call is made one way or the other on the questionable case. i.e.

Input	Expected Option 1	Expected Option 2	Expected Option 3
-DsomeValue={{,}}	-DsomeValue=,	-DsomeValue={,}
{-DsomeValue={,}}	-DsomeValue=,,	{-DsomeValue=,},	-DsomeValue={,}

At which point I think documentation comes in.

[Vampire]

Are you comfortable with how my approach deals with the suggested test cases?

Well, that is what I wrote as expected, isn't it, so yes, of course.

Should I add some more? perhaps

Sure why not.
But ultimately up to the maintainer.

I guess I'm asking if it's worth iterating on this if the test cases are wrong or incomplete or when a call is made one way or the other on the questionable case. i.e.

They are valid user inputs, so there should be a defined behaviour that does not change with code changes, so there should be tests verifying the behavior.

At which point I think documentation comes in.

Documentation should definitely be added too as I said, as currently you can only find out from source code that this is even possible.
But nevertheless tests should verify the expected behavior, whatever it is.