You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2014/10/03 01:13:57 UTC
Re: Review Request 24535: Added functionality to create SVN based
diffs of arbitrary strings.
-----------------------------------------------------------
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
>
>
Re: Review Request 24535: Added functionality to create SVN based
diffs of arbitrary strings.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> 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
>
>
Re: Review Request 24535: Added functionality to create SVN based
diffs of arbitrary strings.
Posted by Ben Mahler <be...@gmail.com>.
> On Oct. 2, 2014, 11:13 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp, line 31
> > <https://reviews.apache.org/r/24535/diff/2/?file=708152#file708152line31>
> >
> > Does this constructor provide any value?
Nevermind!
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24535/#review55228
-----------------------------------------------------------
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
>
>