On Fri, 2005-03-11 at 08:42 +0100, Peter N. Lundblad wrote:
> In addition to Philip's review:
>
> On Thu, 10 Mar 2005 fitz@tigris.org wrote:
>
> > Author: fitz
> > Date: Thu Mar 10 15:52:46 2005
> > New Revision: 13359
> >
> > Modified: branches/locking/subversion/clients/cmdline/lock-cmd.c
> > Url: http://svn.collab.net/viewcvs/svn/branches/locking/subversion/clients/cmdline/lock-cmd.c?view=diff&rev=13359&p1=branches/locking/subversion/clients/cmdline/lock-cmd.c&r1=13358&p2=branches/locking/subversion/clients/cmdline/lock-cmd.c&r2=13359
> > ==============================================================================
> > --- branches/locking/subversion/clients/cmdline/lock-cmd.c (original)
> > +++ branches/locking/subversion/clients/cmdline/lock-cmd.c Thu Mar 10 15:52:46 2005
> > @@ -107,8 +107,11 @@
> > svn_error_t *err;
> > struct lock_baton *lb = baton;
> >
> > - err = svn_cmdline_printf (lb->pool, _("'%s' locked by user '%s'.\n"),
> > - path, lock->owner);
> > + if (ra_err)
> > + err = svn_cmdline_printf (lb->pool, _("Error: %s\n"), ra_err->message);
>
> Why don't you use the standard error handling routine here? This wil just
> print the first error message in the chain, and, more seriously, there is
> nothing guaranteeing that ra_err->message is non-NULL. Kaboom!
Oversight. Fixed in r13372.
> > Modified: branches/locking/subversion/clients/cmdline/unlock-cmd.c
> > Url: http://svn.collab.net/viewcvs/svn/branches/locking/subversion/clients/cmdline/unlock-cmd.c?view=diff&rev=13359&p1=branches/locking/subversion/clients/cmdline/unlock-cmd.c&r1=13358&p2=branches/locking/subversion/clients/cmdline/unlock-cmd.c&r2=13359
> > ==============================================================================
> > --- branches/locking/subversion/clients/cmdline/unlock-cmd.c (original)
> > +++ branches/locking/subversion/clients/cmdline/unlock-cmd.c Thu Mar 10 15:52:46 2005
> > @@ -54,7 +54,10 @@
> > svn_error_t *err;
> > struct lock_baton *lb = baton;
> >
> > - err = svn_cmdline_printf (lb->pool, _("Unlocked '%s'.\n"), path);
> > + if (ra_err)
> > + err = svn_cmdline_printf (lb->pool, _("Error: %s\n"), ra_err->message);
> > + else
> > + err = svn_cmdline_printf (lb->pool, _("Unlocked '%s'.\n"), path);
> >
> Dito. Also, should we put the path a the beginning of the line to be
> consistentwith the lock command? (I know who wrote that linte in the first
> place, just came to think of it now that I reread it:-)
I like seeing the error first as I think it stands out more.
> > Modified: branches/locking/subversion/include/svn_error.h
> > Url: http://svn.collab.net/viewcvs/svn/branches/locking/subversion/include/svn_error.h?view=diff&rev=13359&p1=branches/locking/subversion/include/svn_error.h&r1=13358&p2=branches/locking/subversion/include/svn_error.h&r2=13359
> > ==============================================================================
> > --- branches/locking/subversion/include/svn_error.h (original)
> > +++ branches/locking/subversion/include/svn_error.h Thu Mar 10 15:52:46 2005
> > @@ -218,6 +218,21 @@
> >
> > /** @} */
> >
> > +/*
> > + * @since New in 1.2.
> > + *
> > + * Return TRUE if @a err is an error specifically related to locking a
> > + * path, FALSE otherwise. */
> > +svn_boolean_t svn_error_is_lock_error (svn_error_t *err);
> > +
> I am not sure if I like putting this in the generic error file. Maybe we'd
> better have an svn_lock.h file where we put stuff related to locking. As
> it stands, we have the svn_lock_t and the callback in svn_types.h and
> these utility functions in svn_error.h. Also, we need to be more careful
> in disambiguiting this kind of locks with the locks in the WC for the
> admin area. I think this docstring needs to be more clear.
I've fixed the docstring in r13372 for both this and
svn_error_is_unlock_error to specify that we're talking about repository
locks.
I don't know that I feel that strongly about the location of this and
other locking related prototypes... does anyone else have any feelings
on this?
-Fitz
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Mar 11 19:45:22 2005