You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Madan U Sreenivasan <ma...@collab.net> on 2005/06/28 12:01:41 UTC
[PATCH] Partial fix for Issue #443: post-commit hook script
(error) output lost.
- Have extracted only the ra_dav part of the patch (for ease of
review ) and fixed the factorizations.
- Also attached are design files that help understand the patch.
- I have tested this with a test case on all three protocols ( the
test currently passes only over ra_dav )
[[[
Partial fix for Issue #443: post-commit hook script (error) output
lost.
Includes post-commit hook stderr propagation for ra_dav
Does not include post-commit hook stderr propagation for ra_local
and ra_svn.
Does not include the pre-revprop-change fix.
* subversion/libsvn_ra/ra_loader.c
(svn_ra_find_protocol): New API to find out which
schema/protocol
would be used, given a URL.
(svn_ra_get_commit_editor2): New version of the API to use
the new commit_editor function and hence use the new callback.
* subversion/libsvn_ra/ra_loader.h
(svn_ra__vtable_t): Added comments for get_commit_editor and
intoduced a new member get_commit_editor2 which now uses the
svn_commit_callback2_t.
* subversion/include/svn_repos.h
(svn_repos_get_commit_editor3): New API to accomodate the
svn_commit_callback2_t callback type.
* subversion/include/svn_types.h
(svn_commit_callback2_t): Ah, the core change. A callback that
takes the post_commit_err parameter.
* subversion/include/svn_client.h
(svn_client_commit_info2_t): The structure that carries the
commit success parameters now has to carry the post_commit_err
too.
(svn_client_mkdir): mkdir should now use the new structure -
svn_client_commit_info2_t.
(svn_client_delete): delete should now use the new structure -
svn_client_commit_info2_t.
(svn_client_commit3): New API that uses
svn_client_commit_info2_t.
(svn_client_copy2): New API that uses svn_client_commit_info2_t.
* subversion/include/svn_ra.h
(svn_ra_find_protocol): New API declaration added.
(svn_ra_get_commit_editor2): New API that uses the new callback.
* subversion/libsvn_ra_local/ra_plugin.c
(ra_local_vtable): ra_local's vtable does NOT have callback2.
* subversion/libsvn_client/delete.c
(delete_urls): delete_urls now uses the
svn_client_commit_info2_t structure.
Call svn_ra_get_commit_editor2() if ra_dav.
Call svn_ra_get_commit_editor() otherwise.
(svn_client_delete): svn_client_delete should now use
svn_client_commit_info2_t.
* subversion/libsvn_client/client.h
(svn_client__commit_get_baton): Should now use the new commit
info structrure - svn_client_commit_info2_t.
(svn_client__commit_callback2): New callback that now takes
post_commit_err as one of its parameters.
* subversion/libsvn_client/copy.c
(repos_to_repos_copy): Now uses the svn_client_commit_info2_t
structure.
Call svn_ra_get_commit_editor2() if ra_dav.
Call svn_ra_get_commit_editor() otherwise.
(wc_to_repos_copy): Now uses the svn_client_commit_info2_t
structure.
Call svn_ra_get_commit_editor2() if ra_dav.
Call svn_ra_get_commit_editor() otherwise.
(setup_copy): Now uses svn_client_commit_info2_t.
(svn_client_copy2): New version of API that takes a
svn_client_commit_info2_t for commit_info.
(svn_client_move3): New version of API that takes a
svn_client_commit_info2_t for commit_info.
* subversion/libsvn_client/commit_util.c
(commit_baton): Now uses svn_client_commit_info2_t.
(svn_client__commit_get_baton): Now uses
svn_client_commit_info2_t.
(svn_client__commit_callback2): This callback function
fills the svn_client_commit_info2_t structure with the
post_commit_err, if any.
* subversion/libsvn_client/add.c
(mkdir_urls): Now uses svn_client_commit_info2_t.
Calls svn_ra_get_commit_editor2() if ra_dav.
Calls svn_ra_get_commit_editor() otherwise.
(svn_client_mkdir): Now uses svn_client_commit_info2_t.
* subversion/libsvn_client/commit.c
(get_ra_editor): Now uses svn_client_commit_info2_t.
Calls svn_ra_get_commit_editor2() if ra_dav.
Calls svn_ra_get_commit_editor() otherwise.
(svn_client_import2): get_ra_editor now passed typecasted
commit_info.
(svn_client_commit3): New version of API that uses
svn_client_commit_info2_t.
(svn_client_commit2): A svn_client_commit_info2_t is allocated
and passed back as svn_client_commit_info_t. This doesn't matter
as svn_client_commit_info2_t is one member more than
svn_client_commit_info_t.
* subversion/mod_dav_svn/merge.c
(#include): Included svn_xml.h
(dav_svn__merge_response): Now takes an extra post_commit_err
parameter. If the post_commit_err is available,
- Adds the SVN namespace
- post-commit-err element to hold the post_commit_err
to the merge response.
* subversion/mod_dav_svn/dav_svn.h
(dav_svn__merge_response): Now takes a new post_commit_err
parameter.
* subversion/mod_dav_svn/version.c
(dav_svn_merge): If post-commit hook returns an error,
pass it along to dav_svn__merge_response.
* subversion/clients/cmdline/cl.h
(svn_cl__print_commit_info2): Declaration for the new function
that ultimately prints out the post-commit hook's stderr to
the user.
* subversion/clients/cmdline/mkdir-cmd.c
(svn_cl__mkdir): Now uses svn_client_commit_info2_t and
svn_cl__print_commit_info2.
* subversion/clients/cmdline/move-cmd.c
(svn_cl__move): Now uses svn_client_commit_info2_t and
svn_cl__print_commit_info2.
* subversion/clients/cmdline/copy-cmd.c
(svn_cl__copy): Now uses svn_client_commit_info2_t and
svn_cl__print_commit_info2.
* subversion/clients/cmdline/util.c
(svn_cl__print_commit_info2): Home at last!!! New version of
API.
svn_cl__print_commit_info2 is the function that ultimately
prints out the post_commit_error to the user.
* subversion/clients/cmdline/commit-cmd.c
(svn_cl__commit): Now uses svn_client_commit_info2_t and
svn_cl__print_commit_info2 for callback.
* subversion/clients/cmdline/delete-cmd.c
(svn_cl__delete): Now uses svn_client_commit_info2_t and
svn_cl__print_commit_info2 for callback.
* subversion/tests/clients/cmdline/commit_tests.py
(post_commit_hook_test): New function to test the post-commit
hook's propogation of stderr to the client.
(test_list): Added post_commit_hook_test to list of tests.
* subversion/libsvn_repos/hooks.c
(svn_repos__hooks_post_commit): Now runs the post-commit hook
with the read_errstream parameter as TRUE.
* subversion/libsvn_repos/commit.c
(edit_baton): Added new member - callback2, to hold a callback
of type svn_commit_callback2_t. Also added comments that
callback
should be removed once all code uses callback2.
(close_edit): Handles the post-commit stderr. Decides whether
to use the callback or the callback2 member of the edit baton.
(svn_repos_get_commit_editor3): Uses svn_commit_callback2_t for
callback type, and fills callback2 of the edit baton.
(svn_repos_get_commit_editor2): Still uses svn_commit_callback_t
and fills the callback member of the edit baton.
* subversion/libsvn_ra_svn/client.c
(ra_svn_vtable): ra_svn's vtable doesnt have a
get_commit_editor2 member.
* subversion/libsvn_ra_dav/merge.c
(merge_elements): Added the post-commit-err element.
(merge_ctx_t): Added the post_commit_err member.
(validate_element): Make ELEM_post_commit_err a valid element.
(end_element): Add case, and handle ELEM_post_commit_err.
(svn_ra_dav__merge_activity2): New merge function to handle
post-commit hook's stderr.
* subversion/libsvn_ra_dav/ra_dav.h
(svn_ra_dav__get_commit_editor2): New version of API, using
svn_commit_callback2_t.
(enum): Added ELEM_post_commit_err member.
(svn_ra_dav__merge_activity2): Declaration for new version of
API.
* subversion/libsvn_ra_dav/session.c
(dav_vtable): Now contains a get_commit_editor2 function, but no
get_commit_editor function.
* subversion/libsvn_ra_dav/commit.c
(commit_ctx_t): Added the callback2 member.
(commit_close_edit): Now uses te svn_ra_dav__merge_activity2
function.
(svn_ra_dav__get_commit_editor2): Now fills the callback2 member
(svn_ra_dav__get_commit_editor): Still fills the callback member
]]]
Re: [PATCH] Partial fix for Issue #443: post-commit hook script
(error) output lost.
Posted by Madan U Sreenivasan <ma...@collab.net>.
On Thu, 2005-06-30 at 20:04, kfogel@collab.net wrote:
> Madan U Sreenivasan <ma...@collab.net> writes:
> > /me requests a review....
>
> Believe me, I've had it marked for review since it appeared... just
> some test suite work I was doing ended up going much, much deeper than
> I anticipated. Sorry for the delay, I hope to review this (and other
> patches) today.
:)... no probs Karl. Thanks.
>
> Best,
> -Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Partial fix for Issue #443: post-commit hook script (error) output lost.
Posted by kf...@collab.net.
kfogel@collab.net writes:
> Madan U Sreenivasan <ma...@collab.net> writes:
> > /me requests a review....
>
> Believe me, I've had it marked for review since it appeared... just
> some test suite work I was doing ended up going much, much deeper than
> I anticipated. Sorry for the delay, I hope to review this (and other
> patches) today.
Just FYI, Madan, I have started on the review of this. It is going to
take about a day, so I hope to send completed review tomorrow. The
patch is very big, and there's a lot to say (which is not a reflection
on the quality of the patch, it's just complex so naturally there's a
lot to say), that's why it's taking so long.
Best,
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Partial fix for Issue #443: post-commit hook script (error) output lost.
Posted by kf...@collab.net.
Madan U Sreenivasan <ma...@collab.net> writes:
> /me requests a review....
Believe me, I've had it marked for review since it appeared... just
some test suite work I was doing ended up going much, much deeper than
I anticipated. Sorry for the delay, I hope to review this (and other
patches) today.
Best,
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Partial fix for Issue #443: post-commit hook script
(error) output lost.
Posted by Madan U Sreenivasan <ma...@collab.net>.
/me requests a review....
On Tue, 2005-06-28 at 17:31, Madan U Sreenivasan wrote:
> - Have extracted only the ra_dav part of the patch (for ease of
> review ) and fixed the factorizations.
> - Also attached are design files that help understand the patch.
> - I have tested this with a test case on all three protocols ( the
> test currently passes only over ra_dav )
>
> [[[
> Partial fix for Issue #443: post-commit hook script (error) output
> lost.
> Includes post-commit hook stderr propagation for ra_dav
> Does not include post-commit hook stderr propagation for ra_local
> and ra_svn.
> Does not include the pre-revprop-change fix.
>
> * subversion/libsvn_ra/ra_loader.c
> (svn_ra_find_protocol): New API to find out which
> schema/protocol
> would be used, given a URL.
> (svn_ra_get_commit_editor2): New version of the API to use
> the new commit_editor function and hence use the new callback.
> * subversion/libsvn_ra/ra_loader.h
> (svn_ra__vtable_t): Added comments for get_commit_editor and
> intoduced a new member get_commit_editor2 which now uses the
> svn_commit_callback2_t.
> * subversion/include/svn_repos.h
> (svn_repos_get_commit_editor3): New API to accomodate the
> svn_commit_callback2_t callback type.
> * subversion/include/svn_types.h
> (svn_commit_callback2_t): Ah, the core change. A callback that
> takes the post_commit_err parameter.
> * subversion/include/svn_client.h
> (svn_client_commit_info2_t): The structure that carries the
> commit success parameters now has to carry the post_commit_err
> too.
> (svn_client_mkdir): mkdir should now use the new structure -
> svn_client_commit_info2_t.
> (svn_client_delete): delete should now use the new structure -
> svn_client_commit_info2_t.
> (svn_client_commit3): New API that uses
> svn_client_commit_info2_t.
> (svn_client_copy2): New API that uses svn_client_commit_info2_t.
> * subversion/include/svn_ra.h
> (svn_ra_find_protocol): New API declaration added.
> (svn_ra_get_commit_editor2): New API that uses the new callback.
> * subversion/libsvn_ra_local/ra_plugin.c
> (ra_local_vtable): ra_local's vtable does NOT have callback2.
> * subversion/libsvn_client/delete.c
> (delete_urls): delete_urls now uses the
> svn_client_commit_info2_t structure.
> Call svn_ra_get_commit_editor2() if ra_dav.
> Call svn_ra_get_commit_editor() otherwise.
> (svn_client_delete): svn_client_delete should now use
> svn_client_commit_info2_t.
> * subversion/libsvn_client/client.h
> (svn_client__commit_get_baton): Should now use the new commit
> info structrure - svn_client_commit_info2_t.
> (svn_client__commit_callback2): New callback that now takes
> post_commit_err as one of its parameters.
> * subversion/libsvn_client/copy.c
> (repos_to_repos_copy): Now uses the svn_client_commit_info2_t
> structure.
> Call svn_ra_get_commit_editor2() if ra_dav.
> Call svn_ra_get_commit_editor() otherwise.
> (wc_to_repos_copy): Now uses the svn_client_commit_info2_t
> structure.
> Call svn_ra_get_commit_editor2() if ra_dav.
> Call svn_ra_get_commit_editor() otherwise.
> (setup_copy): Now uses svn_client_commit_info2_t.
> (svn_client_copy2): New version of API that takes a
> svn_client_commit_info2_t for commit_info.
> (svn_client_move3): New version of API that takes a
> svn_client_commit_info2_t for commit_info.
> * subversion/libsvn_client/commit_util.c
> (commit_baton): Now uses svn_client_commit_info2_t.
> (svn_client__commit_get_baton): Now uses
> svn_client_commit_info2_t.
> (svn_client__commit_callback2): This callback function
> fills the svn_client_commit_info2_t structure with the
> post_commit_err, if any.
> * subversion/libsvn_client/add.c
> (mkdir_urls): Now uses svn_client_commit_info2_t.
> Calls svn_ra_get_commit_editor2() if ra_dav.
> Calls svn_ra_get_commit_editor() otherwise.
> (svn_client_mkdir): Now uses svn_client_commit_info2_t.
> * subversion/libsvn_client/commit.c
> (get_ra_editor): Now uses svn_client_commit_info2_t.
> Calls svn_ra_get_commit_editor2() if ra_dav.
> Calls svn_ra_get_commit_editor() otherwise.
> (svn_client_import2): get_ra_editor now passed typecasted
> commit_info.
> (svn_client_commit3): New version of API that uses
> svn_client_commit_info2_t.
> (svn_client_commit2): A svn_client_commit_info2_t is allocated
> and passed back as svn_client_commit_info_t. This doesn't matter
> as svn_client_commit_info2_t is one member more than
> svn_client_commit_info_t.
> * subversion/mod_dav_svn/merge.c
> (#include): Included svn_xml.h
> (dav_svn__merge_response): Now takes an extra post_commit_err
> parameter. If the post_commit_err is available,
> - Adds the SVN namespace
> - post-commit-err element to hold the post_commit_err
> to the merge response.
> * subversion/mod_dav_svn/dav_svn.h
> (dav_svn__merge_response): Now takes a new post_commit_err
> parameter.
> * subversion/mod_dav_svn/version.c
> (dav_svn_merge): If post-commit hook returns an error,
> pass it along to dav_svn__merge_response.
> * subversion/clients/cmdline/cl.h
> (svn_cl__print_commit_info2): Declaration for the new function
> that ultimately prints out the post-commit hook's stderr to
> the user.
> * subversion/clients/cmdline/mkdir-cmd.c
> (svn_cl__mkdir): Now uses svn_client_commit_info2_t and
> svn_cl__print_commit_info2.
> * subversion/clients/cmdline/move-cmd.c
> (svn_cl__move): Now uses svn_client_commit_info2_t and
> svn_cl__print_commit_info2.
> * subversion/clients/cmdline/copy-cmd.c
> (svn_cl__copy): Now uses svn_client_commit_info2_t and
> svn_cl__print_commit_info2.
> * subversion/clients/cmdline/util.c
> (svn_cl__print_commit_info2): Home at last!!! New version of
> API.
> svn_cl__print_commit_info2 is the function that ultimately
> prints out the post_commit_error to the user.
> * subversion/clients/cmdline/commit-cmd.c
> (svn_cl__commit): Now uses svn_client_commit_info2_t and
> svn_cl__print_commit_info2 for callback.
> * subversion/clients/cmdline/delete-cmd.c
> (svn_cl__delete): Now uses svn_client_commit_info2_t and
> svn_cl__print_commit_info2 for callback.
> * subversion/tests/clients/cmdline/commit_tests.py
> (post_commit_hook_test): New function to test the post-commit
> hook's propogation of stderr to the client.
> (test_list): Added post_commit_hook_test to list of tests.
> * subversion/libsvn_repos/hooks.c
> (svn_repos__hooks_post_commit): Now runs the post-commit hook
> with the read_errstream parameter as TRUE.
> * subversion/libsvn_repos/commit.c
> (edit_baton): Added new member - callback2, to hold a callback
> of type svn_commit_callback2_t. Also added comments that
> callback
> should be removed once all code uses callback2.
> (close_edit): Handles the post-commit stderr. Decides whether
> to use the callback or the callback2 member of the edit baton.
> (svn_repos_get_commit_editor3): Uses svn_commit_callback2_t for
> callback type, and fills callback2 of the edit baton.
> (svn_repos_get_commit_editor2): Still uses svn_commit_callback_t
> and fills the callback member of the edit baton.
> * subversion/libsvn_ra_svn/client.c
> (ra_svn_vtable): ra_svn's vtable doesnt have a
> get_commit_editor2 member.
> * subversion/libsvn_ra_dav/merge.c
> (merge_elements): Added the post-commit-err element.
> (merge_ctx_t): Added the post_commit_err member.
> (validate_element): Make ELEM_post_commit_err a valid element.
> (end_element): Add case, and handle ELEM_post_commit_err.
> (svn_ra_dav__merge_activity2): New merge function to handle
> post-commit hook's stderr.
> * subversion/libsvn_ra_dav/ra_dav.h
> (svn_ra_dav__get_commit_editor2): New version of API, using
> svn_commit_callback2_t.
> (enum): Added ELEM_post_commit_err member.
> (svn_ra_dav__merge_activity2): Declaration for new version of
> API.
> * subversion/libsvn_ra_dav/session.c
> (dav_vtable): Now contains a get_commit_editor2 function, but no
> get_commit_editor function.
> * subversion/libsvn_ra_dav/commit.c
> (commit_ctx_t): Added the callback2 member.
> (commit_close_edit): Now uses te svn_ra_dav__merge_activity2
> function.
> (svn_ra_dav__get_commit_editor2): Now fills the callback2 member
> (svn_ra_dav__get_commit_editor): Still fills the callback member
> ]]]
>
> ______________________________________________________________________
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Partial fix for Issue #443: post-commit hook script
(error) output lost.
Posted by Madan U Sreenivasan <ma...@collab.net>.
On Thu, 2005-07-07 at 18:48, kfogel@collab.net wrote:
> Madan U Sreenivasan <ma...@collab.net> writes:
> [snip]
> Ah, I see. What I meant to express is: there's a difference between
> revving an API to take a new struct2, and actually changing the code
> behind that API to *use* the new fields in struct2. I'm only
> proposing the former; I agree that changing the code behind those
> other APIs can happen in a separate stage.
Okay.
>
> > >
> [snip]
> So, I think to keep the patches manageable (i.e., reviewable :-) ),
> the stages could be:
>
> 1. Introduce svn_client_commit_info2_t and its constructor.
>
> 2. Rev all APIs that take svn_client_commit_info_t to take
> svn_client_commit_info2_t instead, but don't actually do
> anything with the new field yet. IOW, no behavioral change,
> just the introduction of a new type, in preparation for
> stages 3, 4, and 5 below.
>
> 3. In one RA layer, start using the new field.
>
> 4. Same for the next RA layer :-).
>
> 5. Same for yet another RA layer :-).
>
> And if you can think of other intermediate stages that ought to be
> inserted into this process, go ahead -- the above is just a rough
> outline.
I think the server side changes ( for all three layers) should go in
before 3. the rest of the changes could be for the client side.
But sadly, a new test case cant be used for any of the intermediate
steps. It will have to come only with the last piece of change. :(
Whatever it is, am determined to get this into 1.3. I'll do the best I
can...
>
> Best,
> -Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Partial fix for Issue #443: post-commit hook script (error) output lost.
Posted by kf...@collab.net.
Madan U Sreenivasan <ma...@collab.net> writes:
> > * Avoids the need for svn_ra_find_protocol() and the conditionals
> > resulting therefrom.
> > * Really deprecates the things it deprecates, everywhere, and revs
> > APIs as necessary to compensate.
> hmmmm... that was what my earlier patches on #443 did - using the new
> APIs all the way. But then along the way, while refactoring some
> functions, and, I thought if the issue was taken care in phases ( ra_dav
> first, ra_local and ra_svn subsequently), two things can be achieved...
> - Ease of review : as the patch is only half the size
> - Help as a inherent test of the current use of the deprecable API
> functions ( that is I have NOT broken existing API usage )
Ah, I see. What I meant to express is: there's a difference between
revving an API to take a new struct2, and actually changing the code
behind that API to *use* the new fields in struct2. I'm only
proposing the former; I agree that changing the code behind those
other APIs can happen in a separate stage.
> > * Provides a constructor function for any structures we had to rev,
> > and documents on the new versions of the structures that they
> > should always be allocated by the constructor, so that at least
> > future revs will not be necessary.
> hmmmm, This should be done as a separate patch, before introducing the
> new structures, shouldn't it? IMHumbleO, The intent of introducing the
> constructor type of function doesnt inherently figure in the purpose of
> #443.
Yes -- good call, thanks for pointing it out.
And in fact, the revving of the APIs themselves could happen in a
separate patch, if you want. (I.e., introduce
svn_client_commit_info2_t, with its new field, everywhere, but don't
use it -- then the next patch would make use of it in ra_dav).
> > * Has a log message in the more compact, more standard style that
> > I've tried to point out above.
>
> There a lot for me to take home from your review. Thank you for the
> detailed review and suggestions. I will spend time on your comments. You
> can expect better logs from me next time on. Thank you.
Well, thank *you* for your patience. I realize there are a lot of
conventions to learn here!
> > Or, if any of these things (especially the first three) seem like bad
> > ideas to you, please explain why, and we'll figure out the best design
> > on the list here.
>
> Oh, no, nothing wrong with the idea.... not at all, in fact I would have
> been happy to do that ( specifically, I wasnt happy to introduce the
> svn_ra_find_protocol() function at all - just had to do it to reduce the
> size of the patch.)
>
> Summary : If a longer patch is okay, I will take care of 1,2. Point 3 to
> be taken care as a separate patch prior to #443. Point 4 is a MUST DO
> for me. I will take care of this in future. Thanks.
>
> Karl, pl. let me know what you think.
So, I think to keep the patches manageable (i.e., reviewable :-) ),
the stages could be:
1. Introduce svn_client_commit_info2_t and its constructor.
2. Rev all APIs that take svn_client_commit_info_t to take
svn_client_commit_info2_t instead, but don't actually do
anything with the new field yet. IOW, no behavioral change,
just the introduction of a new type, in preparation for
stages 3, 4, and 5 below.
3. In one RA layer, start using the new field.
4. Same for the next RA layer :-).
5. Same for yet another RA layer :-).
And if you can think of other intermediate stages that ought to be
inserted into this process, go ahead -- the above is just a rough
outline.
Best,
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Partial fix for Issue #443: post-commit hook script
(error) output lost.
Posted by Madan U Sreenivasan <ma...@collab.net>.
Hi Karl,
> manner described in the following IRC conversation, which I will quote
> in full, so you can see the love and care with which I enter into code
> reviews:
I totally understand Karl. :)
[snip]
> Okay, at this point enough questions have been raised in the log
> message that I think it doesn't make sense for me to review the patch.
> Instead, can you produce a new patch that
>
> * Avoids the need for svn_ra_find_protocol() and the conditionals
> resulting therefrom.
> * Really deprecates the things it deprecates, everywhere, and revs
> APIs as necessary to compensate.
hmmmm... that was what my earlier patches on #443 did - using the new
APIs all the way. But then along the way, while refactoring some
functions, and, I thought if the issue was taken care in phases ( ra_dav
first, ra_local and ra_svn subsequently), two things can be achieved...
- Ease of review : as the patch is only half the size
- Help as a inherent test of the current use of the deprecable API
functions ( that is I have NOT broken existing API usage )
>
> * Provides a constructor function for any structures we had to rev,
> and documents on the new versions of the structures that they
> should always be allocated by the constructor, so that at least
> future revs will not be necessary.
hmmmm, This should be done as a separate patch, before introducing the
new structures, shouldn't it? IMHumbleO, The intent of introducing the
constructor type of function doesnt inherently figure in the purpose of
#443.
>
> * Has a log message in the more compact, more standard style that
> I've tried to point out above.
There a lot for me to take home from your review. Thank you for the
detailed review and suggestions. I will spend time on your comments. You
can expect better logs from me next time on. Thank you.
>
> Or, if any of these things (especially the first three) seem like bad
> ideas to you, please explain why, and we'll figure out the best design
> on the list here.
Oh, no, nothing wrong with the idea.... not at all, in fact I would have
been happy to do that ( specifically, I wasnt happy to introduce the
svn_ra_find_protocol() function at all - just had to do it to reduce the
size of the patch.)
Summary : If a longer patch is okay, I will take care of 1,2. Point 3 to
be taken care as a separate patch prior to #443. Point 4 is a MUST DO
for me. I will take care of this in future. Thanks.
Karl, pl. let me know what you think.
Regards,
Madan.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Partial fix for Issue #443: post-commit hook script (error) output lost.
Posted by kf...@collab.net.
Madan U Sreenivasan <ma...@collab.net> writes:
> - Have extracted only the ra_dav part of the patch (for ease of
> review ) and fixed the factorizations.
> - Also attached are design files that help understand the patch.
If this were committed, where should that content go? Into the notes/
directory, or into comments in the code, or somewhere else?
In other words, if those files are needed (or even just very helpful)
to understand the patch, then they should be associated with the
change when it is committed.
However, usually we do not use diagrams or flow descriptions to
explain patches. The log message, code, and comments should be
enough. Also, I personally found that the diagram made my head spin
and only confused me :-). (This may be more a statement about me than
about the diagram -- I'm not a very visual person.)
Anyway, I'm not reviewing the diagram and accompanying explanation. I
get the feeling they were more things you drew up for own benefit
while writing the change (?), which is fine, but then they probably
needn't be submitted along with the patch.
> - I have tested this with a test case on all three protocols ( the
> test currently passes only over ra_dav )
Okay.
Now, on to the log message.
By the way, you included your log message inline in your message,
*and* attached it. I diff'd the two: there were many differences,
although they mainly appeared to be formatting issues, not significant
differences of content. Still, please include it in exactly one place
-- it doesn't matter which, just don't duplicate it. When there are
two or more, then a reviewer doesn't know which one to review! :-)
> [[[
> Partial fix for Issue #443: post-commit hook script (error) output
> lost.
> Includes post-commit hook stderr propagation for ra_dav
> Does not include post-commit hook stderr propagation for ra_local
> and ra_svn.
Those last two sentences could be made into a more compact one, easier
to understand:
"This change propagates post-commit hook stderr output for ra_dav,
but not for ra_svn or ra_local."
> Does not include the pre-revprop-change fix.
Someone reading this change later will not know what you mean by "the
pre-revprop-change" fix. Any context around this patch will have been
lost by then, except what can be reached from the issue and from the
change itself.
> * subversion/libsvn_ra/ra_loader.c
> (svn_ra_find_protocol): New API to find out which
> schema/protocol
> would be used, given a URL.
(Is your mailer inserting odd line breaks, or is that actually in the
log message?)
> (svn_ra_get_commit_editor2): New version of the API to use
> the new commit_editor function and hence use the new callback.
When you refer to things like "the API" and "the new commit_editor
function", it's confusing. Which API? What is the "commit_editor"
function? There is no new function with that name in your log
message.
Just refer to things by their exact names, whenever there is any
possibility of ambiguity, that will make things easier for readers.
> * subversion/libsvn_ra/ra_loader.h
> (svn_ra__vtable_t): Added comments for get_commit_editor and
> intoduced a new member get_commit_editor2 which now uses the
> svn_commit_callback2_t.
"introduced" :-)
At this point, I'm beginning to sense the overall strategy for the
patch: you're adding a upgraded version of a callback function, and
using it for ra_dav, while continuing to use the older (non-upgraded)
function for ra_svn and ra_local.
I wonder: why not use the new function everywhere, but just design it
in such a way that if certain functionality isn't available, then the
appropriate flag values are returned (NULL or whatever)? Then there
would be no need for the new svn_ra_find_protocol() function, no need
for certain conditional checks later, etc.
I haven't gotten far enough to know whether my speculation makes sense
yet, I just wanted to plant the thought.
> * subversion/include/svn_repos.h
> (svn_repos_get_commit_editor3): New API to accomodate the
> svn_commit_callback2_t callback type.
> * subversion/include/svn_types.h
> (svn_commit_callback2_t): Ah, the core change. A callback that
> takes the post_commit_err parameter.
Heh! Nice, I like the fact that you called it out as "the core
change", that actually helps me understand the change a lot.
Again, I find myself thinking: why can't everyone use this new
callback, and just have 'post_commit_err' be NULL if not available?
("not available" either because there was no error, or the server is
not equipped to tell the client whether or not there was any error.)
> * subversion/include/svn_client.h
> (svn_client_commit_info2_t): The structure that carries the
> commit success parameters now has to carry the post_commit_err
> too.
Wait -- this is a new structure, right? Then say it's new, otherwise
I'll think you're extending an old structure in-place. Just say
(svn_client_commit_info2_t): New struct, carries post_commit_err.
(svn_client_commit_info_t): Deprecate.
> (svn_client_mkdir): mkdir should now use the new structure -
> svn_client_commit_info2_t.
> (svn_client_delete): delete should now use the new structure -
> svn_client_commit_info2_t.
Just say "Use new svn_client_commit_info2_t structure." for
these. In fact, then you can put them as one item, like this:
(svn_client_mkdir, svn_client_delete): Use the new
svn_client_commit_info2_t structure.
More importantly:
We decided a long time ago that when there's a transparent, public
structure like this, our only choice when adding a new field is to rev
the entire structure (i.e., create a new struct). That's what you do,
and that's fine.
However, we can at least alleviate this problem for the future, in a
manner described in the following IRC conversation, which I will quote
in full, so you can see the love and care with which I enter into code
reviews:
<kfogel> Didn't we decide that it's okay to add new fields to the
end of a public structure? Such as 'svn_client_commit_info_t'?
Such as a new post_commit_err field?
<kfogel> I'm reviewing Madan's issue #443 patch (post-commit hook
stderr output lost, never seen by client).
<kfogel> He adds a new post_commit_err field to that structure; so
he revs to svn_client_commit_info2_t, which requires new
public APIs for every svn_client_foo() function that can
cause a commit.
<kfogel> It would be so nice to avoid that cascading API change.
<kfogel> EOT. Comments welcome.
<breser> Depends.
[...]
<kfogel> Ooooooh. Not EOT, wait a sec.
<kfogel> We don't have a function for allocating those structures.
<sussman> ergo, we're deda
<kfogel> So if someone passes one in, and we seek to its
post_commit_err field, and it's from an older library, then ...
<kfogel> boom
<sussman> dead
<breser> Yup.
<kfogel> So there's no way to avoid this now.
<breser> It's okay to add fields to structures only we generate.
<kfogel> Except, at least we can document that one should always
use our function (which we'll now supply) for allocating
these structures.
<kfogel> So this never need happen again w/ this particular structure.
<kfogel> Whew.
<kfogel> Thoughts?
<sussman> moral of the story: next time we design a big API, maybe
we should stick to constructors/accessors
<sussman> :-(
<breser> Frankly I think we've made a number of API design mistakes
with respect to making our life easier with our compatablity
gurantees.
<breser> E.G. booleans to functions.
<kfogel> breser: ?
<kfogel> Instead of masks or something?
<breser> We shouldn't have had a list of booleans to functions for
options to that function...
<breser> Yup should have used masks.
<kfogel> Yeah.
<breser> And we should have made init functions for all our structs.
<kfogel> Yup.
* kfogel 's neck hurts what with all the nodding agreeing with breser
<breser> I tried to say that a long time ago and everyone said we
didn't need them. :)
<kfogel> Heh.
<kfogel> Was I one of the ones disagreeing?
<breser> I'm not sure this was pre 1.0.
<breser> ISTR you guys didn't want to clean up these sorts of
issues becuase you wanted 1.0 done *NOW*.
<breser> Other mistakes that were made was not clearly defining
what was and wasn't valid data for certain things.
<sussman> well, luckily, svn 2.0 will be entirely written in python!
<kfogel> breser: well, if it was a matter of getting 1.0 out the
door, I think it might have been okay still.
<kfogel> After all, the foo2() convention is merely inconvenient,
it's not really hurting us big time.
<kfogel> It would have taken a long time to clean everything up,
and I'm sure we still would have found problems afterwards.
<kfogel> This way, we're finding the problems, but at least
Subversion is out there for people to use.
<kfogel> 2.0 will have this stuff fixed.
Heh. So anyway, the point is, we have to rev this structure now,
you're right. But let's also add an allocator function:
svn_client_create_commit_info()
or whatever, that takes a pool and returns a structure, and then
document in the structure declaration that one should *only* pass such
a structure to Subversion libraries when one has allocated it via this
new constructor function. That way, in the future, we can add new
fields to the end of the structure without worrying about binary
compatibility.
> (svn_client_commit3): New API that uses
> svn_client_commit_info2_t.
> (svn_client_copy2): New API that uses svn_client_commit_info2_t.
> * subversion/include/svn_ra.h
> (svn_ra_find_protocol): New API declaration added.
Say "New prototype." for svn_ra_find_protocol().
> (svn_ra_get_commit_editor2): New API that uses the new callback.
Don't just say "the new callback", say exactly which one.
> * subversion/libsvn_ra_local/ra_plugin.c
> (ra_local_vtable): ra_local's vtable does NOT have callback2.
This is a statement of fact, but it's not a description of a change
you made to ra_local_vtable. What did you *do* to it, that warranted
mentioning it in a log message?
> * subversion/libsvn_client/delete.c
> (delete_urls): delete_urls now uses the
> svn_client_commit_info2_t structure.
> Call svn_ra_get_commit_editor2() if ra_dav.
> Call svn_ra_get_commit_editor() otherwise.
> (svn_client_delete): svn_client_delete should now use
> svn_client_commit_info2_t.
Again, no need to repeat the name of the function in the entry for
that function. Just say "Use the new svn_client_commit_info2_t
structure.".
> * subversion/libsvn_client/client.h
> (svn_client__commit_get_baton): Should now use the new commit
> info structrure - svn_client_commit_info2_t.
Heh, here you say "the new commit info structure" but then you name
it. Get rid of the lead-up, and just use the name in the first place.
> (svn_client__commit_callback2): New callback that now takes
> post_commit_err as one of its parameters.
A better way to phrase this is:
(svn_client__commit_callback2): New callback, like
svn_client__commit_callback but takes post_commit_err parameter.
(Similar advice applies to various other items in the log message.)
> * subversion/libsvn_client/copy.c
> (repos_to_repos_copy): Now uses the svn_client_commit_info2_t
> structure.
> Call svn_ra_get_commit_editor2() if ra_dav.
> Call svn_ra_get_commit_editor() otherwise.
> (wc_to_repos_copy): Now uses the svn_client_commit_info2_t
> structure.
> Call svn_ra_get_commit_editor2() if ra_dav.
> Call svn_ra_get_commit_editor() otherwise.
> (setup_copy): Now uses svn_client_commit_info2_t.
> (svn_client_copy2): New version of API that takes a
> svn_client_commit_info2_t for commit_info.
> (svn_client_move3): New version of API that takes a
> svn_client_commit_info2_t for commit_info.
> * subversion/libsvn_client/commit_util.c
> (commit_baton): Now uses svn_client_commit_info2_t.
> (svn_client__commit_get_baton): Now uses
> svn_client_commit_info2_t.
> (svn_client__commit_callback2): This callback function
> fills the svn_client_commit_info2_t structure with the
> post_commit_err, if any.
Again, the important thing to say in the log message is that it is a
new function:
(svn_client__commit_callback2): New function.
If you can place that near its declaration, that's even better. The
description of what the function does can be left for the code. The
log message should contain mainly high-level, general statements
indicating the nature of the change -- detailed descriptions belong in
the code itself.
> * subversion/libsvn_client/add.c
> (mkdir_urls): Now uses svn_client_commit_info2_t.
> Calls svn_ra_get_commit_editor2() if ra_dav.
> Calls svn_ra_get_commit_editor() otherwise.
> (svn_client_mkdir): Now uses svn_client_commit_info2_t.
> * subversion/libsvn_client/commit.c
> (get_ra_editor): Now uses svn_client_commit_info2_t.
> Calls svn_ra_get_commit_editor2() if ra_dav.
> Calls svn_ra_get_commit_editor() otherwise.
> (svn_client_import2): get_ra_editor now passed typecasted
> commit_info.
I see where you're going with this casting strategy, but IMHO, we've
decided to deprecate 'svn_client_commit_info_t' in favor of
'svn_client_commit_info2_t', so therefore we should be replacing *all*
uses of svn_client_commit_info_t... which means rev'ving every API
that currently takes svn_client_commit_info_t. I don't see any clean
way to avoid this, unfortunately. (Casting is not clean :-) ).
> (svn_client_commit3): New version of API that uses
> svn_client_commit_info2_t.
> (svn_client_commit2): A svn_client_commit_info2_t is allocated
> and passed back as svn_client_commit_info_t. This doesn't matter
> as svn_client_commit_info2_t is one member more than
> svn_client_commit_info_t.
Clever plan, yup -- I totally understand why it's tempting. But see
above about the need to really deprecate if we're going to deprecate.
> * subversion/mod_dav_svn/merge.c
> (#include): Included svn_xml.h
> (dav_svn__merge_response): Now takes an extra post_commit_err
> parameter. If the post_commit_err is available,
> - Adds the SVN namespace
> - post-commit-err element to hold the post_commit_err
> to the merge response.
Here's a rewrite of that bit:
* subversion/mod_dav_svn/merge.c: Include svn_xml.h.
(dav_svn__merge_response): Take new post_commit_err parameter,
package it in "post-commit-err" namespace.
> * subversion/mod_dav_svn/dav_svn.h
> (dav_svn__merge_response): Now takes a new post_commit_err
> parameter.
> * subversion/mod_dav_svn/version.c
> (dav_svn_merge): If post-commit hook returns an error,
> pass it along to dav_svn__merge_response.
> * subversion/clients/cmdline/cl.h
> (svn_cl__print_commit_info2): Declaration for the new function
> that ultimately prints out the post-commit hook's stderr to
> the user.
You would just say "New prototype.". But:
This is an internal private function -- that's why it has double
underscore. So, you can just add the parameter, no need to create a
new2() version of the function. It's not part of the public API.
> * subversion/clients/cmdline/mkdir-cmd.c
> (svn_cl__mkdir): Now uses svn_client_commit_info2_t and
> svn_cl__print_commit_info2.
> * subversion/clients/cmdline/move-cmd.c
> (svn_cl__move): Now uses svn_client_commit_info2_t and
> svn_cl__print_commit_info2.
> * subversion/clients/cmdline/copy-cmd.c
> (svn_cl__copy): Now uses svn_client_commit_info2_t and
> svn_cl__print_commit_info2.
> * subversion/clients/cmdline/util.c
> (svn_cl__print_commit_info2): Home at last!!! New version of
> API.
> svn_cl__print_commit_info2 is the function that ultimately
> prints out the post_commit_error to the user.
> * subversion/clients/cmdline/commit-cmd.c
> (svn_cl__commit): Now uses svn_client_commit_info2_t and
> svn_cl__print_commit_info2 for callback.
> * subversion/clients/cmdline/delete-cmd.c
> (svn_cl__delete): Now uses svn_client_commit_info2_t and
> svn_cl__print_commit_info2 for callback.
> * subversion/tests/clients/cmdline/commit_tests.py
> (post_commit_hook_test): New function to test the post-commit
> hook's propogation of stderr to the client.
> (test_list): Added post_commit_hook_test to list of tests.
> * subversion/libsvn_repos/hooks.c
> (svn_repos__hooks_post_commit): Now runs the post-commit hook
> with the read_errstream parameter as TRUE.
> * subversion/libsvn_repos/commit.c
> (edit_baton): Added new member - callback2, to hold a callback
> of type svn_commit_callback2_t. Also added comments that
> callback
> should be removed once all code uses callback2.
> (close_edit): Handles the post-commit stderr. Decides whether
> to use the callback or the callback2 member of the edit baton.
> (svn_repos_get_commit_editor3): Uses svn_commit_callback2_t for
> callback type, and fills callback2 of the edit baton.
> (svn_repos_get_commit_editor2): Still uses svn_commit_callback_t
> and fills the callback member of the edit baton.
So was a change made to svn_repos_get_commit_editor2() or not? I
think what you want to say is just:
(svn_repos_get_commit_editor2): Deprecate.
And that should appear in the log message right after the item about
the new svn_repos_get_commit_editor3() API.
> * subversion/libsvn_ra_svn/client.c
> (ra_svn_vtable): ra_svn's vtable doesnt have a
> get_commit_editor2 member.
> * subversion/libsvn_ra_dav/merge.c
> (merge_elements): Added the post-commit-err element.
> (merge_ctx_t): Added the post_commit_err member.
> (validate_element): Make ELEM_post_commit_err a valid element.
> (end_element): Add case, and handle ELEM_post_commit_err.
> (svn_ra_dav__merge_activity2): New merge function to handle
> post-commit hook's stderr.
> * subversion/libsvn_ra_dav/ra_dav.h
> (svn_ra_dav__get_commit_editor2): New version of API, using
> svn_commit_callback2_t.
> (enum): Added ELEM_post_commit_err member.
> (svn_ra_dav__merge_activity2): Declaration for new version of
> API.
> * subversion/libsvn_ra_dav/session.c
> (dav_vtable): Now contains a get_commit_editor2 function, but no
> get_commit_editor function.
> * subversion/libsvn_ra_dav/commit.c
> (commit_ctx_t): Added the callback2 member.
> (commit_close_edit): Now uses te svn_ra_dav__merge_activity2
> function.
> (svn_ra_dav__get_commit_editor2): Now fills the callback2 member
> (svn_ra_dav__get_commit_editor): Still fills the callback member
> ]]]
Okay, at this point enough questions have been raised in the log
message that I think it doesn't make sense for me to review the patch.
Instead, can you produce a new patch that
* Avoids the need for svn_ra_find_protocol() and the conditionals
resulting therefrom.
* Really deprecates the things it deprecates, everywhere, and revs
APIs as necessary to compensate.
* Provides a constructor function for any structures we had to rev,
and documents on the new versions of the structures that they
should always be allocated by the constructor, so that at least
future revs will not be necessary.
* Has a log message in the more compact, more standard style that
I've tried to point out above.
Or, if any of these things (especially the first three) seem like bad
ideas to you, please explain why, and we'll figure out the best design
on the list here.
Hope this helps,
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org