Thank you for your reply. > Should we copy file permission bits as well as content? > For example, with the svn_io_copy_perms() function?
As far as I can see the subsequent OP_FILE_INSTALL will not copy permissions, so copying permissions for the temporary file is not going to change the behavior. > 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. The svn_io_copy_file() function makes another temp file and performs a svn_io_file_rename2() on for atomicity. For this case, I think that it would be better to avoid creating additional temporary files. I've updated the log message -- firstly I've missed changes in tests. 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. * subversion/tests/libsvn_client/conflicts-test.c (test_resolve_choose_merged_non_existing_path): New test. (test_funcs): Add new test to list. Patch by: sergey.raevskiy{_AT_}visualsvn.com ]]] On Thu, Apr 21, 2022 at 10:04 PM Stefan Sperling <s...@elego.de> wrote: > 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. >