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

[Jie Kang]

----- Original Message -----
> 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.

Hello,

Looks fine to me;


Regards,

> 
> > 
> > Rest of the patch seems good to me :)
> > 
> 
> Thank you,
> Lukasz Dracz

-- 

Jie Kang

OpenJDK Team - Software Engineering Intern