-1 on just returning non errors from the commit info callback and making
callers suffer. I added that comment after finding that all callers suffer and
there is no way to see the difference, causing all kinds of problems. You can
only return errors there to fail fast.
You will cause broken working copies by just returning errors there. All the
client side post commit processing will be skipped when the commit function
returns an error… That is exactly what you want to do.
Adding that commit info hook on that level after the original api was already
fixed for several Subversion versions was an error… we shouldn’t have allowed
that callback to return errors in that way. Now where is the time machine to
fix it?
That api was designed (around 1.5/1.6) to just store the result and then
handling it post commit. In later versions we thought up that it was easier to
just print from there…
… but that turns post commit errors in normal commit errors.
which then behave different for every RA layer.
I remember that you spend time with me reviewing several bugs caused by this
problem and related bugfixes.
Please don’t just reintroduce this very serious problem to ‘not hide a
warning’. There are better ways to fix that problem… and not just for your
favorite RA layer, ignoring others.
These warnings on the fs layer were designed to be logged server side (e.g. in
the apache error log); not to be transferred to the client.
Bert
Sent from Mail for Windows 10
From: Daniel Shahaf
Sent: maandag 19 september 2016 09:01
To: dev@subversion.apache.org
Subject: Re: [PATCH] Re: ra_local doesn't report post-commit errors
Daniel Shahaf wrote on Sat, Sep 17, 2016 at 06:52:44 +0000:
> What concerns me at the moment is that the patch makes deltify_etc()
> return an error in situations where previously it did not. I think
> there are three possible solutions to this: revv the API deltify_etc()
> implements; make the change in 1.10 as an API erratum; or move the new
> logic from deltify_etc() to the libsvn_client consumers (svn and
> svnmucc). Haven't yet chosen among them.
Looking into this, there are two warning comments in the cmdline
client's commit callback, svn_cl__print_commit_info().
One of them is from r857282 (r17208), almost 11 years old now, saying
that scripts may interpret having stderr as implying "commit failed".
The other, 18 months young, is similar:
/* Be very careful with returning errors from this callback as those
will be returned as errors from editor->close_edit(...), which may
cause callers to assume that the commit itself failed.
⋮
*/
It seems to me that:
- If there is a post-commit error, its error chain should be printed to
stderr. We can signal to scripts that the commit succeeded by setting
the exit code to EXIT_SUCCESS or by using "W160013" rather than
"E160013" for the error messages ("W" for "warning").
- svn_cl__print_commit_info() should just return an error if there was
a post-commit error. If that confuses some libsvn_client caller into
wrongly thinking the commit failed, then that caller has a bug. (The
commit callback can already return an error even if the commit
succeeded, for example, whenever writing to stdout fails.)
Cheers,
Daniel
