Re: prop edit: lost user edit bug

[Johan Corveleyn <jc...@gmail.com>]

On Mon, Dec 23, 2013 at 8:13 PM, Gabriela Gibson
<ga...@gmail.com> wrote:
> 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?

It would sure be nice if the user's edits could be salvaged after such
an error. I don't know how I'd implement it, but behavior-wise I think
I'd be content with something like commit's behavior with the commit
message (saving it to a temporary file, and saying so in the error
message):

[[[
[~/dev/testwc]$ svn commit
<< editor pops up for commit message >>
Sending test.txt
Transmitting file data ..svn: Commit failed (details follow):
svn: Commit blocked by pre-commit hook (exit code 1) with output:
ERROR
svn: Your commit message was left in a temporary file:
svn: '/Users/jcorvel/dev/testwc/svn-commit.tmp'
]]]

Perhaps you can start by studying how this is implemented for commit?

-- 
Johan