Re: Review Request 59686: GEODE-2983: correctly handling --J option value that has ", " inside.

[Emily Yeh]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59686/#review176783
---


Ship it!




Ship It!

- Emily Yeh


On May 31, 2017, 10:44 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59686/
> ---
> 
> (Updated May 31, 2017, 10:44 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
> Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-2983: correctly handling --J option value that has "," inside.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java
>  288ea054ae1230c480d141c0159d6ccf9c299a7d 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java
>  74acfd6e03613ac4d0c62fcdd4ea859d1c74d2f2 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserParsingTest.java
>  4467792f60a2a3bf7cc53cf35e997e7462882539 
> 
> 
> Diff: https://reviews.apache.org/r/59686/diff/2/
> 
> 
> Testing
> ---
> 
> precheckin running
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 59686: GEODE-2983: correctly handling --J option value that has ", " inside.

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59686/#review176544
---


Ship it!




Ship It!

- Jared Stewart


On May 31, 2017, 10:44 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59686/
> ---
> 
> (Updated May 31, 2017, 10:44 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
> Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-2983: correctly handling --J option value that has "," inside.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java
>  288ea054ae1230c480d141c0159d6ccf9c299a7d 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java
>  74acfd6e03613ac4d0c62fcdd4ea859d1c74d2f2 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserParsingTest.java
>  4467792f60a2a3bf7cc53cf35e997e7462882539 
> 
> 
> Diff: https://reviews.apache.org/r/59686/diff/2/
> 
> 
> Testing
> ---
> 
> precheckin running
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 59686: GEODE-2983: correctly handling --J option value that has ", " inside.

[Jinmei Liao]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59686/
---

(Updated May 31, 2017, 10:44 p.m.)


Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Repository: geode


Description
---

GEODE-2983: correctly handling --J option value that has "," inside.


Diffs (updated)
-

  
geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java
 288ea054ae1230c480d141c0159d6ccf9c299a7d 
  
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java
 74acfd6e03613ac4d0c62fcdd4ea859d1c74d2f2 
  
geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserParsingTest.java
 4467792f60a2a3bf7cc53cf35e997e7462882539 


Diff: https://reviews.apache.org/r/59686/diff/2/

Changes: https://reviews.apache.org/r/59686/diff/1-2/


Testing
---

precheckin running


Thanks,

Jinmei Liao

Re: Review Request 59686: GEODE-2983: correctly handling --J option value that has ", " inside.

[Ken Howe]

> On May 31, 2017, 10:18 p.m., Jared Stewart wrote:
> > geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java
> > Lines 48 (patched)
> > 
> >
> > I worry that a user may at some point specify a `--J` option that 
> > includes `,,`.  I think our code could be more robust by using a delimeter 
> > that a user can't type on a standard keyboard.  The ASCII "Unit separator" 
> > character (decimal code 31, hex 0x1f) seems like a natural candidate.  That 
> > would make this look like: 
> > 
> > ```
> >   private static final char ASCII_UNIT_SEPARATOR = '\u001F';
> >   public static final String JARGUMENT_SPLIREGX = "" + 
> > ASCII_UNIT_SEPARATOR;
> > ```
> > 
> > 
> > I also think that `J_ARGUMENT_DELIMITER` might be a more informative 
> > name.

+1


- Ken


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59686/#review176535
---


On May 31, 2017, 5:42 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59686/
> ---
> 
> (Updated May 31, 2017, 5:42 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
> Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-2983: correctly handling --J option value that has "," inside.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java
>  288ea054ae1230c480d141c0159d6ccf9c299a7d 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java
>  74acfd6e03613ac4d0c62fcdd4ea859d1c74d2f2 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserParsingTest.java
>  4467792f60a2a3bf7cc53cf35e997e7462882539 
> 
> 
> Diff: https://reviews.apache.org/r/59686/diff/1/
> 
> 
> Testing
> ---
> 
> precheckin running
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 59686: GEODE-2983: correctly handling --J option value that has ", " inside.

[Jared Stewart]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59686/#review176535
---




geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java
Lines 48 (patched)


I worry that a user may at some point specify a `--J` option that includes 
`,,`.  I think our code could be more robust by using a delimeter that a user 
can't type on a standard keyboard.  The ASCII "Unit separator" character 
(decimal code 31, hex 0x1f) seems like a natural candidate.  That would make 
this look like: 

```
  private static final char ASCII_UNIT_SEPARATOR = '\u001F';
  public static final String JARGUMENT_SPLIREGX = "" + ASCII_UNIT_SEPARATOR;
```

I also think that `J_ARGUMENT_DELIMITER` might be a more informative name.



geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java
Lines 233 (patched)


What do you think about adding a field to `GfshParser` that can be 
referenced by all of these `optionContexts`?  Basically:

```

class GfshParser { 
... 
J_ARGUMENT_OPTION_CONTEXT = "splittingRegex=" + JARGUMENT_SPLIREGX;
}
```

```
 @CliOption(key = CliStrings.START_LOCATOR__J,
  optionContext = GfshParser.J_ARGUMENT_OPTION_CONTEXT,
  help = CliStrings.START_LOCATOR__J__HELP) final String[] 
jvmArgsOpts,
```


- Jared Stewart


On May 31, 2017, 5:42 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59686/
> ---
> 
> (Updated May 31, 2017, 5:42 p.m.)
> 
> 
> Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and 
> Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-2983: correctly handling --J option value that has "," inside.
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/GfshParser.java
>  288ea054ae1230c480d141c0159d6ccf9c299a7d 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java
>  74acfd6e03613ac4d0c62fcdd4ea859d1c74d2f2 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserParsingTest.java
>  4467792f60a2a3bf7cc53cf35e997e7462882539 
> 
> 
> Diff: https://reviews.apache.org/r/59686/diff/1/
> 
> 
> Testing
> ---
> 
> precheckin running
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>