apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <wr...@rowe-clan.net>
Subject Re: svn commit: r422157 - /apr/apr/trunk/file_io/win32/filepath.c
Date Mon, 17 Jul 2006 17:30:09 GMT
Justin Erenkrantz wrote:
> On 7/17/06, William A. Rowe, Jr. <wrowe@rowe-clan.net> wrote:
>> Justin Erenkrantz wrote:
>> >
>> > The problem is that the APR code relies on the MSVC run-time being
>> > consistent: as we have demonstrated, it's not.  It can and does report
>> > c:\ in several circumstances.
>>
>> Yes, and so what?  This should be harmless... please indicate the bug
>> that the VETOED code supposedly corrects?
>>
>> (And Mr. Committer, revert your vetoed code already.)
> 
> Why the rush to revert?  We're trying to understand the codebase here
> to ensure we're making the right change rather than rushing to revert:
> the testcases indicate a clearly false assumption in the APR runtime -
> that drive letters are always upper-cased by the runtime.

Piddle with your experiments in a sandbox.  Vetoed code needs to go when
it's vetoed.  This veto is over the fact that you've CHANGED security
related behavior, and that won't become acceptable.

The fact that it hasn't been reverted shows really bad etiquite by the
committer.  Commit then review means just that; this is committed, the
fact that it breaks canonical comparisons was observed, now that it's
reviewed it needs to be reverted.

Then we can discuss what the correct fix is, in terms of solving the
test cases.

>> > Note that all APR was doing was
>> > toupper() which doesn't handle Unicode either.
>>
>> No need.  Drive LETTERS aren't full unicode, the drive letter is ascii.
> 
> Then why bring up Unicode at all?

Because if you compare the entire filepath string you need to canonicalize
the full utf-8 set to a known state.  The best known state is the true path.

>> > Again, these are the testnames tests that were failing.
>>
>> Cite them.
> 
> As Paul said, the testnames cases fail.  I believe the specific test
> case is  root_from_cwd_and_back().  apr_filepath_get and
> apr_filepath_root are assumed to be identity, but they are not under
> APR and Win32.  MSVC runtime reports c:\ via apr_filepath_get(), but
> APR upcases that to C:\ when TRUENAME is set.  

Actually, before you run the test, why not try cd C:\Path-to-apr\ ???
You will realize you just flipped from failing under one case to failing
under another case.

> Hence, c != C.  That is
> the root of the incorrect behavior and what's fixed in 422157.

Nope, not fixed.  TRUENAME has a meaning and you just broke it, not
by canonicalizing to lower case (a silly half-assed solution seeing
as it breaks the other half of the cases) but by removing the promise
that TRUENAME performs canonicalization.

>> > Win32 reports c:\ and APR expects it to be C:\.   So, the tests fail.
>>
>> Test(s) plural?  Cite them.
> 
> I think there are more assumptions in Win32 testnames that rely upon
> the identity functions working - so they have to be fixed up too.  In
> short, testnames would need to be written to be drive-letter
> case-insensitive on Win32.  -- justin

Sure thing.  That sounds like a solution.

Mime
View raw message