On Wed, 27 Sep 2006 16:40:59 +0530, Kamesh Jayachandran
<kamesh@collab.net> wrote:
>
>> +/* Log callback for obtaining the copyfrom revision of + a given
>> path. */
>> +static svn_error_t *
>> +copyfrom_receiver(void *baton,
>> + apr_hash_t *changed_paths,
>> + svn_revnum_t revision,
>> + const char *author,
>> + const char *date,
>> + const char *message,
>> + apr_pool_t *pool)
>> +{
>> + svn_revnum_t *svn_revnum = baton;
>> + *svn_revnum = revision;
>>
> I could see duplicate assignment here ???
No, the first statement assigns the address and the second one assigns a
value.
>> + return SVN_NO_ERROR;
>> +}
>> +
>> +/* Obtain the copyfrom mergeinfo of the given path. */
>> static svn_error_t *
>> +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)
>> +{
>> + svn_revnum_t copyfrom_rev;
>> + svn_merge_range_t *copyfrom_mergerange;
>> + apr_array_header_t *copyfrom_mergerange_list;
>> + apr_array_header_t *rel_paths = apr_array_make(pool, 1,
>> + sizeof(const char *));
>> +
>> + *copyfrom_mergeinfo = NULL;
>> +
>> + APR_ARRAY_PUSH(rel_paths, const char *) = rel_path;
>> +
>> + /* Extract loginfo for finding the copyfrom rev */
>> + SVN_ERR(svn_ra_get_log(ra_session, rel_paths, 1, src_revnum,
>> + 1, FALSE, TRUE, copyfrom_receiver,
>> + ©from_rev, pool));
>> +
>> + copyfrom_mergerange = apr_palloc(pool, sizeof(*copyfrom_mergerange));
>> + copyfrom_mergerange->start = copyfrom_rev;
>> + copyfrom_mergerange->end = src_revnum;
>> + copyfrom_mergerange_list = apr_array_make(pool, 1,
>> +
>> sizeof(copyfrom_mergerange));
>> + APR_ARRAY_PUSH(copyfrom_mergerange_list, svn_merge_range_t *)
>> + = copyfrom_mergerange;
>> + *copyfrom_mergeinfo = apr_hash_make(pool);
>> + apr_hash_set(*copyfrom_mergeinfo, copyfrom_path,
>> + sizeof(copyfrom_mergerange_list),
>> copyfrom_mergerange_list);
>> +
>>
> Should it not be
>
> apr_hash_set(*copyfrom_mergeinfo, copyfrom_path,
> APR_HASH_KEY_STRING, copyfrom_mergerange_list);
hmm, yes. I misread the docs. will change. Thanks.
I will wait for other review comments before sending a new patch.
>> + return SVN_NO_ERROR;
>> +}
>> +
>> +/* Obtain the copyfrom mergeinfo and the existing mergeinfo of
>> + 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,
>> + const char *src_rel,
>> + 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 */
>> + SVN_ERR(svn_ra_get_repos_root(ra_session, &repos_root, pool));
>> + while (*copyfrom_path++ == *repos_root++ );
>> + copyfrom_path--;
>>
> I prefer preincrement as it does not need a temp copy of variable, may
> be the compiler could be intelligent in making them preincrements in
> void contexts like this.
> Why not
>
> while (*(--copyfrom_path) == *(--repos_root) );
you mean pre-*increment*, right?
pre-increment will go against the logic in this case (we have to check
equality character by character). Preincrement will avoid check the first
character.
> --copyfrom_path;
I could use predecrement for the second line, but IMHO, thats really not
of much significance. If you really feel strong about this, lets change
this.
Regards,
Madan.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Sep 27 13:29:26 2006