Re: [PATCH] issue #2740: "out-of-date" error text varies depending on access method

Stefan Sperling wrote:
> Hello,
>
> To get my feet wet with subversion issues I've looked into
> issue #2740 and here is a trivial patch to fix it:

I've added a note to issue #2740 in the issue tracker pointing to this
patch.

-Hyrum

> Index: subversion/libsvn_repos/commit.c
> ===================================================================
> --- subversion/libsvn_repos/commit.c (revision 25496)
> +++ subversion/libsvn_repos/commit.c (working copy)
> @@ -122,7 +122,8 @@
> out_of_date(const char *path, const char *txn_name)
> {
> return svn_error_createf(SVN_ERR_FS_TXN_OUT_OF_DATE, NULL,
> - _("Out of date: '%s' in transaction '%s'"),
> + _("Your file or directory '%s' is probably "
> + "out-of-date in transaction '%s'"),
> path, txn_name);
> }
>
> I've also attached a recipe I used to reproduce the bug.
>
> Here's an example log message as per guidelines:
>
> [[[
> Fix issue #2740: "out-of-date" error text varies depending on access method
>
> * subversion/libsvn_repos/commit.c
> (out_of_date): Use similar wording as in libsvn_ra_neon/commit.c
> for out-of-date error message.
> ]]]
>
> I'm not sure how to update the locales for the message.
>
> In spite of the issue submitter stating
>
> "that first message is a bit user-hostile: we poor users
> don't know what transactions are!"
>
> I decided to keep the "in transaction '%s'" bit in for now.
> I'm not sure whether it's better to just remove it from the file://
> case or not. I'm not sure whether an equivalent of txn_name can be
> retrieved from a commit_ctx_t object as used in libsvn_ra_neon/commit.c
> so we could add the transaction name to the http case error message.
>
> One issue that remains is that the path printed in the error
> message is still different for each type of access method.
>
> The following examples are based on a simple project that only
> contains one directory and one file:
>
> dir/
> dir/file
>
> This is what is printed by the file access method without my patch,
> using subversion-1.4.3_2 from FreeBSD ports:
>
> [stsp@ted ~/elego/issues/2740/ws2-file]$ svn commit -m ""
> Sending dir/file
> svn: Commit failed (details follow):
> svn: Out of date: '/dir/file' in transaction '2-1'
>
> And this is what is printed with the http access method:
>
> [stsp@ted ~/elego/issues/2740/ws2-http]$ svn commit -m ""
> Sending dir/file
> svn: Commit failed (details follow):
> svn: Your file or directory 'file' is probably out-of-date
> svn: The version resource does not correspond to the resource within the transaction. Either the requested version resource is out of date (needs to be updated), or the requested version resource is newer than the transaction root (restart the commit).
>
>
> With current svn as of yesterday + my patch I now get:
>
> [stsp@ted ~/elego/issues/2740/ws2-file]$ ../../../prefix/bin/svn commit -m ""
> Sending dir/file
> svn: Commit failed (details follow):
> svn: Your file or directory '/dir/file' is probably out-of-date in transaction 'ted-stsp-lan-75017-1182527566183574-1'
>
> And with http:
>
> [stsp@ted ~/elego/issues/2740/ws2-http]$ ../../../prefix/bin/svn commit -m ""
> Sending dir/file
> svn: Commit failed (details follow):
> svn: Your file or directory 'file' is probably out-of-date
> svn: The version resource does not correspond to the resource within the transaction. Either the requested version resource is out of date (needs to be updated), or the requested version resource is newer than the transaction root (restart the commit).
>
>
> So we get '/dir/file' with file:// and just 'file' with http://
>
> I've tried to make both messages print '/dir/file', but it seems
> that the full path is not yet available when the error is thrown
> in libsvn_ra_neon.
>
> Code snippets below are from libsvn_ra_neon/commit.c.
>
> It seems that rsrc->wr_url is supposed to contain the file
> path relative to the working copy:
> ** WR_URL refers to a working resource for this resource
>
> But it also says:
> ** WR_URL can be NULL if the resource has not (yet) been checked out.
>
> And indeed at the point the error is thrown, rsrc->wr_url is still NULL.
> So currently the error message prints rsrc->local_path instead,
> which is just "file" in my example, even without piping it through
> svn_path_local_style() before printing:
>
> if (err->apr_err == SVN_ERR_FS_CONFLICT)
> return svn_error_createf
> (err->apr_err, err,
> _("Your file or directory '%s' is probably out-of-date"),
> svn_path_local_style(rsrc->local_path, pool));
>
> The code that initialises rsrc->wr_url is a couple of lines later,
> after the checkout has completed successfully.
>
> Is there a nice way to normalise the path consistently in both cases?
>
> Or do you think that the current patch makes the error messages
> similar enough to satisfy #2740? Users see an additional error
> message thrown by the neon layer anyway already when using http,
> so the client output will never be exactly the same...
>
> Sorry for ranting so long about a trivial issue :)
> I will possibly try to work on more subversion issues in the
> future so this is just to get me started.
>
> Thanks,