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

[Lukasz Dracz]

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.

 
> 
> Also unit tests for this stuff would be great :D

Yeah unit tests are a good idea and I'm working on it.

Thank you,
Lukasz Dracz
 
> Regards,
> 
> > 
> > Thank you,
> > Lukasz Dracz
> 
> --
> 
> Jie Kang
> 
> OpenJDK Team - Software Engineering Intern
>