We have a baton in the callback, so no reason for bottle magic. Bert
-----Original Message----- From: "Gabriela Gibson" <gabriela.gib...@gmail.com> Sent: 23-12-2013 20:14 To: "Subversion Development" <dev@subversion.apache.org> Subject: prop edit: lost user edit bug Hi, I found the other day that if my network fails for some reason whilst editing a prop, the entire edit is lost. I took a look at the propedit code and found the following: in subversion/svn/propedit-cmd.c in line 145 and 278 we call: SVN_ERR(svn_cmdline__edit_string_externally( &propval, NULL, ..... which lets the user write the prop but removes the file. This function is defined in subversion/include/private/svn_cmdline_private.h: and the code is located in subversion/svn/propedit-cmd.c: svn_error_t * svn_cmdline__edit_string_externally(svn_string_t **edited_contents, const char **tmpfile_left, ... In the function body, the variable tmpfile_left is actually never used as a data conduit, but as a boolean flag in order to set remove_file and then reassigned internally if it's detected inside this function ie: if (tmpfile_left) { *tmpfile_left = svn_dirent_join(base_dir, tmpfile_name, pool); remove_file = FALSE; } However, this assigned variable isn't used anywhere either, only the remove_file flag is utilised. Callers of this function are located in ./svnmucc/svnmucc.c:767, ./svn/propedit-cmd.c:143: ./svn/propedit-cmd.c:278: where in every case, tmpfile_left is seeded as NULL, and ./svn/util.c:431: where it's called with an value that's carried via struct log_msg_baton *lmb = log_msg_baton; like so: err = svn_cmdline__edit_string_externally(&msg_string, lmb->tmpfile_left, ... I could change the calls in subversion/svn/propedit-cmd.c to give this a value and prevent the removal of the tmp file, but that would leave the file sitting around even if the commit of the prop is successful. Also, if the commit fails, the user would still not be informed where to find their work so they can resubmit after fixing their network issues. We could write to console to inform the user and just live with one extra file in /tmp. I think the tmpfile_left could be changed to svn_boolean_t, and if lmb->tmpfile_left needs updating it probably is clearer if that happens in the calling code rather than in a service function, since it's just built from the parameters that were passed in. I'm not sure what to do next, would you have some advice for me please? Gabriela