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

[Lukasz Dracz]

Hello,

> > 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
> 
> I think you can do a test with:
> 
> +        String[] args = {
> +                "set", "key", "value1 value2"
> 
> to simulate.

Ah okay, thanks ! Added two tests one with space between values and one with space between keys.
Patch is attached.

> 
> Rest of the patch seems good to me :)
> 

Thank you,
Lukasz Dracz
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fixSetCommandDuplicateTests-2.patch
Type: text/x-patch
Size: 10616 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20150313/57fd3814/fixSetCommandDuplicateTests-2.patch>