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
Received on 2013-12-23 20:14:19 CET