> On Oct. 2, 2014, 11:13 p.m., Ben Mahler wrote: > > Very cool!
Thanks for such a thorough review Ben! Very appreciated. > On Oct. 2, 2014, 11:13 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/3rdparty/Makefile.am, lines 188-189 > > <https://reviews.apache.org/r/24535/diff/2/?file=708151#file708151line188> > > > > 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? I was treating these standard libraries like SASL. > On Oct. 2, 2014, 11:13 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp, line 39 > > <https://reviews.apache.org/r/24535/diff/2/?file=708152#file708152line39> > > > > Why not call apr_terminate() when leaving this function? Can you > > include a comment? > > > > Ditto for svn::patch(). Ah, good point. I introduced a helper struct to handle apr_initialize and apr_terminate, thanks Ben! > On Oct. 2, 2014, 11:13 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp, lines 46-49 > > <https://reviews.apache.org/r/24535/diff/2/?file=708152#file708152line46> > > > > 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. (Sarcasm coming ...) What the heck Ben, you renamed 'input' to 'delta' but you forget to change it everywhere in your comment! ;-) Thanks for the code, setting it up this way sounds great. All of the examples I found online set it up in reverse, but I agree it's more straightforward to set it up this way. Thank you thank you! > On Oct. 2, 2014, 11:13 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp, line 79 > > <https://reviews.apache.org/r/24535/diff/2/?file=708152#file708152line79> > > > > 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(). Awesome, thanks Ben. > On Oct. 2, 2014, 11:13 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp, lines 91-137 > > <https://reviews.apache.org/r/24535/diff/2/?file=708152#file708152line91> > > > > 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); > > > > ... > > ``` Yes, thanks for the suggestions. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24535/#review55228 ----------------------------------------------------------- On Oct. 12, 2014, 4:14 a.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24535/ > ----------------------------------------------------------- > > (Updated Oct. 12, 2014, 4:14 a.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 > 256df0bb5557ebe6c75099d35c284804c9e57253 > 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 > >
