Did you ever get anywhere with this bug?
--dave
On 9/10/07, Charles Acknin <charlesacknin@gmail.com> wrote:
> 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
>
>
--
David Glasser | glasser_at_davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Sep 28 00:38:52 2007