Bert said on IRC: "julianf: I would recommend fixing the api implementation (to follow our common rule: all passed paths are canonical, unless documented otherwise) and just backporting the fix."
The minimal fix is committed to trunk in r1691928. Note that this fixes the *only* caller that exists so far that didn't canonicalize the resulting path. A better fix, but one less suitable for back-porting, is committed in r1691952. That makes svn_user_get_homedir() always return a canonical path. I tried writing a unit test for it, and came up with the attached 'test_svn_user_get_homedir.patch'. I don't like it much: it messes with the 'HOME' variable in the process's environment, and although it makes some attempt to avoid parallel execution and tries to reset the variable afterwards, it doesn't do so in all cases. Notably it errors out if the 'HOME' env var is unset to start with. Do we want a unit test something like this, and if so how should I make it more Good? I still intend to nominate the minimal fix r1691928 for backport to 1.8.x and 1.9.x. - Julian On 20 July 2015 at 14:05, Julian Foad <julianf...@btopenworld.com> wrote: > I (Julian Foad) wrote: >> I have a one-line fix for this issue, which I've tested by hand: >> [...] >> I don't have an automated regression test. Should I go ahead and commit >> the fix anyway, and propose for backport to 1.8 and 1.9? > > Of course, we would like to have a regression test. Can anyone help > me write one? I took a quick look in > subversion/tests/libsvn_subr/auth-test.c for inspiration and am a bit > lost where to start at that level. > > If we go for a 'proper' fix, we could write a unit test for > svn_user_get_homedir(), which would be much easier than a regression > test for the authentication scenario with GPG-agent. > > - Julian
Add a unit test for svn_user_get_homedir(). * build.conf (user-test): New section. * subversion/tests/libsvn_subr/user-test.c New file. --This line, and those below, will be ignored-- Index: build.conf =================================================================== --- build.conf (revision 1691952) +++ build.conf (working copy) @@ -1113,12 +1113,20 @@ description = Test time functions type = exe path = subversion/tests/libsvn_subr sources = time-test.c install = test libs = libsvn_test libsvn_subr apriconv apr +[user-test] +description = Test user functions +type = exe +path = subversion/tests/libsvn_subr +sources = user-test.c +install = test +libs = libsvn_test libsvn_subr apriconv apr + [utf-test] description = Test UTF-8 functions type = exe path = subversion/tests/libsvn_subr sources = utf-test.c install = test Index: subversion/tests/libsvn_subr/user-test.c =================================================================== --- subversion/tests/libsvn_subr/user-test.c (nonexistent) +++ subversion/tests/libsvn_subr/user-test.c (working copy) @@ -0,0 +1,94 @@ +/* + * user-test.c -- test the user functions + * + * ==================================================================== + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * ==================================================================== + */ + +#include <apr_env.h> + +#include "../svn_test.h" +#include "svn_user.h" +#include "svn_pools.h" + + +#define APR_ERR(status_expr) \ + do { apr_status_t _apr_err = (status_expr); \ + if (_apr_err != APR_SUCCESS) \ + return svn_error_wrap_apr(_apr_err, NULL); \ + } while (0) + + +/* Values to use in test_svn_user_get_homedir(). */ +#ifdef WIN32 +#define HOME_NON_CANONICAL "C:\\user\\" +#define HOME_CANONICALIZED "C:\\user" +#else +#define HOME_NON_CANONICAL "/user/" +#define HOME_CANONICALIZED "/user" +#endif + +static svn_error_t * +test_svn_user_get_homedir(apr_pool_t *pool) +{ + char *old_home; + const char *homedir; + + /* Save the value of HOME so we can restore it at the end. + * ### This won't save and restore it correctly if it was undefined. */ + APR_ERR(apr_env_get(&old_home, "HOME", pool)); + + /* Test getting the home directory from HOME */ + APR_ERR(apr_env_set("HOME", HOME_NON_CANONICAL, pool)); + homedir = svn_user_get_homedir(pool); + if (! homedir) + return svn_error_createf(SVN_ERR_TEST_FAILED, NULL, + "svn_user_get_homedir() returned NULL when HOME defined"); + SVN_TEST_ASSERT(svn_dirent_is_canonical(homedir, pool)); + SVN_TEST_STRING_ASSERT(homedir, HOME_CANONICALIZED); + + /* Test getting the home directory when HOME is undefined */ + APR_ERR(apr_env_delete("HOME", pool)); + homedir = svn_user_get_homedir(pool); + if (! homedir) + return svn_error_createf(SVN_ERR_TEST_FAILED, NULL, + "svn_user_get_homedir() returned NULL when HOME undefined"); + SVN_TEST_ASSERT(svn_dirent_is_canonical(homedir, pool)); + printf("%s\n", homedir); + + /* Restore the original value so this test won't affect subsequent tests */ + APR_ERR(apr_env_set("HOME", old_home, pool)); + return SVN_NO_ERROR; +} + + + +/* The test table. */ + +static int max_threads = 1; + +static struct svn_test_descriptor_t test_funcs[] = + { + SVN_TEST_NULL, + SVN_TEST_PASS2(test_svn_user_get_homedir, + "test svn_user_get_homedir"), + SVN_TEST_NULL + }; + +SVN_TEST_MAIN Property changes on: subversion/tests/libsvn_subr/user-test.c ___________________________________________________________________ Added: svn:eol-style ## -0,0 +1 ## +native \ No newline at end of property
