Jim Correia | 1 Sep 01:48 2005
Picon

Re: [RFC] Subversion 1.3 distribution tar slimmer?

On Aug 31, 2005, at 3:30 PM, Ben Collins-Sussman wrote:
> Sorry we didn't speak up earlier, but kfogel and I are both very  
> much +1 on slimming down the release.  The early days of "everyone  
> builds from source" are over.  Pretty much all users use binaries  
> now.  Those who do build from source are either developers or  
> packagers, both of which are already plenty familiar with svn's  
> external dependencies.  Those groups, I'm sure, are already passing  
> complex arguments to our ./configure.

Well I'm a non-subversion developer who always builds from source.  
The binary packages for Mac OS X all have something wrong with them  
which prevent me from using them (yes, I'm picky.) :-)

They either build too much, install into a weird location, build the  
dynamic version, don't pass the correct flags to configure so the man  
pages end up in the right place, etc.

I find it is easier to build my own static binaries.

On Aug 31, 2005, at 5:57 PM, Justin Erenkrantz wrote:

> I'd be fine with our 'standard' 1.3 distro not having the  
> dependencies in it; but I'd prefer still having a 'full'  
> distribution that includes those dependencies.

I'd prefer that the full distro with required dependencies is kept as  
well. It makes building the source package myself so much easier than  
having to track down the dependencies (and figure out which versions  
of each I should be using.)

(Continue reading)

Ben Collins-Sussman | 1 Sep 01:53 2005
Picon

Re: [RFC] Subversion 1.3 distribution tar slimmer?


On Aug 31, 2005, at 6:48 PM, Jim Correia wrote:
>
>
>> I'd be fine with our 'standard' 1.3 distro not having the  
>> dependencies in it; but I'd prefer still having a 'full'  
>> distribution that includes those dependencies.
>>
>
> I'd prefer that the full distro with required dependencies is kept  
> as well. It makes building the source package myself so much easier  
> than having to track down the dependencies (and figure out which  
> versions of each I should be using.)

I'm sort of confused... I'm like Justin, in that sure, whenever I get  
a new box, I "bootstrap" by building everything from scratch.  But  
it's a one-time thing, right?  You only have to hunt down the  
dependencies and install them exactly once.  After that, subsequent  
upgrades of subversion keep on using the same dependencies;  the ones  
in the extra-large tarball are either useless, ignored, or contradict  
what you already have installed...?

--

-- 
www.collab.net  <>  CollabNet  |  Distributed Development On Demand
Justin Erenkrantz | 1 Sep 02:01 2005

Re: [RFC] Subversion 1.3 distribution tar slimmer?

--On August 31, 2005 6:53:22 PM -0500 Ben Collins-Sussman 
<sussman <at> collab.net> wrote:

> I'm sort of confused... I'm like Justin, in that sure, whenever I get  a
> new box, I "bootstrap" by building everything from scratch.  But  it's a
> one-time thing, right?  You only have to hunt down the  dependencies and
> install them exactly once.  After that, subsequent  upgrades of
> subversion keep on using the same dependencies;  the ones  in the
> extra-large tarball are either useless, ignored, or contradict  what you
> already have installed...?

For me, it's two phases: 1) bootstrap with the current SVN release 
including all bundled dependencies; 2) then replace all of the dependencies 
and Subversion itself with current copies (i.e. trunk).

So, I have two sets of dirs: subversion-1.2.3 and 
svn-trunk/apr-trunk/neon-trunk.

Once svn is installed, I don't care about the dependencies as I'll use the 
current versions; but to bootstrap, it'd be annoying as I don't really want 
to be compile every dependency twice - to fetch the latest release 
independently and then upgrade everything again to trunk after I get SVN 
installed.  *shrug*  -- justin
Jim Correia | 1 Sep 02:02 2005
Picon

Re: [RFC] Subversion 1.3 distribution tar slimmer?

On Aug 31, 2005, at 7:53 PM, Ben Collins-Sussman wrote:

> On Aug 31, 2005, at 6:48 PM, Jim Correia wrote:
>
>> I'd prefer that the full distro with required dependencies is kept  
>> as well. It makes building the source package myself so much  
>> easier than having to track down the dependencies (and figure out  
>> which versions of each I should be using.)
>
> I'm sort of confused... I'm like Justin, in that sure, whenever I  
> get a new box, I "bootstrap" by building everything from scratch.   
> But it's a one-time thing, right?  You only have to hunt down the  
> dependencies and install them exactly once.  After that, subsequent  
> upgrades of subversion keep on using the same dependencies;  the  
> ones in the extra-large tarball are either useless, ignored, or  
> contradict what you already have installed...?

