Ping!
Daniel
Karl Fogel wrote on Fri, 12 Sep 2008 at 14:47 -0400:
> (Issue #3282 is some kind of cosmic can-opener... The more I turn the
> handle, the more other bugs get revealed.)
>
> The documentation for svn_ra_reporter3_t.svn_ra_set_path() in
> include/svn_ra.h claims that the 'revision' parameter can be
> SVN_INVALID_REVNUM:
>
> /**
> * [...]
> * @a revision may be SVN_INVALID_REVNUM if (for example) @a path
> * represents a locally-added path with no revision number, or @a
> * depth is @c svn_depth_exclude.
> * [...]
> */
> svn_error_t *(*set_path)(void *report_baton,
> const char *path,
> svn_revnum_t revision,
> svn_depth_t depth,
> svn_boolean_t start_empty,
> const char *lock_token,
> apr_pool_t *pool);
>
> But watch what happens if you actually try it over svn://...
>
> I have a patch to libsvn_wc/adm_crawler.c that causes some code to
> look like this:
>
> [...]
> /* ... or report a differing revision or lock token, or the
> mere presence of the file in a depth-empty dir, or a
> replaced-without-history file. */
> else if (current_entry->revision != dir_rev
> || current_entry->lock_token
> || dot_entry->depth == svn_depth_empty
> || (current_entry->schedule == svn_wc_schedule_replace
> && current_entry->copyfrom_url == NULL))
> SVN_ERR(reporter->set_path(report_baton,
> this_path,
> effective_rev,
> current_entry->depth,
> FALSE,
> current_entry->lock_token,
> iterpool));
> [...]
>
> The new 'effective_rev' argument is sometimes SVN_INVALID_REVNUM
> (i.e., -1), to indicate that what we have locally is a
> replaced-without-history file. The above code leads directly to this
> in libsvn_client/status.c:reporter_set_path():
>
> return rb->wrapped_reporter->set_path(rb->wrapped_report_baton, path,
> revision, depth, start_empty,
> lock_token, pool);
>
> Which leads to this in libsvn_ra_svn/client.c:ra_svn_set_path():
>
> SVN_ERR(svn_ra_svn_write_cmd(b->conn, pool, "set-path", "crb(?c)w",
> path, rev, start_empty, lock_token,
> svn_depth_to_word(depth)));
>
> Which leads to this code in svn_ra_svn_write_cmd():
>
> va_start(ap, fmt);
> err = vwrite_tuple(conn, pool, fmt, ap);
> va_end(ap);
>
> Which leads to this in vwrite_tuple():
>
> else if (*fmt == 'r')
> {
> rev = va_arg(ap, svn_revnum_t);
> SVN_ERR_ASSERT(opt || SVN_IS_VALID_REVNUM(rev));
> if (SVN_IS_VALID_REVNUM(rev))
> SVN_ERR(svn_ra_svn_write_number(conn, pool, rev));
> }
>
> And there's our problem: 'rev' is obviously -1, and 'opt' is 0,
> because the controlling fmt string here is "crb(?c)w", in which the
> "r" is not 'opt'ional.
>
> I can't just change the "r" to "(?r)", because old servers won't
> expect the "r" to be optional inside its own tuple -- which is the way
> SVN_INVALID_REVNUM should be represented, if I understand the
> documentation to svn_ra_svn_write_tuple() correctly.
>
> So there's a mismatch between how we intended ra->set_path() to work,
> and how svnserve actually expects it to work. We intended to be able
> to represent SVN_INVALID_REVNUM; in practice, I think we can't.
>
> Are there any brilliant, compatible solutions to this that I'm missing?
>
> In the meantime, I think I may be able to solve my issue #3282 problem
> by sending the base revision of the replaced file (that is, the
> shadowed file), since we still have its .svn-revert bases sitting
> around and can use them if the server sends a delta against the old
> text base.
>
> -Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-10-05 17:50:05 CEST