[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH][MERGE-TRACKING] Step 2 of repos to repos copyfrom info recording

From: Madan U Sreenivasan <madan_at_collab.net>
Date: 2006-09-28 08:58:19 CEST

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:


>> +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

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.


>> 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
>> +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?


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


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

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.