Unlike Justin, I don't work on Apache, or have any other software  
that uses APR, neon, etc. I don't even build or install them, so I  
always uses the versions bundled with subversion.

Jim
John Peacock | 1 Sep 03:50 2005

Re: "svn diff" and "svn log" timestamp weirdness

Justin Erenkrantz wrote:
> I agree [with] Brane and Michael: fix the UI to return it over the implicit 
> period in question.  Removing any level of precision in the repository 
> from what APR produces (64-bit microseconds) gets a -1 from me too.
> 
> (There were lots of heated debates over 'time' in APR.  Ugh.)  -- justin

I'm resigned to lose this argument.  Correct handling of time values arouses 
passion in the few people who understand how thoroughly broken default 
implementations are currently.  I've fought this before (and probably will again).

John

--

-- 
John Peacock
Director of Information Research and Technology
Rowman & Littlefield Publishing Group
4720 Boston Way
Lanham, MD 20706
301-459-3366 x.5010
fax 301-429-5747
Ben Collins-Sussman | 1 Sep 03:58 2005
Picon

Re: "svn diff" and "svn log" timestamp weirdness


On Aug 31, 2005, at 8:50 PM, John Peacock wrote:

> Justin Erenkrantz wrote:
>
>> I agree [with] Brane and Michael: fix the UI to return it over the  
>> implicit period in question.  Removing any level of precision in  
>> the repository from what APR produces (64-bit microseconds) gets a  
>> -1 from me too.
>> (There were lots of heated debates over 'time' in APR.  Ugh.)  --  
>> justin
>>
>
> I'm resigned to lose this argument.  Correct handling of time  
> values arouses passion in the few people who understand how  
> thoroughly broken default implementations are currently.  I've  
> fought this before (and probably will again).

Trying to get back on track here...

So the original "bug" was that from the user's point of view, it  
looks like there ought to be a 1 to 1 mapping between the dates 'svn  
log' displays and the dates one can specify with -r{DATE}.  But in  
fact, there isn't a direct mapping, due to rounding to the nearest  
second.  If 'svn log' says that rN happened at time "2005-08-23  
00:14:17", when the user types -r{"2005-08-23 00:14:17"}, he gets r 
(N-1) back, because it turns out that the commit-time is actually  
recorded as 00:14:17.23983.

Are you guys suggesting, therefore, that
(Continue reading)

Chris Leishman | 1 Sep 03:30 2005
Picon

Relative externals (was: Final(?) Relative Externals format)

Hi all,

I was wondering if something had been decided on this issue and, if
so, what will be implemented and when?

cheers,
chris

On Wed, 26 Jan 2005 14:19:53 -0500, Michael Sweet wrote:
<snip>
> I know I don't have a vote, but "repos" would seem to be unambiguous,
> giving us the following acceptable forms:
>
>    file:/path                    (absolute)
>    file:///path                  (absolute)
>    http://host/path              (absolute)
>    https://host/path             (absolute)
>    repos://host/path             (relative to current scheme)
>    repos:///path                 (relative to current scheme and host)
>    repos:path                    (relative URI to current)
>    repos:../path                 (relative URI to current)
>    svn://host/path               (absolute)
>    svn+ssh://host/path           (absolute)
<snip>
Justin Erenkrantz | 1 Sep 04:24 2005

Re: "svn diff" and "svn log" timestamp weirdness

On Wed, Aug 31, 2005 at 08:58:33PM -0500, Ben Collins-Sussman wrote:
> So the original "bug" was that from the user's point of view, it  
> looks like there ought to be a 1 to 1 mapping between the dates 'svn  
> log' displays and the dates one can specify with -r{DATE}.  But in  
> fact, there isn't a direct mapping, due to rounding to the nearest  
> second.  If 'svn log' says that rN happened at time "2005-08-23  
> 00:14:17", when the user types -r{"2005-08-23 00:14:17"}, he gets r 
> (N-1) back, because it turns out that the commit-time is actually  
> recorded as 00:14:17.23983.

For 'svn up -r{DATE}'?

