You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ko...@apache.org on 2018/11/27 18:10:02 UTC
svn commit: r1847572 - in /subversion/trunk/subversion: libsvn_fs_fs/tree.c
tests/libsvn_fs/fs-test.c
Author: kotkov
Date: Tue Nov 27 18:10:01 2018
New Revision: 1847572
URL: http://svn.apache.org/viewvc?rev=1847572&view=rev
Log:
fsfs: Fix an issue with the DAG open_path() that was causing unexpected
SVN_ERR_FS_NOT_DIRECTORY errors when attempting to open a path with
`open_path_node_only | open_path_allow_null` flags.
The implication of this is that certain svn_fs_closest_copy() calls could be
returing unexpected errors as well. For the end user, this means that such
errors were possible, for example, during certain `svn update`s.
The root cause of this behavior is the implementation of the optimization
within the open_path() routine. The optimization attempts to do a cache
lookup of the prospective parent directory of the path to be opened.
If the cache lookup is successful, the parent node is assumed — but not
checked — to be a directory. The absense of the check was causing
unexpected errors instead of returning `NULL` in a case where:
- open_path() is called with `open_path_node_only | open_path_allow_null`
- the path to be opened points to a non-existing path, but its parent path
is a file
- the DAG node of the "parent" file is cached
See https://lists.apache.org/thread.html/693a95b0da834387e78a7f08df2392b634397d32f37428c81c02f8c5@%3Cdev.subversion.apache.org%3E
* subversion/libsvn_fs_fs/tree.c
(err_not_directory): New helper function factored out from...
(open_path): ...here. Check the kind of the DAG node for the prospective
parent returned from cache in the `open_path_node_only` optimization.
Handle the case where it is not a directory; utilize the new helper to
construct the appropriate error.
* subversion/tests/libsvn_fs/fs-test.c
(test_closest_copy_file_replaced_with_dir): New regression test.
(test_funcs): Run new test.
Modified:
subversion/trunk/subversion/libsvn_fs_fs/tree.c
subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tree.c?rev=1847572&r1=1847571&r2=1847572&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Tue Nov 27 18:10:01 2018
@@ -920,6 +920,25 @@ try_match_last_node(dag_node_t **node_p,
return SVN_NO_ERROR;
}
+/* Helper for open_path() that constructs and returns an appropriate
+ SVN_ERR_FS_NOT_DIRECTORY error. */
+static svn_error_t *
+err_not_directory(svn_fs_root_t *root,
+ const char *path,
+ apr_pool_t *scratch_pool)
+{
+ const char *msg;
+
+ msg = root->is_txn_root
+ ? apr_psprintf(scratch_pool,
+ _("Failure opening '%s' in transaction '%s'"),
+ path, root->txn)
+ : apr_psprintf(scratch_pool,
+ _("Failure opening '%s' in revision %ld"),
+ path, root->rev);
+
+ return svn_error_quick_wrap(SVN_FS__ERR_NOT_DIRECTORY(root->fs, path), msg);
+}
/* Open the node identified by PATH in ROOT, allocating in POOL. Set
*PARENT_PATH_P to a path from the node up to ROOT. The resulting
@@ -1016,12 +1035,26 @@ open_path(parent_path_t **parent_path_p,
SVN_ERR(dag_node_cache_get(&here, root, directory, pool));
/* Did the shortcut work? */
- if (here)
+ if (here && svn_fs_fs__dag_node_kind(here) == svn_node_dir)
{
apr_size_t dirname_len = strlen(directory);
path_so_far->len = dirname_len;
rest = path + dirname_len + 1;
}
+ else if (here)
+ {
+ /* The parent node is not a directory. We are looking for some
+ sub-path, so that sub-path will not exist. That will be o.k.
+ if we are just here to check for the path's existence, but
+ should result in an error otherwise. */
+ if (flags & open_path_allow_null)
+ {
+ *parent_path_p = NULL;
+ return SVN_NO_ERROR;
+ }
+ else
+ return err_not_directory(root, directory, pool);
+ }
}
}
@@ -1144,8 +1177,6 @@ open_path(parent_path_t **parent_path_p,
/* The path isn't finished yet; we'd better be in a directory. */
if (svn_fs_fs__dag_node_kind(child) != svn_node_dir)
{
- const char *msg;
-
/* Since this is not a directory and we are looking for some
sub-path, that sub-path will not exist. That will be o.k.,
if we are just here to check for the path's existence. */
@@ -1156,14 +1187,7 @@ open_path(parent_path_t **parent_path_p,
}
/* It's really a problem ... */
- msg = root->is_txn_root
- ? apr_psprintf(iterpool,
- _("Failure opening '%s' in transaction '%s'"),
- path, root->txn)
- : apr_psprintf(iterpool,
- _("Failure opening '%s' in revision %ld"),
- path, root->rev);
- SVN_ERR_W(SVN_FS__ERR_NOT_DIRECTORY(fs, path_so_far->data), msg);
+ return err_not_directory(root, path_so_far->data, iterpool);
}
rest = next;
Modified: subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_fs/fs-test.c?rev=1847572&r1=1847571&r2=1847572&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_fs/fs-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_fs/fs-test.c Tue Nov 27 18:10:01 2018
@@ -7369,6 +7369,78 @@ closest_copy_test_svn_4677(const svn_tes
return SVN_NO_ERROR;
}
+static svn_error_t *
+test_closest_copy_file_replaced_with_dir(const svn_test_opts_t *opts,
+ apr_pool_t *pool)
+{
+ svn_fs_t *fs;
+ svn_fs_txn_t *txn;
+ svn_fs_root_t *txn_root;
+ svn_fs_root_t *rev_root;
+ svn_revnum_t youngest_rev;
+ svn_fs_root_t *copy_root;
+ const char *copy_path;
+
+ /* Prepare a filesystem. */
+ SVN_ERR(svn_test__create_fs(&fs, "test-closest-copy-file-replaced-with-dir",
+ opts, pool));
+
+ youngest_rev = 0;
+
+ /* Modeled after the case described in the thread:
+ "[PATCH] A test for "Can't get entries" error"
+ https://lists.apache.org/thread.html/693a95b0da834387e78a7f08df2392b634397d32f37428c81c02f8c5@%3Cdev.subversion.apache.org%3E
+ */
+ /* r1: Add a directory with a file. */
+ SVN_ERR(svn_fs_begin_txn2(&txn, fs, youngest_rev, 0, pool));
+ SVN_ERR(svn_fs_txn_root(&txn_root, txn, pool));
+ SVN_ERR(svn_fs_make_dir(txn_root, "/A", pool));
+ SVN_ERR(svn_fs_make_file(txn_root, "/A/mu", pool));
+ SVN_ERR(test_commit_txn(&youngest_rev, txn, NULL, pool));
+ SVN_TEST_INT_ASSERT(youngest_rev, 1);
+
+ /* r2: Copy the directory. */
+ SVN_ERR(svn_fs_begin_txn2(&txn, fs, youngest_rev, 0, pool));
+ SVN_ERR(svn_fs_txn_root(&txn_root, txn, pool));
+ SVN_ERR(svn_fs_revision_root(&rev_root, fs, 1, pool));
+ SVN_ERR(svn_fs_copy(rev_root, "/A", txn_root, "/B", pool));
+ SVN_ERR(test_commit_txn(&youngest_rev, txn, NULL, pool));
+ SVN_TEST_INT_ASSERT(youngest_rev, 2);
+
+ /* r3: Delete the file. */
+ SVN_ERR(svn_fs_begin_txn2(&txn, fs, youngest_rev, 0, pool));
+ SVN_ERR(svn_fs_txn_root(&txn_root, txn, pool));
+ SVN_ERR(svn_fs_delete(txn_root, "/B/mu", pool));
+ SVN_ERR(test_commit_txn(&youngest_rev, txn, NULL, pool));
+ SVN_TEST_INT_ASSERT(youngest_rev, 3);
+
+ /* r4: Replace the file with a new directory containing a file. */
+ SVN_ERR(svn_fs_begin_txn2(&txn, fs, youngest_rev, 0, pool));
+ SVN_ERR(svn_fs_txn_root(&txn_root, txn, pool));
+ SVN_ERR(svn_fs_make_dir(txn_root, "/B/mu", pool));
+ SVN_ERR(svn_fs_make_file(txn_root, "/B/mu/iota", pool));
+ SVN_ERR(test_commit_txn(&youngest_rev, txn, NULL, pool));
+ SVN_TEST_INT_ASSERT(youngest_rev, 4);
+
+ /* Test a couple of svn_fs_closest_copy() calls; the second call used
+ to fail with an unexpected SVN_ERR_FS_NOT_DIRECTORY error. */
+
+ SVN_ERR(svn_fs_revision_root(&rev_root, fs, 2, pool));
+ SVN_ERR(svn_fs_closest_copy(©_root, ©_path, rev_root, "/B/mu", pool));
+
+ SVN_TEST_ASSERT(copy_root != NULL);
+ SVN_TEST_INT_ASSERT(svn_fs_revision_root_revision(copy_root), 2);
+ SVN_TEST_STRING_ASSERT(copy_path, "/B");
+
+ SVN_ERR(svn_fs_revision_root(&rev_root, fs, 4, pool));
+ SVN_ERR(svn_fs_closest_copy(©_root, ©_path, rev_root, "/B/mu/iota", pool));
+
+ SVN_TEST_ASSERT(copy_root == NULL);
+ SVN_TEST_ASSERT(copy_path == NULL);
+
+ return SVN_NO_ERROR;
+}
+
/* ------------------------------------------------------------------------ */
/* The test table. */
@@ -7513,6 +7585,8 @@ static struct svn_test_descriptor_t test
"test rep-sharing on content rather than SHA1"),
SVN_TEST_OPTS_PASS(closest_copy_test_svn_4677,
"test issue SVN-4677 regression"),
+ SVN_TEST_OPTS_PASS(test_closest_copy_file_replaced_with_dir,
+ "svn_fs_closest_copy after replacing file with dir"),
SVN_TEST_NULL
};
Re: svn commit: r1847572 - in /subversion/trunk/subversion:
libsvn_fs_fs/tree.c tests/libsvn_fs/fs-test.c
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Branko Čibej <br...@apache.org> writes:
> > + else
> > + return err_not_directory(root, directory, pool);
> > + }
> > }
> > }
> >
>
> Would be nice to wrap this return value in svn_error_trace(), as that
> makes it easier to find where the error came from.
Makes sense, committed in https://svn.apache.org/r1847596
Thanks,
Evgeny Kotkov
Re: svn commit: r1847572 - in /subversion/trunk/subversion:
libsvn_fs_fs/tree.c tests/libsvn_fs/fs-test.c
Posted by Branko Čibej <br...@apache.org>.
On 27.11.2018 19:10, kotkov@apache.org wrote:
> Author: kotkov
> Date: Tue Nov 27 18:10:01 2018
> New Revision: 1847572
>
> URL: http://svn.apache.org/viewvc?rev=1847572&view=rev
> Log:
> fsfs: Fix an issue with the DAG open_path() that was causing unexpected
> SVN_ERR_FS_NOT_DIRECTORY errors when attempting to open a path with
> `open_path_node_only | open_path_allow_null` flags.
[...]
> @@ -1016,12 +1035,26 @@ open_path(parent_path_t **parent_path_p,
> SVN_ERR(dag_node_cache_get(&here, root, directory, pool));
>
> /* Did the shortcut work? */
> - if (here)
> + if (here && svn_fs_fs__dag_node_kind(here) == svn_node_dir)
> {
> apr_size_t dirname_len = strlen(directory);
> path_so_far->len = dirname_len;
> rest = path + dirname_len + 1;
> }
> + else if (here)
> + {
> + /* The parent node is not a directory. We are looking for some
> + sub-path, so that sub-path will not exist. That will be o.k.
> + if we are just here to check for the path's existence, but
> + should result in an error otherwise. */
> + if (flags & open_path_allow_null)
> + {
> + *parent_path_p = NULL;
> + return SVN_NO_ERROR;
> + }
> + else
> + return err_not_directory(root, directory, pool);
> + }
> }
> }
>
Would be nice to wrap this return value in svn_error_trace(), as that
makes it easier to find where the error came from.
> @@ -1144,8 +1177,6 @@ open_path(parent_path_t **parent_path_p,
> /* The path isn't finished yet; we'd better be in a directory. */
> if (svn_fs_fs__dag_node_kind(child) != svn_node_dir)
> {
> - const char *msg;
> -
> /* Since this is not a directory and we are looking for some
> sub-path, that sub-path will not exist. That will be o.k.,
> if we are just here to check for the path's existence. */
> @@ -1156,14 +1187,7 @@ open_path(parent_path_t **parent_path_p,
> }
>
> /* It's really a problem ... */
> - msg = root->is_txn_root
> - ? apr_psprintf(iterpool,
> - _("Failure opening '%s' in transaction '%s'"),
> - path, root->txn)
> - : apr_psprintf(iterpool,
> - _("Failure opening '%s' in revision %ld"),
> - path, root->rev);
> - SVN_ERR_W(SVN_FS__ERR_NOT_DIRECTORY(fs, path_so_far->data), msg);
> + return err_not_directory(root, path_so_far->data, iterpool);
> }
And here.
-- Brane