Agree and will act on most of the comments. Thank you. Regarding the rest,
please find comments inline.
On Thu, 28 Sep 2006 05:41:43 +0530, Daniel Rall <dlr@collab.net> wrote:
> On Wed, 27 Sep 2006, Madan S. wrote:
[snip]
>> +get_copyfrom_mergeinfo(svn_ra_session_t *ra_session,
>> + apr_hash_t **copyfrom_mergeinfo,
>> + const char *rel_path,
>> + const char *copyfrom_path,
>> + svn_revnum_t src_revnum,
>> + apr_pool_t *pool)
>
> I think more of our APIs use an extra underscore in the
> xxx_merge_info() function names:
>
> get_copyfrom_merge_info()
I agree we have to be consistent in our naming. But a quick run though
svn_mergeinfo.h tells me that none of our mergeinfo APIs use a _merge_info
convention.
I also ran a cscope search for the strings _mergeinfo and _merge_info
(please note that this includes function declarations, calls and even doc
strings containing the strings):
_merge_info: 67 occurances
_mergeinfo: 198 occurances
Personally I think that _ is an extra byte giving no extra information.
Just like we mostly use copyfrom instead of copy_from. We could do away
with it. Please let me know what you think.
[snip]
>> sizeof(copyfrom_mergerange));
> ...
>> +/* Obtain the copyfrom mergeinfo and the existing mergeinfo of
>
> There should be a space between the words "merge" and "info" in the
> doc string.
doc string? Will this be visible when we generate the doxygen comments? or
do you just mean doc string (without implying doxygen). Could you please
tell me *how* this is important?
btw, will do the change... just wanting to know the reason, so that I can
use it for future judgement.
>> + the source path, merge them and set as a svn_string_t in
>> + TARGET_MERGEINFO. */
>> +static svn_error_t *
>> +calculate_target_mergeinfo(svn_ra_session_t *ra_session,
>> + svn_string_t **target_mergeinfo,
>> + const char *src_url,
>
> Isn't this COPYFROM_URL?
>
>> + const char *src_rel,
>
> SRC_REL_PATH, perhaps?
>
>> + svn_revnum_t src_revnum,
>> + apr_pool_t *pool)
>> +{
>> + const char *repos_root;
>> + const char *copyfrom_path = src_url;
>> + apr_hash_t *copyfrom_mergeinfo;
>> +
>> + /* Find src path relative to the repos root */
>
> Do we have code to do this anywhere else?
I dont think so. I checked again, and was not able to find equivalent code
anywhere else.
But, you are right... this is kinda generic. Should we make it an
svn_path.h API?
[snip]
>> + copyfrom_path--;
>
> Is this kosher if the "while" loop never executes? (Which might not be
> able to happen...)
Even if the strings are totally different, the pointer *has* to move at
least once according to the while loop. So, this should be okay, I think.
Regards,
Madan.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Sep 28 08:59:26 2006