Joshua Varner <jlvarner@gmail.com> writes:
> On 7/6/05, kfogel@tigris.org <kfogel@tigris.org> wrote:
> > Author: kfogel
> > Date: Wed Jul 6 23:16:13 2005
> > New Revision: 15282
> >
> <snip>
> >
> ==============================================================================
> > --- trunk/subversion/clients/cmdline/util.c (original)
> > +++ trunk/subversion/clients/cmdline/util.c Wed Jul 6 23:16:13 2005
> > @@ -545,7 +545,7 @@
> > else /* non_interactive flag says we can't pop up an editor, so error */
> > {
> > return svn_error_create
> > - (SVN_ERR_CL_NO_EDITOR_WHEN_NONINTERACTIVE, NULL,
> > + (SVN_ERR_NONINTERACTIVE, NULL,
> > _("Cannot invoke editor to get log message "
> > "when non-interactive"));
> > }
> >
> I may be missing something, but why isn't this check occuring at line 478:
> while (! message)
> {
> /* We still don't have a valid commit message. Use $EDITOR to
> get one. Note that svn_cl__edit_externally will still return
> a UTF-8'ized log message. */
>
> This whole loop is exists to invoke the editor, so why enter it at all
> if in interactive mode?
> Then if that's the case shouldn't the error be either
> SVN_ERR_REPOS_BAD_ARGS or SVN_ERR_CL_MUTUALLY_EXCLUSIVE_ARGS, since
> specifying non-interactive without specifying a commit message on the
> command line is the error. Or maybe a new error
> SVN_ERR_CL_REQUIRED_ARGS_MISSING.
Thanks for reviewing the commit!
Regarding the placement of the check:
Well, I'm not sure that one way is better than the other. I
considered placing it at the top of the loop, but felt that then the
check wouldn't be localized enough. After all, the sin we're trying
to avoid is that of actually starting up the editor, not that of doing
the prep work for starting the editor -- prep work that might,
someday, become applicable to something else in addition to starting
an editor, at which point my overeager conditional would become a bug
source.
I don't feel strongly that one way is right and the other wrong
though; if there's a good argument for placing the check earlier, we
should do that. However I think that the argument "This way we'd
avoid doing the prep work" would not be good enough, since we're going
to return error for the first iteration anyway (I'm not sure you were
actually making that argument, I'm just anticipating it).
Regarding the error code:
Oops, you're right. I didn't notice those codes when I was looking
for an appropriate one to use. Actually there's an even more
appropriate one: SVN_ERR_CL_INSUFFICIENT_ARGS. In r15285 I've gotten
rid of the new error code and just used SVN_ERR_CL_INSUFFICIENT_ARGS.
(I mean, lightblue.bikeshed.com and all that, but still you're right
that we should avoid introducing new error codes if we can avoid it.)
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Jul 7 16:31:41 2005