You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Justin Erenkrantz <je...@apache.org> on 2002/05/09 23:25:41 UTC
Re: svn commit: rev 1919 - trunk/subversion/clients/cmdline
On Thu, May 09, 2002 at 06:07:56PM -0500, philip@tigris.org wrote:
> Modified: trunk/subversion/clients/cmdline/util.c
> ==============================================================================
> --- trunk/subversion/clients/cmdline/util.c (original)
> +++ trunk/subversion/clients/cmdline/util.c Thu May 9 18:07:50 2002
> @@ -370,33 +370,30 @@
> const char *cmd;
> apr_file_t *tmp_file;
> svn_stringbuf_t *tmpfile_name;
> - apr_status_t apr_err;
> + apr_status_t apr_err, apr_err2;
> 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. */
> + /* Abort if there is no editor specified */
> if (! editor)
> return svn_error_create
> (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"
> - "one of them, check if you also need to `export' it.\n");
> + "None of the environment variables "
> + "SVN_EDITOR, VISUAL or EDITOR is set.");
s/is/are/
> @@ -408,92 +405,76 @@
> apr_err = apr_file_write_full (tmp_file, contents->data,
> contents->len, &written);
>
> - /* Close the file. */
> - apr_file_close (tmp_file);
> + apr_err2 = apr_file_close (tmp_file);
> + if (APR_STATUS_IS_SUCCESS(apr_err))
> + apr_err = apr_err2;
You can just do apr_err == APR_SUCCESS. We've defined APR_SUCCESS
to always be 0. So, you could even do if (!apr_err). (In fact,
that's the construct used in the next line.)
> /* 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;
> + }
Again, I'd say to just use apr_err != APR_SUCCESS or !apr_err.
<snip, snip>
> /* 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;
> + }
See above. -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 1919 - trunk/subversion/clients/cmdline
Posted by Mark Benedetto King <bk...@answerfriend.com>.
On Thu, May 09, 2002 at 06:41:28PM -0500, cmpilato@collab.net wrote:
> Philip Martin <ph...@codematters.co.uk> writes:
>
> > Justin Erenkrantz <je...@apache.org> writes:
> >
> > > > - "searched SVN_EDITOR, EDITOR, VISUAL, in that order. If you set\n"
> > > > - "one of them, check if you also need to `export' it.\n");
> > > > + "None of the environment variables "
> > > > + "SVN_EDITOR, VISUAL or EDITOR is set.");
> > >
> > > s/is/are/
> >
> > None is singular, at least it is in the UK.
>
> U.S., too. This comment is fine.
>
Stop, you're both right: "None" can generally take
both plural and singular verbs.
--ben
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 1919 - trunk/subversion/clients/cmdline
Posted by Branko Čibej <br...@xbc.nu>.
Branko Čibej wrote:
> Karl Fogel wrote:
>
>> Justin Erenkrantz <je...@apache.org> writes:
>>
>>
>>> In case you aren't following dev@apr, I just brought up removing
>>> this macro. -- justin
>>>
>>
>>
>> +1 from me (although I'm *still* wondering about those OS2 and WIN32
>> cases!)
>>
>>
> It's nonsense, at least on Windows. Look at how APR_FROM_OS_ERROR is
> defined. ERROR_SUCCESS is also guaranteed to be zero, so it's mapped
> directly ti APR_SUCCESS.
>
> Come to think of it, Win32 also has NO_ERROR, which is the same as
> ERROR_SUCCESS. OS/2 legacy, no doubt.
>
> I'm all for tossing that macro.
>
And I just noticed that APR_TO_OS:_ERROR is wrong. Actually, both are
wrong :-)
Should be:
#define APR_FROM_OS_ERROR(e) ((e) == ERROR_SUCCESS ? APR_SUCCESS : (e) + APR_OS_START_SYSERR)
#define APR_TO_OS_ERROR(e) ((e) == APR_SUCCESS ? ERROR_SUCCESS : (e) + APR_OS_START_SYSERR)
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 1919 - trunk/subversion/clients/cmdline
Posted by Branko Čibej <br...@xbc.nu>.
Karl Fogel wrote:
>Justin Erenkrantz <je...@apache.org> writes:
>
>
>>In case you aren't following dev@apr, I just brought up removing
>>this macro. -- justin
>>
>>
>
>+1 from me (although I'm *still* wondering about those OS2 and WIN32
>cases!)
>
>
It's nonsense, at least on Windows. Look at how APR_FROM_OS_ERROR is
defined. ERROR_SUCCESS is also guaranteed to be zero, so it's mapped
directly ti APR_SUCCESS.
Come to think of it, Win32 also has NO_ERROR, which is the same as
ERROR_SUCCESS. OS/2 legacy, no doubt.
I'm all for tossing that macro.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 1919 - trunk/subversion/clients/cmdline
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Justin Erenkrantz <je...@apache.org> writes:
> In case you aren't following dev@apr, I just brought up removing
> this macro. -- justin
+1 from me (although I'm *still* wondering about those OS2 and WIN32
cases!)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 1919 - trunk/subversion/clients/cmdline
Posted by Justin Erenkrantz <je...@apache.org>.
On Fri, May 10, 2002 at 03:42:23PM -0500, Karl Fogel wrote:
> Don't get me wrong: I too would like to test explicitly for it, but
> the code above makes me think the macro is necessary (also, the
> *existence* of the macro would make anyone think it's necessary to use
> it -- if direct comparison is just as good, then is there any chance
> of persuading the other APR developers to toss the macro?)
In case you aren't following dev@apr, I just brought up removing
this macro. -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 1919 - trunk/subversion/clients/cmdline
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Greg Stein <gs...@lyra.org> writes:
> > > Subversion currently has a mixture of APR_STATUS_IS_SUCCESS and
> > > explict tests. I much prefer consistency, would it be OK to change
> > > all Subversion's APR_STATUS_IS_SUCCESS() uses into explict tests?
> >
> > Yes. And +1 to doing so.
>
> Definitely. And another +1 from me.
>
> APR_SUCCESS is absolutely and always defined to be zero. Explicitly. And
> with the sole purpose being able to easily test for an error.
This issue keeps coming up, and every time I ask the same question.
Perhaps I'm just forgetting the answer each time? Anyway:
If APR_SUCCESS is absolutely and always defined to be zero, then why
does apr_errno.h define the APR_STATUS_IS_SUCCESS() macro like this
under WIN32
#define APR_STATUS_IS_SUCCESS(s) ((s) == APR_SUCCESS \
|| (s) == APR_OS_START_SYSERR + ERROR_SUCCESS)
and like this under OS2:
#define APR_STATUS_IS_SUCCESS(s) ((s) == APR_SUCCESS \
|| (s) == APR_OS_START_SYSERR + NO_ERROR)
That looks a bit more complex than just "== 0" to me... ?
Don't get me wrong: I too would like to test explicitly for it, but
the code above makes me think the macro is necessary (also, the
*existence* of the macro would make anyone think it's necessary to use
it -- if direct comparison is just as good, then is there any chance
of persuading the other APR developers to toss the macro?)
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 1919 - trunk/subversion/clients/cmdline
Posted by Greg Stein <gs...@lyra.org>.
On Fri, May 10, 2002 at 03:54:06PM +0200, Sander Striker wrote:
> > Justin Erenkrantz <je...@apache.org> writes:
> >
> > > The code for APR_STATUS_IS_SUCCESS isn't
> > > really used anywhere because it's a bad way to do things. It's
> > > fine, but I'd rather not set a precedent for that. -- justin
> >
> > Are you saying that the second value tested by the
> > APR_STATUS_IS_SUCCESS macro will never occur? That it is always
> > correct to test against zero? If so I assume the macros in
> > apr_errno.h should be changed.
> >
> > Subversion currently has a mixture of APR_STATUS_IS_SUCCESS and
> > explict tests. I much prefer consistency, would it be OK to change
> > all Subversion's APR_STATUS_IS_SUCCESS() uses into explict tests?
>
> Yes. And +1 to doing so.
Definitely. And another +1 from me.
APR_SUCCESS is absolutely and always defined to be zero. Explicitly. And
with the sole purpose being able to easily test for an error.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: svn commit: rev 1919 - trunk/subversion/clients/cmdline
Posted by Sander Striker <st...@apache.org>.
> Justin Erenkrantz <je...@apache.org> writes:
>
> > The code for APR_STATUS_IS_SUCCESS isn't
> > really used anywhere because it's a bad way to do things. It's
> > fine, but I'd rather not set a precedent for that. -- justin
>
> Are you saying that the second value tested by the
> APR_STATUS_IS_SUCCESS macro will never occur? That it is always
> correct to test against zero? If so I assume the macros in
> apr_errno.h should be changed.
>
> Subversion currently has a mixture of APR_STATUS_IS_SUCCESS and
> explict tests. I much prefer consistency, would it be OK to change
> all Subversion's APR_STATUS_IS_SUCCESS() uses into explict tests?
Yes. And +1 to doing so.
> Philip
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 1919 - trunk/subversion/clients/cmdline
Posted by Philip Martin <ph...@codematters.co.uk>.
Justin Erenkrantz <je...@apache.org> writes:
> The code for APR_STATUS_IS_SUCCESS isn't
> really used anywhere because it's a bad way to do things. It's
> fine, but I'd rather not set a precedent for that. -- justin
Are you saying that the second value tested by the
APR_STATUS_IS_SUCCESS macro will never occur? That it is always
correct to test against zero? If so I assume the macros in
apr_errno.h should be changed.
Subversion currently has a mixture of APR_STATUS_IS_SUCCESS and
explict tests. I much prefer consistency, would it be OK to change
all Subversion's APR_STATUS_IS_SUCCESS() uses into explict tests?
--
Philip
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 1919 - trunk/subversion/clients/cmdline
Posted by Justin Erenkrantz <je...@apache.org>.
On Thu, May 09, 2002 at 06:41:28PM -0500, cmpilato@collab.net wrote:
> > None is singular, at least it is in the UK.
>
> U.S., too. This comment is fine.
http://www.dictionary.com/cgi-bin/dict.pl?term=none&r=67
It's a point of debate. =)
"None are set" sounds better to my ear than "none is set." But,
it is your call. The code for APR_STATUS_IS_SUCCESS isn't
really used anywhere because it's a bad way to do things. It's
fine, but I'd rather not set a precedent for that. -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 1919 - trunk/subversion/clients/cmdline
Posted by cm...@collab.net.
Philip Martin <ph...@codematters.co.uk> writes:
> Justin Erenkrantz <je...@apache.org> writes:
>
> > > - "searched SVN_EDITOR, EDITOR, VISUAL, in that order. If you set\n"
> > > - "one of them, check if you also need to `export' it.\n");
> > > + "None of the environment variables "
> > > + "SVN_EDITOR, VISUAL or EDITOR is set.");
> >
> > s/is/are/
>
> None is singular, at least it is in the UK.
U.S., too. This comment is fine.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 1919 - trunk/subversion/clients/cmdline
Posted by Philip Martin <ph...@codematters.co.uk>.
Justin Erenkrantz <je...@apache.org> writes:
> > - "searched SVN_EDITOR, EDITOR, VISUAL, in that order. If you set\n"
> > - "one of them, check if you also need to `export' it.\n");
> > + "None of the environment variables "
> > + "SVN_EDITOR, VISUAL or EDITOR is set.");
>
> s/is/are/
None is singular, at least it is in the UK.
>
> > @@ -408,92 +405,76 @@
> > apr_err = apr_file_write_full (tmp_file, contents->data,
> > contents->len, &written);
> >
> > - /* Close the file. */
> > - apr_file_close (tmp_file);
> > + apr_err2 = apr_file_close (tmp_file);
> > + if (APR_STATUS_IS_SUCCESS(apr_err))
> > + apr_err = apr_err2;
>
> You can just do apr_err == APR_SUCCESS. We've defined APR_SUCCESS
> to always be 0. So, you could even do if (!apr_err). (In fact,
> that's the construct used in the next line.)
So this in apr_errno.h is junk?
#define APR_STATUS_IS_SUCCESS(s) ((s) == APR_SUCCESS \
|| (s) == APR_OS_START_SYSERR + NO_ERROR)
--
Philip
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org