Re: svn commit: r1757761 - /subversion/trunk/subversion/libsvn_client/conflicts.c

On 26 August 2016 at 10:53, Bert Huijben <bert_at_qqmail.nl> wrote:
>> -----Original Message-----
>> From: ivan_at_apache.org [mailto:ivan_at_apache.org]
>> Sent: vrijdag 26 augustus 2016 00:08
>> To: commits_at_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