OK, I've worked some more and here's an even simpler reprod recipe:
% svn cp iota iotacopy
% svn ci
(commits r2)
% svn up -r1
% svn diff -r2
!error here when looking for iotacopy text-base!
It turns out libsvn_wc/diff.c(close_file) doesn't check the
copied-file case (obviously because there's no copyfrom support yet)
and blindly call svn_wc__text_base_path() against a missing path (the
added/copied file), which raises the bug a few lines below. So
basically I did provide file_baton structure with copyfrom_path field
so that we know we're having a new copied file at hand and we're then
able to DTRT.
Here's a patch and its non-reg test coming along with. All 'make
check' tests pass.
[[[
Fix a repos-base diff bug with copied files.
* subversion/libsvn_wc/diff.c
(file_baton): add copyfrom_path member.
(make_file_baton): add copyfrom_path argument.
(svnpatch_open_file, svnpatch_add_file, add_file, open_file): update
calls to make_file_baton according to the new file_baton structure.
(close_file): refine test conditions so that when addressing a copied
file we don't use a missing path and prevent a "no such file" error.
* subversion/test/cmdline/diff_tests.py
(diff_repos_base_copy): new reg-test against the bug this patch is
fixing.
]]]
[[[
Index: subversion/libsvn_wc/diff.c
===================================================================
--- subversion/libsvn_wc/diff.c (revision 26511)
+++ subversion/libsvn_wc/diff.c (working copy)
@@ -249,6 +249,9 @@
/* Set when dealing with svnpatch diff. */
const char *token;
+ /* The path in the WC the current file was copied from. */
+ const char *copyfrom_path;
+
apr_pool_t *pool;
};
@@ -349,6 +352,7 @@
svn_boolean_t added,
struct dir_baton *parent_baton,
const char *token,
+ const char *copyfrom_path,
apr_pool_t *pool)
{
struct file_baton *file_baton = apr_pcalloc(pool, sizeof(*file_baton));
@@ -361,6 +365,7 @@
file_baton->path = path;
file_baton->token = token;
file_baton->dir_baton = parent_baton;
+ file_baton->copyfrom_path = copyfrom_path;
/* If the parent directory is added rather than replaced it does not
exist in the working copy. Determine a working copy path whose
@@ -1341,7 +1346,7 @@
SVN_ERR(svn_wc_write_cmd(eb->svnpatch_stream, eb->pool,
"open-file", "ccc", path, pb->token,
token));
- *file_baton = make_file_baton(path, FALSE, pb, token, file_pool);
+ *file_baton = make_file_baton(path, FALSE, pb, token, NULL, file_pool);
return SVN_NO_ERROR;
}
@@ -1360,7 +1365,7 @@
SVN_ERR(svn_wc_write_cmd(eb->svnpatch_stream, eb->pool,
"add-file", "ccc(?c)", path, pb->token,
token, copyfrom_path));
- *file_baton = make_file_baton(path, TRUE, pb, token, file_pool);
+ *file_baton = make_file_baton(path, TRUE, pb, token, copyfrom_path,
file_pool);
return SVN_NO_ERROR;
}
@@ -2074,7 +2079,7 @@
/* ### TODO: support copyfrom? */
full_path = svn_path_join(pb->edit_baton->anchor_path, path, file_pool);
- b = make_file_baton(full_path, TRUE, pb, token, file_pool);
+ b = make_file_baton(full_path, TRUE, pb, token, copyfrom_path, file_pool);
*file_baton = b;
if (eb->svnpatch_stream && eb->reverse_order)
@@ -2105,7 +2110,7 @@
const char *token = make_token('c', eb, file_pool);
full_path = svn_path_join(pb->edit_baton->anchor_path, path, file_pool);
- b = make_file_baton(full_path, FALSE, pb, token, file_pool);
+ b = make_file_baton(full_path, FALSE, pb, token, NULL, file_pool);
*file_baton = b;
if (eb->svnpatch_stream)
@@ -2259,10 +2264,16 @@
/* The repository version of the file is in the temp file we applied
the BASE->repos delta to. If we haven't seen any changes, it's
- the same as BASE. */
+ the same as BASE. Additionally, if this file was copied there can't
+ be any text-base available and we don't want to display diffs. */
temp_file_path = b->temp_file_path;
if (!temp_file_path)
- temp_file_path = svn_wc__text_base_path(b->path, FALSE, b->pool);
+ {
+ if (b->added && b->copyfrom_path)
+ temp_file_path = empty_file;
+ else
+ temp_file_path = svn_wc__text_base_path(b->path, FALSE, b->pool);
+ }
/* If the file isn't in the working copy (either because it was added
Index: subversion/tests/cmdline/diff_tests.py
===================================================================
--- subversion/tests/cmdline/diff_tests.py (revision 26511)
+++ subversion/tests/cmdline/diff_tests.py (working copy)
@@ -3145,6 +3145,28 @@
svntest.actions.run_and_verify_svn(None, expected_infinity, [],
'diff', '-rHEAD', '--depth', 'infinity')
+def diff_repos_base_copy(sbox):
+ "repos-base diff with copied file"
+
+ sbox.build()
+ wc_dir = sbox.wc_dir
+ os.chdir(wc_dir)
+
+ # copy a file
+ mu_path = os.path.join('A', 'mu')
+ mucp_path = os.path.join('A', 'mucopy')
+ svntest.main.run_svn(None, 'cp', mu_path, mucp_path)
+
+ # commit r2 and update back to r1
+ svntest.main.run_svn(None, 'ci', '-m', 'log msg')
+ svntest.main.run_svn(None, 'up', '-r1')
+
+ # diff r2 against base
+ # we ought to see no error and an empty output
+ svntest.actions.run_and_verify_svn(None, [], [],
+ 'diff', '-r' , '2:BASE')
+
+
########################################################################
#Run the tests
@@ -3194,6 +3216,7 @@
diff_in_renamed_folder,
diff_svnpatch,
diff_with_depth,
+ diff_repos_base_copy,
]
if __name__ == '__main__':
]]]
BTW, should svn_wc__text_base_path() check for the existence of the
path it returns (and return NULL when missing)? I think it would be
pretty relevant to expect such a function to do so. And this could
also prevent such situations to show up.
Cheers,
Charles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Sep 11 01:07:02 2007