[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: Updated [patch] new part 1 for log command

From: Greg Stein <gstein_at_lyra.org>
Date: 2002-03-01 02:00:09 CET

On Thu, Feb 28, 2002 at 07:15:20PM -0500, Ben Collins wrote:
> On Thu, Feb 28, 2002 at 06:40:30PM -0500, Daniel Berlin wrote:
> >
> > On Thursday, February 28, 2002, at 06:10 PM, Ben Collins wrote:
> >
> > >This is an updated patch. Basically it expands log usage to allow for
> > >URL's and the -D option (which works with ra_local, but not ra_dav yet).
> > >Updated man page aswell.
> > >
> > > * libsvn_fs/tree.c(svn_fs_revisions_changed): Change to take an
> > > array of stringbuf's for paths instead of one path. It will then
> > > iterate to get a list of all revs for all paths (reduces amount
> > > of trails for getting info on multiple paths).
> > While you are in here, is there any reason to use stringbufs here (IE
> > could it be an array of const char *)
> > Do we ever modify the results?
>
> Can't do it. Some things do modify the paths. For instance, when taking
> off a component to get basename/dir, or adding components, or similar.

The FS API does not use stringbufs anywhere, except for 'svn_fs_unparse_id'.
I think it would be best if svn_fs_revisions_changed() took an array of
'const char *'. Especially given that all other repository paths are
modelled as that type. If you look at the code you added to
revisions_changed, you'll also note that you only use buf->data.

I would also point out that the svn_repos_get_logs() is not going to be
modifying the parameters, so those should (technically) be 'const char *'
also. However, that means some pushback against the callers which you
probably don't want to deal with at this stage.

Now, the practical result of making revisions_changed take 'const char *' is
that you'll need to map the get_logs' incoming array into a new array.

I'll review your patch now, but have these two points:

* the stringbuf in the FS API is effectively a showstopper
* you can simplify the apr_array_header_t access by using APR_ARRAY_IDX()
  from the svn_types.h header.

More in a bit...

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Oct 21 14:37:10 2006

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.