----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24535/#review55228 -----------------------------------------------------------
Very cool! 3rdparty/libprocess/3rdparty/Makefile.am <https://reviews.apache.org/r/24535/#comment95623> Is there something different about subversion and apache portable runtime that means we aren't bundling them with fallbacks to installed versions, as we've done with all of our other libraries? 3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp <https://reviews.apache.org/r/24535/#comment95643> Does this constructor provide any value? 3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp <https://reviews.apache.org/r/24535/#comment95624> Why not call apr_terminate() when leaving this function? Can you include a comment? Ditto for svn::patch(). 3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp <https://reviews.apache.org/r/24535/#comment95657> This function was a pretty tough read, this comment doesn't provide much assistance to dummies like me :) I still don't understand why you set up the pipeline in reverse. You can compute the svn_txdelta without having setup the window handler, so is there any reason to not move the handler set up down below? Doing it in the expected order and along with detailed comments would be great to guide those that are not familiar with the svn library. For example: ``` apr_initialize(); apr_pool_t* pool = svn_pool_create(NULL); // First we need to produce a text delta stream by // diffing 'source' against 'target'. svn_string_t source; source.data = from.data(); source.len = from.length(); svn_string_t target; target.data = to.data(); target.len = to.length(); svn_txdelta_stream_t* delta; svn_txdelta( &delta, svn_stream_from_string(&source, pool), svn_stream_from_string(&target, pool), pool); // Now we want to convert this text delta stream into an // svndiff-format based diff. Setup the handler that will // consume the text delta and produce the svndiff. svn_txdelta_window_handler_t handler; void* baton = NULL; svn_stringbuf_t* diff = svn_stringbuf_create_ensure(1024, pool); svn_txdelta_to_svndiff2( &handler, &baton, svn_stream_from_stringbuf(diff, pool), 0, pool); // Now feed the text delta to the handler. svn_error_t* error = svn_txdelta_send_txstream(input, handler, baton, pool); ... ``` Note that I renamed `input` to `delta` and `stringbuf` to `diff` to try to make it a bit clearer. 3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp <https://reviews.apache.org/r/24535/#comment95648> Why not use an explicit NULL check here and in svn::patch().? 3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp <https://reviews.apache.org/r/24535/#comment95649> svn_err_best_message returns a char*, which either points to error->msg or to message, so this doesn't look right. Correct: ``` return Error(std::string(svn_err_best_message(error, message, 1024))); ``` Ditto for svn::patch(). 3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp <https://reviews.apache.org/r/24535/#comment95661> Some guiding comments would be great here as well: ``` // We want to apply the svndiff format diff to the source tring to // produce a result. First setup a handler for applying a text delta to // the source stream. ... svn_txdelta_apply(...); // Setup a stream that converts an svndiff format diff to a text delta, // so that we can use our handler to patch the source string. svn_stream_t* stream = svn_tx_delta_parse_svndiff(...); // Now feed the diff into the stream to compute the patched result. const char* data = diff.data.data(); apr_size_t length = diff.data.length(); svn_error_t* error = svn_stream_write(stream, data, &length); ... ``` 3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp <https://reviews.apache.org/r/24535/#comment95662> Calling this `stringbuf` seems a bit too generic, since this represents the patched result, how about we call this `patched` or `result`? - Ben Mahler On Sept. 29, 2014, 12:45 p.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24535/ > ----------------------------------------------------------- > > (Updated Sept. 29, 2014, 12:45 p.m.) > > > Review request for mesos, Ben Mahler and Jie Yu. > > > Repository: mesos-git > > > Description > ------- > > See summary. > > Note that this hard codes the location of the subversion and Apache Portable > Runtime (APR) headers. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/Makefile.am > bd1dc8df0259a318a9171a9c045a223800e64f47 > 3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp PRE-CREATION > 3rdparty/libprocess/3rdparty/stout/tests/svn_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/24535/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
