You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2010/04/12 16:35:54 UTC
Re: svn commit: r933272 - in
/subversion/trunk/subversion/libsvn_wc: update_editor.c wc_db.c
On Mon, 2010-04-12 at 11:48 -0400, Greg Stein wrote:
> On Mon, Apr 12, 2010 at 11:19, <ju...@apache.org> wrote:
> >...
> > +++ subversion/trunk/subversion/libsvn_wc/wc_db.c Mon Apr 12 15:19:23 2010
> > @@ -7254,13 +7254,17 @@ svn_wc__db_get_pristine_md5(const svn_ch
> > SVN_ERR(svn_sqlite__step(&have_row, stmt));
> > if (!have_row)
> > {
> > - *md5_checksum = NULL; /* ### that's not what we want. Report an error
> > - instead. */
> > - return svn_error_return(svn_sqlite__reset(stmt));
> > + *md5_checksum = NULL;
>
> There is no need to worry about the OUT params if you throw an error.
Oops, I missed that.
> > + SVN_ERR(svn_sqlite__reset(stmt));
> > + return svn_error_createf(SVN_ERR_WC_PATH_NOT_FOUND, NULL,
> > + _("The pristine text with checksum '%s' was "
> > + "not found"),
> > + svn_checksum_to_cstring_display(sha1_checksum,
> > + scratch_pool));
> > }
>
> You could write it as:
>
> return svn_error_createf(ERR, svn_sqlite__reset(stmt), ...);
>
> *shrug*
I think nesting an error normally implies that the nested error was the
cause of the top-level error, so that way doesn't look right to me. My
way is used in some places, that way in other places.
> I don't think that error code is appropriate, however. I would suggest
> either SVN_ERR_WC_DB_ERROR or creating a new code.
Ah, yes - didn't notice that.
Fixed in r933307.
Thanks.
- Julian
Re: svn commit: r933272 - in /subversion/trunk/subversion/libsvn_wc:
update_editor.c wc_db.c
Posted by Greg Stein <gs...@gmail.com>.
On Mon, Apr 12, 2010 at 13:11, Julian Foad <ju...@wandisco.com> wrote:
> Greg Stein wrote:
>> On Mon, Apr 12, 2010 at 12:35, Julian Foad <ju...@btopenworld.com> wrote:
>> > On Mon, 2010-04-12 at 11:48 -0400, Greg Stein wrote:
>> >> On Mon, Apr 12, 2010 at 11:19, <ju...@apache.org> wrote:
>> >> >...
>> >...
>> >> > + SVN_ERR(svn_sqlite__reset(stmt));
>> >> > + return svn_error_createf(SVN_ERR_WC_PATH_NOT_FOUND, NULL,
>> >> > + _("The pristine text with checksum '%s' was "
>> >> > + "not found"),
>> >> > + svn_checksum_to_cstring_display(sha1_checksum,
>> >> > + scratch_pool));
>> >> > }
>> >>
>> >> You could write it as:
>> >>
>> >> return svn_error_createf(ERR, svn_sqlite__reset(stmt), ...);
>> >>
>> >> *shrug*
>> >
>> > I think nesting an error normally implies that the nested error was the
>> > cause of the top-level error, so that way doesn't look right to me. My
>> > way is used in some places, that way in other places.
>>
>> Well... I don't think you want a sqlite error to be returned as
>> primary.
>
> Ah, you mean if the reset returns an error, because I used SVN_ERR?
> Yes, you're right. I was thinking of
> svn_error_clear(svn_sqlite__reset(stmt)) as is done in
> svn_wc__db_scan_addition(),
Ah. Yeah, just clearing it would be fine in this case, tho I tend to
like keeping them. When I wrote that line in scan_addition, I may not
have been up-to-date on the composition stuffs. Who knows.
> whereas I copied the code from
> svn_wc__db_base_get_dav_cache() which uses this undesirable construct.
Yah. Ugh.
>> The PATH_NOT_FOUND is primary. Then, there is a secondary
>> error around reset. Basically, it is just using create's CHILD param
>> as a cheap composition of the errors (rather than
>> svn_error_compose_create)
>
> I see many of the functions compose the secondary error onto the primary
> error's chain, in one way or another. But it doesn't make much sense to
> me, theoretically. As I said, I think of the chain as being the
> hierarchy of errors that lead to the primary error, so it seems wrong to
> also put follow-up/clean-up errors in that chain.
Sure. *shrug*
>...
Cheers,
-g
Re: svn commit: r933272 - in
/subversion/trunk/subversion/libsvn_wc: update_editor.c wc_db.c
Posted by Julian Foad <ju...@wandisco.com>.
Greg Stein wrote:
> On Mon, Apr 12, 2010 at 12:35, Julian Foad <ju...@btopenworld.com> wrote:
> > On Mon, 2010-04-12 at 11:48 -0400, Greg Stein wrote:
> >> On Mon, Apr 12, 2010 at 11:19, <ju...@apache.org> wrote:
> >> >...
> >...
> >> > + SVN_ERR(svn_sqlite__reset(stmt));
> >> > + return svn_error_createf(SVN_ERR_WC_PATH_NOT_FOUND, NULL,
> >> > + _("The pristine text with checksum '%s' was "
> >> > + "not found"),
> >> > + svn_checksum_to_cstring_display(sha1_checksum,
> >> > + scratch_pool));
> >> > }
> >>
> >> You could write it as:
> >>
> >> return svn_error_createf(ERR, svn_sqlite__reset(stmt), ...);
> >>
> >> *shrug*
> >
> > I think nesting an error normally implies that the nested error was the
> > cause of the top-level error, so that way doesn't look right to me. My
> > way is used in some places, that way in other places.
>
> Well... I don't think you want a sqlite error to be returned as
> primary.
Ah, you mean if the reset returns an error, because I used SVN_ERR?
Yes, you're right. I was thinking of
svn_error_clear(svn_sqlite__reset(stmt)) as is done in
svn_wc__db_scan_addition(), whereas I copied the code from
svn_wc__db_base_get_dav_cache() which uses this undesirable construct.
> The PATH_NOT_FOUND is primary. Then, there is a secondary
> error around reset. Basically, it is just using create's CHILD param
> as a cheap composition of the errors (rather than
> svn_error_compose_create)
I see many of the functions compose the secondary error onto the primary
error's chain, in one way or another. But it doesn't make much sense to
me, theoretically. As I said, I think of the chain as being the
hierarchy of errors that lead to the primary error, so it seems wrong to
also put follow-up/clean-up errors in that chain.
Maybe each error should have both a "child" error chain that is
basically its "cause", and a list of "follow-up" error chains that are
the subsequent errors encountered in cleaning up after it. So an error
report is a tree rather than a chain. Pah. That's a thought for
another day... or never.
- Julian
Re: svn commit: r933272 - in /subversion/trunk/subversion/libsvn_wc:
update_editor.c wc_db.c
Posted by Greg Stein <gs...@gmail.com>.
On Mon, Apr 12, 2010 at 12:35, Julian Foad <ju...@btopenworld.com> wrote:
> On Mon, 2010-04-12 at 11:48 -0400, Greg Stein wrote:
>> On Mon, Apr 12, 2010 at 11:19, <ju...@apache.org> wrote:
>> >...
>...
>> > + SVN_ERR(svn_sqlite__reset(stmt));
>> > + return svn_error_createf(SVN_ERR_WC_PATH_NOT_FOUND, NULL,
>> > + _("The pristine text with checksum '%s' was "
>> > + "not found"),
>> > + svn_checksum_to_cstring_display(sha1_checksum,
>> > + scratch_pool));
>> > }
>>
>> You could write it as:
>>
>> return svn_error_createf(ERR, svn_sqlite__reset(stmt), ...);
>>
>> *shrug*
>
> I think nesting an error normally implies that the nested error was the
> cause of the top-level error, so that way doesn't look right to me. My
> way is used in some places, that way in other places.
Well... I don't think you want a sqlite error to be returned as
primary. The PATH_NOT_FOUND is primary. Then, there is a secondary
error around reset. Basically, it is just using create's CHILD param
as a cheap composition of the errors (rather than
svn_error_compose_create)
>...
Cheers,
-g