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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html