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:
> 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;