On 26 August 2016 at 10:53, Bert Huijben <b...@qqmail.nl> wrote: >> -----Original Message----- >> From: i...@apache.org [mailto:i...@apache.org] >> Sent: vrijdag 26 augustus 2016 00:08 >> To: comm...@subversion.apache.org >> Subject: svn commit: r1757761 - >> /subversion/trunk/subversion/libsvn_client/conflicts.c >> >> Author: ivan >> Date: Thu Aug 25 22:08:19 2016 >> New Revision: 1757761 >> >> URL: http://svn.apache.org/viewvc?rev=1757761&view=rev >> Log: >> Simplify tree conflict resolution code a bit. >> >> * subversion/libsvn_client/conflicts.c >> (resolve_update_incoming_added_file_replace): Pass NULL as FILE to >> svn_io_open_uniquely_named() -- in this case it will close temporary file >> automatically. >> >> Modified: >> subversion/trunk/subversion/libsvn_client/conflicts.c >> >> Modified: subversion/trunk/subversion/libsvn_client/conflicts.c >> URL: >> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/conflic >> ts.c?rev=1757761&r1=1757760&r2=1757761&view=diff >> ================================================================ >> ============== >> --- subversion/trunk/subversion/libsvn_client/conflicts.c (original) >> +++ subversion/trunk/subversion/libsvn_client/conflicts.c Thu Aug 25 22:08:19 >> 2016 >> @@ -5402,7 +5402,6 @@ resolve_update_incoming_added_file_repla >> const char *lock_abspath; >> svn_client_ctx_t *ctx = conflict->ctx; >> svn_error_t *err; >> - apr_file_t *backup_file; >> const char *backup_path; >> >> option_id = svn_client_conflict_option_get_id(option); >> @@ -5422,7 +5421,7 @@ resolve_update_incoming_added_file_repla >> * which means it does not exist in the repository. So it's a good idea >> * to keep a backup, just in case someone picks this option by accident. >> * First, reserve a name in the filesystem. */ >> - err = svn_io_open_uniquely_named(&backup_file, &backup_path, >> + err = svn_io_open_uniquely_named(NULL, &backup_path, >> svn_dirent_dirname(local_abspath, >> scratch_pool), >> svn_dirent_basename(local_abspath, >> @@ -5433,13 +5432,9 @@ resolve_update_incoming_added_file_repla >> if (err) >> goto unlock_wc; >> >> - /* Close and remove the file. We're going to move the conflict victim >> - * on top and, at least on Windows, open files can't be replaced. >> + /* Remove the file. We're going to move the conflict victim on top and, at >> + * least on Windows, open files can't be replaced. >> * The WC is locked so anything racing us here is external to SVN. */ >> - err = svn_io_file_close(backup_file, scratch_pool); >> - if (err) >> - goto unlock_wc; >> - >> err = svn_error_compose_create(err, svn_io_remove_file2(backup_path, >> TRUE, >> scratch_pool)); > > This remove file right after the change will not always work if the file is > not > explicitly closed before. On Windows it depends on apr opening the file > with delete share flags (10% perf hit in some cases) and nobody else opening > the file. > > In general strictly closing before deleting is a safer procedure. > Hi Bert, thanks for review!
I'm aware about Windows behavior of deleting opened file, but it's not that case, because svn_io_open_uniquely_named() *explicitly* closes file @a file is NULL: [[[ * Open a new file (for reading and writing) with a unique name based on * utf-8 encoded @a filename, in the directory @a dirpath. The file handle is * returned in @a *file, and the name, which ends with @a suffix, is returned * in @a *unique_name, also utf8-encoded. Either @a file or @a unique_name * may be @c NULL. If @a file is @c NULL, the file will be created but not * open. ]]] -- Ivan Zhakov
