Re: libsvn_wc bug: pristine contents for locally replaced file

Bert Huijben wrote:
> Use svn cat_at_BASE to get the file.

Same problem. 'svn cat' seems to select BASE by default.
My point is that that fails with an error that shouldn't happen.

> File is moved to revertbase to allow replacement with history.

But thanks, I understand now: the original base contents are, in this case,
in the revert-base.

The point remains that svn_wc__get_pristine_contents() acts buggy when it's
called on a locally replaced file.

The following patch solves the problem and passes `make check'. But I'm not
sure if simply judging the state from the existence of revert- and text-base
files is "Good". It makes sense logically, but is it the right method?

[[[
Fix error when WC tries to get the pristine contents of a locally replaced
file. To reproduce, see this mail to dev_at_s.a.o:

Message-ID: <4B524246.5060203_at_elego.de>
Date: Sat, 16 Jan 2010 23:48:38 +0100
From: Neels J Hofmeyr <neels_at_elego.de>
Subject: libsvn_wc bug: pristine contents for locally replaced file

* subversion/libsvn_wc/adm_ops.c
  (svn_wc__get_pristine_contents): Prefer a revert-base if present.

Patch by: Neels J. Hofmeyr <neels_at_elego.de>

--This line, and those below, will be ignored--
Index: subversion/libsvn_wc/adm_ops.c
===================================================================
--- subversion/libsvn_wc/adm_ops.c (revision 900159)
+++ subversion/libsvn_wc/adm_ops.c (working copy)
@@ -2337,9 +2337,16 @@ svn_wc__get_pristine_contents(svn_stream
                               apr_pool_t *scratch_pool)
 {
   const char *text_base;
+ svn_node_kind_t kind;

- SVN_ERR(svn_wc__text_base_path(&text_base, db, local_abspath, FALSE,
- scratch_pool));
+ SVN_ERR(svn_wc__text_revert_path(&text_base, db, local_abspath,
+ scratch_pool));
+ SVN_ERR(svn_io_check_path(text_base, &kind, scratch_pool));
+ if (kind == svn_node_none)
+ {
+ SVN_ERR(svn_wc__text_base_path(&text_base, db, local_abspath, FALSE,
+ scratch_pool));
+ }

   if (text_base == NULL)
     {
]]]

~Neels

>
> Bert (mobile phone)
>
> ----- Oorspronkelijk bericht -----
> Van: Neels J Hofmeyr <neels_at_elego.de>
> Verzonden: zaterdag 16 januari 2010 23:48
> Aan: dev_at_subversion.apache.org
> Onderwerp: libsvn_wc bug: pristine contents for locally replaced file
>
> Hi,
>
> is there a good reason why a local replace has to get rid of the pristine
> base file? Because, if the file was kept, the problems described below would
> be resolved.
>
>
> <tell-mode>
> I stumbled over an error using 'svn cat <wc_path>' on a locally replaced
> file. (Not a common use case, but read on.) 'svn cat <wc_path>' appears to
> want to output the pristine base content (which is not documented).
>
> But when a file is locally replaced (not committed), it currently has no
> pristine base file, apparently;
> 'svn cat wc/locally_replaced_file'
> calls
> svn_wc__get_pristine_contents()
> which errors with:
>
> [[[
> $ svn cat file
> subversion/svn/cat-cmd.c:81: (apr_err=2)
> subversion/svn/util.c:960: (apr_err=2)
> subversion/libsvn_client/cat.c:88: (apr_err=2)
> subversion/libsvn_client/cat.c:88: (apr_err=2)
> subversion/libsvn_subr/stream.c:774: (apr_err=2)
> subversion/libsvn_subr/stream.c:774: (apr_err=2)
> subversion/libsvn_subr/io.c:2711: (apr_err=2)
> svn: Can't open file '/tmp/wc/.svn/text-base/file.svn-base': No such file or
> directory
> ]]]
> (reproduction script attached)
>
> 'svn cat' is just an example of how to hit this. This same function is used
> in many other places. A quick impact grep study suggests at least export,
> copy, update, diff, and probably others.
> (Todo: investigation on whether current callers can hit a locally replaced
> file and whether they work around it.)
> </tell-mode>
>
> <bug-hunting>
> I guess svn_wc__get_pristine_contents() wants to return the contents of the
> file that were committed in revision <BASE>. But the implementation expects
> a file to exist which isn't there:
>
> [[[
> svn_error_t *
> svn_wc__get_pristine_contents(svn_stream_t **contents,
> svn_wc__db_t *db,
> const char *local_abspath,
> apr_pool_t *result_pool,
> apr_pool_t *scratch_pool)
> {
> const char *text_base;
>
> SVN_ERR(svn_wc__text_base_path(&text_base, db, local_abspath, FALSE,
> scratch_pool));
>
> if (text_base == NULL)
> {
> *contents = NULL;
> return SVN_NO_ERROR;
> }
>
> return svn_stream_open_readonly(contents, text_base, result_pool,
> scratch_pool);
> // ^^^^^ hits error here, file *text_base does not exist.
> }
> ]]]
>
>
> I see two ill things:
>
> (1) Looking at the function's intention, it should return an empty stream if
> there is no base file. But svn_wc__text_base_path() returns a path that
> doesn't exist.
>
> (2) When the file is locally replaced, it theoretically *does* have a
> pristine base, i.e. the file's content committed at revision <BASE>. The
> function fails to return that content.
> </bug-hunting>
>
>
> So, back to the question: is there a good reason why a local delete followed
> by a local add has to get rid of the pristine base file?
>
> Thanks,
> ~Neels
>