Re: [WIP PATCH 1/7] Add skeleton RA svnclient

[Ramkumar Ramachandra <artagnon@gmail.com>]

Hi Jonathan and Daniel,

Jonathan: First, thanks for bringing up these questions- they will
definitely help future reviewers. I'll now sprinkle Daniel's replies
with a few of my observations.
Daniel: Thanks for responding to the questions- your insights as a
core Subversion developer are most appreciated. I'll add a few notes
to your answers now.

Daniel Shahaf wrote:
> Jonathan Nieder wrote on Fri, 25 Jun 2010 at 03:14 -0000:
>> For now, just some naïve questions.  Warning: I know nothing about
>> svn internals.

That's alright. I highly encourage naïve questions- I'm new to SVN
myself. I also highly recommend reading the API documentation and
going through the code for answers to "why is it like THIS" questions
as I haven't manged to clean out the Subversion style yet. Hopefully,
I'll have some good notes that I can attach with my next series to put
in the trunk.

>> I assume this corresponds to the ra-svn branch of
>> <http://github.com/artagnon/svn-dump-fast-export.git>.  Has the
>> relevant code changed much since you sent it?

Yes. I've managed to fix the deltified dump, and have now started
working on a full text dump. Due to my low familiarity with the libsvn
API, the cleanups are mixed with my code in the history; it needs a
thorough line-by-line scrubbing.

>> What is a baton?
>>
>
> The context object for a callback.
>
> You call:
>
>    some_function(your_callback_function, your_baton)
>
> which then calls:
>
>    your_callback_function(your_baton, other_arguments)

In general, I've found that batons are void * objects in which you can
stuff anything you like and pass around from function to function:
I've abused them quite heavily in my code by stuffing all kinds of
things into them: see fb->eb->* in my current code for an example of
this.

>> [...]
>> > +  void *wrapped_edit_baton;
>> [...]
>> > +  void *edit_baton;
>> > +  void *wrapped_dir_baton;
>> [...]
>> > +  void *edit_baton;
>> > +  void *wrapped_file_baton;
>>
>> Are these opaque types necessary?
>>
>
> The convention in Subversion's code is to convert the void * to
> a concrete_baton_t * only inside the callback.  If you wish to declare
> these, e.g., as
>
>    debug_editor_baton_t *wrapped_baton;
>
> You can probably do that too.

The function prototypes in libsvn contain void * parameters
corresponding to batons, so I'd have to typecast explicitly to avoid
any warnings. I think Jonathan's also referring to the absence of the
"struct" keyword everywhere, as that is against Git policy.
Unfortunately, everything is typedef'ed in libsvn, and we cannot do
much about that.

>> What does this do?  Is SVN_ERR for debugging?
>
> That's how we implement exception throwing in C.  SVN_ERR means "if this
> returned a non-NULL svn_error_t *, then return that error to our
> caller".
>
> The other pattern does
>
>    svn_error_t *err = svn_stream_printf();
>
> and then inspects err and err->apr_err to decide whether to ignore the
> error or return it (possibly wrapped).

>> Where does the output go?
>>
>
> SVN_ERR does not print anything.  It may return(), though.

Embarrassingly enough, write_indent does exactly what it says it does:
It writes some spaces (or indent) to eb->out, which you'll find is
actually stderr in svn_delta__get_debug. In the cleanup, I should
probably get rid of this. As far as the error handling is concerned,
to be terse, SVN_ERR and SVN_INT_ERR are cpp macros. They are defined
as follows in svn_error.h (with line numbers from the trunk file).
Note however, that even with all this error handling, the most common
type of error I get by far is the segfault: I'll make an effort to
document the pitfalls.

00284 #define SVN_ERR(expr)                           \
00285   do {                                          \
00286     svn_error_t *svn_err__temp = (expr);        \
00287     if (svn_err__temp)                          \
00288       return svn_error_return(svn_err__temp);   \
00289   } while (0)

00336 #define SVN_INT_ERR(expr)                                        \
00337   do {                                                           \
00338     svn_error_t *svn_err__temp = (expr);                         \
00339     if (svn_err__temp) {                                         \
00340       svn_handle_error2(svn_err__temp, stderr, FALSE, "svn: ");  \
00341       svn_error_clear(svn_err__temp);                            \
00342       return EXIT_FAILURE; }                                     \
00343   } while (0)

>> I take it these are callbacks?  Is there overview documentation for
>> them somewhere?
>>
>
> svn_delta_editor_t in
> http://svn.apache.org/repos/asf/subversion/trunk/subversion/include/svn_delta.h

Yes, they are callbacks that are fired automatically by the editor. In
svn_delta.h, look at struct svn_delta_editor_t (and the corresponding
doxygen-style comments).

>> I take it that the fields of svn_delta_editor_t do not have a
>> well-defined order?  Ugh.

No, they don't. I've tried to stick to the order in the struct
svn_delta_editor_t.

>> In any case, I suspect this would be easier to read rearranged a little:
>>
>>  1. declarations for callbacks
>>  2. get_debug_editor implementation
>>  3. definitions of types not needed in get_debug_editor()
>>  4. implementations of callbacks
>>
>> That way, a person reading straight through can figure out what’s
>> going on a little earlier.

Agreed. I'm still struggling to clean up the Subversion-style code. We
can probably discuss this at length on IRC?

>> What is svn_cmdline_init?
>
> A Subversion API that does some necessary initializations (e.g., calls
> apr_initialize()).  See subversion/include/svn_cmdline.h for docs
> (and subversion/libsvn_subr/cmdline.c for the implementation).

These svn_cmdline functions are actually shortcuts- they do all the
initializations required for a "typical" command line SVN client. It
saves me the trouble of having to figure out what I missed
initializing: I'll be using more of them in future; to eliminate the
auth baton creation by hand, for example.

>> Is this code destined for inclusion in svn upstream, and if so, where
>> can one find the surrounding code this should fit in with?

Yes, but with a lot of style transformations. In svnsync/main.c.
Atleast that's the plan, as per the discussion on #svn-dev

-- Ram