You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by David O'Shea <da...@s3group.com> on 2008/05/05 21:34:18 UTC
[PATCH] Fix merging with broken softlink as target
Hi,
When merging a branch with a symbolic link change onto another branch
where the original link destination doesn't exist, the merge fails.
(e.g http://svn.haxx.se/dev/archive-2008-02/0772.shtml)
This is a problem in 1.4.x and on the trunk. The attached patch (with
test included) against the trunk fixes this.
Regards,
David.
--
The information contained in this e-mail and in any attachments is confidential and is designated solely for the attention of the intended recipient(s). If you are not an intended recipient, you must not use, disclose, copy, distribute or retain this e-mail or any part thereof. If you have received this e-mail in error, please notify the sender by return e-mail and delete all copies of this e-mail from your computer system(s).
Please direct any additional queries to: communications@s3group.com.
Thank You.
Silicon and Software Systems Limited. Registered in Ireland no. 378073.
Registered Office: South County Business Park, Leopardstown, Dublin 18
Re: [PATCH] Fix merging with broken softlink as target
Posted by David O'Shea <da...@s3group.com>.
On 06/05/2008 01:13, Karl Fogel wrote:
> "David O'Shea" <da...@s3group.com> writes:
>> Index: subversion/libsvn_wc/merge.c
>> ===================================================================
>> --- subversion/libsvn_wc/merge.c (revision 31014)
>> +++ subversion/libsvn_wc/merge.c (working copy)
>> @@ -656,7 +656,10 @@
>> {
>> svn_boolean_t same;
>> SVN_ERR(svn_io_files_contents_same_p(&same, result_target,
>> - merge_target, pool));
>> + (is_binary ?
>> + merge_target :
>> + tmp_target),
>> + pool));
>>
>> *merge_outcome = same ? svn_wc_merge_unchanged : svn_wc_merge_merged;
>> }
>
> I've looked at the context around this change, and I'm still not seeing
> the connection between is_binary and broken symbolic links. That's more
> a statement about my ignorance than about your patch -- after all, you
> did fix the bug! -- but if you could go into more detail about exactly
> how this fixes the problem, that would be great. (Putting the
> explanation in a comment in the code might be best, in fact.)
Sure - I don't have the trunk checked out here, but will add a comment
and post a new patch this evening. However, basically the file is only
translated if it's not binary (i.e. !is_binary implies tmp_target exists
and contains the repository normalised version of merge_target, created
near the start of the same function).
David.
--
The information contained in this e-mail and in any attachments is confidential and is designated solely for the attention of the intended recipient(s). If you are not an intended recipient, you must not use, disclose, copy, distribute or retain this e-mail or any part thereof. If you have received this e-mail in error, please notify the sender by return e-mail and delete all copies of this e-mail from your computer system(s).
Please direct any additional queries to: communications@s3group.com.
Thank You.
Silicon and Software Systems Limited. Registered in Ireland no. 378073.
Registered Office: South County Business Park, Leopardstown, Dublin 18
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix merging with broken softlink as target
Posted by David O'Shea <da...@s3group.com>.
On 07/05/2008 22:10, Karl Fogel wrote:
> "David O'Shea" <da...@s3group.com> writes:
>> Updated patch attached.
>
> 'make check' says it fails two regression tests:
>
> update_tests.py 28: handle eol-style propchange during update
> merge_tests.py 41: handle eol-style propchange during merge
Hmm. I did run the merge tests, but don't remember seeing this failure.
I'll check again!
David.
--
The information contained in this e-mail and in any attachments is confidential and is designated solely for the attention of the intended recipient(s). If you are not an intended recipient, you must not use, disclose, copy, distribute or retain this e-mail or any part thereof. If you have received this e-mail in error, please notify the sender by return e-mail and delete all copies of this e-mail from your computer system(s).
Please direct any additional queries to: communications@s3group.com.
Thank You.
Silicon and Software Systems Limited. Registered in Ireland no. 378073.
Registered Office: South County Business Park, Leopardstown, Dublin 18
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix merging with broken softlink as target
Posted by David Glasser <gl...@davidglasser.net>.
On Thu, May 8, 2008 at 12:47 PM, Erik Huelsmann <eh...@gmail.com> wrote:
> On Thu, May 8, 2008 at 9:03 PM, Karl Fogel <kf...@red-bean.com> wrote:
>> "David O'Shea" <da...@s3group.com> writes:
>>
>> > Ok, this is interesting. I've been looking at merge test 41 for a
>> > while and the difference with the patch applied is that a merge of a
>> > propset eol-style change into the working copy applies the propset but
>> > does not rework the line endings in the local file until committed.
>> >
>> > Without the patch applied, the line endings of the file in the wc get
>> > updated immediately as part of the merge.
>> >
>> > However, the first situation (where the patch is applied) means that
>> > merging the propset now has the same effect as manually doing the
>> > propset (i.e. manually doing svn ps eol-style does not change the line
>> > endings in the local file until you commit).
>> >
>> > So, my question is this... is the fact that the line endings were
>> > changed as part of the merge of a propset, but not by a manual propset
>> > a bug that this patch (inadvertently) fixes or was that difference a
>> > deliberate decision?
>>
>> Hmm, that's interesting.
>>
>> Yes, it looks like an inconsistency on the surface, but I'm not sure
>> it's inconsistent when considered deeply...
>>
>> When you receive a propset of svn:eol-style from the repository, you're
>> also receiving whatever effect that propset had on the file it was
>> committed on. That is, in the merge-from-repos case, the commit has
>> already taken place, so it's natural that the line endings would change,
>> along with the property, on the receiver's side.
>>
>> IOW, receiving the propset via update/merge is not quite the same as
>> doing the propset manually in your working copy. One represents a
>> commit that actually *has* taken place, the other represents an intent
>> to commit.
>>
>> So I think the current behavior should be preserved.
>
> So do I :-) It took me a few weeks before I figured out what the right
> merge behaviour given an svn:eol-style property change, minimizing at
> the same time the number of eol-translations. This is a change way
> past 1.0 though. Can't remember exactly when ...
And in fact this stuff is pretty subtle... I just fixed a bug in it
recently (r30756).
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix merging with broken softlink as target
Posted by David O'Shea <da...@s3group.com>.
On 14/05/2008 17:26, Karl Fogel wrote:
> "David O'Shea" <da...@s3group.com> writes:
>> On 13/05/2008 20:53, Karl Fogel wrote:
>>> ...and I've applied the patch in r31159. Thanks!
>> Great! Any chance of getting this in 1.5 too?
>
> Well, I can nominate it, but it may still wait till 1.5.1, as 1.5.0 is
> already in release candidate stage (and we hope not to have to restart
> its soak period!).
1.5.1 would be great.
Cheers,
David.
--
The information contained in this e-mail and in any attachments is confidential and is designated solely for the attention of the intended recipient(s). If you are not an intended recipient, you must not use, disclose, copy, distribute or retain this e-mail or any part thereof. If you have received this e-mail in error, please notify the sender by return e-mail and delete all copies of this e-mail from your computer system(s).
Please direct any additional queries to: communications@s3group.com.
Thank You.
Silicon and Software Systems Limited. Registered in Ireland no. 378073.
Registered Office: South County Business Park, Leopardstown, Dublin 18
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix merging with broken softlink as target
Posted by Karl Fogel <kf...@red-bean.com>.
"David O'Shea" <da...@s3group.com> writes:
> On 13/05/2008 20:53, Karl Fogel wrote:
>> ...and I've applied the patch in r31159. Thanks!
>
> Great! Any chance of getting this in 1.5 too?
Well, I can nominate it, but it may still wait till 1.5.1, as 1.5.0 is
already in release candidate stage (and we hope not to have to restart
its soak period!).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix merging with broken softlink as target
Posted by David O'Shea <da...@s3group.com>.
On 13/05/2008 20:53, Karl Fogel wrote:
> ...and I've applied the patch in r31159. Thanks!
Great! Any chance of getting this in 1.5 too?
Thanks,
David.
--
The information contained in this e-mail and in any attachments is confidential and is designated solely for the attention of the intended recipient(s). If you are not an intended recipient, you must not use, disclose, copy, distribute or retain this e-mail or any part thereof. If you have received this e-mail in error, please notify the sender by return e-mail and delete all copies of this e-mail from your computer system(s).
Please direct any additional queries to: communications@s3group.com.
Thank You.
Silicon and Software Systems Limited. Registered in Ireland no. 378073.
Registered Office: South County Business Park, Leopardstown, Dublin 18
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix merging with broken softlink as target
Posted by Karl Fogel <kf...@red-bean.com>.
"David O'Shea" <da...@s3group.com> writes:
> I have neither neoncheck nor serfcheck targets in my makefile. FC8
> comes with neon 0.25.5 which configure seems to pick up alright and
> the build did produce libsvn_ra_neon-1.a.
That's odd. I just produce my Makefile the way INSTALL says to...
>> The above failure is from 'make neoncheck'. Do you know what's causing
>> it? (I haven't looked into it yet, having choosen instead to
>> shamelessly toss the ball back into your court :-) .)
>
> Luckily for me Daniel has taken responsibility for this one :-)
...and I've applied the patch in r31159. Thanks!
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix merging with broken softlink as target
Posted by David O'Shea <da...@s3group.com>.
On 12/05/2008 21:26, Karl Fogel wrote:
> Karl Fogel <kf...@red-bean.com> writes:
>> Hmmm. If the goal is to check for symlinks, not just specialness, maybe
>> it would be a good idea to check that the contents of the special file
>> begins with "link:", indicating a symlink? There may be other kinds of
>> special files in the future...
>>
>> Hmm, then again, it's highly likely that future kinds of special files
>> would want the same treatment anyway. So I've just adjusted the comment
>> a bit to talk about that; I'm running 'make check' now (paranoia, I know
>> you already did it) and will commit soon.
Yes, that thought did occur to me but I came to the same conclusion - I
guess I should have made this obvious in the comment - sorry!
> Hmmm. I'm still getting a failure over ra_neon:
>
> prop_tests.py 26: test handling invalid svn:* property values
>
> When I say I run 'make check', what I really mean is I run the
> regression test suite over at least three RA layers:
>
> make check CLEANUP=true
> make svncheck CLEANUP=true
> make neoncheck CLEANUP=true
> make serfcheck CLEANUP=true /* Usually run this, not always */
I have neither neoncheck nor serfcheck targets in my makefile. FC8 comes
with neon 0.25.5 which configure seems to pick up alright and the build
did produce libsvn_ra_neon-1.a.
> The above failure is from 'make neoncheck'. Do you know what's causing
> it? (I haven't looked into it yet, having choosen instead to
> shamelessly toss the ball back into your court :-) .)
Luckily for me Daniel has taken responsibility for this one :-)
David.
--
The information contained in this e-mail and in any attachments is confidential and is designated solely for the attention of the intended recipient(s). If you are not an intended recipient, you must not use, disclose, copy, distribute or retain this e-mail or any part thereof. If you have received this e-mail in error, please notify the sender by return e-mail and delete all copies of this e-mail from your computer system(s).
Please direct any additional queries to: communications@s3group.com.
Thank You.
Silicon and Software Systems Limited. Registered in Ireland no. 378073.
Registered Office: South County Business Park, Leopardstown, Dublin 18
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: prop_tests 26 over neon
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Karl Fogel wrote on Tue, 13 May 2008 at 15:49 -0400:
> Daniel Shahaf <d....@daniel.shahaf.co.il> writes:
> >> Thanks. I checked the buildbot now (should have done that before, too),
> >> and it's my fault (not David's); the neon buildbot fails:
> >>
> >> I assume that the correct fix is to mark test 26 (which requires
> >> revprop changes to be enabled) as XFail over DAV.
> >
> > Done in r31157; sorry for not fixing that sooner.
>
> Thanks, Daniel. Due to the clock above Brian Fitzpatrick's desk being
> an hour ahead yesterday (blame Fitz! blame Fitz!), I never got a chance
> to do this, despite saying I would in IRC. I was busy frantically
> packing up to leave, only to arrive at my destination an hour early :-).
>
Was this clock another practical joke? :) Thanks for the followup.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: prop_tests 26 over neon
Posted by Karl Fogel <kf...@red-bean.com>.
Daniel Shahaf <d....@daniel.shahaf.co.il> writes:
>> Thanks. I checked the buildbot now (should have done that before, too),
>> and it's my fault (not David's); the neon buildbot fails:
>>
>> I assume that the correct fix is to mark test 26 (which requires
>> revprop changes to be enabled) as XFail over DAV.
>
> Done in r31157; sorry for not fixing that sooner.
Thanks, Daniel. Due to the clock above Brian Fitzpatrick's desk being
an hour ahead yesterday (blame Fitz! blame Fitz!), I never got a chance
to do this, despite saying I would in IRC. I was busy frantically
packing up to leave, only to arrive at my destination an hour early :-).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: prop_tests 26 over neon (was: Re: [PATCH] Fix merging with broken
softlink as target)
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Daniel Shahaf wrote on Tue, 13 May 2008 at 00:00 +0300:
> Karl Fogel wrote on Mon, 12 May 2008 at 16:52 -0400:
> > Daniel Shahaf <d....@daniel.shahaf.co.il> writes:
> > > Karl, does it pass without David's patch? I only ran file:// checks
> > > here...
> > >
> > > (Normally I run either svnserveautocheck or davautocheck, chosen at
> > > random)
> >
> > Erm, I'm pretty sure it does, because I had run the test suite earlier
> > for some other changes. I'd check right now, but I have another patch
> > in-tree that I'm testing while I review it (Troy Curtis'), so it would
> > be hard to confirm that a pristine tree passes until that's done.
> >
> > -Karl
> >
>
> Thanks. I checked the buildbot now (should have done that before, too),
> and it's my fault (not David's); the neon buildbot fails:
>
...
>
> I assume that the correct fix is to mark test 26 (which requires
> revprop changes to be enabled) as XFail over DAV.
>
Done in r31157; sorry for not fixing that sooner.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
prop_tests 26 over neon (was: Re: [PATCH] Fix merging with broken
softlink as target)
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Karl Fogel wrote on Mon, 12 May 2008 at 16:52 -0400:
> Daniel Shahaf <d....@daniel.shahaf.co.il> writes:
> > Karl, does it pass without David's patch? I only ran file:// checks
> > here...
> >
> > (Normally I run either svnserveautocheck or davautocheck, chosen at
> > random)
>
> Erm, I'm pretty sure it does, because I had run the test suite earlier
> for some other changes. I'd check right now, but I have another patch
> in-tree that I'm testing while I review it (Troy Curtis'), so it would
> be hard to confirm that a pristine tree passes until that's done.
>
> -Karl
>
Thanks. I checked the buildbot now (should have done that before, too),
and it's my fault (not David's); the neon buildbot fails:
EXPECTED STDERR (regexp):
.*unexpected property value.*|.*Bogus date.*
ACTUAL STDERR:
subversion/libsvn_ra_neon/fetch.c:1175: (apr_err=175002)
svn: DAV request failed; it's possible that the repository's pre-revprop-change hook either failed or is non-existent
subversion/libsvn_ra_neon/props.c:1151: (apr_err=175008)
svn: At least one property change failed; repository is unchanged
subversion/libsvn_ra_neon/util.c:206: (apr_err=175002)
svn: Error setting property 'date':
Could not execute PROPPATCH.
EXCEPTION: SVNUnmatchedError
Traceback (most recent call last):
File "/Users/lgo/slavedir/osx10.4-gcc4.0.1-ia32/build/subversion/tests/cmdline/svntest/main.py", line 1083, in run
rc = apply(self.pred.run, (), kw)
File "/Users/lgo/slavedir/osx10.4-gcc4.0.1-ia32/build/subversion/tests/cmdline/svntest/testcase.py", line 121, in run
return self.func(sandbox)
File "/Users/lgo/slavedir/osx10.4-gcc4.0.1-ia32/build/subversion/tests/cmdline/prop_tests.py", line 1649, in invalid_propvalues
repo_url)
File "/Users/lgo/slavedir/osx10.4-gcc4.0.1-ia32/build/subversion/tests/cmdline/svntest/actions.py", line 211, in run_and_verify_svn
expected_exit, *varargs)
File "/Users/lgo/slavedir/osx10.4-gcc4.0.1-ia32/build/subversion/tests/cmdline/svntest/actions.py", line 248, in run_and_verify_svn2
verify.verify_outputs(message, out, err, expected_stdout, expected_stderr)
File "/Users/lgo/slavedir/osx10.4-gcc4.0.1-ia32/build/subversion/tests/cmdline/svntest/verify.py", line 331, in verify_outputs
compare_and_display_lines(message, label, expected, actual, raisable)
File "/Users/lgo/slavedir/osx10.4-gcc4.0.1-ia32/build/subversion/tests/cmdline/svntest/verify.py", line 303, in compare_and_display_lines
raise raisable
SVNUnmatchedError
FAIL: prop_tests.py 26: test handling invalid svn:* property values
END: prop_tests.py
Based on the comment in prop_tests.py:test_list,
# If we learn how to write a pre-revprop-change hook for
# non-Posix platforms, we won't have to skip here:
# TODO(epg): Removed Skip as long as we have this XFail
# because I couldn't get Skip and XFail to interact
# properly (it kept showing the failure and then
# printing PASS instead of XFAIL).
#Skip(revprop_change, is_non_posix_and_non_windows_os),
XFail(revprop_change, svntest.main.is_ra_type_dav),
I assume that the correct fix is to mark test 26 (which requires
revprop changes to be enabled) as XFail over DAV.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix merging with broken softlink as target
Posted by Karl Fogel <kf...@red-bean.com>.
Daniel Shahaf <d....@daniel.shahaf.co.il> writes:
> Karl, does it pass without David's patch? I only ran file:// checks
> here...
>
> (Normally I run either svnserveautocheck or davautocheck, chosen at
> random)
Erm, I'm pretty sure it does, because I had run the test suite earlier
for some other changes. I'd check right now, but I have another patch
in-tree that I'm testing while I review it (Troy Curtis'), so it would
be hard to confirm that a pristine tree passes until that's done.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix merging with broken softlink as target
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Karl Fogel wrote on Mon, 12 May 2008 at 16:26 -0400:
> Karl Fogel <kf...@red-bean.com> writes:
> > Hmmm. If the goal is to check for symlinks, not just specialness, maybe
> > it would be a good idea to check that the contents of the special file
> > begins with "link:", indicating a symlink? There may be other kinds of
> > special files in the future...
> >
> > Hmm, then again, it's highly likely that future kinds of special files
> > would want the same treatment anyway. So I've just adjusted the comment
> > a bit to talk about that; I'm running 'make check' now (paranoia, I know
> > you already did it) and will commit soon.
>
> Hmmm. I'm still getting a failure over ra_neon:
>
> prop_tests.py 26: test handling invalid svn:* property values
>
Karl, does it pass without David's patch? I only ran file:// checks
here...
(Normally I run either svnserveautocheck or davautocheck, chosen at
random)
Daniel
> When I say I run 'make check', what I really mean is I run the
> regression test suite over at least three RA layers:
>
> make check CLEANUP=true
> make svncheck CLEANUP=true
> make neoncheck CLEANUP=true
> make serfcheck CLEANUP=true /* Usually run this, not always */
>
> Now, the latter two, Neon and Serf, require that I start up the httpd
> server of course. I usually do that by hand, but 'make davautocheck'
> should automate the process if you want.
>
> The above failure is from 'make neoncheck'. Do you know what's causing
> it? (I haven't looked into it yet, having choosen instead to
> shamelessly toss the ball back into your court :-) .)
>
> Below is the exact patch I was testing, using approximately r31143 of
> trunk:
>
> [[[
> Fix "file not found" error when a merge target is a broken symbolic link.
>
> Patch by: David O'Shea <da...@s3group.com>
>
> * subversion/libsvn_wc/merge.c
> (svn_wc__merge_internal): Diff against detranslated file if the
> target is "special".
>
> * subversion/tests/cmdlink/merge_tests.py
> (merge_broken_link): New test function.
> (test_list): Call the new test function.
> ]]]
>
> Index: subversion/libsvn_wc/merge.c
> ===================================================================
> --- subversion/libsvn_wc/merge.c (revision 31142)
> +++ subversion/libsvn_wc/merge.c (working copy)
> @@ -674,9 +674,18 @@
> }
> else
> {
> - svn_boolean_t same;
> + svn_boolean_t same, special;
> + /* If 'special', then use the detranslated form of the
> + target file. This is so we don't try to follow symlinks,
> + but the same treatment is probably also appropriate for
> + whatever special file types we may invent in the future. */
> + SVN_ERR(svn_wc__get_special(&special, merge_target,
> + adm_access, pool));
> SVN_ERR(svn_io_files_contents_same_p(&same, result_target,
> - merge_target, pool));
> + (special ?
> + tmp_target :
> + merge_target),
> + pool));
>
> *merge_outcome = same ? svn_wc_merge_unchanged : svn_wc_merge_merged;
> }
> Index: subversion/tests/cmdline/merge_tests.py
> ===================================================================
> --- subversion/tests/cmdline/merge_tests.py (revision 31142)
> +++ subversion/tests/cmdline/merge_tests.py (working copy)
> @@ -11258,6 +11258,7 @@
> expected_status.tweak('H_COPY/psi_moved', status='MM')
> svntest.actions.run_and_verify_status(wc_dir, expected_status)
>
> +
> #----------------------------------------------------------------------
> # Issue #3157
> def dont_explicitly_record_implicit_mergeinfo(sbox):
> @@ -11349,6 +11350,32 @@
> None, None, None, None, None, 1)
>
>
> +# Test for issue where merging a change to a broken link fails
> +def merge_broken_link(sbox):
> + "merge with broken symlinks in target"
> +
> + # Create our good 'ole greek tree.
> + sbox.build()
> + wc_dir = sbox.wc_dir
> + src_path = os.path.join(wc_dir, 'A', 'B', 'E')
> + copy_path = os.path.join(wc_dir, 'A', 'B', 'E_COPY')
> + link_path = os.path.join(src_path, 'beta_link')
> +
> + os.symlink('beta_broken', link_path)
> + svntest.main.run_svn(None, 'add', link_path)
> + svntest.main.run_svn(None, 'commit', '-m', 'Create a broken link', link_path)
> + svntest.main.run_svn(None, 'copy', src_path, copy_path)
> + svntest.main.run_svn(None, 'commit', '-m', 'Copy the tree with the broken link',
> + copy_path)
> + os.unlink(link_path)
> + os.symlink('beta', link_path)
> + svntest.main.run_svn(None, 'commit', '-m', 'Fix a broken link', link_path)
> + svntest.actions.run_and_verify_svn(
> + None,
> + expected_merge_output([[4]], 'U ' + copy_path + '/beta_link\n'),
> + [], 'merge', '-c4', src_path, copy_path)
> +
> +
> ########################################################################
> # Run the tests
>
> @@ -11515,6 +11542,7 @@
> server_has_mergeinfo)),
> SkipUnless(dont_explicitly_record_implicit_mergeinfo,
> server_has_mergeinfo),
> + merge_broken_link,
> ]
>
> if __name__ == '__main__':
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix merging with broken softlink as target
Posted by Karl Fogel <kf...@red-bean.com>.
Karl Fogel <kf...@red-bean.com> writes:
> Hmmm. If the goal is to check for symlinks, not just specialness, maybe
> it would be a good idea to check that the contents of the special file
> begins with "link:", indicating a symlink? There may be other kinds of
> special files in the future...
>
> Hmm, then again, it's highly likely that future kinds of special files
> would want the same treatment anyway. So I've just adjusted the comment
> a bit to talk about that; I'm running 'make check' now (paranoia, I know
> you already did it) and will commit soon.
Hmmm. I'm still getting a failure over ra_neon:
prop_tests.py 26: test handling invalid svn:* property values
When I say I run 'make check', what I really mean is I run the
regression test suite over at least three RA layers:
make check CLEANUP=true
make svncheck CLEANUP=true
make neoncheck CLEANUP=true
make serfcheck CLEANUP=true /* Usually run this, not always */
Now, the latter two, Neon and Serf, require that I start up the httpd
server of course. I usually do that by hand, but 'make davautocheck'
should automate the process if you want.
The above failure is from 'make neoncheck'. Do you know what's causing
it? (I haven't looked into it yet, having choosen instead to
shamelessly toss the ball back into your court :-) .)
Below is the exact patch I was testing, using approximately r31143 of
trunk:
[[[
Fix "file not found" error when a merge target is a broken symbolic link.
Patch by: David O'Shea <da...@s3group.com>
* subversion/libsvn_wc/merge.c
(svn_wc__merge_internal): Diff against detranslated file if the
target is "special".
* subversion/tests/cmdlink/merge_tests.py
(merge_broken_link): New test function.
(test_list): Call the new test function.
]]]
Index: subversion/libsvn_wc/merge.c
===================================================================
--- subversion/libsvn_wc/merge.c (revision 31142)
+++ subversion/libsvn_wc/merge.c (working copy)
@@ -674,9 +674,18 @@
}
else
{
- svn_boolean_t same;
+ svn_boolean_t same, special;
+ /* If 'special', then use the detranslated form of the
+ target file. This is so we don't try to follow symlinks,
+ but the same treatment is probably also appropriate for
+ whatever special file types we may invent in the future. */
+ SVN_ERR(svn_wc__get_special(&special, merge_target,
+ adm_access, pool));
SVN_ERR(svn_io_files_contents_same_p(&same, result_target,
- merge_target, pool));
+ (special ?
+ tmp_target :
+ merge_target),
+ pool));
*merge_outcome = same ? svn_wc_merge_unchanged : svn_wc_merge_merged;
}
Index: subversion/tests/cmdline/merge_tests.py
===================================================================
--- subversion/tests/cmdline/merge_tests.py (revision 31142)
+++ subversion/tests/cmdline/merge_tests.py (working copy)
@@ -11258,6 +11258,7 @@
expected_status.tweak('H_COPY/psi_moved', status='MM')
svntest.actions.run_and_verify_status(wc_dir, expected_status)
+
#----------------------------------------------------------------------
# Issue #3157
def dont_explicitly_record_implicit_mergeinfo(sbox):
@@ -11349,6 +11350,32 @@
None, None, None, None, None, 1)
+# Test for issue where merging a change to a broken link fails
+def merge_broken_link(sbox):
+ "merge with broken symlinks in target"
+
+ # Create our good 'ole greek tree.
+ sbox.build()
+ wc_dir = sbox.wc_dir
+ src_path = os.path.join(wc_dir, 'A', 'B', 'E')
+ copy_path = os.path.join(wc_dir, 'A', 'B', 'E_COPY')
+ link_path = os.path.join(src_path, 'beta_link')
+
+ os.symlink('beta_broken', link_path)
+ svntest.main.run_svn(None, 'add', link_path)
+ svntest.main.run_svn(None, 'commit', '-m', 'Create a broken link', link_path)
+ svntest.main.run_svn(None, 'copy', src_path, copy_path)
+ svntest.main.run_svn(None, 'commit', '-m', 'Copy the tree with the broken link',
+ copy_path)
+ os.unlink(link_path)
+ os.symlink('beta', link_path)
+ svntest.main.run_svn(None, 'commit', '-m', 'Fix a broken link', link_path)
+ svntest.actions.run_and_verify_svn(
+ None,
+ expected_merge_output([[4]], 'U ' + copy_path + '/beta_link\n'),
+ [], 'merge', '-c4', src_path, copy_path)
+
+
########################################################################
# Run the tests
@@ -11515,6 +11542,7 @@
server_has_mergeinfo)),
SkipUnless(dont_explicitly_record_implicit_mergeinfo,
server_has_mergeinfo),
+ merge_broken_link,
]
if __name__ == '__main__':
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix merging with broken softlink as target
Posted by Karl Fogel <kf...@red-bean.com>.
"David O'Shea" <da...@s3group.com> writes:
> Updated patch attached. This now explicitly checks the target's
> "special" status instead of relying on is_binary (should have done
> that in the first place really...)
>
> "make check" passes with this patch.
Thanks (and thanks for testing it). See comments below...
[[[
> Fix a file not found error when a merge target is a broken symbolic link.
>
> * subversion/libsvn_wc/merge.c
> (svn_wc__merge_internal): Diff translated target file if special.
>
> * subversion/tests/cmdlink/merge_tests.py
> (merge_broken_link): New test function.
> (test_list): Call the new test function.
> ]]]
>
> Index: subversion/libsvn_wc/merge.c
> ===================================================================
> --- subversion/libsvn_wc/merge.c (revision 31119)
> +++ subversion/libsvn_wc/merge.c (working copy)
> @@ -674,9 +674,16 @@
> }
> else
> {
> - svn_boolean_t same;
> + svn_boolean_t same, special;
> + /* Make sure we use the repository normalised form of the
> + target file if it is special (i.e. don't try to follow
> + symlinks). */
> + SVN_ERR(svn_wc__get_special(&special, merge_target, adm_access, pool));
> SVN_ERR(svn_io_files_contents_same_p(&same, result_target,
> - merge_target, pool));
> + (special ?
> + tmp_target :
> + merge_target),
> + pool));
>
> *merge_outcome = same ? svn_wc_merge_unchanged : svn_wc_merge_merged;
> }
Hmmm. If the goal is to check for symlinks, not just specialness, maybe
it would be a good idea to check that the contents of the special file
begins with "link:", indicating a symlink? There may be other kinds of
special files in the future...
Hmm, then again, it's highly likely that future kinds of special files
would want the same treatment anyway. So I've just adjusted the comment
a bit to talk about that; I'm running 'make check' now (paranoia, I know
you already did it) and will commit soon.
Thank you!
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix merging with broken softlink as target
Posted by David O'Shea <da...@s3group.com>.
Hi,
On 09/05/2008 09:27, David O'Shea wrote:
> Ok - I'll take another look at the patch over the weekend and see if I
> can figure anything out. Things are rarely as simple as they first seem!
Updated patch attached. This now explicitly checks the target's
"special" status instead of relying on is_binary (should have done that
in the first place really...)
"make check" passes with this patch.
David.
--
The information contained in this e-mail and in any attachments is confidential and is designated solely for the attention of the intended recipient(s). If you are not an intended recipient, you must not use, disclose, copy, distribute or retain this e-mail or any part thereof. If you have received this e-mail in error, please notify the sender by return e-mail and delete all copies of this e-mail from your computer system(s).
Please direct any additional queries to: communications@s3group.com.
Thank You.
Silicon and Software Systems Limited. Registered in Ireland no. 378073.
Registered Office: South County Business Park, Leopardstown, Dublin 18
Re: [PATCH] Fix merging with broken softlink as target
Posted by David O'Shea <da...@s3group.com>.
On 08/05/2008 20:47, Erik Huelsmann wrote:
> On Thu, May 8, 2008 at 9:03 PM, Karl Fogel <kf...@red-bean.com> wrote:
>> "David O'Shea" <da...@s3group.com> writes:
>>
>> > So, my question is this... is the fact that the line endings were
>> > changed as part of the merge of a propset, but not by a manual propset
>> > a bug that this patch (inadvertently) fixes or was that difference a
>> > deliberate decision?
>>
>> So I think the current behavior should be preserved.
>
> So do I :-)
Ok - I'll take another look at the patch over the weekend and see if I
can figure anything out. Things are rarely as simple as they first seem!
Cheers,
David.
--
The information contained in this e-mail and in any attachments is confidential and is designated solely for the attention of the intended recipient(s). If you are not an intended recipient, you must not use, disclose, copy, distribute or retain this e-mail or any part thereof. If you have received this e-mail in error, please notify the sender by return e-mail and delete all copies of this e-mail from your computer system(s).
Please direct any additional queries to: communications@s3group.com.
Thank You.
Silicon and Software Systems Limited. Registered in Ireland no. 378073.
Registered Office: South County Business Park, Leopardstown, Dublin 18
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix merging with broken softlink as target
Posted by Erik Huelsmann <eh...@gmail.com>.
On Thu, May 8, 2008 at 9:03 PM, Karl Fogel <kf...@red-bean.com> wrote:
> "David O'Shea" <da...@s3group.com> writes:
>
> > Ok, this is interesting. I've been looking at merge test 41 for a
> > while and the difference with the patch applied is that a merge of a
> > propset eol-style change into the working copy applies the propset but
> > does not rework the line endings in the local file until committed.
> >
> > Without the patch applied, the line endings of the file in the wc get
> > updated immediately as part of the merge.
> >
> > However, the first situation (where the patch is applied) means that
> > merging the propset now has the same effect as manually doing the
> > propset (i.e. manually doing svn ps eol-style does not change the line
> > endings in the local file until you commit).
> >
> > So, my question is this... is the fact that the line endings were
> > changed as part of the merge of a propset, but not by a manual propset
> > a bug that this patch (inadvertently) fixes or was that difference a
> > deliberate decision?
>
> Hmm, that's interesting.
>
> Yes, it looks like an inconsistency on the surface, but I'm not sure
> it's inconsistent when considered deeply...
>
> When you receive a propset of svn:eol-style from the repository, you're
> also receiving whatever effect that propset had on the file it was
> committed on. That is, in the merge-from-repos case, the commit has
> already taken place, so it's natural that the line endings would change,
> along with the property, on the receiver's side.
>
> IOW, receiving the propset via update/merge is not quite the same as
> doing the propset manually in your working copy. One represents a
> commit that actually *has* taken place, the other represents an intent
> to commit.
>
> So I think the current behavior should be preserved.
So do I :-) It took me a few weeks before I figured out what the right
merge behaviour given an svn:eol-style property change, minimizing at
the same time the number of eol-translations. This is a change way
past 1.0 though. Can't remember exactly when ...
> I'm not saying it
> was a conscious decision (it may have been, I just don't remember). But
> I'm pretty sure we don't want to change a merge behavior that seems to
> have been working well for some years.
>
> Possibly we could change 'svn propset' to fix up the line endings at
> propset time... But surely we must have considered that and decided
> against? It's too obvious not to have been discussed. Did you see a
> comment anywhere indicating why we don't?
HTH,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix merging with broken softlink as target
Posted by Karl Fogel <kf...@red-bean.com>.
"David O'Shea" <da...@s3group.com> writes:
> Ok, this is interesting. I've been looking at merge test 41 for a
> while and the difference with the patch applied is that a merge of a
> propset eol-style change into the working copy applies the propset but
> does not rework the line endings in the local file until committed.
>
> Without the patch applied, the line endings of the file in the wc get
> updated immediately as part of the merge.
>
> However, the first situation (where the patch is applied) means that
> merging the propset now has the same effect as manually doing the
> propset (i.e. manually doing svn ps eol-style does not change the line
> endings in the local file until you commit).
>
> So, my question is this... is the fact that the line endings were
> changed as part of the merge of a propset, but not by a manual propset
> a bug that this patch (inadvertently) fixes or was that difference a
> deliberate decision?
Hmm, that's interesting.
Yes, it looks like an inconsistency on the surface, but I'm not sure
it's inconsistent when considered deeply...
When you receive a propset of svn:eol-style from the repository, you're
also receiving whatever effect that propset had on the file it was
committed on. That is, in the merge-from-repos case, the commit has
already taken place, so it's natural that the line endings would change,
along with the property, on the receiver's side.
IOW, receiving the propset via update/merge is not quite the same as
doing the propset manually in your working copy. One represents a
commit that actually *has* taken place, the other represents an intent
to commit.
So I think the current behavior should be preserved. I'm not saying it
was a conscious decision (it may have been, I just don't remember). But
I'm pretty sure we don't want to change a merge behavior that seems to
have been working well for some years.
Possibly we could change 'svn propset' to fix up the line endings at
propset time... But surely we must have considered that and decided
against? It's too obvious not to have been discussed. Did you see a
comment anywhere indicating why we don't?
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix merging with broken softlink as target
Posted by David O'Shea <da...@s3group.com>.
On 07/05/2008 22:10, Karl Fogel wrote:
> "David O'Shea" <da...@s3group.com> writes:
>> Updated patch attached.
>
> 'make check' says it fails two regression tests:
>
> update_tests.py 28: handle eol-style propchange during update
> merge_tests.py 41: handle eol-style propchange during merge
Ok, this is interesting. I've been looking at merge test 41 for a while
and the difference with the patch applied is that a merge of a propset
eol-style change into the working copy applies the propset but does not
rework the line endings in the local file until committed.
Without the patch applied, the line endings of the file in the wc get
updated immediately as part of the merge.
However, the first situation (where the patch is applied) means that
merging the propset now has the same effect as manually doing the
propset (i.e. manually doing svn ps eol-style does not change the line
endings in the local file until you commit).
So, my question is this... is the fact that the line endings were
changed as part of the merge of a propset, but not by a manual propset a
bug that this patch (inadvertently) fixes or was that difference a
deliberate decision?
David.
--
The information contained in this e-mail and in any attachments is confidential and is designated solely for the attention of the intended recipient(s). If you are not an intended recipient, you must not use, disclose, copy, distribute or retain this e-mail or any part thereof. If you have received this e-mail in error, please notify the sender by return e-mail and delete all copies of this e-mail from your computer system(s).
Please direct any additional queries to: communications@s3group.com.
Thank You.
Silicon and Software Systems Limited. Registered in Ireland no. 378073.
Registered Office: South County Business Park, Leopardstown, Dublin 18
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix merging with broken softlink as target
Posted by Karl Fogel <kf...@red-bean.com>.
"David O'Shea" <da...@s3group.com> writes:
> Updated patch attached.
'make check' says it fails two regression tests:
update_tests.py 28: handle eol-style propchange during update
merge_tests.py 41: handle eol-style propchange during merge
(I strongly urge running the tests -- you'd be surprised how often a
change has unexpected consequences.)
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix merging with broken softlink as target
Posted by David O'Shea <da...@s3group.com>.
Hi,
Updated patch attached.
David.
--
The information contained in this e-mail and in any attachments is confidential and is designated solely for the attention of the intended recipient(s). If you are not an intended recipient, you must not use, disclose, copy, distribute or retain this e-mail or any part thereof. If you have received this e-mail in error, please notify the sender by return e-mail and delete all copies of this e-mail from your computer system(s).
Please direct any additional queries to: communications@s3group.com.
Thank You.
Silicon and Software Systems Limited. Registered in Ireland no. 378073.
Registered Office: South County Business Park, Leopardstown, Dublin 18
Re: [PATCH] Fix merging with broken softlink as target
Posted by Karl Fogel <kf...@red-bean.com>.
"David O'Shea" <da...@s3group.com> writes:
> When merging a branch with a symbolic link change onto another branch
> where the original link destination doesn't exist, the merge fails.
>
> (e.g http://svn.haxx.se/dev/archive-2008-02/0772.shtml)
>
> This is a problem in 1.4.x and on the trunk. The attached patch (with
> test included) against the trunk fixes this.
Thanks for the fix (and with a test, too). Some questions...
> [[[
> Fix a file not found error when a merge target is a broken symbolic link.
>
> * subversion/libsvn_wc/merge.c
> (svn_wc__merge_internal): Diff translated target file if not binary.
>
> * subversion/tests/cmdlink/merge_tests.py
> (merge_broken_link): New test function.
> (test_list): Call the new test function.
> ]]]
>
> Index: subversion/libsvn_wc/merge.c
> ===================================================================
> --- subversion/libsvn_wc/merge.c (revision 31014)
> +++ subversion/libsvn_wc/merge.c (working copy)
> @@ -656,7 +656,10 @@
> {
> svn_boolean_t same;
> SVN_ERR(svn_io_files_contents_same_p(&same, result_target,
> - merge_target, pool));
> + (is_binary ?
> + merge_target :
> + tmp_target),
> + pool));
>
> *merge_outcome = same ? svn_wc_merge_unchanged : svn_wc_merge_merged;
> }
I've looked at the context around this change, and I'm still not seeing
the connection between is_binary and broken symbolic links. That's more
a statement about my ignorance than about your patch -- after all, you
did fix the bug! -- but if you could go into more detail about exactly
how this fixes the problem, that would be great. (Putting the
explanation in a comment in the code might be best, in fact.)
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org