Re: svn commit: r21825 - in branches/incomplete-directories/subversion: include libsvn_client libsvn_ra_svn libsvn_wc svn

Thanks for the review, Kamesh! I've committed some fixes to the
branch due to your comments. A few responses below:

Kamesh Jayachandran <kamesh@collab.net> writes:
> 1)svn_ra_do_diff3 still has the member name as recurse of type
> svn_boolean_t should it not be svn_depth_t now?

Yup, I forgot to change the prototype. The compiler doesn't know the
difference between an enum and an int and a boolean, unfortunately, so
it doesn't warn about this sort of error. I guess I'll actually need
to learn to code! :-)

> 2)svn_wc_adm_open_anchor has 'depth' of type 'int' should it not be
> svn_depth_t'?

Ah, no, that depth is different (and probably should use a different
name, but I don't want to go down that route right now). See the doc
string there for details.

> 'do_merge' calls 'svn_client__get_diff_editor' with the new signature.
> 'do_merge' calls 'svn_ra_do_diff3' with 'depth' rather than 'recurse'.

Yes, these are correct. svn_client__get_diff_editor has the new
signature, and svn_ra_do_diff3 does too (except for my oversight in
revving its prototype, which you caught).

>> (svn_client_diff4, svn_client_diff_peg3, svn_client_diff_summarize2,
>> svn_client_diff_summarize_peg2, svn_client_merge3, svn_client_merge_peg3):
>> Define new functions, taking depth.
>> (svn_client_diff3, svn_client_diff_peg2, svn_client_diff_summarize,
>> svn_client_diff_summarize_peg, svn_client_merge2,
>> svn_client_merge_peg2): Reimplement as wrappers.
>>
>>
> Signature change of diff_repos_wc of is not logged.

Fixed log message.

> It should also have info about the change in call to
> svn_wc_adm_open_anchor'.

Well, I think that follows from the parameter change (elsewhere in the
log message, I don't enumerate all the consequent changes).

> Should we not call 'svn_wc_get_diff_editor4' than
> svn_wc_get_diff_editor3' here?

Yup. Fixed.

> It should have the info about the change in call to 'svn_ra_do_diff3'.
> It should have the info about the change in call to
> svn_wc_crawl_revisions3'

Same about being an implied change. (Sometimes I list them, sometimes
I don't. Lately I lean toward not listing them, as it bulks up the
log message without contributing to understanding the change, IMHO.)

>> * subversion/libsvn_client/client.h
>> (svn_client__get_diff_editor): Take depth instead of recurse.
>>
> Should callers of this new routine be logged?

They're already listed as having changed from recurse to depth anyway,
so this part is implied.

Thanks for catching my oversights. I've fixed the r21825 log message,
and committed r21856 for the code changes.

-Karl

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Oct 9 22:21:02 2006