On 07 Jul 2005 08:43:16 -0500, kfogel@collab.net <kfogel@collab.net> wrote:
> 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).
>
Based on the comment at the top of that loop, I assumed it would only
apply to using the editor, and yes thought the prep work could be
avoided. But I see you point on that issue, I'm just not sure what
other method could use that prep work and still be non-interactive.
Given that argument the only other reason is personal style, to me it
would show the logical flow better to put the check before the loop,
and adding a new method of generating the comment would then either
fall naturally into the interactive or non-interactive part of the
function.
> 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.)
>
I've never read that before, but agree 100%. With this particular
detail, I just believed from the viewpoint of a user with no knowledge
of the function, that they would not know what to do differently from
that error message. That is, they would have to follow this chain of
logic:
-Note my inner monologues are usually on the sarcastic end (not a
reflection on svn, more on my self)
Write script
Run it - damn didn't work
Find error - Can't launch the editor when non-interactive
Well, duh, I told you not to be interactive
Wait why are you trying to launch the editor
Oh, I'm an idiot I forgot the log message
I was just trying to skip the Well and Wait steps, because I have many
a day when that train of logic could take a lot longer than typing it,
especially in a large script with other possible sources of error.
Josh
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Jul 7 17:37:16 2005