Madan U Sreenivasan <madan@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
Received on Wed Jul 6 21:20:34 2005