Code review comment for lp:~luoyonggang/subvertpy/unittest-win32

2012/3/12 Jelmer Vernooij <email address hidden>

> Am 12/03/12 02:10, schrieb Yonggang Luo:
>
> 2012/3/12 Yonggang Luo<email address hidden>
>>
>> 2012/3/12 Jelmer Vernooij<email address hidden>
>>>
>>> 2012/3/11 Jelmer Vernooij<email address hidden>
>>>>>
>>>>> Please read the test result carefully. And you will know why do the
>>>>>>>
>>>>>> things in
>>>>>>
>>>>>>> this way.
>>>>>>> The test result is under
>>>>>>>
>>>>>>> Description of the Change
>>>>>>>
>>>>>> That only explains *what* is failing for you. It doesn't explain
>>>>>>
>>>>> *why*
>>>
>>>> it
>>>>
>>>>> is failing, and why your fix addresses that. That's the bit I'm
>>>>>>
>>>>> interested
>>>>
>>>>> in.
>>>>>>
>>>>> It's failed just because Python's maketemp didn't return the corrent
>>>>> tempfile file path.
>>>>> For example, tempfile.mkdtemp() on windows will gives us with the file
>>>>>
>>>> path
>>>>
>>>>> c:\users\uername\appdata\**local\temp\hash_code , but the real file
>>>>> path
>>>>> is C:\Users\username\AppData\**Local\Temp\hash_code.
>>>>> When I use
>>>>> tempfile.mkdtemp(dir =
>>>>> os.getenv("TEMP", None) or os.getenv("TMP", None))
>>>>> it's will give us the exact file path with the windows file system.
>>>>> TEMP and TMP is recorded by the OS. and even if TEMP and TMP is not
>>>>>
>>>> exist,
>>>>
>>>>> it's sill works, just because dir accept None as
>>>>> a default input.
>>>>>
>>>> TEMP and TMP can be set by the user too as far as I know. What if the
>>>>
>>> user
>>>
>>>> has set them to "C:\users\USeRName\AppData\**Local\Temp\hash_code" ?
>>>>
>>> That's a
>>>
>>>> perfectly valid path, as path lookups are case insensitive.
>>>>
>>>> If TEMP and TMP do not exist, we get the same behaviour as currently so
>>>> users who have that will still hit that bg - and that means this fix is
>>>> incomplete in that situation.
>>>>
>>>> The unittest failed, is just because Subversion changed the file path
>>>>> from c:\users\uername\appdata\**local\temp\hash_code
>>>>> to C:\users\uername\appdata\**local\temp\hash_code, but under Windows
>>>>>
>>>> file
>>>
>>>> system, these two path represents the same path,
>>>>> so it's should not be FAILED.
>>>>>
>>>> Then why not simply call os.path.normcase() on both paths before
>>>>
>>> comparing
>>>
>>>> them? That seems like a much cleaner solution to me.
>>>>
>>>> That's why I ask you better solution, thanks.
>>>
>>> It's not works, still return the path with incorrect case.
>>
> Can you please elaborate? What doesn't work ? Does os.path.normcase not
> return a normalized case?
>
It's didn't change the result, just act with os.path.normcase 's documented
case.
The path is return without change except \ are replaced with /.

>
> Cheers,
>
> Jelmer
>

--
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo