On Thu, Apr 21, 2022 at 08:25:32PM +0300, Sergey Raevskiy wrote:
> I've noticed that in some cases a working copy can become inoperable
> after merge text conflict resolution with `svn_wc_conflict_choose_merged'
> option.
>
> The problem can occur in the following scenario:
>
> 1. There is a text conflict, that is resolved with
> `svn_wc_conflict_choose_merged' option and specifying some
> path as MERGED_FILE. According to the API it can be any file,
> let's say "D:\merged.txt".
>
> 2. Previous step adds a FILE_INSTALL item into workqueue with a
> reference to "D:\merged.txt" and then runs the workqueue.
>
> 3. An error happens when running the workqueue.
> For example a working file cannot be overwritten because
> of 'Access denied' error.
>
> 4. The "D:\merged.txt" file gets deleted for some reason.
>
> 5. At this point the working copy is inoperable and cannot be
> fixed by 'svn cleanup', because workqueue contains absolute
> path to non-existing file ("D:\merged.txt").
>
> I've attached a patch with fix for this problem. The patch contains a
> simplified regression test that reproduces problem by returning
> non-existing path from resolver callback.
>
> Log message:
> [[[
> Make sure resolving a conflict with `svn_wc_conflict_choose_merged' can't
> break the workqueue by referencing a non-existing file
>
> * subversion/libsvn_wc/conflicts.c
> (build_text_conflict_resolve_items): Create private copy of MERGED_FILE
> contents in case of `svn_wc_conflict_choose_merged' is specified.
> Doing that to avoid referencing a potentially absent file in FILE_INSTALL
> workqueue record.
>
> Patch by: sergey.raevskiy{_AT_}visualsvn.com
> ]]]
Thanks, Sergey! Your fix makes sense to me. One question below:
> Index: subversion/libsvn_wc/conflicts.c
> ===================================================================
> --- subversion/libsvn_wc/conflicts.c (revision 1900114)
> +++ subversion/libsvn_wc/conflicts.c (working copy)
> @@ -1706,9 +1706,49 @@ build_text_conflict_resolve_items(svn_skel_t **wor
> good to use". */
> case svn_wc_conflict_choose_merged:
> {
> - install_from_abspath = merged_file
> - ? merged_file
> - : local_abspath;
> + if (merged_file)
> + {
> + /* Create private copy of merged MERGED_FILE contents to
> + avoid referencing a potentially absent file in FILE_INSTALL
> + workqueue record. */
> +
> + const char *temp_dir_abspath;
> + const char *temp_file_abspath;
> + svn_stream_t *merged_contents;
> + svn_stream_t *tmp_contents;
> + apr_file_t *file;
> +
> + SVN_ERR(svn_io_file_open(&file, merged_file,
> + APR_READ, APR_OS_DEFAULT,
> + scratch_pool));
> + merged_contents = svn_stream_from_aprfile2(file, FALSE,
> + scratch_pool);
> +
> + SVN_ERR(svn_wc__db_temp_wcroot_tempdir(&temp_dir_abspath, db,
> + local_abspath,
> + scratch_pool,
> + scratch_pool));
> +
> + SVN_ERR(svn_stream_open_unique(&tmp_contents,
> + &temp_file_abspath,
> + temp_dir_abspath,
> + svn_io_file_del_none,
> + scratch_pool, scratch_pool));
> +
> + SVN_ERR(svn_stream_copy3(merged_contents, tmp_contents,
> + cancel_func, cancel_baton,
> + scratch_pool));
> +
> + install_from_abspath = temp_file_abspath;
Should we copy file permission bits as well as content?
For example, with the svn_io_copy_perms() function?
Or perhaps this entire copy operation could be done with svn_io_copy_file(),
and with the copy_perms parameter set to TRUE? If this is possible then the
code would be a lot simpler.
> +static svn_error_t *
> +test_resolve_choose_merged_non_existing_path(const svn_test_opts_t *opts,
> + apr_pool_t *pool)
> +{
We could also verify matching file system permission bits in this test.
