You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@wandisco.com> on 2012/04/25 14:37:25 UTC
Re: svn commit: r1330058 - in /subversion/trunk/subversion: include/svn_error_codes.h include/svn_fs.h libsvn_fs/editor.c libsvn_repos/commit.c
gstein@apache.org writes:
> + else
> + {
> + SVN_ERR_ASSERT(err != NULL);
> + if (err->apr_err == SVN_ERR_FS_CONFLICT)
> + {
> + /* Case 2. */
> +
> + /* Copy this into the correct pool (see note above). */
> + *conflict_path = apr_pstrdup(result_pool, inner_conflict_path);
> +
> + /* Return sucess. The caller should inspect CONFLICT_PATH to
> + determine this particular case. */
> + svn_error_clear(err);
> + err = SVN_NO_ERROR;
> + }
By clearing err we limit the information about the conflict to just the
conflict path--if there is a more detailed explanation in err it is
discarded. Is that what we want? The FS layers detect all sorts of
conflicts that could be described in err, although they don't do so at
present.
I suppose that the user resolves the conflict by updating the working
copy, but that assumes that there is a working copy and that the update
will use the same revision and get the same conflict.
--
Philip
Re: svn commit: r1330058 - in /subversion/trunk/subversion:
include/svn_error_codes.h include/svn_fs.h libsvn_fs/editor.c
libsvn_repos/commit.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Philip Martin wrote on Fri, Apr 27, 2012 at 18:29:09 +0100:
> Perhaps we should keep the current behaviour and return both an error
> and a path and let the caller decide which to one use.
We could use the existing svn_error_t ** argument for that; the
semantics would be that it is the post-commit error when
SVN_IS_VALID_REVNUM(*revision), the conflict error when *conflict_path
is set, and has an unspecified value if the commit failed for a reason
other than a conflict.
Re: svn commit: r1330058 - in /subversion/trunk/subversion:
include/svn_error_codes.h include/svn_fs.h libsvn_fs/editor.c
libsvn_repos/commit.c
Posted by Daniel Shahaf <da...@elego.de>.
Greg Stein wrote on Fri, Apr 27, 2012 at 15:53:12 -0400:
> On Fri, Apr 27, 2012 at 15:25, Philip Martin <ph...@wandisco.com> wrote:
> > Greg Stein <gs...@gmail.com> writes:
> >
> >> The current err->message says "Conflict at '/some/path'." (no real context)
> >>
> >> mod_dav_svn returns a message such as:
> >>
> >> "A conflict occurred during the CHECKIN processing. The problem
> >> occurred with the \"%s\" resource."
> >> or
> >> "Conflict when committing '%s'."
> >> or
> >> "A conflict occurred during the MERGE processing. The problem occurred
> >> with the \"%s\" resource."
> >>
> >> To construct each of these, it needs the path. And I argue the above
> >> messages are more understandable.
> >
> > That's what goes in the Apache error log but it's not what the client
> > sees. Using recent trunk for client and server:
>
> Yeah. I was trying to track down some error message propagation for
> lock_tests 1. Something is jammed up, and I don't know why the human
> readable errors aren't marshalled like in other situations. Separate
> bug.
IIRC mod_dav_svn marshals just the top error in the chain, not the whole
chain. (grepping for svn_error_purge_tracing() should find that code)
Re: svn commit: r1330058 - in /subversion/trunk/subversion:
include/svn_error_codes.h include/svn_fs.h libsvn_fs/editor.c libsvn_repos/commit.c
Posted by Greg Stein <gs...@gmail.com>.
On Fri, Apr 27, 2012 at 15:25, Philip Martin <ph...@wandisco.com> wrote:
> Greg Stein <gs...@gmail.com> writes:
>
>> The current err->message says "Conflict at '/some/path'." (no real context)
>>
>> mod_dav_svn returns a message such as:
>>
>> "A conflict occurred during the CHECKIN processing. The problem
>> occurred with the \"%s\" resource."
>> or
>> "Conflict when committing '%s'."
>> or
>> "A conflict occurred during the MERGE processing. The problem occurred
>> with the \"%s\" resource."
>>
>> To construct each of these, it needs the path. And I argue the above
>> messages are more understandable.
>
> That's what goes in the Apache error log but it's not what the client
> sees. Using recent trunk for client and server:
Yeah. I was trying to track down some error message propagation for
lock_tests 1. Something is jammed up, and I don't know why the human
readable errors aren't marshalled like in other situations. Separate
bug.
Cheers,
-g
Re: svn commit: r1330058 - in /subversion/trunk/subversion: include/svn_error_codes.h include/svn_fs.h libsvn_fs/editor.c libsvn_repos/commit.c
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
> The current err->message says "Conflict at '/some/path'." (no real context)
>
> mod_dav_svn returns a message such as:
>
> "A conflict occurred during the CHECKIN processing. The problem
> occurred with the \"%s\" resource."
> or
> "Conflict when committing '%s'."
> or
> "A conflict occurred during the MERGE processing. The problem occurred
> with the \"%s\" resource."
>
> To construct each of these, it needs the path. And I argue the above
> messages are more understandable.
That's what goes in the Apache error log but it's not what the client
sees. Using recent trunk for client and server:
svn mkdir -mm http://localhost:8888/obj/repo/A --config-option servers:global:http-library=serf
../src/subversion/svn/mkdir-cmd.c:100: (apr_err=160024)
../src/subversion/libsvn_client/add.c:980: (apr_err=160024)
../src/subversion/libsvn_ra_serf/commit.c:2216: (apr_err=160024)
../src/subversion/libsvn_ra_serf/util.c:734: (apr_err=160024)
../src/subversion/libsvn_ra_serf/util.c:1837: (apr_err=160024)
../src/subversion/libsvn_ra_serf/util.c:1781: (apr_err=160024)
../src/subversion/libsvn_ra_serf/util.c:880: (apr_err=160024)
svn: E160024: Conflict at '/A'
xsvn mkdir -mm http://localhost:8888/obj/repo/A --config-option servers:global:http-library=neon
../src/subversion/svn/mkdir-cmd.c:100: (apr_err=160024)
../src/subversion/libsvn_client/add.c:980: (apr_err=160024)
../src/subversion/libsvn_ra_neon/commit.c:1526: (apr_err=160024)
../src/subversion/libsvn_ra_neon/merge.c:759: (apr_err=160024)
../src/subversion/libsvn_ra_neon/util.c:1324: (apr_err=160024)
../src/subversion/libsvn_ra_neon/util.c:1565: (apr_err=160024)
../src/subversion/libsvn_ra_neon/util.c:780: (apr_err=160024)
svn: E160024: Conflict at '/A'
svn mkdir -mm svn://localhost/repo/A
../src/subversion/svn/mkdir-cmd.c:100: (apr_err=160024)
../src/subversion/libsvn_client/add.c:980: (apr_err=160024)
../src/subversion/libsvn_repos/commit.c:790: (apr_err=160024)
../src/subversion/libsvn_fs/fs-loader.c:675: (apr_err=160024)
../src/subversion/libsvn_fs_fs/tree.c:1712: (apr_err=160024)
../src/subversion/libsvn_fs_fs/tree.c:1575: (apr_err=160024)
../src/subversion/libsvn_fs_fs/tree.c:1158: (apr_err=160024)
svn: E160024: Conflict at '/A'
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: svn commit: r1330058 - in /subversion/trunk/subversion:
include/svn_error_codes.h include/svn_fs.h libsvn_fs/editor.c
libsvn_repos/commit.c
Posted by Daniel Shahaf <da...@elego.de>.
Greg Stein wrote on Fri, Apr 27, 2012 at 17:33:18 -0400:
> I'm growing weary of this.
>
> Daniel and I spent a couple hours discussing calling patterns, error
> states, and everything. This felt the best approach.
>
> If you have a concrete suggestion, and a patch, then I'll take a look.
>
Index: subversion/include/svn_fs.h
===================================================================
--- subversion/include/svn_fs.h (revision 1331716)
+++ subversion/include/svn_fs.h (working copy)
@@ -1090,19 +1090,19 @@ svn_fs_editor_create_for(svn_editor_t **editor,
* reason, then @a *revision will be set to #SVN_INVALID_REVNUM.
*
* If a conflict occurs during the commit, then @a *conflict_path will
- * be set to a path that caused the conflict. #SVN_NO_ERROR will be returned.
- * Callers may want to construct an #SVN_ERR_FS_CONFLICT error with a
- * message that incorporates @a *conflict_path.
+ * be set to a path that caused the conflict, and @a *copc_err will be
+ * be set to an error chain describing that error in a human-readable manner.
+ * #SVN_NO_ERROR will be returned.
*
* If a non-conflict error occurs during the commit, then that error will
* be returned.
- * As is standard with any Subversion API, @a revision, @a post_commit_err,
+ * As is standard with any Subversion API, @a revision, @a copc_err,
* and @a conflict_path (the OUT parameters) have an indeterminate value if
* an error is returned.
*
* If the commit completes (and a revision is returned in @a *revision), then
* it is still possible for an error to occur during the cleanup process.
- * Any such error will be returned in @a *post_commit_err. The caller must
+ * Any such error will be returned in @a *copc_err. The caller must
* properly use or clear that error.
*
* If svn_editor_complete() has already been called on @a editor, then
@@ -1117,15 +1117,16 @@ svn_fs_editor_create_for(svn_editor_t **editor,
* @a scratch_pool will be used for all temporary allocations.
*
* @note To summarize, there are three possible outcomes of this function:
- * successful commit (with or without an associated @a *post_commit_err);
- * failed commit due to a conflict (reported via @a *conflict_path); and
- * failed commit for some other reason (reported via the returned error.)
+ * successful commit (with or without an associated @a *copc_err);
+ * failed commit due to a conflict (reported via both @a *conflict_path
+ * and @a *copc_err); and failed commit for some other reason (reported via
+ * the returned error, with undefined values for the OUT parameters.)
*
* @since New in 1.8.
*/
svn_error_t *
svn_fs_editor_commit(svn_revnum_t *revision,
- svn_error_t **post_commit_err,
+ svn_error_t **copc_err, /* conflict or post-commit */
const char **conflict_path,
svn_editor_t *editor,
apr_pool_t *result_pool,
Index: subversion/libsvn_fs/editor.c
===================================================================
--- subversion/libsvn_fs/editor.c (revision 1331716)
+++ subversion/libsvn_fs/editor.c (working copy)
@@ -450,7 +450,7 @@ svn_fs_editor_create_for(svn_editor_t **editor,
svn_error_t *
svn_fs_editor_commit(svn_revnum_t *revision,
- svn_error_t **post_commit_err,
+ svn_error_t **copc_err,
const char **conflict_path,
svn_editor_t *editor,
apr_pool_t *result_pool,
@@ -466,7 +466,7 @@ svn_fs_editor_commit(svn_revnum_t *revision,
NULL, NULL);
*revision = SVN_INVALID_REVNUM;
- *post_commit_err = NULL;
+ *copc_err = NULL;
*conflict_path = NULL;
/* Clean up internal resources (eg. eb->root). This also allows the
@@ -489,7 +489,7 @@ svn_fs_editor_commit(svn_revnum_t *revision,
/* Case 3. ERR is a post-commit (cleanup) error. */
/* Pass responsibility via POST_COMMIT_ERR. */
- *post_commit_err = err;
+ *copc_err = err;
err = SVN_NO_ERROR;
}
/* else: Case 1. */
@@ -504,9 +504,8 @@ svn_fs_editor_commit(svn_revnum_t *revision,
/* Copy this into the correct pool (see note above). */
*conflict_path = apr_pstrdup(result_pool, inner_conflict_path);
- /* Return sucess. The caller should inspect CONFLICT_PATH to
- determine this particular case. */
- svn_error_clear(err);
+ /* Pass responsibility via POST_COMMIT_ERR. */
+ *copc_err = err;
err = SVN_NO_ERROR;
}
/* else: Case 4. */
Index: subversion/libsvn_repos/commit.c
===================================================================
--- subversion/libsvn_repos/commit.c (revision 1331716)
+++ subversion/libsvn_repos/commit.c (working copy)
@@ -1214,6 +1214,7 @@ complete_cb(void *baton,
{
struct ev2_baton *eb = baton;
svn_revnum_t revision;
+ svn_error_t *copc_err;
svn_error_t *post_commit_err;
const char *conflict_path;
svn_error_t *err;
@@ -1224,18 +1225,19 @@ complete_cb(void *baton,
SVN_ERR(svn_repos__hooks_pre_commit(eb->repos, eb->txn_name, scratch_pool));
/* Hook is done. Let's do the actual commit. */
- SVN_ERR(svn_fs_editor_commit(&revision, &post_commit_err, &conflict_path,
+ SVN_ERR(svn_fs_editor_commit(&revision, &copc_err, &conflict_path,
eb->inner, scratch_pool, scratch_pool));
/* Did a conflict occur during the commit process? */
if (conflict_path != NULL)
- return svn_error_createf(SVN_ERR_FS_CONFLICT, NULL,
+ return svn_error_createf(SVN_ERR_FS_CONFLICT, copc_err,,
_("Conflict at '%s'"), conflict_path);
/* Since did not receive an error during the commit process, and no
conflict was specified... we committed a revision. Run the hooks.
Other errors may have occurred within the FS (specified by the
POST_COMMIT_ERR localvar), but we need to run the hooks. */
+ post_commit_err = copc_err;
SVN_ERR_ASSERT(SVN_IS_VALID_REVNUM(revision));
err = svn_repos__hooks_post_commit(eb->repos, revision, eb->txn_name,
scratch_pool);
Re: svn commit: r1330058 - in /subversion/trunk/subversion:
include/svn_error_codes.h include/svn_fs.h libsvn_fs/editor.c libsvn_repos/commit.c
Posted by Greg Stein <gs...@gmail.com>.
I'm growing weary of this.
Daniel and I spent a couple hours discussing calling patterns, error
states, and everything. This felt the best approach.
If you have a concrete suggestion, and a patch, then I'll take a look.
-g
On Fri, Apr 27, 2012 at 16:02, Philip Martin <ph...@wandisco.com> wrote:
> Greg Stein <gs...@gmail.com> writes:
>
>> And that pattern was very annoying. Today, you can
>> SVN_ERR(svn_fs_editor_commit(...)). With the pattern you're
>> suggesting, it gets uglier:
>>
>> err = svn_fs_editor_commit(...);
>> if (SVN_IS_VALID_REVNUM(revision))
>> ... do some stuff
>> else if (err == SVN_ERR_FS_CONFLICT)
>> ... do some other stuff
>> else if (post_commit_err)
>> ... yet more stuff
>
> I don't think you are comparing like with like there! Your first
> example is:
>
> SVN_ERR(svn_fs_editor_commit(...))
> if (confict)
> conflict occurred
> else
> if (post_commit_err)
> post commit occurred
> commit occurred
>
> The second example is
>
> err = svn_fs_editor_commit():
> if (err == CONFLICT)
> conflict occurred
> else if (err) # extra
> return err # extra
> else
> if (post_commit_err)
> post commit occurred
> commit occurred
>
> Some difference, but not that much.
>
>
>> And meanwhile, you're trying to manage whether an error was returned,
>> which needs to be cleared, wrapped, returned, etc. And throw in there
>> some transaction aborting, too.
>>
>> Take a look at libsvn_repos/commit.c:complete_cb(). The error handling
>> logic is quite sane. Compare that to any caller of svn_fs_commit_txn()
>> (a few) and the more-used svn_repos_fs_commit_txn(). They follow
>> roughly similar patterns, but not all the same. Some drop the error
>> anyways. All are more complicated.
>>
>> I chose a calling pattern that seems rational (looking at
>> complete_cb), and takes into account the *current* behavior of FS and
>> the low-quality conflict errors that it returns. If/when the FS wants
>> to return something more, then great. My advice would be: don't make
>> it an error. Return some kind of rich data structure that describes
>> the conflict. Leave human-readable message generation to an exterior
>> function which consumes that structure. That allows
>> SVN_ERR(svn_fs_commit_txn()), relegating returned errors to true
>> errors instead of semantic issues within the txn that are
>> best-described with a mechanism other than svn_error_t. That is the
>> pattern/design of svn_fs_editor_commit(), though the "structure" is
>> just a simple path at the moment.
>
> You could say the same about all our error reporting. Why is this one
> special?
>
> At the momement if an FS backend has some new conflict information it
> can be added by the FS code that generate SVN_ERR_FS_CONFLICT. If I
> tweak the error in FSFS and commit I get:
>
> svn mkdir -mm http://localhost:8888/obj/repo/A
> svn: Conflict at path '/A' due to some special condition
>
> With your scheme we need to change the FS/editor API to get the
> information out of the FS into the server, then have the server marshal
> it across the wire (extend the protocol? pack it in an svn_error_t?)
> then have the client construct an error for the user.
>
> Maybe it's academic and the FS errors are never going to change, but I
> don't see why this error is being handled differently.
>
> --
> uberSVN: Apache Subversion Made Easy
> http://www.uberSVN.com
Re: svn commit: r1330058 - in /subversion/trunk/subversion: include/svn_error_codes.h include/svn_fs.h libsvn_fs/editor.c libsvn_repos/commit.c
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
> And that pattern was very annoying. Today, you can
> SVN_ERR(svn_fs_editor_commit(...)). With the pattern you're
> suggesting, it gets uglier:
>
> err = svn_fs_editor_commit(...);
> if (SVN_IS_VALID_REVNUM(revision))
> ... do some stuff
> else if (err == SVN_ERR_FS_CONFLICT)
> ... do some other stuff
> else if (post_commit_err)
> ... yet more stuff
I don't think you are comparing like with like there! Your first
example is:
SVN_ERR(svn_fs_editor_commit(...))
if (confict)
conflict occurred
else
if (post_commit_err)
post commit occurred
commit occurred
The second example is
err = svn_fs_editor_commit():
if (err == CONFLICT)
conflict occurred
else if (err) # extra
return err # extra
else
if (post_commit_err)
post commit occurred
commit occurred
Some difference, but not that much.
> And meanwhile, you're trying to manage whether an error was returned,
> which needs to be cleared, wrapped, returned, etc. And throw in there
> some transaction aborting, too.
>
> Take a look at libsvn_repos/commit.c:complete_cb(). The error handling
> logic is quite sane. Compare that to any caller of svn_fs_commit_txn()
> (a few) and the more-used svn_repos_fs_commit_txn(). They follow
> roughly similar patterns, but not all the same. Some drop the error
> anyways. All are more complicated.
>
> I chose a calling pattern that seems rational (looking at
> complete_cb), and takes into account the *current* behavior of FS and
> the low-quality conflict errors that it returns. If/when the FS wants
> to return something more, then great. My advice would be: don't make
> it an error. Return some kind of rich data structure that describes
> the conflict. Leave human-readable message generation to an exterior
> function which consumes that structure. That allows
> SVN_ERR(svn_fs_commit_txn()), relegating returned errors to true
> errors instead of semantic issues within the txn that are
> best-described with a mechanism other than svn_error_t. That is the
> pattern/design of svn_fs_editor_commit(), though the "structure" is
> just a simple path at the moment.
You could say the same about all our error reporting. Why is this one
special?
At the momement if an FS backend has some new conflict information it
can be added by the FS code that generate SVN_ERR_FS_CONFLICT. If I
tweak the error in FSFS and commit I get:
svn mkdir -mm http://localhost:8888/obj/repo/A
svn: Conflict at path '/A' due to some special condition
With your scheme we need to change the FS/editor API to get the
information out of the FS into the server, then have the server marshal
it across the wire (extend the protocol? pack it in an svn_error_t?)
then have the client construct an error for the user.
Maybe it's academic and the FS errors are never going to change, but I
don't see why this error is being handled differently.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: svn commit: r1330058 - in /subversion/trunk/subversion:
include/svn_error_codes.h include/svn_fs.h libsvn_fs/editor.c libsvn_repos/commit.c
Posted by Greg Stein <gs...@gmail.com>.
On Fri, Apr 27, 2012 at 13:29, Philip Martin <ph...@wandisco.com> wrote:
> Greg Stein <gs...@gmail.com> writes:
>> On Fri, Apr 27, 2012 at 11:20, Philip Martin <ph...@wandisco.com> wrote:
>>> Greg Stein <gs...@gmail.com> writes:
>>>
>>>> On Wed, Apr 25, 2012 at 16:43, Philip Martin <ph...@wandisco.com> wrote:
>>>>> Perhaps we should keep the error and toss the path?
>>>>
>>>> mod_dav_svn uses that path to construct a "nice" error message based
>>>> on the context of the commit.
>>>
>>> mod_dav_svn isn't the only user of this API.
>>
>> WELL aware of that fact. I was simply giving a data point for how the
>> path was used.
>>
>> What's your point?
>
> I don't understand why the fact that mod_dav_svn uses the path is a
> reason to discard the error. Particularly when svnserve currently
> discards the path and returns the error to the client.
The current err->message says "Conflict at '/some/path'." (no real context)
mod_dav_svn returns a message such as:
"A conflict occurred during the CHECKIN processing. The problem
occurred with the \"%s\" resource."
or
"Conflict when committing '%s'."
or
"A conflict occurred during the MERGE processing. The problem occurred
with the \"%s\" resource."
To construct each of these, it needs the path. And I argue the above
messages are more understandable.
Now: if we returned a chain, then we could have that simplistic
message, then wrapped with something like "Conflict occurred during
the CHECKIN processing", and omit the path. But, alas, we don't have
chain reporting (at this time).
>>>>> So currently reporting "conflict on path 'foo'" is probably sufficient
>>>>> but it's odd to limit ourselves. We have an error reporting mechanism
>>>>> why would we choose not to use it?
>>>>
>>>> Well... we return that conflict_path so that callers can make
>>>> nicer/contextual error messages with the path. We don't transfer the
>>>> whole chain over the wire, so if the path is on the inner error (say,
>>>> if we just wrapped context and relied on the path to be in the inner
>>>> error), then the user would never see the path.
>>>>
>>>> If we could/would marshal the whole error chain, then yes: we could
>>>> rely on our error reporting mechanism. But we don't, so we can't.
>>>
>>> svnserve returns an error chain, see svn_ra_svn_write_cmd_failure.
>>
>> And other RA layers do not.
>
> You seem to be suggesting that because mod_dav_svn doesn't return the
> error chain the error should not be available to other RA layers.
If the error had something interesting in it, then I might have
designed something to incorporate that error and produce an error
chain. And then maybe figured out some way to marshal that back to the
client, in a backwards/forwards compatible fashion. But that's a lot
of work when the returned errors have nothing of consequence anyways.
> What happens if an FS layer wants to return more information about a
> conflict? Does it return SVN_ERR_FS_CONFLICT_RICH to bypass the
> discarding of SVN_ERR_FS_CONFLICT?
When it does, then we can revisit the problem.
> Perhaps we should keep the current behaviour and return both an error
> and a path and let the caller decide which to one use.
And that pattern was very annoying. Today, you can
SVN_ERR(svn_fs_editor_commit(...)). With the pattern you're
suggesting, it gets uglier:
err = svn_fs_editor_commit(...);
if (SVN_IS_VALID_REVNUM(revision))
... do some stuff
else if (err == SVN_ERR_FS_CONFLICT)
... do some other stuff
else if (post_commit_err)
... yet more stuff
And meanwhile, you're trying to manage whether an error was returned,
which needs to be cleared, wrapped, returned, etc. And throw in there
some transaction aborting, too.
Take a look at libsvn_repos/commit.c:complete_cb(). The error handling
logic is quite sane. Compare that to any caller of svn_fs_commit_txn()
(a few) and the more-used svn_repos_fs_commit_txn(). They follow
roughly similar patterns, but not all the same. Some drop the error
anyways. All are more complicated.
I chose a calling pattern that seems rational (looking at
complete_cb), and takes into account the *current* behavior of FS and
the low-quality conflict errors that it returns. If/when the FS wants
to return something more, then great. My advice would be: don't make
it an error. Return some kind of rich data structure that describes
the conflict. Leave human-readable message generation to an exterior
function which consumes that structure. That allows
SVN_ERR(svn_fs_commit_txn()), relegating returned errors to true
errors instead of semantic issues within the txn that are
best-described with a mechanism other than svn_error_t. That is the
pattern/design of svn_fs_editor_commit(), though the "structure" is
just a simple path at the moment.
Cheers,
-g
Re: svn commit: r1330058 - in /subversion/trunk/subversion: include/svn_error_codes.h include/svn_fs.h libsvn_fs/editor.c libsvn_repos/commit.c
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
> On Fri, Apr 27, 2012 at 11:20, Philip Martin <ph...@wandisco.com> wrote:
>> Greg Stein <gs...@gmail.com> writes:
>>
>>> On Wed, Apr 25, 2012 at 16:43, Philip Martin <ph...@wandisco.com> wrote:
>>>> Perhaps we should keep the error and toss the path?
>>>
>>> mod_dav_svn uses that path to construct a "nice" error message based
>>> on the context of the commit.
>>
>> mod_dav_svn isn't the only user of this API.
>
> WELL aware of that fact. I was simply giving a data point for how the
> path was used.
>
> What's your point?
I don't understand why the fact that mod_dav_svn uses the path is a
reason to discard the error. Particularly when svnserve currently
discards the path and returns the error to the client.
>>>> So currently reporting "conflict on path 'foo'" is probably sufficient
>>>> but it's odd to limit ourselves. We have an error reporting mechanism
>>>> why would we choose not to use it?
>>>
>>> Well... we return that conflict_path so that callers can make
>>> nicer/contextual error messages with the path. We don't transfer the
>>> whole chain over the wire, so if the path is on the inner error (say,
>>> if we just wrapped context and relied on the path to be in the inner
>>> error), then the user would never see the path.
>>>
>>> If we could/would marshal the whole error chain, then yes: we could
>>> rely on our error reporting mechanism. But we don't, so we can't.
>>
>> svnserve returns an error chain, see svn_ra_svn_write_cmd_failure.
>
> And other RA layers do not.
You seem to be suggesting that because mod_dav_svn doesn't return the
error chain the error should not be available to other RA layers.
What happens if an FS layer wants to return more information about a
conflict? Does it return SVN_ERR_FS_CONFLICT_RICH to bypass the
discarding of SVN_ERR_FS_CONFLICT?
Perhaps we should keep the current behaviour and return both an error
and a path and let the caller decide which to one use.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: svn commit: r1330058 - in /subversion/trunk/subversion:
include/svn_error_codes.h include/svn_fs.h libsvn_fs/editor.c libsvn_repos/commit.c
Posted by Greg Stein <gs...@gmail.com>.
On Fri, Apr 27, 2012 at 11:20, Philip Martin <ph...@wandisco.com> wrote:
> Greg Stein <gs...@gmail.com> writes:
>
>> On Wed, Apr 25, 2012 at 16:43, Philip Martin <ph...@wandisco.com> wrote:
>>> Perhaps we should keep the error and toss the path?
>>
>> mod_dav_svn uses that path to construct a "nice" error message based
>> on the context of the commit.
>
> mod_dav_svn isn't the only user of this API.
WELL aware of that fact. I was simply giving a data point for how the
path was used.
What's your point?
>>> So currently reporting "conflict on path 'foo'" is probably sufficient
>>> but it's odd to limit ourselves. We have an error reporting mechanism
>>> why would we choose not to use it?
>>
>> Well... we return that conflict_path so that callers can make
>> nicer/contextual error messages with the path. We don't transfer the
>> whole chain over the wire, so if the path is on the inner error (say,
>> if we just wrapped context and relied on the path to be in the inner
>> error), then the user would never see the path.
>>
>> If we could/would marshal the whole error chain, then yes: we could
>> rely on our error reporting mechanism. But we don't, so we can't.
>
> svnserve returns an error chain, see svn_ra_svn_write_cmd_failure.
And other RA layers do not.
-g
Re: svn commit: r1330058 - in /subversion/trunk/subversion: include/svn_error_codes.h include/svn_fs.h libsvn_fs/editor.c libsvn_repos/commit.c
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
> On Wed, Apr 25, 2012 at 16:43, Philip Martin <ph...@wandisco.com> wrote:
>> Perhaps we should keep the error and toss the path?
>
> mod_dav_svn uses that path to construct a "nice" error message based
> on the context of the commit.
mod_dav_svn isn't the only user of this API.
>> So currently reporting "conflict on path 'foo'" is probably sufficient
>> but it's odd to limit ourselves. We have an error reporting mechanism
>> why would we choose not to use it?
>
> Well... we return that conflict_path so that callers can make
> nicer/contextual error messages with the path. We don't transfer the
> whole chain over the wire, so if the path is on the inner error (say,
> if we just wrapped context and relied on the path to be in the inner
> error), then the user would never see the path.
>
> If we could/would marshal the whole error chain, then yes: we could
> rely on our error reporting mechanism. But we don't, so we can't.
svnserve returns an error chain, see svn_ra_svn_write_cmd_failure.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: svn commit: r1330058 - in /subversion/trunk/subversion:
include/svn_error_codes.h include/svn_fs.h libsvn_fs/editor.c libsvn_repos/commit.c
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Apr 25, 2012 at 16:43, Philip Martin <ph...@wandisco.com> wrote:
> Greg Stein <gs...@gmail.com> writes:
>
>>> By clearing err we limit the information about the conflict to just the
>>> conflict path--if there is a more detailed explanation in err it is
>>> discarded. Is that what we want? The FS layers detect all sorts of
>>> conflicts that could be described in err, although they don't do so at
>>> present.
>>
>> Well... only described in a human-readable fashion. The only
>> programmatic piece we have is conflict_path. The code can't do much
>> based on the human-readable portion except maybe get that sent back to
>> the user.
>
> Sending it back to the user is what we lose.
Right now, the loss is basically nothing. The message isn't very
informative. The repos Ev2 commit editor essentially rebuilds the same
error string when it deems it proper to return an svn_error_t.
>> And as you note: the string is set by
>> libsvn_fs_*/tree.c:conflict_err() and is very simplistic. I deemed it
>> "okay" to toss that out, in favor of trying to simplify the various
>> result conditions of committing.
>>
>> Do you think that is going to change any time soon?
>>
>> Part of the problem here is returning an error (from the underlying
>> svn_fs_commit_txn()) *and* expecting the caller to examine an OUT
>> parameter. We normally state OUT parameters are invalid upon an error
>> return. I didn't want to propagate that behavior to this function's
>> error/OUT behavior.
>
> Perhaps we should keep the error and toss the path?
mod_dav_svn uses that path to construct a "nice" error message based
on the context of the commit.
>>> I suppose that the user resolves the conflict by updating the working
>>> copy, but that assumes that there is a working copy and that the update
>>> will use the same revision and get the same conflict.
>>
>> An 'svn update' might not produce a conflict, but simply merge some
>> unrelated text into the file. The server can't do that, but the client
>> is much smarter there.
>
> My point is that we cannot rely on the user running "svn update" to
> understand the reason for the conflict. Often it will explain the
> conflict and in other cases, "svn rm URL" say, repeating the commit will
> generate an explanation.
>
> So currently reporting "conflict on path 'foo'" is probably sufficient
> but it's odd to limit ourselves. We have an error reporting mechanism
> why would we choose not to use it?
Well... we return that conflict_path so that callers can make
nicer/contextual error messages with the path. We don't transfer the
whole chain over the wire, so if the path is on the inner error (say,
if we just wrapped context and relied on the path to be in the inner
error), then the user would never see the path.
If we could/would marshal the whole error chain, then yes: we could
rely on our error reporting mechanism. But we don't, so we can't.
Further insight/ideas would be welcome. I'm not super keen on the
solution either, but I think it is balanced reasonably at this point.
Cheers,
-g
Re: svn commit: r1330058 - in /subversion/trunk/subversion: include/svn_error_codes.h include/svn_fs.h libsvn_fs/editor.c libsvn_repos/commit.c
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
>> By clearing err we limit the information about the conflict to just the
>> conflict path--if there is a more detailed explanation in err it is
>> discarded. Is that what we want? The FS layers detect all sorts of
>> conflicts that could be described in err, although they don't do so at
>> present.
>
> Well... only described in a human-readable fashion. The only
> programmatic piece we have is conflict_path. The code can't do much
> based on the human-readable portion except maybe get that sent back to
> the user.
Sending it back to the user is what we lose.
> And as you note: the string is set by
> libsvn_fs_*/tree.c:conflict_err() and is very simplistic. I deemed it
> "okay" to toss that out, in favor of trying to simplify the various
> result conditions of committing.
>
> Do you think that is going to change any time soon?
>
> Part of the problem here is returning an error (from the underlying
> svn_fs_commit_txn()) *and* expecting the caller to examine an OUT
> parameter. We normally state OUT parameters are invalid upon an error
> return. I didn't want to propagate that behavior to this function's
> error/OUT behavior.
Perhaps we should keep the error and toss the path?
>
>> I suppose that the user resolves the conflict by updating the working
>> copy, but that assumes that there is a working copy and that the update
>> will use the same revision and get the same conflict.
>
> An 'svn update' might not produce a conflict, but simply merge some
> unrelated text into the file. The server can't do that, but the client
> is much smarter there.
My point is that we cannot rely on the user running "svn update" to
understand the reason for the conflict. Often it will explain the
conflict and in other cases, "svn rm URL" say, repeating the commit will
generate an explanation.
So currently reporting "conflict on path 'foo'" is probably sufficient
but it's odd to limit ourselves. We have an error reporting mechanism
why would we choose not to use it?
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: svn commit: r1330058 - in /subversion/trunk/subversion:
include/svn_error_codes.h include/svn_fs.h libsvn_fs/editor.c libsvn_repos/commit.c
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Apr 25, 2012 at 08:37, Philip Martin <ph...@wandisco.com> wrote:
> gstein@apache.org writes:
>
>> + else
>> + {
>> + SVN_ERR_ASSERT(err != NULL);
>> + if (err->apr_err == SVN_ERR_FS_CONFLICT)
>> + {
>> + /* Case 2. */
>> +
>> + /* Copy this into the correct pool (see note above). */
>> + *conflict_path = apr_pstrdup(result_pool, inner_conflict_path);
>> +
>> + /* Return sucess. The caller should inspect CONFLICT_PATH to
>> + determine this particular case. */
>> + svn_error_clear(err);
>> + err = SVN_NO_ERROR;
>> + }
>
> By clearing err we limit the information about the conflict to just the
> conflict path--if there is a more detailed explanation in err it is
> discarded. Is that what we want? The FS layers detect all sorts of
> conflicts that could be described in err, although they don't do so at
> present.
Well... only described in a human-readable fashion. The only
programmatic piece we have is conflict_path. The code can't do much
based on the human-readable portion except maybe get that sent back to
the user.
And as you note: the string is set by
libsvn_fs_*/tree.c:conflict_err() and is very simplistic. I deemed it
"okay" to toss that out, in favor of trying to simplify the various
result conditions of committing.
Do you think that is going to change any time soon?
Part of the problem here is returning an error (from the underlying
svn_fs_commit_txn()) *and* expecting the caller to examine an OUT
parameter. We normally state OUT parameters are invalid upon an error
return. I didn't want to propagate that behavior to this function's
error/OUT behavior.
> I suppose that the user resolves the conflict by updating the working
> copy, but that assumes that there is a working copy and that the update
> will use the same revision and get the same conflict.
An 'svn update' might not produce a conflict, but simply merge some
unrelated text into the file. The server can't do that, but the client
is much smarter there.
Cheers,
-g