Karl, your r3484 chante to io.c:svn_io_copy_file...

[Branko Čibej <br...@xbc.nu>]

...breaks the Win32 tests badly. Why did you ignore (and remove) the
Win32-specific warning in the comment there?

I really don't understand what you're trying to achieve with copying
permissions after the rename. On Unix, a rename preserves permisions, as
long as it's within the same device -- as this rename is, being in the
same directory. So you could just tell apr_file_copy to copy permissions
to the temporary file, and then rename it. In fact, the way things stand
now, the permission copying is _not_ atomic, so your comment in the log
message is misleading.

For now, I'll disable the copy_perms flag on Windows, so that I can run
the test, and put that warning back, so that people don't forget about
the issue. We can decide about a real solution after we've discussed
this a bit more.

-- 
Brane Čibej   <br...@xbc.nu>   http://www.xbc.nu/brane/


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Karl, your r3484 chante to io.c:svn_io_copy_file...

[Branko Čibej <br...@xbc.nu>]

Karl Fogel wrote:

>(Of course, you would *think* that if you call apr_get_file_info() on
>Platform Foo, and pass the results along as perms to apr_file_copy
>also on Platform Foo, that this could not be a problem.  Silly me.)
>
Ah, but this is not Platform Foo we're talking about -- it's Platform
Eeeek! :-)
Seriously: apr_file_info_get() does try hard to map the Windows ACLs to
Unix permissions, but the mapping is simply not possible in general.

>>For now, I'll disable the copy_perms flag on Windows, so that I can run
>>the test, and put that warning back, so that people don't forget about
>>the issue. We can decide about a real solution after we've discussed
>>this a bit more.
>>    
>>
>
>I think the cleanest solution would be to make the tmp copy with perms
>(except on Win32) and then rely on the rename behavior described
>above.
>
>How does that sound to you?
>  
>
That would bring the implementation close to what it was, except for the
additional rename, correct? And the warning put back. Yes, I agree.
Later on we can teach apr_file_copy to honour APR_FILE_SOURCE_PERMS on
Windows, too.


-- 
Brane Čibej   <br...@xbc.nu>   http://www.xbc.nu/brane/


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Karl, your r3484 chante to io.c:svn_io_copy_file...

[Karl Fogel <kf...@newton.ch.collab.net>]

Branko Čibej <br...@xbc.nu> writes:
> ...breaks the Win32 tests badly. Why did you ignore (and remove) the
> Win32-specific warning in the comment there?

Because apr_get_file_info() was not documented to return
APR_INCOMPLETE, sir.

I didn't ignore the warning.  I did indeed remove it :-).  It said
"apr_file_copy with perms may fail on Win32", and I was changing the
code to no longer call apr_file_copy with perms, ergo... ?

(Of course, you would *think* that if you call apr_get_file_info() on
Platform Foo, and pass the results along as perms to apr_file_copy
also on Platform Foo, that this could not be a problem.  Silly me.)

> I really don't understand what you're trying to achieve with copying
> permissions after the rename. On Unix, a rename preserves permisions, as
> long as it's within the same device -- as this rename is, being in the
> same directory. So you could just tell apr_file_copy to copy permissions
> to the temporary file, and then rename it. In fact, the way things stand
> now, the permission copying is _not_ atomic, so your comment in the log
> message is misleading.

Thanks, I wasn't aware that the rename perms preservation was
completely reliable within the device.  Since it is, the way you
describe would be a better way to do it.

You're right that the log message was misleading.  I will point out
that the doc string I wrote *did* describe the true state of affairs,
however.

> For now, I'll disable the copy_perms flag on Windows, so that I can run
> the test, and put that warning back, so that people don't forget about
> the issue. We can decide about a real solution after we've discussed
> this a bit more.

I think the cleanest solution would be to make the tmp copy with perms
(except on Win32) and then rely on the rename behavior described
above.

How does that sound to you?

-K

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org