> -----Original Message-----
> From: Ivan Zhakov [mailto:ivan_at_visualsvn.com]
> Sent: vrijdag 26 augustus 2016 11:58
> To: Bert Huijben <bert_at_qqmail.nl>
> Cc: dev_at_subversion.apache.org
> Subject: 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.
> ]]]
Ack, +1. Thanks!
Bert
Received on 2016-08-26 12:38:54 CEST