> Are you guys suggesting, therefore, that
> 
>       -r{"2005-08-23 00:14:17"}
> 
> ...actually be interpreted as if the user had typed
> 
>       -r{"2005-08-23 00:14:17.00"}:{"2005-08-23 00:14:18.00"}
> 
> ?

Yes.  (Assuming that it treats 14:18.00 as < not <=.)

> The result might be a range of revisions.  This is all fine and good  
> for 'svn log', but a lot of commands don't take revision ranges.   
> What do we do then?

The only case I can come up with is co/up that would fit that scenario
where we have to pick a single revision.
(Continue reading)

Jean-Marc Godbout | 1 Sep 07:19 2005
Picon

Re: [PATCH] Fix for issue 2151 "'svn ls' is slow over ra_dav"

Woo, thanks for the comments. My notes below. As well I have attached my revised patch.

[[[
Fix for issue 2151 "'svn ls' is slow over ra_dav"

This patch implements a solution to issue 2151.
We now only request the needed props in the PROPFIND
for server listings. 'svn ls' is now noticably faster.
In most cases 'ls' takes about half the time and half
the bandwidth - In some case even better results.

For backward compatibility, before doing the long PROPFIND
we make another simpler PROPFIND to see if the server
supports the new type of request (supports the
deadprop-count prop). If it does we use the new scheme
and performance is improved - If not then we use the old
scheme and the slowness persists.

To summarise: only patched servers and patched clients
have improved speed. Mismatched configurations (old client
or server) are not improved but still work. Regular dav
clients are still slow.

Patch by: Jean-Marc Godbout <jean-marc <at> computrad.com>

Suggested by: Erik Scrafford <erik <at> scrafford.org>
              Ben Collins-Sussman <sussman <at> collab.net>
              Everyone else who contributed to issue 2151
             

* subversion/mod_dav_svn/liveprops.c
 (SVN_PROPID_deadprop_count): Added a PROPID to support the new prop
 (dav_svn_props[]): Added the deadprop-count prop to the list of
  supported props.
 (dav_svn_insert_prop): Returns the number of deadprops
  found in the fs for that file. 

* subversion/libsvn_ra_dav/ra_dav.h
  (SVN_RA_DAV__PROP_DEADPROP_COUNT): Added the prop name for deadprop
  (ELEM_deadprop_count): Added the prop to the list of props

 * subversion/libsvn_ra_dav/props.c
  (elem_definition, propfind_elements): Added ELEM_deadprop_count
   to the list of propfind props
 
 * subversion/libsvn_ra_dav/fetch.c
  (dirent_props[]): New array, list of props to get in a dirent PROPFIND
  (deadprop_count_support_props[

]): New array, list of props to
   get in a root PROPFIND
  (svn_ra_dav__get_dir):  check if server supports 'deadprop-count',
   and if so, request fewer properties
  
]]]

- Show quoted text -
On 8/30/05, Ben Collins-Sussman <sussman <at> collab.net> wrote:
Thanks, this patch looks pretty good!

This is a review from me and kfogel.

The review includes both formatting problems, minor nits, and also
some substantive comments on the changes.  We look forward to the next
iteration of the patch.  :-)


