On Wed, 2009-08-12 at 13:58 +0800, yellow.flying wrote:
> Hey Hyrum, Julian,
>
> I rewrite the patch as you like and have tested it, see below:
Thanks, Huihuang.
I have now reviewed the whole patch and it all looks good to me except
for one thing: the "loggy_path" change.
> Log:
> [[[
> Rip out some adm_access usage in svn_wc_translated_file3().
>
> * subversion/include/svn_wc.h
> (svn_wc_translated_file3): New.
> (svn_wc_translated_file2): Deprecate.
>
> * subversion/libsvn_wc/deprecated.c
> (svn_wc_translated_file2): Reimplement as a wrapper.
>
> * subversion/libsvn_wc/translate.c
> (svn_wc__internal_translated_file): New.
> (svn_wc_translated_file3): New.
> (svn_wc_translated_file2): Remove.
>
> * subversion/libsvn_wc/translate.h
> (svn_wc__internal_translated_file): New.
>
> * subversion/libsvn_wc/log.c
> (loggy_path): Add a apr_pool_t * parameter and make it able to deal with absolute paths.
> ]]]
> Index: subversion/libsvn_wc/deprecated.c
> ===================================================================
[...]
> +svn_error_t *
> +svn_wc_translated_file2(const char **xlated_path,
> + const char *src,
> + const char *versioned_file,
> + svn_wc_adm_access_t *adm_access,
> + apr_uint32_t flags,
> + apr_pool_t *pool)
> +{
> + const char *versioned_abspath;
> + const char *root;
> + const char *tmp_root;
> + svn_wc_context_t *wc_ctx;
> +
> + SVN_ERR(svn_dirent_get_absolute(&versioned_abspath, versioned_file, pool));
> + SVN_ERR(svn_wc__context_create_with_db(&wc_ctx, NULL,
> + svn_wc__adm_get_db(adm_access),
> + pool));
> +
> + SVN_ERR(svn_wc_translated_file3(xlated_path, src, wc_ctx, versioned_abspath,
> + flags, pool, pool));
> + if (! svn_dirent_is_absolute(versioned_file))
> + {
> + SVN_ERR(svn_io_temp_dir(&tmp_root, pool));
> + if (! svn_dirent_is_child(tmp_root, *xlated_path, pool))
> + {
> + SVN_ERR(svn_dirent_get_absolute(&root, "", pool));
> + *xlated_path = svn_dirent_is_child(root, *xlated_path, pool);
> + }
> + }
> +
> + return svn_error_return(svn_wc_context_destroy(wc_ctx));
> +}
I wasn't sure at first whether this big "if ()" block really produces
the desired relative path. To check that this wrapper function really
does produce the same result as the previous version, I added some
debugging code to output the result of each call to
svn_wc_translated_file2(), and I compared these results with and without
your patch. They were identical. That's good.
(I compared the results produced during all the tests in diff_tests.py,
log_tests.py, update_tests.py, merge_tests.py.)
> Index: subversion/libsvn_wc/log.c
> ===================================================================
> --- subversion/libsvn_wc/log.c （版本 38693）
> +++ subversion/libsvn_wc/log.c （工作副本）
> @@ -1729,12 +1729,31 @@
> * directory. PATH must not be outside that directory. */
> static const char *
> loggy_path(const char *path,
> - svn_wc_adm_access_t *adm_access)
> + svn_wc_adm_access_t *adm_access,
> + apr_pool_t *pool)
> {
> + const char *adm_abspath = NULL;
> + const char *abspath = NULL;
> const char *adm_path = svn_wc_adm_access_path(adm_access);
> - const char *local_path = svn_dirent_is_child(adm_path, path, NULL);
> + const char *local_path = NULL;
> + svn_error_t *err1 = NULL, *err2 = NULL;
>
> - if (! local_path && strcmp(path, adm_path) == 0)
> + err1 = svn_dirent_get_absolute(&adm_abspath, adm_path, pool);
> + if (err1)
> + {
> + svn_error_clear(err1);
> + return NULL;
> + }
> + err2 = svn_dirent_get_absolute(&abspath, path, pool);
> + if (err2)
> + {
> + svn_error_clear(err2);
> + return NULL;
> + }
> +
> + local_path = svn_dirent_is_child(adm_abspath, abspath, NULL);
> +
> + if (! local_path && strcmp(abspath, adm_abspath) == 0)
> local_path = SVN_WC_ENTRY_THIS_DIR;
>
> return local_path;
This function should never return NULL. To catch the possible errors
from "..._get_absolute()" I think you will have to change the function's
prototype to return an error, and provide the result through an output
argument.
Don't initialize variables to NULL when they are going to be initialized
to the correct value later. Doing so prevents compilers and "valgrind"
from detecting use before initialization.
This comment at the top of the file should be updated to mention that
the paths can now be absolute:
/* The svn_wc__loggy_* functions in this section take path arguments
with the same base as with which the adm_access was opened.
*/
This whole "loggy_path()" change is big enough and independent enough
that it could usefully be a separate patch.
Thanks.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2382865
Received on 2009-08-12 13:57:35 CEST