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

> -----Original Message-----
> From: Daniel Shahaf [mailto:d.s_at_daniel.shahaf.name]
> Sent: dinsdag 20 september 2016 11:11
> To: bert_at_qqmail.nl
> Cc: dev_at_subversion.apache.org
> Subject: Re: [PATCH] Re: ra_local doesn't report post-commit errors
>
> bert_at_qqmail.nl wrote on Mon, Sep 19, 2016 at 09:37:35 +0200:
> > -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.
>
> Veto duly noted, but:
>
> 1. I think those restrictions should be stated not in a comment in
> svn.c, but in the documentation of svn_commit_callback2_t.

I remember that we already put some comments there... But usually the api user implements both svn_commit_callback2_t and the processing of the result of close_edit(). The problem in libsvn_client is more subtle, because the standard implementation is shared between many code locations.

> 2. The commit callback can fail even when the commit succeeded, for
> example, when stdout is redirected to a file on a full partition the
> printf in svn/util.c will fail, or when a SIGINT occurs while the commit
> callback is running. The client/wc layer needs to cope with these
> scenarios.

Yes... it can always break... but usually the implementation knows their own scenarios. The current code explicitly ignores certain errors to allow the client side post commit processing to occur anyway on pipe errors.

Editor v2 will handle this in a better way, if we ever get to release that.

The problem is that we usually return errors directly, and this specific callback doesn't really fit in... We added it very late (5 or 6 versions after the editor) and didn't move the commit finalization to this callback.

>
> > 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 assume we agree that differences between RA layers should be minimal.

+1

> > 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.
>
> As I said, this is not only about post-commit FS processing errors but
> also about post-commit hook errors.
>
> % svnadmin create r
> % ln -s /usr/bin/false r/hooks/post-commit
> % svnmucc mkdir -U file://$PWD/r -mm $RANDOM
> r1 committed by daniel at 2016-09-20T09:04:13.915123Z
> % echo $?
> 0
> %
>
> svn reports post-commit hook errors. svnmucc doesn't. Does either of
> them need to be changed?

I think svnmucc just doesn't have the code to look at post commit errors (which are usually handed as warnings if I remember correctly)... I do remember that many versions of AnkhSVN didn't report such post commit error messages either. (Fixed years ago)

We should certainly change that and I would be +1 on backporting such a fix to 1.9 (and if possible 1.8).

To keep translations easy I would recommend trying if we can just copy &paste some message support from 'svn'.

        Bert
Received on 2016-09-20 11:48:56 CEST