You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Arwin Arni <ar...@collab.net> on 2011/03/04 11:59:06 UTC
Re: svn commit: r1075802 - in /subversion/trunk/subversion: libsvn_client/merge.c
libsvn_wc/merge.c tests/cmdline/merge_tests.py
On Tuesday 01 March 2011 06:30 PM, stsp@apache.org wrote:
> Author: stsp
> Date: Tue Mar 1 13:00:47 2011
> New Revision: 1075802
>
> URL: http://svn.apache.org/viewvc?rev=1075802&view=rev
> Log:
> Fix issue #3686 - executable bit not set during merge.
>
> The cause was the special case in libsvn_client, which bypassed the use of
> the workqueue. This logic has now been moved into libsvn_wc.
>
> Additionally, this change allows the status of binary files (during a
> dry-run merge) to be reported correctly (previously, all binary files
> were reported as conflicted).
>
> * subversion/libsvn_client/merge.c
> (merge_file_changed): Remove binary-merge special case (now in libsvn_wc).
> Remove merge_required variable (resulting in indentation changes).
>
> * subversion/libsvn_wc/merge.c
> (merge_binary_file): Add dry_run parameter. Add the special case merging
> of binary files.
> (svn_wc__internal_merge): Remove dry_run check for binary files, and pass
> to merge_binary_file instead.
>
> * subversion/tests/cmdline/merge_tests.py
> (merge_change_to_file_with_executable): Remove @XFail decorator.
>
>
> Patch by: Daniel Becroft<dj...@gmail.com>
>
> Modified:
> subversion/trunk/subversion/libsvn_client/merge.c
> subversion/trunk/subversion/libsvn_wc/merge.c
> subversion/trunk/subversion/tests/cmdline/merge_tests.py
Post this fix, I noticed that **merge --dry-run** throws an interactive
conflict resolution dialog and create the merge-left and merge-right files.
This alters the Working Copy (creating filename.merge-left.rXYZ,
filename.merge-right.rABC) without marking a conflict (because it's a
dry-run). the test-case attached to the issue, only tests for the
executable flag. It doesn't run_and_verify_merge (which would make sure
the dry-run doesn't alter the Working Copy.)
Now this poses an issue when the conflicting file is marked conflicted
by some other operation.
This other operation will create two files (
filename._/*2*/_.merge-left.rXYZ, filename._/*2*/_.merge-right.rDEF ).
Currently, the resolve is reading from the highest numbered set of files
(I think).
As a side-effect of this experiment, I noticed this :
Removing these files causes the node to lose it's conflict state (why
should the status command rely on the presence/absence of conflict
resolution files to display the status).
Regards,
Arwin Arni
Re: svn commit: r1075802 - in /subversion/trunk/subversion:
libsvn_client/merge.c libsvn_wc/merge.c tests/cmdline/merge_tests.py
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Mar 04, 2011 at 07:05:33PM +0530, Arwin Arni wrote:
> On Friday 04 March 2011 05:15 PM, Philip Martin wrote:
> >Arwin Arni<ar...@collab.net> writes:
> >
> >>On Friday 04 March 2011 04:52 PM, Philip Martin wrote:
> >>>Arwin Arni<ar...@collab.net> writes:
> >>>
> >>>>Post this fix, I noticed that **merge --dry-run** throws an
> >>>>interactive conflict resolution dialog and create the merge-left and
> >>>>merge-right files.
> >>>
> >>>Does this apply to all files or just binary files?
> >>>
> >>This applies only to binary files. For text files (dry-run), no
> >>conflict resolution dialog, no left/right files..
> >
> >So dry-run on a text-file conflict gives notification but no resolution
> >and no wc mods?
> Absolutely right.
> >That sounds sensible, and I think binary files should
> >do the same.
> >
> Yes, I too think so. But post r1075802 this behaviour has changed
> and none of the regression tests catch this because they don't do a
> run_and_verify_merge with dry_run as TRUE in the "binary merge with
> conflict" scenario.
The test added in r1075802 doesn't involve a merge conflict.
So just making it use run_and_verify_merge isn't enough to make it
check for this problem.
I think we should add a new test for this.
Re: svn commit: r1075802 - in /subversion/trunk/subversion:
libsvn_client/merge.c libsvn_wc/merge.c tests/cmdline/merge_tests.py
Posted by Daniel Becroft <dj...@gmail.com>.
On Sat, Mar 5, 2011 at 8:27 AM, Daniel Becroft <dj...@gmail.com> wrote:
> On Fri, Mar 4, 2011 at 11:35 PM, Arwin Arni <ar...@collab.net> wrote:
>
>> On Friday 04 March 2011 05:15 PM, Philip Martin wrote:
>>
>>> Arwin Arni<ar...@collab.net> writes:
>>>
>>> On Friday 04 March 2011 04:52 PM, Philip Martin wrote:
>>>>
>>>>> Arwin Arni<ar...@collab.net> writes:
>>>>>
>>>>> Post this fix, I noticed that **merge --dry-run** throws an
>>>>>> interactive conflict resolution dialog and create the merge-left and
>>>>>> merge-right files.
>>>>>>
>>>>>
>>>>> Does this apply to all files or just binary files?
>>>>>
>>>>> This applies only to binary files. For text files (dry-run), no
>>>> conflict resolution dialog, no left/right files..
>>>>
>>>
>>> So dry-run on a text-file conflict gives notification but no resolution
>>> and no wc mods?
>>>
>> Absolutely right.
>>
>> That sounds sensible, and I think binary files should
>>> do the same.
>>>
>>> Yes, I too think so. But post r1075802 this behaviour has changed and
>> none of the regression tests catch this because they don't do a
>> run_and_verify_merge with dry_run as TRUE in the "binary merge with
>> conflict" scenario.
>>
>
> Yep, my fault. Sorry.
>
> I've got a patch to fix up the binary merge to not raise the conflict
> notification. I'll add a test case for this as well
Hey guys,
I'm still working through some issues with my test case (and patch) for
this. For some reason, my reproduction script now works with the below
patch, but the test case does not (it creates the conflict files for both
text and binary). I'm almost 100% certain that it's something to do with how
I've written the test case, so I'll keep looking at it.
Cheers,
Daniel B.
Index: subversion/libsvn_wc/merge.c
===================================================================
--- subversion/libsvn_wc/merge.c (revision 1078960)
+++ subversion/libsvn_wc/merge.c (working copy)
@@ -1144,6 +1144,11 @@ merge_binary_file(svn_skel_t **work_items,
*merge_outcome = svn_wc_merge_merged;
return SVN_NO_ERROR;
}
+ else if (dry_run)
+ {
+ *merge_outcome = svn_wc_merge_conflict;
+ return SVN_NO_ERROR;
+ }
/* Give the conflict resolution callback a chance to clean
up the conflict before we mark the file 'conflicted' */
Re: svn commit: r1075802 - in /subversion/trunk/subversion:
libsvn_client/merge.c libsvn_wc/merge.c tests/cmdline/merge_tests.py
Posted by Daniel Becroft <dj...@gmail.com>.
On Fri, Mar 4, 2011 at 11:35 PM, Arwin Arni <ar...@collab.net> wrote:
> On Friday 04 March 2011 05:15 PM, Philip Martin wrote:
>
>> Arwin Arni<ar...@collab.net> writes:
>>
>> On Friday 04 March 2011 04:52 PM, Philip Martin wrote:
>>>
>>>> Arwin Arni<ar...@collab.net> writes:
>>>>
>>>> Post this fix, I noticed that **merge --dry-run** throws an
>>>>> interactive conflict resolution dialog and create the merge-left and
>>>>> merge-right files.
>>>>>
>>>>
>>>> Does this apply to all files or just binary files?
>>>>
>>>> This applies only to binary files. For text files (dry-run), no
>>> conflict resolution dialog, no left/right files..
>>>
>>
>> So dry-run on a text-file conflict gives notification but no resolution
>> and no wc mods?
>>
> Absolutely right.
>
> That sounds sensible, and I think binary files should
>> do the same.
>>
>> Yes, I too think so. But post r1075802 this behaviour has changed and
> none of the regression tests catch this because they don't do a
> run_and_verify_merge with dry_run as TRUE in the "binary merge with
> conflict" scenario.
>
Yep, my fault. Sorry.
I've got a patch to fix up the binary merge to not raise the conflict
notification. I'll add a test case for this as well.
Cheers,
Daniel B.
Re: svn commit: r1075802 - in /subversion/trunk/subversion: libsvn_client/merge.c
libsvn_wc/merge.c tests/cmdline/merge_tests.py
Posted by Arwin Arni <ar...@collab.net>.
On Friday 04 March 2011 05:15 PM, Philip Martin wrote:
> Arwin Arni<ar...@collab.net> writes:
>
>> On Friday 04 March 2011 04:52 PM, Philip Martin wrote:
>>> Arwin Arni<ar...@collab.net> writes:
>>>
>>>> Post this fix, I noticed that **merge --dry-run** throws an
>>>> interactive conflict resolution dialog and create the merge-left and
>>>> merge-right files.
>>>
>>> Does this apply to all files or just binary files?
>>>
>> This applies only to binary files. For text files (dry-run), no
>> conflict resolution dialog, no left/right files..
>
> So dry-run on a text-file conflict gives notification but no resolution
> and no wc mods?
Absolutely right.
> That sounds sensible, and I think binary files should
> do the same.
>
Yes, I too think so. But post r1075802 this behaviour has changed and
none of the regression tests catch this because they don't do a
run_and_verify_merge with dry_run as TRUE in the "binary merge with
conflict" scenario.
Re: svn commit: r1075802 - in /subversion/trunk/subversion: libsvn_client/merge.c libsvn_wc/merge.c tests/cmdline/merge_tests.py
Posted by Philip Martin <ph...@wandisco.com>.
Arwin Arni <ar...@collab.net> writes:
> On Friday 04 March 2011 04:52 PM, Philip Martin wrote:
>> Arwin Arni<ar...@collab.net> writes:
>>
>>> Post this fix, I noticed that **merge --dry-run** throws an
>>> interactive conflict resolution dialog and create the merge-left and
>>> merge-right files.
>>
>> Does this apply to all files or just binary files?
>>
> This applies only to binary files. For text files (dry-run), no
> conflict resolution dialog, no left/right files..
So dry-run on a text-file conflict gives notification but no resolution
and no wc mods? That sounds sensible, and I think binary files should
do the same.
--
Philip
Re: svn commit: r1075802 - in /subversion/trunk/subversion: libsvn_client/merge.c
libsvn_wc/merge.c tests/cmdline/merge_tests.py
Posted by Arwin Arni <ar...@collab.net>.
On Friday 04 March 2011 04:52 PM, Philip Martin wrote:
> Arwin Arni<ar...@collab.net> writes:
>
>> Post this fix, I noticed that **merge --dry-run** throws an
>> interactive conflict resolution dialog and create the merge-left and
>> merge-right files.
>
> Does this apply to all files or just binary files?
>
This applies only to binary files. For text files (dry-run), no conflict
resolution dialog, no left/right files..
>> This alters the Working Copy (creating filename.merge-left.rXYZ,
>> filename.merge-right.rABC) without marking a conflict (because it's a
>> dry-run). the test-case attached to the issue, only tests for the
>> executable flag. It doesn't run_and_verify_merge (which would make
>> sure the dry-run doesn't alter the Working Copy.)
>>
>> Now this poses an issue when the conflicting file is marked conflicted
>> by some other operation.
>> This other operation will create two files (
>> filename._/*2*/_.merge-left.rXYZ, filename._/*2*/_.merge-right.rDEF ).
>>
>> Currently, the resolve is reading from the highest numbered set of
>> files (I think).
>>
>> As a side-effect of this experiment, I noticed this :
>>
>> Removing these files causes the node to lose it's conflict state (why
>> should the status command rely on the presence/absence of conflict
>> resolution files to display the status).
>
> It's long standing behaviour that removing the left/right files is one
> way to resolve a conflict.
>
Oh.. okay...
Re: svn commit: r1075802 - in /subversion/trunk/subversion:
libsvn_client/merge.c libsvn_wc/merge.c tests/cmdline/merge_tests.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Philip Martin wrote on Fri, Mar 04, 2011 at 11:22:37 +0000:
> It's long standing behaviour that removing the left/right files is one
> way to resolve a conflict.
/me learnt something new
Re: svn commit: r1075802 - in /subversion/trunk/subversion: libsvn_client/merge.c libsvn_wc/merge.c tests/cmdline/merge_tests.py
Posted by Philip Martin <ph...@wandisco.com>.
Arwin Arni <ar...@collab.net> writes:
> Post this fix, I noticed that **merge --dry-run** throws an
> interactive conflict resolution dialog and create the merge-left and
> merge-right files.
Does this apply to all files or just binary files?
> This alters the Working Copy (creating filename.merge-left.rXYZ,
> filename.merge-right.rABC) without marking a conflict (because it's a
> dry-run). the test-case attached to the issue, only tests for the
> executable flag. It doesn't run_and_verify_merge (which would make
> sure the dry-run doesn't alter the Working Copy.)
>
> Now this poses an issue when the conflicting file is marked conflicted
> by some other operation.
> This other operation will create two files (
> filename._/*2*/_.merge-left.rXYZ, filename._/*2*/_.merge-right.rDEF ).
>
> Currently, the resolve is reading from the highest numbered set of
> files (I think).
>
> As a side-effect of this experiment, I noticed this :
>
> Removing these files causes the node to lose it's conflict state (why
> should the status command rely on the presence/absence of conflict
> resolution files to display the status).
It's long standing behaviour that removing the left/right files is one
way to resolve a conflict.
--
Philip