> [[[
> Fix for issue 2151 "'svn ls' is slow over ra_dav"
>
> This patch implements a solution to issue 2151. We now only request
> the needed props in the PROPFIND for server listings. 'svn ls' is now
> noticably faster. In most cases 'ls' takes about half the time and
> half the bandwidth - In some case even better results.
>
> For backward compatibility, before doing the long PROPFIND we make
> another simpler PROPFIND to see if the server supports the new
type of
> request (supports the deadprop-count prop). If it does we use the new
> scheme and performance is improved - If not then we use the old
scheme
> and the slowness persists.
>

Here's another strategy that we've been pondering:

We see that the new 'deadprop-count' property is already in your short
list of properties to fetch.  Instead of unconditionally doing an
extra PROPFIND for the new 'deadprop-count' property, why not just
*always* request the short list in the depth-1 PROPFIND?  If the new
property doesn't come back, then repeat the depth-1 PROPFIND, asking
for <allprops>.

The problem with this is that 'svn ls' will now be about twice as slow
against *old* servers -- doing two depth-1 PROPFINDs instead of one.
So we wouldn't just be rewarding those who upgrade servers with 2x
speed, we'd be actively *punishing* people who don't upgrade their
servers with 1/2 speed.  This may cause riots.

So we're thinking that maybe we should stick with your strategy for
now, but say, two releases from now, go with our strategy.  By then,
most servers will have upgraded, and it'll be silly for clients to be
wasting time on an extra unconditional turnaround.

Does that sound like a good idea?


Yes. There is another strategy which is to add the state of the server to the session. That way you would only do the propfind if you are unsure if the server supports the deadprop-count prop because you can't determine it from other clues (result of an OPTIONS request for example). This could be transferred to issue 1161 (excess propfinds). I will add this to the bottom of the issue's discussion once I get home (I'm currently on the road).

1. Add the supports_deadprop_count variable to the session. If 2 "svn ls" are done in the same session, then the second time around you wouldn't need to do that discovery propfind. This also simplifies #2.

2. Make an OPTIONS request to get the server options, then set the variable in the session. "ls" would then always know and wouldn't need to do the propfind.

The additional OPTIONS would be a round trip that would replace the propfind. It's currently not better than the propfind (it's still a round trip) but if we also use it for other things, then we could roll in the deadprop-count discovery in that.

This is another patch though I think. Another note about your solution is that this extra propfind is not that expensive - we already have 7-8 comparable propfinds, this is simply one more.. and it could be removed on implementation of the OPTIONS solution eventually. But I agree that the ultimate goal would be to have fully patched clients and servers be as efficient as possible, but I'm thinking that in this case the drawback of unpatched servers would be very dramatic!

Another change that would require an upgrade would be to make the server be more DeltaV compliant by simply not sending any DeltaV props on an allprop request (as described in issue #2151). This would mean that all older clients couldn't do "ls"s against patched servers... but it would make other dav clients be much faster withe the subversion server.

> To summarise: only patched servers and patched clients have improved
> speed. Mismatched configurations (old client or server) are not
> improved but still work. Regular dav
> clients are still slow.
>
> Patch by: Jean-Marc Godbout < jean-marc <at> computrad.com>
>
> Suggested by: Erik Scrafford < erik <at> scrafford.org>
>                       Ben Collins-Sussman <sussman <at> collab.net>
>                       Everyone else who contributed to issue 2151
>
>
> * subversion/mod_dav_svn/liveprops.c
>  (SVN_PROPID_deadprop_count): Added a PROPID to support the new prop
>  (dav_svn_props[]): Added the deadprop-count prop to the list of
>   supported props.
>  (case SVN_PROPID_deadprop_count): Returns the number of deadprops
>   found in the fs for that file.
>


The name of the affected function goes in the parentheses:  the 'case'
shouldn't be in there, but rather "(dav_svn_insert_prop)".


Fixed

> * subversion/libsvn_ra_dav/ra_dav.h
>   (SVN_RA_DAV__PROP_DEADPROP_COUNT): Added the prop name for deadprop
>   (ELEM_deadprop_count): Added the prop to the list of props
>
>  * subversion/libsvn_ra_dav/props.c
>   (elem_definition, propfind_elements): Added ELEM_deadprop_count
>    to the list of propfind props
>
>  * subversion/libsvn_ra_dav/fetch.c
>   (dirent_props[]): list of props to get in a dirent PROPFIND
>   (root_props[]): list of props to get in a root PROPFIND


Just a nit: when we add a new top-level symbol to the codebase, our
general convention is to indicate that it's new, e.g.

   (dirent_props[]): New array, a list of props to get in a dirent
PROPFIND.
   (root_props[]): New array, a list of props to get in a root PROPFIND.

Fixed

>   (supports_deadprop_count): indicates if the server supports
>    the deadprop_count prop
>   (prop_count): the number of user props for the current dirent
>    as returned by the server
> ]]]


Non-top-level symbols don't get listed in parens.  Instead, mention
the function in which these variables were added, and describe the
overall change to the function, e.g.

     (svn_ra_dav__get_dir):  check if server supports 'deadprop-count',
       and if so, request fewer properties.

Done

- Show quoted text -
>
>
>
> Index: subversion/mod_dav_svn/liveprops.c
> ===================================================================
> --- subversion/mod_dav_svn/liveprops.c    (revision 15946)
> +++ subversion/mod_dav_svn/liveprops.c    (working copy)
> <at> <at> -67,7 +67,8 <at> <at>
>  enum {
>    SVN_PROPID_baseline_relative_path = 1,
>    SVN_PROPID_md5_checksum,
> -  SVN_PROPID_repository_uuid
> +  SVN_PROPID_repository_uuid,
> +  SVN_PROPID_deadprop_count
>  };
>
>  static const dav_liveprop_spec dav_svn_props[] =
> <at> <at> -96,6 +97,7 <at> <at>
>    SVN_RO_SVN_PROP(baseline_relative_path, baseline-relative-path),
>    SVN_RO_SVN_PROP(md5_checksum, md5-checksum),
>    SVN_RO_SVN_PROP(repository_uuid, repository-uuid),
> +  SVN_RO_SVN_PROP(deadprop_count, deadprop-count),
>
>    { 0 } /* sentinel */
>  };
> <at> <at> -563,6 +565,30 <at> <at>
>          }
>        break;
>
> +    case SVN_PROPID_deadprop_count:
> +      {
> +        if (resource->type != DAV_RESOURCE_TYPE_REGULAR)
> +            return DAV_PROP_INSERT_NOTSUPP;
> +
> +        unsigned int propcount;
> +
> +        apr_hash_t *proplist;


We write to C89 standards (not C99), which means you can't declare
variables right in the middle of a block like that.  It needs to be
declared at the beginning of a block.

Fixed. Also, is there a way to have the build fail if the code isn't C89 compliant?
 

> +        serr = svn_fs_node_proplist(&proplist,
> +                                    resource->info->root.root,
> +                                    resource->info->repos_path, p);
> +        if (serr != NULL)
> +          {
> +            /* ### what to do? */
> +            svn_error_clear(serr);
> +            value = "###error###";
> +            break;
> +          }
> +
> +        propcount = apr_hash_count(proplist);
> +        value = apr_psprintf(p, "%u", propcount);
> +        break;
> +      }
> +
>      default:
>        /* ### what the heck was this property? */
>        return DAV_PROP_INSERT_NOTDEF;
> Index: subversion/libsvn_ra_dav/ra_dav.h
> ===================================================================
> --- subversion/libsvn_ra_dav/ra_dav.h    (revision 15946)
> +++ subversion/libsvn_ra_dav/ra_dav.h    (working copy)
> <at> <at> -368,6 +368,7 <at> <at>
>
>  #define SVN_RA_DAV__PROP_REPOSITORY_UUID SVN_DAV_PROP_NS_DAV
"repository-uuid"
>
> +#define SVN_RA_DAV__PROP_DEADPROP_COUNT "deadprop-count"


