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.
> @@ -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. */
?
> @@ -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...
> @@ -1228,12 +1240,14 @@
> rrep = (SVN_IS_VALID_REVNUM(rev)) ?
> apr_psprintf(pool, "+%ld:", rev) : "-";
>
> - if (depth == svn_depth_empty)
> + if (depth == svn_depth_exclude)
> drep = "+0:";
> - else if (depth == svn_depth_files)
> + else if (depth == svn_depth_empty)
> drep = "+1:";
> - else if (depth == svn_depth_immediates)
> + else if (depth == svn_depth_files)
> drep = "+2:";
> + else if (depth == svn_depth_immediates)
> + drep = "+3:";
> else if (depth == svn_depth_infinity)
> drep = "-";
> else
This is more code that would change with the "X" thing.
> --- trunk/subversion/tests/libsvn_repos/repos-test.c (original)
> +++ trunk/subversion/tests/libsvn_repos/repos-test.c Sun Nov 25 23:46:56 2007
> @@ -1988,6 +1988,153 @@
> }
>
> [...]
> +
> + /* Run an update from r1 to r2, excluding iota and everything under
> + A/D. Record the editor commands in a temporary txn. */
The way to make the reporter go haywire might be to call set_path3()
with svn_depth_exclude on "A/D", and then call it (with some
non-exclude depth) on, say, "A/D/G" or "A/D/G/pi". What should the
reporter's behavior be then? IMHO, either error or skip is okay.
What would not be okay would be including the second path in the
subsequent update response.
What the heck, let's try it:
$ subversion/tests/libsvn_repos/repos-test 11
PASS: lt-repos-test 11: test reporter and svn_depth_exclude
$ $EDITOR subversion/tests/libsvn_repos/repos-test.c
$ svn diff
Index: subversion/tests/libsvn_repos/repos-test.c
===================================================================
--- subversion/tests/libsvn_repos/repos-test.c (revision 28091)
+++ subversion/tests/libsvn_repos/repos-test.c (working copy)
@@ -2091,6 +2091,9 @@
SVN_ERR(svn_repos_set_path3(report_baton, "A/D", SVN_INVALID_REVNUM,
svn_depth_exclude,
FALSE, NULL, subpool));
+ SVN_ERR(svn_repos_set_path3(report_baton, "A/D/G/pi", SVN_INVALID_REVNUM,
+ svn_depth_infinity,
+ FALSE, NULL, subpool));
SVN_ERR(svn_repos_finish_report(report_baton, subpool));
/* Confirm the contents of the txn. */
$ subversion/tests/libsvn_repos/repos-test 11
subversion/libsvn_repos/reporter.c:756: (apr_err=160013)
svn_tests: Working copy path 'A/D' does not exist in repository
FAIL: lt-repos-test 11: test reporter and svn_depth_exclude
$
Okay, that's good. I'll make a change to deliberately test that we
get that error, in fact (fancier than the above hack, I mean). Watch
for it in a moment.
-Karl
---------------------------------------------------------------------
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:01:15 2007