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! :-)

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.

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_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.

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.

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.

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.