Madan U Sreenivasan wrote:
> On Thu, 26 Oct 2006 12:31:38 +0530, Kamesh Jayachandran
> <kamesh@collab.net> wrote:
>
>>
>>>>> +static svn_error_t *
>>>>> +get_src_merge_info(svn_ra_session_t *ra_session,
>>>>> + apr_hash_t **src_mergeinfo,
>>>>> + const char *src_path,
>>>>> + svn_revnum_t src_rev,
>>>>> + apr_pool_t *pool)
>>>>>
>>>> May be we should export this function so that
>>>> subversion/libsvn_client/diff.c:get_wc_target_merge_info can make
>>>> use of the same. I could see the scope for its usage when we might
>>>> have 'mergeaudit' subcommands.
>>>
>>> Could you explain a bit more in detail. All this function does is
>>> call svn_ra_get_merge_info(), and wraps it up in a mergeinfo hash.
>> Yes we have a similar code lying out in the function
>> 'get_wc_target_merge_info' inside subversion/libsvn_client/diff.c. If
>> you make get_src_merge_info public(that is what I meant when I said
>> export(static functions are not exported) it ) we can reuse it there
>> and possible future consumers like subcommands like 'svn mergeaudit'.
>> May be we can call it svn_client_get_mergeinfo_for_single_path() ??
>
> No, these are not the same...
<snip from libsvn_client/diff.c>
APR_ARRAY_PUSH(mergeinfo_paths, const char *) = repos_rel_path;
/* ### TODO: To handle sub-tree merge info, the list will need to
### include the those child paths which have merge info which
### differs from that of TARGET_WCPATH, and if those paths are
### directories, their children as well. */
SVN_ERR(svn_ra_get_merge_info(ra_session, &repos_mergeinfo,
mergeinfo_paths,
target_rev, TRUE, pool));
//This line is independent of the earlier one and the following one so
can be moved above or below.
SVN_ERR(parse_merge_info(target_mergeinfo, *entry, target_wcpath,
adm_access, ctx, pool));
if (repos_mergeinfo != NULL)
{
apr_hash_t *target_repos_mergeinfo =
apr_hash_get(repos_mergeinfo, repos_rel_path, APR_HASH_KEY_STRING);
</snip>
<snip from your patch>
apr_hash_t *merge_info;
apr_array_header_t *rel_paths = apr_array_make(pool, 1,
sizeof(src_path));
APR_ARRAY_PUSH(rel_paths, const char *) = src_path;
SVN_ERR(svn_ra_get_merge_info(ra_session, &merge_info, rel_paths,
src_rev, TRUE, pool));
/* Dereference to obtain only the merge info of the src_path provided */
if (merge_info)
*src_mergeinfo = apr_hash_get(merge_info, src_path, APR_HASH_KEY_STRING);
else
*src_mergeinfo = NULL;
return SVN_NO_ERROR;
</snip>
I meant to reuse this implementation in
libsvn_client/diff.c:get_wc_target_merge_info.
Hope I made it clear.
>
>>>>> +{
>>>>> + /* Dereference to obtain only the merge info of the src_path
>>>>> provided */
>>>>> + if (merge_info)
>>>>>
>>>> No need to check for nullity of merge_info. Unless rel_paths->nelts
>>>> == 0 merge_info will never be null.
>>>
>>> We need to ensure that src_mergeinfo is set to NULL, thats why the
>>> null-check.
>> No need to ensure that. Across our code, atleast the places where I
>> have seen, the callee always sets the apr_hash_t*. Checking for NULL
>> there is redundancy.
>>
>> In this particular case refer 'svn_repos_fs_get_merge_info' in
>> subversion/libsvn_repos/fs-wrap.c you will realize that
>> 'src_mergeinfo' will never be NULL.
>
> Please read the code. We also have to extract the inner-hash of the
> mergeinfo. To do this, we have to know whether the mergeinfo hash is
> non-NULL before we can call the apr_hash_get() function.
Please read the implementation of the following carefully.
a) 'svn_repos_fs_get_merge_info' in subversion/libsvn_repos/fs-wrap.c
b)svn_fs_merge_info__get_merge_info in
subversion/libsvn_fs_util/merge-info-sqlite-index.c
You will understand that for this case(you have some meaningful stuff
inside your rel_paths array) that you work with 'mergeinfo' hash will
never be null, as with many of our library functions having
'apr_hash_t**' kind of out params.
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Oct 26 21:02:27 2006