[rfc][icedtea-web] fix itweb-settings set command to allow duplicate strings

[Lukasz Dracz]

Hello,


> > Hello,
> > 
> > > > Hello,
> > > > 
> > > > This patch fixes the issue where the itweb-settings Set would not allow
> > > > the
> > > > key and value to be the same value.
> > > > Such as set blah blah. This was due to the isKey() method using
> > > > .indexOf()
> > > > which would always return the index of the first "blah" registering two
> > > > keys
> > > > with no values. The fix is to just alternate between the two parts of
> > > > the
> > > > if
> > > > statement. It does this by using null for key and checking when it is
> > > > null,
> > > > to assign a key. When a value is assigned and written then the key is
> > > > assigned back to null.
> > > 
> > > Hello,
> > > 
> > > Thanks for the fix :)
> > > 
> > > Nits below:
> > > 
> > > -            if (isKey(arg)) {
> > > +            if (key == null) {
> > > 
> > > I think you should still make sure that the argument is actually a key
> > > before
> > > assigning it. So something like:
> > > 
> > > if (key == null && isKey(arg)) { [...] }
> > 
> > Hmm, I don't agree with this.
> > 
> > Seems redundant to me and I think it would introduce a new bug at least
> > using
> > the current isKey() implementation.
> > 
> > The current isKey would look at the args index in order to find out whether
> > it was even or odd in the order to help
> > decide whether it was a key or value. The current for loop has this order
> > from args and so when it runs all we need to do
> > is alternate the first part of the if and the else part which the current
> > patch I sent does with the null.
> > 
> > The bug that I believe would result in the case for example:
> > 
> > set blah blue blue red
> > 
> > 1. blah: key is null and index is 0 so enters Key Assigning Part
> > 2. blue: key is blah and index is 1 of blue so enters Value Assigning Part
> > 3. blue: key is null and index found is 1 of blue so enters Value Assigning
> > Part with key null and value blue
> > 
> > Now I suppose I can rewrite isKey to keep track of the last index which I
> > considered in the first place,
> > but since the for loop goes over the terms in the order we want (as isKey
> > and
> > args get the params in the same way)
> > All that is needed is to alternate between the first if and the else part.
> > For me it seems redundant,
> > but perhaps I am looking at this wrong.
> 
> 
> Okay. I didn't really look at what isKey() does. I just assumed that only
> certain inputs were accepted as 'keys' and isKey() would check that. But it
> looks like you can put anything as a key so that suggestion doesn't make
> sense.
> 
> 
> 
> Patch looks fine to me. My one concern that you can optionally address is:
> It's not very obvious the way it's coded now that you're iterating over the
> args in key value pairs. Can you maybe restructure it or add comments to
> make it more clear?

Yeah, I have rewritten it with a boolean, hopefully this is much clearer.

> Also, what happens on input like:
> 
> 'set a b c'
> 
> Does 'c' get ignored?

The OptionParser throws an UnevenParameterException (Added test)

> How about 'set a b\ c'? Ie. can users add spaces to their key/value? I think
> these would be some good test cases.

Hmm, I am having issues writing a test in this case, since adding "\ " is an illegal escape
character in java it seems, and adding a space in the String[] of args, I can't seem to simulate it
"a", "b ", "c" throws UnevenParameterException and "a", "b", " c", "d" results in "a=b" and " c=d" in 
the deployment properties files.

>From testing of itweb-settings it becomes 'a=b c'.

I have attached a patch that adds unit tests for the set command in CommandLine.

Thank you,
Lukasz Dracz


-------------- next part --------------
A non-text attachment was scrubbed...
Name: fixSetCommandDuplicateTests.patch
Type: text/x-patch
Size: 8976 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20150313/dd99d64a/fixSetCommandDuplicateTests.patch>