Our liveprops all live in the SVN_DAV_PROP_NS_DAV namespace;  see the
repository-uuid definition as an example.  Just add that constant to
your declaration.


Done

>
>  typedef struct {
>    /* what is the URL for this resource */
> <at> <at> -707,7 +708,8 <at> <at>
>    ELEM_lock_owner,
>    ELEM_lock_comment,
>    ELEM_lock_creationdate,
> -  ELEM_lock_expirationdate
> +  ELEM_lock_expirationdate,
> +  ELEM_deadprop_count,
>  };


In C89, you can't put a comma after the last element in a list.  If
only this were python.  :-)

Done,
- Show quoted text -

>
>  /* ### docco */
> Index: subversion/libsvn_ra_dav/props.c
> ===================================================================
> --- subversion/libsvn_ra_dav/props.c    (revision 15946)
> +++ subversion/libsvn_ra_dav/props.c    (working copy)
> <at> <at> -111,6 +111,7 <at> <at>
>    { ELEM_baseline_relpath, SVN_RA_DAV__PROP_BASELINE_RELPATH, 1 },
>    { ELEM_md5_checksum, SVN_RA_DAV__PROP_MD5_CHECKSUM, 1 },
>    { ELEM_repository_uuid, SVN_RA_DAV__PROP_REPOSITORY_UUID, 1 },
> +  { ELEM_deadprop_count, SVN_RA_DAV__PROP_DEADPROP_COUNT, 1 },
>    { 0 }
>  };
>
> <at> <at> -147,6 +148,8 <at> <at>
>      SVN_RA_DAV__XML_CDATA },
>    { SVN_DAV_PROP_NS_DAV, "repository-uuid", ELEM_repository_uuid,
>      SVN_RA_DAV__XML_CDATA },
> +  { SVN_DAV_PROP_NS_DAV, "deadprop-count", ELEM_deadprop_count,
> +    SVN_RA_DAV__XML_CDATA },
>
>    /* Unknowns */
>    { "", "", ELEM_unknown, SVN_RA_DAV__XML_COLLECT },
> Index: subversion/libsvn_ra_dav/fetch.c
> ===================================================================
> --- subversion/libsvn_ra_dav/fetch.c    (revision 15946)
> +++ subversion/libsvn_ra_dav/fetch.c    (working copy)
> <at> <at> -914,7 +914,25 <at> <at>
>    return SVN_NO_ERROR;
>  }
>
> +/* the properties we need to fill in an svn_dirent_t, used by
> +   svn_ra_get_dir(). */
> +static const ne_propname dirent_props[] =
> +{
> +  { "DAV:", "resourcetype" },         /* kind */
> +  { "DAV:", "getcontentlength" },     /* size */
> +  { "DAV:", "version-name" },         /* created_rev */
> +  { "DAV:", "creationdate" },         /* time */
> +  { "DAV:", "creator-displayname" },  /* last_author */
> +  { SVN_DAV_PROP_NS_DAV, "deadprop-count" },       /* has_props */
> +  { NULL }
> +};


