On Wed, Apr 25, 2012 at 16:43, Philip Martin <philip.martin_at_wandisco.com> wrote:
> Greg Stein <gstein_at_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
Received on 2012-04-27 16:27:32 CEST