[PATCH] external editor, does this work on Windows...

Hi

I believe we should not be ignoring errors when running the external
editor, and that we should report more error information so that the
user can determine why it failed. This patch works on Linux, can
someone with a Windows box tell me if it works there.

Put $VISUAL in the conventional place before $EDITOR.

Report system errors if the external editor fails.

* subversion/clients/cmdline/util.c (svn_cl__edit_externally): Check
  VISUAL before EDITOR. Add system() return value check. Check all
  function return values. Don't always return success.

Index: ./subversion/clients/cmdline/util.c
===================================================================
--- ./subversion/clients/cmdline/util.c
+++ ./subversion/clients/cmdline/util.c Thu May 9 01:02:11 2002
@@ -374,13 +374,14 @@
   apr_size_t written;
   apr_finfo_t finfo_before, finfo_after;
   svn_error_t *err = SVN_NO_ERROR;
+ int sys_err;
 
   /* Try to find an editor in the environment. */
   editor = getenv ("SVN_EDITOR");
   if (! editor)
- editor = getenv ("EDITOR");
- if (! editor)
     editor = getenv ("VISUAL");
+ if (! editor)
+ editor = getenv ("EDITOR");
 
   /* If there is no editor specified, return an error stating that a
      log message should be supplied via command-line options. */
@@ -389,7 +390,7 @@
       (SVN_ERR_CL_NO_EXTERNAL_EDITOR, 0, NULL, pool,
        "Could not find an external text editor in the usual environment "
        "variables;\n"
- "searched SVN_EDITOR, EDITOR, VISUAL, in that order. If you set\n"
+ "searched SVN_EDITOR, VISUAL, EDITOR, in that order. If you set\n"
        "one of them, check if you also need to `export' it.\n");
 
   /* By now, we had better have an EDITOR to work with. */
@@ -409,32 +410,67 @@
                                  contents->len, &written);
 
   /* Close the file. */
- apr_file_close (tmp_file);
+ if (APR_STATUS_IS_SUCCESS(apr_err))
+ apr_err = apr_file_close (tmp_file);
   
   /* Make sure the whole CONTENTS were written, else return an error. */
   if (apr_err || (written != contents->len))
     {
- err = svn_error_create
+ err = svn_error_createf
         (apr_err ? apr_err : SVN_ERR_INCOMPLETE_DATA, 0, NULL, pool,
- "Unable to write initial contents to temporary file.");
+ "failed writing '%s'", tmpfile_name->data);
       goto cleanup;
     }
 
   /* Get information about the temporary file before the user has
      been allowed to edit its contents. */
- apr_stat (&finfo_before, tmpfile_name->data,
- APR_FINFO_MTIME | APR_FINFO_SIZE, pool);
+ apr_err = apr_stat (&finfo_before, tmpfile_name->data,
+ APR_FINFO_MTIME | APR_FINFO_SIZE, pool);
+ if (! APR_STATUS_IS_SUCCESS(apr_err))
+ {
+ err = svn_error_createf (apr_err, 0, NULL, pool,
+ "failed to stat '%s'", tmpfile_name->data);
+ goto cleanup;
+ }
 
- /* Now, run the editor command line. Ignore the return values; all
- we really care about (for now) is whether or not our tmpfile
- contents have changed. */
+ /* Now, run the editor command line. */
   cmd = apr_psprintf (pool, "%s %s", editor, tmpfile_name->data);
 
- system (cmd);
+ sys_err = system (cmd);
+ if (sys_err == -1)
+ {
+ err = svn_error_createf (SVN_ERR_EXTERNAL_PROGRAM, 0, NULL, pool,
+ "system('%s') returned -1", cmd);
+ }
+ else if (! WIFEXITED(sys_err))
+ {
+ if (WIFSIGNALED(sys_err))
+ err = svn_error_createf (SVN_ERR_EXTERNAL_PROGRAM, 0, NULL, pool,
+ "system('%s') interrupted by signal %d",
+ cmd, WTERMSIG(sys_err));
+ else
+ /* Something weird happened! */
+ err = svn_error_createf (SVN_ERR_EXTERNAL_PROGRAM, 0, NULL, pool,
+ "system('%s') failed", cmd);
+ }
+ else if (WEXITSTATUS(sys_err) != 0)
+ {
+ err = svn_error_createf (SVN_ERR_EXTERNAL_PROGRAM, 0, NULL, pool,
+ "system('%s') returned %d",
+ cmd, WEXITSTATUS(sys_err));
+ }
+ if (err)
+ goto cleanup;
   
   /* Get information about the temporary file after the assumed editing. */
- apr_stat (&finfo_after, tmpfile_name->data,
- APR_FINFO_MTIME | APR_FINFO_SIZE, pool);
+ apr_err = apr_stat (&finfo_after, tmpfile_name->data,
+ APR_FINFO_MTIME | APR_FINFO_SIZE, pool);
+ if (! APR_STATUS_IS_SUCCESS(apr_err))
+ {
+ err = svn_error_createf (apr_err, 0, NULL, pool,
+ "failed to stat '%s'", tmpfile_name->data);
+ goto cleanup;
+ }
   
   /* If the file looks changed... */
   if ((finfo_before.mtime != finfo_after.mtime) ||
@@ -450,8 +486,8 @@
         {
           /* This is an annoying situation, as the file seems to have
              been edited but we can't read it! */
-
- /* ### todo: Handle error here. */
+ err = svn_error_createf (apr_err, 0, NULL, pool,
+ "failed to open '%s'", tmpfile_name->data);
           goto cleanup;
         }
       else
@@ -466,13 +502,16 @@
           new_contents->data[new_contents->len] = 0;
           
           /* Close the file */
- apr_file_close (tmp_file);
+ if (APR_STATUS_IS_SUCCESS(apr_err))
+ apr_err = apr_file_close (tmp_file);
           
           /* Make sure we read the whole file, or return an error if we
              didn't. */
           if (apr_err || (new_contents->len != finfo_after.size))
             {
- /* ### todo: Handle error here. */
+ err = svn_error_createf
+ (apr_err ? apr_err : SVN_ERR_INCOMPLETE_DATA, 0, NULL, pool,
+ "failed reading '%s'", tmpfile_name->data);
               goto cleanup;
             }
         }
@@ -489,11 +528,17 @@
 
  cleanup:
 
- /* Destroy the temp file if we created one. */
- apr_file_remove (tmpfile_name->data, pool); /* ignore status */
+ /* Remove the temp file */
+ apr_err = apr_file_remove (tmpfile_name->data, pool);
+
+ /* Only report remove error if there was no previous error. */
+ if (! err && ! APR_STATUS_IS_SUCCESS(apr_err))
+ err = svn_error_createf (apr_err, 0, NULL, pool,
+ "failed to remove '%s'", tmpfile_name->data);
+
 
   /* ...and return! */
- return SVN_NO_ERROR;
+ return err;
 }
 
 

-- 
Philip
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org