On Fri, Dec 8, 2017, 3:49 AM Julian Foad <julianfoad_at_apache.org> wrote:
> Troy Curtis Jr wrote:
> > Julian Foad wrote:
> > [[[
> > Index: subversion/include/svn_client.h
> > ===================================================================
> > --- subversion/include/svn_client.h (revision 1817399)
> > +++ subversion/include/svn_client.h ( will
>
...
> > - * Because the callback is invoked before the patching attempt is
> made,
> > + * ### ? Because the callback is invoked before the patching
> > attempt is made,
> > * there is no guarantee that the target file will actually be
> patched
> > * successfully. Client implementations must pay attention to
> > notification
> > * feedback provided by svn_client_patch() to find out which paths
> > were
> > ]]]
> >
> >
> > How about:
> >
> > The callback is invoked before the patching attempt is made and
> > therefore there is
> > no guarantee that the target file will actually be patched successfully.
>
> Sorry, I didn't explain why I queried this paragraph. To me, "the
> patching attempt" began before the callback, which is the main thing I
> was clarifying in the first hunk above, so this paragraph is inaccurate.
> It is not clear to me whether the client really needs to pay attention
> to notifications, or if the callback can just look at whether any
> non-empty "reject" file was created, to know whether there was a
> problem. If there was no problem at this stage, then the patching is
> more or less guaranteed to be successful -- there are very few reasons
> why the final "move into place" step might fail.
>
Ah, I should have known it would not not be that simple!
I haven't looked at the underlying code, but based on your description it
seems like the warning is still reasonable. To me it more clearly indicates
the purpose of the callback: to hook in and modify the patching process.
There may not be many current reasons it could fail, but subversion apis
are very long lived, and additional cases may be found later on to bail
after this point.
You are right though, the patching process has already started, so perhaps
it should say something more like "When this callback is invoked, the
patching process is not yet complete, and could still fail for other
reasons."
Even if it is currently unlikely to fail, it limits the intended role for
this particular callback, leaving future changes to the implementation more
freedom to work within established bounds.
Troy
>
> - Julian
>
Received on 2017-12-08 13:19:04 CET