Re: [PATCH][merge-tracking] get_merge_info_for_path eliding fails when grand parent has mergeinfo or grand parent has null mergeinfo.

Kamesh Jayachandran wrote:
> Today I reposted this patch with almost same subject(with Take2
> appended) which has one small correction to this one.

Thanks. I've got the new patch, I just hadn't made it that far down the
queue.

> Hyrum K. Wright wrote:
>> Kamesh Jayachandran wrote:
>>
>>> Hi All,
>>> Find the attached patch and log.
>>>
>>
>> Ping...
>>
>> Has anybody taken a look at Kamesh's patch? If no one comments in the
>> next few days, I'll file it as an issue.
>>
>> Well, I'll probably give is a little longer than normal, given the
>> Summit and all. Just wanted to make sure this wasn't forgotten.
>>
>> Thanks,
>> -Hyrum
>>
>>
>>> ------------------------------------------------------------------------
>>>
>>> [[[
>>> Patch by: Kamesh Jayachandran <kamesh@collab.net>
>>>
>>> get_merge_info_path fails in the following cases,
>>> a)if the path is '/a/b/c/d/e' and the repo has the mergeinfo only
>>> for '/a/b/c' and no direct mergeinfo for '/a/b/c/d'.
>>> b)if the path is '/a/b/c/d/e' and the repo has null
>>> mergeinfo(meaning record in mergeinfo_changed no record in mergeinfo)
>>> for '/a/b/c' and no direct mergeinfo for '/a/b/c/d'. The moment
>>> 'null mergeinfo' is encountered eliding has to stop.
>>>
>>> * subversion/libsvn_fs_util/merge-info-sqlite-index.c
>>> (get_merge_info_for_path): No need to flag null mergeinfo with
>>> 'has_no_mergeinfo' they can be better signalled with
>>> NEGATIVE_CACHE_RESULT for the 'path' in the cache hash.
>>> No need of 'pathresult' variable as we can use 'cacheresult' itself.
>>> When we have null merge info signal the same by setting 'cache'
>>> hash for the
>>> key 'path' with NEGATIVE_CACHE_RESULT.
>>> As long as we have a record in 'mergeinfo_changed' for this path
>>> and revision<=rev return no need to recurse further.
>>> Irrespective of 'result' translate the hash keys of 'elided
>>> parent->path'
>>> and set it to cache, so that we don't loose the 'mergeinfo' of
>>> '/a/b/c' during unwinding phase of recursion with path '/a/b/c/d'.
>>> ]]]
>>>
>>>
>>>
>>> ------------------------------------------------------------------------
>>>
>>> Index: subversion/libsvn_fs_util/merge-info-sqlite-index.c
>>> ===================================================================
>>> --- subversion/libsvn_fs_util/merge-info-sqlite-index.c (revision
>>> 21879)
>>> +++ subversion/libsvn_fs_util/merge-info-sqlite-index.c (working
>>> copy)
>>> @@ -400,7 +400,6 @@
>>> sqlite3_stmt *stmt;
>>> int sqlite_result;
>>> sqlite_int64 count;
>>> - svn_boolean_t has_no_mergeinfo = FALSE;
>>>
>>> cacheresult = apr_hash_get(cache, path, APR_HASH_KEY_STRING);
>>> if (cacheresult != 0)
>>> @@ -430,26 +429,23 @@
>>> mergeinfo hash */
>>> if (count > 0)
>>> {
>>> - apr_hash_t *pathresult = NULL;
>>> -
>>> - SVN_ERR(parse_mergeinfo_from_db(db, path, rev, &pathresult,
>>> pool));
>>> - if (pathresult)
>>> + SVN_ERR(parse_mergeinfo_from_db(db, path, rev, &cacheresult,
>>> pool));
>>> + if (cacheresult)
>>> {
>>> if (result)
>>> - apr_hash_set(result, path, APR_HASH_KEY_STRING,
>>> pathresult);
>>> - apr_hash_set(cache, path, APR_HASH_KEY_STRING, pathresult);
>>> + apr_hash_set(result, path, APR_HASH_KEY_STRING,
>>> cacheresult);
>>> + apr_hash_set(cache, path, APR_HASH_KEY_STRING, cacheresult);
>>> }
>>> else
>>> - has_no_mergeinfo = TRUE;
>>> + apr_hash_set(cache, path, APR_HASH_KEY_STRING,
>>> NEGATIVE_CACHE_RESULT);
>>> + return SVN_NO_ERROR;
>>> }
>>>
>>> /* If this path has no mergeinfo, and we are asked to, check our
>>> parent */
>>> - if ((count == 0 || has_no_mergeinfo) && include_parents)
>>> + if ((count == 0) && include_parents)
>>> {
>>> svn_stringbuf_t *parentpath;
>>>
>>> - apr_hash_set(cache, path, APR_HASH_KEY_STRING,
>>> NEGATIVE_CACHE_RESULT);
>>> -
>>> /* It is possible we are already at the root. */
>>> if (strcmp(path, "") == 0)
>>> return SVN_NO_ERROR;
>>> @@ -465,23 +461,20 @@
>>> SVN_ERR(get_merge_info_for_path(db, parentpath->data, rev,
>>> NULL, cache, include_parents,
>>> pool));
>>> - if (result)
>>> + cacheresult = apr_hash_get(cache, parentpath->data,
>>> APR_HASH_KEY_STRING);
>>> + if (cacheresult == NEGATIVE_CACHE_RESULT)
>>> + apr_hash_set(cache, path, APR_HASH_KEY_STRING, NULL);
>>> + else if (cacheresult)
>>> {
>>> /* Now translate the result for our parent to our path */
>>> - cacheresult = apr_hash_get(cache, parentpath->data,
>>> - APR_HASH_KEY_STRING);
>>> - if (cacheresult == NEGATIVE_CACHE_RESULT)
>>> - apr_hash_set(result, path, APR_HASH_KEY_STRING, NULL);
>>> - else if (cacheresult)
>>> - {
>>> - apr_hash_t *translatedhash;
>>> - const char *toappend = &path[parentpath->len + 1];
>>> + apr_hash_t *translatedhash;
>>> + const char *toappend = &path[parentpath->len + 1];
>>>
>>> - append_component_to_paths(&translatedhash, cacheresult,
>>> - toappend, pool);
>>> - apr_hash_set(result, path, APR_HASH_KEY_STRING,
>>> - translatedhash);
>>> - }
>>> + append_component_to_paths(&translatedhash, cacheresult,
>>> + toappend, pool);
>>> + apr_hash_set(cacheresult, path, APR_HASH_KEY_STRING,
>>> translatedhash);
>>> + if (result)
>>> + apr_hash_set(result, path, APR_HASH_KEY_STRING,
>>> translatedhash);
>>> }
>>> }
>>> return SVN_NO_ERROR;