Re: ra_serf bug(s)

Lieven Govaerts wrote:
> On Fri, Oct 10, Julian Foad wrote:
>> Lieven Govaerts wrote:
>>> IMO googlecode is doing the wrong thing here, there's nothing special
>>> about r0 (it's empty, but still a valid revision).
>>
>> Hi Lieven. I disagree with that point. There are two different meanings of
>> "revision": a snapshot, and a change between one snapshot and the next. r0
>> is a valid snapshot, but it is not a valid *change*. A "replay" request
>> sounds to me like a request to tell us what changed between snapshot (X - 1)
>> and snapshot X. (Although it would be *possible* to define that a replay
>> request for r0 should respond that the difference between revision (-1) and
>> revision (0) is a valid, empty difference, in my opinion it is not logical
>> to do so.)
>
> There's another way of looking at things: r0 is the change that brings
> the repository in its initial state. It's automatically created by
> 'svnadmin create', but it exists as change,

Hi, Lieven.

We need to be more precise. From the point of view of the *whole system*, that's true, it's a change; but we're not talking about the whole system. We're talking about replaying changes of the versioned data, using the "replay" API. The original report was about the HTTP 'replay-report', which in mod_dav_svn is implemented using svn_repos_replay2(), so let's talk about that.

svn_repos_replay2 deals with changes from one versioned state to another, using svn_delta_editor_t to represent the changes. In order to describe a change using this editor, the API requires to start with an "open_root" call that says "open an existing directory so that we can make changes in it". It *cannot* represent whole-system changes such as "create a repository" or even "create the initial snapshot in a repository that doesn't yet have one".

> it can have revprops
> (which can relate to the snapshot r0 and to the change r0).

The snapshot r0 can have rev-props. The creation of these rev-props can also be considered as part of a change of state of the whole system from no repository, or from a repository containing no revision snapshots, to a repository containing r0. Again, not as a change against a previous revision because there is no previous revision (snapshot) in this repository.

(The svn_ra_replay_range() API provides the rev-props for a given revision in full rather than as changes against a previous revision, so it *can* (and does) describe the rev-props for r0.)

> I say that the fact that that initial state is empty is an
> implementation detail, and thus (theoretically) can change in the
> future. For instance, suppose that it would have been handy to set an
> initial svn:mergeinfo property on new repositories, would you have
> said: ah no we can't do that because r0 is supposed to be empty?

Revision 0 being empty was certainly part of Subversion's design, but although some parts of the system encode this assumption, I agree it seems more or less an implementation detail in the sense that there don't seem to be any parts of the design that strongly depend on it. In most respects r0 is simply "the initial state", and it is not hard to imagine making an alternative version of Subversion that supports starting with a non-empty r0.

> Regardless of its definition, nowhere in the documentation is stated
> that you can't replay r0, so I maintain that googlecode is doing the
> wrong thing here.

The replay APIs are poorly documented with regard to r0, so let's look at the code. svn_repos_replay2(r0) actually just calls the editor's set_target_revision() method -- that's all. It doesn't call the open_root() method or attempt to describe "changes" for r0 in any way: in particular it does not attempt to say that the root directory is created.

That behaviour is fine, as far as it goes, except it should be documented. Note how it would not be possible to drive the editor in any valid way to describe the initial creation of a root directory or any other initial state in r0. It is one of the parts of the system whose design assumes that r0 does not have any interesting content other than rev-props.

Going back to the original report about the HTTP 'replay-report' method, the relevant question is how that should respond to a request for r0. If the response is built according to the editor API, then it can't possibly describe the creation of a root directory in r0, so the two possible responses are:

  * an empty response that does not attempt to describe the content -- like svn_repos_replay2()

or

  * error

I can't comment on which of those is more desirable.

- Julian
Received on 2014-10-13 13:14:08 CEST