Daniel Shahaf <d.s_at_daniel.shahaf.name> writes:
> Ping!
Thanks for asking. I never figured out a quick solution for this. I
ended up fixing issue #3282 in a different (and better) way, so it's not
so pressing now. I've filed new issue #3293; it's probably solveable
via some protocol changes, but I'm not sure it should be a high priority
right now.
-Karl
> 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-07 20:31:13 CEST