subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bert Huijben" <b...@qqmail.nl>
Subject RE: [PATCH] WC-NG properties API - complete doc strings
Date Tue, 02 Mar 2010 11:20:44 GMT


> -----Original Message-----
> From: Greg Stein [mailto:gstein@gmail.com]
> Sent: dinsdag 2 maart 2010 0:18
> To: Julian Foad
> Cc: dev@subversion.apache.org
> Subject: Re: [PATCH] WC-NG properties API - complete doc strings
> 
> On Mon, Mar 1, 2010 at 07:23, Julian Foad <julian.foad@wandisco.com>
> wrote:
> > For review, please.  The patch below aims to provide complete doc
> > strings for (the existing implementation of) the WC-NG properties API.
> 
> Looks great, thanks.
> 
> > I assume "properties" refers to regular, versioned properties
> > throughout.
> 
> Yes.
> 
> > I checked the implementation with regard to returning an error when the
> > node is not found.
> 
> It should always return an error. We were very inconsistent before,
> and my intent was to error in all cases when a node is not found
> (thus, my slight concern around the read_kind() function).
> 
> If it doesn't error, then that should be considered a bug.
> 
> > I believe svn_wc__db_base_get_props() is meant to return an empty
> > (non-NULL) hash to represent "no properties", both because that's
> 
> Correct.
> 
> > consistent with the input and outputs of the rest of these functions and
> > because the rest of these functions never write "null" to the
> > "properties" column of the BASE table when writing an empty set of
> > properties.  The non-nullness of that SQL column is not (yet) documented
> > so I am not certain that there are no other functions that could write
> > "null".
> 
> A null value in ACTUAL_NODE.properties means "no changes w.r.t the
> pristine properties". Any value in there is the complete set of
> (changed/deleted/added) properties.

I would remove that part of '(changed/deleted/added)' as it also contains
the unmodified properties. It is really the complete set.

> 
> I don't think that a null value in WORKING_NODE or BASE_NODE makes
> sense. We could interpret that as "incomplete", or as a short-form for
> "no properties". iirc, a skel for empty-property-hash is "()" (ie.
> just 2 bytes).

I think the current code assumes that the properties in WORKING_NODE are
NULL, for the base-deleted case. (And not sure about deleted from a copy,
but if that case is NULL it would be a bug... The tests completed by the end
of October, so they should be ok)

	Bert


Mime
View raw message