You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Blair Zajac <bl...@orcaware.com> on 2008/06/12 15:00:44 UTC
Validating svn_wc_entry_t fields in write_entry()?
I am using a svn_opt_revision_t to store the file external revision number, but
I only allow the svn_opt_revision_unspecified, svn_opt_revision_head and
svn_opt_revision_number kinds.
I'd like to have write_entry() check the validity of the svn_opt_revision_t
value before it writes it out, but currently write_entry() returns void.
I was thinking of having write_entry() return a svn_error_t * and then I could
bubble up any errors.
Is this a good place to put a check that the rest of my code has properly set
the svn_wc_entry_t fields?
Blair
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Validating svn_wc_entry_t fields in write_entry()?
Posted by Julian Foad <ju...@btopenworld.com>.
On Fri, 2008-06-13 at 08:25 -0700, Blair Zajac wrote:
> I decided that I wasn't happy ignoring errors here, even if consistency checks
> can be made earlier: r31729.
>
> With this change, I can also remove an assert(): r31730.
- assert(name);
+ if (! name)
+ return svn_error_createf
+ (SVN_ERR_INCORRECT_PARAMS, NULL,
+ _("write_entry() cannot be called with a NULL name argument"));
This change implies that returning an svn_error_t with a localisable
message is inherently "better" than an assertion, but it trades a
potential behavioural improvement (if such a bug exists in our library,
a program using it can trap it cleanly rather than aborting) for a
decrease in readability (4 lines just to indicate one simple fact) and
an increase in translator effort.
I can't single out and complain against this particular case: it's a
dilemma we've always had. However, this has prompted me into proposing a
solution. See the new thread entitled "[RFC] Replacement for "assert" in
the libraries".
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Validating svn_wc_entry_t fields in write_entry()?
Posted by Blair Zajac <bl...@orcaware.com>.
I decided that I wasn't happy ignoring errors here, even if consistency checks
can be made earlier: r31729.
With this change, I can also remove an assert(): r31730.
Blair
Blair Zajac wrote:
> Thanks, so I'll add checking code later.
>
> However, if an invalid revision kind does appear there, there's no
> mechanism to bubble up an error through write_entry. Should we have one?
>
> Without the ability to propagate an error, I'm going to have the file
> external just not write the file externals information into the entries
> file, which will leave it as a normal switched file to successive svn
> operations. If a user sees a file external turn into a normal switched
> file, then hopefully they'll report a bug.
>
> C. Michael Pilato wrote:
>> Seems to me like it'd make more sense to validate the entry bits quite
>> a bit earlier, like in svn_wc__modify_entry() or one of its helper
>> functions. Entries are cached, so writing them out can happen later
>> than when they are modified. You don't want to allow the opportunity
>> to refer to a cached bogus entry that wasn't caught simply because it
>> hasn't been flushed to disk yet.
>>
>>
>> Blair Zajac wrote:
>>> I am using a svn_opt_revision_t to store the file external revision
>>> number, but I only allow the svn_opt_revision_unspecified,
>>> svn_opt_revision_head and svn_opt_revision_number kinds.
>>>
>>> I'd like to have write_entry() check the validity of the
>>> svn_opt_revision_t value before it writes it out, but currently
>>> write_entry() returns void.
>>>
>>> I was thinking of having write_entry() return a svn_error_t * and
>>> then I could bubble up any errors.
>>>
>>> Is this a good place to put a check that the rest of my code has
>>> properly set the svn_wc_entry_t fields?
>>>
>>> Blair
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Validating svn_wc_entry_t fields in write_entry()?
Posted by Blair Zajac <bl...@orcaware.com>.
Thanks, so I'll add checking code later.
However, if an invalid revision kind does appear there, there's no mechanism to
bubble up an error through write_entry. Should we have one?
Without the ability to propagate an error, I'm going to have the file external
just not write the file externals information into the entries file, which will
leave it as a normal switched file to successive svn operations. If a user sees
a file external turn into a normal switched file, then hopefully they'll report
a bug.
C. Michael Pilato wrote:
> Seems to me like it'd make more sense to validate the entry bits quite a
> bit earlier, like in svn_wc__modify_entry() or one of its helper
> functions. Entries are cached, so writing them out can happen later than
> when they are modified. You don't want to allow the opportunity to
> refer to a cached bogus entry that wasn't caught simply because it
> hasn't been flushed to disk yet.
>
>
> Blair Zajac wrote:
>> I am using a svn_opt_revision_t to store the file external revision
>> number, but I only allow the svn_opt_revision_unspecified,
>> svn_opt_revision_head and svn_opt_revision_number kinds.
>>
>> I'd like to have write_entry() check the validity of the
>> svn_opt_revision_t value before it writes it out, but currently
>> write_entry() returns void.
>>
>> I was thinking of having write_entry() return a svn_error_t * and then
>> I could bubble up any errors.
>>
>> Is this a good place to put a check that the rest of my code has
>> properly set the svn_wc_entry_t fields?
>>
>> Blair
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Validating svn_wc_entry_t fields in write_entry()?
Posted by "C. Michael Pilato" <cm...@collab.net>.
Seems to me like it'd make more sense to validate the entry bits quite a bit
earlier, like in svn_wc__modify_entry() or one of its helper functions.
Entries are cached, so writing them out can happen later than when they are
modified. You don't want to allow the opportunity to refer to a cached
bogus entry that wasn't caught simply because it hasn't been flushed to disk
yet.
Blair Zajac wrote:
> I am using a svn_opt_revision_t to store the file external revision
> number, but I only allow the svn_opt_revision_unspecified,
> svn_opt_revision_head and svn_opt_revision_number kinds.
>
> I'd like to have write_entry() check the validity of the
> svn_opt_revision_t value before it writes it out, but currently
> write_entry() returns void.
>
> I was thinking of having write_entry() return a svn_error_t * and then I
> could bubble up any errors.
>
> Is this a good place to put a check that the rest of my code has
> properly set the svn_wc_entry_t fields?
>
> Blair
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand