You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bert Huijben <be...@qqmail.nl> on 2016/08/26 07:53:42 UTC
RE: svn commit: r1757761 - /subversion/trunk/subversion/libsvn_client/conflicts.c
> -----Original Message-----
> From: ivan@apache.org [mailto:ivan@apache.org]
> Sent: vrijdag 26 augustus 2016 00:08
> To: commits@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.
Bert
RE: svn commit: r1757761 - /subversion/trunk/subversion/libsvn_client/conflicts.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Ivan Zhakov [mailto:ivan@visualsvn.com]
> Sent: vrijdag 26 augustus 2016 11:58
> To: Bert Huijben <be...@qqmail.nl>
> Cc: dev@subversion.apache.org
> Subject: Re: svn commit: r1757761 -
> /subversion/trunk/subversion/libsvn_client/conflicts.c
>
> On 26 August 2016 at 10:53, Bert Huijben <be...@qqmail.nl> wrote:
> >> -----Original Message-----
> >> From: ivan@apache.org [mailto:ivan@apache.org]
> >> Sent: vrijdag 26 augustus 2016 00:08
> >> To: commits@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
Re: svn commit: r1757761 - /subversion/trunk/subversion/libsvn_client/conflicts.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 26 August 2016 at 10:53, Bert Huijben <be...@qqmail.nl> wrote:
>> -----Original Message-----
>> From: ivan@apache.org [mailto:ivan@apache.org]
>> Sent: vrijdag 26 augustus 2016 00:08
>> To: commits@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