Review request for 7034570 - java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited

[Ulf Zibis]

Am 15.04.2011 11:45, schrieb Michael McMahon:
> Ulf,
>
> On "SystemRoot" vs "systemroot", I had thought that Windows required the sort
> to be case-sensitive. But, I double-checked and it doesn't. So we can use "SystemRoot".

If that would be true, we wouldn't need to sort by CASE_INSENSITIVE_ORDER or rather NameComparator. ;-)

Again my question: What you think of my below code?
I think we only need 1 call site to insert the potentially missing "SystemRoot" variable, a simple 
array is faster than initializing a needless j.u.List, and boolean foundSysRoot is redundant to int cmp.
Additionally I think having the addEnv method is too disproportionate. We can just inline 2 
different String-catenations, and it should compile to 3/4 sb.append() under the hood.

So here my shortest version:

     // Only for use by ProcessImpl.start()
     String toEnvironmentBlock() {
         // Sort Unicode-case-insensitively by name
         Map.Entry<String,String>[] list = entrySet().toArray(new Map.Entry<>[size()]);
         Arrays.sort(list, entryComparator);

         StringBuilder sb = new StringBuilder(list.length*30);
         for (int i = 0, cmp = -1; ; i++) {
             Map.Entry<String,String> e = i < list.length ? list[i] : null;
             // Some versions of MSVCRT.DLL require SystemRoot to be set:
             final String ROOT = "SystemRoot";
             if (cmp < 0 && (e == null || (cmp = nameComparator.compare(e.getKey(), ROOT)) > 0)) {
                 String value = getenv(ROOT);
                 if (value != null)
                     sb.append(ROOT+'='+value+'\u0000');
                 // Ensure double NUL termination, even if environment is empty.
                 else if (list.length == 0)
                     sb.append('\u0000');
             }
             if (e == null)  break;
             sb.append(e.getKey()+'='+e.getValue()+'\u0000');
         }
         // Ensure double NUL termination
         return sb.append('\u0000').toString();
     }


-Ulf


>
> Ulf Zibis wrote:
>> You can have the code much shorter (and faster):
>>
>>     // Only for use by ProcessImpl.start()
>>     String toEnvironmentBlock() {
>>         // Sort Unicode-case-insensitively by name
>>         Map.Entry<String,String>[] list = entrySet().toArray(new Map.Entry<>[size()]);
>>         Arrays.sort(list, entryComparator);
>>
>>         StringBuilder sb = new StringBuilder(list.length*30);
>>         for (int i = 0, cmp = -1; ; i++) {
>>             Map.Entry<String,String> e = i < list.length ? list[i] : null;
>>             final String ROOT = "SystemRoot";
>>             if (cmp < 0 && (e == null || (cmp = nameComparator.compare(e.getKey(), ROOT)) > 0)) {
>>                 String value = getenv(ROOT);
>>                 if (value != null)
>>                     addEnv(sb, ROOT, value);
>>                 // Ensure double NUL termination, even if environment is empty.
>>                 else if (list.length == 0)
>>                     sb.append('\u0000');
>>             }
>>             if (e == null)  break;
>>             addEnv(sb, e.getKey(), e.getValue());
>>         }
>>         // Ensure double NUL termination
>>         return sb.append('\u0000').toString();
>>     }
>>
>>     // add the environment variable to the child
>>     private static void addEnv(StringBuilder sb, String key, String value) {
>>         sb.append(key+'='+value+'\u0000'); // under the hood, it compiles to 4 sb.append()
>>     }
>>
>> Do you like it?
>>
>> Note: I think, if we use NameComparator, we can use "SystemRoot".
>>
>> -Ulf