On 27 Aug 2008 at 9:51, Hyrum K. Wright wrote:
> alan.wood_at_clear.net.nz wrote:
> > I have done the same tests with your patch and it seems to work the same as what I
> > was using.
> > I have attached my patch which does what you were thinking and doesn't wait for
> > time stamps. It can't be that important to wait as none of the other error conditions
> > wait for them.
> >
> > Alan
> >
> > On 26 Aug 2008 at 16:23, Hyrum K. Wright wrote:
> >
> >> alan.wood_at_clear.net.nz wrote:
> >>> I have just traced a bug in the "svn copy URL_at_rev file" command.
> >>> While following instructions in the svn-book to retrieve a deleted file
> >>> svn copy svn://fred.ncs.local/ncs/truck/.../a.txt_at_211 a.txt
> >>> <segmentation fault>
> >>> This occurred on a Linux machine.
> >>> I repeated the command from windows command line with the same result.
> >>>
> >>> tested on svn version 1.5.1(Linux and win32) and 1.5.x branch(win32)
> >>>
> >>> I have traced this through to libsvn_client: copy.c line 1455
> >>>
> >>> The return from svn_wc_add_repos_file2() is not checked with SVN_ERR().
> >>>
> >>> This fails when the repository name does not exactly match the
> >>> repository name on the command line.
> >>> In my case the Linux attempt was different as I used the machines
> >>> actual name "fred" as opposed to "subversion.ncs.local" which the
> >>> working copy was built from. On the win32 case I was using a
> >>> file:///c:/... on the command line, but the working copy was checked
> >>> out by TortoiseSVN and had file:///C:/... in the working copy.
> >>>
> >>> The segmentation fault actually occurs a few lines down when calling
> >>> extend_wc_mergeinfo() with dst_entry==NULL.
> >>>
> >>> After changing to catch the error from the svn_wc_add_repos_file2()
> >>> I do get an error message about the repositories having different
> >>> roots which with careful inspection of C vs c may help the user to
> >>> get it right next time. Would be nicer to to check the repository by
> >>> uuid rather than name, then check the path within the repository as
> >>> a string. Or maybe I missed something completely.
> >> I haven't thoroughly tested this, but figured I'd throw it out here. Alan, if
> >> you could check and see if this fixes your problem, that'd be great. The patch
> >> is against trunk, but should apply cleanly against 1.5.x.
> >>
> >> I do wonder why we don't just wrap the call to svn_wc_add_repos_file2 with
> >> SVN_ERR(). Is there a particular reason why we need to wait for timestamps
> >> before returning the error?
> >>
> >> -Hyrum
> >>
> >> [[[
> >> If we can't add the target of the copy to the working copy, don't continue
> >> trying to calculate mergeinfo based upon the non-existent path. This fixes a
> >> segfault reported here:
> >> http://svn.haxx.se/dev/archive-2008-08/0667.shtml
> >>
> >> Found by: Alan Wood <alan.wood_at_clear.net.nz>
> >>
> >> * subversion/libsvn_client/copy.c
> >> (repos_to_wc_copy_single): Don't continue with mergeinfo calculation if the
> >> target of the copy couldn't be added to the working copy.
> >> ]]]
> >>
> > Index: copy.c
> > ===================================================================
> > --- copy.c (revision 32704)
> > +++ copy.c (working copy)
> > @@ -1452,12 +1452,12 @@
> >
> > if (! SVN_IS_VALID_REVNUM(src_revnum))
> > src_revnum = real_rev;
> >
> > - err = svn_wc_add_repos_file2
> > + SVN_ERR(err = svn_wc_add_repos_file2
> > (pair->dst, adm_access,
> > new_text_path, NULL, new_props, NULL,
> > same_repositories ? pair->src : NULL,
> > same_repositories ? src_revnum : SVN_INVALID_REVNUM,
> > - pool);
> > + pool));
> >
> > SVN_ERR(svn_wc_entry(&dst_entry, pair->dst, adm_access, FALSE, pool));
> > SVN_ERR(calculate_target_mergeinfo(ra_session, &src_mergeinfo,
>
> The general idea looks sound, but I'd still like some feedback about the
> sleeping for timestamps issue. In addition to the above, we should get rid for
> the err variable, and the subsequent use of it in SVN_ERR(err) at the end of the
> function.
The alternative though is to make sure that the error returns from the calls to
svn_wc_entry(), calculate_target_mergeinfo() & extend_wc_mergeinfo()
also call svn_sleep_for_timestamps() on the way out of the function.
Looking at the rest of copy.c there are examples of both with the
handling of the sleep.
My first reaction would be for the sleep to be done at places in
the code where it is important for the normal flow and also done at
the end of the public client routines in the case of an error. In
our case the svn_sleep_for_timestamps() would be done in
svn_client_move5() & svn_client_copy4() when (err).
This would keep the error handling code as simple as possible.
> Also, per our patch guidelines[1], could you attach the next version of the
> patch as text/x-diff, text/x-patch, or text/plain? This one was encoded as
> Application/Octet-stream, which made it difficult to review.
Thanks for the notes, I should read that again, I haven't sent patches before.
> Thanks,
> -Hyrum
>
> [1] http://subversion.tigris.org/hacking.html#patches
>
>
Received on 2008-08-27 23:39:26 CEST