On Nov 27, 2007 4:01 PM, Karl Fogel <kfogel@red-bean.com> wrote:
> glasser@tigris.org writes:
> > --- trunk/subversion/libsvn_repos/reporter.c (original)
> > +++ trunk/subversion/libsvn_repos/reporter.c Sun Nov 25 23:46:56 2007
> > @@ -51,7 +51,8 @@
> > <revnum>: Revnum of set_path or link_path
> > +/- '+' indicates depth other than svn_depth_infinity
> > If previous is +:
> > - <depth>: "0","1","2" => svn_depth_{empty,files,immediates}
> > + <depth>: "0","1","2","3" =>
> > + svn_depth_{exclude,empty,files,immediates}
> > +/- '+' indicates start_empty field set
> > +/- '+' indicates presence of lock_token field.
> > If previous is +:
>
> Before this change, the digits matched the actual values of the depth
> enums. Now they are shifted off by one. How about we use "X" for
> exclude, and "0","1","2" as before for the others?
>
> I can make the change, if you're in favor. It would involve tweaking
> read_number() or augmenting it with another funtcoin, but it's
> certainly doable.
Please do. read_depth would be easy to write.
> > @@ -213,18 +214,21 @@
> > switch (num)
> > {
> > case 0:
> > - (*pi)->depth = svn_depth_empty;
> > + (*pi)->depth = svn_depth_exclude;
> > break;
> > case 1:
> > - (*pi)->depth = svn_depth_files;
> > + (*pi)->depth = svn_depth_empty;
> > break;
> > case 2:
> > + (*pi)->depth = svn_depth_files;
> > + break;
> > + case 3:
> > (*pi)->depth = svn_depth_immediates;
> > break;
> >
> > /* Note that we do not tolerate explicit representation of
> > - svn_depth_infinity as "3" here, because that's the
> > - default and should never be sent. */
> > + svn_depth_infinity as "4" here, because that's just not
> > + how write_path_info writes it. */
> > default:
> > return svn_error_createf(SVN_ERR_REPOS_BAD_REVISION_REPORT, NULL,
> > _("Invalid depth (%s) for path '%s'"),
>
> Would change above comment, of course.
>
> > @@ -948,7 +952,10 @@
> > if (!name)
> > break;
> >
> > - if (info && !SVN_IS_VALID_REVNUM(info->rev))
> > + /* Invalid revnum means either delete or excluded subpath. */
> > + if (info
> > + && !SVN_IS_VALID_REVNUM(info->rev)
> > + && info->depth != svn_depth_exclude)
> > {
> > /* We want to perform deletes before non-replacement adds,
> > for graceful handling of case-only renames on
>
> That comment isn't quite right, I think. How about:
>
> /* Invalid revnum means we should delete, unless this is just an
> excluded subpath. */
>
> ?
Sure.
> > @@ -967,13 +974,18 @@
> > s_entry = s_entries ?
> > apr_hash_get(s_entries, name, APR_HASH_KEY_STRING) : NULL;
> >
> > - /* The only special case here is when requested_depth is files
> > - but the reported path is a directory. This is technically
> > - a client error, but we handle it anyway, by skipping the
> > - entry. */
> > - if (requested_depth != svn_depth_files
> > - || ((! t_entry || t_entry->kind != svn_node_dir)
> > - && (! s_entry || s_entry->kind != svn_node_dir)))
> > + /* The only special cases here are
> > +
> > + - When requested_depth is files but the reported path is
> > + a directory. This is technically a client error, but we
> > + handle it anyway, by skipping the entry.
> > +
> > + - When the reported depth is svn_depth_exclude.
> > + */
> > + if ((! info || info->depth != svn_depth_exclude)
> > + && (requested_depth != svn_depth_files
> > + || ((! t_entry || t_entry->kind != svn_node_dir)
> > + && (! s_entry || s_entry->kind != svn_node_dir))))
> > SVN_ERR(update_entry(b, s_rev, s_fullpath, s_entry, t_fullpath,
> > t_entry, dir_baton, e_fullpath, info,
> > info ? info->depth
>
> I pondered this for a long time back when I was in Chicago (when you
> posted a preliminary patch for review), and I've pondered it for a
> long time now. I *think* it's right. Sorry for not replying earlier;
> I wasn't sure then and didn't want to mislead.
>
> The question isn't whether the conditional is correct: obviously, it
> is. The question is whether we should call skip_path_info(), as
> update_entry() would have done. Of course, if we should, then the old
> skip should have too, but I think the old skip never actually happened
> in practice. So yes, this question does predate your change.
>
> I suspect it is possible to manually construct a report that would
> cause this code to haywire (that is, to descend into subtrees the
> client doesn't need to hear about), though I don't know of any actual
> security problems that might result. I'm not sure this is worth doing
> anything about, but it would be nice (if we had time, sigh) to test
> it. Well, see below...
Yes, we should document on both RA and repos set_path APIs that you
may not descend below an excluded path. (Can you? Deep in sqlite-land.)
--dave
--
David Glasser | glasser_at_davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Nov 28 01:39:54 2007