You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@codematters.co.uk> on 2002/05/09 00:10:08 UTC
[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
Re: [PATCH] external editor, does this work on Windows...
Posted by Philip Martin <ph...@codematters.co.uk>.
cmpilato@collab.net writes:
> > * 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.
>
> I get errors on undefined macros WIFEXITED, WIFSIGNALED, WTERMSIG,
> WEXITSTATUS. I don't think those things are portable.
I wondered what VC would make of those. Is there a <sys/wait.h> file?
I suppose we have to just print the raw return value, which
unfortunately makes it harder for a user to determine why the command
failed. Can we at least assume that zero is success?
Looking at APR's Win32 implementation of apr_proc_wait for inspiration
I see that it doesn't even set the exitwhy return value. This could
be a problem, since on Unix subversion should really be checking that
before deciding that an exit status of zero indicates success.
--
Philip
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] external editor, does this work on Windows...
Posted by cm...@collab.net.
Philip Martin <ph...@codematters.co.uk> writes:
> 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.
I get errors on undefined macros WIFEXITED, WIFSIGNALED, WTERMSIG,
WEXITSTATUS. I don't think those things are portable.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org