You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2014/08/11 05:34:16 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/
-----------------------------------------------------------
(Updated Aug. 11, 2014, 3:34 a.m.)
Review request for mesos, Connor Doyle and Tobi Knaup.
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 497d03892a1457138043cf7b68ea82a2f75ad162
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. 26, 2014, 5:58 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp, line 43
> > <https://reviews.apache.org/r/24535/diff/4/?file=733453#file733453line43>
> >
> > "since"
Thanks.
> On Oct. 26, 2014, 5:58 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp, line 39
> > <https://reviews.apache.org/r/24535/diff/4/?file=733453#file733453line39>
> >
> > Should this be in an internal namespace? Otherwise, callers might think they need to call initialize before the other functions?
I decided to put it in the public namespace in case someone actually did have another APR library that they're using and wants to serialize the initialization of APR by initializing both libraries before starting to do things in a multi-threaded capacity. It seemed cleaner to have people call svn::initialize than svn::diff("", "") or something similar. Nevertheless, I added a note explaining why this function was exposed.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24535/#review58564
-----------------------------------------------------------
On Oct. 25, 2014, 11:18 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24535/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2014, 11:18 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 1e24886628d07a788385e5056f05869b373d970a
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 41360620ee28bd2fca50f4b57ebe5803b10437cf
> 3rdparty/libprocess/3rdparty/stout/include/Makefile.am d5290130a5864aa38dc6aadb0d9664efb1424488
> 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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24535/#review58564
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp
<https://reviews.apache.org/r/24535/#comment99625>
Should this be in an internal namespace? Otherwise, callers might think they need to call initialize before the other functions?
3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp
<https://reviews.apache.org/r/24535/#comment99624>
"since"
- Ben Mahler
On Oct. 25, 2014, 11:18 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24535/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2014, 11:18 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 1e24886628d07a788385e5056f05869b373d970a
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 41360620ee28bd2fca50f4b57ebe5803b10437cf
> 3rdparty/libprocess/3rdparty/stout/include/Makefile.am d5290130a5864aa38dc6aadb0d9664efb1424488
> 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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24535/
-----------------------------------------------------------
(Updated Oct. 25, 2014, 11:18 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 (updated)
-----
3rdparty/libprocess/3rdparty/Makefile.am 1e24886628d07a788385e5056f05869b373d970a
3rdparty/libprocess/3rdparty/stout/Makefile.am 41360620ee28bd2fca50f4b57ebe5803b10437cf
3rdparty/libprocess/3rdparty/stout/include/Makefile.am d5290130a5864aa38dc6aadb0d9664efb1424488
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. 15, 2014, 12:22 a.m., Ben Mahler wrote:
> > Looks good modulo some issues below, would like to take a final pass when you update the APR() abstraction to be thread safe.
>
> Ben Mahler wrote:
> Hm.. have you looked at whether the other calls here are thread safe?
>
> For example, svn_pool_create().
Yes, according to http://goo.gl/NX0hps, apr_pool_create_ex is thread safe, which is what svn_pool_create calls (through svn_pool_create_ex). I added a comment and pointed to this documentation.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24535/#review56597
-----------------------------------------------------------
On Oct. 25, 2014, 11:18 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24535/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2014, 11:18 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 1e24886628d07a788385e5056f05869b373d970a
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 41360620ee28bd2fca50f4b57ebe5803b10437cf
> 3rdparty/libprocess/3rdparty/stout/include/Makefile.am d5290130a5864aa38dc6aadb0d9664efb1424488
> 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. 15, 2014, 12:22 a.m., Ben Mahler wrote:
> > Looks good modulo some issues below, would like to take a final pass when you update the APR() abstraction to be thread safe.
Hm.. have you looked at whether the other calls here are thread safe?
For example, svn_pool_create().
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24535/#review56597
-----------------------------------------------------------
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24535/#review56597
-----------------------------------------------------------
Looks good modulo some issues below, would like to take a final pass when you update the APR() abstraction to be thread safe.
3rdparty/libprocess/3rdparty/Makefile.am
<https://reviews.apache.org/r/24535/#comment96964>
You also need to update stout's two makefiles to add:
svn.hpp
svn_tests.cpp
3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp
<https://reviews.apache.org/r/24535/#comment96977>
s/finalize/initialize/
3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp
<https://reviews.apache.org/r/24535/#comment96983>
These apr_lifetime calls are not thread safe, so I believe there is an issue here when `svn::diff()` and `svn::patch()` are both called for the first time concurrently from different threads.
At that point, we make two concurrent calls to `apr_initialize()`, which is undefined.
We could add locking here, at which point we could also have apr_terminate() safely inside the destructor.
There is a thread about the thread safety (no pun intended), here:
http://mail-archives.apache.org/mod_mbox/apr-dev/201309.mbox/%3CCANe84rwsDAPTdiPRV6iNAwX2VDttfGGygGAxSsRh4xamPCA4gw@mail.gmail.com%3E
3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp
<https://reviews.apache.org/r/24535/#comment96976>
Why setup an atexit handler as opposed to calling apr_terminate() directly within ~APR()?
I ask because, presumably the point of this APR abstraction is to provide APR initialization and termination bounded to the scope of an APR() object. Anything I'm missing?
3rdparty/libprocess/3rdparty/stout/tests/svn_tests.cpp
<https://reviews.apache.org/r/24535/#comment96997>
Any chance you could add another test for empty diff / patch?
- Ben Mahler
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 Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
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 (updated)
-----
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 Benjamin Hindman <be...@berkeley.edu>.
> On Sept. 30, 2014, 4:48 p.m., Jie Yu wrote:
> >
Thanks for the great review Jie!
> On Sept. 30, 2014, 4:48 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/3rdparty/Makefile.am, lines 188-189
> > <https://reviews.apache.org/r/24535/diff/2/?file=708151#file708151line188>
> >
> > Should we add some check in configure.ac? Also, seems that not all distributions install headers under apr-1:
> >
> > http://packages.ubuntu.com/trusty/amd64/libaprutil1-dev/filelist
Tim Chen has done that in https://reviews.apache.org/r/26214.
> On Sept. 30, 2014, 4:48 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp, line 41
> > <https://reviews.apache.org/r/24535/diff/2/?file=708152#file708152line41>
> >
> > Do we need to do the pool allocation everytime? Or could we just do it once? I am not sure whether would cause any performance issue or not.
Unfortunately I don't know how to deallocate any memory that might have been allocated by functions that I passed the pool to, thus, I need to destroy the pool at the end of these functions and create a new one at the beginning (otherwise, IIUC, the pool will grow unbounded).
> On Sept. 30, 2014, 4:48 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp, line 53
> > <https://reviews.apache.org/r/24535/diff/2/?file=708152#file708152line53>
> >
> > This seems to be deprecated.
> > http://subversion.apache.org/docs/api/latest/group__svn__delta__txt__delta.html#gaf7354f923339d3338c3083e91bfee472
> >
> > Consider using svn_txdelta_to_svndiff3?
Tim Chen replaced the use of these in https://reviews.apache.org/r/26214.
> On Sept. 30, 2014, 4:48 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp, line 80
> > <https://reviews.apache.org/r/24535/diff/2/?file=708152#file708152line80>
> >
> > destroy pool before return?
Thank you thank you!
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24535/#review54857
-----------------------------------------------------------
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 Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24535/#review54857
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/Makefile.am
<https://reviews.apache.org/r/24535/#comment95171>
Should we add some check in configure.ac? Also, seems that not all distributions install headers under apr-1:
http://packages.ubuntu.com/trusty/amd64/libaprutil1-dev/filelist
3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp
<https://reviews.apache.org/r/24535/#comment95314>
Do we need to do the pool allocation everytime? Or could we just do it once? I am not sure whether would cause any performance issue or not.
3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp
<https://reviews.apache.org/r/24535/#comment95188>
This seems to be deprecated.
http://subversion.apache.org/docs/api/latest/group__svn__delta__txt__delta.html#gaf7354f923339d3338c3083e91bfee472
Consider using svn_txdelta_to_svndiff3?
3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp
<https://reviews.apache.org/r/24535/#comment95189>
This is deprecated as well.
3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp
<https://reviews.apache.org/r/24535/#comment95172>
destroy pool before return?
3rdparty/libprocess/3rdparty/stout/include/stout/svn.hpp
<https://reviews.apache.org/r/24535/#comment95173>
destroy pool before return?
- Jie Yu
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
>
>
Re: Review Request 24535: Added functionality to create SVN based
diffs of arbitrary strings.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
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>.
-----------------------------------------------------------
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24535/
-----------------------------------------------------------
(Updated Sept. 29, 2014, 12:23 p.m.)
Review request for mesos, Connor Doyle and Tobi Knaup.
Changes
-------
Rebased.
Repository: mesos-git
Description
-------
See summary.
Note that this hard codes the location of the subversion and Apache Portable Runtime (APR) headers.
Diffs (updated)
-----
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