RE: [PATCH] Re: ra_local doesn't report post-commit errors

-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_at_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
Received on 2016-09-19 09:37:43 CEST