You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2008/10/05 11:32:08 UTC
Re: Using ra->set_path() over svn:// disallows SVN_INVALID_REVNUM?
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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Using ra->set_path() over svn:// disallows SVN_INVALID_REVNUM?
Posted by Karl Fogel <kf...@red-bean.com>.
Daniel Shahaf <d....@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org