Just for readability, can you make the comments line up?

Yes sir.

>
> +static const ne_propname root_props[] =
> +{
> +  { SVN_DAV_PROP_NS_DAV, "deadprop-count" },
> +  { NULL }
> +};
> +


Can we have a different name for this array?  We're not sure what
'root' has to do with anything...

Also, can you add a docstring before the array, just like
dirent_props[] has?

(Also, assuming you agree with our long-term strategy, this array will
eventually go away, since we won't be doing the extra PROPFIND.)

Good point, I'm sorry about that unfortunate name.. for a while it made sense in my head. Renamed to  deadprop_count_support_props. More descriptive.

Also the array will disappear, yes - whichever strategy is used.

>  svn_error_t *svn_ra_dav__get_dir(svn_ra_session_t *session,
>                                   const char *path,
>                                   svn_revnum_t revision,
> <at> <at> -955,13 +973,46 <at> <at>
>          *fetched_rev = got_rev;
>      }
>
> +  /* For issue 2151:
> +     See if we are dealing with a server that understand the
> +     deadprop-count prop. If it doesn't we'll need to do an
> +     allprop PROPFIND - If it does, we'll execute a more
> +     targetted PROPFIND.
> +  */
> +  svn_boolean_t supports_deadprop_count;
> +


Again, new variables need to be declared at the beginning of a block.


Done

> +  {
> +    const svn_string_t *deadprop_count;
> +
> +    SVN_ERR( svn_ra_dav__get_props_resource(&rsrc, ras->sess,
> +                                            final_url, NULL,
root_props,
> +                                            pool) );
> +
> +    deadprop_count = apr_hash_get(rsrc->propset, "deadprop-
count", APR_HASH_KEY_STRING);


Oops, this line is more than 80 chars.   Please fix.


Done

> +
> +    if (deadprop_count == NULL)
> +      supports_deadprop_count = FALSE;
> +    else
> +      supports_deadprop_count = TRUE;
> +  }
> +


Someday this whole block will go away, when we stop doing the extra
PROPFIND.

Yes.
- Show quoted text -

>    if (dirents)
>      {
>        /* Just like Nautilus, Cadaver, or any other browser, we do a
>           PROPFIND on the directory of depth 1. */
> -      SVN_ERR( svn_ra_dav__get_props(&resources, ras->sess,
> -                                     final_url, NE_DEPTH_ONE,
> -                                     NULL, NULL /* all props */,
pool) );
> +      if (supports_deadprop_count == TRUE)
> +        {
> +          SVN_ERR( svn_ra_dav__get_props(&resources, ras->sess,
> +                                         final_url, NE_DEPTH_ONE,
> +                                         NULL, dirent_props,
pool) );
> +        }
> +      else /* The server doesn't support the deadprop_count prop,
fallback */
> +        {
> +          SVN_ERR( svn_ra_dav__get_props(&resources, ras->sess,
> +                                         final_url, NE_DEPTH_ONE,
> +                                         NULL, NULL /* allprops */,
> +                                         pool) );
> +        }
>
>        /* Count the number of path components in final_url. */
>        final_url_n_components = svn_path_component_count(final_url);
> <at> <at> -981,6 +1032,7 <at> <at>
>            const svn_string_t *propval;
>            apr_hash_index_t *h;
>            svn_dirent_t *entry;
> +          apr_int64_t prop_count;
>
>            apr_hash_this (hi, &key, NULL, &val);
>            childname =  key;
> <at> <at> -1009,20 +1061,41 <at> <at>
>
>            /* does this resource contain any 'svn' or 'custom'
properties,
>               i.e.  ones actually created and set by the user? */
> -          for (h = apr_hash_first (pool, resource->propset);
> -               h; h = apr_hash_next (h))
> +          if (supports_deadprop_count == TRUE)
>              {
> -              const void *kkey;
> -              void *vval;
> -              apr_hash_this (h, &kkey, NULL, &vval);
> +              propval = apr_hash_get(resource->propset,
> +
SVN_RA_DAV__PROP_DEADPROP_COUNT,
> +                                     APR_HASH_KEY_STRING);
> +
> +              if (propval == NULL)
> +                entry->has_props = FALSE;


If (supports_deadprop_count == TRUE) and there's no property
value... that's an error, isn't it?  It means the server has sent
bogus data, no?  Shouldn't we treat this as an error condition, rather
than just setting has_props = FALSE?

My reasoning behind that is that if you were listing something NOT DAV_RESOURCE_TYPE_REGULAR, then the server wouldn't have a deadprop-count for that resource. This would mean that has_props should be false. We don't list anything that's not DAV_RESOURCE_TYPE_REGULAR though, so this is moot. Changed. I was debating between SVN_ERR_INCOMPLETE_DATA and SVN_ERR_PROPERTY_NOT_FOUND. I settled on the former as from the point of vue of the user who would recieve this error it provides a better picture of what actually happenned (users don't care about properties in this case - they care about a server sending invalid data). I don't feel too strongly about this and you might have a better idea.

> +              else
> +                {
> +                  prop_count = svn__atoui64(propval->data);
>
> -              if (strncmp((const char *)kkey,
SVN_DAV_PROP_NS_CUSTOM,
> -                          sizeof(SVN_DAV_PROP_NS_CUSTOM) - 1) == 0)
> -                entry->has_props = TRUE;
> -
> -              else if (strncmp((const char *)kkey,
SVN_DAV_PROP_NS_SVN,
> -                               sizeof(SVN_DAV_PROP_NS_SVN) - 1)
== 0)
> -                entry->has_props = TRUE;
> +                  if (prop_count == 0)
> +                    entry->has_props = FALSE;
> +                  else
> +                    entry->has_props = TRUE;
> +                }
> +             }
> +          else /* The server doesn't support the deadprop_count
prop, fallback */
> +            {
> +          for (h = apr_hash_first (pool, resource->propset);


Whoops!  You accidentally inserted a tab character in the line above.
Make sure your editor isn't doing that.  We use only spaces in our
source code.

Indeed I had changed my editor mid way through my changes. I tried to remove all tabs. I missed that one evidently. Fixed.


 

> +                h; h = apr_hash_next (h))
> +                {
> +                  const void *kkey;
> +                  void *vval;
> +                  apr_hash_this (h, &kkey, NULL, &vval);
> +

We know this isn't your code here, but we just noticed that 'vval' is
never used.  Can you clean it up by just removing that variable and
passing NULL as the last argument to apr_hash_this() ?

Done.

> +                  if (strncmp((const char *)kkey,
SVN_DAV_PROP_NS_CUSTOM,
> +                              sizeof(SVN_DAV_PROP_NS_CUSTOM) - 1)
== 0)
> +                    entry->has_props = TRUE;
> +
> +                  else if (strncmp((const char *)kkey,
SVN_DAV_PROP_NS_SVN,
> +                                   sizeof(SVN_DAV_PROP_NS_SVN) -
1) == 0)
> +                    entry->has_props = TRUE;
> +                }
>              }
>
>            /* created_rev & friends */
Attachment (patch.2151.2): application/octet-stream, 12 KiB
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe <at> subversion.tigris.org
For additional commands, e-mail: dev-help <at> subversion.tigris.org
Greg Hudson | 1 Sep 08:12 2005
Picon

Re: "svn diff" and "svn log" timestamp weirdness

On Wed, 2005-08-31 at 19:24 -0700, Justin Erenkrantz wrote:
> The only case I can come up with is co/up that would fit that scenario
> where we have to pick a single revision.

blame, cat, copy, export, info, ...

brane is proposing a pretty deep architectural change here.

Gmane